[PATCH 0/8] Improve tag change completion

[PATCH 0/8] Improve tag change completion

[Austin Clements]

This series improves tag change completion in various ways for
commands like +, -, and *.

From a user perspective, this provides command-specific prompts like
"Tag message" and "Tag all" instead of the generic "Tag" prompt, and
bases tag removal completions on the tags that are in the buffer,
rather than the current tags in the database, providing a more
predicable experience.

From an implementation perspective, this new tag removal completion
behavior improves efficiency and eliminates a road block to fixing the
tagging race bug (which otherwise results in massive queries just to
compute removal completions).  The new code is also more "Elispy" and
predictable because all tag change prompting now occurs at the
interactive entry points, rather than buried under several layers of
non-interactive calls.

This is a spiritual successor to
id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though
it takes a very different approach.  This is also a prerequisite to
the tag race fix in
id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to
send an updated version of that series when this one is accepted.

Patches 1, 5, and 6 could be pushed on their own.  They fix bugs or
sort of bugs that get in the way of the rest of the series.

[PATCH 1/8] emacs: Fix misuse of `notmuch-tag'

[Austin Clements]

The calling convention for `notmuch-tag' changed in commit 97aa3c06 to
take a list of tag changes instead of a &rest argument, but the call
from `notmuch-search-tag-all' still passed a &rest argument.  This
happened to work for interactive calls because tag-changes would be
nil, so the `apply' call would pass only the query string to
`notmuch-tag' and simply omit the &optional tag-changes argument.
---
 emacs/notmuch.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c47c6b5..e0511c8 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -837,7 +837,7 @@ non-authors is found, assume that all of the authors match."
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
   (interactive)
-  (apply 'notmuch-tag notmuch-search-query-string tag-changes))
+  (notmuch-tag notmuch-search-query-string tag-changes))
 
 (defun notmuch-search-buffer-title (query)
   "Returns the title for a buffer with notmuch search results."
-- 
1.8.4.rc3

[PATCH 2/8] emacs: Take prompt and current tags in `notmuch-read-tag-changes'

[Austin Clements]

This modifies the interface of `notmuch-read-tag-changes' to take an
optional prompt string as well as a list of existing tags instead of a
query.  This list of tags is used to populate the tag removal
completions and lets the caller compute these in a more
efficient/consistent manner than performing a potentially large or
complex query.  This patch also updates the sole current caller of
`notmuch-read-tag-changes'.
---
 emacs/notmuch-tag.el | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 064cfa8..f9c1740 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -188,7 +188,7 @@ the messages that were tagged"
   "Variable to store minibuffer history for
 `notmuch-read-tag-changes' function.")
 
-(defun notmuch-tag-completions (&optional search-terms)
+(defun notmuch-tag-completions (&rest search-terms)
   (if (null search-terms)
       (setq search-terms (list "*")))
   (split-string
@@ -202,14 +202,21 @@ the messages that were tagged"
   (let ((tag-list (notmuch-tag-completions search-terms)))
     (completing-read prompt tag-list nil nil nil 'notmuch-select-tag-history)))
 
-(defun notmuch-read-tag-changes (&optional initial-input &rest search-terms)
+(defun notmuch-read-tag-changes (current-tags &optional prompt initial-input)
+  "Prompt for tag changes in the minibuffer.
+
+CURRENT-TAGS is a list of tags that are present on the message or
+messages to be changed.  These are offered as tag removal
+completions.  CURRENT-TAGS may contain duplicates.  PROMPT, if
+non-nil, is the query string to present in the minibuffer.  It
+defaults to \"Tags\".  INITIAL-INPUT, if non-nil, will be the
+initial input in the minibuffer."
+
   (let* ((all-tag-list (notmuch-tag-completions))
 	 (add-tag-list (mapcar (apply-partially 'concat "+") all-tag-list))
-	 (remove-tag-list (mapcar (apply-partially 'concat "-")
-				  (if (null search-terms)
-				      all-tag-list
-				    (notmuch-tag-completions search-terms))))
+	 (remove-tag-list (mapcar (apply-partially 'concat "-") current-tags))
 	 (tag-list (append add-tag-list remove-tag-list))
+	 (prompt (concat (or prompt "Tags") " (+add -drop): "))
 	 (crm-separator " ")
 	 ;; By default, space is bound to "complete word" function.
 	 ;; Re-bind it to insert a space instead.  Note that <tab>
@@ -219,7 +226,7 @@ the messages that were tagged"
 	    (set-keymap-parent map crm-local-completion-map)
 	    (define-key map " " 'self-insert-command)
 	    map)))
-    (delete "" (completing-read-multiple "Tags (+add -drop): "
+    (delete "" (completing-read-multiple prompt
 		tag-list nil nil initial-input
 		'notmuch-read-tag-changes-history))))
 
@@ -260,7 +267,8 @@ notmuch-after-tag-hook will be run."
   ;; Perform some validation
   (if (string-or-null-p tag-changes)
       (if (or (string= tag-changes "-") (string= tag-changes "+") (null tag-changes))
-	  (setq tag-changes (notmuch-read-tag-changes tag-changes query))
+	  (setq tag-changes (notmuch-read-tag-changes
+			     (notmuch-tag-completions query) nil tag-changes))
 	(setq tag-changes (list tag-changes))))
   (mapc (lambda (tag-change)
 	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
-- 
1.8.4.rc3

[PATCH 3/8] emacs: Use interactive specifications for tag changes in show

[Austin Clements]

This modifies all tagging operations in show to call
`notmuch-read-tag-changes' in their interactive specification to input
tag changes, rather than depending on lower-level functions to prompt
for tag changes regardless of their calling context.

