[PATCH v2] nmbug: write tags out to a temporary file, not 'nmbug.index'

[PATCH v2] nmbug: write tags out to a temporary file, not 'nmbug.index'

[Sean Whitton]

If more than nmbug process is running at once, then each will try to
read and write the same file.  The particular failure I've seen is
that the process which finishes first deletes nmbug.index, and then
the other process dies with a FileNotFoundError.  So use a distinct
temporary file per process.
---
 devel/nmbug/nmbug | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Here is a second attempt, though I'm afraid I have little idea whether it is
idiomatic Python.

diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
index 043c1863..396098d0 100755
--- a/devel/nmbug/nmbug
+++ b/devel/nmbug/nmbug
@@ -586,37 +586,38 @@ def get_status():
         'deleted': {},
         'missing': {},
         }
-    index = _index_tags()
-    maybe_deleted = _diff_index(index=index, filter='D')
-    for id, tags in maybe_deleted.items():
-        (_, stdout, stderr) = _spawn(
-            args=['notmuch', 'search', '--output=files', 'id:{0}'.format(id)],
-            stdout=_subprocess.PIPE,
-            wait=True)
-        if stdout:
-            status['deleted'][id] = tags
-        else:
-            status['missing'][id] = tags
-    status['added'] = _diff_index(index=index, filter='A')
-    _os.remove(index)
-    return status
-
-
-def _index_tags():
-    "Write notmuch tags to the nmbug.index."
-    path = _os.path.join(NMBGIT, 'nmbug.index')
+    (_, index) = _tempfile.mkstemp()
+    try:
+        _index_tags(index)
+        maybe_deleted = _diff_index(index=index, filter='D')
+        for id, tags in maybe_deleted.items():
+            (_, stdout, stderr) = _spawn(
+                args=['notmuch', 'search', '--output=files', 'id:{0}'.format(id)],
+                stdout=_subprocess.PIPE,
+                wait=True)
+            if stdout:
+                status['deleted'][id] = tags
+            else:
+                status['missing'][id] = tags
+                status['added'] = _diff_index(index=index, filter='A')
+                return status
+    finally:
+        _os.remove(index)
+
+def _index_tags(index):
+    "Write notmuch tags to a file."
     query = ' '.join('tag:"{tag}"'.format(tag=tag) for tag in get_tags())
     prefix = '+{0}'.format(_ENCODED_TAG_PREFIX)
     _git(
         args=['read-tree', '--empty'],
-        additional_env={'GIT_INDEX_FILE': path}, wait=True)
+        additional_env={'GIT_INDEX_FILE': index}, wait=True)
     with _spawn(
             args=['notmuch', 'dump', '--format=batch-tag', '--', query],
             stdout=_subprocess.PIPE) as notmuch:
         with _git(
                 args=['update-index', '--index-info'],
                 stdin=_subprocess.PIPE,
-                additional_env={'GIT_INDEX_FILE': path}) as git:
+                additional_env={'GIT_INDEX_FILE': index}) as git:
             for line in notmuch.stdout:
                 if line.strip().startswith('#'):
                     continue
@@ -629,7 +630,6 @@ def _index_tags():
                 for line in _index_tags_for_message(
                         id=id, status='A', tags=tags):
                     git.stdin.write(line)
-    return path
 
 
 def _index_tags_for_message(id, status, tags):
-- 
2.30.2

Re: [PATCH v2] nmbug: write tags out to a temporary file, not 'nmbug.index'

[Kyle Meyer]

[ drive-by comment based on a past mistake :/ ]

Sean Whitton writes:

> -def _index_tags():
> -    "Write notmuch tags to the nmbug.index."
> -    path = _os.path.join(NMBGIT, 'nmbug.index')
> +    (_, index) = _tempfile.mkstemp()
[...]
>      _git(
>          args=['read-tree', '--empty'],
> -        additional_env={'GIT_INDEX_FILE': path}, wait=True)
> +        additional_env={'GIT_INDEX_FILE': index}, wait=True)

It's better to put the temporary index in $GIT_DIR due to this bit noted
in git-read-tree(1):

      The file must allow to be rename(2)ed into from a temporary file
      that is created next to the usual index file; typically this means
      it needs to be on the same filesystem as the index file itself, and
      you need write permission to the directories the index file and
      index output file are located in.

So, assuming NMBGIT matches $GIT_DIR, perhaps change the above mkstemp
call to something like

  _tempfile.mkstemp(dir=NMBGIT, prefix="nmbug", suffix=".index")

Re: [PATCH v2] nmbug: write tags out to a temporary file, not 'nmbug.index'

[Sean Whitton]

Hello,

On Sun 13 Feb 2022 at 09:54am -07, Sean Whitton wrote:

> If more than nmbug process is running at once, then each will try to
> read and write the same file.  The particular failure I've seen is
> that the process which finishes first deletes nmbug.index, and then
> the other process dies with a FileNotFoundError.  So use a distinct
> temporary file per process.
> ---
>  devel/nmbug/nmbug | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
>
> Here is a second attempt, though I'm afraid I have little idea whether it is
> idiomatic Python.

It would seem this causes 'nmbug status' to output just one result.

I'll leave the fix to someone with more Python experience.  Sorry for
the improperly tested v2 patch.

-- 
Sean Whitton

Re: [PATCH v2] nmbug: write tags out to a temporary file, not 'nmbug.index'

[Tomi Ollila]

On Sun, Feb 13 2022, Sean Whitton wrote:

> Hello,
>
> On Sun 13 Feb 2022 at 09:54am -07, Sean Whitton wrote:
>
>> If more than nmbug process is running at once, then each will try to
>> read and write the same file.  The particular failure I've seen is
>> that the process which finishes first deletes nmbug.index, and then
>> the other process dies with a FileNotFoundError.  So use a distinct
>> temporary file per process.
>> ---
>>  devel/nmbug/nmbug | 44 ++++++++++++++++++++++----------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> Here is a second attempt, though I'm afraid I have little idea whether it is
>> idiomatic Python.
>
> It would seem this causes 'nmbug status' to output just one result.
>
> I'll leave the fix to someone with more Python experience.  Sorry for
> the improperly tested v2 patch.

One option would be using the first patch, but instead of mkstemp(),
NamedTemporaryFile(dir=NMBGIT, prefix="nmbug.index") to be used instead
(and then path.name and index.name in place of path and index...)

Tomi

>
> -- 
> Sean Whitton