[PATCH 00/11] Fix search tagging races

[PATCH 00/11] Fix search tagging races

[Austin Clements]

I was hacking on undo support for notmuch-emacs and sort of
accidentally wrote this instead.  This series fixes a set of
well-known races where tagging from search-mode unexpectedly affects
messages that arrived after the search was performed (and hence the
user doesn't know they're tagging them).  We've attacked this a few
times before, but have always run up against something that was
missing.  It turns out the pieces are finally all in place.

The first five patches just clean various things up in preparation.
Patches 6 and 7 add support for tagging large queries, which would
otherwise become a problem when later patches start using explicit
message ID-based queries for tagging.  The remaining four patches
actually fix the search tagging races using explicit message ID-based
queries.

It's a fairly long series, but none of the patches are very big.

[PATCH 01/11] schemata: Disambiguate non-terminal names

[Austin Clements]

Previously, the show schema and the search schema used different
"thread" non-terminals.  While these schemata don't interact, this is
still confusing, so rename search's "thread" to "thread_summary".  To
further limit confusion, prefix all top-level search non-terminals now
begin with "search_".
---
 devel/schemata | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/devel/schemata b/devel/schemata
index 2405756..cdd0e43 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -122,21 +122,21 @@ notmuch search schema
 ---------------------
 
 # --output=summary
-summary = [thread*]
+search_summary = [thread_summary*]
 
 # --output=threads
-threads = [threadid*]
+search_threads = [threadid*]
 
 # --output=messages
-messages = [messageid*]
+search_messages = [messageid*]
 
 # --output=files
-files = [string*]
+search_files = [string*]
 
 # --output=tags
-tags = [string*]
+search_tags = [string*]
 