Besides being more Elispy and providing a more consistent programmatic
API, this enables callers to provide two call site-specific pieces of
information: an appropriate prompt, and the set of visible tags.  The
prompt lets us differentiate * from +/-.  Providing visible tags
enables a more consistent user experience than retrieving the
(potentially different) tags from the database, and avoids a
round-trip to the CLI and database.
---
 emacs/notmuch-show.el | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 7325792..ef77839 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1780,23 +1780,29 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
       (notmuch-tag (notmuch-show-get-message-id) tag-changes)
       (notmuch-show-set-tags new-tags))))
 
-(defun notmuch-show-tag (&optional tag-changes)
+(defun notmuch-show-tag (tag-changes)
   "Change tags for the current message.
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
-  (interactive)
-  (let* ((tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
-	 (current-tags (notmuch-show-get-tags))
+  (interactive (list (notmuch-read-tag-changes (notmuch-show-get-tags)
+					       "Tag message")))
+  (notmuch-tag (notmuch-show-get-message-id) tag-changes)
+  (let* ((current-tags (notmuch-show-get-tags))
 	 (new-tags (notmuch-update-tags current-tags tag-changes)))
     (unless (equal current-tags new-tags)
       (notmuch-show-set-tags new-tags))))
 
-(defun notmuch-show-tag-all (&optional tag-changes)
+(defun notmuch-show-tag-all (tag-changes)
   "Change tags for all messages in the current show buffer.
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
-  (interactive)
-  (setq tag-changes (notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
+  (interactive
+   (list (let (tags)
+	   (notmuch-show-mapc
+	    (lambda () (setq tags (append (notmuch-show-get-tags) tags))))
+	   (delete-dups tags)
+	   (notmuch-read-tag-changes tags "Tag thread"))))
+  (notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes)
   (notmuch-show-mapc
    (lambda ()
      (let* ((current-tags (notmuch-show-get-tags))
@@ -1804,19 +1810,21 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
        (unless (equal current-tags new-tags)
 	 (notmuch-show-set-tags new-tags))))))
 
-(defun notmuch-show-add-tag ()
+(defun notmuch-show-add-tag (tag-changes)
   "Change tags for the current message (defaulting to add).
 
 Same as `notmuch-show-tag' but sets initial input to '+'."
-  (interactive)
-  (notmuch-show-tag "+"))
+  (interactive
+   (list (notmuch-read-tag-changes (notmuch-show-get-tags) "Tag message" "+")))
+  (notmuch-show-tag tag-changes))
 
-(defun notmuch-show-remove-tag ()
+(defun notmuch-show-remove-tag (tag-changes)
   "Change tags for the current message (defaulting to remove).
 
 Same as `notmuch-show-tag' but sets initial input to '-'."
-  (interactive)
-  (notmuch-show-tag "-"))
+  (interactive
+   (list (notmuch-read-tag-changes (notmuch-show-get-tags) "Tag message" "-")))
+  (notmuch-show-tag tag-changes))
 
 (defun notmuch-show-toggle-visibility-headers ()
   "Toggle the visibility of the current message headers."
-- 
1.8.4.rc3

[PATCH 4/8] emacs: Use interactive specifications for tag changes in search

[Austin Clements]

This is similar to the previous commit, but applies to search.

Search is somewhat more complicated because its tagging operations can
also apply to a region.  Hence, this lifts interactive prompting into
a helper function.  This also takes advantage of the new ability to
provide a prompt to distinguish tagging a single thread from tagging a
region of threads.
---
 emacs/notmuch.el | 70 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index e0511c8..5492e1b 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -560,43 +560,59 @@ will be signaled."
 	(setq output (append output (notmuch-search-get-tags pos)))))
     output))
 
-(defun notmuch-search-tag-region (beg end &optional tag-changes)
-  "Change tags for threads in the given region."
+(defun notmuch-search-interactive-region ()
+  "Return the bounds of the current interactive region.
+
+This returns (BEG END), where BEG and END are the bounds of the
+region if the region is active, or both `point' otherwise."
+  (if (region-active-p)
+      (list (region-beginning) (region-end))
+    (list (point) (point))))
+
+(defun notmuch-search-interactive-tag-changes (&optional initial-input)
+  "Prompt for tag changes for the current thread or region.
+
+Returns (TAG-CHANGES REGION-BEGIN REGION-END)."
+  (let* ((region (notmuch-search-interactive-region))
+	 (beg (first region)) (end (second region))
+	 (prompt (if (= beg end) "Tag thread" "Tag region")))
+    (cons (notmuch-read-tag-changes
+	   (notmuch-search-get-tags-region beg end) prompt initial-input)
+	  region)))
+
+(defun notmuch-search-tag (tag-changes &optional beg end)
+  "Change tags for the currently selected thread or region.
+
+See `notmuch-tag' for information on the format of TAG-CHANGES.
+If BEG or END are nil, applies to the thread at point."
+  (interactive (notmuch-search-interactive-tag-changes))
+  (unless (and beg end) (setq beg (point) end (point)))
   (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
-    (setq tag-changes (notmuch-tag search-string tag-changes))
+    (notmuch-tag search-string tag-changes)
     (notmuch-search-foreach-result beg end
       (lambda (pos)
 	(notmuch-search-set-tags
 	 (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
 	 pos)))))
 
-(defun notmuch-search-tag (&optional tag-changes)
-  "Change tags for the currently selected thread or region.
-
-See `notmuch-tag' for information on the format of TAG-CHANGES."
-  (interactive)
-  (let* ((beg (if (region-active-p) (region-beginning) (point)))
-	 (end (if (region-active-p) (region-end) (point))))
-    (notmuch-search-tag-region beg end tag-changes)))
-
-(defun notmuch-search-add-tag ()
-  "Change tags for the current thread (defaulting to add).
+(defun notmuch-search-add-tag (tag-changes &optional beg end)
+  "Change tags for the current thread or region (defaulting to add).
 
 Same as `notmuch-search-tag' but sets initial input to '+'."
-  (interactive)
-  (notmuch-search-tag "+"))
+  (interactive (notmuch-search-interactive-tag-changes "+"))
+  (notmuch-search-tag tag-changes beg end))
 
-(defun notmuch-search-remove-tag ()
-  "Change tags for the current thread (defaulting to remove).
+(defun notmuch-search-remove-tag (tag-changes &optional beg end)
+  "Change tags for the current thread or region (defaulting to remove).
 
 Same as `notmuch-search-tag' but sets initial input to '-'."
-  (interactive)
-  (notmuch-search-tag "-"))
+  (interactive (notmuch-search-interactive-tag-changes "-"))
+  (notmuch-search-tag tag-changes beg end))
 
 (put 'notmuch-search-archive-thread 'notmuch-prefix-doc
      "Un-archive the currently selected thread.")
-(defun notmuch-search-archive-thread (&optional unarchive)
-  "Archive the currently selected thread.
+(defun notmuch-search-archive-thread (&optional unarchive beg end)
+  "Archive the currently selected thread or region.
 
 Archive each message in the currently selected thread by applying
 the tag changes in `notmuch-archive-tags' to each (remove the
@@ -605,10 +621,10 @@ messages will be \"unarchived\" (i.e. the tag changes in
 `notmuch-archive-tags' will be reversed).
 
 This function advances the next thread when finished."
-  (interactive "P")
+  (interactive (cons current-prefix-arg (notmuch-search-interactive-region)))
   (when notmuch-archive-tags
     (notmuch-search-tag
-     (notmuch-tag-change-list notmuch-archive-tags unarchive)))
+     (notmuch-tag-change-list notmuch-archive-tags unarchive) beg end))
   (notmuch-search-next-thread))
 
 (defun notmuch-search-update-result (result &optional pos)
@@ -832,11 +848,13 @@ non-authors is found, assume that all of the authors match."
 	(notmuch-sexp-parse-partial-list 'notmuch-search-show-result
 					 results-buf)))))
 
-(defun notmuch-search-tag-all (&optional tag-changes)
+(defun notmuch-search-tag-all (tag-changes)
   "Add/remove tags from all messages in current search buffer.
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
-  (interactive)
+  (interactive
+   (list (notmuch-read-tag-changes
+	  (notmuch-search-get-tags-region (point-min) (point-max)) "Tag all")))
   (notmuch-tag notmuch-search-query-string tag-changes))
 
 (defun notmuch-search-buffer-title (query)
-- 
1.8.4.rc3

[PATCH 5/8] pick: Fix incorrect use of `notmuch-pick-tag'

[Austin Clements]

`notmuch-pick-tag' takes a list of tag changes, but
`notmuch-pick-archive-message' passes it a &rest argument.  This
happens to work if `notmuch-archive-tags' contains a single tag (which
it usually does), but will break if it does not.
---
 contrib/notmuch-pick/notmuch-pick.el | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 0aa651e..8f504ed 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -542,8 +542,7 @@ message will be \"unarchived\", i.e. the tag changes in
 `notmuch-archive-tags' will be reversed."
   (interactive "P")
   (when notmuch-archive-tags
-    (apply 'notmuch-pick-tag
-	   (notmuch-tag-change-list notmuch-archive-tags unarchive))))
+    (notmuch-pick-tag (notmuch-tag-change-list notmuch-archive-tags unarchive))))
 
 (defun notmuch-pick-archive-message-then-next (&optional unarchive)
   "Archive the current message and move to next matching message."
-- 
1.8.4.rc3

[PATCH 6/8] pick: Use list form of tag-changes in test

[Austin Clements]

Currently we support both string and list forms of tag-changes for
historical reasons.  This is about to change, so fix pick's tests that
use the legacy string form of tag-changes.
---
 contrib/notmuch-pick/test/emacs-pick | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/notmuch-pick/test/emacs-pick b/contrib/notmuch-pick/test/emacs-pick
index a802d0e..40b0738 100755
--- a/contrib/notmuch-pick/test/emacs-pick
+++ b/contrib/notmuch-pick/test/emacs-pick
@@ -44,7 +44,7 @@ test_emacs '(add-to-list (quote load-path) "'$PICK_DIR'")
 	    (notmuch-pick "tag:inbox")
 	    (notmuch-test-wait)
 	    (forward-line)
-	    (notmuch-pick-tag "+test_tag")
+	    (notmuch-pick-tag (list "+test_tag"))
 	    (test-output)
 	    (delete-other-windows)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-pick-tag-inbox-tagged
@@ -59,7 +59,7 @@ test_emacs '(add-to-list (quote load-path) "'$PICK_DIR'")
 	    (notmuch-pick "tag:inbox")
 	    (notmuch-test-wait)
 	    (forward-line)
-	    (notmuch-pick-tag "-test_tag")
+	    (notmuch-pick-tag (list "-test_tag"))
 	    (test-output)
 	    (delete-other-windows)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-pick-tag-inbox
@@ -75,7 +75,7 @@ test_emacs '(add-to-list (quote load-path) "'$PICK_DIR'")
 	    (notmuch-test-wait)
 	    ;; move to a sizable thread
 	    (forward-line 26)
-	    (notmuch-pick-tag-thread "+test_thread_tag")
+	    (notmuch-pick-tag-thread (list "+test_thread_tag"))
 	    (test-output)
 	    (delete-other-windows)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-pick-tag-inbox-thread-tagged
@@ -98,7 +98,7 @@ test_emacs '(add-to-list (quote load-path) "'$PICK_DIR'")
 	    (notmuch-test-wait)
 	    ;; move to the same sizable thread as above
 	    (forward-line 26)
-	    (notmuch-pick-tag-thread "-test_thread_tag")
+	    (notmuch-pick-tag-thread (list "-test_thread_tag"))
 	    (test-output)
 	    (delete-other-windows)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-pick-tag-inbox
-- 
1.8.4.rc3

[PATCH 7/8] pick: Use interactive specifications for tag changes

[Austin Clements]

---
 contrib/notmuch-pick/notmuch-pick.el | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 8f504ed..13682ea 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -37,7 +37,7 @@
 (declare-function notmuch-show-strip-re "notmuch-show" (subject))
 (declare-function notmuch-show-spaces-n "notmuch-show" (n))
 (declare-function notmuch-read-query "notmuch" (prompt))
-(declare-function notmuch-read-tag-changes "notmuch" (&optional initial-input &rest search-terms))
+(declare-function notmuch-read-tag-changes "notmuch" (current-tags &optional prompt initial-input))
 (declare-function notmuch-update-tags "notmuch" (current-tags tag-changes))
 (declare-function notmuch-hello-trim "notmuch-hello" (search))
 (declare-function notmuch-search-find-thread-id "notmuch" ())
@@ -372,21 +372,24 @@ Does NOT change the database."
       (notmuch-pick-set-tags new-tags)
       (notmuch-pick-refresh-result))))
 
-(defun notmuch-pick-tag (&optional tag-changes)
+(defun notmuch-pick-tag (tag-changes)
   "Change tags for the current message"
-  (interactive)
-  (setq tag-changes (notmuch-tag (notmuch-pick-get-message-id) tag-changes))
+  (interactive
+   (list (notmuch-read-tag-changes (notmuch-pick-get-tags) "Tag message")))
+  (notmuch-tag (notmuch-pick-get-message-id) tag-changes)
   (notmuch-pick-tag-update-display tag-changes))
 
-(defun notmuch-pick-add-tag ()
+(defun notmuch-pick-add-tag (tag-changes)
   "Same as `notmuch-pick-tag' but sets initial input to '+'."
-  (interactive)
-  (notmuch-pick-tag "+"))
+  (interactive
+   (list (notmuch-read-tag-changes (notmuch-pick-get-tags) "Tag message" "+")))
+  (notmuch-pick-tag tag-changes))
 
-(defun notmuch-pick-remove-tag ()
+(defun notmuch-pick-remove-tag (tag-changes)
   "Same as `notmuch-pick-tag' but sets initial input to '-'."
-  (interactive)
-  (notmuch-pick-tag "-"))
+  (interactive
+   (list (notmuch-read-tag-changes (notmuch-pick-get-tags) "Tag message" "-")))
+  (notmuch-pick-tag tag-changes))
 
 ;; The next two functions close the message window before searching or
 ;; picking but they do so after the user has entered the query (in
@@ -626,13 +629,16 @@ message will be \"unarchived\", i.e. the tag changes in
 	     (notmuch-pick-thread-mapcar 'notmuch-pick-get-message-id)
 	     " or "))
 
-(defun notmuch-pick-tag-thread (&optional tag-changes)
+(defun notmuch-pick-tag-thread (tag-changes)
   "Tag all messages in the current thread"
-  (interactive)
+  (interactive
+   (let ((tags (apply #'append (notmuch-pick-thread-mapcar
+				(lambda () (notmuch-pick-get-tags))))))
+     (list (notmuch-read-tag-changes tags "Tag thread"))))
   (when (notmuch-pick-get-message-properties)
-    (let ((tag-changes (notmuch-tag (notmuch-pick-get-messages-ids-thread-search) tag-changes)))
-      (notmuch-pick-thread-mapcar
-       (lambda () (notmuch-pick-tag-update-display tag-changes))))))
+    (notmuch-tag (notmuch-pick-get-messages-ids-thread-search) tag-changes)
+    (notmuch-pick-thread-mapcar
+     (lambda () (notmuch-pick-tag-update-display tag-changes)))))
 
 (defun notmuch-pick-archive-thread (&optional unarchive)
   "Archive each message in thread.
-- 
1.8.4.rc3

[PATCH 8/8] emacs: Remove interactive behavior of `notmuch-tag'

[Austin Clements]

We no longer use this, since we've lifted all interactive behavior to
the appropriate interactive entry points.  Because of this,
`notmuch-tag' also no longer needs to return the tag changes list,
since the caller always passes it in.
---
 emacs/notmuch-tag.el | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index f9c1740..feee17c 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -249,27 +249,18 @@ from TAGS if present."
 	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
     (sort result-tags 'string<)))
 
-(defun notmuch-tag (query &optional tag-changes)
+(defun notmuch-tag (query tag-changes)
   "Add/remove tags in TAG-CHANGES to messages matching QUERY.
 
 QUERY should be a string containing the search-terms.
-TAG-CHANGES can take multiple forms.  If TAG-CHANGES is a list of
-strings of the form \"+tag\" or \"-tag\" then those are the tag
-changes applied.  If TAG-CHANGES is a string then it is
-interpreted as a single tag change.  If TAG-CHANGES is the string
-\"-\" or \"+\", or null, then the user is prompted to enter the
-tag changes.
+TAG-CHANGES is a list of strings of the form \"+tag\" or
+\"-tag\" to add or remove tags, respectively.
 
 Note: Other code should always use this function alter tags of
 messages instead of running (notmuch-call-notmuch-process \"tag\" ..)
 directly, so that hooks specified in notmuch-before-tag-hook and
 notmuch-after-tag-hook will be run."
   ;; Perform some validation
-  (if (string-or-null-p tag-changes)
-      (if (or (string= tag-changes "-") (string= tag-changes "+") (null tag-changes))
-	  (setq tag-changes (notmuch-read-tag-changes
-			     (notmuch-tag-completions query) nil tag-changes))
-	(setq tag-changes (list tag-changes))))
   (mapc (lambda (tag-change)
 	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
 	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
@@ -278,9 +269,7 @@ notmuch-after-tag-hook will be run."
     (run-hooks 'notmuch-before-tag-hook)
     (apply 'notmuch-call-notmuch-process "tag"
 	   (append tag-changes (list "--" query)))
-    (run-hooks 'notmuch-after-tag-hook))
-  ;; in all cases we return tag-changes as a list
-  tag-changes)
+    (run-hooks 'notmuch-after-tag-hook)))
 
 (defun notmuch-tag-change-list (tags &optional reverse)
   "Convert TAGS into a list of tag changes.
-- 
1.8.4.rc3

Re: [PATCH 0/8] Improve tag change completion

[Mark Walters]

This looks good to me +1. It makes the code clearer and nicer to read as
well as giving a better user experience, and it is makes fixing the long
standing tagging races simpler.

I have a couple of docstring comments:

In patch 2 perhaps notmuch-tag-completions could have a docstring.

In Patch 4 I think the docstring for notmuch-search-tag is outdated: it
 is "Change tags for the currently selected thread or region." but 
 beg and end can now be specified by the caller.

and one actual comment:

in patch 3 (for show) delete-dups is called before the list is passed to
notmuch-read-tag-changes whereas it is not for search or pick.
Obviously this is not actually a problem but it might be worth being
consistent.

But that was all I found. All tests pass and everything I try behaves
exactly as expected.

Best wishes

Mark


On Tue, 22 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> This series improves tag change completion in various ways for
> commands like +, -, and *.
>
> From a user perspective, this provides command-specific prompts like
> "Tag message" and "Tag all" instead of the generic "Tag" prompt, and
> bases tag removal completions on the tags that are in the buffer,
> rather than the current tags in the database, providing a more
> predicable experience.
>
> From an implementation perspective, this new tag removal completion
> behavior improves efficiency and eliminates a road block to fixing the
> tagging race bug (which otherwise results in massive queries just to
> compute removal completions).  The new code is also more "Elispy" and
> predictable because all tag change prompting now occurs at the
> interactive entry points, rather than buried under several layers of
> non-interactive calls.
>
> This is a spiritual successor to
> id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though
> it takes a very different approach.  This is also a prerequisite to
> the tag race fix in
> id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to
> send an updated version of that series when this one is accepted.
>
> Patches 1, 5, and 6 could be pushed on their own.  They fix bugs or
> sort of bugs that get in the way of the rest of the series.
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 0/8] Improve tag change completion

[Austin Clements]

Quoth Mark Walters on Oct 22 at 10:43 pm:
> This looks good to me +1. It makes the code clearer and nicer to read as
> well as giving a better user experience, and it is makes fixing the long
> standing tagging races simpler.
> 
> I have a couple of docstring comments:
> 
> In patch 2 perhaps notmuch-tag-completions could have a docstring.

Added.  I noticed that I had failed to update the call from
`notmuch-select-tag-with-completion', so I fixed that, too.  I don't
understand why we take lists of search terms in random places and
never use more than one element, but I suppose this series doesn't
make that any worse.

> In Patch 4 I think the docstring for notmuch-search-tag is outdated: it
>  is "Change tags for the currently selected thread or region." but 
>  beg and end can now be specified by the caller.

I've left the first sentence as it is, since it's good interactive
documentation and a typical way to describe functions even if they
take a region as arguments (see, for example, `kill-region').  But
I've elaborated the rest of the docstring to be clearer about this.

> and one actual comment:
> 
> in patch 3 (for show) delete-dups is called before the list is passed to
> notmuch-read-tag-changes whereas it is not for search or pick.
> Obviously this is not actually a problem but it might be worth being
> consistent.

Ah, whoops.  I'd done this before I decided to handle duplicates in
`notmuch-read-tag-changes'.  Since it's redundant, I've removed it.

> But that was all I found. All tests pass and everything I try behaves
> exactly as expected.
> 
> Best wishes
> 
> Mark
> 
> 
> On Tue, 22 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > This series improves tag change completion in various ways for
> > commands like +, -, and *.
> >
> > From a user perspective, this provides command-specific prompts like
> > "Tag message" and "Tag all" instead of the generic "Tag" prompt, and
> > bases tag removal completions on the tags that are in the buffer,
> > rather than the current tags in the database, providing a more
> > predicable experience.
> >
> > From an implementation perspective, this new tag removal completion
> > behavior improves efficiency and eliminates a road block to fixing the
> > tagging race bug (which otherwise results in massive queries just to
> > compute removal completions).  The new code is also more "Elispy" and
> > predictable because all tag change prompting now occurs at the
> > interactive entry points, rather than buried under several layers of
> > non-interactive calls.
> >
> > This is a spiritual successor to
> > id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though
> > it takes a very different approach.  This is also a prerequisite to
> > the tag race fix in
> > id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to
> > send an updated version of that series when this one is accepted.
> >
> > Patches 1, 5, and 6 could be pushed on their own.  They fix bugs or
> > sort of bugs that get in the way of the rest of the series.

Re: [PATCH 0/8] Improve tag change completion

[Mark Walters]

Hi

On Wed, 23 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Oct 22 at 10:43 pm:
>> This looks good to me +1. It makes the code clearer and nicer to read as
>> well as giving a better user experience, and it is makes fixing the long
>> standing tagging races simpler.
>> 
>> I have a couple of docstring comments:
>> 
>> In patch 2 perhaps notmuch-tag-completions could have a docstring.
>
> Added.  I noticed that I had failed to update the call from
> `notmuch-select-tag-with-completion', so I fixed that, too.  I don't
> understand why we take lists of search terms in random places and
> never use more than one element, but I suppose this series doesn't
> make that any worse.

As far as I can see, at the end of the series, notmuch-tag-completions
is only called with no argument: i.e., it's always just finding the list
of all tags. This is because notmuch-select-tag-with-completion is only
called once from "notmuch-search-filter-by-tag" with no search-terms
argument. So it might be nice to just remove the search-terms
completely. (The only downside is we might break user lisp.)

Best wishes

Mark


>> In Patch 4 I think the docstring for notmuch-search-tag is outdated: it
>>  is "Change tags for the currently selected thread or region." but 
>>  beg and end can now be specified by the caller.
>
> I've left the first sentence as it is, since it's good interactive
> documentation and a typical way to describe functions even if they
> take a region as arguments (see, for example, `kill-region').  But
> I've elaborated the rest of the docstring to be clearer about this.
>
>> and one actual comment:
>> 
>> in patch 3 (for show) delete-dups is called before the list is passed to
>> notmuch-read-tag-changes whereas it is not for search or pick.
>> Obviously this is not actually a problem but it might be worth being
>> consistent.
>
> Ah, whoops.  I'd done this before I decided to handle duplicates in
> `notmuch-read-tag-changes'.  Since it's redundant, I've removed it.
>
>> But that was all I found. All tests pass and everything I try behaves
>> exactly as expected.
>> 
>> Best wishes
>> 
>> Mark
>> 
>> 
>> On Tue, 22 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> > This series improves tag change completion in various ways for
>> > commands like +, -, and *.
>> >
>> > From a user perspective, this provides command-specific prompts like
>> > "Tag message" and "Tag all" instead of the generic "Tag" prompt, and
>> > bases tag removal completions on the tags that are in the buffer,
>> > rather than the current tags in the database, providing a more
>> > predicable experience.
>> >
>> > From an implementation perspective, this new tag removal completion
>> > behavior improves efficiency and eliminates a road block to fixing the
>> > tagging race bug (which otherwise results in massive queries just to
>> > compute removal completions).  The new code is also more "Elispy" and
>> > predictable because all tag change prompting now occurs at the
>> > interactive entry points, rather than buried under several layers of
>> > non-interactive calls.
>> >
>> > This is a spiritual successor to
>> > id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though
>> > it takes a very different approach.  This is also a prerequisite to
>> > the tag race fix in
>> > id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to
>> > send an updated version of that series when this one is accepted.
>> >
>> > Patches 1, 5, and 6 could be pushed on their own.  They fix bugs or
>> > sort of bugs that get in the way of the rest of the series.

Re: [PATCH 0/8] Improve tag change completion

[Austin Clements]

Quoth Mark Walters on Oct 23 at 10:56 am:
> 
> Hi
> 
> On Wed, 23 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > Quoth Mark Walters on Oct 22 at 10:43 pm:
> >> This looks good to me +1. It makes the code clearer and nicer to read as
> >> well as giving a better user experience, and it is makes fixing the long
> >> standing tagging races simpler.
> >> 
> >> I have a couple of docstring comments:
> >> 
> >> In patch 2 perhaps notmuch-tag-completions could have a docstring.
> >
> > Added.  I noticed that I had failed to update the call from
> > `notmuch-select-tag-with-completion', so I fixed that, too.  I don't
> > understand why we take lists of search terms in random places and
> > never use more than one element, but I suppose this series doesn't
> > make that any worse.
> 
> As far as I can see, at the end of the series, notmuch-tag-completions
> is only called with no argument: i.e., it's always just finding the list
> of all tags. This is because notmuch-select-tag-with-completion is only
> called once from "notmuch-search-filter-by-tag" with no search-terms
> argument. So it might be nice to just remove the search-terms
> completely. (The only downside is we might break user lisp.)

That's true.  Though it doesn't significantly complicate the code and
it's hard to say if we may need this for something else in the future,
so I'd just as soon leave it until we have a more compelling reason to
remove it (e.g., if we add tag list caching or something).

> Best wishes
> 
> Mark
> 
> 
> >> In Patch 4 I think the docstring for notmuch-search-tag is outdated: it
> >>  is "Change tags for the currently selected thread or region." but 
> >>  beg and end can now be specified by the caller.
> >
> > I've left the first sentence as it is, since it's good interactive
> > documentation and a typical way to describe functions even if they
> > take a region as arguments (see, for example, `kill-region').  But
> > I've elaborated the rest of the docstring to be clearer about this.
> >
> >> and one actual comment:
> >> 
> >> in patch 3 (for show) delete-dups is called before the list is passed to
> >> notmuch-read-tag-changes whereas it is not for search or pick.
> >> Obviously this is not actually a problem but it might be worth being
> >> consistent.
> >
> > Ah, whoops.  I'd done this before I decided to handle duplicates in
> > `notmuch-read-tag-changes'.  Since it's redundant, I've removed it.
> >
> >> But that was all I found. All tests pass and everything I try behaves
> >> exactly as expected.
> >> 
> >> Best wishes
> >> 
> >> Mark
> >> 
> >> 
> >> On Tue, 22 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> >> > This series improves tag change completion in various ways for
> >> > commands like +, -, and *.
> >> >
> >> > From a user perspective, this provides command-specific prompts like
> >> > "Tag message" and "Tag all" instead of the generic "Tag" prompt, and
> >> > bases tag removal completions on the tags that are in the buffer,
> >> > rather than the current tags in the database, providing a more
> >> > predicable experience.
> >> >
> >> > From an implementation perspective, this new tag removal completion
> >> > behavior improves efficiency and eliminates a road block to fixing the
> >> > tagging race bug (which otherwise results in massive queries just to
> >> > compute removal completions).  The new code is also more "Elispy" and
> >> > predictable because all tag change prompting now occurs at the
> >> > interactive entry points, rather than buried under several layers of
> >> > non-interactive calls.
> >> >
> >> > This is a spiritual successor to
> >> > id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though
> >> > it takes a very different approach.  This is also a prerequisite to
> >> > the tag race fix in
> >> > id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to
> >> > send an updated version of that series when this one is accepted.
> >> >
> >> > Patches 1, 5, and 6 could be pushed on their own.  They fix bugs or
> >> > sort of bugs that get in the way of the rest of the series.

Re: [PATCH 8/8] emacs: Remove interactive behavior of `notmuch-tag'

[Jameson Graef Rollins]

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

On Tue, Oct 22 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> We no longer use this, since we've lifted all interactive behavior to
> the appropriate interactive entry points.  Because of this,
> `notmuch-tag' also no longer needs to return the tag changes list,
> since the caller always passes it in.
> ---
>  emacs/notmuch-tag.el | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index f9c1740..feee17c 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -249,27 +249,18 @@ from TAGS if present."
>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
>      (sort result-tags 'string<)))
>  
> -(defun notmuch-tag (query &optional tag-changes)
> +(defun notmuch-tag (query tag-changes)
>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
>  
>  QUERY should be a string containing the search-terms.
> -TAG-CHANGES can take multiple forms.  If TAG-CHANGES is a list of
> -strings of the form \"+tag\" or \"-tag\" then those are the tag
> -changes applied.  If TAG-CHANGES is a string then it is
> -interpreted as a single tag change.  If TAG-CHANGES is the string
> -\"-\" or \"+\", or null, then the user is prompted to enter the
> -tag changes.
> +TAG-CHANGES is a list of strings of the form \"+tag\" or
> +\"-tag\" to add or remove tags, respectively.
>  
>  Note: Other code should always use this function alter tags of
>  messages instead of running (notmuch-call-notmuch-process \"tag\" ..)
>  directly, so that hooks specified in notmuch-before-tag-hook and
>  notmuch-after-tag-hook will be run."
>    ;; Perform some validation
> -  (if (string-or-null-p tag-changes)
> -      (if (or (string= tag-changes "-") (string= tag-changes "+") (null tag-changes))
> -	  (setq tag-changes (notmuch-read-tag-changes
> -			     (notmuch-tag-completions query) nil tag-changes))
> -	(setq tag-changes (list tag-changes))))

Hey folks.  After a recent upgrade to 0.16+120~gfd733a4+1 I found that
my custom tagging functions weren't working, which I traced back to the
removal of this section.

Previously notmuch-tag accepted tag changes in two forms: as a list of
tag changes:

(list "+foo" "-bar")

or as a space-separated string:

"+foo -bar"

This removed section is what would turn the space-separated string into
a list.

I have custom tagging functions that were written like this:

(notmuch-search-tag "+spam")

notmuch-search-tag passes the TAG-CHANGES to notmuch-tag, which was
changing it to a list in this removed section.  Since its removal, all
of my custom functions started throwing the following error:

Wrong type argument: stringp, 43

I didn't pay super close attention to the rest of the patch series, but
From a backwards compatibility point of view, do we really need to
remove this section?  Is there a problem with specifying tag changes as
a string?  I think it was actually me that last modified notmuch-tag,
and I vaguely remember spending time thinking about how to preserve that
particular way to specify the changes, so that things would be simpler
for simple tag operations.

In any event, some amount of backwards compatibility has been removed,
so if we do want to keep it removed, we should add a good NEWS item to
explain that peoples custom bindings might break.

jamie.

>    (mapc (lambda (tag-change)
>  	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
>  	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
> @@ -278,9 +269,7 @@ notmuch-after-tag-hook will be run."
>      (run-hooks 'notmuch-before-tag-hook)
>      (apply 'notmuch-call-notmuch-process "tag"
>  	   (append tag-changes (list "--" query)))
> -    (run-hooks 'notmuch-after-tag-hook))
> -  ;; in all cases we return tag-changes as a list
> -  tag-changes)
> +    (run-hooks 'notmuch-after-tag-hook)))
>  
>  (defun notmuch-tag-change-list (tags &optional reverse)
>    "Convert TAGS into a list of tag changes.
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

Re: [PATCH 8/8] emacs: Remove interactive behavior of `notmuch-tag'

[Austin Clements]

Quoth Jameson Graef Rollins on Nov 03 at  4:42 pm:
> On Tue, Oct 22 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > We no longer use this, since we've lifted all interactive behavior to
> > the appropriate interactive entry points.  Because of this,
> > `notmuch-tag' also no longer needs to return the tag changes list,
> > since the caller always passes it in.
> > ---
> >  emacs/notmuch-tag.el | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> > index f9c1740..feee17c 100644
> > --- a/emacs/notmuch-tag.el
> > +++ b/emacs/notmuch-tag.el
> > @@ -249,27 +249,18 @@ from TAGS if present."
> >  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
> >      (sort result-tags 'string<)))
> >  
> > -(defun notmuch-tag (query &optional tag-changes)
> > +(defun notmuch-tag (query tag-changes)
> >    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
> >  
> >  QUERY should be a string containing the search-terms.
> > -TAG-CHANGES can take multiple forms.  If TAG-CHANGES is a list of
> > -strings of the form \"+tag\" or \"-tag\" then those are the tag
> > -changes applied.  If TAG-CHANGES is a string then it is
> > -interpreted as a single tag change.  If TAG-CHANGES is the string
> > -\"-\" or \"+\", or null, then the user is prompted to enter the
> > -tag changes.
> > +TAG-CHANGES is a list of strings of the form \"+tag\" or
> > +\"-tag\" to add or remove tags, respectively.
> >  
> >  Note: Other code should always use this function alter tags of
> >  messages instead of running (notmuch-call-notmuch-process \"tag\" ..)
> >  directly, so that hooks specified in notmuch-before-tag-hook and
> >  notmuch-after-tag-hook will be run."
> >    ;; Perform some validation
> > -  (if (string-or-null-p tag-changes)
> > -      (if (or (string= tag-changes "-") (string= tag-changes "+") (null tag-changes))
> > -	  (setq tag-changes (notmuch-read-tag-changes
> > -			     (notmuch-tag-completions query) nil tag-changes))
> > -	(setq tag-changes (list tag-changes))))
> 
> Hey folks.  After a recent upgrade to 0.16+120~gfd733a4+1 I found that
> my custom tagging functions weren't working, which I traced back to the
> removal of this section.
> 
> Previously notmuch-tag accepted tag changes in two forms: as a list of
> tag changes:
> 
> (list "+foo" "-bar")
> 
> or as a space-separated string:
> 
> "+foo -bar"
> 
> This removed section is what would turn the space-separated string into
> a list.
> 
> I have custom tagging functions that were written like this:
> 
> (notmuch-search-tag "+spam")
> 
> notmuch-search-tag passes the TAG-CHANGES to notmuch-tag, which was
> changing it to a list in this removed section.  Since its removal, all
> of my custom functions started throwing the following error:
> 
> Wrong type argument: stringp, 43
> 
> I didn't pay super close attention to the rest of the patch series, but
> From a backwards compatibility point of view, do we really need to
> remove this section?  Is there a problem with specifying tag changes as
> a string?  I think it was actually me that last modified notmuch-tag,
> and I vaguely remember spending time thinking about how to preserve that
> particular way to specify the changes, so that things would be simpler
> for simple tag operations.

The difficulty is that notmuch-tag used to play a dual role of both
parsing the space-separated string into a list of tag changes and
actually performing the tagging operation and, as a result, other
functions that were rooted in notmuch-tag also accepted both (though
functions that weren't, like notmuch-update-tags, required the list
form).  This dual role was strange from an API perspective, but also,
because of other restructuring, notmuch-tag is no longer at the core
of many operations, so it's not in a good position to be a parser.

Hence, while we could make `notmuch-tag' itself support the string
form, if we wanted this to work from other tagging entry-points, we'd
need to support both forms in all of the tagging functions.  This
would be doable, but it would be messy, and without the single parsing
point it would be difficult to maintain.

> In any event, some amount of backwards compatibility has been removed,
> so if we do want to keep it removed, we should add a good NEWS item to
> explain that peoples custom bindings might break.

I'd rather keep the backwards compatibility removed, but you're
definitely right that we need a good NEWS item for it.  I've posted a
NEWS item in id:1384296252-17884-1-git-send-email-amdragon@mit.edu,
but let me know if you think it can be clearer or more helpful.

> jamie.
> 
> >    (mapc (lambda (tag-change)
> >  	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
> >  	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
> > @@ -278,9 +269,7 @@ notmuch-after-tag-hook will be run."
> >      (run-hooks 'notmuch-before-tag-hook)
> >      (apply 'notmuch-call-notmuch-process "tag"
> >  	   (append tag-changes (list "--" query)))
> > -    (run-hooks 'notmuch-after-tag-hook))
> > -  ;; in all cases we return tag-changes as a list
> > -  tag-changes)
> > +    (run-hooks 'notmuch-after-tag-hook)))
> >  
> >  (defun notmuch-tag-change-list (tags &optional reverse)
> >    "Convert TAGS into a list of tag changes.
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch