[PATCH] emacs: Mention race condition safety in user visible documentation

[PATCH] emacs: Mention race condition safety in user visible documentation

[Michal Sojka]

After recent rework of a/A/x/X key bindings, the important paragraph in
documentation of `notmuch-show-archive-thread' stayed hidden from users,
because no key is bound to this function.

This patch copies the important paragraph to the documentation of
functions currently bound to keys.
---
 emacs/notmuch-show.el |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index aa9ccee..1916146 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1721,13 +1721,23 @@ buffer."
     (notmuch-show-tag-all (concat op "inbox"))))
 
 (defun notmuch-show-archive-thread-then-next ()
-  "Archive each message in thread, then show next thread from search."
+  "Archive each message in thread, then show next thread from search.
+
+Note: This command is safe from any race condition of new messages
+being delivered to the same thread. It does not archive the
+entire thread, but only the messages shown in the current
+buffer."
   (interactive)
   (notmuch-show-archive-thread)
   (notmuch-show-next-thread t))
 
 (defun notmuch-show-archive-thread-then-exit ()
-  "Archive each message in thread, then exit back to search results."
+  "Archive each message in thread, then exit back to search results.
+
+Note: This command is safe from any race condition of new messages
+being delivered to the same thread. It does not archive the
+entire thread, but only the messages shown in the current
+buffer."
   (interactive)
   (notmuch-show-archive-thread)
   (notmuch-show-next-thread))
-- 
1.7.7.3

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

On Feb 19, 2012 12:44 AM, "Michal Sojka" <sojkam1@fel.cvut.cz> wrote:
>
> After recent rework of a/A/x/X key bindings, the important paragraph in
> documentation of `notmuch-show-archive-thread' stayed hidden from users,
> because no key is bound to this function.
>
> This patch copies the important paragraph to the documentation of
> functions currently bound to keys.

Hi Michal, this was discussed earlier, and Dmitry convinced me that the
code does exactly what the user expects it to do (archives only the
messages in the buffer) so it would be unnecessary and confusing to have
this documentation. It could be added as a comment for developers I guess.

BR,
Jani.

> ---
>  emacs/notmuch-show.el |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index aa9ccee..1916146 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1721,13 +1721,23 @@ buffer."
>     (notmuch-show-tag-all (concat op "inbox"))))
>
>  (defun notmuch-show-archive-thread-then-next ()
> -  "Archive each message in thread, then show next thread from search."
> +  "Archive each message in thread, then show next thread from search.
> +
> +Note: This command is safe from any race condition of new messages
> +being delivered to the same thread. It does not archive the
> +entire thread, but only the messages shown in the current
> +buffer."
>   (interactive)
>   (notmuch-show-archive-thread)
>   (notmuch-show-next-thread t))
>
>  (defun notmuch-show-archive-thread-then-exit ()
> -  "Archive each message in thread, then exit back to search results."
> +  "Archive each message in thread, then exit back to search results.
> +
> +Note: This command is safe from any race condition of new messages
> +being delivered to the same thread. It does not archive the
> +entire thread, but only the messages shown in the current
> +buffer."
>   (interactive)
>   (notmuch-show-archive-thread)
>   (notmuch-show-next-thread))
> --
> 1.7.7.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: text/html, Size: 2848 bytes --]

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Michal Sojka]

On Sun, 19 Feb 2012, Jani Nikula wrote:
> On Feb 19, 2012 12:44 AM, "Michal Sojka" <sojkam1@fel.cvut.cz> wrote:
> >
> > After recent rework of a/A/x/X key bindings, the important paragraph in
> > documentation of `notmuch-show-archive-thread' stayed hidden from users,
> > because no key is bound to this function.
> >
> > This patch copies the important paragraph to the documentation of
> > functions currently bound to keys.
> 
> Hi Michal, this was discussed earlier, and Dmitry convinced me that the
> code does exactly what the user expects it to do (archives only the
> messages in the buffer) so it would be unnecessary and confusing to have
> this documentation. It could be added as a comment for developers I
> guess.

I'm not sure. As a developer, I'm used to think about race conditions
and the mentioned documentation sounds very racy to me. I guess a lot of
other notmuch users are developers who may think the same.

> >  (defun notmuch-show-archive-thread-then-next ()
> > -  "Archive each message in thread, then show next thread from search."

What about changing it to "Archive each message in the current buffer, ..."?

-Michal

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Mark Walters]

On Sun, 19 Feb 2012 08:33:00 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> On Sun, 19 Feb 2012, Jani Nikula wrote:
> > On Feb 19, 2012 12:44 AM, "Michal Sojka" <sojkam1@fel.cvut.cz> wrote:
> > >
> > > After recent rework of a/A/x/X key bindings, the important paragraph in
> > > documentation of `notmuch-show-archive-thread' stayed hidden from users,
> > > because no key is bound to this function.
> > >
> > > This patch copies the important paragraph to the documentation of
> > > functions currently bound to keys.
> > 
> > Hi Michal, this was discussed earlier, and Dmitry convinced me that the
> > code does exactly what the user expects it to do (archives only the
> > messages in the buffer) so it would be unnecessary and confusing to have
> > this documentation. It could be added as a comment for developers I
> > guess.
> 
> I'm not sure. As a developer, I'm used to think about race conditions
> and the mentioned documentation sounds very racy to me. I guess a lot of
> other notmuch users are developers who may think the same.
> 
> > >  (defun notmuch-show-archive-thread-then-next ()
> > > -  "Archive each message in thread, then show next thread from search."
> 
> What about changing it to "Archive each message in the current buffer, ..."?

I like this suggested wording (or perhaps "Archive all messages in the
current buffer, ..."). I do not think `Archive each message in thread' is
completely clear (is the thread the thread as it is now or as it was
then).

Best wishes

Mark

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Michal Sojka]

On Tue, 21 Feb 2012, Mark Walters wrote:
> On Sun, 19 Feb 2012 08:33:00 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > On Sun, 19 Feb 2012, Jani Nikula wrote:
> > > On Feb 19, 2012 12:44 AM, "Michal Sojka" <sojkam1@fel.cvut.cz> wrote:
> > > >
> > > > After recent rework of a/A/x/X key bindings, the important paragraph in
> > > > documentation of `notmuch-show-archive-thread' stayed hidden from users,
> > > > because no key is bound to this function.
> > > >
> > > > This patch copies the important paragraph to the documentation of
> > > > functions currently bound to keys.
> > > 
> > > Hi Michal, this was discussed earlier, and Dmitry convinced me that the
> > > code does exactly what the user expects it to do (archives only the
> > > messages in the buffer) so it would be unnecessary and confusing to have
> > > this documentation. It could be added as a comment for developers I
> > > guess.
> > 
> > I'm not sure. As a developer, I'm used to think about race conditions
> > and the mentioned documentation sounds very racy to me. I guess a lot of
> > other notmuch users are developers who may think the same.
> > 
> > > >  (defun notmuch-show-archive-thread-then-next ()
> > > > -  "Archive each message in thread, then show next thread from search."
> > 
> > What about changing it to "Archive each message in the current buffer, ..."?
> 
> I like this suggested wording (or perhaps "Archive all messages in the
> current buffer, ..."). I do not think `Archive each message in thread' is
> completely clear (is the thread the thread as it is now or as it was
> then).

OK, at least one supporter here :) The updated patch follows:

-- >8 --
Subject: [PATCH] emacs: Clarify description of thread manipulating functions

It is not clear whether the term "thread" refers to the thread in the
database or to the thread currently shown in a buffer. Those two
meanings may refer to different sets of messages, e.g. when a new email
is added to the database while the buffer shows the state before the new
email arrived.

This patch replaces the term thread with the term current buffer, which
is hopefully less ambiguous.
---
 emacs/notmuch-show.el |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index aa9ccee..3515d03 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1628,7 +1628,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
     (apply 'notmuch-show-tag-message tag-changes)))
 
 (defun notmuch-show-tag-all (&rest tag-changes)
-  "Change tags for all messages in the current thread.
+  "Change tags for all messages in the current buffer.
 
 TAG-CHANGES is a list of tag operations for `notmuch-tag'."
   (interactive (notmuch-read-tag-changes nil notmuch-show-thread-id))
@@ -1721,13 +1721,13 @@ buffer."
     (notmuch-show-tag-all (concat op "inbox"))))
 
 (defun notmuch-show-archive-thread-then-next ()
-  "Archive each message in thread, then show next thread from search."
+  "Archive all messages in the current buffer, then show next thread from search."
   (interactive)
   (notmuch-show-archive-thread)
   (notmuch-show-next-thread t))
 
 (defun notmuch-show-archive-thread-then-exit ()
-  "Archive each message in thread, then exit back to search results."
+  "Archive all messages in the current buffer, then exit back to search results."
   (interactive)
   (notmuch-show-archive-thread)
   (notmuch-show-next-thread))
-- 
1.7.7.3

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Pieter Praet]

On Tue, 21 Feb 2012 10:15:08 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> [...]
> OK, at least one supporter here :) The updated patch follows:
> 
> -- >8 --
> Subject: [PATCH] emacs: Clarify description of thread manipulating functions
> 
> It is not clear whether the term "thread" refers to the thread in the
> database or to the thread currently shown in a buffer. Those two
> meanings may refer to different sets of messages, e.g. when a new email
> is added to the database while the buffer shows the state before the new
> email arrived.
> 
> This patch replaces the term thread with the term current buffer, which
> is hopefully less ambiguous.
> ---

+1


Also, the inverse is true for the comment about `notmuch-search-tag-all'
in `notmuch-search-mode's docstring.  Patch follows.


Peace

-- 
Pieter

[PATCH] emacs: correct `notmuch-search-mode's docstring wrt `notmuch-search-tag-all'

[Pieter Praet]

* emacs/notmuch.el (notmuch-search-mode):
  `notmuch-search-tag-all' currently uses the current query string
  instead of `notmuch-search-find-thread-id-region-search', which
  might cause a race condition.
---
 emacs/notmuch.el |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f851c6f..0a9fffd 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -435,8 +435,9 @@ (defun notmuch-search-mode ()
 Pressing \\[notmuch-search-show-thread] on any line displays that thread. The '\\[notmuch-search-add-tag]' and '\\[notmuch-search-remove-tag]'
 keys can be used to add or remove tags from a thread. The '\\[notmuch-search-archive-thread]' key
 is a convenience for archiving a thread (removing the \"inbox\"
-tag). The '\\[notmuch-search-tag-all]' key can be used to add or remove a tag from all
-threads in the current buffer.
+tag). The '\\[notmuch-search-tag-all]' key can be used to add and/or remove tags from all
+messages (as opposed to threads) that match the current query.  Use with caution, as this
+will also tag matching messages that arrived *after* constructing the buffer.
 
 Other useful commands are '\\[notmuch-search-filter]' for filtering the current search
 based on an additional query string, '\\[notmuch-search-filter-by-tag]' for filtering to include
-- 
1.7.8.1

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Pieter Praet]

On Sat, 18 Feb 2012 23:44:18 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> After recent rework of a/A/x/X key bindings, the important paragraph in
> documentation of `notmuch-show-archive-thread' stayed hidden from users,
> because no key is bound to this function.
> 
> This patch copies the important paragraph to the documentation of
> functions currently bound to keys.
> ---

FYI, I've removed the 'needs-review' flag from this patch, as
it's obsoleted by the one @ id:"87linwsg2r.fsf@steelpick.2x.cz".

Keep in mind that one needs to run `git-am' with the '--scissors'
option when applying the latter.


Peace

-- 
Pieter

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[David Bremner]

On Tue, 21 Feb 2012 10:15:08 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> -- >8 --
> Subject: [PATCH] emacs: Clarify description of thread manipulating functions
> 
> It is not clear whether the term "thread" refers to the thread in the
> database or to the thread currently shown in a buffer. Those two
> meanings may refer to different sets of messages, e.g. when a new email
> is added to the database while the buffer shows the state before the new
> email arrived.

(will be) pushed.

For future reference, it's slightly inconvenient for me to use "git am
-c". Not a big deal, but a sequence of those would make me cry...

d

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[Pieter Praet]

On Wed, 29 Feb 2012 22:49:46 -0400, David Bremner <david@tethera.net> wrote:
> On Tue, 21 Feb 2012 10:15:08 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > -- >8 --
> > Subject: [PATCH] emacs: Clarify description of thread manipulating functions
> > 
> > It is not clear whether the term "thread" refers to the thread in the
> > database or to the thread currently shown in a buffer. Those two
> > meanings may refer to different sets of messages, e.g. when a new email
> > is added to the database while the buffer shows the state before the new
> > email arrived.
> 
> (will be) pushed.
> 
> For future reference, it's slightly inconvenient for me to use "git am
> -c". Not a big deal, but a sequence of those would make me cry...
>

Something like this might help save you some time:
(assuming you'd rather not leave Emacs to apply patches)

  #+begin_src emacs-lisp
    (defvar praet/projects
      '(("notmuch@notmuchmail.org" . "~/src/dev/notmuch/")
        ("foo@bar.org" . "~/src/dev/foobar/")))

    (defun praet/notmuch-patch-apply (&optional all-open)
      "Apply a patch from `notmuch-show-mode' using git-am(1).
    If ALL-OPEN is non-nil, (try to) apply all open messages."
      (interactive "P")
      (let* ((msg (notmuch-show-get-message-id t))
             (rcpts (concat (notmuch-show-get-to)
                            (notmuch-show-get-cc)))
             (projs (loop for project in praet/projects
                          if (string-match (car project) rcpts)
                          collect (cdr project)))
             (proj (if (> (length projs) 1)
                       (ido-completing-read "Apply in: " projs)
                     (car projs)))
             (cmd "git am --abort ; git am")
             (optargs '("--signoff" "--3way" "--scissors"))
             (args (read-string "Args: " nil '(optargs . 0))))
        (message "Applying in '%s': '%s' (using '%s %s')"
                 proj msg cmd args)
        (notmuch-show-pipe-message all-open
         (concat "(cd " proj " && (" cmd " " args ")" ")"))))

    (define-key notmuch-show-mode-map (kbd "C-c p a")
      'praet/notmuch-patch-apply)
  #+end_src


> d
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch


Peace

-- 
Pieter

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[David Bremner]

On Thu, 01 Mar 2012 18:13:27 +0100, Pieter Praet <pieter@praet.org> wrote:
> Something like this might help save you some time:
> (assuming you'd rather not leave Emacs to apply patches)

Thanks, I already have a one line git alias ;).

d

Re: [PATCH] emacs: Mention race condition safety in user visible documentation

[David Bremner]

On Thu, 01 Mar 2012 14:21:26 -0400, David Bremner <david@tethera.net> wrote:
> On Thu, 01 Mar 2012 18:13:27 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Something like this might help save you some time:
> > (assuming you'd rather not leave Emacs to apply patches)
> 
> Thanks, I already have a one line git alias ;).
> 

err, in the interests of completeness

[alias]
	nmam = "!f() { notmuch show --format=raw $1 | git am -; }; f"

d

Re: [PATCH] emacs: correct `notmuch-search-mode's docstring wrt `notmuch-search-tag-all'

[Mark Walters]

On Wed, 22 Feb 2012, Pieter Praet <pieter@praet.org> wrote:
> * emacs/notmuch.el (notmuch-search-mode):
>   `notmuch-search-tag-all' currently uses the current query string
>   instead of `notmuch-search-find-thread-id-region-search', which
>   might cause a race condition.

This looks correct and is a significant improvement in wording as it
also correct the fact that * in search view (tag-all) operates on
matching messages not matching threads.

+1

Best wishes

Mark

> ---
>  emacs/notmuch.el |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index f851c6f..0a9fffd 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -435,8 +435,9 @@ (defun notmuch-search-mode ()
>  Pressing \\[notmuch-search-show-thread] on any line displays that thread. The '\\[notmuch-search-add-tag]' and '\\[notmuch-search-remove-tag]'
>  keys can be used to add or remove tags from a thread. The '\\[notmuch-search-archive-thread]' key
>  is a convenience for archiving a thread (removing the \"inbox\"
> -tag). The '\\[notmuch-search-tag-all]' key can be used to add or remove a tag from all
> -threads in the current buffer.
> +tag). The '\\[notmuch-search-tag-all]' key can be used to add and/or remove tags from all
> +messages (as opposed to threads) that match the current query.  Use with caution, as this
> +will also tag matching messages that arrived *after* constructing the buffer.
>  
>  Other useful commands are '\\[notmuch-search-filter]' for filtering the current search
>  based on an additional query string, '\\[notmuch-search-filter-by-tag]' for filtering to include
> -- 
> 1.7.8.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: correct `notmuch-search-mode's docstring wrt `notmuch-search-tag-all'

[David Bremner]

Pieter Praet <pieter@praet.org> writes:

> * emacs/notmuch.el (notmuch-search-mode):
>   `notmuch-search-tag-all' currently uses the current query string
>   instead of `notmuch-search-find-thread-id-region-search', which
>   might cause a race condition.

pushed, finally.

d