[PATCH] New function notmuch-search-operate-all: operate on all messages in the current query.

[PATCH] New function notmuch-search-operate-all: operate on all messages in the current query.

[Jed Brown]

It is often convenient to change tags on several messages at once.  This
function applies any number of tag whitespace-delimited tag
modifications to all messages matching the current query.

I have bound this to `*'.

Signed-off-by: Jed Brown <jed@59A2.org>
---
 notmuch.el |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index a547415..b848e9a 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -764,6 +764,7 @@ thread from that buffer can be show when done with this one)."
     (define-key map (kbd "RET") 'notmuch-search-show-thread)
     (define-key map "+" 'notmuch-search-add-tag)
     (define-key map "-" 'notmuch-search-remove-tag)
+    (define-key map "*" 'notmuch-search-operate-all)
     (define-key map "<" 'beginning-of-buffer)
     (define-key map ">" 'notmuch-search-goto-last-thread)
     (define-key map "=" 'notmuch-search-refresh-view)
@@ -940,6 +941,29 @@ This function advances point to the next line when finished."
   (notmuch-search-remove-tag "inbox")
   (forward-line))
 
+(defun notmuch-search-operate-all (action)
+  "Operate on all messages matching the current query.  Any
+number of whitespace separated actions can be given.  Each action
+must have one of the two forms
+
+  +tagname              Add the tag `tagname'
+  -tagname              Remove the tag `tagname'
+
+Each character of the tag name may consist of alphanumeric
+characters as well as `_.+-'.
+"
+  (interactive "sOperation (+add -drop): notmuch tag ")
+  (let ((action-split (split-string action " +")))
+    ;; Perform some validation
+    (let ((words action-split))
+      (when (null words) (error "No operation given"))
+      (while words
+	(unless (string-match-p "^[\+\-][_\+\-\\w]+$" (car words))
+	  (error "Action must be of the form `+thistag -that_tag'"))
+	(setq words (cdr words))))
+    (apply 'notmuch-call-notmuch-process "tag"
+	   (append action-split (list notmuch-search-query-string) nil))))
+
 (defun notmuch-search (query &optional oldest-first)
   "Run \"notmuch search\" with the given query string and display results."
   (interactive "sNotmuch search: ")
-- 
1.6.5.3

Re: [PATCH] New function notmuch-search-operate-all: operate on all messages in the current query.

[Carl Worth]

On Sun, 22 Nov 2009 21:12:16 +0100, Jed Brown <jed@59A2.org> wrote:
> It is often convenient to change tags on several messages at once.  This
> function applies any number of tag whitespace-delimited tag
> modifications to all messages matching the current query.
> 
> I have bound this to `*'.

Very nice, Jed!

I've been wanting this feature for a while, and I think you just
improved on my idea in at least two ways.

First I was imagining that the '*' would be a prefix command, but I like
it just prompting for the '+' or '-' as part of the argument. That's no
more typing and allows for doing multiple tags at once.

Second, I like that you just used the search string again, (as opposed
to just walking through the buffer looking at thread IDs). That seems
elegant.

On second thought, however, using the search string is problematic for
at least two reasons:

First, this creates a race condition in that the user will rightly
expect to only be adding/removing tags from the visible results. But if
new mail has been incorporated between creating the current view and
running '*' then threads could be tagged that were never seen and that
could be problematic.

Second, since we're in the search view which shows threads, we should
really be operating on threads. But this tag command won't work like the
'+' and '-' commands in this buffer. Those commands will add/remove a
tag to/from every message in the thread[*]. The new '*' command, however
will only be changing tags on messages that match the query.

So I think we should fix both of these issues by looping over each line
of the buffer and calling (notmuch-search-add-tag) or
(notmuch-search-remove-tag).

What do you think?

-Carl

[*] These existing '+' and '-' operations (as well as 'a') that act on
the entire thread also have a race condition. They are potentially
modifying more messages than the original search matched. This is often
harmless, (you aren't even *seeing* the messages so how can you complain
if more get modified than were there when you did the search.). But it
can actually be fatal:

Imagine I sent a message to the list, and then in the search view I see
that message appear and it has [1/1] indicating it's the only message in
the thread. I might archive this "knowing" I've read the message
already, but this could actually archive a reply as well that arrived
between when I did the search and when I archived.

So that's a bug that we really should fix. [And noted that archiving an
entire thread from the notmuch-show-mode does not have this bug since it
acts only on the explicit messages that are presented.] One option to
fix this would be for "notmuch search" to list all the message IDs that
matched in place of the thread ID (for notmuch-search-mode to hide away
like it does with the thread ID currently). But that seems like it might
get problematic with some hugely long threads.

Anyway, I'm interested in ideas for what we could do here.

Oh, here's one: We could add something like "notmuch tag --expecting=
1/23 <search-terms>" that would not do the tag unless the search string
matched 1 message out of 23 total messages in the thread. Then we could
give a warning to the user. That works for the single-thread case, but
the warning would be harder for the user to deal with in the '*'
case. Or maybe not---the emacs function could just stop on the first
line with the error and then the user would see what's going on.

And we could take a prefix argument to ignore such errors.

Any other thoughts on this?

Re: [PATCH] New function notmuch-search-operate-all: operate on all messages in the current query.

[Jed Brown]

On Mon, 23 Nov 2009 05:14:25 +0100, Carl Worth <cworth@cworth.org> wrote:
> Second, I like that you just used the search string again, (as opposed
> to just walking through the buffer looking at thread IDs). That seems
> elegant.

It was *easy*.

> First, this creates a race condition in that the user will rightly
> expect to only be adding/removing tags from the visible results. But if
> new mail has been incorporated between creating the current view and
> running '*' then threads could be tagged that were never seen and that
> could be problematic.

Agreed and I had also thought of the case you described.  Note that
Gmail does not solve the race condition.  When in summary mode:

* Marking a thread as read applies to all messages in the thread.  The
  thread contents are being updated concurrently so you may mark threads
  you have already seen.

* Same story with archiving (aka. remove inbox).

* Starring a thread applies to the last message in the thread, this
  could be a newly arrived message that is not currently displayed.

I think that handling a concurrent changes to the match results is a
somewhat hard problem.  You have to store the message ids of everything
in the current query if you want to avoid the race.

> Second, since we're in the search view which shows threads, we should
> really be operating on threads. But this tag command won't work like the
> '+' and '-' commands in this buffer. Those commands will add/remove a
> tag to/from every message in the thread[*]. The new '*' command, however
> will only be changing tags on messages that match the query.

I'm not convinced that we want to operate on the entire thread.
Applying the tag only to the matched messages is enough to find the
thread, and it may really be desirable to only have it applied to
certain messages in the thread.  For example, I might have constructed
an elaborate query to find five messages, including a couple that are
burried in 100-message threads in which case I would definitely not want
to tag entire threads.

> So I think we should fix both of these issues by looping over each line
> of the buffer and calling (notmuch-search-add-tag) or
> (notmuch-search-remove-tag).

Presumably you don't mean this literally because that would be horribly
slow.  What you want is to gather up all the message ids in all the
threads with at least one message matching the current query (and
currently displayed).  I think this is only possible if notmuch-search
holds message IDs for everything in the matched threads.  If we are
happy to just tag the matched messages instead of the entire thread, we
would only store the message ids for the matched messages.

> Oh, here's one: We could add something like "notmuch tag --expecting=
> 1/23 <search-terms>" that would not do the tag unless the search string
> matched 1 message out of 23 total messages in the thread. Then we could
> give a warning to the user.

I like this, but it still presents a minor race condition (suppose
another client swaps the unread tag on two messages in the same thread).
The only way to guarantee that you are operating on the displayed
messages is to store their message ids.

> That works for the single-thread case, but the warning would be harder
> for the user to deal with in the '*' case. Or maybe not---the emacs
> function could just stop on the first line with the error and then the
> user would see what's going on.

If you can implement notmuch tag --expecting with similar efficiency to
the present, then I would vote for notifying the user and asking for
confirmation if the number of matches has changed.  This would be
significantly safer than what Gmail does which ought to be sufficient
for now given the age of notmuch.


Jed

Re: [PATCH] New function notmuch-search-operate-all: operate on all messages in the current query.

[Carl Worth]

On Mon, 23 Nov 2009 14:07:20 +0100, Jed Brown <jed@59A2.org> wrote:
> > Second, since we're in the search view which shows threads, we should
> > really be operating on threads. But this tag command won't work like the
> > '+' and '-' commands in this buffer. Those commands will add/remove a
> > tag to/from every message in the thread[*]. The new '*' command, however
> > will only be changing tags on messages that match the query.
> 
> I'm not convinced that we want to operate on the entire thread.
> Applying the tag only to the matched messages is enough to find the
> thread, and it may really be desirable to only have it applied to
> certain messages in the thread.  For example, I might have constructed
> an elaborate query to find five messages, including a couple that are
> burried in 100-message threads in which case I would definitely not want
> to tag entire threads.

That's a legitimate point.

But my point is that whatever behavior we choose here, I want the
commands that operate on a single thread (+, -, a) to operate exactly
the same as the command that operates on all threads (*). Having these
behave subtly different, (as in the current patch) is going to lead to
confusion on the part of the user.

So, tagging only matching messages could make a lot of sense. If so,
let's make the operations on single threads work the same.

The race-condition issues apply to both operations, so I won't make the
current patch block on resolving those.

-Carl

[PATCH 1/2] New function notmuch-search-operate-all: operate on all messages in the current query.

[Jed Brown]

It is often convenient to change tags on several messages at once.  This
function applies any number of tag whitespace-delimited tag
modifications to all messages matching the current query.

I have bound this to `*'.

Signed-off-by: Jed Brown <jed@59A2.org>
---
 notmuch.el |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index d7c973c..6adac9e 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -801,6 +801,7 @@ thread from that buffer can be show when done with this one)."
     (define-key map [mouse-1] 'notmuch-search-show-thread)
     (define-key map "+" 'notmuch-search-add-tag)
     (define-key map "-" 'notmuch-search-remove-tag)
+    (define-key map "*" 'notmuch-search-operate-all)
     (define-key map "<" 'beginning-of-buffer)
     (define-key map ">" 'notmuch-search-goto-last-thread)
     (define-key map "=" 'notmuch-search-refresh-view)
@@ -1001,6 +1002,29 @@ This function advances point to the next line when finished."
 		  (set 'more nil))))))
       (delete-process proc))))
 
+(defun notmuch-search-operate-all (action)
+  "Operate on all messages matching the current query.  Any
+number of whitespace separated actions can be given.  Each action
+must have one of the two forms
+
+  +tagname              Add the tag `tagname'
+  -tagname              Remove the tag `tagname'
+
+Each character of the tag name may consist of alphanumeric
+characters as well as `_.+-'.
+"
+  (interactive "sOperation (+add -drop): notmuch tag ")
+  (let ((action-split (split-string action " +")))
+    ;; Perform some validation
+    (let ((words action-split))
+      (when (null words) (error "No operation given"))
+      (while words
+	(unless (string-match-p "^[\+\-][_\+\-\\w]+$" (car words))
+	  (error "Action must be of the form `+thistag -that_tag'"))
+	(setq words (cdr words))))
+    (apply 'notmuch-call-notmuch-process "tag"
+	   (append action-split (list notmuch-search-query-string) nil))))
+
 (defun notmuch-search (query &optional oldest-first)
   "Run \"notmuch search\" with the given query string and display results."
   (interactive "sNotmuch search: ")
-- 
1.6.5.3

[PATCH 2/2] notmuch-search-add/remove-tag: restrict to messages in current query

[Jed Brown]

Rather than tagging the everything in the thread.  This is arguably more
desirable behavior and is consistent with clearly desirably behavior of
notmuch-search-operate-all.

Note that this change applies indirectly to
notmuch-search-archive-thread (which is actually equivalent behavior
since this function is primarily used when browsing an inbox).

Signed-off-by: Jed Brown <jed@59A2.org>
---
 notmuch.el |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 6adac9e..e9786c0 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -935,15 +935,19 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	(split-string (buffer-substring beg end))))))
 
 (defun notmuch-search-add-tag (tag)
+  "Add a tag to messages in the current thread matching the
+active query."
   (interactive
    (list (notmuch-select-tag-with-completion "Tag to add: ")))
-  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id))
+  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id) " and " notmuch-search-query-string)
   (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<))))
 
 (defun notmuch-search-remove-tag (tag)
+  "Remove a tag from messages in the current thread matching the
+active query."
   (interactive
    (list (notmuch-select-tag-with-completion "Tag to remove: " (notmuch-search-find-thread-id))))
-  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id))
+  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id) " and " notmuch-search-query-string)
   (notmuch-search-set-tags (delete tag (notmuch-search-get-tags))))
 
 (defun notmuch-search-archive-thread ()
-- 
1.6.5.3

Re: [PATCH 1/2] New function notmuch-search-operate-all: operate on all messages in the current query.

[Carl Worth]

On Thu, 26 Nov 2009 22:36:49 +0100, Jed Brown <jed@59A2.org> wrote:
> It is often convenient to change tags on several messages at once.  This
> function applies any number of tag whitespace-delimited tag
> modifications to all messages matching the current query.

Thanks Jed,

I've applied both this and the following patch, (giving the consistent
treatment to '+, '-', and 'a'). One or two minor comments:

> +  +tagname              Add the tag `tagname'
> +  -tagname              Remove the tag `tagname'
> +
> +Each character of the tag name may consist of alphanumeric
> +characters as well as `_.+-'.

Since this operates via a single call to "notmuch tag" you might mention
here that all tag removals occur before any tag additions.

> +	(unless (string-match-p "^[\+\-][_\+\-\\w]+$" (car words))
> +	  (error "Action must be of the form `+thistag -that_tag'"))

The error message has inconsistent "thistag" and "that_tag".

But niether nit was worth blocking the commit on.

Thanks again,

-Carl

Re: [PATCH 1/2] New function notmuch-search-operate-all: operate on all messages in the current query.

[Jed Brown]

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

On Fri, 27 Nov 2009 06:02:45 -0800, Carl Worth <cworth@cworth.org> wrote:
> Since this operates via a single call to "notmuch tag" you might mention
> here that all tag removals occur before any tag additions.

I was unaware of this point, if I do

  notmuch tag -inbox +star tag:inbox some-expression

I will have starred nothing?

> > +	(unless (string-match-p "^[\+\-][_\+\-\\w]+$" (car words))
> > +	  (error "Action must be of the form `+thistag -that_tag'"))
> 
> The error message has inconsistent "thistag" and "that_tag".

That was somewhat intentional to illustrate that non-alphanumeric
characters could be used in tags.  Should the alphabet for tags be based
on a whitelist or blacklist?  It would be rather hard to validate a tag
operation when there is no assumption/restriction on the alphabet.

Jed

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] New function notmuch-search-operate-all: operate on all messages in the current query.

