[PATCH v2 0/7] Fix emacs tagging race

[PATCH v2 0/7] Fix emacs tagging race

[markwalters1009]

This is version 2 of this series: version 1 is at
id:1352487491-31512-1-git-send-email-markwalters1009@gmail.com but
this is a much more complete version. Version 1 roughly corresponds to
patches 5-7.

The first two patches allows queries to come from stdin (if the query
string is "-"). This is necessary to avoid ARGMAX limits in some
cases. They are independent of the rest of the series. The main thing
needed for these two (apart from review!) is a manpage but I wasn't
sure whether that should go in notmuch-search-terms or somewhere else.

Patches 3 and 4 make the emacs interface use this new functionality to
pass the tagging query. These two patches depend on the previous two
but are independent of the later patches. Note that it is possible (if
unlikely) to trigger the ARGMAX problem in current notmuch: highlight
most or all of a large search buffer and then try to tag the region.

Patches 5-7 actually fix the race. They do this by appending two query
strings to each search: one query string for the matching messages and
one for the non-matching messages. The front-end can then combine
these query strings to make sure it only tags messages that were
present/matched when the search buffer was created.

The main changes from v1 are to append query-string rather than all
the message-ids (so if we had a better way of constructing the queries
we could switch to that later) and to use Austin's suggestion of
--queries=true to add the queries. I think we do want the choice as
appending the string could easily double the size of the output.

This version (since rebasing and tidying) is not heavily tested (all
tests pass) but I have been running a similar version for some time
without problems.

Best wishes

Mark




Mark Walters (7):
  cli: allow query to come from stdin
  test: for the new query from stdin functionality
  emacs: notmuch.el split call-process into call-process-region
  emacs: make emacs tagging use the stdin query functionality
  test: test for race when tagging from emacs search
  cli: allow search mode to include msg-ids with JSON output
  emacs: make emacs use message-ids for tagging

 emacs/notmuch-tag.el |   14 +++++---
 emacs/notmuch.el     |   47 ++++++++++++++++++++----
 notmuch-search.c     |   95 ++++++++++++++++++++++++++++++++++++++++++++++---
 query-string.c       |   41 +++++++++++++++++++++
 test/emacs           |   21 +++++++++++
 test/tagging         |    9 +++++
 6 files changed, 208 insertions(+), 19 deletions(-)

-- 
1.7.9.1

[PATCH v2 1/7] cli: allow query to come from stdin

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

After this series there will be times when a caller will want to pass
a very large query string to notmuch (eg a list of 10,000 message-ids)
and this can exceed the size of ARG_MAX. Hence allow notmuch to take
the query from stdin (if the query is -).
---
 query-string.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/query-string.c b/query-string.c
index 6536512..b1fbdeb 100644
--- a/query-string.c
+++ b/query-string.c
@@ -20,6 +20,44 @@
 
 #include "notmuch-client.h"
 
+/* Read a single query string from STDIN, using
+ * 'ctx' as the talloc owner for all allocations.
+ *
+ * This function returns NULL in case of insufficient memory or read
+ * errors.
+ */
+static char *
+query_string_from_stdin (void *ctx)
+{
+    char *query_string;
+    char buf[4096];
+    ssize_t remain;
+
+    query_string = talloc_strdup (ctx, "");
+    if (query_string == NULL)
+	return NULL;
+
+    for (;;) {
+	remain = read (STDIN_FILENO, buf, sizeof(buf) - 1);
+	if (remain == 0)
+	    break;
+	if (remain < 0) {
+	    if (errno == EINTR)
+		continue;
+	    fprintf (stderr, "Error: reading from standard input: %s\n",
+		     strerror (errno));
+	    return NULL;
+	}
+
+	buf[remain] = '\0';
+	query_string = talloc_strdup_append (query_string, buf);
+	if (query_string == NULL)
+	    return NULL;
+    }
+
+    return query_string;
+}
+
 /* Construct a single query string from the passed arguments, using
  * 'ctx' as the talloc owner for all allocations.
  *
@@ -35,6 +73,9 @@ query_string_from_args (void *ctx, int argc, char *argv[])
     char *query_string;
     int i;
 
+    if ((argc == 1) && (strcmp ("-", argv[0]) == 0))
+	return query_string_from_stdin (ctx);
+
     query_string = talloc_strdup (ctx, "");
     if (query_string == NULL)
 	return NULL;
-- 
1.7.9.1

[PATCH v2 2/7] test: for the new query from stdin functionality

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

---
 test/tagging |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/test/tagging b/test/tagging
index 980ff92..eb7d61c 100755
--- a/test/tagging
+++ b/test/tagging
@@ -19,6 +19,15 @@ test_expect_equal "$output" "\
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (inbox tag3 unread)
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag3 unread)"
 
+test_begin_subtest "Adding tags. Query from stdin"
+echo -n "subject:One" | notmuch tag +intag1 +intag2 -- -
+echo DONE
+output=$(notmuch search \* | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (inbox intag1 intag2 tag3 unread)
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag3 unread)"
+notmuch tag -intag1 -intag2 \*
+
 test_expect_code 1 "No tag operations" 'notmuch tag One'
 test_expect_code 1 "No query" 'notmuch tag +tag2'
 
-- 
1.7.9.1

[PATCH v2 3/7] emacs: notmuch.el split call-process into call-process-region

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

We add a new function notmuch-call-process-region so that functions
can call notmuch with some region sent to stdin. This is preparation
for using the new query from stdin functionality.
---
 emacs/notmuch.el |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f9454d8..64b9474 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -533,15 +533,17 @@ 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.
+(defun notmuch-call-notmuch-process-region (beg end &rest args)
+  "Synchronously invoke \"notmuch\" with the given list of arguments and pipe region.
 
-Output from the process will be presented to the user as an error
-and will also appear in a buffer named \"*Notmuch errors*\"."
+The region from beg to end in the current buffer will be piped to
+stdin for the notmuch process.  Output from the process will be
+presented to the user as an error and will also appear in a
+buffer named \"*Notmuch errors*\"."
   (let ((error-buffer (get-buffer-create "*Notmuch errors*")))
     (with-current-buffer error-buffer
 	(erase-buffer))
-    (if (eq (apply 'call-process notmuch-command nil error-buffer nil args) 0)
+    (if (eq (apply 'call-process-region beg end notmuch-command nil error-buffer nil args) 0)
 	(point)
       (progn
 	(with-current-buffer error-buffer
@@ -550,6 +552,13 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	    (error (buffer-substring beg end))
 	    ))))))
 
+(defun notmuch-call-notmuch-process (&rest args)
+  "Synchronously invoke \"notmuch\" with the given list of arguments.
+
+Output from the process will be presented to the user as an error
+and will also appear in a buffer named \"*Notmuch errors*\"."
+  (apply 'notmuch-call-notmuch-process-region (point) (point) args))
+
 (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.7.9.1

[PATCH v2 4/7] emacs: make emacs tagging use the stdin query functionality

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

In preparation for the use of large queries in some cases make tagging
from emacs use the new query on stdin functionality. Currently uses
this for all tagging (as I could not see a reason not to).
---
 emacs/notmuch-tag.el |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 4fce3a9..4634b0d 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -59,9 +59,10 @@ the messages that were tagged"
       (setq search-terms (list "*")))
   (split-string
    (with-output-to-string
-     (with-current-buffer standard-output
-       (apply 'call-process notmuch-command nil t
-	      nil "search" "--output=tags" "--exclude=false" search-terms)))
+     (with-temp-buffer
+       (insert (mapconcat 'identity search-terms " "))
+       (apply 'call-process-region (point-min) (point-max) notmuch-command nil
+	      standard-output nil "search" "--output=tags" "--exclude=false" (list "-"))))
    "\n+" t))
 
 (defun notmuch-select-tag-with-completion (prompt &rest search-terms)
@@ -134,8 +135,11 @@ 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)))
+    (with-temp-buffer
+      (insert query)
+      (apply 'notmuch-call-notmuch-process-region
+	     (point-min) (point-max)
+	     "tag" (append tag-changes (list "--" "-"))))
     (run-hooks 'notmuch-after-tag-hook))
   ;; in all cases we return tag-changes as a list
   tag-changes)
-- 
1.7.9.1

[PATCH v2 5/7] test: test for race when tagging from emacs search

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

When tagging from search view in emacs there is a race condition: it
tags all messages in the thread even ones which arrived after the
search was made. This can cause dataloss (if, for example, a thread is
archived it could archive messages the user has never seen).

Mark this test known broken.
---
 test/emacs |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 77265b0..3788439 100755
--- a/test/emacs
+++ b/test/emacs
@@ -122,6 +122,29 @@ 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 "Tag all matching messages from search view"
+test_subtest_known_broken
+notmuch tag +test-tag-race from:cworth
+test_emacs "(notmuch-search \"tag:test-tag-race\")
+	    (notmuch-test-wait)"
+notmuch tag +test-tag-race "id:1258471718-6781-2-git-send-email-dottedmag@dottedmag.net"
+test_emacs "(execute-kbd-macro \"*+test-tag-race-2\")"
+output=$(notmuch count tag:test-tag-race-2)
+test_expect_equal "$output" "12"
+notmuch tag -test-tag-race '*'
+notmuch tag -test-tag-race-2 '*'
+
+test_begin_subtest "Change tags from search view: another message arriving after thread lookup"
+test_subtest_known_broken
+typsos_id="878we4qdqf.fsf@yoom.home.cworth.org"
+typsos_thread=$(notmuch search --output=threads id:$typsos_id)
+test_emacs "(notmuch-search \"$typsos_thread\")
+	    (notmuch-test-wait)"
+add_message "[subject]=\"new-thread-message\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"new-thread-message\"" "[in-reply-to]=\"<$typsos_id>\""
+test_emacs "(execute-kbd-macro \"+tag-from-search-view -unread\")"
+output=$(notmuch search tag:tag-from-search-view | notmuch_search_sanitize)
+test_expect_equal "$output" "thread:XXX   2009-11-18 [2/3] Ingmar Vanhassel, Carl Worth| Notmuch Test Suite; [notmuch] [PATCH] Typsos (inbox tag-from-search-view unread)"
+
 test_begin_subtest "Add tag from notmuch-show view"
 test_emacs "(notmuch-show \"$os_x_darwin_thread\")
 	    (execute-kbd-macro \"+tag-from-show-view\")"
-- 
1.7.9.1

[PATCH v2 6/7] cli: allow search mode to include msg-ids with JSON output

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

This adds a --queries=true option which modifies the summary output of
notmuch search by including two extra query strings with each result:
one query string specifies all matching messages and one query string
all non-matching messages. Currently these are just lists of message
ids joined with " or " but that could change in future.

Currently this is not implemented for text format.
---
 notmuch-search.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 830c4e4..c8fc9a6 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -26,7 +26,8 @@ typedef enum {
     OUTPUT_THREADS,
     OUTPUT_MESSAGES,
     OUTPUT_FILES,
-    OUTPUT_TAGS
+    OUTPUT_TAGS,
+    OUTPUT_SUMMARY_WITH_QUERIES
 } output_t;
 
 static char *
@@ -46,6 +47,57 @@ sanitize_string (const void *ctx, const char *str)
     return out;
 }
 
+/* This function takes a message id and returns an escaped string
+ * which can be used as a Xapian query. This involves prefixing with
+ * `id:', putting the id inside double quotes, and doubling any
+ * occurence of a double quote in the message id itself.*/
+static char *
+xapian_escape_id (const void *ctx,
+	   const char *msg_id)
+{
+    const char *c;
+    char *escaped_msg_id;
+    escaped_msg_id = talloc_strdup (ctx, "id:\"");
+    for (c=msg_id; *c; c++)
+	if (*c == '"')
+	    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"\"");
+	else
+	    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "%c", *c);
+    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"");
+    return escaped_msg_id;
+}
+
+static char *
+output_msg_query (const void *ctx,
+		sprinter_t *format,
+		notmuch_bool_t matching,
+		notmuch_bool_t first,
+		notmuch_messages_t *messages)
+{
+    notmuch_message_t *message;
+    char *query, *escaped_msg_id;
+    query = talloc_strdup (ctx, "");
+    for (;
+	 notmuch_messages_valid (messages);
+	 notmuch_messages_move_to_next (messages))
+    {
+	message = notmuch_messages_get (messages);
+	if (notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) == matching) {
+	    escaped_msg_id = xapian_escape_id (ctx, notmuch_message_get_message_id (message));
+	    if (first) {
+		query = talloc_asprintf_append (query, "%s", escaped_msg_id);
+		first = FALSE;
+	    }
+	    else
+		query = talloc_asprintf_append (query, " or %s", escaped_msg_id);
+	    talloc_free (escaped_msg_id);
+	}
+	/* output_msg_query already starts with an ` or' */
+	query = talloc_asprintf_append (query, "%s", output_msg_query (ctx, format, matching, first, notmuch_message_get_replies (message)));
+    }
+    return query;
+}
+
 static int
 do_search_threads (sprinter_t *format,
 		   notmuch_query_t *query,
@@ -88,7 +140,7 @@ do_search_threads (sprinter_t *format,
 	    format->string (format,
 			    notmuch_thread_get_thread_id (thread));
 	    format->separator (format);
-	} else { /* output == OUTPUT_SUMMARY */
+	} else { /* output == OUTPUT_SUMMARY or OUTPUT_SUMMARY_WITH_QUERIES */
 	    void *ctx_quote = talloc_new (thread);
 	    const char *authors = notmuch_thread_get_authors (thread);
 	    const char *subject = notmuch_thread_get_subject (thread);
@@ -108,7 +160,7 @@ do_search_threads (sprinter_t *format,
 	    relative_date = notmuch_time_relative_date (ctx_quote, date);
 
 	    if (format->is_text_printer) {
-                /* Special case for the text formatter */
+               /* Special case for the text formatter */
 		printf ("thread:%s %12s [%d/%d] %s; %s (",
 			thread_id,
 			relative_date,
@@ -133,8 +185,6 @@ do_search_threads (sprinter_t *format,
 		format->string (format, subject);
 	    }
 
-	    talloc_free (ctx_quote);
-
 	    format->map_key (format, "tags");
 	    format->begin_list (format);
 
@@ -145,7 +195,7 @@ do_search_threads (sprinter_t *format,
 		const char *tag = notmuch_tags_get (tags);
 
 		if (format->is_text_printer) {
-                  /* Special case for the text formatter */
+		    /* Special case for the text formatter */
 		    if (first_tag)
 			first_tag = FALSE;
 		    else
@@ -160,8 +210,25 @@ do_search_threads (sprinter_t *format,
 		printf (")");
 
 	    format->end (format);
+
+	    if (output == OUTPUT_SUMMARY_WITH_QUERIES) {
+		char *query;
+		query = output_msg_query (ctx_quote, format, TRUE, TRUE, notmuch_thread_get_toplevel_messages (thread));
+		if (strlen (query)) {
+		    format->map_key (format, "matching_msg_query");
+		    format->string (format, query);
+		}
+		query = output_msg_query (ctx_quote, format, FALSE, TRUE, notmuch_thread_get_toplevel_messages (thread));
+		if (strlen (query)) {
+		    format->map_key (format, "nonmatching_msg_query");
+		    format->string (format, query);
+		}
+	    }
+
 	    format->end (format);
 	    format->separator (format);
+
+	    talloc_free (ctx_quote);
 	}
 
 	notmuch_thread_destroy (thread);
@@ -303,6 +370,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     int offset = 0;
     int limit = -1; /* unlimited */
     int exclude = EXCLUDE_TRUE;
+    notmuch_bool_t with_queries = FALSE;
     unsigned int i;
 
     enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
@@ -323,12 +391,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 				  { "messages", OUTPUT_MESSAGES },
 				  { "files", OUTPUT_FILES },
 				  { "tags", OUTPUT_TAGS },
+				  { "with-queries", OUTPUT_SUMMARY_WITH_QUERIES },
 				  { 0, 0 } } },
         { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
           (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
                                   { "false", EXCLUDE_FALSE },
                                   { "flag", EXCLUDE_FLAG },
                                   { 0, 0 } } },
+        { NOTMUCH_OPT_BOOLEAN, &with_queries, "queries", 'b', 0 },
 	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
 	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
 	{ 0, 0, 0, 0, 0 }
@@ -398,6 +468,19 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 	    notmuch_query_set_omit_excluded (query, FALSE);
     }
 
+    if (with_queries) {
+	if (format_sel == NOTMUCH_FORMAT_TEXT) {
+	    fprintf (stderr, "Warning: --queries=true not implemented for text format.\n");
+	    with_queries = FALSE;
+	}
+	if (output != OUTPUT_SUMMARY) {
+	    fprintf (stderr, "Warning: --queries=true only implemented for --output=summary.\n");
+	    with_queries = FALSE;
+	}
+    }
+
+    if (with_queries) output = OUTPUT_SUMMARY_WITH_QUERIES;
+
     switch (output) {
     default:
     case OUTPUT_SUMMARY:
-- 
1.7.9.1

[PATCH v2 7/7] emacs: make emacs use message-ids for tagging

[markwalters1009]

From: Mark Walters <markwalters1009@gmail.com>

This makes emacs use the new --queries=true in search mode and use
this for tagging.  This fixes the race condition in tagging from
search mode so mark the tests fixed.
---
 emacs/notmuch.el |   28 +++++++++++++++++++++++++---
 test/emacs       |    2 --
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 64b9474..6e8ef83 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -473,7 +473,8 @@ BEG."
   (let (output)
     (notmuch-search-foreach-result beg end
       (lambda (pos)
-	(push (plist-get (notmuch-search-get-result pos) property) output)))
+	(let ((value (plist-get (notmuch-search-get-result pos) property)))
+	  (when value (push value output)))))
     output))
 
 (defun notmuch-search-find-thread-id (&optional bare)
@@ -483,6 +484,7 @@ 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))
@@ -492,6 +494,23 @@ If BARE is set then do not prefix with \"thread:\""
   "Return a search string for threads for the current region"
   (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or "))
 
+;; The following two functions are similar to the previous two but
+;; they only match messages that were in the the thread when the
+;; initial search was run. This means that they can be used where it
+;; is important to avoid races: e.g. when tagging.
+(defun notmuch-search-find-queries-region (beg end &optional only-matching)
+  (interactive)
+  "Return a list of queries for the current region"
+  (append (notmuch-search-properties-in-region :matching_msg_query beg end)
+	  (unless only-matching
+	    (notmuch-search-properties-in-region :nonmatching_msg_query beg end))))
+
+(defun notmuch-search-find-queries-region-search (beg end &optional only-matching)
+  "Return a search string for messages in threads in the current region"
+  (mapconcat 'identity
+	     (notmuch-search-find-queries-region beg end only-matching)
+	     " or "))
+
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
   (plist-get (notmuch-search-get-result) :authors))
@@ -575,7 +594,7 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 
 (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)))
+  (let ((search-string (notmuch-search-find-queries-region-search beg end)))
     (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
     (notmuch-search-foreach-result beg end
       (lambda (pos)
@@ -851,7 +870,9 @@ 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))
+  (apply 'notmuch-tag (notmuch-search-find-queries-region-search
+		       (point-min) (point-max) t)
+	 tag-changes))
 
 (defun notmuch-search-buffer-title (query)
   "Returns the title for a buffer with notmuch search results."
@@ -948,6 +969,7 @@ Other optional parameters are used as follows:
 		     "notmuch-search" buffer
 		     notmuch-command "search"
 		     "--format=json"
+		     "--output=with-queries"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
diff --git a/test/emacs b/test/emacs
index 3788439..132768f 100755
--- a/test/emacs
+++ b/test/emacs
@@ -123,7 +123,6 @@ 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 "Tag all matching messages from search view"
-test_subtest_known_broken
 notmuch tag +test-tag-race from:cworth
 test_emacs "(notmuch-search \"tag:test-tag-race\")
 	    (notmuch-test-wait)"
@@ -135,7 +134,6 @@ notmuch tag -test-tag-race '*'
 notmuch tag -test-tag-race-2 '*'
 
 test_begin_subtest "Change tags from search view: another message arriving after thread lookup"
-test_subtest_known_broken
 typsos_id="878we4qdqf.fsf@yoom.home.cworth.org"
 typsos_thread=$(notmuch search --output=threads id:$typsos_id)
 test_emacs "(notmuch-search \"$typsos_thread\")
-- 
1.7.9.1

Re: [PATCH v2 1/7] cli: allow query to come from stdin

[Mark Walters]

I should have said the code for reading from stdin was 
stolen from Peter's patch for notmuch-insert
id:1343223767-9812-4-git-send-email-novalazy@gmail.com
(errors are of course mine)

My apologies for not mentioning this in the commit message.

Mark



On Sat, 24 Nov 2012, markwalters1009 <markwalters1009@gmail.com> wrote:
> From: Mark Walters <markwalters1009@gmail.com>
>
> After this series there will be times when a caller will want to pass
> a very large query string to notmuch (eg a list of 10,000 message-ids)
> and this can exceed the size of ARG_MAX. Hence allow notmuch to take
> the query from stdin (if the query is -).
> ---
>  query-string.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/query-string.c b/query-string.c
> index 6536512..b1fbdeb 100644
> --- a/query-string.c
> +++ b/query-string.c
> @@ -20,6 +20,44 @@
>  
>  #include "notmuch-client.h"
>  
> +/* Read a single query string from STDIN, using
> + * 'ctx' as the talloc owner for all allocations.
> + *
> + * This function returns NULL in case of insufficient memory or read
> + * errors.
> + */
> +static char *
> +query_string_from_stdin (void *ctx)
> +{
> +    char *query_string;
> +    char buf[4096];
> +    ssize_t remain;
> +
> +    query_string = talloc_strdup (ctx, "");
> +    if (query_string == NULL)
> +	return NULL;
> +
> +    for (;;) {
> +	remain = read (STDIN_FILENO, buf, sizeof(buf) - 1);
> +	if (remain == 0)
> +	    break;
> +	if (remain < 0) {
> +	    if (errno == EINTR)
> +		continue;
> +	    fprintf (stderr, "Error: reading from standard input: %s\n",
> +		     strerror (errno));
> +	    return NULL;
> +	}
> +
> +	buf[remain] = '\0';
> +	query_string = talloc_strdup_append (query_string, buf);
> +	if (query_string == NULL)
> +	    return NULL;
> +    }
> +
> +    return query_string;
> +}
> +
>  /* Construct a single query string from the passed arguments, using
>   * 'ctx' as the talloc owner for all allocations.
>   *
> @@ -35,6 +73,9 @@ query_string_from_args (void *ctx, int argc, char *argv[])
>      char *query_string;
>      int i;
>  
> +    if ((argc == 1) && (strcmp ("-", argv[0]) == 0))
> +	return query_string_from_stdin (ctx);
> +
>      query_string = talloc_strdup (ctx, "");
>      if (query_string == NULL)
>  	return NULL;
> -- 
> 1.7.9.1

Re: [PATCH v2 1/7] cli: allow query to come from stdin

[Austin Clements]

Quoth markwalters1009 on Nov 24 at  1:20 pm:
> From: Mark Walters <markwalters1009@gmail.com>
> 
> After this series there will be times when a caller will want to pass
> a very large query string to notmuch (eg a list of 10,000 message-ids)
> and this can exceed the size of ARG_MAX. Hence allow notmuch to take
> the query from stdin (if the query is -).
> ---
>  query-string.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 41 insertions(+), 0 deletions(-)
> 
> diff --git a/query-string.c b/query-string.c
> index 6536512..b1fbdeb 100644
> --- a/query-string.c
> +++ b/query-string.c
> @@ -20,6 +20,44 @@
>  
>  #include "notmuch-client.h"
>  
> +/* Read a single query string from STDIN, using
> + * 'ctx' as the talloc owner for all allocations.
> + *
> + * This function returns NULL in case of insufficient memory or read
> + * errors.
> + */
> +static char *
> +query_string_from_stdin (void *ctx)
> +{
> +    char *query_string;
> +    char buf[4096];
> +    ssize_t remain;
> +
> +    query_string = talloc_strdup (ctx, "");
> +    if (query_string == NULL)
> +	return NULL;
> +
> +    for (;;) {
> +	remain = read (STDIN_FILENO, buf, sizeof(buf) - 1);
> +	if (remain == 0)
> +	    break;
> +	if (remain < 0) {
> +	    if (errno == EINTR)
> +		continue;
> +	    fprintf (stderr, "Error: reading from standard input: %s\n",
> +		     strerror (errno));

talloc_free (query_string) ?

> +	    return NULL;
> +	}
> +
> +	buf[remain] = '\0';
> +	query_string = talloc_strdup_append (query_string, buf);

Eliminate the NUL in buf and instead
 talloc_strndup_append (query_string, buf, remain) ?

Should there be some (large) bound on the size of the query string to
prevent runaway?

> +	if (query_string == NULL)

Technically it would be good to talloc_free the old pointer here, too.

> +	    return NULL;
> +    }
> +
> +    return query_string;
> +}
> +

This whole approach is O(n^2), which might actually matter for large
query strings.  How about (tested, but only a little):

#define MAX_QUERY_STRING_LENGTH (16 * 1024 * 1024)

/* Read a single query string from STDIN, using 'ctx' as the talloc
 * owner for all allocations.
 *
 * This function returns NULL in case of insufficient memory or read
 * errors.
 */
static char *
query_string_from_stdin (void *ctx)
{
    char *query_string = NULL, *new_qs;
    size_t pos = 0, end = 0;
    ssize_t got;

    for (;;) {
	if (end - pos < 512) {
	    end = MAX(end * 2, 1024);
	    if (end >= MAX_QUERY_STRING_LENGTH) {
		fprintf (stderr, "Error: query too long\n");
		goto FAIL;
	    }
	    new_qs = talloc_realloc (ctx, query_string, char, end);
	    if (new_qs == NULL)
		goto FAIL;
	    query_string = new_qs;
	}

	got = read (STDIN_FILENO, query_string + pos, end - pos - 1);
	if (got == 0)
	    break;
	if (got < 0) {
	   if (errno == EINTR)
	       continue;
	   fprintf (stderr, "Error: reading from standard input: %s\n",
		    strerror (errno));
	   goto FAIL;
	}
	pos += got;
    }

    query_string[pos] = '\0';
    return query_string;

 FAIL:
    talloc_free (query_string);
    return NULL;
}

>  /* Construct a single query string from the passed arguments, using
>   * 'ctx' as the talloc owner for all allocations.
>   *
> @@ -35,6 +73,9 @@ query_string_from_args (void *ctx, int argc, char *argv[])
>      char *query_string;
>      int i;
>  
> +    if ((argc == 1) && (strcmp ("-", argv[0]) == 0))
> +	return query_string_from_stdin (ctx);
> +
>      query_string = talloc_strdup (ctx, "");
>      if (query_string == NULL)
>  	return NULL;

Re: [PATCH v2 4/7] emacs: make emacs tagging use the stdin query functionality

[Austin Clements]

Quoth markwalters1009 on Nov 24 at  1:20 pm:
> From: Mark Walters <markwalters1009@gmail.com>
> 
> In preparation for the use of large queries in some cases make tagging
> from emacs use the new query on stdin functionality. Currently uses
> this for all tagging (as I could not see a reason not to).
> ---
>  emacs/notmuch-tag.el |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 4fce3a9..4634b0d 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -59,9 +59,10 @@ the messages that were tagged"
>        (setq search-terms (list "*")))
>    (split-string
>     (with-output-to-string
> -     (with-current-buffer standard-output
> -       (apply 'call-process notmuch-command nil t
> -	      nil "search" "--output=tags" "--exclude=false" search-terms)))
> +     (with-temp-buffer
> +       (insert (mapconcat 'identity search-terms " "))

#'identity ?

> +       (apply 'call-process-region (point-min) (point-max) notmuch-command nil

#'call-process-region ?

> +	      standard-output nil "search" "--output=tags" "--exclude=false" (list "-"))))

If you use funcall instead of apply here, you won't need to put "-" in
a list.

Also, the lines seem a little long (but maybe that's just diff and
quoting?)

>     "\n+" t))
>  
>  (defun notmuch-select-tag-with-completion (prompt &rest search-terms)
> @@ -134,8 +135,11 @@ 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)))
> +    (with-temp-buffer
> +      (insert query)
> +      (apply 'notmuch-call-notmuch-process-region

#'notmuch-call-notmuch-process-region ?

> +	     (point-min) (point-max)
> +	     "tag" (append tag-changes (list "--" "-"))))
>      (run-hooks 'notmuch-after-tag-hook))
>    ;; in all cases we return tag-changes as a list
>    tag-changes)

Re: [PATCH v2 6/7] cli: allow search mode to include msg-ids with JSON output

[Tomi Ollila]

On Sat, Nov 24 2012, markwalters1009 wrote:

> From: Mark Walters <markwalters1009@gmail.com>
>
> This adds a --queries=true option which modifies the summary output of
> notmuch search by including two extra query strings with each result:
> one query string specifies all matching messages and one query string
> all non-matching messages. Currently these are just lists of message
> ids joined with " or " but that could change in future.


Please see (mostly formatting) comments inline below.


> Currently this is not implemented for text format.
> ---
>  notmuch-search.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 89 insertions(+), 6 deletions(-)
>
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 830c4e4..c8fc9a6 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -26,7 +26,8 @@ typedef enum {
>      OUTPUT_THREADS,
>      OUTPUT_MESSAGES,
>      OUTPUT_FILES,
> -    OUTPUT_TAGS
> +    OUTPUT_TAGS,
> +    OUTPUT_SUMMARY_WITH_QUERIES
>  } output_t;
>  
>  static char *
> @@ -46,6 +47,57 @@ sanitize_string (const void *ctx, const char *str)
>      return out;
>  }
>  
> +/* This function takes a message id and returns an escaped string
> + * which can be used as a Xapian query. This involves prefixing with
> + * `id:', putting the id inside double quotes, and doubling any
> + * occurence of a double quote in the message id itself.*/
> +static char *
> +xapian_escape_id (const void *ctx,
> +	   const char *msg_id)

second line indentation, not at '(' level as elsewhere

> +{
> +    const char *c;
> +    char *escaped_msg_id;
> +    escaped_msg_id = talloc_strdup (ctx, "id:\"");
> +    for (c=msg_id; *c; c++)

spacing above

> +	if (*c == '"')
> +	    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"\"");
> +	else
> +	    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "%c", *c);
> +    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"");
> +    return escaped_msg_id;
> +}