-thread = {
+thread_summary = {
     thread:         threadid,
     timestamp:      unix_time,
     date_relative:  string,   # user-friendly timestamp
-- 
1.8.4.rc3

[PATCH 02/11] cli: Separate current and deprecated format version

[Austin Clements]

Previously, the CLI would print a deprecation warning if a client
requested any format version other than the current one.  However, if
we add fields that are backwards-compatible, but want clients to be
able to depend on, we need to bump the version, but that doesn't make
the older version deprecated.

Hence, separate out the "minimum active" version and only print a
warning for requests below this version number.
---
 notmuch-client.h | 5 +++++
 notmuch.c        | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index afb0ddf..8d986f4 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -142,6 +142,11 @@ chomp_newline (char *str)
 /* The minimum supported structured output format version.  Requests
  * for format versions below this will return an error. */
 #define NOTMUCH_FORMAT_MIN 1
+/* The minimum non-deprecated structured output format version.
+ * Requests for format versions below this will print a stern warning.
+ * Must be >= NOTMUCH_FORMAT_MIN and < NOTMUCH_FORMAT_CUR.
+ */
+#define NOTMUCH_FORMAT_MIN_ACTIVE 1
 
 /* The output format version requested by the caller on the command
  * line.  If no format version is requested, this will be set to
diff --git a/notmuch.c b/notmuch.c
index 7300c21..6588003 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -125,7 +125,7 @@ by the notmuch CLI (it requires at least version %d).  You may need to\n\
 upgrade your notmuch front-end.\n",
 		 notmuch_format_version, NOTMUCH_FORMAT_MIN);
 	exit (NOTMUCH_EXIT_FORMAT_TOO_OLD);
-    } else if (notmuch_format_version != NOTMUCH_FORMAT_CUR) {
+    } else if (notmuch_format_version < NOTMUCH_FORMAT_MIN_ACTIVE) {
 	/* Warn about old version requests so compatibility issues are
 	 * less likely when we drop support for a deprecated format
 	 * versions. */
-- 
1.8.4.rc3

Re: [PATCH 02/11] cli: Separate current and deprecated format version

[Mark Walters]

On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, the CLI would print a deprecation warning if a client
> requested any format version other than the current one.  However, if
> we add fields that are backwards-compatible, but want clients to be
> able to depend on, we need to bump the version, but that doesn't make
> the older version deprecated.
>
> Hence, separate out the "minimum active" version and only print a
> warning for requests below this version number.
> ---
>  notmuch-client.h | 5 +++++
>  notmuch.c        | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index afb0ddf..8d986f4 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -142,6 +142,11 @@ chomp_newline (char *str)
>  /* The minimum supported structured output format version.  Requests
>   * for format versions below this will return an error. */
>  #define NOTMUCH_FORMAT_MIN 1
> +/* The minimum non-deprecated structured output format version.
> + * Requests for format versions below this will print a stern warning.
> + * Must be >= NOTMUCH_FORMAT_MIN and < NOTMUCH_FORMAT_CUR.
> + */

Should this be <= NOTMUCH_FORMAT_CUR ?

Best wishes

Mark

> +#define NOTMUCH_FORMAT_MIN_ACTIVE 1
>  
>  /* The output format version requested by the caller on the command
>   * line.  If no format version is requested, this will be set to
> diff --git a/notmuch.c b/notmuch.c
> index 7300c21..6588003 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -125,7 +125,7 @@ by the notmuch CLI (it requires at least version %d).  You may need to\n\
>  upgrade your notmuch front-end.\n",
>  		 notmuch_format_version, NOTMUCH_FORMAT_MIN);
>  	exit (NOTMUCH_EXIT_FORMAT_TOO_OLD);
> -    } else if (notmuch_format_version != NOTMUCH_FORMAT_CUR) {
> +    } else if (notmuch_format_version < NOTMUCH_FORMAT_MIN_ACTIVE) {
>  	/* Warn about old version requests so compatibility issues are
>  	 * less likely when we drop support for a deprecated format
>  	 * versions. */
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 02/11] cli: Separate current and deprecated format version

[Austin Clements]

Quoth Mark Walters on Oct 08 at  7:48 am:
> On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > Previously, the CLI would print a deprecation warning if a client
> > requested any format version other than the current one.  However, if
> > we add fields that are backwards-compatible, but want clients to be
> > able to depend on, we need to bump the version, but that doesn't make
> > the older version deprecated.
> >
> > Hence, separate out the "minimum active" version and only print a
> > warning for requests below this version number.
> > ---
> >  notmuch-client.h | 5 +++++
> >  notmuch.c        | 2 +-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index afb0ddf..8d986f4 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -142,6 +142,11 @@ chomp_newline (char *str)
> >  /* The minimum supported structured output format version.  Requests
> >   * for format versions below this will return an error. */
> >  #define NOTMUCH_FORMAT_MIN 1
> > +/* The minimum non-deprecated structured output format version.
> > + * Requests for format versions below this will print a stern warning.
> > + * Must be >= NOTMUCH_FORMAT_MIN and < NOTMUCH_FORMAT_CUR.
> > + */
> 
> Should this be <= NOTMUCH_FORMAT_CUR ?

Yes.

> Best wishes
> 
> Mark

[PATCH 03/11] lib: Document extent of some return values

[Austin Clements]

This documents the extent of the notmuch_messages_t* pointers returned
by notmuch_thread_get_toplevel_messages and
notmuch_thread_get_messages.
---
 lib/notmuch.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 998a4ae..217bf46 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -746,12 +746,16 @@ notmuch_thread_get_total_messages (notmuch_thread_t *thread);
  * This iterator will not necessarily iterate over all of the messages
  * in the thread. It will only iterate over the messages in the thread
  * which are not replies to other messages in the thread.
+ *
+ * The returned list will be destroyed when the thread is destroyed.
  */
 notmuch_messages_t *
 notmuch_thread_get_toplevel_messages (notmuch_thread_t *thread);
 
 /* Get a notmuch_thread_t iterator for all messages in 'thread' in
  * oldest-first order.
+ *
+ * The returned list will be destroyed when the thread is destroyed.
  */
 notmuch_messages_t *
 notmuch_thread_get_messages (notmuch_thread_t *thread);
-- 
1.8.4.rc3

[PATCH 04/11] test: Fix missing erase-buffer in emacs test

[Austin Clements]

The first subprocess error exit code test assumed the *Notmuch errors*
buffer would be empty.  Rather than assuming, make it so.
---
 test/emacs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/emacs b/test/emacs
index 5bc3efc..05295af 100755
--- a/test/emacs
+++ b/test/emacs
@@ -864,6 +864,8 @@ EOF
 chmod a+x notmuch_fail
 test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
 	       (with-current-buffer \"*Messages*\" (erase-buffer))
+	       (with-current-buffer (get-buffer-create \"*Notmuch errors*\")
+                 (erase-buffer))
 	       (notmuch-search \"tag:inbox\")
 	       (notmuch-test-wait)
 	       (with-current-buffer \"*Messages*\"
@@ -893,7 +895,8 @@ EOF
 chmod a+x notmuch_fail
 test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
 	       (with-current-buffer \"*Messages*\" (erase-buffer))
-	       (with-current-buffer \"*Notmuch errors*\" (erase-buffer))
+	       (with-current-buffer (get-buffer-create \"*Notmuch errors*\")
+                 (erase-buffer))
 	       (notmuch-search \"tag:inbox\")
 	       (notmuch-test-wait)
 	       (with-current-buffer \"*Messages*\"
-- 
1.8.4.rc3

[PATCH 05/11] emacs: Move `notmuch-call-notmuch-process' to notmuch-lib

[Austin Clements]

Previously, this was in notmuch.el, but all of the other notmuch call
wrappers were in notmuch-lib.el.  Move `notmuch-call-notmuch-process'
to live with its friends.  This happens to fix a missing dependency
from notmuch-tag.el, which required notmuch-lib, but not notmuch.
---
 emacs/notmuch-lib.el | 11 +++++++++++
 emacs/notmuch.el     | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 58f3313..4e0604e 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -534,6 +534,17 @@ You may need to restart Emacs or upgrade your notmuch package."))
 	;; `notmuch-logged-error' does not return.
 	))))
 
+(defun notmuch-call-notmuch-process (&rest args)
+  "Synchronously invoke \"notmuch\" with the given list of arguments.
+
+If notmuch exits with a non-zero status, output from the process
+will appear in a buffer named \"*Notmuch errors*\" and an error
+will be signaled."
+  (with-temp-buffer
+    (let ((status (apply #'call-process notmuch-command nil t nil args)))
+      (notmuch-check-exit-status status (cons notmuch-command args)
+				 (buffer-string)))))
+
 (defun notmuch-call-notmuch-sexp (&rest args)
   "Invoke `notmuch-command' with ARGS and return the parsed S-exp output.
 
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 0ff248b..2c7678d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -511,17 +511,6 @@ If BARE is set then do not prefix with \"thread:\""
   (let ((message-id (notmuch-search-find-thread-id)))
     (notmuch-mua-new-reply message-id prompt-for-sender nil)))
 
-(defun notmuch-call-notmuch-process (&rest args)
-  "Synchronously invoke \"notmuch\" with the given list of arguments.
-
-If notmuch exits with a non-zero status, output from the process
-will appear in a buffer named \"*Notmuch errors*\" and an error
-will be signaled."
-  (with-temp-buffer
-    (let ((status (apply #'call-process notmuch-command nil t nil args)))
-      (notmuch-check-exit-status status (cons notmuch-command args)
-				 (buffer-string)))))
-
 (defun notmuch-search-set-tags (tags &optional pos)
   (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
     (notmuch-search-update-result new-result pos)))
-- 
1.8.4.rc3

[PATCH 06/11] emacs: Support passing input via `notmuch-call-notmuch-*'

[Austin Clements]

This adds support for passing a string to write to notmuch's stdin to
`notmuch-call-notmuch-process' and `notmuch-call-notmuch-sexp'.  Since
this makes both interfaces a little more complicated, it also unifies
their documentation better.
---
 emacs/notmuch-lib.el | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 4e0604e..22156f1 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -534,28 +534,55 @@ You may need to restart Emacs or upgrade your notmuch package."))
 	;; `notmuch-logged-error' does not return.
 	))))
 
+(defun notmuch-call-notmuch--helper (destination args)
+  "Helper for synchronous notmuch invocation commands.
+
+This wraps `call-process'.  DESTINATION has the same meaning as
+for `call-process'.  ARGS is as described for
+`notmuch-call-notmuch-process'."
+
+  (let (stdin-string)
+    (while (keywordp (car args))
+      (case (car args)
+	(:stdin-string (setq stdin-string (cadr args)
+			     args (cddr args)))
+	(otherwise
+	 (error "Unknown keyword argument: %s" (car args)))))
+    (if (null stdin-string)
+	(apply #'call-process notmuch-command nil destination nil args)
+      (insert stdin-string)
+      (apply #'call-process-region (point-min) (point-max)
+	     notmuch-command t destination nil args))))
+
 (defun notmuch-call-notmuch-process (&rest args)
-  "Synchronously invoke \"notmuch\" with the given list of arguments.
+  "Synchronously invoke `notmuch-command' with ARGS.
+
+The caller may provide keyword arguments before ARGS.  Currently
+supported keyword arguments are:
+
+  :stdin-string STRING - Write STRING to stdin
 
 If notmuch exits with a non-zero status, output from the process
 will appear in a buffer named \"*Notmuch errors*\" and an error
 will be signaled."
   (with-temp-buffer
-    (let ((status (apply #'call-process notmuch-command nil t nil args)))
+    (let ((status (notmuch-call-notmuch--helper t args)))
       (notmuch-check-exit-status status (cons notmuch-command args)
 				 (buffer-string)))))
 
 (defun notmuch-call-notmuch-sexp (&rest args)
   "Invoke `notmuch-command' with ARGS and return the parsed S-exp output.
 
-If notmuch exits with a non-zero status, this will pop up a
-buffer containing notmuch's output and signal an error."
+This is equivalent to `notmuch-call-notmuch-process', but parses
+notmuch's output as an S-expression and returns the parsed value.
+Like `notmuch-call-notmuch-process', if notmuch exits with a
+non-zero status, this will report its output and signal an
+error."
 
   (with-temp-buffer
     (let ((err-file (make-temp-file "nmerr")))
       (unwind-protect
-	  (let ((status (apply #'call-process
-			       notmuch-command nil (list t err-file) nil args)))
+	  (let ((status (notmuch-call-notmuch--helper (list t err-file) args)))
 	    (notmuch-check-exit-status status (cons notmuch-command args)
 				       (buffer-string) err-file)
 	    (goto-char (point-min))
-- 
1.8.4.rc3

[PATCH 07/11] emacs: Use notmuch tag --batch for large tag queries

[Austin Clements]

(Unfortunately, it's difficult to first demonstrate this problem with
a known-broken test because modern Linux kernels have argument length
limits in the megabytes, which makes Emacs really slow!)
---
 emacs/notmuch-lib.el |  8 ++++++++
 emacs/notmuch-tag.el | 12 ++++++++++--
 test/emacs           |  8 ++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 22156f1..348112b 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -261,6 +261,14 @@ user-friendly queries."
   "Return a query that matches the message with id ID."
   (concat "id:" (notmuch-escape-boolean-term id)))
 
+(defun notmuch-hex-encode (str)
+  "Hex-encode STR (e.g., as used by batch tagging).
+
+This replaces spaces, percents, and double quotes in STR with
+%NN where NN is the hexadecimal value of the character."
+  (replace-regexp-in-string
+   "[ %\"]" (lambda (match) (format "%%%02x" (aref match 0))) str))
+
 ;;
 
 (defun notmuch-common-do-stash (text)
diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 064cfa8..a4eec14 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -242,6 +242,8 @@ from TAGS if present."
 	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
     (sort result-tags 'string<)))
 
+(defconst notmuch-tag-argument-limit 1000)
+
 (defun notmuch-tag (query &optional tag-changes)
   "Add/remove tags in TAG-CHANGES to messages matching QUERY.
 
@@ -268,8 +270,14 @@ notmuch-after-tag-hook will be run."
 	tag-changes)
   (unless (null tag-changes)
     (run-hooks 'notmuch-before-tag-hook)
-    (apply 'notmuch-call-notmuch-process "tag"
-	   (append tag-changes (list "--" query)))
+    (if (<= (length query) notmuch-tag-argument-limit)
+	(apply 'notmuch-call-notmuch-process "tag"
+	       (append tag-changes (list "--" query)))
+      ;; Use batch tag mode to avoid argument length limitations
+      (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
+			      " -- " query)))
+	(message "Batch tagging with %s" batch-op)
+	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
     (run-hooks 'notmuch-after-tag-hook))
   ;; in all cases we return tag-changes as a list
   tag-changes)
diff --git a/test/emacs b/test/emacs
index 05295af..2917189 100755
--- a/test/emacs
+++ b/test/emacs
@@ -122,6 +122,14 @@ test_emacs "(notmuch-search \"$os_x_darwin_thread\")
 output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)"
 
+test_begin_subtest "Add tag (large query)"
+# We use a long query to force us into batch mode and use a funny tag
+# that requires escaping for batch tagging.
+test_emacs "(notmuch-tag (concat \"$os_x_darwin_thread\" \" or \" (make-string notmuch-tag-argument-limit ?x)) \"+tag-from-%-large-query\")"
+output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
+test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox tag-from-%-large-query unread)"
+notmuch tag -tag-from-%-large-query $os_x_darwin_thread
+
 test_begin_subtest "notmuch-show: add single tag to single message"
 test_emacs "(notmuch-show \"$os_x_darwin_thread\")
 	    (execute-kbd-macro \"+tag-from-show-view\")"
-- 
1.8.4.rc3

Re: [PATCH 07/11] emacs: Use notmuch tag --batch for large tag queries

[Mark Walters]

On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> (Unfortunately, it's difficult to first demonstrate this problem with
> a known-broken test because modern Linux kernels have argument length
> limits in the megabytes, which makes Emacs really slow!)
> ---
>  emacs/notmuch-lib.el |  8 ++++++++
>  emacs/notmuch-tag.el | 12 ++++++++++--
>  test/emacs           |  8 ++++++++
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 22156f1..348112b 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -261,6 +261,14 @@ user-friendly queries."
>    "Return a query that matches the message with id ID."
>    (concat "id:" (notmuch-escape-boolean-term id)))
>  
> +(defun notmuch-hex-encode (str)
> +  "Hex-encode STR (e.g., as used by batch tagging).
> +
> +This replaces spaces, percents, and double quotes in STR with
> +%NN where NN is the hexadecimal value of the character."
> +  (replace-regexp-in-string
> +   "[ %\"]" (lambda (match) (format "%%%02x" (aref match 0))) str))
> +
>  ;;
>  
>  (defun notmuch-common-do-stash (text)
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 064cfa8..a4eec14 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -242,6 +242,8 @@ from TAGS if present."
>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
>      (sort result-tags 'string<)))
>  
> +(defconst notmuch-tag-argument-limit 1000)
> +

Another triviality: I think this should have a doc string saying use
batch tag if the query is longer than this.

The other patches in the series up to this point LGTM.

Mark


>  (defun notmuch-tag (query &optional tag-changes)
>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
>  
> @@ -268,8 +270,14 @@ notmuch-after-tag-hook will be run."
>  	tag-changes)
>    (unless (null tag-changes)
>      (run-hooks 'notmuch-before-tag-hook)
> -    (apply 'notmuch-call-notmuch-process "tag"
> -	   (append tag-changes (list "--" query)))
> +    (if (<= (length query) notmuch-tag-argument-limit)
> +	(apply 'notmuch-call-notmuch-process "tag"
> +	       (append tag-changes (list "--" query)))
> +      ;; Use batch tag mode to avoid argument length limitations
> +      (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
> +			      " -- " query)))
> +	(message "Batch tagging with %s" batch-op)
> +	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
>      (run-hooks 'notmuch-after-tag-hook))
>    ;; in all cases we return tag-changes as a list
>    tag-changes)
> diff --git a/test/emacs b/test/emacs
> index 05295af..2917189 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -122,6 +122,14 @@ test_emacs "(notmuch-search \"$os_x_darwin_thread\")
>  output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
>  test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)"
>  
> +test_begin_subtest "Add tag (large query)"
> +# We use a long query to force us into batch mode and use a funny tag
> +# that requires escaping for batch tagging.
> +test_emacs "(notmuch-tag (concat \"$os_x_darwin_thread\" \" or \" (make-string notmuch-tag-argument-limit ?x)) \"+tag-from-%-large-query\")"
> +output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
> +test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox tag-from-%-large-query unread)"
> +notmuch tag -tag-from-%-large-query $os_x_darwin_thread
> +
>  test_begin_subtest "notmuch-show: add single tag to single message"
>  test_emacs "(notmuch-show \"$os_x_darwin_thread\")
>  	    (execute-kbd-macro \"+tag-from-show-view\")"
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 07/11] emacs: Use notmuch tag --batch for large tag queries

[Austin Clements]

Quoth Mark Walters on Oct 08 at  8:27 am:
> On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > (Unfortunately, it's difficult to first demonstrate this problem with
> > a known-broken test because modern Linux kernels have argument length
> > limits in the megabytes, which makes Emacs really slow!)
> > ---
> >  emacs/notmuch-lib.el |  8 ++++++++
> >  emacs/notmuch-tag.el | 12 ++++++++++--
> >  test/emacs           |  8 ++++++++
> >  3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> > index 22156f1..348112b 100644
> > --- a/emacs/notmuch-lib.el
> > +++ b/emacs/notmuch-lib.el
> > @@ -261,6 +261,14 @@ user-friendly queries."
> >    "Return a query that matches the message with id ID."
> >    (concat "id:" (notmuch-escape-boolean-term id)))
> >  
> > +(defun notmuch-hex-encode (str)
> > +  "Hex-encode STR (e.g., as used by batch tagging).
> > +
> > +This replaces spaces, percents, and double quotes in STR with
> > +%NN where NN is the hexadecimal value of the character."
> > +  (replace-regexp-in-string
> > +   "[ %\"]" (lambda (match) (format "%%%02x" (aref match 0))) str))
> > +
> >  ;;
> >  
> >  (defun notmuch-common-do-stash (text)
> > diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> > index 064cfa8..a4eec14 100644
> > --- a/emacs/notmuch-tag.el
> > +++ b/emacs/notmuch-tag.el
> > @@ -242,6 +242,8 @@ from TAGS if present."
> >  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
> >      (sort result-tags 'string<)))
> >  
> > +(defconst notmuch-tag-argument-limit 1000)
> > +
> 
> Another triviality: I think this should have a doc string saying use
> batch tag if the query is longer than this.

Done.

> The other patches in the series up to this point LGTM.
> 
> Mark

Re: [PATCH 07/11] emacs: Use notmuch tag --batch for large tag queries

[Jani Nikula]

On Tue, 08 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> (Unfortunately, it's difficult to first demonstrate this problem with
> a known-broken test because modern Linux kernels have argument length
> limits in the megabytes, which makes Emacs really slow!)
> ---
>  emacs/notmuch-lib.el |  8 ++++++++
>  emacs/notmuch-tag.el | 12 ++++++++++--
>  test/emacs           |  8 ++++++++
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 22156f1..348112b 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -261,6 +261,14 @@ user-friendly queries."
>    "Return a query that matches the message with id ID."
>    (concat "id:" (notmuch-escape-boolean-term id)))
>  
> +(defun notmuch-hex-encode (str)
> +  "Hex-encode STR (e.g., as used by batch tagging).
> +
> +This replaces spaces, percents, and double quotes in STR with
> +%NN where NN is the hexadecimal value of the character."
> +  (replace-regexp-in-string
> +   "[ %\"]" (lambda (match) (format "%%%02x" (aref match 0))) str))
> +
>  ;;
>  
>  (defun notmuch-common-do-stash (text)
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 064cfa8..a4eec14 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -242,6 +242,8 @@ from TAGS if present."
>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
>      (sort result-tags 'string<)))
>  
> +(defconst notmuch-tag-argument-limit 1000)
> +
>  (defun notmuch-tag (query &optional tag-changes)
>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
>  
> @@ -268,8 +270,14 @@ notmuch-after-tag-hook will be run."
>  	tag-changes)
>    (unless (null tag-changes)
>      (run-hooks 'notmuch-before-tag-hook)
> -    (apply 'notmuch-call-notmuch-process "tag"
> -	   (append tag-changes (list "--" query)))
> +    (if (<= (length query) notmuch-tag-argument-limit)
> +	(apply 'notmuch-call-notmuch-process "tag"
> +	       (append tag-changes (list "--" query)))
> +      ;; Use batch tag mode to avoid argument length limitations
> +      (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
> +			      " -- " query)))
> +	(message "Batch tagging with %s" batch-op)

Why the message?

Jani.

> +	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
>      (run-hooks 'notmuch-after-tag-hook))
>    ;; in all cases we return tag-changes as a list
>    tag-changes)
> diff --git a/test/emacs b/test/emacs
> index 05295af..2917189 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -122,6 +122,14 @@ test_emacs "(notmuch-search \"$os_x_darwin_thread\")
>  output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
>  test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)"
>  
> +test_begin_subtest "Add tag (large query)"
> +# We use a long query to force us into batch mode and use a funny tag
> +# that requires escaping for batch tagging.
> +test_emacs "(notmuch-tag (concat \"$os_x_darwin_thread\" \" or \" (make-string notmuch-tag-argument-limit ?x)) \"+tag-from-%-large-query\")"
> +output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
> +test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox tag-from-%-large-query unread)"
> +notmuch tag -tag-from-%-large-query $os_x_darwin_thread
> +
>  test_begin_subtest "notmuch-show: add single tag to single message"
>  test_emacs "(notmuch-show \"$os_x_darwin_thread\")
>  	    (execute-kbd-macro \"+tag-from-show-view\")"
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 07/11] emacs: Use notmuch tag --batch for large tag queries

[Mark Walters]

On Wed, 09 Oct 2013, Jani Nikula <jani@nikula.org> wrote:
> On Tue, 08 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> (Unfortunately, it's difficult to first demonstrate this problem with
>> a known-broken test because modern Linux kernels have argument length
>> limits in the megabytes, which makes Emacs really slow!)
>> ---
>>  emacs/notmuch-lib.el |  8 ++++++++
>>  emacs/notmuch-tag.el | 12 ++++++++++--
>>  test/emacs           |  8 ++++++++
>>  3 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> index 22156f1..348112b 100644
>> --- a/emacs/notmuch-lib.el
>> +++ b/emacs/notmuch-lib.el
>> @@ -261,6 +261,14 @@ user-friendly queries."
>>    "Return a query that matches the message with id ID."
>>    (concat "id:" (notmuch-escape-boolean-term id)))
>>  
>> +(defun notmuch-hex-encode (str)
>> +  "Hex-encode STR (e.g., as used by batch tagging).
>> +
>> +This replaces spaces, percents, and double quotes in STR with
>> +%NN where NN is the hexadecimal value of the character."
>> +  (replace-regexp-in-string
>> +   "[ %\"]" (lambda (match) (format "%%%02x" (aref match 0))) str))
>> +
>>  ;;
>>  
>>  (defun notmuch-common-do-stash (text)
>> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
>> index 064cfa8..a4eec14 100644
>> --- a/emacs/notmuch-tag.el
>> +++ b/emacs/notmuch-tag.el
>> @@ -242,6 +242,8 @@ from TAGS if present."
>>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
>>      (sort result-tags 'string<)))
>>  
>> +(defconst notmuch-tag-argument-limit 1000)
>> +
>>  (defun notmuch-tag (query &optional tag-changes)
>>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
>>  
>> @@ -268,8 +270,14 @@ notmuch-after-tag-hook will be run."
>>  	tag-changes)
>>    (unless (null tag-changes)
>>      (run-hooks 'notmuch-before-tag-hook)
>> -    (apply 'notmuch-call-notmuch-process "tag"
>> -	   (append tag-changes (list "--" query)))
>> +    (if (<= (length query) notmuch-tag-argument-limit)
>> +	(apply 'notmuch-call-notmuch-process "tag"
>> +	       (append tag-changes (list "--" query)))
>> +      ;; Use batch tag mode to avoid argument length limitations
>> +      (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
>> +			      " -- " query)))
>> +	(message "Batch tagging with %s" batch-op)
>
> Why the message?
>
> Jani.

I had missed that: this message is presumably for debugging as it
includes the (possibly megabyte sized) query. I think some message would
be sensible as this could be a slow operation so it's probably worth
telling the user why the interface has locked.

Best wishes

Mark

>
>> +	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
>>      (run-hooks 'notmuch-after-tag-hook))
>>    ;; in all cases we return tag-changes as a list
>>    tag-changes)
>> diff --git a/test/emacs b/test/emacs
>> index 05295af..2917189 100755
>> --- a/test/emacs
>> +++ b/test/emacs
>> @@ -122,6 +122,14 @@ test_emacs "(notmuch-search \"$os_x_darwin_thread\")
>>  output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
>>  test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)"
>>  
>> +test_begin_subtest "Add tag (large query)"
>> +# We use a long query to force us into batch mode and use a funny tag
>> +# that requires escaping for batch tagging.
>> +test_emacs "(notmuch-tag (concat \"$os_x_darwin_thread\" \" or \" (make-string notmuch-tag-argument-limit ?x)) \"+tag-from-%-large-query\")"
>> +output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
>> +test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox tag-from-%-large-query unread)"
>> +notmuch tag -tag-from-%-large-query $os_x_darwin_thread
>> +
>>  test_begin_subtest "notmuch-show: add single tag to single message"
>>  test_emacs "(notmuch-show \"$os_x_darwin_thread\")
>>  	    (execute-kbd-macro \"+tag-from-show-view\")"
>> -- 
>> 1.8.4.rc3
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 07/11] emacs: Use notmuch tag --batch for large tag queries

[Austin Clements]

Quoth Mark Walters on Oct 09 at  8:38 am:
> 
> On Wed, 09 Oct 2013, Jani Nikula <jani@nikula.org> wrote:
> > On Tue, 08 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> >> (Unfortunately, it's difficult to first demonstrate this problem with
> >> a known-broken test because modern Linux kernels have argument length
> >> limits in the megabytes, which makes Emacs really slow!)
> >> ---
> >>  emacs/notmuch-lib.el |  8 ++++++++
> >>  emacs/notmuch-tag.el | 12 ++++++++++--
> >>  test/emacs           |  8 ++++++++
> >>  3 files changed, 26 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> >> index 22156f1..348112b 100644
> >> --- a/emacs/notmuch-lib.el
> >> +++ b/emacs/notmuch-lib.el
> >> @@ -261,6 +261,14 @@ user-friendly queries."
> >>    "Return a query that matches the message with id ID."
> >>    (concat "id:" (notmuch-escape-boolean-term id)))
> >>  
> >> +(defun notmuch-hex-encode (str)
> >> +  "Hex-encode STR (e.g., as used by batch tagging).
> >> +
> >> +This replaces spaces, percents, and double quotes in STR with
> >> +%NN where NN is the hexadecimal value of the character."
> >> +  (replace-regexp-in-string
> >> +   "[ %\"]" (lambda (match) (format "%%%02x" (aref match 0))) str))
> >> +
> >>  ;;
> >>  
> >>  (defun notmuch-common-do-stash (text)
> >> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> >> index 064cfa8..a4eec14 100644
> >> --- a/emacs/notmuch-tag.el
> >> +++ b/emacs/notmuch-tag.el
> >> @@ -242,6 +242,8 @@ from TAGS if present."
> >>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
> >>      (sort result-tags 'string<)))
> >>  
> >> +(defconst notmuch-tag-argument-limit 1000)
> >> +
> >>  (defun notmuch-tag (query &optional tag-changes)
> >>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
> >>  
> >> @@ -268,8 +270,14 @@ notmuch-after-tag-hook will be run."
> >>  	tag-changes)
> >>    (unless (null tag-changes)
> >>      (run-hooks 'notmuch-before-tag-hook)
> >> -    (apply 'notmuch-call-notmuch-process "tag"
> >> -	   (append tag-changes (list "--" query)))
> >> +    (if (<= (length query) notmuch-tag-argument-limit)
> >> +	(apply 'notmuch-call-notmuch-process "tag"
> >> +	       (append tag-changes (list "--" query)))
> >> +      ;; Use batch tag mode to avoid argument length limitations
> >> +      (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
> >> +			      " -- " query)))
> >> +	(message "Batch tagging with %s" batch-op)
> >
> > Why the message?
> >
> > Jani.
> 
> I had missed that: this message is presumably for debugging as it
> includes the (possibly megabyte sized) query. I think some message would
> be sensible as this could be a slow operation so it's probably worth
> telling the user why the interface has locked.

You're right, this was a leftover debug message.  While I wouldn't
mind a progress message for long tagging operations, query length
isn't a good approximation of how long it's going to take.  I was
planning to include messages on tagging operations in my undo series,
which I think can do it in a more principled way (assuming I can find
a clean way to update tags in the UI on undo).

> Best wishes
> 
> Mark

[PATCH 08/11] search: Add stable queries to thread search results

[Austin Clements]

These queries will match exactly the set of messages currently in the
thread, even if more messages later arrive.  Two queries are provided:
one for matched messages and one for unmatched messages.

This can be used to fix race conditions with tagging threads from
search results.  While tagging based on a thread: query can affect
messages that arrived after the search, tagging based on stable
queries affects only the messages the user was shown in the search UI.

Since we want clients to be able to depend on the presence of these
queries, this ushers in schema version 2.
---
 devel/schemata       | 22 +++++++++++++++++--
 notmuch-client.h     |  2 +-
 notmuch-search.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/json            |  2 ++
 test/missing-headers |  6 ++++--
 test/sexp            |  4 ++--
 6 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/devel/schemata b/devel/schemata
index cdd0e43..41dc4a6 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -14,7 +14,17 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
 colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
 nil, true as t and false as nil.
 
-This is version 1 of the structured output format.
+This is version 2 of the structured output format.
+
+Version history
+---------------
+
+v1
+- First versioned schema release.
+- Added part.content-length and part.content-transfer-encoding fields.
+
+v2
+- Added the thread_summary.query field.
 
 Common non-terminals
 --------------------
@@ -145,7 +155,15 @@ thread_summary = {
     authors:        string,   # comma-separated names with | between
                               # matched and unmatched
     subject:        string,
-    tags:           [string*]
+    tags:           [string*],
+
+    # Two stable query strings identifying exactly the matched and
+    # unmatched messages currently in this thread.  The messages
+    # matched by these queries will not change even if more messages
+    # arrive in the thread.  If there are no matched or unmatched
+    # messages, the corresponding query will be null (there is no
+    # query that matches nothing).  (Added in schema version 2.)
+    query:          [string|null, string|null],
 }
 
 notmuch reply schema
diff --git a/notmuch-client.h b/notmuch-client.h
index 8d986f4..1b14910 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -138,7 +138,7 @@ chomp_newline (char *str)
  * this.  New (required) map fields can be added without increasing
  * this.
  */
-#define NOTMUCH_FORMAT_CUR 1
+#define NOTMUCH_FORMAT_CUR 2
 /* The minimum supported structured output format version.  Requests
  * for format versions below this will return an error. */
 #define NOTMUCH_FORMAT_MIN 1
diff --git a/notmuch-search.c b/notmuch-search.c
index d9d39ec..1d14651 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -20,6 +20,7 @@
 
 #include "notmuch-client.h"
 #include "sprinter.h"
+#include "string-util.h"
 
 typedef enum {
     OUTPUT_SUMMARY,
@@ -46,6 +47,46 @@ sanitize_string (const void *ctx, const char *str)
     return out;
 }
 
+/* Return two stable query strings that identify exactly the matched
+ * and unmatched messages currently in thread.  If there are no
+ * matched or unmatched messages, the returned buffers will be
+ * NULL. */
+static int
+get_thread_query (notmuch_thread_t *thread,
+		  char **matched_out, char **unmached_out)
+{
+    notmuch_messages_t *messages;
+    char *escaped = NULL;
+    size_t escaped_len = 0;
+
+    *matched_out = *unmached_out = NULL;
+
+    for (messages = notmuch_thread_get_messages (thread);
+	 notmuch_messages_valid (messages);
+	 notmuch_messages_move_to_next (messages))
+    {
+	notmuch_message_t *message = notmuch_messages_get (messages);
+	const char *mid = notmuch_message_get_message_id (message);
+	/* Determine which query buffer to extend */
+	char **buf = notmuch_message_get_flag (
+	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmached_out;
+	/* Allocate the query buffer is this is the first message */
+	if (!*buf && (*buf = talloc_strdup (thread, "")) == NULL)
+	    return -1;
+	/* Add this message's id: query.  Since "id" is an exclusive
+	 * prefix, it is implicitly 'or'd together, so we only need to
+	 * join queries with a space. */
+	if (make_boolean_term (thread, "id", mid, &escaped, &escaped_len) < 0)
+	    return -1;
+	*buf = talloc_asprintf_append_buffer (
+	    *buf, "%s%s", **buf ? " " : "", escaped);
+	if (!*buf)
+	    return -1;
+    }
+    talloc_free (escaped);
+    return 0;
+}
+
 static int
 do_search_threads (sprinter_t *format,
 		   notmuch_query_t *query,
@@ -131,6 +172,25 @@ do_search_threads (sprinter_t *format,
 		format->string (format, authors);
 		format->map_key (format, "subject");
 		format->string (format, subject);
+		if (notmuch_format_version >= 2) {
+		    char *matched_query, *unmatched_query;
+		    if (get_thread_query (thread, &matched_query,
+					  &unmatched_query) < 0) {
+			fprintf (stderr, "Out of memory\n");
+			return 1;
+		    }
+		    format->map_key (format, "query");
+		    format->begin_list (format);
+		    if (matched_query)
+			format->string (format, matched_query);
+		    else
+			format->null (format);
+		    if (unmatched_query)
+			format->string (format, unmatched_query);
+		    else
+			format->null (format);
+		    format->end (format);
+		}
 	    }
 
 	    talloc_free (ctx_quote);
diff --git a/test/json b/test/json
index b87b7f6..e07a290 100755
--- a/test/json
+++ b/test/json
@@ -26,6 +26,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
  \"total\": 1,
  \"authors\": \"Notmuch Test Suite\",
  \"subject\": \"json-search-subject\",
+ \"query\": [\"id:$gen_msg_id\", null],
  \"tags\": [\"inbox\",
  \"unread\"]}]"
 
@@ -59,6 +60,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
  \"total\": 1,
  \"authors\": \"Notmuch Test Suite\",
  \"subject\": \"json-search-utf8-body-sübjéct\",
+ \"query\": [\"id:$gen_msg_id\", null],
  \"tags\": [\"inbox\",
  \"unread\"]}]"
 
diff --git a/test/missing-headers b/test/missing-headers
index f14b878..43e861b 100755
--- a/test/missing-headers
+++ b/test/missing-headers
@@ -43,7 +43,8 @@ test_expect_equal_json "$output" '
         ],
         "thread": "XXX",
         "timestamp": 978709437,
-        "total": 1
+        "total": 1,
+        "query": ["id:notmuch-sha1-7a6e4eac383ef958fcd3ebf2143db71b8ff01161", null]
     },
     {
         "authors": "Notmuch Test Suite",
@@ -56,7 +57,8 @@ test_expect_equal_json "$output" '
         ],
         "thread": "XXX",
         "timestamp": 0,
-        "total": 1
+        "total": 1,
+        "query": ["id:notmuch-sha1-ca55943aff7a72baf2ab21fa74fab3d632401334", null]
     }
 ]'
 
diff --git a/test/sexp b/test/sexp
index 492a82f..be815e1 100755
--- a/test/sexp
+++ b/test/sexp
@@ -19,7 +19,7 @@ test_expect_equal "$output" "((((:id \"${gen_msg_id}\" :match t :excluded nil :f
 test_begin_subtest "Search message: sexp"
 add_message "[subject]=\"sexp-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"sexp-search-message\""
 output=$(notmuch search --format=sexp "sexp-search-message" | notmuch_search_sanitize)
-test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :tags (\"inbox\" \"unread\")))"
+test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
 
 test_begin_subtest "Show message: sexp, utf-8"
 add_message "[subject]=\"sexp-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\""
@@ -44,7 +44,7 @@ test_expect_equal "$output" "((((:id \"$id\" :match t :excluded nil :filename \"
 test_begin_subtest "Search message: sexp, utf-8"
 add_message "[subject]=\"sexp-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\""
 output=$(notmuch search --format=sexp "jsön-search-méssage" | notmuch_search_sanitize)
-test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :tags (\"inbox\" \"unread\")))"
+test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
 
 
 test_done
-- 
1.8.4.rc3

Re: [PATCH 08/11] search: Add stable queries to thread search results

[Mark Walters]

On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> These queries will match exactly the set of messages currently in the
> thread, even if more messages later arrive.  Two queries are provided:
> one for matched messages and one for unmatched messages.
>
> This can be used to fix race conditions with tagging threads from
> search results.  While tagging based on a thread: query can affect
> messages that arrived after the search, tagging based on stable
> queries affects only the messages the user was shown in the search UI.
>
> Since we want clients to be able to depend on the presence of these
> queries, this ushers in schema version 2.
> ---
>  devel/schemata       | 22 +++++++++++++++++--
>  notmuch-client.h     |  2 +-
>  notmuch-search.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  test/json            |  2 ++
>  test/missing-headers |  6 ++++--
>  test/sexp            |  4 ++--
>  6 files changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/devel/schemata b/devel/schemata
> index cdd0e43..41dc4a6 100644
> --- a/devel/schemata
> +++ b/devel/schemata
> @@ -14,7 +14,17 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
>  colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
>  nil, true as t and false as nil.
>  
> -This is version 1 of the structured output format.
> +This is version 2 of the structured output format.
> +
> +Version history
> +---------------
> +
> +v1
> +- First versioned schema release.
> +- Added part.content-length and part.content-transfer-encoding fields.
> +
> +v2
> +- Added the thread_summary.query field.
>  
>  Common non-terminals
>  --------------------
> @@ -145,7 +155,15 @@ thread_summary = {
>      authors:        string,   # comma-separated names with | between
>                                # matched and unmatched
>      subject:        string,
> -    tags:           [string*]
> +    tags:           [string*],
> +
> +    # Two stable query strings identifying exactly the matched and
> +    # unmatched messages currently in this thread.  The messages
> +    # matched by these queries will not change even if more messages
> +    # arrive in the thread.  If there are no matched or unmatched
> +    # messages, the corresponding query will be null (there is no
> +    # query that matches nothing).  (Added in schema version 2.)
> +    query:          [string|null, string|null],
>  }
>  
>  notmuch reply schema
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 8d986f4..1b14910 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -138,7 +138,7 @@ chomp_newline (char *str)
>   * this.  New (required) map fields can be added without increasing
>   * this.
>   */
> -#define NOTMUCH_FORMAT_CUR 1
> +#define NOTMUCH_FORMAT_CUR 2
>  /* The minimum supported structured output format version.  Requests
>   * for format versions below this will return an error. */
>  #define NOTMUCH_FORMAT_MIN 1
> diff --git a/notmuch-search.c b/notmuch-search.c
> index d9d39ec..1d14651 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -20,6 +20,7 @@
>  
>  #include "notmuch-client.h"
>  #include "sprinter.h"
> +#include "string-util.h"
>  
>  typedef enum {
>      OUTPUT_SUMMARY,
> @@ -46,6 +47,46 @@ sanitize_string (const void *ctx, const char *str)
>      return out;
>  }
>  
> +/* Return two stable query strings that identify exactly the matched
> + * and unmatched messages currently in thread.  If there are no
> + * matched or unmatched messages, the returned buffers will be
> + * NULL. */
> +static int
> +get_thread_query (notmuch_thread_t *thread,
> +		  char **matched_out, char **unmached_out)
> +{
> +    notmuch_messages_t *messages;
> +    char *escaped = NULL;
> +    size_t escaped_len = 0;
> +
> +    *matched_out = *unmached_out = NULL;

A missing t in unmatched? (and once below where it is used)

> +
> +    for (messages = notmuch_thread_get_messages (thread);
> +	 notmuch_messages_valid (messages);
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	notmuch_message_t *message = notmuch_messages_get (messages);
> +	const char *mid = notmuch_message_get_message_id (message);
> +	/* Determine which query buffer to extend */
> +	char **buf = notmuch_message_get_flag (
> +	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmached_out;
> +	/* Allocate the query buffer is this is the first message */
> +	if (!*buf && (*buf = talloc_strdup (thread, "")) == NULL)
> +	    return -1;
> +	/* Add this message's id: query.  Since "id" is an exclusive
> +	 * prefix, it is implicitly 'or'd together, so we only need to
> +	 * join queries with a space. */
> +	if (make_boolean_term (thread, "id", mid, &escaped, &escaped_len) < 0)
> +	    return -1;
> +	*buf = talloc_asprintf_append_buffer (
> +	    *buf, "%s%s", **buf ? " " : "", escaped);
> +	if (!*buf)
> +	    return -1;
> +    }
> +    talloc_free (escaped);
> +    return 0;
> +}
> +
>  static int
>  do_search_threads (sprinter_t *format,
>  		   notmuch_query_t *query,
> @@ -131,6 +172,25 @@ do_search_threads (sprinter_t *format,
>  		format->string (format, authors);
>  		format->map_key (format, "subject");
>  		format->string (format, subject);
> +		if (notmuch_format_version >= 2) {
> +		    char *matched_query, *unmatched_query;
> +		    if (get_thread_query (thread, &matched_query,
> +					  &unmatched_query) < 0) {
> +			fprintf (stderr, "Out of memory\n");
> +			return 1;
> +		    }
> +		    format->map_key (format, "query");
> +		    format->begin_list (format);
> +		    if (matched_query)
> +			format->string (format, matched_query);
> +		    else
> +			format->null (format);
> +		    if (unmatched_query)
> +			format->string (format, unmatched_query);
> +		    else
> +			format->null (format);
> +		    format->end (format);
> +		}
>  	    }
>  
>  	    talloc_free (ctx_quote);
> diff --git a/test/json b/test/json
> index b87b7f6..e07a290 100755
> --- a/test/json
> +++ b/test/json
> @@ -26,6 +26,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
>   \"total\": 1,
>   \"authors\": \"Notmuch Test Suite\",
>   \"subject\": \"json-search-subject\",
> + \"query\": [\"id:$gen_msg_id\", null],
>   \"tags\": [\"inbox\",
>   \"unread\"]}]"
>  
> @@ -59,6 +60,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
>   \"total\": 1,
>   \"authors\": \"Notmuch Test Suite\",
>   \"subject\": \"json-search-utf8-body-sübjéct\",
> + \"query\": [\"id:$gen_msg_id\", null],
>   \"tags\": [\"inbox\",
>   \"unread\"]}]"
>  
> diff --git a/test/missing-headers b/test/missing-headers
> index f14b878..43e861b 100755
> --- a/test/missing-headers
> +++ b/test/missing-headers
> @@ -43,7 +43,8 @@ test_expect_equal_json "$output" '
>          ],
>          "thread": "XXX",
>          "timestamp": 978709437,
> -        "total": 1
> +        "total": 1,
> +        "query": ["id:notmuch-sha1-7a6e4eac383ef958fcd3ebf2143db71b8ff01161", null]
>      },
>      {
>          "authors": "Notmuch Test Suite",
> @@ -56,7 +57,8 @@ test_expect_equal_json "$output" '
>          ],
>          "thread": "XXX",
>          "timestamp": 0,
> -        "total": 1
> +        "total": 1,
> +        "query": ["id:notmuch-sha1-ca55943aff7a72baf2ab21fa74fab3d632401334", null]
>      }
>  ]'
>  
> diff --git a/test/sexp b/test/sexp
> index 492a82f..be815e1 100755
> --- a/test/sexp
> +++ b/test/sexp
> @@ -19,7 +19,7 @@ test_expect_equal "$output" "((((:id \"${gen_msg_id}\" :match t :excluded nil :f
>  test_begin_subtest "Search message: sexp"
>  add_message "[subject]=\"sexp-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"sexp-search-message\""
>  output=$(notmuch search --format=sexp "sexp-search-message" | notmuch_search_sanitize)
> -test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :tags (\"inbox\" \"unread\")))"
> +test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
>  
>  test_begin_subtest "Show message: sexp, utf-8"
>  add_message "[subject]=\"sexp-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\""
> @@ -44,7 +44,7 @@ test_expect_equal "$output" "((((:id \"$id\" :match t :excluded nil :filename \"
>  test_begin_subtest "Search message: sexp, utf-8"
>  add_message "[subject]=\"sexp-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\""
>  output=$(notmuch search --format=sexp "jsön-search-méssage" | notmuch_search_sanitize)
> -test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :tags (\"inbox\" \"unread\")))"
> +test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"

Would it be worth adding an explicit test when unmatched is non-nil?

Best wishes 

Mark


>  
>  test_done
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 08/11] search: Add stable queries to thread search results

[Jani Nikula]

On Tue, 08 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> These queries will match exactly the set of messages currently in the
> thread, even if more messages later arrive.  Two queries are provided:
> one for matched messages and one for unmatched messages.
>
> This can be used to fix race conditions with tagging threads from
> search results.  While tagging based on a thread: query can affect
> messages that arrived after the search, tagging based on stable
> queries affects only the messages the user was shown in the search UI.
>
> Since we want clients to be able to depend on the presence of these
> queries, this ushers in schema version 2.
> ---
>  devel/schemata       | 22 +++++++++++++++++--
>  notmuch-client.h     |  2 +-
>  notmuch-search.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  test/json            |  2 ++
>  test/missing-headers |  6 ++++--
>  test/sexp            |  4 ++--
>  6 files changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/devel/schemata b/devel/schemata
> index cdd0e43..41dc4a6 100644
> --- a/devel/schemata
> +++ b/devel/schemata
> @@ -14,7 +14,17 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
>  colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
>  nil, true as t and false as nil.
>  
> -This is version 1 of the structured output format.
> +This is version 2 of the structured output format.
> +
> +Version history
> +---------------
> +
> +v1
> +- First versioned schema release.
> +- Added part.content-length and part.content-transfer-encoding fields.
> +
> +v2
> +- Added the thread_summary.query field.
>  
>  Common non-terminals
>  --------------------
> @@ -145,7 +155,15 @@ thread_summary = {
>      authors:        string,   # comma-separated names with | between
>                                # matched and unmatched
>      subject:        string,
> -    tags:           [string*]
> +    tags:           [string*],
> +
> +    # Two stable query strings identifying exactly the matched and
> +    # unmatched messages currently in this thread.  The messages
> +    # matched by these queries will not change even if more messages
> +    # arrive in the thread.  If there are no matched or unmatched
> +    # messages, the corresponding query will be null (there is no
> +    # query that matches nothing).  (Added in schema version 2.)
> +    query:          [string|null, string|null],
>  }
>  
>  notmuch reply schema
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 8d986f4..1b14910 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -138,7 +138,7 @@ chomp_newline (char *str)
>   * this.  New (required) map fields can be added without increasing
>   * this.
>   */
> -#define NOTMUCH_FORMAT_CUR 1
> +#define NOTMUCH_FORMAT_CUR 2
>  /* The minimum supported structured output format version.  Requests
>   * for format versions below this will return an error. */
>  #define NOTMUCH_FORMAT_MIN 1
> diff --git a/notmuch-search.c b/notmuch-search.c
> index d9d39ec..1d14651 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -20,6 +20,7 @@
>  
>  #include "notmuch-client.h"
>  #include "sprinter.h"
> +#include "string-util.h"
>  
>  typedef enum {
>      OUTPUT_SUMMARY,
> @@ -46,6 +47,46 @@ sanitize_string (const void *ctx, const char *str)
>      return out;
>  }
>  
> +/* Return two stable query strings that identify exactly the matched
> + * and unmatched messages currently in thread.  If there are no
> + * matched or unmatched messages, the returned buffers will be
> + * NULL. */
> +static int
> +get_thread_query (notmuch_thread_t *thread,
> +		  char **matched_out, char **unmached_out)
> +{
> +    notmuch_messages_t *messages;
> +    char *escaped = NULL;
> +    size_t escaped_len = 0;
> +
> +    *matched_out = *unmached_out = NULL;
> +
> +    for (messages = notmuch_thread_get_messages (thread);
> +	 notmuch_messages_valid (messages);
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	notmuch_message_t *message = notmuch_messages_get (messages);
> +	const char *mid = notmuch_message_get_message_id (message);
> +	/* Determine which query buffer to extend */
> +	char **buf = notmuch_message_get_flag (
> +	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmached_out;
> +	/* Allocate the query buffer is this is the first message */
> +	if (!*buf && (*buf = talloc_strdup (thread, "")) == NULL)
> +	    return -1;

I think it would improve clarity if you dropped the above...

> +	/* Add this message's id: query.  Since "id" is an exclusive
> +	 * prefix, it is implicitly 'or'd together, so we only need to
> +	 * join queries with a space. */
> +	if (make_boolean_term (thread, "id", mid, &escaped, &escaped_len) < 0)
> +	    return -1;
> +	*buf = talloc_asprintf_append_buffer (
> +	    *buf, "%s%s", **buf ? " " : "", escaped);

...and turned this into:

	if (*buf)
	    *buf = talloc_asprintf_append_buffer (*buf, " %s", escaped);
	else
	    *buf = talloc_strdup (thread, escaped);

Also one talloc less. Which brings me to the main worry:
performance. What's the impact?

BR,
Jani.


> +	if (!*buf)
> +	    return -1;
> +    }
> +    talloc_free (escaped);
> +    return 0;
> +}
> +
>  static int
>  do_search_threads (sprinter_t *format,
>  		   notmuch_query_t *query,
> @@ -131,6 +172,25 @@ do_search_threads (sprinter_t *format,
>  		format->string (format, authors);
>  		format->map_key (format, "subject");
>  		format->string (format, subject);
> +		if (notmuch_format_version >= 2) {
> +		    char *matched_query, *unmatched_query;
> +		    if (get_thread_query (thread, &matched_query,
> +					  &unmatched_query) < 0) {
> +			fprintf (stderr, "Out of memory\n");
> +			return 1;
> +		    }
> +		    format->map_key (format, "query");
> +		    format->begin_list (format);
> +		    if (matched_query)
> +			format->string (format, matched_query);
> +		    else
> +			format->null (format);
> +		    if (unmatched_query)
> +			format->string (format, unmatched_query);
> +		    else
> +			format->null (format);
> +		    format->end (format);
> +		}
>  	    }
>  
>  	    talloc_free (ctx_quote);
> diff --git a/test/json b/test/json
> index b87b7f6..e07a290 100755
> --- a/test/json
> +++ b/test/json
> @@ -26,6 +26,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
>   \"total\": 1,
>   \"authors\": \"Notmuch Test Suite\",
>   \"subject\": \"json-search-subject\",
> + \"query\": [\"id:$gen_msg_id\", null],
>   \"tags\": [\"inbox\",
>   \"unread\"]}]"
>  
> @@ -59,6 +60,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
>   \"total\": 1,
>   \"authors\": \"Notmuch Test Suite\",
>   \"subject\": \"json-search-utf8-body-sübjéct\",
> + \"query\": [\"id:$gen_msg_id\", null],
>   \"tags\": [\"inbox\",
>   \"unread\"]}]"
>  
> diff --git a/test/missing-headers b/test/missing-headers
> index f14b878..43e861b 100755
> --- a/test/missing-headers
> +++ b/test/missing-headers
> @@ -43,7 +43,8 @@ test_expect_equal_json "$output" '
>          ],
>          "thread": "XXX",
>          "timestamp": 978709437,
> -        "total": 1
> +        "total": 1,
> +        "query": ["id:notmuch-sha1-7a6e4eac383ef958fcd3ebf2143db71b8ff01161", null]
>      },
>      {
>          "authors": "Notmuch Test Suite",
> @@ -56,7 +57,8 @@ test_expect_equal_json "$output" '
>          ],
>          "thread": "XXX",
>          "timestamp": 0,
> -        "total": 1
> +        "total": 1,
> +        "query": ["id:notmuch-sha1-ca55943aff7a72baf2ab21fa74fab3d632401334", null]
>      }
>  ]'
>  
> diff --git a/test/sexp b/test/sexp
> index 492a82f..be815e1 100755
> --- a/test/sexp
> +++ b/test/sexp
> @@ -19,7 +19,7 @@ test_expect_equal "$output" "((((:id \"${gen_msg_id}\" :match t :excluded nil :f
>  test_begin_subtest "Search message: sexp"
>  add_message "[subject]=\"sexp-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"sexp-search-message\""
>  output=$(notmuch search --format=sexp "sexp-search-message" | notmuch_search_sanitize)
> -test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :tags (\"inbox\" \"unread\")))"
> +test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
>  
>  test_begin_subtest "Show message: sexp, utf-8"
>  add_message "[subject]=\"sexp-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\""
> @@ -44,7 +44,7 @@ test_expect_equal "$output" "((((:id \"$id\" :match t :excluded nil :filename \"
>  test_begin_subtest "Search message: sexp, utf-8"
>  add_message "[subject]=\"sexp-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\""
>  output=$(notmuch search --format=sexp "jsön-search-méssage" | notmuch_search_sanitize)
> -test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :tags (\"inbox\" \"unread\")))"
> +test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
>  
>  
>  test_done
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 08/11] search: Add stable queries to thread search results

[Austin Clements]

Quoth Jani Nikula on Oct 09 at  9:41 am:
> On Tue, 08 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > These queries will match exactly the set of messages currently in the
> > thread, even if more messages later arrive.  Two queries are provided:
> > one for matched messages and one for unmatched messages.
> >
> > This can be used to fix race conditions with tagging threads from
> > search results.  While tagging based on a thread: query can affect
> > messages that arrived after the search, tagging based on stable
> > queries affects only the messages the user was shown in the search UI.
> >
> > Since we want clients to be able to depend on the presence of these
> > queries, this ushers in schema version 2.
> > ---
> >  devel/schemata       | 22 +++++++++++++++++--
> >  notmuch-client.h     |  2 +-
> >  notmuch-search.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  test/json            |  2 ++
> >  test/missing-headers |  6 ++++--
> >  test/sexp            |  4 ++--
> >  6 files changed, 89 insertions(+), 7 deletions(-)
> >
> > diff --git a/devel/schemata b/devel/schemata
> > index cdd0e43..41dc4a6 100644
> > --- a/devel/schemata
> > +++ b/devel/schemata
> > @@ -14,7 +14,17 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
> >  colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
> >  nil, true as t and false as nil.
> >  
> > -This is version 1 of the structured output format.
> > +This is version 2 of the structured output format.
> > +
> > +Version history
> > +---------------
> > +
> > +v1
> > +- First versioned schema release.
> > +- Added part.content-length and part.content-transfer-encoding fields.
> > +
> > +v2
> > +- Added the thread_summary.query field.
> >  
> >  Common non-terminals
> >  --------------------
> > @@ -145,7 +155,15 @@ thread_summary = {
> >      authors:        string,   # comma-separated names with | between
> >                                # matched and unmatched
> >      subject:        string,
> > -    tags:           [string*]
> > +    tags:           [string*],
> > +
> > +    # Two stable query strings identifying exactly the matched and
> > +    # unmatched messages currently in this thread.  The messages
> > +    # matched by these queries will not change even if more messages
> > +    # arrive in the thread.  If there are no matched or unmatched
> > +    # messages, the corresponding query will be null (there is no
> > +    # query that matches nothing).  (Added in schema version 2.)
> > +    query:          [string|null, string|null],
> >  }
> >  
> >  notmuch reply schema
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index 8d986f4..1b14910 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -138,7 +138,7 @@ chomp_newline (char *str)
> >   * this.  New (required) map fields can be added without increasing
> >   * this.
> >   */
> > -#define NOTMUCH_FORMAT_CUR 1
> > +#define NOTMUCH_FORMAT_CUR 2
> >  /* The minimum supported structured output format version.  Requests
> >   * for format versions below this will return an error. */
> >  #define NOTMUCH_FORMAT_MIN 1
> > diff --git a/notmuch-search.c b/notmuch-search.c
> > index d9d39ec..1d14651 100644
> > --- a/notmuch-search.c
> > +++ b/notmuch-search.c
> > @@ -20,6 +20,7 @@
> >  
> >  #include "notmuch-client.h"
> >  #include "sprinter.h"
> > +#include "string-util.h"
> >  
> >  typedef enum {
> >      OUTPUT_SUMMARY,
> > @@ -46,6 +47,46 @@ sanitize_string (const void *ctx, const char *str)
> >      return out;
> >  }
> >  
> > +/* Return two stable query strings that identify exactly the matched
> > + * and unmatched messages currently in thread.  If there are no
> > + * matched or unmatched messages, the returned buffers will be
> > + * NULL. */
> > +static int
> > +get_thread_query (notmuch_thread_t *thread,
> > +		  char **matched_out, char **unmached_out)
> > +{
> > +    notmuch_messages_t *messages;
> > +    char *escaped = NULL;
> > +    size_t escaped_len = 0;
> > +
> > +    *matched_out = *unmached_out = NULL;
> > +
> > +    for (messages = notmuch_thread_get_messages (thread);
> > +	 notmuch_messages_valid (messages);
> > +	 notmuch_messages_move_to_next (messages))
> > +    {
> > +	notmuch_message_t *message = notmuch_messages_get (messages);
> > +	const char *mid = notmuch_message_get_message_id (message);
> > +	/* Determine which query buffer to extend */
> > +	char **buf = notmuch_message_get_flag (
> > +	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmached_out;
> > +	/* Allocate the query buffer is this is the first message */
> > +	if (!*buf && (*buf = talloc_strdup (thread, "")) == NULL)
> > +	    return -1;
> 
> I think it would improve clarity if you dropped the above...
> 
> > +	/* Add this message's id: query.  Since "id" is an exclusive
> > +	 * prefix, it is implicitly 'or'd together, so we only need to
> > +	 * join queries with a space. */
> > +	if (make_boolean_term (thread, "id", mid, &escaped, &escaped_len) < 0)
> > +	    return -1;
> > +	*buf = talloc_asprintf_append_buffer (
> > +	    *buf, "%s%s", **buf ? " " : "", escaped);
> 
> ...and turned this into:
> 
> 	if (*buf)
> 	    *buf = talloc_asprintf_append_buffer (*buf, " %s", escaped);
> 	else
> 	    *buf = talloc_strdup (thread, escaped);

Much nicer!

> Also one talloc less. Which brings me to the main worry:
> performance. What's the impact?

Seems to be about 1%-3% for CLI search (tested on the medium corpus).
It's hard to measure what the effect on Emacs search is, though I
would expect it to be similarly negligible.  Some work I did several
attempts at this ago suggests that this slows down tagging (though I
doubt it would be noticeable for single threads), but I also found
that switching to docid-based queries significantly sped things up:
id:CAH-f9WsPj=1Eu=g3sOePJgCTBFs6HrLdLq18xMEnJ8aZ00yCEg@mail.gmail.com
Actually, docid queries probably make tagging faster than it is *now*,
but I didn't measure that when I did the experiments.

> BR,
> Jani.
> 
> 
> > +	if (!*buf)
> > +	    return -1;
> > +    }
> > +    talloc_free (escaped);
> > +    return 0;
> > +}
> > +
> >  static int
> >  do_search_threads (sprinter_t *format,
> >  		   notmuch_query_t *query,
> > @@ -131,6 +172,25 @@ do_search_threads (sprinter_t *format,
> >  		format->string (format, authors);
> >  		format->map_key (format, "subject");
> >  		format->string (format, subject);
> > +		if (notmuch_format_version >= 2) {
> > +		    char *matched_query, *unmatched_query;
> > +		    if (get_thread_query (thread, &matched_query,
> > +					  &unmatched_query) < 0) {
> > +			fprintf (stderr, "Out of memory\n");
> > +			return 1;
> > +		    }
> > +		    format->map_key (format, "query");
> > +		    format->begin_list (format);
> > +		    if (matched_query)
> > +			format->string (format, matched_query);
> > +		    else
> > +			format->null (format);
> > +		    if (unmatched_query)
> > +			format->string (format, unmatched_query);
> > +		    else
> > +			format->null (format);
> > +		    format->end (format);
> > +		}
> >  	    }
> >  
> >  	    talloc_free (ctx_quote);
> > diff --git a/test/json b/test/json
> > index b87b7f6..e07a290 100755
> > --- a/test/json
> > +++ b/test/json
> > @@ -26,6 +26,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
> >   \"total\": 1,
> >   \"authors\": \"Notmuch Test Suite\",
> >   \"subject\": \"json-search-subject\",
> > + \"query\": [\"id:$gen_msg_id\", null],
> >   \"tags\": [\"inbox\",
> >   \"unread\"]}]"
> >  
> > @@ -59,6 +60,7 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
> >   \"total\": 1,
> >   \"authors\": \"Notmuch Test Suite\",
> >   \"subject\": \"json-search-utf8-body-sübjéct\",
> > + \"query\": [\"id:$gen_msg_id\", null],
> >   \"tags\": [\"inbox\",
> >   \"unread\"]}]"
> >  
> > diff --git a/test/missing-headers b/test/missing-headers
> > index f14b878..43e861b 100755
> > --- a/test/missing-headers
> > +++ b/test/missing-headers
> > @@ -43,7 +43,8 @@ test_expect_equal_json "$output" '
> >          ],
> >          "thread": "XXX",
> >          "timestamp": 978709437,
> > -        "total": 1
> > +        "total": 1,
> > +        "query": ["id:notmuch-sha1-7a6e4eac383ef958fcd3ebf2143db71b8ff01161", null]
> >      },
> >      {
> >          "authors": "Notmuch Test Suite",
> > @@ -56,7 +57,8 @@ test_expect_equal_json "$output" '
> >          ],
> >          "thread": "XXX",
> >          "timestamp": 0,
> > -        "total": 1
> > +        "total": 1,
> > +        "query": ["id:notmuch-sha1-ca55943aff7a72baf2ab21fa74fab3d632401334", null]
> >      }
> >  ]'
> >  
> > diff --git a/test/sexp b/test/sexp
> > index 492a82f..be815e1 100755
> > --- a/test/sexp
> > +++ b/test/sexp
> > @@ -19,7 +19,7 @@ test_expect_equal "$output" "((((:id \"${gen_msg_id}\" :match t :excluded nil :f
> >  test_begin_subtest "Search message: sexp"
> >  add_message "[subject]=\"sexp-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"sexp-search-message\""
> >  output=$(notmuch search --format=sexp "sexp-search-message" | notmuch_search_sanitize)
> > -test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :tags (\"inbox\" \"unread\")))"
> > +test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
> >  
> >  test_begin_subtest "Show message: sexp, utf-8"
> >  add_message "[subject]=\"sexp-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\""
> > @@ -44,7 +44,7 @@ test_expect_equal "$output" "((((:id \"$id\" :match t :excluded nil :filename \"
> >  test_begin_subtest "Search message: sexp, utf-8"
> >  add_message "[subject]=\"sexp-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\""
> >  output=$(notmuch search --format=sexp "jsön-search-méssage" | notmuch_search_sanitize)
> > -test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :tags (\"inbox\" \"unread\")))"
> > +test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
> >  
> >  
> >  test_done
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.

Re: [PATCH 08/11] search: Add stable queries to thread search results

[Jani Nikula]

On Wed, 09 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Oct 09 at  9:41 am:
>> Also one talloc less. Which brings me to the main worry:
>> performance. What's the impact?
>
> Seems to be about 1%-3% for CLI search (tested on the medium corpus).
> It's hard to measure what the effect on Emacs search is, though I
> would expect it to be similarly negligible.

I can live with that. :)

> Some work I did several attempts at this ago suggests that this slows
> down tagging (though I doubt it would be noticeable for single
> threads), but I also found that switching to docid-based queries
> significantly sped things up:
> id:CAH-f9WsPj=1Eu=g3sOePJgCTBFs6HrLdLq18xMEnJ8aZ00yCEg@mail.gmail.com
> Actually, docid queries probably make tagging faster than it is *now*,
> but I didn't measure that when I did the experiments.

Looks like there's a few hurdles in adding that concept
nicely. Something for the future.

BR,
Jani.

[PATCH 09/11] Add TODO about more efficient stable thread queries

[Austin Clements]

---
 devel/TODO | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/devel/TODO b/devel/TODO
index 844555e..f212483 100644
--- a/devel/TODO
+++ b/devel/TODO
@@ -114,6 +114,12 @@ are multiple ideas that might make sense. Some consensus needs to be
 reached on this issue, and then both reply formats should be updated
 to be consistent.
 
+Return docid-based queries in thread search results, instead of
+message-id-based queries.  These are much more compact and much faster
+to later query.  This will require support from the library, both for
+retrieving docids (probably as an opaque "get a query that identifies
+this message") and for docid queries.
+
 notmuch library
 ---------------
 Add support for custom flag<->tag mappings. In the notmuch
-- 
1.8.4.rc3

[PATCH 10/11] emacs: Add known-broken tests for search tagging races

[Austin Clements]

These tests check that both thread-local and global search tagging
operations are race-free.  They are currently known-broken because
they aren't race-free.
---
 test/emacs | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/test/emacs b/test/emacs
index 2917189..e0dcd3c 100755
--- a/test/emacs
+++ b/test/emacs
@@ -922,4 +922,30 @@ This is a warning (see *Notmuch errors* for more details)
 This is a warning
 This is another warning"
 
+test_begin_subtest "Search thread tag operations are race-free"
+test_subtest_known_broken
+add_message '[subject]="Search race test"'
+gen_msg_id_1=$gen_msg_id
+generate_message '[in-reply-to]="<'$gen_msg_id_1'>"' \
+	    '[references]="<'$gen_msg_id_1'>"' \
+	    '[subject]="Search race test two"'
+test_emacs '(notmuch-search "subject:\"search race test\"")
+	    (notmuch-test-wait)
+	    (notmuch-poll)
+	    (execute-kbd-macro "+search-thread-race-tag")'
+output=$(notmuch search --output=messages 'tag:search-thread-race-tag')
+test_expect_equal "$output" "id:$gen_msg_id_1"
+
+test_begin_subtest "Search global tag operations are race-free"
+test_subtest_known_broken
+generate_message '[in-reply-to]="<'$gen_msg_id_1'>"' \
+	    '[references]="<'$gen_msg_id_1'>"' \
+	    '[subject]="Re: Search race test"'
+test_emacs '(notmuch-search "subject:\"search race test\" -subject:two")
+	    (notmuch-test-wait)
+	    (notmuch-poll)
+	    (execute-kbd-macro "*+search-global-race-tag")'
+output=$(notmuch search --output=messages 'tag:search-global-race-tag')
+test_expect_equal "$output" "id:$gen_msg_id_1"
+
 test_done
-- 
1.8.4.rc3

Re: [PATCH 10/11] emacs: Add known-broken tests for search tagging races

[Mark Walters]

On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> These tests check that both thread-local and global search tagging
> operations are race-free.  They are currently known-broken because
> they aren't race-free.
> ---
>  test/emacs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/test/emacs b/test/emacs
> index 2917189..e0dcd3c 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -922,4 +922,30 @@ This is a warning (see *Notmuch errors* for more details)
>  This is a warning
>  This is another warning"
>  
> +test_begin_subtest "Search thread tag operations are race-free"
> +test_subtest_known_broken
> +add_message '[subject]="Search race test"'
> +gen_msg_id_1=$gen_msg_id
> +generate_message '[in-reply-to]="<'$gen_msg_id_1'>"' \
> +	    '[references]="<'$gen_msg_id_1'>"' \
> +	    '[subject]="Search race test two"'
> +test_emacs '(notmuch-search "subject:\"search race test\"")
> +	    (notmuch-test-wait)
> +	    (notmuch-poll)
> +	    (execute-kbd-macro "+search-thread-race-tag")'
> +output=$(notmuch search --output=messages 'tag:search-thread-race-tag')
> +test_expect_equal "$output" "id:$gen_msg_id_1"
> +
> +test_begin_subtest "Search global tag operations are race-free"
> +test_subtest_known_broken
> +generate_message '[in-reply-to]="<'$gen_msg_id_1'>"' \
> +	    '[references]="<'$gen_msg_id_1'>"' \
> +	    '[subject]="Re: Search race test"'
> +test_emacs '(notmuch-search "subject:\"search race test\" -subject:two")
> +	    (notmuch-test-wait)
> +	    (notmuch-poll)
> +	    (execute-kbd-macro "*+search-global-race-tag")'
> +output=$(notmuch search --output=messages 'tag:search-global-race-tag')
> +test_expect_equal "$output" "id:$gen_msg_id_1"
> +
>  test_done

For the global tagging test would it be worth also adding a matching
message in a different thread?

Best wishes

Mark



> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 11/11] emacs: Fix search tagging races

[Austin Clements]

This fixes races in thread-local and global tagging in notmuch-search
(e.g., "+", "-", "a", "*", etc.).  Previously, these would modify tags
of new messages that arrived after the search.  Now they only operate
on the messages that were in the threads when the search was
performed.  This prevents surprises like archiving messages that
arrived in a thread after the search results were shown.

This eliminates `notmuch-search-find-thread-id-region(-search)'
because these functions strongly encouraged racy usage.

This fixes the two broken tests added by the previous patch.
---
 devel/TODO       |  5 -----
 emacs/notmuch.el | 40 +++++++++++++++++++++++++++-------------
 test/emacs       |  4 +---
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/devel/TODO b/devel/TODO
index f212483..1cf4089 100644
--- a/devel/TODO
+++ b/devel/TODO
@@ -14,11 +14,6 @@ sender's name containing ';' which causes emacs to drop a search
 result.) This may require removing the outer array from the current
 "notmuch search --format=json" results.
 
-Fix '*' to work by simply calling '+' or '-' on a region consisting of
-the entire buffer, (this would avoid one race condition---while still
-leaving other race conditions---but could also potentially make '*' a
-very expensive operation).
-
 Add a global keybinding table for notmuch, and then view-specific
 tables that add to it.
 	
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 2c7678d..1c64745 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -461,14 +461,25 @@ If BARE is set then do not prefix with \"thread:\""
   (let ((thread (plist-get (notmuch-search-get-result) :thread)))
     (when thread (concat (unless bare "thread:") thread))))
 
-(defun notmuch-search-find-thread-id-region (beg end)
-  "Return a list of threads for the current region"
-  (mapcar (lambda (thread) (concat "thread:" thread))
-	  (notmuch-search-properties-in-region :thread beg end)))
-
-(defun notmuch-search-find-thread-id-region-search (beg end)
-  "Return a search string for threads for the current region"
-  (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or "))
+(defun notmuch-search-find-stable-query ()
+  "Return the stable queries for the current thread.
+
+This returns a list (MATCHED-QUERY UNMATCHED-QUERY) for the
+matched and unmatched messages in the current thread."
+  (plist-get (notmuch-search-get-result) :query))
+
+(defun notmuch-search-find-stable-query-region (beg end &optional only-matched)
+  "Return the stable query for the current region.
+
+If ONLY-MATCHED is non-nil, include only matched messages.  If it
+is nil, include both matched and unmatched messages."
+  (let ((query-list nil) (all (not only-matched)))
+    (dolist (queries (notmuch-search-properties-in-region :query beg end))
+      (when (first queries)
+	(push (first queries) query-list))
+      (when (and all (second queries))
+	(push (second queries) query-list)))
+    (concat "(" (mapconcat 'identity query-list ") or (") ")")))
 
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
@@ -525,9 +536,12 @@ If BARE is set then do not prefix with \"thread:\""
 	(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."
-  (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
+(defun notmuch-search-tag-region (beg end &optional tag-changes only-matched)
+  "Change tags for threads in the given region.
+
+If ONLY-MATCHED is non-nil, only tag matched messages."
+  (let ((search-string (notmuch-search-find-stable-query-region
+			beg end only-matched)))
     (setq tag-changes (notmuch-tag search-string tag-changes))
     (notmuch-search-foreach-result beg end
       (lambda (pos)
@@ -796,7 +810,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-search-tag-region (point-min) (point-max) tag-changes t))
 
 (defun notmuch-search-buffer-title (query)
   "Returns the title for a buffer with notmuch search results."
@@ -899,7 +913,7 @@ the configured default sort order."
       (save-excursion
 	(let ((proc (notmuch-start-notmuch
 		     "notmuch-search" buffer #'notmuch-search-process-sentinel
-		     "search" "--format=sexp" "--format-version=1"
+		     "search" "--format=sexp" "--format-version=2"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
diff --git a/test/emacs b/test/emacs
index e0dcd3c..88eb1e5 100755
--- a/test/emacs
+++ b/test/emacs
@@ -889,7 +889,7 @@ $PWD/notmuch_fail exited with status 1 (see *Notmuch errors* for more details)
 ---
 [XXX]
 $PWD/notmuch_fail exited with status 1
-command: $PWD/notmuch_fail search --format\=sexp --format-version\=1 --sort\=newest-first tag\:inbox
+command: $PWD/notmuch_fail search --format\=sexp --format-version\=2 --sort\=newest-first tag\:inbox
 exit status: 1"
 
 test_begin_subtest "Search handles subprocess warnings"
@@ -923,7 +923,6 @@ This is a warning
 This is another warning"
 
 test_begin_subtest "Search thread tag operations are race-free"
-test_subtest_known_broken
 add_message '[subject]="Search race test"'
 gen_msg_id_1=$gen_msg_id
 generate_message '[in-reply-to]="<'$gen_msg_id_1'>"' \
@@ -937,7 +936,6 @@ output=$(notmuch search --output=messages 'tag:search-thread-race-tag')
 test_expect_equal "$output" "id:$gen_msg_id_1"
 
 test_begin_subtest "Search global tag operations are race-free"
-test_subtest_known_broken
 generate_message '[in-reply-to]="<'$gen_msg_id_1'>"' \
 	    '[references]="<'$gen_msg_id_1'>"' \
 	    '[subject]="Re: Search race test"'
-- 
1.8.4.rc3

Re: [PATCH 00/11] Fix search tagging races

[Mark Walters]

Hello

It's great that this might finally get done. But there is one problem
currently.

If you open a large search buffer and then do *-<tab> it will die as the
tagging routine runs notmuch search to find a completion-list for the
tag. (it runs notmuch search --output=tags <query>)

We could just return all tags in this case. Or we could do something
like the series
id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com
which makes completion happen based on the tags visible to the user, not
the tags actually in the database.

There is also a little discussion of this in my earlier attempt at
fixing this: eg id:87mwy4smad.fsf@qmul.ac.uk

Best wishes

Mark

On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> I was hacking on undo support for notmuch-emacs and sort of
> accidentally wrote this instead.  This series fixes a set of
> well-known races where tagging from search-mode unexpectedly affects
> messages that arrived after the search was performed (and hence the
> user doesn't know they're tagging them).  We've attacked this a few
> times before, but have always run up against something that was
> missing.  It turns out the pieces are finally all in place.
>
> The first five patches just clean various things up in preparation.
> Patches 6 and 7 add support for tagging large queries, which would
> otherwise become a problem when later patches start using explicit
> message ID-based queries for tagging.  The remaining four patches
> actually fix the search tagging races using explicit message ID-based
> queries.
>
> It's a fairly long series, but none of the patches are very big.
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 00/11] Fix search tagging races

[Austin Clements]

Quoth Mark Walters on Oct 08 at  8:56 am:
> 
> Hello
> 
> It's great that this might finally get done. But there is one problem
> currently.
> 
> If you open a large search buffer and then do *-<tab> it will die as the
> tagging routine runs notmuch search to find a completion-list for the
> tag. (it runs notmuch search --output=tags <query>)

Hmm.  If we implement docid queries (described in the TODO added by
this series), we should be able to get away with what we're doing now
without any serious performance problems...

> We could just return all tags in this case. Or we could do something
> like the series
> id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com
> which makes completion happen based on the tags visible to the user, not
> the tags actually in the database.

OTOH, I think what you were going for in this series is the right
thing to do from a UI perspective anyway.  I'll try implementing
something along these lines, though I've got an idea that I think will
by more Elispy.  Currently `notmuch-tag' has this strange interface
where it can interactively prompt in some cases.  This isn't the right
way to do this.  `notmuch-tag' should be non-interactive and the
interactive tagging commands should have an interactive specification
that prompts for tags to change, right at the interactive entry point.
This should give us a clean place to provide a list of existing tags,
and would also let us do other nice things like specify a different
tag prompt for * commands, and maybe add a y/n prompt to confirm a *
tagging command.  It would also provide a convenient place to wait for
search results to finish coming in after prompting in the * command,
which would be awkward to do right now.

> There is also a little discussion of this in my earlier attempt at
> fixing this: eg id:87mwy4smad.fsf@qmul.ac.uk
> 
> Best wishes
> 
> Mark
> 
> On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > I was hacking on undo support for notmuch-emacs and sort of
> > accidentally wrote this instead.  This series fixes a set of
> > well-known races where tagging from search-mode unexpectedly affects
> > messages that arrived after the search was performed (and hence the
> > user doesn't know they're tagging them).  We've attacked this a few
> > times before, but have always run up against something that was
> > missing.  It turns out the pieces are finally all in place.
> >
> > The first five patches just clean various things up in preparation.
> > Patches 6 and 7 add support for tagging large queries, which would
> > otherwise become a problem when later patches start using explicit
> > message ID-based queries for tagging.  The remaining four patches
> > actually fix the search tagging races using explicit message ID-based
> > queries.
> >
> > It's a fairly long series, but none of the patches are very big.
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 00/11] Fix search tagging races

[Mark Walters]

On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> I was hacking on undo support for notmuch-emacs and sort of
> accidentally wrote this instead.  This series fixes a set of
> well-known races where tagging from search-mode unexpectedly affects
> messages that arrived after the search was performed (and hence the
> user doesn't know they're tagging them).  We've attacked this a few
> times before, but have always run up against something that was
> missing.  It turns out the pieces are finally all in place.
>
> The first five patches just clean various things up in preparation.
> Patches 6 and 7 add support for tagging large queries, which would
> otherwise become a problem when later patches start using explicit
> message ID-based queries for tagging.  The remaining four patches
> actually fix the search tagging races using explicit message ID-based
> queries.
>
> It's a fairly long series, but none of the patches are very big.

One more thought on this: what should "*+tag" do if the search buffer is
still filling?

As it stands this is a substantial change: previously you could
look at the first few threads to make sure the query was doing what you
expected and then tag all the matching threads. Now you would have to
wait for the buffer to fill and there is not even a clear indication to
the user of when that happens (except scroll to the bottom and see if it
says "End of search results.")

Best wishes

Mark

Re: [PATCH 00/11] Fix search tagging races

[Austin Clements]

Quoth Mark Walters on Oct 09 at  8:43 am:
> 
> On Mon, 07 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > I was hacking on undo support for notmuch-emacs and sort of
> > accidentally wrote this instead.  This series fixes a set of
> > well-known races where tagging from search-mode unexpectedly affects
> > messages that arrived after the search was performed (and hence the
> > user doesn't know they're tagging them).  We've attacked this a few
> > times before, but have always run up against something that was
> > missing.  It turns out the pieces are finally all in place.
> >
> > The first five patches just clean various things up in preparation.
> > Patches 6 and 7 add support for tagging large queries, which would
> > otherwise become a problem when later patches start using explicit
> > message ID-based queries for tagging.  The remaining four patches
> > actually fix the search tagging races using explicit message ID-based
> > queries.
> >
> > It's a fairly long series, but none of the patches are very big.
> 
> One more thought on this: what should "*+tag" do if the search buffer is
> still filling?
> 
> As it stands this is a substantial change: previously you could
> look at the first few threads to make sure the query was doing what you
> expected and then tag all the matching threads. Now you would have to
> wait for the buffer to fill and there is not even a clear indication to
> the user of when that happens (except scroll to the bottom and see if it
> says "End of search results.")

That's a very interesting point.

Arguably, this is one situation where it's almost certainly safe to
use the original query: if the search hasn't finished yet, almost
certainly none of the threads have changed.  So, that's one
possibility.

Another possibility would be to simply echo "Waiting for all search
results...  (Press C-g to cancel)" and wait until the search is done.

> Best wishes
> 
> Mark