[Carl Worth]

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

On Fri, 27 Nov 2009 15:32:44 +0100, Jed Brown <jed@59A2.org> wrote:
> On Fri, 27 Nov 2009 06:02:45 -0800, Carl Worth <cworth@cworth.org> wrote:
> > Since this operates via a single call to "notmuch tag" you might mention
> > here that all tag removals occur before any tag additions.
> 
> I was unaware of this point, if I do
> 
>   notmuch tag -inbox +star tag:inbox some-expression
> 
> I will have starred nothing?

Oh no, it's not that.

It does one search and then does a single pass over all the messages
returned. So the above will star things.

The clarification about removing and adding really only matters if you
have the same tag appearing as both -foo and +foo in the same command
line, (which doesn't seem that useful, but there you have it).

I don't recall now why I made the special effort to handle tag removal
before tag addition rather than just applying tag changes in order. But
since that's what the current "notmuch tag" code does, I thought I
should point it out.

> > > +	(unless (string-match-p "^[\+\-][_\+\-\\w]+$" (car words))
> > > +	  (error "Action must be of the form `+thistag -that_tag'"))
> > 
> > The error message has inconsistent "thistag" and "that_tag".
> 
> That was somewhat intentional to illustrate that non-alphanumeric
> characters could be used in tags.  Should the alphabet for tags be based
> on a whitelist or blacklist?  It would be rather hard to validate a tag
> operation when there is no assumption/restriction on the alphabet.

Yeah, that's something we need to document. The current interface does
use space-separated tag lists without any escaping. But the command line
doesn't yet impose any restriction on tag names, and I did accidentally
add a tag once of just " " (oops!).

We should nail this down so that interface authors can know how to
validate things consistently. Any suggestions?

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] More portable and easier to read regex in notmuch-search-operate-all