If Austin sees fit he can comment the O(...) of the addition of msgids
to query strings -- it would take quite an overhaul to the functionality
if the escaped msgid's were directly written to query string...

> +
> +static char *
> +output_msg_query (const void *ctx,
> +		sprinter_t *format,
> +		notmuch_bool_t matching,
> +		notmuch_bool_t first,
> +		notmuch_messages_t *messages)

indentation level above

> +{
> +    notmuch_message_t *message;
> +    char *query, *escaped_msg_id;
> +    query = talloc_strdup (ctx, "");
> +    for (;
> +	 notmuch_messages_valid (messages);
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	message = notmuch_messages_get (messages);
> +	if (notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) == matching) {
> +	    escaped_msg_id = xapian_escape_id (ctx, notmuch_message_get_message_id (message));
> +	    if (first) {
> +		query = talloc_asprintf_append (query, "%s", escaped_msg_id);
> +		first = FALSE;
> +	    }
> +	    else
> +		query = talloc_asprintf_append (query, " or %s", escaped_msg_id);
> +	    talloc_free (escaped_msg_id);
> +	}
> +	/* output_msg_query already starts with an ` or' */
> +	query = talloc_asprintf_append (query, "%s", output_msg_query (ctx, format, matching, first, notmuch_message_get_replies (message)));
> +    }
> +    return query;
> +}
> +
>  static int
>  do_search_threads (sprinter_t *format,
>  		   notmuch_query_t *query,
> @@ -88,7 +140,7 @@ do_search_threads (sprinter_t *format,
>  	    format->string (format,
>  			    notmuch_thread_get_thread_id (thread));
>  	    format->separator (format);
> -	} else { /* output == OUTPUT_SUMMARY */
> +	} else { /* output == OUTPUT_SUMMARY or OUTPUT_SUMMARY_WITH_QUERIES */
>  	    void *ctx_quote = talloc_new (thread);
>  	    const char *authors = notmuch_thread_get_authors (thread);
>  	    const char *subject = notmuch_thread_get_subject (thread);
> @@ -108,7 +160,7 @@ do_search_threads (sprinter_t *format,
>  	    relative_date = notmuch_time_relative_date (ctx_quote, date);
>  
>  	    if (format->is_text_printer) {
> -                /* Special case for the text formatter */
> +               /* Special case for the text formatter */

irrelevant spacing change

>  		printf ("thread:%s %12s [%d/%d] %s; %s (",
>  			thread_id,
>  			relative_date,
> @@ -133,8 +185,6 @@ do_search_threads (sprinter_t *format,
>  		format->string (format, subject);
>  	    }
>  
> -	    talloc_free (ctx_quote);
> -
>  	    format->map_key (format, "tags");
>  	    format->begin_list (format);
>  
> @@ -145,7 +195,7 @@ do_search_threads (sprinter_t *format,
>  		const char *tag = notmuch_tags_get (tags);
>  
>  		if (format->is_text_printer) {
> -                  /* Special case for the text formatter */
> +		    /* Special case for the text formatter */

irrelevant spacing change

>  		    if (first_tag)
>  			first_tag = FALSE;
>  		    else
> @@ -160,8 +210,25 @@ do_search_threads (sprinter_t *format,
>  		printf (")");
>  
>  	    format->end (format);
> +
> +	    if (output == OUTPUT_SUMMARY_WITH_QUERIES) {
> +		char *query;
> +		query = output_msg_query (ctx_quote, format, TRUE, TRUE, notmuch_thread_get_toplevel_messages (thread));
> +		if (strlen (query)) {
> +		    format->map_key (format, "matching_msg_query");
> +		    format->string (format, query);
> +		}
> +		query = output_msg_query (ctx_quote, format, FALSE, TRUE, notmuch_thread_get_toplevel_messages (thread));
> +		if (strlen (query)) {
> +		    format->map_key (format, "nonmatching_msg_query");
> +		    format->string (format, query);
> +		}
> +	    }
> +
>  	    format->end (format);
>  	    format->separator (format);
> +
> +	    talloc_free (ctx_quote);
>  	}
>  
>  	notmuch_thread_destroy (thread);
> @@ -303,6 +370,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      int offset = 0;
>      int limit = -1; /* unlimited */
>      int exclude = EXCLUDE_TRUE;
> +    notmuch_bool_t with_queries = FALSE;
>      unsigned int i;
>  
>      enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
> @@ -323,12 +391,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  				  { "messages", OUTPUT_MESSAGES },
>  				  { "files", OUTPUT_FILES },
>  				  { "tags", OUTPUT_TAGS },
> +				  { "with-queries", OUTPUT_SUMMARY_WITH_QUERIES },

this "with-queries" confuses me -- old version or undocumented feature ?

>  				  { 0, 0 } } },
>          { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
>            (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
>                                    { "false", EXCLUDE_FALSE },
>                                    { "flag", EXCLUDE_FLAG },
>                                    { 0, 0 } } },
> +        { NOTMUCH_OPT_BOOLEAN, &with_queries, "queries", 'b', 0 },
>  	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
>  	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
>  	{ 0, 0, 0, 0, 0 }
> @@ -398,6 +468,19 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  	    notmuch_query_set_omit_excluded (query, FALSE);
>      }
>  
> +    if (with_queries) {
> +	if (format_sel == NOTMUCH_FORMAT_TEXT) {
> +	    fprintf (stderr, "Warning: --queries=true not implemented for text format.\n");
> +	    with_queries = FALSE;
> +	}
> +	if (output != OUTPUT_SUMMARY) {
> +	    fprintf (stderr, "Warning: --queries=true only implemented for --output=summary.\n");
> +	    with_queries = FALSE;
> +	}
> +    }
> +
> +    if (with_queries) output = OUTPUT_SUMMARY_WITH_QUERIES;

newline before output = ...

> +
>      switch (output) {
>      default:
>      case OUTPUT_SUMMARY:
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 1/7] cli: allow query to come from stdin

[Tomi Ollila]

On Sat, Nov 24 2012, markwalters1009 wrote:

> From: Mark Walters <markwalters1009@gmail.com>
>
> After this series there will be times when a caller will want to pass
> a very large query string to notmuch (eg a list of 10,000 message-ids)
> and this can exceed the size of ARG_MAX. Hence allow notmuch to take
> the query from stdin (if the query is -).


> ---
>  query-string.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/query-string.c b/query-string.c
> index 6536512..b1fbdeb 100644
> --- a/query-string.c
> +++ b/query-string.c
> @@ -20,6 +20,44 @@
>  
>  #include "notmuch-client.h"
>  
> +/* Read a single query string from STDIN, using
> + * 'ctx' as the talloc owner for all allocations.
> + *
> + * This function returns NULL in case of insufficient memory or read
> + * errors.
> + */
> +static char *
> +query_string_from_stdin (void *ctx)

Austin provided pretty nice alternative implementation of 
query_string_from_stdin() in his reply so I decline to
comment minor formatting issue below :D

> +{
> +    char *query_string;
> +    char buf[4096];
> +    ssize_t remain;
> +
> +    query_string = talloc_strdup (ctx, "");
> +    if (query_string == NULL)
> +	return NULL;
> +
> +    for (;;) {
> +	remain = read (STDIN_FILENO, buf, sizeof(buf) - 1);
> +	if (remain == 0)
> +	    break;
> +	if (remain < 0) {
> +	    if (errno == EINTR)
> +		continue;
> +	    fprintf (stderr, "Error: reading from standard input: %s\n",
> +		     strerror (errno));
> +	    return NULL;
> +	}
> +
> +	buf[remain] = '\0';
> +	query_string = talloc_strdup_append (query_string, buf);
> +	if (query_string == NULL)
> +	    return NULL;
> +    }
> +
> +    return query_string;
> +}
> +
>  /* Construct a single query string from the passed arguments, using
>   * 'ctx' as the talloc owner for all allocations.
>   *
> @@ -35,6 +73,9 @@ query_string_from_args (void *ctx, int argc, char *argv[])
>      char *query_string;
>      int i;
>  
> +    if ((argc == 1) && (strcmp ("-", argv[0]) == 0))

the argument order in strcmp() is not consistent with all the 
other uses of strcmp () in the codebase.

> +	return query_string_from_stdin (ctx);
> +
>      query_string = talloc_strdup (ctx, "");
>      if (query_string == NULL)
>  	return NULL;
> -- 
> 1.7.9.1

Tomi

Re: [PATCH v2 6/7] cli: allow search mode to include msg-ids with JSON output

[Austin Clements]

Quoth markwalters1009 on Nov 24 at  1:20 pm:
> From: Mark Walters <markwalters1009@gmail.com>
> 
> This adds a --queries=true option which modifies the summary output of
> notmuch search by including two extra query strings with each result:
> one query string specifies all matching messages and one query string
> all non-matching messages. Currently these are just lists of message
> ids joined with " or " but that could change in future.
> 
> Currently this is not implemented for text format.
> ---
>  notmuch-search.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 830c4e4..c8fc9a6 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -26,7 +26,8 @@ typedef enum {
>      OUTPUT_THREADS,
>      OUTPUT_MESSAGES,
>      OUTPUT_FILES,
> -    OUTPUT_TAGS
> +    OUTPUT_TAGS,
> +    OUTPUT_SUMMARY_WITH_QUERIES
>  } output_t;
>  
>  static char *
> @@ -46,6 +47,57 @@ sanitize_string (const void *ctx, const char *str)
>      return out;
>  }
>  
> +/* This function takes a message id and returns an escaped string
> + * which can be used as a Xapian query. This involves prefixing with
> + * `id:', putting the id inside double quotes, and doubling any
> + * occurence of a double quote in the message id itself.*/
> +static char *
> +xapian_escape_id (const void *ctx,
> +	   const char *msg_id)
> +{
> +    const char *c;
> +    char *escaped_msg_id;
> +    escaped_msg_id = talloc_strdup (ctx, "id:\"");

talloc_strdup can fail.

> +    for (c=msg_id; *c; c++)

Missing spaces around =.

> +	if (*c == '"')
> +	    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"\"");
> +	else
> +	    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "%c", *c);

.. as can talloc_asprintf_append.

> +    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"");
> +    return escaped_msg_id;
> +}

Unfortunately, this approach will cause reallocation and copying for
every character in msg_id (as well as requiring talloc_asprintf_append
to re-scan escaped_msg_id to find the end on every iteration).  How
about pre-allocating a large enough buffer and keeping your position
in it, like

/* This function takes a message id and returns an escaped string
 * which can be used as a Xapian query. This involves prefixing with
 * `id:', putting the id inside double quotes, and doubling any
 * occurence of a double quote in the message id itself. Returns NULL
 * if memory allocation fails. */
static char *
xapian_escape_id (const void *ctx,
		  const char *msg_id)
{
    const char *in;
    char *out;
    char *escaped_msg_id = talloc_array (ctx, char, 6 + strlen (msg_id) * 2);
    if (!escaped_msg_id)
	return NULL;
    strcpy (escaped_msg_id, "id:\"");
    out = escaped_msg_id + 4;
    for (in = msg_id; *in; ++in) {
	if (*in == '"')
	    *(out++) = '"';
	*(out++) = *in;
    }
    strcpy(out, "\"");
    return escaped_msg_id;
}

> +
> +static char *
> +output_msg_query (const void *ctx,
> +		sprinter_t *format,
> +		notmuch_bool_t matching,
> +		notmuch_bool_t first,
> +		notmuch_messages_t *messages)
> +{
> +    notmuch_message_t *message;
> +    char *query, *escaped_msg_id;
> +    query = talloc_strdup (ctx, "");
> +    for (;
> +	 notmuch_messages_valid (messages);
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	message = notmuch_messages_get (messages);
> +	if (notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) == matching) {
> +	    escaped_msg_id = xapian_escape_id (ctx, notmuch_message_get_message_id (message));

Two long lines.

> +	    if (first) {
> +		query = talloc_asprintf_append (query, "%s", escaped_msg_id);
> +		first = FALSE;
> +	    }
> +	    else

"} else".

> +		query = talloc_asprintf_append (query, " or %s", escaped_msg_id);

The "or" is unnecessary, since id is registered with the query parser
as an exclusive boolean term.

You could simplify this to

  query = talloc_asprintf_append (query, "%s%s", first ? "" : " ", 
                                  escaped_msg_id);

Technically this loop has the same O(n^2) problem as xapian_escape_id
and query_string_from_args, but given that threads rarely have more
than a few dozen messages in them, perhaps it doesn't matter.  OTOH,
this may deal poorly with pathological threads (autogenerated messages
and such).

I wonder if we should have some simple linear-time talloc string
accumulation abstraction in util/...

> +	    talloc_free (escaped_msg_id);
> +	}
> +	/* output_msg_query already starts with an ` or' */
> +	query = talloc_asprintf_append (query, "%s", output_msg_query (ctx, format, matching, first, notmuch_message_get_replies (message)));

Oof, how unfortunate.  I've got a patch that adds an iterator over all
of the messages in a thread, which would make this much simpler (I
don't know how we've gotten this far without such an API).  I'll clean
that up and send it.  This shouldn't block this patch, but whichever
goes in second should clean this up.

Also, long line.

> +    }
> +    return query;
> +}
> +
>  static int
>  do_search_threads (sprinter_t *format,
>  		   notmuch_query_t *query,
> @@ -88,7 +140,7 @@ do_search_threads (sprinter_t *format,
>  	    format->string (format,
>  			    notmuch_thread_get_thread_id (thread));
>  	    format->separator (format);
> -	} else { /* output == OUTPUT_SUMMARY */
> +	} else { /* output == OUTPUT_SUMMARY or OUTPUT_SUMMARY_WITH_QUERIES */
>  	    void *ctx_quote = talloc_new (thread);
>  	    const char *authors = notmuch_thread_get_authors (thread);
>  	    const char *subject = notmuch_thread_get_subject (thread);
> @@ -108,7 +160,7 @@ do_search_threads (sprinter_t *format,
>  	    relative_date = notmuch_time_relative_date (ctx_quote, date);
>  
>  	    if (format->is_text_printer) {
> -                /* Special case for the text formatter */
> +               /* Special case for the text formatter */

Unintentional whitespace change?  (Actually, this line isn't indented
with tabs either before or after this change; how'd that happen?)

>  		printf ("thread:%s %12s [%d/%d] %s; %s (",
>  			thread_id,
>  			relative_date,
> @@ -133,8 +185,6 @@ do_search_threads (sprinter_t *format,
>  		format->string (format, subject);
>  	    }
>  
> -	    talloc_free (ctx_quote);
> -
>  	    format->map_key (format, "tags");
>  	    format->begin_list (format);
>  
> @@ -145,7 +195,7 @@ do_search_threads (sprinter_t *format,
>  		const char *tag = notmuch_tags_get (tags);
>  
>  		if (format->is_text_printer) {
> -                  /* Special case for the text formatter */
> +		    /* Special case for the text formatter */

Same?  Looks like here you converted it to tabs.

>  		    if (first_tag)
>  			first_tag = FALSE;
>  		    else
> @@ -160,8 +210,25 @@ do_search_threads (sprinter_t *format,
>  		printf (")");
>  
>  	    format->end (format);
> +
> +	    if (output == OUTPUT_SUMMARY_WITH_QUERIES) {
> +		char *query;
> +		query = output_msg_query (ctx_quote, format, TRUE, TRUE, notmuch_thread_get_toplevel_messages (thread));
> +		if (strlen (query)) {
> +		    format->map_key (format, "matching_msg_query");

Maybe just matching_query?

> +		    format->string (format, query);
> +		}
> +		query = output_msg_query (ctx_quote, format, FALSE, TRUE, notmuch_thread_get_toplevel_messages (thread));
> +		if (strlen (query)) {
> +		    format->map_key (format, "nonmatching_msg_query");

nonmatching_query?

Also, don't forget to update devel/schema.

> +		    format->string (format, query);
> +		}
> +	    }
> +
>  	    format->end (format);
>  	    format->separator (format);
> +
> +	    talloc_free (ctx_quote);
>  	}
>  
>  	notmuch_thread_destroy (thread);
> @@ -303,6 +370,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      int offset = 0;
>      int limit = -1; /* unlimited */
>      int exclude = EXCLUDE_TRUE;
> +    notmuch_bool_t with_queries = FALSE;
>      unsigned int i;
>  
>      enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
> @@ -323,12 +391,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  				  { "messages", OUTPUT_MESSAGES },
>  				  { "files", OUTPUT_FILES },
>  				  { "tags", OUTPUT_TAGS },
> +				  { "with-queries", OUTPUT_SUMMARY_WITH_QUERIES },

Was this intentional?

>  				  { 0, 0 } } },
>          { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
>            (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
>                                    { "false", EXCLUDE_FALSE },
>                                    { "flag", EXCLUDE_FLAG },
>                                    { 0, 0 } } },
> +        { NOTMUCH_OPT_BOOLEAN, &with_queries, "queries", 'b', 0 },

Wrong indentation (since the next line is indented correctly you can
be both locally and globally consistent).

>  	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
>  	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
>  	{ 0, 0, 0, 0, 0 }
> @@ -398,6 +468,19 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  	    notmuch_query_set_omit_excluded (query, FALSE);
>      }
>  
> +    if (with_queries) {
> +	if (format_sel == NOTMUCH_FORMAT_TEXT) {
> +	    fprintf (stderr, "Warning: --queries=true not implemented for text format.\n");
> +	    with_queries = FALSE;
> +	}
> +	if (output != OUTPUT_SUMMARY) {
> +	    fprintf (stderr, "Warning: --queries=true only implemented for --output=summary.\n");
> +	    with_queries = FALSE;
> +	}
> +    }
> +
> +    if (with_queries) output = OUTPUT_SUMMARY_WITH_QUERIES;

Out of curiosity, why do this as a separate output type, rather than
just passing a with_queries flag into do_search_threads?

> +
>      switch (output) {
>      default:
>      case OUTPUT_SUMMARY:

Re: [PATCH v2 7/7] emacs: make emacs use message-ids for tagging

[Austin Clements]

Quoth markwalters1009 on Nov 24 at  1:20 pm:
> From: Mark Walters <markwalters1009@gmail.com>
> 
> This makes emacs use the new --queries=true in search mode and use
> this for tagging.  This fixes the race condition in tagging from
> search mode so mark the tests fixed.
> ---
>  emacs/notmuch.el |   28 +++++++++++++++++++++++++---
>  test/emacs       |    2 --
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 64b9474..6e8ef83 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -473,7 +473,8 @@ BEG."
>    (let (output)
>      (notmuch-search-foreach-result beg end
>        (lambda (pos)
> -	(push (plist-get (notmuch-search-get-result pos) property) output)))
> +	(let ((value (plist-get (notmuch-search-get-result pos) property)))
> +	  (when value (push value output)))))

Why is this necessary?  (Assuming it is, it could use a comment, and
probably an update to the docstring.)

>      output))
>  
>  (defun notmuch-search-find-thread-id (&optional bare)
> @@ -483,6 +484,7 @@ 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))))
>  
> +

Unintentional?

>  (defun notmuch-search-find-thread-id-region (beg end)
>    "Return a list of threads for the current region"
>    (mapcar (lambda (thread) (concat "thread:" thread))
> @@ -492,6 +494,23 @@ If BARE is set then do not prefix with \"thread:\""
>    "Return a search string for threads for the current region"
>    (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or "))
>  
> +;; The following two functions are similar to the previous two but
> +;; they only match messages that were in the the thread when the
> +;; initial search was run. This means that they can be used where it
> +;; is important to avoid races: e.g. when tagging.
> +(defun notmuch-search-find-queries-region (beg end &optional only-matching)
> +  (interactive)
> +  "Return a list of queries for the current region"
> +  (append (notmuch-search-properties-in-region :matching_msg_query beg end)
> +	  (unless only-matching
> +	    (notmuch-search-properties-in-region :nonmatching_msg_query beg end))))

Two minor performance nits: Using nconc instead of append will avoid
copying the first list and swapping the two arguments will avoid
needlessly traversing the non-matching list when only-matching is nil.

> +
> +(defun notmuch-search-find-queries-region-search (beg end &optional only-matching)
> +  "Return a search string for messages in threads in the current region"
> +  (mapconcat 'identity

#'identity

> +	     (notmuch-search-find-queries-region beg end only-matching)
> +	     " or "))
> +
>  (defun notmuch-search-find-authors ()
>    "Return the authors for the current thread"
>    (plist-get (notmuch-search-get-result) :authors))
> @@ -575,7 +594,7 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>  
>  (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)))
> +  (let ((search-string (notmuch-search-find-queries-region-search beg end)))
>      (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
>      (notmuch-search-foreach-result beg end
>        (lambda (pos)
> @@ -851,7 +870,9 @@ 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))
> +  (apply 'notmuch-tag (notmuch-search-find-queries-region-search
> +		       (point-min) (point-max) t)
> +	 tag-changes))
>  
>  (defun notmuch-search-buffer-title (query)
>    "Returns the title for a buffer with notmuch search results."
> @@ -948,6 +969,7 @@ Other optional parameters are used as follows:
>  		     "notmuch-search" buffer
>  		     notmuch-command "search"
>  		     "--format=json"
> +		     "--output=with-queries"

--output=with-queries or --queries=true?

>  		     (if oldest-first
>  			 "--sort=oldest-first"
>  		       "--sort=newest-first")
> diff --git a/test/emacs b/test/emacs
> index 3788439..132768f 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -123,7 +123,6 @@ 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 "Tag all matching messages from search view"
> -test_subtest_known_broken
>  notmuch tag +test-tag-race from:cworth
>  test_emacs "(notmuch-search \"tag:test-tag-race\")
>  	    (notmuch-test-wait)"
> @@ -135,7 +134,6 @@ notmuch tag -test-tag-race '*'
>  notmuch tag -test-tag-race-2 '*'
>  
>  test_begin_subtest "Change tags from search view: another message arriving after thread lookup"
> -test_subtest_known_broken
>  typsos_id="878we4qdqf.fsf@yoom.home.cworth.org"
>  typsos_thread=$(notmuch search --output=threads id:$typsos_id)
>  test_emacs "(notmuch-search \"$typsos_thread\")

Re: [PATCH v2 1/7] cli: allow query to come from stdin

[Mark Walters]

Hi

Many thanks for all the reviews: I have incorporated most of your and
Tomi's suggestions in my latest version. However, for this patch I
wonder whether just using David's batch tagging would be sufficient. It
does mean that I can't construct the list of possible tag removals
correctly for a large query but I can just return all tags in this
case. I think this is probably an acceptable trade off: you don't get
the correct list of possible completions if you are tagging more than
5000 messages at once.

This patch is not very complicated but it does add another
feature/option to the command line so if it is not needed I am inclined
not to add it. If people think that being able to do searches for
queries in excess of ARGMAX (possible 2MB or so) is useful then we could
add it.

Incidentally for the tag completions: my view is the correct thing is to
offer completions (for tag removal) based on what tags the buffer shows
(ie what was there when the query was run) rather than what is actually
tags are present now: this would be easy to add if anyone cared
sufficiently.

Any thoughts?

Best wishes

Mark

Austin Clements <amdragon@MIT.EDU> writes:

> Quoth markwalters1009 on Nov 24 at  1:20 pm:
>> From: Mark Walters <markwalters1009@gmail.com>
>> 
>> After this series there will be times when a caller will want to pass
>> a very large query string to notmuch (eg a list of 10,000 message-ids)
>> and this can exceed the size of ARG_MAX. Hence allow notmuch to take
>> the query from stdin (if the query is -).
>> ---
>>  query-string.c |   41 +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 41 insertions(+), 0 deletions(-)
>> 
>> diff --git a/query-string.c b/query-string.c
>> index 6536512..b1fbdeb 100644
>> --- a/query-string.c
>> +++ b/query-string.c
>> @@ -20,6 +20,44 @@
>>  
>>  #include "notmuch-client.h"
>>  
>> +/* Read a single query string from STDIN, using
>> + * 'ctx' as the talloc owner for all allocations.
>> + *
>> + * This function returns NULL in case of insufficient memory or read
>> + * errors.
>> + */
>> +static char *
>> +query_string_from_stdin (void *ctx)
>> +{
>> +    char *query_string;
>> +    char buf[4096];
>> +    ssize_t remain;
>> +
>> +    query_string = talloc_strdup (ctx, "");
>> +    if (query_string == NULL)
>> +	return NULL;
>> +
>> +    for (;;) {
>> +	remain = read (STDIN_FILENO, buf, sizeof(buf) - 1);
>> +	if (remain == 0)
>> +	    break;
>> +	if (remain < 0) {
>> +	    if (errno == EINTR)
>> +		continue;
>> +	    fprintf (stderr, "Error: reading from standard input: %s\n",
>> +		     strerror (errno));
>
> talloc_free (query_string) ?
>
>> +	    return NULL;
>> +	}
>> +
>> +	buf[remain] = '\0';
>> +	query_string = talloc_strdup_append (query_string, buf);
>
> Eliminate the NUL in buf and instead
>  talloc_strndup_append (query_string, buf, remain) ?
>
> Should there be some (large) bound on the size of the query string to
> prevent runaway?
>
>> +	if (query_string == NULL)
>
> Technically it would be good to talloc_free the old pointer here, too.
>
>> +	    return NULL;
>> +    }
>> +
>> +    return query_string;
>> +}
>> +
>
> This whole approach is O(n^2), which might actually matter for large
> query strings.  How about (tested, but only a little):
>
> #define MAX_QUERY_STRING_LENGTH (16 * 1024 * 1024)
>
> /* Read a single query string from STDIN, using 'ctx' as the talloc
>  * owner for all allocations.
>  *
>  * This function returns NULL in case of insufficient memory or read
>  * errors.
>  */
> static char *
> query_string_from_stdin (void *ctx)
> {
>     char *query_string = NULL, *new_qs;
>     size_t pos = 0, end = 0;
>     ssize_t got;
>
>     for (;;) {
> 	if (end - pos < 512) {
> 	    end = MAX(end * 2, 1024);
> 	    if (end >= MAX_QUERY_STRING_LENGTH) {
> 		fprintf (stderr, "Error: query too long\n");
> 		goto FAIL;
> 	    }
> 	    new_qs = talloc_realloc (ctx, query_string, char, end);
> 	    if (new_qs == NULL)
> 		goto FAIL;
> 	    query_string = new_qs;
> 	}
>
> 	got = read (STDIN_FILENO, query_string + pos, end - pos - 1);
> 	if (got == 0)
> 	    break;
> 	if (got < 0) {
> 	   if (errno == EINTR)
> 	       continue;
> 	   fprintf (stderr, "Error: reading from standard input: %s\n",
> 		    strerror (errno));
> 	   goto FAIL;
> 	}
> 	pos += got;
>     }
>
>     query_string[pos] = '\0';
>     return query_string;
>
>  FAIL:
>     talloc_free (query_string);
>     return NULL;
> }
>
>>  /* Construct a single query string from the passed arguments, using
>>   * 'ctx' as the talloc owner for all allocations.
>>   *
>> @@ -35,6 +73,9 @@ query_string_from_args (void *ctx, int argc, char *argv[])
>>      char *query_string;
>>      int i;
>>  
>> +    if ((argc == 1) && (strcmp ("-", argv[0]) == 0))
>> +	return query_string_from_stdin (ctx);
>> +
>>      query_string = talloc_strdup (ctx, "");
>>      if (query_string == NULL)
>>  	return NULL;