[Jed Brown]

The former one worked in 23.1.50.1 but not in 23.1.1.

Signed-off-by: Jed Brown <jed@59A2.org>
---
 notmuch.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 96c5d96..65473ba 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -1152,7 +1152,7 @@ characters as well as `_.+-'.
     (let ((words action-split))
       (when (null words) (error "No operation given"))
       (while words
-	(unless (string-match-p "^[\+\-][_\+\-\\w]+$" (car words))
+	(unless (string-match-p "^[-+][-+_.[:word:]]+$" (car words))
 	  (error "Action must be of the form `+thistag -that_tag'"))
 	(setq words (cdr words))))
     (apply 'notmuch-call-notmuch-process "tag"
-- 
1.6.5.3

Re: [PATCH] More portable and easier to read regex in notmuch-search-operate-all

[Keith Packard]

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

On Sat, 28 Nov 2009 20:51:13 +0100, Jed Brown <jed@59A2.org> wrote:
> The former one worked in 23.1.50.1 but not in 23.1.1.
> 
> Signed-off-by: Jed Brown <jed@59A2.org>

Tested-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] More portable and easier to read regex in notmuch-search-operate-all

[Carl Worth]

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

On Sat, 28 Nov 2009 11:58:27 -0800, Keith Packard <keithp@keithp.com> wrote:
> On Sat, 28 Nov 2009 20:51:13 +0100, Jed Brown <jed@59A2.org> wrote:
> > The former one worked in 23.1.50.1 but not in 23.1.1.
> > 
> > Signed-off-by: Jed Brown <jed@59A2.org>
> 
> Tested-by: Keith Packard <keithp@keithp.com>

Pushed-by: Carl Worth <cworth@cworth.org>

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]