[PATCH 0/8] JSON-based search-mode

[PATCH 0/8] JSON-based search-mode

[Austin Clements]

This patch series replaces the text format parser used for search in
Emacs with a parser for the JSON format.  This should address the
escaping and flexibility problems that have plagued the text format.
Like the text format, it supports incremental output.

Patches 1-4 simply clean up the Emacs search code and could be pushed
before the rest of the series.  Patch 5 switches to the JSON plist
representation internally, but retains the text parser.  This requires
some changes to the text parser to keep things working, but don't get
too hung up on them since it's about to get replaced entirely.  Patch
6 adds a test.

Finally, patches 7 and 8 are the real meat.  Patch 7 introduces a
general incremental JSON parser.  For search, we could probably get
away with a simpler, hacky approach, but an incremental JSON parser is
the type of thing you only want to write once---hacky or not---and it
seems like the type of thing that could be useful elsewhere, too.
It's general enough to support things like incremental show buffer
rendering.

Patch 8 rewrites the search output parser to use the JSON format via
this incremental parser.

[PATCH 1/8] emacs: Clean up notmuch-search-show-result

[Austin Clements]

This simplifies the code and makes it no longer cubic in the number of
result fields.
---
 emacs/notmuch.el |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c6236db..be217a2 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -707,29 +707,29 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field date count authors subject tags)
+(defun notmuch-search-insert-field (field format date count authors subject tags)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
+    (insert (propertize (format format date)
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
+    (insert (propertize (format format count)
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
+    (insert (propertize (format format subject)
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
+    (notmuch-search-insert-authors format authors))
 
    ((string-equal field "tags")
-    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
+    (insert
+     (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))
 
 (defun notmuch-search-show-result (date count authors subject tags)
-  (let ((fields) (field))
-    (setq fields (mapcar 'car notmuch-search-result-format))
-    (loop for field in fields
-	  do (notmuch-search-insert-field field date count authors subject tags)))
+  (dolist (format notmuch-search-result-format)
+    (notmuch-search-insert-field (car format) (cdr format)
+				 date count authors subject tags))
   (insert "\n"))
 
 (defun notmuch-search-process-filter (proc string)
-- 
1.7.10

[PATCH 2/8] emacs: Separate search line parsing and display

[Austin Clements]

Previously, much of the display of search lines was done in the same
function that parsed the CLI's output.  Now the parsing function only
parses, and notmuch-search-show-result fully inserts the search result
in the search buffer.
---
 emacs/notmuch.el |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index be217a2..dadc6d6 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -726,11 +726,19 @@ non-authors is found, assume that all of the authors match."
     (insert
      (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))
 
-(defun notmuch-search-show-result (date count authors subject tags)
-  (dolist (format notmuch-search-result-format)
-    (notmuch-search-insert-field (car format) (cdr format)
-				 date count authors subject tags))
-  (insert "\n"))
+(defun notmuch-search-show-result (thread-id date count authors subject tags)
+  ;; Ignore excluded matches
+  (unless (eq (aref count 1) ?0)
+    (let ((beg (point))
+	  (tags-str (mapconcat 'identity tags " ")))
+      (dolist (format notmuch-search-result-format)
+	(notmuch-search-insert-field (car format) (cdr format)
+				     date count authors subject tags-str))
+      (insert "\n")
+      (notmuch-search-color-line beg (point) tags)
+      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
+      (put-text-property beg (point) 'notmuch-search-authors authors)
+      (put-text-property beg (point) 'notmuch-search-subject subject))))
 
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
@@ -758,17 +766,10 @@ non-authors is found, assume that all of the authors match."
 		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
 			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
-		      ;; We currently just throw away excluded matches.
-		      (unless (eq (aref count 1) ?0)
-			(let ((beg (point)))
-			  (notmuch-search-show-result date count authors subject tags)
-			  (notmuch-search-color-line beg (point) tag-list)
-			  (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-			  (put-text-property beg (point) 'notmuch-search-authors authors)
-			  (put-text-property beg (point) 'notmuch-search-subject subject)
-			  (when (string= thread-id notmuch-search-target-thread)
-			    (set 'found-target beg)
-			    (set 'notmuch-search-target-thread "found"))))
+		      (when (string= thread-id notmuch-search-target-thread)
+			(set 'found-target (point))
+			(set 'notmuch-search-target-thread "found"))
+		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.10

[PATCH 3/8] emacs: Move search-target logic to `notmuch-search-show-result'

[Austin Clements]

This is a simpler place to do this, since we can avoid any point
motion and hence any save-excursions in
`notmuch-search-process-filter', which in turn lets us put all of the
search-target logic outside of any save-excursions.

`notmuch-search-process-filter' could use some reindentation after
this, but we're about to rewrite it entirely, so we won't bother.
---
 emacs/notmuch.el |   33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index dadc6d6..4a6490a 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -729,24 +729,27 @@ non-authors is found, assume that all of the authors match."
 (defun notmuch-search-show-result (thread-id date count authors subject tags)
   ;; Ignore excluded matches
   (unless (eq (aref count 1) ?0)
-    (let ((beg (point))
+    (let ((beg (point-max))
 	  (tags-str (mapconcat 'identity tags " ")))
-      (dolist (format notmuch-search-result-format)
-	(notmuch-search-insert-field (car format) (cdr format)
-				     date count authors subject tags-str))
-      (insert "\n")
-      (notmuch-search-color-line beg (point) tags)
-      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-      (put-text-property beg (point) 'notmuch-search-authors authors)
-      (put-text-property beg (point) 'notmuch-search-subject subject))))
+      (save-excursion
+	(goto-char beg)
+	(dolist (format notmuch-search-result-format)
+	  (notmuch-search-insert-field (car format) (cdr format)
+				       date count authors subject tags-str))
+	(insert "\n")
+	(notmuch-search-color-line beg (point) tags)
+	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
+	(put-text-property beg (point) 'notmuch-search-authors authors)
+	(put-text-property beg (point) 'notmuch-search-subject subject))
+      (when (string= thread-id notmuch-search-target-thread)
+	(setq notmuch-search-target-thread "found")
+	(goto-char beg)))))
 
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
-  (let ((buffer (process-buffer proc))
-	(found-target nil))
+  (let ((buffer (process-buffer proc)))
     (if (buffer-live-p buffer)
 	(with-current-buffer buffer
-	  (save-excursion
 	    (let ((line 0)
 		  (more t)
 		  (inhibit-read-only t)
@@ -763,12 +766,8 @@ non-authors is found, assume that all of the authors match."
 			   (subject (match-string 5 string))
 			   (tags (match-string 6 string))
 			   (tag-list (if tags (save-match-data (split-string tags)))))
-		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
 			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
-		      (when (string= thread-id notmuch-search-target-thread)
-			(set 'found-target (point))
-			(set 'notmuch-search-target-thread "found"))
 		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
@@ -777,8 +776,6 @@ non-authors is found, assume that all of the authors match."
 		  (if (< line (length string))
 		      (setq notmuch-search-process-filter-data (substring string line)))
 		  ))))
-	  (if found-target
-	      (goto-char found-target)))
       (delete-process proc))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
-- 
1.7.10

[PATCH 4/8] emacs: Helper for reporting search parsing errors

[Austin Clements]

This removes the last bit of direct output from the parsing function.
With the parser now responsible solely for parsing, we can swap it out
for another parser.
---
 emacs/notmuch.el |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 4a6490a..a073367 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -745,6 +745,13 @@ non-authors is found, assume that all of the authors match."
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
 
+(defun notmuch-search-show-error (string &rest objects)
+  (save-excursion
+    (goto-char (point-max))
+    (insert "Error: Unexpected output from notmuch search:\n")
+    (insert (apply #'format string objects))
+    (insert "\n")))
+
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
   (let ((buffer (process-buffer proc)))
@@ -767,7 +774,8 @@ non-authors is found, assume that all of the authors match."
 			   (tags (match-string 6 string))
 			   (tag-list (if tags (save-match-data (split-string tags)))))
 		      (if (/= (match-beginning 1) line)
-			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
+			  (notmuch-search-show-error
+			   (substring string line (match-beginning 1))))
 		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
-- 
1.7.10

[PATCH 5/8] emacs: Pass plist to `notmuch-search-show-result'

[Austin Clements]

Rather than passing lots of arguments and then further passing those
to `notmuch-search-insert-field', pass a plist containing all of the
search result information.  This plist is compatible JSON format
search results.
---
 emacs/notmuch.el |   65 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index a073367..084cec6 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -707,41 +707,46 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field format date count authors subject tags)
+(defun notmuch-search-insert-field (field format result)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format format date)
+    (insert (propertize (format format (plist-get result :date_relative))
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format format count)
+    (insert (propertize (format format (format "[%s/%s]"
+					       (plist-get result :matched)
+					       (plist-get result :total)))
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format format subject)
+    (insert (propertize (format format (plist-get result :subject))
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors format authors))
+    (notmuch-search-insert-authors format (plist-get result :authors)))
 
    ((string-equal field "tags")
     (insert
-     (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))
+     (format format (propertize
+		     (mapconcat 'identity (plist-get result :tags) " ")
+		     'font-lock-face 'notmuch-tag-face))))))
 
-(defun notmuch-search-show-result (thread-id date count authors subject tags)
+(defun notmuch-search-show-result (result)
   ;; Ignore excluded matches
-  (unless (eq (aref count 1) ?0)
-    (let ((beg (point-max))
-	  (tags-str (mapconcat 'identity tags " ")))
+  (unless (= (plist-get result :matched) 0)
+    (let ((beg (point-max)))
       (save-excursion
 	(goto-char beg)
 	(dolist (format notmuch-search-result-format)
-	  (notmuch-search-insert-field (car format) (cdr format)
-				       date count authors subject tags-str))
+	  (notmuch-search-insert-field (car format) (cdr format) result))
 	(insert "\n")
-	(notmuch-search-color-line beg (point) tags)
-	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-	(put-text-property beg (point) 'notmuch-search-authors authors)
-	(put-text-property beg (point) 'notmuch-search-subject subject))
-      (when (string= thread-id notmuch-search-target-thread)
+	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-thread-id
+			   (concat "thread:" (plist-get result :thread)))
+	(put-text-property beg (point) 'notmuch-search-authors
+			   (plist-get result :authors))
+	(put-text-property beg (point) 'notmuch-search-subject
+			   (plist-get result :subject)))
+      (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
 
@@ -765,18 +770,24 @@ non-authors is found, assume that all of the authors match."
 	      (while more
 		(while (and (< line (length string)) (= (elt string line) ?\n))
 		  (setq line (1+ line)))
-		(if (string-match "^\\(thread:[0-9A-Fa-f]*\\) \\([^][]*\\) \\(\\[[0-9/]*\\]\\) \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
+		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
 		    (let* ((thread-id (match-string 1 string))
-			   (date (match-string 2 string))
-			   (count (match-string 3 string))
-			   (authors (match-string 4 string))
-			   (subject (match-string 5 string))
-			   (tags (match-string 6 string))
-			   (tag-list (if tags (save-match-data (split-string tags)))))
-		      (if (/= (match-beginning 1) line)
+			   (tags-str (match-string 7 string))
+			   (result (list :thread thread-id
+					 :date_relative (match-string 2 string)
+					 :matched (string-to-number
+						   (match-string 3 string))
+					 :total (string-to-number
+						 (match-string 4 string))
+					 :authors (match-string 5 string)
+					 :subject (match-string 6 string)
+					 :tags (if tags-str
+						   (save-match-data
+						     (split-string tags-str))))))
+		      (if (/= (match-beginning 0) line)
 			  (notmuch-search-show-error
-			   (substring string line (match-beginning 1))))
-		      (notmuch-search-show-result thread-id date count authors subject tag-list)
+			   (substring string line (match-beginning 0))))
+		      (notmuch-search-show-result result)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.10

[PATCH 6/8] test: New test for incremental search output parsing

[Austin Clements]

This advises the search process filter to make it process one
character at a time in order to test the pessimal case for incremental
search output parsing.

The text parser fails this test because it gets tricked into thinking
a parenthetical remark in a subject is the tag list.
---
 test/emacs       |   11 +++++++++++
 test/test-lib.el |    8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/test/emacs b/test/emacs
index e9f954c..293b12a 100755
--- a/test/emacs
+++ b/test/emacs
@@ -35,6 +35,17 @@ test_emacs '(notmuch-search "tag:inbox")
 	    (test-output)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
 
+test_begin_subtest "Incremental parsing of search results"
+test_subtest_known_broken
+test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
+	    (ad-activate 'notmuch-search-process-filter)
+	    (notmuch-search \"tag:inbox\")
+	    (notmuch-test-wait)
+	    (ad-disable-advice 'notmuch-search-process-filter 'around 'pessimal)
+	    (ad-activate 'notmuch-search-process-filter)
+	    (test-output)"
+test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
+
 test_begin_subtest "Navigation of notmuch-hello to search results"
 test_emacs '(notmuch-hello)
 	    (goto-char (point-min))
diff --git a/test/test-lib.el b/test/test-lib.el
index 6271da2..5dd6271 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -89,6 +89,14 @@ nothing."
 (add-hook-counter 'notmuch-hello-mode-hook)
 (add-hook-counter 'notmuch-hello-refresh-hook)
 
+(defadvice notmuch-search-process-filter (around pessimal activate disable)
+  "Feed notmuch-search-process-filter one character at a time."
+  (let ((string (ad-get-arg 1)))
+    (loop for char across string
+	  do (progn
+	       (ad-set-arg 1 (char-to-string char))
+	       ad-do-it))))
+
 (defmacro notmuch-test-run (&rest body)
   "Evaluate a BODY of test expressions and output the result."
   `(with-temp-buffer
-- 
1.7.10

[PATCH 7/8] emacs: Implement an incremental JSON parser

[Austin Clements]

This parser is designed to read streaming JSON whose structure is
known to the caller.  Like a typical JSON parsing interface, it
provides a function to read a complete JSON value from the input.
However, it extends this with an additional function that
requires the next value in the input to be a compound value and
descends into it, allowing its elements to be read one at a time
or further descended into.  Both functions can return 'retry to
indicate that not enough input is available.

The parser supports efficient partial parsing, so there's no need to
frame the input for correctness or performance.

Currently only descending into JSON lists is supported because that's
all we need, but support for descending into JSON objects can be added
in the future.
---
 emacs/notmuch-lib.el |  183 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index c829df3..f7cda33 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -23,6 +23,7 @@
 
 (require 'mm-view)
 (require 'mm-decode)
+(require 'json)
 (eval-when-compile (require 'cl))
 
 (defvar notmuch-command "notmuch"
@@ -296,6 +297,188 @@ was called."
 (defvar notmuch-show-process-crypto nil)
 (make-variable-buffer-local 'notmuch-show-process-crypto)
 
+;; Incremental JSON parsing
+
+(defun notmuch-json-create-parser (buffer)
+  "Return a streaming JSON parser that consumes input from BUFFER.
+
+This parser is designed to read streaming JSON whose structure is
+known to the caller.  Like a typical JSON parsing interface, it
+provides a function to read a complete JSON value from the input.
+However, it extends this with an additional function that
+requires the next value in the input to be a compound value and
+descends into it, allowing its elements to be read one at a time
+or further descended into.  Both functions can return 'retry to
+indicate that not enough input is available.
+
+The parser always consumes input from BUFFER's point.  Hence, the
+caller is allowed to delete and data before point and may
+resynchronize after an error by moving point."
+
+  (list buffer
+	;; Terminator stack: a stack of characters that indicate the
+	;; end of the compound values enclosing point
+	'()
+	;; Next: One of
+	;; * 'expect-value if the next token must be a value, but a
+	;;   value has not yet been reached
+	;; * 'value if point is at the beginning of a value
+	;; * 'expect-comma if the next token must be a comma
+	'expect-value
+	;; Allow terminator: non-nil if the next token may be a
+	;; terminator
+	nil
+	;; Partial parse position: If state is 'value, a marker for
+	;; the position of the partial parser or nil if no partial
+	;; parsing has happened yet
+	nil
+	;; Partial parse state: If state is 'value, the current
+	;; `parse-partial-sexp' state
+	nil))
+
+(defmacro notmuch-json-buffer (jp) `(first ,jp))
+(defmacro notmuch-json-term-stack (jp) `(second ,jp))
+(defmacro notmuch-json-next (jp) `(third ,jp))
+(defmacro notmuch-json-allow-term (jp) `(fourth ,jp))
+(defmacro notmuch-json-partial-pos (jp) `(fifth ,jp))
+(defmacro notmuch-json-partial-state (jp) `(sixth ,jp))
+
+(defvar notmuch-json-syntax-table
+  (let ((table (make-syntax-table)))
+    ;; The standard syntax table is what we need except that "." needs
+    ;; to have word syntax instead of punctuation syntax.
+    (modify-syntax-entry ?. "w" table)
+    table)
+  "Syntax table used for incremental JSON parsing.")
+
+(defun notmuch-json-scan-to-value (jp)
+  ;; Helper function that consumes separators, terminators, and
+  ;; whitespace from point.  Returns nil if it successfully reached
+  ;; the beginning of a value, 'end if it consumed a terminator, or
+  ;; 'retry if not enough input was available to reach a value.  Upon
+  ;; nil return, (notmuch-json-next jp) is always 'value.
+
+  (if (eq (notmuch-json-next jp) 'value)
+      ;; We're already at a value
+      nil
+    ;; Drive the state toward 'expect-value
+    (skip-chars-forward " \t\r\n")
+    (or (when (eobp) 'retry)
+	;; Test for the terminator for the current compound
+	(when (and (notmuch-json-allow-term jp)
+		   (eq (char-after) (car (notmuch-json-term-stack jp))))
+	  ;; Consume it and expect a comma or terminator next
+	  (forward-char)
+	  (setf (notmuch-json-term-stack jp) (cdr (notmuch-json-term-stack jp))
+		(notmuch-json-next jp) 'expect-comma
+		(notmuch-json-allow-term jp) t)
+	  'end)
+	;; Test for a separator
+	(when (eq (notmuch-json-next jp) 'expect-comma)
+	  (when (/= (char-after) ?,)
+	    (signal 'json-readtable-error (list "expected ','")))
+	  ;; Consume it, switch to 'expect-value, and disallow a
+	  ;; terminator
+	  (forward-char)
+	  (skip-chars-forward " \t\r\n")
+	  (setf (notmuch-json-next jp) 'expect-value
+		(notmuch-json-allow-term jp) nil)
+	  ;; We moved point, so test for eobp again and fall through
+	  ;; to the next test if there's more input
+	  (when (eobp) 'retry))
+	;; Next must be 'expect-value and we know this isn't
+	;; whitespace, EOB, or a terminator, so point must be on a
+	;; value
+	(progn
+	  (assert (eq (notmuch-json-next jp) 'expect-value))
+	  (setf (notmuch-json-next jp) 'value)
+	  nil))))
+
+(defun notmuch-json-begin-compound (jp)
+  "Parse the beginning of a compound value and traverse inside it.
+
+Returns 'retry if there is insufficient input to parse the
+beginning of the compound.  If this is able to parse the
+beginning of a compound, it returns t and later calls to
+`notmuch-json-read' will return the compound's elements.
+
+Entering JSON objects is current unimplemented."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    ;; Disallow terminators
+    (setf (notmuch-json-allow-term jp) nil)
+    (or (notmuch-json-scan-to-value jp)
+	(if (/= (char-after) ?\[)
+	    (signal 'json-readtable-error (list "expected '['"))
+	  (forward-char)
+	  (push ?\] (notmuch-json-term-stack jp))
+	  ;; Expect a value or terminator next
+	  (setf (notmuch-json-next jp) 'expect-value
+		(notmuch-json-allow-term jp) t)
+	  t))))
+
+(defun notmuch-json-read (jp)
+  "Parse the value at point in JP's buffer.
+
+Returns 'retry if there is insufficient input to parse a complete
+JSON value.  If the parser is currently inside a compound value
+and the next token ends the list or object, returns 'end.
+Otherwise, returns the value."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    (or
+     ;; Get to a value state
+     (notmuch-json-scan-to-value jp)
+
+     ;; Can we parse a complete value?
+     (let ((complete
+	    (if (looking-at "[-+0-9tfn]")
+		;; This is a number or a keyword, so the partial
+		;; parser isn't going to help us because a truncated
+		;; number or keyword looks like a complete symbol to
+		;; it.  Look for something that clearly ends it.
+		(save-excursion
+		  (skip-chars-forward "^]},: \t\r\n")
+		  (not (eobp)))
+
+	      ;; We're looking at a string, object, or array, which we
+	      ;; can partial parse.  If we just reached the value, set
+	      ;; up the partial parser.
+	      (when (null (notmuch-json-partial-state jp))
+		(setf (notmuch-json-partial-pos jp) (point-marker)))
+
+	      ;; Extend the partial parse until we either reach EOB or
+	      ;; get the whole value
+	      (save-excursion
+		(let ((pstate
+		       (with-syntax-table notmuch-json-syntax-table
+			 (parse-partial-sexp
+			  (notmuch-json-partial-pos jp) (point-max) 0 nil
+			  (notmuch-json-partial-state jp)))))
+		  ;; A complete value is available if we've reached
+		  ;; depth 0 or less and encountered a complete
+		  ;; subexpression.
+		  (if (and (<= (first pstate) 0) (third pstate))
+		      t
+		    ;; Not complete.  Update the partial parser state
+		    (setf (notmuch-json-partial-pos jp) (point-marker)
+			  (notmuch-json-partial-state jp) pstate)
+		    nil))))))
+
+       (if (not complete)
+	   'retry
+	 ;; We have a value.  Reset the partial parse state and expect
+	 ;; a comma or terminator after the value.
+	 (setf (notmuch-json-next jp) 'expect-comma
+	       (notmuch-json-allow-term jp) t
+	       (notmuch-json-partial-pos jp) nil
+	       (notmuch-json-partial-state jp) nil)
+	 ;; Parse the value
+	 (let* ((json-object-type 'plist)
+		(json-array-type 'list)
+		(json-false nil))
+	   (json-read)))))))
+
 (provide 'notmuch-lib)
 
 ;; Local Variables:
-- 
1.7.10

[PATCH 8/8] emacs: Switch from text to JSON format for search results

[Austin Clements]

The JSON format eliminates the complex escaping issues that have
plagued the text search format.  This uses the incremental JSON parser
so that, like the text parser, it can output search results
incrementally.

This slows down the parser by about ~4X, but puts us in a good
position to optimize either by improving the JSON parser (evidence
suggests this can reduce the overhead to ~40% over the text format) or
by switching to S-expressions (evidence suggests this will more than
double performance over the text parser).  [1]

This also fixes the incremental search parsing test.

[1] id:"20110720205007.GB21316@mit.edu"
---
 emacs/notmuch.el |  113 ++++++++++++++++++++++++++++++++----------------------
 test/emacs       |    1 -
 2 files changed, 67 insertions(+), 47 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 084cec6..2a09a98 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -60,7 +60,7 @@
 (require 'notmuch-message)
 
 (defcustom notmuch-search-result-format
-  `(("date" . "%s ")
+  `(("date" . "%12s ")
     ("count" . "%-7s ")
     ("authors" . "%-20s ")
     ("subject" . "%s ")
@@ -557,17 +557,14 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
-(defvar notmuch-search-process-filter-data nil
-  "Data that has not yet been processed.")
-(make-variable-buffer-local 'notmuch-search-process-filter-data)
-
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
 	(status (process-status proc))
 	(exit-status (process-exit-status proc))
 	(never-found-target-thread nil))
-    (if (memq status '(exit signal))
+    (when (memq status '(exit signal))
+	(kill-buffer (process-get proc 'parse-buf))
 	(if (buffer-live-p buffer)
 	    (with-current-buffer buffer
 	      (save-excursion
@@ -577,8 +574,6 @@ This function advances the next thread when finished."
 		  (if (eq status 'signal)
 		      (insert "Incomplete search results (search process was killed).\n"))
 		  (when (eq status 'exit)
-		    (if notmuch-search-process-filter-data
-			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
 		    (insert "End of search results.")
 		    (unless (= exit-status 0)
 		      (insert (format " (process returned %d)" exit-status)))
@@ -757,45 +752,62 @@ non-authors is found, assume that all of the authors match."
     (insert (apply #'format string objects))
     (insert "\n")))
 
+(defvar notmuch-search-process-state nil
+  "Parsing state of the search process filter.")
+
+(defvar notmuch-search-json-parser nil
+  "Incremental JSON parser for the search process filter.")
+
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
-  (let ((buffer (process-buffer proc)))
-    (if (buffer-live-p buffer)
-	(with-current-buffer buffer
-	    (let ((line 0)
-		  (more t)
-		  (inhibit-read-only t)
-		  (string (concat notmuch-search-process-filter-data string)))
-	      (setq notmuch-search-process-filter-data nil)
-	      (while more
-		(while (and (< line (length string)) (= (elt string line) ?\n))
-		  (setq line (1+ line)))
-		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
-		    (let* ((thread-id (match-string 1 string))
-			   (tags-str (match-string 7 string))
-			   (result (list :thread thread-id
-					 :date_relative (match-string 2 string)
-					 :matched (string-to-number
-						   (match-string 3 string))
-					 :total (string-to-number
-						 (match-string 4 string))
-					 :authors (match-string 5 string)
-					 :subject (match-string 6 string)
-					 :tags (if tags-str
-						   (save-match-data
-						     (split-string tags-str))))))
-		      (if (/= (match-beginning 0) line)
-			  (notmuch-search-show-error
-			   (substring string line (match-beginning 0))))
-		      (notmuch-search-show-result result)
-		      (set 'line (match-end 0)))
-		  (set 'more nil)
-		  (while (and (< line (length string)) (= (elt string line) ?\n))
-		    (setq line (1+ line)))
-		  (if (< line (length string))
-		      (setq notmuch-search-process-filter-data (substring string line)))
-		  ))))
-      (delete-process proc))))
+  (let ((results-buf (process-buffer proc))
+	(parse-buf (process-get proc 'parse-buf))
+	(inhibit-read-only t)
+	done)
+    (if (not (buffer-live-p results-buf))
+	(delete-process proc)
+      (with-current-buffer parse-buf
+	;; Insert new data
+	(save-excursion
+	  (goto-char (point-max))
+	  (insert string)))
+      (with-current-buffer results-buf
+	(while (not done)
+	  (condition-case nil
+	      (case notmuch-search-process-state
+		((begin)
+		 ;; Enter the results list
+		 (if (eq (notmuch-json-begin-compound
+			  notmuch-search-json-parser) 'retry)
+		     (setq done t)
+		   (setq notmuch-search-process-state 'result)))
+		((result)
+		 ;; Parse a result
+		 (let ((result (notmuch-json-read notmuch-search-json-parser)))
+		   (case result
+		     ((retry) (setq done t))
+		     ((end) (setq notmuch-search-process-state 'end))
+		     (otherwise (notmuch-search-show-result result)))))
+		((end)
+		 ;; Any trailing data is unexpected
+		 (with-current-buffer parse-buf
+		   (skip-chars-forward " \t\r\n")
+		   (if (eobp)
+		       (setq done t)
+		     (signal 'json-error nil)))))
+	    (json-error
+	     ;; Do our best to resynchronize and ensure forward
+	     ;; progress
+	     (notmuch-search-show-error
+	      "%s"
+	      (with-current-buffer parse-buf
+		(let ((bad (buffer-substring (line-beginning-position)
+					     (line-end-position))))
+		  (forward-line)
+		  bad))))))
+	;; Clear out what we've parsed
+	(with-current-buffer parse-buf
+	  (delete-region (point-min) (point)))))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
   "Add/remove tags from all messages in current search buffer.
@@ -898,10 +910,19 @@ Other optional parameters are used as follows:
 	(let ((proc (start-process
 		     "notmuch-search" buffer
 		     notmuch-command "search"
+		     "--format=json"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
-		     query)))
+		     query))
+	      ;; Use a scratch buffer to accumulate partial output.
+	      ;; This buffer will be killed by the sentinel, which
+	      ;; should be called no matter how the process dies.
+	      (parse-buf (generate-new-buffer " *notmuch search parse*")))
+	  (set (make-local-variable 'notmuch-search-process-state) 'begin)
+	  (set (make-local-variable 'notmuch-search-json-parser)
+	       (notmuch-json-create-parser parse-buf))
+	  (process-put proc 'parse-buf parse-buf)
 	  (set-process-sentinel proc 'notmuch-search-process-sentinel)
 	  (set-process-filter proc 'notmuch-search-process-filter)
 	  (set-process-query-on-exit-flag proc nil))))
diff --git a/test/emacs b/test/emacs
index 293b12a..afe35ba 100755
--- a/test/emacs
+++ b/test/emacs
@@ -36,7 +36,6 @@ test_emacs '(notmuch-search "tag:inbox")
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
 
 test_begin_subtest "Incremental parsing of search results"
-test_subtest_known_broken
 test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
 	    (ad-activate 'notmuch-search-process-filter)
 	    (notmuch-search \"tag:inbox\")
-- 
1.7.10

Re: [PATCH 1/8] emacs: Clean up notmuch-search-show-result

[Mark Walters]

On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This simplifies the code and makes it no longer cubic in the number of
> result fields.

This looks good to me and all tests pass, and I agree that this patch
can be pushed independently of the later patches in the series.

My one comment is that I, as a lisp beginner, found the use of "format"
as a function and a variable confusing (particularly as I had not come
across the dolist macro and that use really makes format look like a
function).

Best wishes

Mark

> ---
>  emacs/notmuch.el |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index c6236db..be217a2 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -707,29 +707,29 @@ non-authors is found, assume that all of the authors match."
>  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
>        (insert padding))))
>  
> -(defun notmuch-search-insert-field (field date count authors subject tags)
> +(defun notmuch-search-insert-field (field format date count authors subject tags)
>    (cond
>     ((string-equal field "date")
> -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
> +    (insert (propertize (format format date)
>  			'face 'notmuch-search-date)))
>     ((string-equal field "count")
> -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
> +    (insert (propertize (format format count)
>  			'face 'notmuch-search-count)))
>     ((string-equal field "subject")
> -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
> +    (insert (propertize (format format subject)
>  			'face 'notmuch-search-subject)))
>  
>     ((string-equal field "authors")
> -    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
> +    (notmuch-search-insert-authors format authors))
>  
>     ((string-equal field "tags")
> -    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
> +    (insert
> +     (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))
>  
>  (defun notmuch-search-show-result (date count authors subject tags)
> -  (let ((fields) (field))
> -    (setq fields (mapcar 'car notmuch-search-result-format))
> -    (loop for field in fields
> -	  do (notmuch-search-insert-field field date count authors subject tags)))
> +  (dolist (format notmuch-search-result-format)
> +    (notmuch-search-insert-field (car format) (cdr format)
> +				 date count authors subject tags))
>    (insert "\n"))
>  
>  (defun notmuch-search-process-filter (proc string)
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/8] emacs: Move search-target logic to `notmuch-search-show-result'

[Mark Walters]

On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This is a simpler place to do this, since we can avoid any point
> motion and hence any save-excursions in
> `notmuch-search-process-filter', which in turn lets us put all of the
> search-target logic outside of any save-excursions.
>
> `notmuch-search-process-filter' could use some reindentation after
> this, but we're about to rewrite it entirely, so we won't bother.
> ---
>  emacs/notmuch.el |   33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index dadc6d6..4a6490a 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -729,24 +729,27 @@ non-authors is found, assume that all of the authors match."
>  (defun notmuch-search-show-result (thread-id date count authors subject tags)
>    ;; Ignore excluded matches
>    (unless (eq (aref count 1) ?0)
> -    (let ((beg (point))
> +    (let ((beg (point-max))
>  	  (tags-str (mapconcat 'identity tags " ")))
> -      (dolist (format notmuch-search-result-format)
> -	(notmuch-search-insert-field (car format) (cdr format)
> -				     date count authors subject tags-str))
> -      (insert "\n")
> -      (notmuch-search-color-line beg (point) tags)
> -      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
> -      (put-text-property beg (point) 'notmuch-search-authors authors)
> -      (put-text-property beg (point) 'notmuch-search-subject subject))))
> +      (save-excursion
> +	(goto-char beg)
> +	(dolist (format notmuch-search-result-format)
> +	  (notmuch-search-insert-field (car format) (cdr format)
> +				       date count authors subject tags-str))
> +	(insert "\n")
> +	(notmuch-search-color-line beg (point) tags)
> +	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
> +	(put-text-property beg (point) 'notmuch-search-authors authors)
> +	(put-text-property beg (point) 'notmuch-search-subject subject))
> +      (when (string= thread-id notmuch-search-target-thread)
> +	(setq notmuch-search-target-thread "found")
> +	(goto-char beg)))))
>  
>  (defun notmuch-search-process-filter (proc string)
>    "Process and filter the output of \"notmuch search\""
> -  (let ((buffer (process-buffer proc))
> -	(found-target nil))
> +  (let ((buffer (process-buffer proc)))
>      (if (buffer-live-p buffer)
>  	(with-current-buffer buffer
> -	  (save-excursion
>  	    (let ((line 0)
>  		  (more t)
>  		  (inhibit-read-only t)
> @@ -763,12 +766,8 @@ non-authors is found, assume that all of the authors match."
>  			   (subject (match-string 5 string))
>  			   (tags (match-string 6 string))
>  			   (tag-list (if tags (save-match-data (split-string tags)))))
> -		      (goto-char (point-max))
>  		      (if (/= (match-beginning 1) line)
>  			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))

Does this mean that error information now comes just before the matching
thread, rather than at the place in the thread list the error actually
occurs? This is not a complaint: just everything else looks like it is
functionally equivalent. (Maybe it is all made irrelevant by a later
patch anyway).

Best wishes

Mark


> -		      (when (string= thread-id notmuch-search-target-thread)
> -			(set 'found-target (point))
> -			(set 'notmuch-search-target-thread "found"))
>  		      (notmuch-search-show-result thread-id date count authors subject tag-list)
>  		      (set 'line (match-end 0)))
>  		  (set 'more nil)
> @@ -777,8 +776,6 @@ non-authors is found, assume that all of the authors match."
>  		  (if (< line (length string))
>  		      (setq notmuch-search-process-filter-data (substring string line)))
>  		  ))))
> -	  (if found-target
> -	      (goto-char found-target)))
>        (delete-process proc))))
>  
>  (defun notmuch-search-tag-all (&optional tag-changes)
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/8] emacs: Helper for reporting search parsing errors

[Mark Walters]

On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This removes the last bit of direct output from the parsing function.
> With the parser now responsible solely for parsing, we can swap it out
> for another parser.
> ---
>  emacs/notmuch.el |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 4a6490a..a073367 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -745,6 +745,13 @@ non-authors is found, assume that all of the authors match."
>  	(setq notmuch-search-target-thread "found")
>  	(goto-char beg)))))
>  
> +(defun notmuch-search-show-error (string &rest objects)
> +  (save-excursion
> +    (goto-char (point-max))
> +    (insert "Error: Unexpected output from notmuch search:\n")
> +    (insert (apply #'format string objects))
> +    (insert "\n")))
> +
>  (defun notmuch-search-process-filter (proc string)
>    "Process and filter the output of \"notmuch search\""
>    (let ((buffer (process-buffer proc)))
> @@ -767,7 +774,8 @@ non-authors is found, assume that all of the authors match."
>  			   (tags (match-string 6 string))
>  			   (tag-list (if tags (save-match-data (split-string tags)))))
>  		      (if (/= (match-beginning 1) line)
> -			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
> +			  (notmuch-search-show-error
> +			   (substring string line (match-beginning 1))))
>  		      (notmuch-search-show-result thread-id date count authors subject tag-list)
>  		      (set 'line (match-end 0)))
>  		  (set 'more nil)

Ok so I should have read this patch before replying to the previous
one. This fixes my one trivial concern with Patch 3 and looks good in
itself.

I have read Patches 1-4 and they all look fine and make things cleaner
and all tests pass. As Austin says, they stand on their own and can go
in without the rest of the series.

On to patches 5-8!

Best wishes

Mark

Re: [PATCH 3/8] emacs: Move search-target logic to `notmuch-search-show-result'

[Austin Clements]

Quoth Mark Walters on Jul 04 at  9:34 am:
> On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This is a simpler place to do this, since we can avoid any point
> > motion and hence any save-excursions in
> > `notmuch-search-process-filter', which in turn lets us put all of the
> > search-target logic outside of any save-excursions.
> >
> > `notmuch-search-process-filter' could use some reindentation after
> > this, but we're about to rewrite it entirely, so we won't bother.
> > ---
> >  emacs/notmuch.el |   33 +++++++++++++++------------------
> >  1 file changed, 15 insertions(+), 18 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index dadc6d6..4a6490a 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -729,24 +729,27 @@ non-authors is found, assume that all of the authors match."
> >  (defun notmuch-search-show-result (thread-id date count authors subject tags)
> >    ;; Ignore excluded matches
> >    (unless (eq (aref count 1) ?0)
> > -    (let ((beg (point))
> > +    (let ((beg (point-max))
> >  	  (tags-str (mapconcat 'identity tags " ")))
> > -      (dolist (format notmuch-search-result-format)
> > -	(notmuch-search-insert-field (car format) (cdr format)
> > -				     date count authors subject tags-str))
> > -      (insert "\n")
> > -      (notmuch-search-color-line beg (point) tags)
> > -      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
> > -      (put-text-property beg (point) 'notmuch-search-authors authors)
> > -      (put-text-property beg (point) 'notmuch-search-subject subject))))
> > +      (save-excursion
> > +	(goto-char beg)
> > +	(dolist (format notmuch-search-result-format)
> > +	  (notmuch-search-insert-field (car format) (cdr format)
> > +				       date count authors subject tags-str))
> > +	(insert "\n")
> > +	(notmuch-search-color-line beg (point) tags)
> > +	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
> > +	(put-text-property beg (point) 'notmuch-search-authors authors)
> > +	(put-text-property beg (point) 'notmuch-search-subject subject))
> > +      (when (string= thread-id notmuch-search-target-thread)
> > +	(setq notmuch-search-target-thread "found")
> > +	(goto-char beg)))))
> >  
> >  (defun notmuch-search-process-filter (proc string)
> >    "Process and filter the output of \"notmuch search\""
> > -  (let ((buffer (process-buffer proc))
> > -	(found-target nil))
> > +  (let ((buffer (process-buffer proc)))
> >      (if (buffer-live-p buffer)
> >  	(with-current-buffer buffer
> > -	  (save-excursion
> >  	    (let ((line 0)
> >  		  (more t)
> >  		  (inhibit-read-only t)
> > @@ -763,12 +766,8 @@ non-authors is found, assume that all of the authors match."
> >  			   (subject (match-string 5 string))
> >  			   (tags (match-string 6 string))
> >  			   (tag-list (if tags (save-match-data (split-string tags)))))
> > -		      (goto-char (point-max))
> >  		      (if (/= (match-beginning 1) line)
> >  			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
> 
> Does this mean that error information now comes just before the matching
> thread, rather than at the place in the thread list the error actually
> occurs? This is not a complaint: just everything else looks like it is
> functionally equivalent. (Maybe it is all made irrelevant by a later
> patch anyway).

In fact, the errors will appear before the first message *until* you
reach the matched message.  I think this was the result of overzealous
rebasing.  I'm happy to fix it here for posterity (possibly by
swapping this and the next patch), though as you point out the next
patch in the series also fixes it, so I'm also happy to leave it
alone.

> Best wishes
> 
> Mark
> 
> 
> > -		      (when (string= thread-id notmuch-search-target-thread)
> > -			(set 'found-target (point))
> > -			(set 'notmuch-search-target-thread "found"))
> >  		      (notmuch-search-show-result thread-id date count authors subject tag-list)
> >  		      (set 'line (match-end 0)))
> >  		  (set 'more nil)
> > @@ -777,8 +776,6 @@ non-authors is found, assume that all of the authors match."
> >  		  (if (< line (length string))
> >  		      (setq notmuch-search-process-filter-data (substring string line)))
> >  		  ))))
> > -	  (if found-target
> > -	      (goto-char found-target)))
> >        (delete-process proc))))
> >  
> >  (defun notmuch-search-tag-all (&optional tag-changes)

Re: [PATCH 1/8] emacs: Clean up notmuch-search-show-result

[Austin Clements]

Quoth Mark Walters on Jul 04 at  8:53 am:
> On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This simplifies the code and makes it no longer cubic in the number of
> > result fields.
> 
> This looks good to me and all tests pass, and I agree that this patch
> can be pushed independently of the later patches in the series.
> 
> My one comment is that I, as a lisp beginner, found the use of "format"
> as a function and a variable confusing (particularly as I had not come
> across the dolist macro and that use really makes format look like a
> function).

I can see how that would be confusing.  Unless this version of the
series is somehow perfect, I'll rename it in the second version.
Perhaps result-format for the loop variable and spec for the format
string argument?

> Best wishes
> 
> Mark
> 
> > ---
> >  emacs/notmuch.el |   20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index c6236db..be217a2 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -707,29 +707,29 @@ non-authors is found, assume that all of the authors match."
> >  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
> >        (insert padding))))
> >  
> > -(defun notmuch-search-insert-field (field date count authors subject tags)
> > +(defun notmuch-search-insert-field (field format date count authors subject tags)
> >    (cond
> >     ((string-equal field "date")
> > -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
> > +    (insert (propertize (format format date)
> >  			'face 'notmuch-search-date)))
> >     ((string-equal field "count")
> > -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
> > +    (insert (propertize (format format count)
> >  			'face 'notmuch-search-count)))
> >     ((string-equal field "subject")
> > -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
> > +    (insert (propertize (format format subject)
> >  			'face 'notmuch-search-subject)))
> >  
> >     ((string-equal field "authors")
> > -    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
> > +    (notmuch-search-insert-authors format authors))
> >  
> >     ((string-equal field "tags")
> > -    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
> > +    (insert
> > +     (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))
> >  
> >  (defun notmuch-search-show-result (date count authors subject tags)
> > -  (let ((fields) (field))
> > -    (setq fields (mapcar 'car notmuch-search-result-format))
> > -    (loop for field in fields
> > -	  do (notmuch-search-insert-field field date count authors subject tags)))
> > +  (dolist (format notmuch-search-result-format)
> > +    (notmuch-search-insert-field (car format) (cdr format)
> > +				 date count authors subject tags))
> >    (insert "\n"))
> >  
> >  (defun notmuch-search-process-filter (proc string)

Re: [PATCH 1/8] emacs: Clean up notmuch-search-show-result

[Tomi Ollila]

On Wed, Jul 04 2012, Austin Clements <amdragon@MIT.EDU> wrote:

> Quoth Mark Walters on Jul 04 at  8:53 am:
>> On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> > This simplifies the code and makes it no longer cubic in the number of
>> > result fields.
>> 
>> This looks good to me and all tests pass, and I agree that this patch
>> can be pushed independently of the later patches in the series.
>> 
>> My one comment is that I, as a lisp beginner, found the use of "format"
>> as a function and a variable confusing (particularly as I had not come
>> across the dolist macro and that use really makes format look like a
>> function).
>
> I can see how that would be confusing.  Unless this version of the
> series is somehow perfect, I'll rename it in the second version.
> Perhaps result-format for the loop variable and spec for the format
> string argument?

There are 'format' variables elsewhere too -- on those places it could
be renamed as 'format-string' (as used else-elsewhere and the format
documentation refers to STRING). As an analogy to that the 'format' in
this particular dolist usage could be 'format-pair' ? (but anything goes,
even the current 'format').

Tomi

>
>> Best wishes
>> 
>> Mark

Re: [PATCH 0/8] JSON-based search-mode

[Tomi Ollila]

On Wed, Jul 04 2012, Austin Clements wrote:

> This patch series replaces the text format parser used for search in
> Emacs with a parser for the JSON format.  This should address the
> escaping and flexibility problems that have plagued the text format.
> Like the text format, it supports incremental output.
>
> Patches 1-4 simply clean up the Emacs search code and could be pushed
> before the rest of the series.  Patch 5 switches to the JSON plist
> representation internally, but retains the text parser.  This requires
> some changes to the text parser to keep things working, but don't get
> too hung up on them since it's about to get replaced entirely.  Patch
> 6 adds a test.
>
> Finally, patches 7 and 8 are the real meat.  Patch 7 introduces a
> general incremental JSON parser.  For search, we could probably get
> away with a simpler, hacky approach, but an incremental JSON parser is
> the type of thing you only want to write once---hacky or not---and it
> seems like the type of thing that could be useful elsewhere, too.
> It's general enough to support things like incremental show buffer
> rendering.
>
> Patch 8 rewrites the search output parser to use the JSON format via
> this incremental parser.

Looks good, works fine, tests pass. 

I am having those patches applied in this copy of notmuch (emacs mua)
I am running and will continue to do so until new series arrives, this
will be pushed, or latest patch series go stale (which I hope doesn't
happen :)

+1

Tomi

Re: [PATCH 1/8] emacs: Clean up notmuch-search-show-result

[Mark Walters]

On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This simplifies the code and makes it no longer cubic in the number of
> result fields.
> ---
>  emacs/notmuch.el |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index c6236db..be217a2 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -707,29 +707,29 @@ non-authors is found, assume that all of the authors match."
>  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
>        (insert padding))))
>  
> -(defun notmuch-search-insert-field (field date count authors subject tags)
> +(defun notmuch-search-insert-field (field format date count authors subject tags)
>    (cond
>     ((string-equal field "date")
> -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
> +    (insert (propertize (format format date)
>  			'face 'notmuch-search-date)))
>     ((string-equal field "count")
> -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
> +    (insert (propertize (format format count)
>  			'face 'notmuch-search-count)))
>     ((string-equal field "subject")
> -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
> +    (insert (propertize (format format subject)
>  			'face 'notmuch-search-subject)))
>  
>     ((string-equal field "authors")
> -    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
> +    (notmuch-search-insert-authors format authors))
>  
>     ((string-equal field "tags")
> -    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
> +    (insert
> +     (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))

Am I missing something or did the search result line previously ignore
the user's specification for tags and automatically print it with inside
()? Now this change does actually obey the user's specification.

In principle that is a good thing, but the tag update code (when
changing a tag on a message) seems to rely on the brackets to find
something and errors out if the user format does not have any
brackets. (The code has things like "(re-search-backward "(")" in it).

Incidentally, patch 8 does change the format slightly by not padding the
the date field string itself and only printing it padded. This (very
mildly) breaks things if the user has customised
notmuch-search-result-format

Best wishes

Mark



>  
>  (defun notmuch-search-show-result (date count authors subject tags)
> -  (let ((fields) (field))
> -    (setq fields (mapcar 'car notmuch-search-result-format))
> -    (loop for field in fields
> -	  do (notmuch-search-insert-field field date count authors subject tags)))
> +  (dolist (format notmuch-search-result-format)
> +    (notmuch-search-insert-field (car format) (cdr format)
> +				 date count authors subject tags))
>    (insert "\n"))
>  
>  (defun notmuch-search-process-filter (proc string)
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/8] emacs: Clean up notmuch-search-show-result

[Austin Clements]

Quoth Mark Walters on Jul 04 at  9:47 pm:
> On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This simplifies the code and makes it no longer cubic in the number of
> > result fields.
> > ---
> >  emacs/notmuch.el |   20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index c6236db..be217a2 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -707,29 +707,29 @@ non-authors is found, assume that all of the authors match."
> >  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
> >        (insert padding))))
> >  
> > -(defun notmuch-search-insert-field (field date count authors subject tags)
> > +(defun notmuch-search-insert-field (field format date count authors subject tags)
> >    (cond
> >     ((string-equal field "date")
> > -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
> > +    (insert (propertize (format format date)
> >  			'face 'notmuch-search-date)))
> >     ((string-equal field "count")
> > -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
> > +    (insert (propertize (format format count)
> >  			'face 'notmuch-search-count)))
> >     ((string-equal field "subject")
> > -    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
> > +    (insert (propertize (format format subject)
> >  			'face 'notmuch-search-subject)))
> >  
> >     ((string-equal field "authors")
> > -    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
> > +    (notmuch-search-insert-authors format authors))
> >  
> >     ((string-equal field "tags")
> > -    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
> > +    (insert
> > +     (format format (propertize tags 'font-lock-face 'notmuch-tag-face))))))
> 
> Am I missing something or did the search result line previously ignore
> the user's specification for tags and automatically print it with inside
> ()? Now this change does actually obey the user's specification.
> 
> In principle that is a good thing, but the tag update code (when
> changing a tag on a message) seems to rely on the brackets to find
> something and errors out if the user format does not have any
> brackets. (The code has things like "(re-search-backward "(")" in it).

Mm, that's a good point.  For now I should probably just revert the
code to ignoring the user's format specification.  A better solution
would be to remember the result's plist and simply rebuild the whole
line when the tags change, rather than trying to incrementally edit
it.

> Incidentally, patch 8 does change the format slightly by not padding the
> the date field string itself and only printing it padded. This (very
> mildly) breaks things if the user has customised
> notmuch-search-result-format

Yes, that's true.  I could pad out the date to keep this backwards
compatible, but since all of the other fields are variable width, I
think I'd rather switch to the variable size date field and just
mention this in the news.

> Best wishes
> 
> Mark
> 
> 
> 
> >  
> >  (defun notmuch-search-show-result (date count authors subject tags)
> > -  (let ((fields) (field))
> > -    (setq fields (mapcar 'car notmuch-search-result-format))
> > -    (loop for field in fields
> > -	  do (notmuch-search-insert-field field date count authors subject tags)))
> > +  (dolist (format notmuch-search-result-format)
> > +    (notmuch-search-insert-field (car format) (cdr format)
> > +				 date count authors subject tags))
> >    (insert "\n"))
> >  
> >  (defun notmuch-search-process-filter (proc string)

Re: [PATCH 7/8] emacs: Implement an incremental JSON parser

[Mark Walters]

On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This parser is designed to read streaming JSON whose structure is
> known to the caller.  Like a typical JSON parsing interface, it
> provides a function to read a complete JSON value from the input.
> However, it extends this with an additional function that
> requires the next value in the input to be a compound value and
> descends into it, allowing its elements to be read one at a time
> or further descended into.  Both functions can return 'retry to
> indicate that not enough input is available.
>
> The parser supports efficient partial parsing, so there's no need to
> frame the input for correctness or performance.
>
> Currently only descending into JSON lists is supported because that's
> all we need, but support for descending into JSON objects can be added
> in the future.
> ---
>  emacs/notmuch-lib.el |  183 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 183 insertions(+)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index c829df3..f7cda33 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -23,6 +23,7 @@
>  
>  (require 'mm-view)
>  (require 'mm-decode)
> +(require 'json)
>  (eval-when-compile (require 'cl))
>  
>  (defvar notmuch-command "notmuch"
> @@ -296,6 +297,188 @@ was called."
>  (defvar notmuch-show-process-crypto nil)
>  (make-variable-buffer-local 'notmuch-show-process-crypto)
>  
> +;; Incremental JSON parsing
> +
> +(defun notmuch-json-create-parser (buffer)
> +  "Return a streaming JSON parser that consumes input from BUFFER.
> +
> +This parser is designed to read streaming JSON whose structure is
> +known to the caller.  Like a typical JSON parsing interface, it
> +provides a function to read a complete JSON value from the input.
> +However, it extends this with an additional function that
> +requires the next value in the input to be a compound value and
> +descends into it, allowing its elements to be read one at a time
> +or further descended into.  Both functions can return 'retry to
> +indicate that not enough input is available.
> +
> +The parser always consumes input from BUFFER's point.  Hence, the
> +caller is allowed to delete and data before point and may
> +resynchronize after an error by moving point."
> +
> +  (list buffer
> +	;; Terminator stack: a stack of characters that indicate the
> +	;; end of the compound values enclosing point
> +	'()
> +	;; Next: One of
> +	;; * 'expect-value if the next token must be a value, but a
> +	;;   value has not yet been reached
> +	;; * 'value if point is at the beginning of a value
> +	;; * 'expect-comma if the next token must be a comma
> +	'expect-value
> +	;; Allow terminator: non-nil if the next token may be a
> +	;; terminator
> +	nil
> +	;; Partial parse position: If state is 'value, a marker for
> +	;; the position of the partial parser or nil if no partial
> +	;; parsing has happened yet
> +	nil
> +	;; Partial parse state: If state is 'value, the current
> +	;; `parse-partial-sexp' state
> +	nil))
> +
> +(defmacro notmuch-json-buffer (jp) `(first ,jp))
> +(defmacro notmuch-json-term-stack (jp) `(second ,jp))
> +(defmacro notmuch-json-next (jp) `(third ,jp))
> +(defmacro notmuch-json-allow-term (jp) `(fourth ,jp))
> +(defmacro notmuch-json-partial-pos (jp) `(fifth ,jp))
> +(defmacro notmuch-json-partial-state (jp) `(sixth ,jp))
> +
> +(defvar notmuch-json-syntax-table
> +  (let ((table (make-syntax-table)))
> +    ;; The standard syntax table is what we need except that "." needs
> +    ;; to have word syntax instead of punctuation syntax.
> +    (modify-syntax-entry ?. "w" table)
> +    table)
> +  "Syntax table used for incremental JSON parsing.")
> +
> +(defun notmuch-json-scan-to-value (jp)
> +  ;; Helper function that consumes separators, terminators, and
> +  ;; whitespace from point.  Returns nil if it successfully reached
> +  ;; the beginning of a value, 'end if it consumed a terminator, or
> +  ;; 'retry if not enough input was available to reach a value.  Upon
> +  ;; nil return, (notmuch-json-next jp) is always 'value.
> +
> +  (if (eq (notmuch-json-next jp) 'value)
> +      ;; We're already at a value
> +      nil
> +    ;; Drive the state toward 'expect-value
> +    (skip-chars-forward " \t\r\n")
> +    (or (when (eobp) 'retry)
> +	;; Test for the terminator for the current compound
> +	(when (and (notmuch-json-allow-term jp)
> +		   (eq (char-after) (car (notmuch-json-term-stack jp))))
> +	  ;; Consume it and expect a comma or terminator next
> +	  (forward-char)
> +	  (setf (notmuch-json-term-stack jp) (cdr (notmuch-json-term-stack jp))
> +		(notmuch-json-next jp) 'expect-comma
> +		(notmuch-json-allow-term jp) t)
> +	  'end)
> +	;; Test for a separator
> +	(when (eq (notmuch-json-next jp) 'expect-comma)
> +	  (when (/= (char-after) ?,)
> +	    (signal 'json-readtable-error (list "expected ','")))
> +	  ;; Consume it, switch to 'expect-value, and disallow a
> +	  ;; terminator
> +	  (forward-char)
> +	  (skip-chars-forward " \t\r\n")
> +	  (setf (notmuch-json-next jp) 'expect-value
> +		(notmuch-json-allow-term jp) nil)
> +	  ;; We moved point, so test for eobp again and fall through
> +	  ;; to the next test if there's more input
> +	  (when (eobp) 'retry))
> +	;; Next must be 'expect-value and we know this isn't
> +	;; whitespace, EOB, or a terminator, so point must be on a
> +	;; value
> +	(progn
> +	  (assert (eq (notmuch-json-next jp) 'expect-value))
> +	  (setf (notmuch-json-next jp) 'value)
> +	  nil))))
> +
> +(defun notmuch-json-begin-compound (jp)
> +  "Parse the beginning of a compound value and traverse inside it.
> +
> +Returns 'retry if there is insufficient input to parse the
> +beginning of the compound.  If this is able to parse the
> +beginning of a compound, it returns t and later calls to
> +`notmuch-json-read' will return the compound's elements.
> +
> +Entering JSON objects is current unimplemented."
> +
> +  (with-current-buffer (notmuch-json-buffer jp)
> +    ;; Disallow terminators
> +    (setf (notmuch-json-allow-term jp) nil)
> +    (or (notmuch-json-scan-to-value jp)
> +	(if (/= (char-after) ?\[)
> +	    (signal 'json-readtable-error (list "expected '['"))
> +	  (forward-char)
> +	  (push ?\] (notmuch-json-term-stack jp))
> +	  ;; Expect a value or terminator next
> +	  (setf (notmuch-json-next jp) 'expect-value
> +		(notmuch-json-allow-term jp) t)
> +	  t))))
> +
> +(defun notmuch-json-read (jp)
> +  "Parse the value at point in JP's buffer.
> +
> +Returns 'retry if there is insufficient input to parse a complete
> +JSON value.  If the parser is currently inside a compound value
> +and the next token ends the list or object, returns 'end.
> +Otherwise, returns the value."


This looks excellent! My only comment is that I think it would be
helpful it the above comment and the one for notmuch-json-begin-compound
said what happens to point when they get called.

Also, I think in practice a lot of the parsing is done by json.el (that
you use this to enter one level of the hierarchy and then use this to
ensure that json.el only gets complete objects to parse) so I assume
this means that your much faster replacement for json.el could be
slotted in? It might be worth mentioning that in the commit message.

Best wishes

Mark

> +
> +  (with-current-buffer (notmuch-json-buffer jp)
> +    (or
> +     ;; Get to a value state
> +     (notmuch-json-scan-to-value jp)
> +
> +     ;; Can we parse a complete value?
> +     (let ((complete
> +	    (if (looking-at "[-+0-9tfn]")
> +		;; This is a number or a keyword, so the partial
> +		;; parser isn't going to help us because a truncated
> +		;; number or keyword looks like a complete symbol to
> +		;; it.  Look for something that clearly ends it.
> +		(save-excursion
> +		  (skip-chars-forward "^]},: \t\r\n")
> +		  (not (eobp)))
> +
> +	      ;; We're looking at a string, object, or array, which we
> +	      ;; can partial parse.  If we just reached the value, set
> +	      ;; up the partial parser.
> +	      (when (null (notmuch-json-partial-state jp))
> +		(setf (notmuch-json-partial-pos jp) (point-marker)))
> +
> +	      ;; Extend the partial parse until we either reach EOB or
> +	      ;; get the whole value
> +	      (save-excursion
> +		(let ((pstate
> +		       (with-syntax-table notmuch-json-syntax-table
> +			 (parse-partial-sexp
> +			  (notmuch-json-partial-pos jp) (point-max) 0 nil
> +			  (notmuch-json-partial-state jp)))))
> +		  ;; A complete value is available if we've reached
> +		  ;; depth 0 or less and encountered a complete
> +		  ;; subexpression.
> +		  (if (and (<= (first pstate) 0) (third pstate))
> +		      t
> +		    ;; Not complete.  Update the partial parser state
> +		    (setf (notmuch-json-partial-pos jp) (point-marker)
> +			  (notmuch-json-partial-state jp) pstate)
> +		    nil))))))
> +
> +       (if (not complete)
> +	   'retry
> +	 ;; We have a value.  Reset the partial parse state and expect
> +	 ;; a comma or terminator after the value.
> +	 (setf (notmuch-json-next jp) 'expect-comma
> +	       (notmuch-json-allow-term jp) t
> +	       (notmuch-json-partial-pos jp) nil
> +	       (notmuch-json-partial-state jp) nil)
> +	 ;; Parse the value
> +	 (let* ((json-object-type 'plist)
> +		(json-array-type 'list)
> +		(json-false nil))
> +	   (json-read)))))))
> +
>  (provide 'notmuch-lib)
>  
>  ;; Local Variables:
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 8/8] emacs: Switch from text to JSON format for search results

[Mark Walters]

On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> The JSON format eliminates the complex escaping issues that have
> plagued the text search format.  This uses the incremental JSON parser
> so that, like the text parser, it can output search results
> incrementally.
>
> This slows down the parser by about ~4X, but puts us in a good
> position to optimize either by improving the JSON parser (evidence
> suggests this can reduce the overhead to ~40% over the text format) or
> by switching to S-expressions (evidence suggests this will more than
> double performance over the text parser).  [1]
>
> This also fixes the incremental search parsing test.
>
> [1] id:"20110720205007.GB21316@mit.edu"
> ---
>  emacs/notmuch.el |  113 ++++++++++++++++++++++++++++++++----------------------
>  test/emacs       |    1 -
>  2 files changed, 67 insertions(+), 47 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 084cec6..2a09a98 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -60,7 +60,7 @@
>  (require 'notmuch-message)
>  
>  (defcustom notmuch-search-result-format
> -  `(("date" . "%s ")
> +  `(("date" . "%12s ")
>      ("count" . "%-7s ")
>      ("authors" . "%-20s ")
>      ("subject" . "%s ")
> @@ -557,17 +557,14 @@ This function advances the next thread when finished."
>    (notmuch-search-tag '("-inbox"))
>    (notmuch-search-next-thread))
>  
> -(defvar notmuch-search-process-filter-data nil
> -  "Data that has not yet been processed.")
> -(make-variable-buffer-local 'notmuch-search-process-filter-data)
> -
>  (defun notmuch-search-process-sentinel (proc msg)
>    "Add a message to let user know when \"notmuch search\" exits"
>    (let ((buffer (process-buffer proc))
>  	(status (process-status proc))
>  	(exit-status (process-exit-status proc))
>  	(never-found-target-thread nil))
> -    (if (memq status '(exit signal))
> +    (when (memq status '(exit signal))
> +	(kill-buffer (process-get proc 'parse-buf))
>  	(if (buffer-live-p buffer)
>  	    (with-current-buffer buffer
>  	      (save-excursion
> @@ -577,8 +574,6 @@ This function advances the next thread when finished."
>  		  (if (eq status 'signal)
>  		      (insert "Incomplete search results (search process was killed).\n"))
>  		  (when (eq status 'exit)
> -		    (if notmuch-search-process-filter-data
> -			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
>  		    (insert "End of search results.")
>  		    (unless (= exit-status 0)
>  		      (insert (format " (process returned %d)" exit-status)))
> @@ -757,45 +752,62 @@ non-authors is found, assume that all of the authors match."
>      (insert (apply #'format string objects))
>      (insert "\n")))
>  
> +(defvar notmuch-search-process-state nil
> +  "Parsing state of the search process filter.")
> +
> +(defvar notmuch-search-json-parser nil
> +  "Incremental JSON parser for the search process filter.")
> +
>  (defun notmuch-search-process-filter (proc string)
>    "Process and filter the output of \"notmuch search\""
> -  (let ((buffer (process-buffer proc)))
> -    (if (buffer-live-p buffer)
> -	(with-current-buffer buffer
> -	    (let ((line 0)
> -		  (more t)
> -		  (inhibit-read-only t)
> -		  (string (concat notmuch-search-process-filter-data string)))
> -	      (setq notmuch-search-process-filter-data nil)
> -	      (while more
> -		(while (and (< line (length string)) (= (elt string line) ?\n))
> -		  (setq line (1+ line)))
> -		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
> -		    (let* ((thread-id (match-string 1 string))
> -			   (tags-str (match-string 7 string))
> -			   (result (list :thread thread-id
> -					 :date_relative (match-string 2 string)
> -					 :matched (string-to-number
> -						   (match-string 3 string))
> -					 :total (string-to-number
> -						 (match-string 4 string))
> -					 :authors (match-string 5 string)
> -					 :subject (match-string 6 string)
> -					 :tags (if tags-str
> -						   (save-match-data
> -						     (split-string tags-str))))))
> -		      (if (/= (match-beginning 0) line)
> -			  (notmuch-search-show-error
> -			   (substring string line (match-beginning 0))))
> -		      (notmuch-search-show-result result)
> -		      (set 'line (match-end 0)))
> -		  (set 'more nil)
> -		  (while (and (< line (length string)) (= (elt string line) ?\n))
> -		    (setq line (1+ line)))
> -		  (if (< line (length string))
> -		      (setq notmuch-search-process-filter-data (substring string line)))
> -		  ))))
> -      (delete-process proc))))
> +  (let ((results-buf (process-buffer proc))
> +	(parse-buf (process-get proc 'parse-buf))
> +	(inhibit-read-only t)
> +	done)
> +    (if (not (buffer-live-p results-buf))
> +	(delete-process proc)
> +      (with-current-buffer parse-buf
> +	;; Insert new data
> +	(save-excursion
> +	  (goto-char (point-max))
> +	  (insert string)))
> +      (with-current-buffer results-buf
> +	(while (not done)
> +	  (condition-case nil
> +	      (case notmuch-search-process-state
> +		((begin)
> +		 ;; Enter the results list
> +		 (if (eq (notmuch-json-begin-compound
> +			  notmuch-search-json-parser) 'retry)
> +		     (setq done t)
> +		   (setq notmuch-search-process-state 'result)))
> +		((result)
> +		 ;; Parse a result
> +		 (let ((result (notmuch-json-read notmuch-search-json-parser)))
> +		   (case result
> +		     ((retry) (setq done t))
> +		     ((end) (setq notmuch-search-process-state 'end))
> +		     (otherwise (notmuch-search-show-result result)))))
> +		((end)
> +		 ;; Any trailing data is unexpected
> +		 (with-current-buffer parse-buf
> +		   (skip-chars-forward " \t\r\n")
> +		   (if (eobp)
> +		       (setq done t)
> +		     (signal 'json-error nil)))))

This looks good to me. Would it make sense to put the "Any trailing
data" as a tiny function in notmuch-lib? something like 

(defun notmuch-json-assert-end-of-data ()
   (skip-chars-forward " \t\r\n") 
   (if (eobp)
       (setq done t) 
     (signal 'json-error nil)))

Best wishes

Mark


> +	    (json-error
> +	     ;; Do our best to resynchronize and ensure forward
> +	     ;; progress
> +	     (notmuch-search-show-error
> +	      "%s"
> +	      (with-current-buffer parse-buf
> +		(let ((bad (buffer-substring (line-beginning-position)
> +					     (line-end-position))))
> +		  (forward-line)
> +		  bad))))))
> +	;; Clear out what we've parsed
> +	(with-current-buffer parse-buf
> +	  (delete-region (point-min) (point)))))))
>  
>  (defun notmuch-search-tag-all (&optional tag-changes)
>    "Add/remove tags from all messages in current search buffer.
> @@ -898,10 +910,19 @@ Other optional parameters are used as follows:
>  	(let ((proc (start-process
>  		     "notmuch-search" buffer
>  		     notmuch-command "search"
> +		     "--format=json"
>  		     (if oldest-first
>  			 "--sort=oldest-first"
>  		       "--sort=newest-first")
> -		     query)))
> +		     query))
> +	      ;; Use a scratch buffer to accumulate partial output.
> +	      ;; This buffer will be killed by the sentinel, which
> +	      ;; should be called no matter how the process dies.
> +	      (parse-buf (generate-new-buffer " *notmuch search parse*")))
> +	  (set (make-local-variable 'notmuch-search-process-state) 'begin)
> +	  (set (make-local-variable 'notmuch-search-json-parser)
> +	       (notmuch-json-create-parser parse-buf))
> +	  (process-put proc 'parse-buf parse-buf)
>  	  (set-process-sentinel proc 'notmuch-search-process-sentinel)
>  	  (set-process-filter proc 'notmuch-search-process-filter)
>  	  (set-process-query-on-exit-flag proc nil))))
> diff --git a/test/emacs b/test/emacs
> index 293b12a..afe35ba 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -36,7 +36,6 @@ test_emacs '(notmuch-search "tag:inbox")
>  test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
>  
>  test_begin_subtest "Incremental parsing of search results"
> -test_subtest_known_broken
>  test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
>  	    (ad-activate 'notmuch-search-process-filter)
>  	    (notmuch-search \"tag:inbox\")
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 7/8] emacs: Implement an incremental JSON parser

[Austin Clements]

Quoth Mark Walters on Jul 05 at  9:30 am:
> On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This parser is designed to read streaming JSON whose structure is
> > known to the caller.  Like a typical JSON parsing interface, it
> > provides a function to read a complete JSON value from the input.
> > However, it extends this with an additional function that
> > requires the next value in the input to be a compound value and
> > descends into it, allowing its elements to be read one at a time
> > or further descended into.  Both functions can return 'retry to
> > indicate that not enough input is available.
> >
> > The parser supports efficient partial parsing, so there's no need to
> > frame the input for correctness or performance.
> >
> > Currently only descending into JSON lists is supported because that's
> > all we need, but support for descending into JSON objects can be added
> > in the future.
> > ---
> >  emacs/notmuch-lib.el |  183 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 183 insertions(+)
> >
> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> > index c829df3..f7cda33 100644
> > --- a/emacs/notmuch-lib.el
> > +++ b/emacs/notmuch-lib.el
> > @@ -23,6 +23,7 @@
> >  
> >  (require 'mm-view)
> >  (require 'mm-decode)
> > +(require 'json)
> >  (eval-when-compile (require 'cl))
> >  
> >  (defvar notmuch-command "notmuch"
> > @@ -296,6 +297,188 @@ was called."
> >  (defvar notmuch-show-process-crypto nil)
> >  (make-variable-buffer-local 'notmuch-show-process-crypto)
> >  
> > +;; Incremental JSON parsing
> > +
> > +(defun notmuch-json-create-parser (buffer)
> > +  "Return a streaming JSON parser that consumes input from BUFFER.
> > +
> > +This parser is designed to read streaming JSON whose structure is
> > +known to the caller.  Like a typical JSON parsing interface, it
> > +provides a function to read a complete JSON value from the input.
> > +However, it extends this with an additional function that
> > +requires the next value in the input to be a compound value and
> > +descends into it, allowing its elements to be read one at a time
> > +or further descended into.  Both functions can return 'retry to
> > +indicate that not enough input is available.
> > +
> > +The parser always consumes input from BUFFER's point.  Hence, the
> > +caller is allowed to delete and data before point and may
> > +resynchronize after an error by moving point."
> > +
> > +  (list buffer
> > +	;; Terminator stack: a stack of characters that indicate the
> > +	;; end of the compound values enclosing point
> > +	'()
> > +	;; Next: One of
> > +	;; * 'expect-value if the next token must be a value, but a
> > +	;;   value has not yet been reached
> > +	;; * 'value if point is at the beginning of a value
> > +	;; * 'expect-comma if the next token must be a comma
> > +	'expect-value
> > +	;; Allow terminator: non-nil if the next token may be a
> > +	;; terminator
> > +	nil
> > +	;; Partial parse position: If state is 'value, a marker for
> > +	;; the position of the partial parser or nil if no partial
> > +	;; parsing has happened yet
> > +	nil
> > +	;; Partial parse state: If state is 'value, the current
> > +	;; `parse-partial-sexp' state
> > +	nil))
> > +
> > +(defmacro notmuch-json-buffer (jp) `(first ,jp))
> > +(defmacro notmuch-json-term-stack (jp) `(second ,jp))
> > +(defmacro notmuch-json-next (jp) `(third ,jp))
> > +(defmacro notmuch-json-allow-term (jp) `(fourth ,jp))
> > +(defmacro notmuch-json-partial-pos (jp) `(fifth ,jp))
> > +(defmacro notmuch-json-partial-state (jp) `(sixth ,jp))
> > +
> > +(defvar notmuch-json-syntax-table
> > +  (let ((table (make-syntax-table)))
> > +    ;; The standard syntax table is what we need except that "." needs
> > +    ;; to have word syntax instead of punctuation syntax.
> > +    (modify-syntax-entry ?. "w" table)
> > +    table)
> > +  "Syntax table used for incremental JSON parsing.")
> > +
> > +(defun notmuch-json-scan-to-value (jp)
> > +  ;; Helper function that consumes separators, terminators, and
> > +  ;; whitespace from point.  Returns nil if it successfully reached
> > +  ;; the beginning of a value, 'end if it consumed a terminator, or
> > +  ;; 'retry if not enough input was available to reach a value.  Upon
> > +  ;; nil return, (notmuch-json-next jp) is always 'value.
> > +
> > +  (if (eq (notmuch-json-next jp) 'value)
> > +      ;; We're already at a value
> > +      nil
> > +    ;; Drive the state toward 'expect-value
> > +    (skip-chars-forward " \t\r\n")
> > +    (or (when (eobp) 'retry)
> > +	;; Test for the terminator for the current compound
> > +	(when (and (notmuch-json-allow-term jp)
> > +		   (eq (char-after) (car (notmuch-json-term-stack jp))))
> > +	  ;; Consume it and expect a comma or terminator next
> > +	  (forward-char)
> > +	  (setf (notmuch-json-term-stack jp) (cdr (notmuch-json-term-stack jp))
> > +		(notmuch-json-next jp) 'expect-comma
> > +		(notmuch-json-allow-term jp) t)
> > +	  'end)
> > +	;; Test for a separator
> > +	(when (eq (notmuch-json-next jp) 'expect-comma)
> > +	  (when (/= (char-after) ?,)
> > +	    (signal 'json-readtable-error (list "expected ','")))
> > +	  ;; Consume it, switch to 'expect-value, and disallow a
> > +	  ;; terminator
> > +	  (forward-char)
> > +	  (skip-chars-forward " \t\r\n")
> > +	  (setf (notmuch-json-next jp) 'expect-value
> > +		(notmuch-json-allow-term jp) nil)
> > +	  ;; We moved point, so test for eobp again and fall through
> > +	  ;; to the next test if there's more input
> > +	  (when (eobp) 'retry))
> > +	;; Next must be 'expect-value and we know this isn't
> > +	;; whitespace, EOB, or a terminator, so point must be on a
> > +	;; value
> > +	(progn
> > +	  (assert (eq (notmuch-json-next jp) 'expect-value))
> > +	  (setf (notmuch-json-next jp) 'value)
> > +	  nil))))
> > +
> > +(defun notmuch-json-begin-compound (jp)
> > +  "Parse the beginning of a compound value and traverse inside it.
> > +
> > +Returns 'retry if there is insufficient input to parse the
> > +beginning of the compound.  If this is able to parse the
> > +beginning of a compound, it returns t and later calls to
> > +`notmuch-json-read' will return the compound's elements.
> > +
> > +Entering JSON objects is current unimplemented."
> > +
> > +  (with-current-buffer (notmuch-json-buffer jp)
> > +    ;; Disallow terminators
> > +    (setf (notmuch-json-allow-term jp) nil)
> > +    (or (notmuch-json-scan-to-value jp)
> > +	(if (/= (char-after) ?\[)
> > +	    (signal 'json-readtable-error (list "expected '['"))
> > +	  (forward-char)
> > +	  (push ?\] (notmuch-json-term-stack jp))
> > +	  ;; Expect a value or terminator next
> > +	  (setf (notmuch-json-next jp) 'expect-value
> > +		(notmuch-json-allow-term jp) t)
> > +	  t))))
> > +
> > +(defun notmuch-json-read (jp)
> > +  "Parse the value at point in JP's buffer.
> > +
> > +Returns 'retry if there is insufficient input to parse a complete
> > +JSON value.  If the parser is currently inside a compound value
> > +and the next token ends the list or object, returns 'end.
> > +Otherwise, returns the value."
> 
> 
> This looks excellent! My only comment is that I think it would be
> helpful it the above comment and the one for notmuch-json-begin-compound
> said what happens to point when they get called.

Good idea.

> Also, I think in practice a lot of the parsing is done by json.el (that
> you use this to enter one level of the hierarchy and then use this to
> ensure that json.el only gets complete objects to parse) so I assume
> this means that your much faster replacement for json.el could be
> slotted in? It might be worth mentioning that in the commit message.

Yes, definitely.  My hope is that we'll skip over the optimized JSON
parser and move straight to S-expressions, which perform ~3X better
than my optimized json.el (and ~8X better than Emacs' json.el).

> Best wishes
> 
> Mark
> 
> > +
> > +  (with-current-buffer (notmuch-json-buffer jp)
> > +    (or
> > +     ;; Get to a value state
> > +     (notmuch-json-scan-to-value jp)
> > +
> > +     ;; Can we parse a complete value?
> > +     (let ((complete
> > +	    (if (looking-at "[-+0-9tfn]")
> > +		;; This is a number or a keyword, so the partial
> > +		;; parser isn't going to help us because a truncated
> > +		;; number or keyword looks like a complete symbol to
> > +		;; it.  Look for something that clearly ends it.
> > +		(save-excursion
> > +		  (skip-chars-forward "^]},: \t\r\n")
> > +		  (not (eobp)))
> > +
> > +	      ;; We're looking at a string, object, or array, which we
> > +	      ;; can partial parse.  If we just reached the value, set
> > +	      ;; up the partial parser.
> > +	      (when (null (notmuch-json-partial-state jp))
> > +		(setf (notmuch-json-partial-pos jp) (point-marker)))
> > +
> > +	      ;; Extend the partial parse until we either reach EOB or
> > +	      ;; get the whole value
> > +	      (save-excursion
> > +		(let ((pstate
> > +		       (with-syntax-table notmuch-json-syntax-table
> > +			 (parse-partial-sexp
> > +			  (notmuch-json-partial-pos jp) (point-max) 0 nil
> > +			  (notmuch-json-partial-state jp)))))
> > +		  ;; A complete value is available if we've reached
> > +		  ;; depth 0 or less and encountered a complete
> > +		  ;; subexpression.
> > +		  (if (and (<= (first pstate) 0) (third pstate))
> > +		      t
> > +		    ;; Not complete.  Update the partial parser state
> > +		    (setf (notmuch-json-partial-pos jp) (point-marker)
> > +			  (notmuch-json-partial-state jp) pstate)
> > +		    nil))))))
> > +
> > +       (if (not complete)
> > +	   'retry
> > +	 ;; We have a value.  Reset the partial parse state and expect
> > +	 ;; a comma or terminator after the value.
> > +	 (setf (notmuch-json-next jp) 'expect-comma
> > +	       (notmuch-json-allow-term jp) t
> > +	       (notmuch-json-partial-pos jp) nil
> > +	       (notmuch-json-partial-state jp) nil)
> > +	 ;; Parse the value
> > +	 (let* ((json-object-type 'plist)
> > +		(json-array-type 'list)
> > +		(json-false nil))
> > +	   (json-read)))))))
> > +
> >  (provide 'notmuch-lib)
> >  
> >  ;; Local Variables:

Re: [PATCH 8/8] emacs: Switch from text to JSON format for search results

[Austin Clements]

Quoth Mark Walters on Jul 05 at  9:37 am:
> On Tue, 03 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > The JSON format eliminates the complex escaping issues that have
> > plagued the text search format.  This uses the incremental JSON parser
> > so that, like the text parser, it can output search results
> > incrementally.
> >
> > This slows down the parser by about ~4X, but puts us in a good
> > position to optimize either by improving the JSON parser (evidence
> > suggests this can reduce the overhead to ~40% over the text format) or
> > by switching to S-expressions (evidence suggests this will more than
> > double performance over the text parser).  [1]
> >
> > This also fixes the incremental search parsing test.
> >
> > [1] id:"20110720205007.GB21316@mit.edu"
> > ---
> >  emacs/notmuch.el |  113 ++++++++++++++++++++++++++++++++----------------------
> >  test/emacs       |    1 -
> >  2 files changed, 67 insertions(+), 47 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index 084cec6..2a09a98 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -60,7 +60,7 @@
> >  (require 'notmuch-message)
> >  
> >  (defcustom notmuch-search-result-format
> > -  `(("date" . "%s ")
> > +  `(("date" . "%12s ")
> >      ("count" . "%-7s ")
> >      ("authors" . "%-20s ")
> >      ("subject" . "%s ")
> > @@ -557,17 +557,14 @@ This function advances the next thread when finished."
> >    (notmuch-search-tag '("-inbox"))
> >    (notmuch-search-next-thread))
> >  
> > -(defvar notmuch-search-process-filter-data nil
> > -  "Data that has not yet been processed.")
> > -(make-variable-buffer-local 'notmuch-search-process-filter-data)
> > -
> >  (defun notmuch-search-process-sentinel (proc msg)
> >    "Add a message to let user know when \"notmuch search\" exits"
> >    (let ((buffer (process-buffer proc))
> >  	(status (process-status proc))
> >  	(exit-status (process-exit-status proc))
> >  	(never-found-target-thread nil))
> > -    (if (memq status '(exit signal))
> > +    (when (memq status '(exit signal))
> > +	(kill-buffer (process-get proc 'parse-buf))
> >  	(if (buffer-live-p buffer)
> >  	    (with-current-buffer buffer
> >  	      (save-excursion
> > @@ -577,8 +574,6 @@ This function advances the next thread when finished."
> >  		  (if (eq status 'signal)
> >  		      (insert "Incomplete search results (search process was killed).\n"))
> >  		  (when (eq status 'exit)
> > -		    (if notmuch-search-process-filter-data
> > -			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
> >  		    (insert "End of search results.")
> >  		    (unless (= exit-status 0)
> >  		      (insert (format " (process returned %d)" exit-status)))
> > @@ -757,45 +752,62 @@ non-authors is found, assume that all of the authors match."
> >      (insert (apply #'format string objects))
> >      (insert "\n")))
> >  
> > +(defvar notmuch-search-process-state nil
> > +  "Parsing state of the search process filter.")
> > +
> > +(defvar notmuch-search-json-parser nil
> > +  "Incremental JSON parser for the search process filter.")
> > +
> >  (defun notmuch-search-process-filter (proc string)
> >    "Process and filter the output of \"notmuch search\""
> > -  (let ((buffer (process-buffer proc)))
> > -    (if (buffer-live-p buffer)
> > -	(with-current-buffer buffer
> > -	    (let ((line 0)
> > -		  (more t)
> > -		  (inhibit-read-only t)
> > -		  (string (concat notmuch-search-process-filter-data string)))
> > -	      (setq notmuch-search-process-filter-data nil)
> > -	      (while more
> > -		(while (and (< line (length string)) (= (elt string line) ?\n))
> > -		  (setq line (1+ line)))
> > -		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
> > -		    (let* ((thread-id (match-string 1 string))
> > -			   (tags-str (match-string 7 string))
> > -			   (result (list :thread thread-id
> > -					 :date_relative (match-string 2 string)
> > -					 :matched (string-to-number
> > -						   (match-string 3 string))
> > -					 :total (string-to-number
> > -						 (match-string 4 string))
> > -					 :authors (match-string 5 string)
> > -					 :subject (match-string 6 string)
> > -					 :tags (if tags-str
> > -						   (save-match-data
> > -						     (split-string tags-str))))))
> > -		      (if (/= (match-beginning 0) line)
> > -			  (notmuch-search-show-error
> > -			   (substring string line (match-beginning 0))))
> > -		      (notmuch-search-show-result result)
> > -		      (set 'line (match-end 0)))
> > -		  (set 'more nil)
> > -		  (while (and (< line (length string)) (= (elt string line) ?\n))
> > -		    (setq line (1+ line)))
> > -		  (if (< line (length string))
> > -		      (setq notmuch-search-process-filter-data (substring string line)))
> > -		  ))))
> > -      (delete-process proc))))
> > +  (let ((results-buf (process-buffer proc))
> > +	(parse-buf (process-get proc 'parse-buf))
> > +	(inhibit-read-only t)
> > +	done)
> > +    (if (not (buffer-live-p results-buf))
> > +	(delete-process proc)
> > +      (with-current-buffer parse-buf
> > +	;; Insert new data
> > +	(save-excursion
> > +	  (goto-char (point-max))
> > +	  (insert string)))
> > +      (with-current-buffer results-buf
> > +	(while (not done)
> > +	  (condition-case nil
> > +	      (case notmuch-search-process-state
> > +		((begin)
> > +		 ;; Enter the results list
> > +		 (if (eq (notmuch-json-begin-compound
> > +			  notmuch-search-json-parser) 'retry)
> > +		     (setq done t)
> > +		   (setq notmuch-search-process-state 'result)))
> > +		((result)
> > +		 ;; Parse a result
> > +		 (let ((result (notmuch-json-read notmuch-search-json-parser)))
> > +		   (case result
> > +		     ((retry) (setq done t))
> > +		     ((end) (setq notmuch-search-process-state 'end))
> > +		     (otherwise (notmuch-search-show-result result)))))
> > +		((end)
> > +		 ;; Any trailing data is unexpected
> > +		 (with-current-buffer parse-buf
> > +		   (skip-chars-forward " \t\r\n")
> > +		   (if (eobp)
> > +		       (setq done t)
> > +		     (signal 'json-error nil)))))
> 
> This looks good to me. Would it make sense to put the "Any trailing
> data" as a tiny function in notmuch-lib? something like 
> 
> (defun notmuch-json-assert-end-of-data ()
>    (skip-chars-forward " \t\r\n") 
>    (if (eobp)
>        (setq done t) 
>      (signal 'json-error nil)))

Also a good idea.

Thanks for the review!  I'll be sending v2 shortly.

> Best wishes
> 
> Mark

[PATCH v2 0/9] JSON-based search-mode

[Austin Clements]

This should account for all of Mark's and Tomi's comments.  This
version
* renames the "format" variables to "format-string" and "spec" to be
  less confusing,
* reverts to the original behavior of ignoring the user's format
  specification for tags (since we make assumptions about this format
  elsewhere),
* swaps the error helper and search-target patches to fix the
  temporary issue with error message placement,
* adds documentation on point movement in the JSON parser,
* breaks out the JSON EOF testing function,
* beefs up a few commit messages,
* and adds a NEWS patch.

For ease of reviewing, the diff diff is below.

I've written most of a follow-on patch series that cleans up the
handling of metadata and tag changes by attaching the JSON result
object to the result.  This means we don't need a proliferation of
text properties to store the result metadata, and we can make updates
to a result (e.g., tag changes) by updating this result object and
then re-rendering the result line from scratch, rather than trying to
update the result line in place.  This makes it possible to obey user
formatting for the tag list and has other perks like recoloring
results when their tags change.  I'll send it along once this patch
series is accepted.

diff --git a/NEWS b/NEWS
index d29ec5b..a1a6e93 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,23 @@ Maildir tag synchronization
   messages (typically causing new messages to not receive the "unread"
   tag).
 
+Emacs Interface
+---------------
+
+Search now uses the JSON format internally
+
+  This should address problems with unusual characters in authors and
+  subject lines that could confuse the old text-based search parser.
+
+The date shown in search results is no longer padded before applying
+user-specified formatting
+
+  Previously, the date in the search results was padded to fixed width
+  before being formatted with `notmuch-search-result-format`.  It is
+  no longer padded.  The default format has been updated, but if
+  you've customized this variable, you may have to change your date
+  format from `"%s "` to `"%12s "`.
+
 Notmuch 0.13.2 (2012-06-02)
 ===========================
 
diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index f7cda33..9e04d97 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -399,8 +399,9 @@ resynchronize after an error by moving point."
 
 Returns 'retry if there is insufficient input to parse the
 beginning of the compound.  If this is able to parse the
-beginning of a compound, it returns t and later calls to
-`notmuch-json-read' will return the compound's elements.
+beginning of a compound, it moves point past the token that opens
+the compound and returns t.  Later calls to `notmuch-json-read'
+will return the compound's elements.
 
 Entering JSON objects is current unimplemented."
 
@@ -423,7 +424,8 @@ Entering JSON objects is current unimplemented."
 Returns 'retry if there is insufficient input to parse a complete
 JSON value.  If the parser is currently inside a compound value
 and the next token ends the list or object, returns 'end.
-Otherwise, returns the value."
+Otherwise, moves point to just past the end of the value and
+returns the value."
 
   (with-current-buffer (notmuch-json-buffer jp)
     (or
@@ -474,11 +476,22 @@ Otherwise, returns the value."
 	       (notmuch-json-partial-pos jp) nil
 	       (notmuch-json-partial-state jp) nil)
 	 ;; Parse the value
-	 (let* ((json-object-type 'plist)
-		(json-array-type 'list)
-		(json-false nil))
+	 (let ((json-object-type 'plist)
+	       (json-array-type 'list)
+	       (json-false nil))
 	   (json-read)))))))
 
+(defun notmuch-json-eof (jp)
+  "Signal a json-error if there is more input in JP's buffer.
+
+Moves point to the beginning of any trailing garbage or to the
+end of the buffer if there is no trailing garbage."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    (skip-chars-forward " \t\r\n")
+    (unless (eobp)
+      (signal 'json-error (list "Trailing garbage following JSON data")))))
+
 (provide 'notmuch-lib)
 
 ;; Local Variables:
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 2a09a98..fabb7c0 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -702,28 +702,29 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field format result)
+(defun notmuch-search-insert-field (field format-string result)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format format (plist-get result :date_relative))
+    (insert (propertize (format format-string (plist-get result :date_relative))
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format format (format "[%s/%s]"
-					       (plist-get result :matched)
-					       (plist-get result :total)))
+    (insert (propertize (format format-string
+				(format "[%s/%s]" (plist-get result :matched)
+					(plist-get result :total)))
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format format (plist-get result :subject))
+    (insert (propertize (format format-string (plist-get result :subject))
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors format (plist-get result :authors)))
+    (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
-    (insert
-     (format format (propertize
-		     (mapconcat 'identity (plist-get result :tags) " ")
-		     'font-lock-face 'notmuch-tag-face))))))
+    ;; Ignore format-string here because notmuch-search-set-tags
+    ;; depends on the format of this
+    (insert (concat "(" (propertize
+			 (mapconcat 'identity (plist-get result :tags) " ")
+			 'font-lock-face 'notmuch-tag-face) ")")))))
 
 (defun notmuch-search-show-result (result)
   ;; Ignore excluded matches
@@ -731,8 +732,8 @@ non-authors is found, assume that all of the authors match."
     (let ((beg (point-max)))
       (save-excursion
 	(goto-char beg)
-	(dolist (format notmuch-search-result-format)
-	  (notmuch-search-insert-field (car format) (cdr format) result))
+	(dolist (spec notmuch-search-result-format)
+	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
 	(put-text-property beg (point) 'notmuch-search-thread-id
@@ -790,11 +791,8 @@ non-authors is found, assume that all of the authors match."
 		     (otherwise (notmuch-search-show-result result)))))
 		((end)
 		 ;; Any trailing data is unexpected
-		 (with-current-buffer parse-buf
-		   (skip-chars-forward " \t\r\n")
-		   (if (eobp)
-		       (setq done t)
-		     (signal 'json-error nil)))))
+		 (notmuch-json-eof notmuch-search-json-parser)
+		 (setq done t)))
 	    (json-error
 	     ;; Do our best to resynchronize and ensure forward
 	     ;; progress

[PATCH v2 1/9] emacs: Clean up notmuch-search-show-result

[Austin Clements]

This simplifies the code and makes it no longer cubic in the number of
result fields.
---
 emacs/notmuch.el |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c6236db..8ad0b68 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -707,29 +707,30 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field date count authors subject tags)
+(defun notmuch-search-insert-field (field format-string date count authors subject tags)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
+    (insert (propertize (format format-string date)
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
+    (insert (propertize (format format-string count)
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
+    (insert (propertize (format format-string subject)
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
+    (notmuch-search-insert-authors format-string authors))
 
    ((string-equal field "tags")
+    ;; Ignore format-string here because notmuch-search-set-tags
+    ;; depends on the format of this
     (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
 
 (defun notmuch-search-show-result (date count authors subject tags)
-  (let ((fields) (field))
-    (setq fields (mapcar 'car notmuch-search-result-format))
-    (loop for field in fields
-	  do (notmuch-search-insert-field field date count authors subject tags)))
+  (dolist (spec notmuch-search-result-format)
+    (notmuch-search-insert-field (car spec) (cdr spec)
+				 date count authors subject tags))
   (insert "\n"))
 
 (defun notmuch-search-process-filter (proc string)
-- 
1.7.10

[PATCH v2 2/9] emacs: Separate search line parsing and display

[Austin Clements]

Previously, much of the display of search lines was done in the same
function that parsed the CLI's output.  Now the parsing function only
parses, and notmuch-search-show-result fully inserts the search result
in the search buffer.
---
 emacs/notmuch.el |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8ad0b68..746d0cb 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -727,11 +727,19 @@ non-authors is found, assume that all of the authors match."
     ;; depends on the format of this
     (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (date count authors subject tags)
-  (dolist (spec notmuch-search-result-format)
-    (notmuch-search-insert-field (car spec) (cdr spec)
-				 date count authors subject tags))
-  (insert "\n"))
+(defun notmuch-search-show-result (thread-id date count authors subject tags)
+  ;; Ignore excluded matches
+  (unless (eq (aref count 1) ?0)
+    (let ((beg (point))
+	  (tags-str (mapconcat 'identity tags " ")))
+      (dolist (spec notmuch-search-result-format)
+	(notmuch-search-insert-field (car spec) (cdr spec)
+				     date count authors subject tags-str))
+      (insert "\n")
+      (notmuch-search-color-line beg (point) tags)
+      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
+      (put-text-property beg (point) 'notmuch-search-authors authors)
+      (put-text-property beg (point) 'notmuch-search-subject subject))))
 
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
@@ -759,17 +767,10 @@ non-authors is found, assume that all of the authors match."
 		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
 			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
-		      ;; We currently just throw away excluded matches.
-		      (unless (eq (aref count 1) ?0)
-			(let ((beg (point)))
-			  (notmuch-search-show-result date count authors subject tags)
-			  (notmuch-search-color-line beg (point) tag-list)
-			  (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-			  (put-text-property beg (point) 'notmuch-search-authors authors)
-			  (put-text-property beg (point) 'notmuch-search-subject subject)
-			  (when (string= thread-id notmuch-search-target-thread)
-			    (set 'found-target beg)
-			    (set 'notmuch-search-target-thread "found"))))
+		      (when (string= thread-id notmuch-search-target-thread)
+			(set 'found-target (point))
+			(set 'notmuch-search-target-thread "found"))
+		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.10

[PATCH v2 3/9] emacs: Helper for reporting search parsing errors

[Austin Clements]

This removes the last bit of direct output from the parsing function.
With the parser now responsible solely for parsing, we can swap it out
for another parser.
---
 emacs/notmuch.el |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 746d0cb..f952fa8 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -741,6 +741,11 @@ non-authors is found, assume that all of the authors match."
       (put-text-property beg (point) 'notmuch-search-authors authors)
       (put-text-property beg (point) 'notmuch-search-subject subject))))
 
+(defun notmuch-search-show-error (string &rest objects)
+  (insert "Error: Unexpected output from notmuch search:\n")
+  (insert (apply #'format string objects))
+  (insert "\n"))
+
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
   (let ((buffer (process-buffer proc))
@@ -766,7 +771,8 @@ non-authors is found, assume that all of the authors match."
 			   (tag-list (if tags (save-match-data (split-string tags)))))
 		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
-			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
+			  (notmuch-search-show-error
+			   (substring string line (match-beginning 1))))
 		      (when (string= thread-id notmuch-search-target-thread)
 			(set 'found-target (point))
 			(set 'notmuch-search-target-thread "found"))
-- 
1.7.10

[PATCH v2 4/9] emacs: Move search-target logic to `notmuch-search-show-result'

[Austin Clements]

This is a simpler place to do this, since we can avoid any point
motion and hence any save-excursions in
`notmuch-search-process-filter', which in turn lets us put all of the
search-target logic outside of any save-excursions.

`notmuch-search-show-{result,error}' are now responsible for their own
point motion.

`notmuch-search-process-filter' could use some reindentation after
this, but we're about to rewrite it entirely, so we won't bother.
---
 emacs/notmuch.el |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f952fa8..5bf01ca 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -730,29 +730,34 @@ non-authors is found, assume that all of the authors match."
 (defun notmuch-search-show-result (thread-id date count authors subject tags)
   ;; Ignore excluded matches
   (unless (eq (aref count 1) ?0)
-    (let ((beg (point))
+    (let ((beg (point-max))
 	  (tags-str (mapconcat 'identity tags " ")))
-      (dolist (spec notmuch-search-result-format)
-	(notmuch-search-insert-field (car spec) (cdr spec)
-				     date count authors subject tags-str))
-      (insert "\n")
-      (notmuch-search-color-line beg (point) tags)
-      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-      (put-text-property beg (point) 'notmuch-search-authors authors)
-      (put-text-property beg (point) 'notmuch-search-subject subject))))
+      (save-excursion
+	(goto-char beg)
+	(dolist (spec notmuch-search-result-format)
+	  (notmuch-search-insert-field (car spec) (cdr spec)
+				       date count authors subject tags-str))
+	(insert "\n")
+	(notmuch-search-color-line beg (point) tags)
+	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
+	(put-text-property beg (point) 'notmuch-search-authors authors)
+	(put-text-property beg (point) 'notmuch-search-subject subject))
+      (when (string= thread-id notmuch-search-target-thread)
+	(setq notmuch-search-target-thread "found")
+	(goto-char beg)))))
 
 (defun notmuch-search-show-error (string &rest objects)
-  (insert "Error: Unexpected output from notmuch search:\n")
-  (insert (apply #'format string objects))
-  (insert "\n"))
+  (save-excursion
+    (goto-char (point-max))
+    (insert "Error: Unexpected output from notmuch search:\n")
+    (insert (apply #'format string objects))
+    (insert "\n")))
 
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
-  (let ((buffer (process-buffer proc))
-	(found-target nil))
+  (let ((buffer (process-buffer proc)))
     (if (buffer-live-p buffer)
 	(with-current-buffer buffer
-	  (save-excursion
 	    (let ((line 0)
 		  (more t)
 		  (inhibit-read-only t)
@@ -769,13 +774,9 @@ non-authors is found, assume that all of the authors match."
 			   (subject (match-string 5 string))
 			   (tags (match-string 6 string))
 			   (tag-list (if tags (save-match-data (split-string tags)))))
-		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
 			  (notmuch-search-show-error
 			   (substring string line (match-beginning 1))))
-		      (when (string= thread-id notmuch-search-target-thread)
-			(set 'found-target (point))
-			(set 'notmuch-search-target-thread "found"))
 		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
@@ -784,8 +785,6 @@ non-authors is found, assume that all of the authors match."
 		  (if (< line (length string))
 		      (setq notmuch-search-process-filter-data (substring string line)))
 		  ))))
-	  (if found-target
-	      (goto-char found-target)))
       (delete-process proc))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
-- 
1.7.10

[PATCH v2 5/9] emacs: Pass plist to `notmuch-search-show-result'

[Austin Clements]

Rather than passing lots of arguments and then further passing those
to `notmuch-search-insert-field', pass a plist containing all of the
search result information.  This plist is compatible with the JSON
format search results.
---
 emacs/notmuch.el |   65 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 5bf01ca..dfeaf35 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -707,42 +707,47 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field format-string date count authors subject tags)
+(defun notmuch-search-insert-field (field format-string result)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format format-string date)
+    (insert (propertize (format format-string (plist-get result :date_relative))
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format format-string count)
+    (insert (propertize (format format-string
+				(format "[%s/%s]" (plist-get result :matched)
+					(plist-get result :total)))
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format format-string subject)
+    (insert (propertize (format format-string (plist-get result :subject))
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors format-string authors))
+    (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
     ;; Ignore format-string here because notmuch-search-set-tags
     ;; depends on the format of this
-    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
+    (insert (concat "(" (propertize
+			 (mapconcat 'identity (plist-get result :tags) " ")
+			 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (thread-id date count authors subject tags)
+(defun notmuch-search-show-result (result)
   ;; Ignore excluded matches
-  (unless (eq (aref count 1) ?0)
-    (let ((beg (point-max))
-	  (tags-str (mapconcat 'identity tags " ")))
+  (unless (= (plist-get result :matched) 0)
+    (let ((beg (point-max)))
       (save-excursion
 	(goto-char beg)
 	(dolist (spec notmuch-search-result-format)
-	  (notmuch-search-insert-field (car spec) (cdr spec)
-				       date count authors subject tags-str))
+	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
-	(notmuch-search-color-line beg (point) tags)
-	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-	(put-text-property beg (point) 'notmuch-search-authors authors)
-	(put-text-property beg (point) 'notmuch-search-subject subject))
-      (when (string= thread-id notmuch-search-target-thread)
+	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-thread-id
+			   (concat "thread:" (plist-get result :thread)))
+	(put-text-property beg (point) 'notmuch-search-authors
+			   (plist-get result :authors))
+	(put-text-property beg (point) 'notmuch-search-subject
+			   (plist-get result :subject)))
+      (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
 
@@ -766,18 +771,24 @@ non-authors is found, assume that all of the authors match."
 	      (while more
 		(while (and (< line (length string)) (= (elt string line) ?\n))
 		  (setq line (1+ line)))
-		(if (string-match "^\\(thread:[0-9A-Fa-f]*\\) \\([^][]*\\) \\(\\[[0-9/]*\\]\\) \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
+		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
 		    (let* ((thread-id (match-string 1 string))
-			   (date (match-string 2 string))
-			   (count (match-string 3 string))
-			   (authors (match-string 4 string))
-			   (subject (match-string 5 string))
-			   (tags (match-string 6 string))
-			   (tag-list (if tags (save-match-data (split-string tags)))))
-		      (if (/= (match-beginning 1) line)
+			   (tags-str (match-string 7 string))
+			   (result (list :thread thread-id
+					 :date_relative (match-string 2 string)
+					 :matched (string-to-number
+						   (match-string 3 string))
+					 :total (string-to-number
+						 (match-string 4 string))
+					 :authors (match-string 5 string)
+					 :subject (match-string 6 string)
+					 :tags (if tags-str
+						   (save-match-data
+						     (split-string tags-str))))))
+		      (if (/= (match-beginning 0) line)
 			  (notmuch-search-show-error
-			   (substring string line (match-beginning 1))))
-		      (notmuch-search-show-result thread-id date count authors subject tag-list)
+			   (substring string line (match-beginning 0))))
+		      (notmuch-search-show-result result)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.10

[PATCH v2 6/9] test: New test for incremental search output parsing

[Austin Clements]

This advises the search process filter to make it process one
character at a time in order to test the pessimal case for incremental
search output parsing.

The text parser fails this test because it gets tricked into thinking
a parenthetical remark in a subject is the tag list.
---
 test/emacs       |   11 +++++++++++
 test/test-lib.el |    8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/test/emacs b/test/emacs
index e9f954c..293b12a 100755
--- a/test/emacs
+++ b/test/emacs
@@ -35,6 +35,17 @@ test_emacs '(notmuch-search "tag:inbox")
 	    (test-output)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
 
+test_begin_subtest "Incremental parsing of search results"
+test_subtest_known_broken
+test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
+	    (ad-activate 'notmuch-search-process-filter)
+	    (notmuch-search \"tag:inbox\")
+	    (notmuch-test-wait)
+	    (ad-disable-advice 'notmuch-search-process-filter 'around 'pessimal)
+	    (ad-activate 'notmuch-search-process-filter)
+	    (test-output)"
+test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
+
 test_begin_subtest "Navigation of notmuch-hello to search results"
 test_emacs '(notmuch-hello)
 	    (goto-char (point-min))
diff --git a/test/test-lib.el b/test/test-lib.el
index 6271da2..5dd6271 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -89,6 +89,14 @@ nothing."
 (add-hook-counter 'notmuch-hello-mode-hook)
 (add-hook-counter 'notmuch-hello-refresh-hook)
 
+(defadvice notmuch-search-process-filter (around pessimal activate disable)
+  "Feed notmuch-search-process-filter one character at a time."
+  (let ((string (ad-get-arg 1)))
+    (loop for char across string
+	  do (progn
+	       (ad-set-arg 1 (char-to-string char))
+	       ad-do-it))))
+
 (defmacro notmuch-test-run (&rest body)
   "Evaluate a BODY of test expressions and output the result."
   `(with-temp-buffer
-- 
1.7.10

[PATCH v2 7/9] emacs: Implement an incremental JSON parser

[Austin Clements]

This parser is designed to read streaming JSON whose structure is
known to the caller.  Like a typical JSON parsing interface, it
provides a function to read a complete JSON value from the input.
However, it extends this with an additional function that
requires the next value in the input to be a compound value and
descends into it, allowing its elements to be read one at a time
or further descended into.  Both functions can return 'retry to
indicate that not enough input is available.

The parser supports efficient partial parsing, so there's no need to
frame the input for correctness or performance.

The bulk of the parsing is still done by Emacs' json.el, so any
improvements or optimizations to that will benefit the incremental
parser as well.

Currently only descending into JSON lists is supported because that's
all we need, but support for descending into JSON objects can be added
in the future.
---
 emacs/notmuch-lib.el |  196 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 196 insertions(+)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index c829df3..9e04d97 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -23,6 +23,7 @@
 
 (require 'mm-view)
 (require 'mm-decode)
+(require 'json)
 (eval-when-compile (require 'cl))
 
 (defvar notmuch-command "notmuch"
@@ -296,6 +297,201 @@ was called."
 (defvar notmuch-show-process-crypto nil)
 (make-variable-buffer-local 'notmuch-show-process-crypto)
 
+;; Incremental JSON parsing
+
+(defun notmuch-json-create-parser (buffer)
+  "Return a streaming JSON parser that consumes input from BUFFER.
+
+This parser is designed to read streaming JSON whose structure is
+known to the caller.  Like a typical JSON parsing interface, it
+provides a function to read a complete JSON value from the input.
+However, it extends this with an additional function that
+requires the next value in the input to be a compound value and
+descends into it, allowing its elements to be read one at a time
+or further descended into.  Both functions can return 'retry to
+indicate that not enough input is available.
+
+The parser always consumes input from BUFFER's point.  Hence, the
+caller is allowed to delete and data before point and may
+resynchronize after an error by moving point."
+
+  (list buffer
+	;; Terminator stack: a stack of characters that indicate the
+	;; end of the compound values enclosing point
+	'()
+	;; Next: One of
+	;; * 'expect-value if the next token must be a value, but a
+	;;   value has not yet been reached
+	;; * 'value if point is at the beginning of a value
+	;; * 'expect-comma if the next token must be a comma
+	'expect-value
+	;; Allow terminator: non-nil if the next token may be a
+	;; terminator
+	nil
+	;; Partial parse position: If state is 'value, a marker for
+	;; the position of the partial parser or nil if no partial
+	;; parsing has happened yet
+	nil
+	;; Partial parse state: If state is 'value, the current
+	;; `parse-partial-sexp' state
+	nil))
+
+(defmacro notmuch-json-buffer (jp) `(first ,jp))
+(defmacro notmuch-json-term-stack (jp) `(second ,jp))
+(defmacro notmuch-json-next (jp) `(third ,jp))
+(defmacro notmuch-json-allow-term (jp) `(fourth ,jp))
+(defmacro notmuch-json-partial-pos (jp) `(fifth ,jp))
+(defmacro notmuch-json-partial-state (jp) `(sixth ,jp))
+
+(defvar notmuch-json-syntax-table
+  (let ((table (make-syntax-table)))
+    ;; The standard syntax table is what we need except that "." needs
+    ;; to have word syntax instead of punctuation syntax.
+    (modify-syntax-entry ?. "w" table)
+    table)
+  "Syntax table used for incremental JSON parsing.")
+
+(defun notmuch-json-scan-to-value (jp)
+  ;; Helper function that consumes separators, terminators, and
+  ;; whitespace from point.  Returns nil if it successfully reached
+  ;; the beginning of a value, 'end if it consumed a terminator, or
+  ;; 'retry if not enough input was available to reach a value.  Upon
+  ;; nil return, (notmuch-json-next jp) is always 'value.
+
+  (if (eq (notmuch-json-next jp) 'value)
+      ;; We're already at a value
+      nil
+    ;; Drive the state toward 'expect-value
+    (skip-chars-forward " \t\r\n")
+    (or (when (eobp) 'retry)
+	;; Test for the terminator for the current compound
+	(when (and (notmuch-json-allow-term jp)
+		   (eq (char-after) (car (notmuch-json-term-stack jp))))
+	  ;; Consume it and expect a comma or terminator next
+	  (forward-char)
+	  (setf (notmuch-json-term-stack jp) (cdr (notmuch-json-term-stack jp))
+		(notmuch-json-next jp) 'expect-comma
+		(notmuch-json-allow-term jp) t)
+	  'end)
+	;; Test for a separator
+	(when (eq (notmuch-json-next jp) 'expect-comma)
+	  (when (/= (char-after) ?,)
+	    (signal 'json-readtable-error (list "expected ','")))
+	  ;; Consume it, switch to 'expect-value, and disallow a
+	  ;; terminator
+	  (forward-char)
+	  (skip-chars-forward " \t\r\n")
+	  (setf (notmuch-json-next jp) 'expect-value
+		(notmuch-json-allow-term jp) nil)
+	  ;; We moved point, so test for eobp again and fall through
+	  ;; to the next test if there's more input
+	  (when (eobp) 'retry))
+	;; Next must be 'expect-value and we know this isn't
+	;; whitespace, EOB, or a terminator, so point must be on a
+	;; value
+	(progn
+	  (assert (eq (notmuch-json-next jp) 'expect-value))
+	  (setf (notmuch-json-next jp) 'value)
+	  nil))))
+
+(defun notmuch-json-begin-compound (jp)
+  "Parse the beginning of a compound value and traverse inside it.
+
+Returns 'retry if there is insufficient input to parse the
+beginning of the compound.  If this is able to parse the
+beginning of a compound, it moves point past the token that opens
+the compound and returns t.  Later calls to `notmuch-json-read'
+will return the compound's elements.
+
+Entering JSON objects is current unimplemented."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    ;; Disallow terminators
+    (setf (notmuch-json-allow-term jp) nil)
+    (or (notmuch-json-scan-to-value jp)
+	(if (/= (char-after) ?\[)
+	    (signal 'json-readtable-error (list "expected '['"))
+	  (forward-char)
+	  (push ?\] (notmuch-json-term-stack jp))
+	  ;; Expect a value or terminator next
+	  (setf (notmuch-json-next jp) 'expect-value
+		(notmuch-json-allow-term jp) t)
+	  t))))
+
+(defun notmuch-json-read (jp)
+  "Parse the value at point in JP's buffer.
+
+Returns 'retry if there is insufficient input to parse a complete
+JSON value.  If the parser is currently inside a compound value
+and the next token ends the list or object, returns 'end.
+Otherwise, moves point to just past the end of the value and
+returns the value."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    (or
+     ;; Get to a value state
+     (notmuch-json-scan-to-value jp)
+
+     ;; Can we parse a complete value?
+     (let ((complete
+	    (if (looking-at "[-+0-9tfn]")
+		;; This is a number or a keyword, so the partial
+		;; parser isn't going to help us because a truncated
+		;; number or keyword looks like a complete symbol to
+		;; it.  Look for something that clearly ends it.
+		(save-excursion
+		  (skip-chars-forward "^]},: \t\r\n")
+		  (not (eobp)))
+
+	      ;; We're looking at a string, object, or array, which we
+	      ;; can partial parse.  If we just reached the value, set
+	      ;; up the partial parser.
+	      (when (null (notmuch-json-partial-state jp))
+		(setf (notmuch-json-partial-pos jp) (point-marker)))
+
+	      ;; Extend the partial parse until we either reach EOB or
+	      ;; get the whole value
+	      (save-excursion
+		(let ((pstate
+		       (with-syntax-table notmuch-json-syntax-table
+			 (parse-partial-sexp
+			  (notmuch-json-partial-pos jp) (point-max) 0 nil
+			  (notmuch-json-partial-state jp)))))
+		  ;; A complete value is available if we've reached
+		  ;; depth 0 or less and encountered a complete
+		  ;; subexpression.
+		  (if (and (<= (first pstate) 0) (third pstate))
+		      t
+		    ;; Not complete.  Update the partial parser state
+		    (setf (notmuch-json-partial-pos jp) (point-marker)
+			  (notmuch-json-partial-state jp) pstate)
+		    nil))))))
+
+       (if (not complete)
+	   'retry
+	 ;; We have a value.  Reset the partial parse state and expect
+	 ;; a comma or terminator after the value.
+	 (setf (notmuch-json-next jp) 'expect-comma
+	       (notmuch-json-allow-term jp) t
+	       (notmuch-json-partial-pos jp) nil
+	       (notmuch-json-partial-state jp) nil)
+	 ;; Parse the value
+	 (let ((json-object-type 'plist)
+	       (json-array-type 'list)
+	       (json-false nil))
+	   (json-read)))))))
+
+(defun notmuch-json-eof (jp)
+  "Signal a json-error if there is more input in JP's buffer.
+
+Moves point to the beginning of any trailing garbage or to the
+end of the buffer if there is no trailing garbage."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    (skip-chars-forward " \t\r\n")
+    (unless (eobp)
+      (signal 'json-error (list "Trailing garbage following JSON data")))))
+
 (provide 'notmuch-lib)
 
 ;; Local Variables:
-- 
1.7.10

[PATCH v2 8/9] emacs: Switch from text to JSON format for search results

[Austin Clements]

The JSON format eliminates the complex escaping issues that have
plagued the text search format.  This uses the incremental JSON parser
so that, like the text parser, it can output search results
incrementally.

This slows down the parser by about ~4X, but puts us in a good
position to optimize either by improving the JSON parser (evidence
suggests this can reduce the overhead to ~40% over the text format) or
by switching to S-expressions (evidence suggests this will more than
double performance over the text parser).  [1]

This also fixes the incremental search parsing test.

This has one minor side-effect on search result formatting.
Previously, the date field was always padded to a fixed width of 12
characters because of how the text parser's regexp was written.  The
JSON format doesn't do this.  We could pad it out in Emacs before
formatting it, but, since all of the other fields are variable width,
we instead fix notmuch-search-result-format to take the variable-width
field and pad it out.  For users who have customized this variable,
we'll mention in the NEWS how to fix this slight format change.

[1] id:"20110720205007.GB21316@mit.edu"
---
 emacs/notmuch.el |  110 +++++++++++++++++++++++++++++++-----------------------
 test/emacs       |    1 -
 2 files changed, 64 insertions(+), 47 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index dfeaf35..fabb7c0 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -60,7 +60,7 @@
 (require 'notmuch-message)
 
 (defcustom notmuch-search-result-format
-  `(("date" . "%s ")
+  `(("date" . "%12s ")
     ("count" . "%-7s ")
     ("authors" . "%-20s ")
     ("subject" . "%s ")
@@ -557,17 +557,14 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
-(defvar notmuch-search-process-filter-data nil
-  "Data that has not yet been processed.")
-(make-variable-buffer-local 'notmuch-search-process-filter-data)
-
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
 	(status (process-status proc))
 	(exit-status (process-exit-status proc))
 	(never-found-target-thread nil))
-    (if (memq status '(exit signal))
+    (when (memq status '(exit signal))
+	(kill-buffer (process-get proc 'parse-buf))
 	(if (buffer-live-p buffer)
 	    (with-current-buffer buffer
 	      (save-excursion
@@ -577,8 +574,6 @@ This function advances the next thread when finished."
 		  (if (eq status 'signal)
 		      (insert "Incomplete search results (search process was killed).\n"))
 		  (when (eq status 'exit)
-		    (if notmuch-search-process-filter-data
-			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
 		    (insert "End of search results.")
 		    (unless (= exit-status 0)
 		      (insert (format " (process returned %d)" exit-status)))
@@ -758,45 +753,59 @@ non-authors is found, assume that all of the authors match."
     (insert (apply #'format string objects))
     (insert "\n")))
 
+(defvar notmuch-search-process-state nil
+  "Parsing state of the search process filter.")
+
+(defvar notmuch-search-json-parser nil
+  "Incremental JSON parser for the search process filter.")
+
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
-  (let ((buffer (process-buffer proc)))
-    (if (buffer-live-p buffer)
-	(with-current-buffer buffer
-	    (let ((line 0)
-		  (more t)
-		  (inhibit-read-only t)
-		  (string (concat notmuch-search-process-filter-data string)))
-	      (setq notmuch-search-process-filter-data nil)
-	      (while more
-		(while (and (< line (length string)) (= (elt string line) ?\n))
-		  (setq line (1+ line)))
-		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
-		    (let* ((thread-id (match-string 1 string))
-			   (tags-str (match-string 7 string))
-			   (result (list :thread thread-id
-					 :date_relative (match-string 2 string)
-					 :matched (string-to-number
-						   (match-string 3 string))
-					 :total (string-to-number
-						 (match-string 4 string))
-					 :authors (match-string 5 string)
-					 :subject (match-string 6 string)
-					 :tags (if tags-str
-						   (save-match-data
-						     (split-string tags-str))))))
-		      (if (/= (match-beginning 0) line)
-			  (notmuch-search-show-error
-			   (substring string line (match-beginning 0))))
-		      (notmuch-search-show-result result)
-		      (set 'line (match-end 0)))
-		  (set 'more nil)
-		  (while (and (< line (length string)) (= (elt string line) ?\n))
-		    (setq line (1+ line)))
-		  (if (< line (length string))
-		      (setq notmuch-search-process-filter-data (substring string line)))
-		  ))))
-      (delete-process proc))))
+  (let ((results-buf (process-buffer proc))
+	(parse-buf (process-get proc 'parse-buf))
+	(inhibit-read-only t)
+	done)
+    (if (not (buffer-live-p results-buf))
+	(delete-process proc)
+      (with-current-buffer parse-buf
+	;; Insert new data
+	(save-excursion
+	  (goto-char (point-max))
+	  (insert string)))
+      (with-current-buffer results-buf
+	(while (not done)
+	  (condition-case nil
+	      (case notmuch-search-process-state
+		((begin)
+		 ;; Enter the results list
+		 (if (eq (notmuch-json-begin-compound
+			  notmuch-search-json-parser) 'retry)
+		     (setq done t)
+		   (setq notmuch-search-process-state 'result)))
+		((result)
+		 ;; Parse a result
+		 (let ((result (notmuch-json-read notmuch-search-json-parser)))
+		   (case result
+		     ((retry) (setq done t))
+		     ((end) (setq notmuch-search-process-state 'end))
+		     (otherwise (notmuch-search-show-result result)))))
+		((end)
+		 ;; Any trailing data is unexpected
+		 (notmuch-json-eof notmuch-search-json-parser)
+		 (setq done t)))
+	    (json-error
+	     ;; Do our best to resynchronize and ensure forward
+	     ;; progress
+	     (notmuch-search-show-error
+	      "%s"
+	      (with-current-buffer parse-buf
+		(let ((bad (buffer-substring (line-beginning-position)
+					     (line-end-position))))
+		  (forward-line)
+		  bad))))))
+	;; Clear out what we've parsed
+	(with-current-buffer parse-buf
+	  (delete-region (point-min) (point)))))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
   "Add/remove tags from all messages in current search buffer.
@@ -899,10 +908,19 @@ Other optional parameters are used as follows:
 	(let ((proc (start-process
 		     "notmuch-search" buffer
 		     notmuch-command "search"
+		     "--format=json"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
-		     query)))
+		     query))
+	      ;; Use a scratch buffer to accumulate partial output.
+	      ;; This buffer will be killed by the sentinel, which
+	      ;; should be called no matter how the process dies.
+	      (parse-buf (generate-new-buffer " *notmuch search parse*")))
+	  (set (make-local-variable 'notmuch-search-process-state) 'begin)
+	  (set (make-local-variable 'notmuch-search-json-parser)
+	       (notmuch-json-create-parser parse-buf))
+	  (process-put proc 'parse-buf parse-buf)
 	  (set-process-sentinel proc 'notmuch-search-process-sentinel)
 	  (set-process-filter proc 'notmuch-search-process-filter)
 	  (set-process-query-on-exit-flag proc nil))))
diff --git a/test/emacs b/test/emacs
index 293b12a..afe35ba 100755
--- a/test/emacs
+++ b/test/emacs
@@ -36,7 +36,6 @@ test_emacs '(notmuch-search "tag:inbox")
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
 
 test_begin_subtest "Incremental parsing of search results"
-test_subtest_known_broken
 test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
 	    (ad-activate 'notmuch-search-process-filter)
 	    (notmuch-search \"tag:inbox\")
-- 
1.7.10

[PATCH v2 9/9] News for JSON-based search

[Austin Clements]

---
 NEWS |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/NEWS b/NEWS
index d29ec5b..a1a6e93 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,23 @@ Maildir tag synchronization
   messages (typically causing new messages to not receive the "unread"
   tag).
 
+Emacs Interface
+---------------
+
+Search now uses the JSON format internally
+
+  This should address problems with unusual characters in authors and
+  subject lines that could confuse the old text-based search parser.
+
+The date shown in search results is no longer padded before applying
+user-specified formatting
+
+  Previously, the date in the search results was padded to fixed width
+  before being formatted with `notmuch-search-result-format`.  It is
+  no longer padded.  The default format has been updated, but if
+  you've customized this variable, you may have to change your date
+  format from `"%s "` to `"%12s "`.
+
 Notmuch 0.13.2 (2012-06-02)
 ===========================
 
-- 
1.7.10

Re: [PATCH v2 0/9] JSON-based search-mode

[Mark Walters]

On Thu, 05 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This should account for all of Mark's and Tomi's comments.  This
> version
> * renames the "format" variables to "format-string" and "spec" to be
>   less confusing,
> * reverts to the original behavior of ignoring the user's format
>   specification for tags (since we make assumptions about this format
>   elsewhere),
> * swaps the error helper and search-target patches to fix the
>   temporary issue with error message placement,
> * adds documentation on point movement in the JSON parser,
> * breaks out the JSON EOF testing function,
> * beefs up a few commit messages,
> * and adds a NEWS patch.
>
> For ease of reviewing, the diff diff is below.
>
> I've written most of a follow-on patch series that cleans up the
> handling of metadata and tag changes by attaching the JSON result
> object to the result.  This means we don't need a proliferation of
> text properties to store the result metadata, and we can make updates
> to a result (e.g., tag changes) by updating this result object and
> then re-rendering the result line from scratch, rather than trying to
> update the result line in place.  This makes it possible to obey user
> formatting for the tag list and has other perks like recoloring
> results when their tags change.  I'll send it along once this patch
> series is accepted.
>
> diff --git a/NEWS b/NEWS
> index d29ec5b..a1a6e93 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,23 @@ Maildir tag synchronization
>    messages (typically causing new messages to not receive the "unread"
>    tag).
>  
> +Emacs Interface
> +---------------
> +
> +Search now uses the JSON format internally
> +
> +  This should address problems with unusual characters in authors and
> +  subject lines that could confuse the old text-based search parser.
> +
> +The date shown in search results is no longer padded before applying
> +user-specified formatting
> +
> +  Previously, the date in the search results was padded to fixed width
> +  before being formatted with `notmuch-search-result-format`.  It is
> +  no longer padded.  The default format has been updated, but if
> +  you've customized this variable, you may have to change your date
> +  format from `"%s "` to `"%12s "`.
> +
>  Notmuch 0.13.2 (2012-06-02)
>  ===========================
>  
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index f7cda33..9e04d97 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -399,8 +399,9 @@ resynchronize after an error by moving point."
>  
>  Returns 'retry if there is insufficient input to parse the
>  beginning of the compound.  If this is able to parse the
> -beginning of a compound, it returns t and later calls to
> -`notmuch-json-read' will return the compound's elements.
> +beginning of a compound, it moves point past the token that opens
> +the compound and returns t.  Later calls to `notmuch-json-read'
> +will return the compound's elements.
>  
>  Entering JSON objects is current unimplemented."
>  
> @@ -423,7 +424,8 @@ Entering JSON objects is current unimplemented."
>  Returns 'retry if there is insufficient input to parse a complete
>  JSON value.  If the parser is currently inside a compound value
>  and the next token ends the list or object, returns 'end.
> -Otherwise, returns the value."
> +Otherwise, moves point to just past the end of the value and
> +returns the value."

I think that point can move when 'retry is returned (past terminators
and commas for example). It might also be worth saying that it returns
the next value passing command and terminators and whitespace after
point.

>  
>    (with-current-buffer (notmuch-json-buffer jp)
>      (or
> @@ -474,11 +476,22 @@ Otherwise, returns the value."
>  	       (notmuch-json-partial-pos jp) nil
>  	       (notmuch-json-partial-state jp) nil)
>  	 ;; Parse the value
> -	 (let* ((json-object-type 'plist)
> -		(json-array-type 'list)
> -		(json-false nil))
> +	 (let ((json-object-type 'plist)
> +	       (json-array-type 'list)
> +	       (json-false nil))
>  	   (json-read)))))))
>  
> +(defun notmuch-json-eof (jp)
> +  "Signal a json-error if there is more input in JP's buffer.

Would `data' be better than `input' (to distinguish allowed whitespace
from disallowed data)?

> +Moves point to the beginning of any trailing garbage or to the
> +end of the buffer if there is no trailing garbage."
> +
> +  (with-current-buffer (notmuch-json-buffer jp)
> +    (skip-chars-forward " \t\r\n")
> +    (unless (eobp)
> +      (signal 'json-error (list "Trailing garbage following JSON data")))))
> +
>  (provide 'notmuch-lib)
>  
>  ;; Local Variables:
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 2a09a98..fabb7c0 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -702,28 +702,29 @@ non-authors is found, assume that all of the authors match."
>  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
>        (insert padding))))
>  
> -(defun notmuch-search-insert-field (field format result)
> +(defun notmuch-search-insert-field (field format-string result)
>    (cond
>     ((string-equal field "date")
> -    (insert (propertize (format format (plist-get result :date_relative))
> +    (insert (propertize (format format-string (plist-get result :date_relative))
>  			'face 'notmuch-search-date)))
>     ((string-equal field "count")
> -    (insert (propertize (format format (format "[%s/%s]"
> -					       (plist-get result :matched)
> -					       (plist-get result :total)))
> +    (insert (propertize (format format-string
> +				(format "[%s/%s]" (plist-get result :matched)
> +					(plist-get result :total)))
>  			'face 'notmuch-search-count)))
>     ((string-equal field "subject")
> -    (insert (propertize (format format (plist-get result :subject))
> +    (insert (propertize (format format-string (plist-get result :subject))
>  			'face 'notmuch-search-subject)))
>  
>     ((string-equal field "authors")
> -    (notmuch-search-insert-authors format (plist-get result :authors)))
> +    (notmuch-search-insert-authors format-string (plist-get result :authors)))
>  
>     ((string-equal field "tags")
> -    (insert
> -     (format format (propertize
> -		     (mapconcat 'identity (plist-get result :tags) " ")
> -		     'font-lock-face 'notmuch-tag-face))))))
> +    ;; Ignore format-string here because notmuch-search-set-tags
> +    ;; depends on the format of this
> +    (insert (concat "(" (propertize
> +			 (mapconcat 'identity (plist-get result :tags) " ")
> +			 'font-lock-face 'notmuch-tag-face) ")")))))
>  
>  (defun notmuch-search-show-result (result)
>    ;; Ignore excluded matches
> @@ -731,8 +732,8 @@ non-authors is found, assume that all of the authors match."
>      (let ((beg (point-max)))
>        (save-excursion
>  	(goto-char beg)
> -	(dolist (format notmuch-search-result-format)
> -	  (notmuch-search-insert-field (car format) (cdr format) result))
> +	(dolist (spec notmuch-search-result-format)
> +	  (notmuch-search-insert-field (car spec) (cdr spec) result))
>  	(insert "\n")
>  	(notmuch-search-color-line beg (point) (plist-get result :tags))
>  	(put-text-property beg (point) 'notmuch-search-thread-id
> @@ -790,11 +791,8 @@ non-authors is found, assume that all of the authors match."
>  		     (otherwise (notmuch-search-show-result result)))))
>  		((end)
>  		 ;; Any trailing data is unexpected
> -		 (with-current-buffer parse-buf
> -		   (skip-chars-forward " \t\r\n")
> -		   (if (eobp)
> -		       (setq done t)
> -		     (signal 'json-error nil)))))
> +		 (notmuch-json-eof notmuch-search-json-parser)
> +		 (setq done t)))

I think this used to only set `done' if there was not trailing data but
now does so anyway?

Best wishes

Mark

>  	    (json-error
>  	     ;; Do our best to resynchronize and ensure forward
>  	     ;; progress

Re: [PATCH v2 0/9] JSON-based search-mode

[Austin Clements]

Quoth Mark Walters on Jul 05 at 10:44 pm:
> On Thu, 05 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This should account for all of Mark's and Tomi's comments.  This
> > version
> > * renames the "format" variables to "format-string" and "spec" to be
> >   less confusing,
> > * reverts to the original behavior of ignoring the user's format
> >   specification for tags (since we make assumptions about this format
> >   elsewhere),
> > * swaps the error helper and search-target patches to fix the
> >   temporary issue with error message placement,
> > * adds documentation on point movement in the JSON parser,
> > * breaks out the JSON EOF testing function,
> > * beefs up a few commit messages,
> > * and adds a NEWS patch.
> >
> > For ease of reviewing, the diff diff is below.
> >
> > I've written most of a follow-on patch series that cleans up the
> > handling of metadata and tag changes by attaching the JSON result
> > object to the result.  This means we don't need a proliferation of
> > text properties to store the result metadata, and we can make updates
> > to a result (e.g., tag changes) by updating this result object and
> > then re-rendering the result line from scratch, rather than trying to
> > update the result line in place.  This makes it possible to obey user
> > formatting for the tag list and has other perks like recoloring
> > results when their tags change.  I'll send it along once this patch
> > series is accepted.
> >
> > diff --git a/NEWS b/NEWS
> > index d29ec5b..a1a6e93 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -14,6 +14,23 @@ Maildir tag synchronization
> >    messages (typically causing new messages to not receive the "unread"
> >    tag).
> >  
> > +Emacs Interface
> > +---------------
> > +
> > +Search now uses the JSON format internally
> > +
> > +  This should address problems with unusual characters in authors and
> > +  subject lines that could confuse the old text-based search parser.
> > +
> > +The date shown in search results is no longer padded before applying
> > +user-specified formatting
> > +
> > +  Previously, the date in the search results was padded to fixed width
> > +  before being formatted with `notmuch-search-result-format`.  It is
> > +  no longer padded.  The default format has been updated, but if
> > +  you've customized this variable, you may have to change your date
> > +  format from `"%s "` to `"%12s "`.
> > +
> >  Notmuch 0.13.2 (2012-06-02)
> >  ===========================
> >  
> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> > index f7cda33..9e04d97 100644
> > --- a/emacs/notmuch-lib.el
> > +++ b/emacs/notmuch-lib.el
> > @@ -399,8 +399,9 @@ resynchronize after an error by moving point."
> >  
> >  Returns 'retry if there is insufficient input to parse the
> >  beginning of the compound.  If this is able to parse the
> > -beginning of a compound, it returns t and later calls to
> > -`notmuch-json-read' will return the compound's elements.
> > +beginning of a compound, it moves point past the token that opens
> > +the compound and returns t.  Later calls to `notmuch-json-read'
> > +will return the compound's elements.
> >  
> >  Entering JSON objects is current unimplemented."
> >  
> > @@ -423,7 +424,8 @@ Entering JSON objects is current unimplemented."
> >  Returns 'retry if there is insufficient input to parse a complete
> >  JSON value.  If the parser is currently inside a compound value
> >  and the next token ends the list or object, returns 'end.
> > -Otherwise, returns the value."
> > +Otherwise, moves point to just past the end of the value and
> > +returns the value."
> 
> I think that point can move when 'retry is returned (past terminators
> and commas for example). It might also be worth saying that it returns
> the next value passing command and terminators and whitespace after
> point.

How about

Returns 'retry if there is insufficient input to parse a complete JSON
value (though it may still move point over separators or whitespace).
If the parser is currently inside a compound value and the next token
ends the list or object, this moves point just past the terminator and
returns 'end.  Otherwise, this moves point to just past the end of the
value and returns the value.

?

> >  
> >    (with-current-buffer (notmuch-json-buffer jp)
> >      (or
> > @@ -474,11 +476,22 @@ Otherwise, returns the value."
> >  	       (notmuch-json-partial-pos jp) nil
> >  	       (notmuch-json-partial-state jp) nil)
> >  	 ;; Parse the value
> > -	 (let* ((json-object-type 'plist)
> > -		(json-array-type 'list)
> > -		(json-false nil))
> > +	 (let ((json-object-type 'plist)
> > +	       (json-array-type 'list)
> > +	       (json-false nil))
> >  	   (json-read)))))))
> >  
> > +(defun notmuch-json-eof (jp)
> > +  "Signal a json-error if there is more input in JP's buffer.
> 
> Would `data' be better than `input' (to distinguish allowed whitespace
> from disallowed data)?

Ah, yes.

  "Signal a json-error if there is more data in JP's buffer.

Moves point to the beginning of any trailing data or to the end
of the buffer if there is only trailing whitespace."

?

> > +Moves point to the beginning of any trailing garbage or to the
> > +end of the buffer if there is no trailing garbage."
> > +
> > +  (with-current-buffer (notmuch-json-buffer jp)
> > +    (skip-chars-forward " \t\r\n")
> > +    (unless (eobp)
> > +      (signal 'json-error (list "Trailing garbage following JSON data")))))
> > +
> >  (provide 'notmuch-lib)
> >  
> >  ;; Local Variables:
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index 2a09a98..fabb7c0 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -702,28 +702,29 @@ non-authors is found, assume that all of the authors match."
> >  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
> >        (insert padding))))
> >  
> > -(defun notmuch-search-insert-field (field format result)
> > +(defun notmuch-search-insert-field (field format-string result)
> >    (cond
> >     ((string-equal field "date")
> > -    (insert (propertize (format format (plist-get result :date_relative))
> > +    (insert (propertize (format format-string (plist-get result :date_relative))
> >  			'face 'notmuch-search-date)))
> >     ((string-equal field "count")
> > -    (insert (propertize (format format (format "[%s/%s]"
> > -					       (plist-get result :matched)
> > -					       (plist-get result :total)))
> > +    (insert (propertize (format format-string
> > +				(format "[%s/%s]" (plist-get result :matched)
> > +					(plist-get result :total)))
> >  			'face 'notmuch-search-count)))
> >     ((string-equal field "subject")
> > -    (insert (propertize (format format (plist-get result :subject))
> > +    (insert (propertize (format format-string (plist-get result :subject))
> >  			'face 'notmuch-search-subject)))
> >  
> >     ((string-equal field "authors")
> > -    (notmuch-search-insert-authors format (plist-get result :authors)))
> > +    (notmuch-search-insert-authors format-string (plist-get result :authors)))
> >  
> >     ((string-equal field "tags")
> > -    (insert
> > -     (format format (propertize
> > -		     (mapconcat 'identity (plist-get result :tags) " ")
> > -		     'font-lock-face 'notmuch-tag-face))))))
> > +    ;; Ignore format-string here because notmuch-search-set-tags
> > +    ;; depends on the format of this
> > +    (insert (concat "(" (propertize
> > +			 (mapconcat 'identity (plist-get result :tags) " ")
> > +			 'font-lock-face 'notmuch-tag-face) ")")))))
> >  
> >  (defun notmuch-search-show-result (result)
> >    ;; Ignore excluded matches
> > @@ -731,8 +732,8 @@ non-authors is found, assume that all of the authors match."
> >      (let ((beg (point-max)))
> >        (save-excursion
> >  	(goto-char beg)
> > -	(dolist (format notmuch-search-result-format)
> > -	  (notmuch-search-insert-field (car format) (cdr format) result))
> > +	(dolist (spec notmuch-search-result-format)
> > +	  (notmuch-search-insert-field (car spec) (cdr spec) result))
> >  	(insert "\n")
> >  	(notmuch-search-color-line beg (point) (plist-get result :tags))
> >  	(put-text-property beg (point) 'notmuch-search-thread-id
> > @@ -790,11 +791,8 @@ non-authors is found, assume that all of the authors match."
> >  		     (otherwise (notmuch-search-show-result result)))))
> >  		((end)
> >  		 ;; Any trailing data is unexpected
> > -		 (with-current-buffer parse-buf
> > -		   (skip-chars-forward " \t\r\n")
> > -		   (if (eobp)
> > -		       (setq done t)
> > -		     (signal 'json-error nil)))))
> > +		 (notmuch-json-eof notmuch-search-json-parser)
> > +		 (setq done t)))
> 
> I think this used to only set `done' if there was not trailing data but
> now does so anyway?

It still won't set done if there's trailing data because
notmuch-json-eof will signal an error, which will unwind to the
condition-case (which will then consume said trailing data and done
will get set on the next time through the loop).

> Best wishes
> 
> Mark
> 
> >  	    (json-error
> >  	     ;; Do our best to resynchronize and ensure forward
> >  	     ;; progress

Re: [PATCH v2 0/9] JSON-based search-mode

[Mark Walters]

On Fri, 06 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Jul 05 at 10:44 pm:
>> On Thu, 05 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> > This should account for all of Mark's and Tomi's comments.  This
>> > version
>> > * renames the "format" variables to "format-string" and "spec" to be
>> >   less confusing,
>> > * reverts to the original behavior of ignoring the user's format
>> >   specification for tags (since we make assumptions about this format
>> >   elsewhere),
>> > * swaps the error helper and search-target patches to fix the
>> >   temporary issue with error message placement,
>> > * adds documentation on point movement in the JSON parser,
>> > * breaks out the JSON EOF testing function,
>> > * beefs up a few commit messages,
>> > * and adds a NEWS patch.
>> >
>> > For ease of reviewing, the diff diff is below.
>> >
>> > I've written most of a follow-on patch series that cleans up the
>> > handling of metadata and tag changes by attaching the JSON result
>> > object to the result.  This means we don't need a proliferation of
>> > text properties to store the result metadata, and we can make updates
>> > to a result (e.g., tag changes) by updating this result object and
>> > then re-rendering the result line from scratch, rather than trying to
>> > update the result line in place.  This makes it possible to obey user
>> > formatting for the tag list and has other perks like recoloring
>> > results when their tags change.  I'll send it along once this patch
>> > series is accepted.
>> >
>> > diff --git a/NEWS b/NEWS
>> > index d29ec5b..a1a6e93 100644
>> > --- a/NEWS
>> > +++ b/NEWS
>> > @@ -14,6 +14,23 @@ Maildir tag synchronization
>> >    messages (typically causing new messages to not receive the "unread"
>> >    tag).
>> >  
>> > +Emacs Interface
>> > +---------------
>> > +
>> > +Search now uses the JSON format internally
>> > +
>> > +  This should address problems with unusual characters in authors and
>> > +  subject lines that could confuse the old text-based search parser.
>> > +
>> > +The date shown in search results is no longer padded before applying
>> > +user-specified formatting
>> > +
>> > +  Previously, the date in the search results was padded to fixed width
>> > +  before being formatted with `notmuch-search-result-format`.  It is
>> > +  no longer padded.  The default format has been updated, but if
>> > +  you've customized this variable, you may have to change your date
>> > +  format from `"%s "` to `"%12s "`.
>> > +
>> >  Notmuch 0.13.2 (2012-06-02)
>> >  ===========================
>> >  
>> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> > index f7cda33..9e04d97 100644
>> > --- a/emacs/notmuch-lib.el
>> > +++ b/emacs/notmuch-lib.el
>> > @@ -399,8 +399,9 @@ resynchronize after an error by moving point."
>> >  
>> >  Returns 'retry if there is insufficient input to parse the
>> >  beginning of the compound.  If this is able to parse the
>> > -beginning of a compound, it returns t and later calls to
>> > -`notmuch-json-read' will return the compound's elements.
>> > +beginning of a compound, it moves point past the token that opens
>> > +the compound and returns t.  Later calls to `notmuch-json-read'
>> > +will return the compound's elements.
>> >  
>> >  Entering JSON objects is current unimplemented."
>> >  
>> > @@ -423,7 +424,8 @@ Entering JSON objects is current unimplemented."
>> >  Returns 'retry if there is insufficient input to parse a complete
>> >  JSON value.  If the parser is currently inside a compound value
>> >  and the next token ends the list or object, returns 'end.
>> > -Otherwise, returns the value."
>> > +Otherwise, moves point to just past the end of the value and
>> > +returns the value."
>> 
>> I think that point can move when 'retry is returned (past terminators
>> and commas for example). It might also be worth saying that it returns
>> the next value passing command and terminators and whitespace after
>> point.
>
> How about
>
> Returns 'retry if there is insufficient input to parse a complete JSON
> value (though it may still move point over separators or whitespace).
> If the parser is currently inside a compound value and the next token
> ends the list or object, this moves point just past the terminator and
> returns 'end.  Otherwise, this moves point to just past the end of the
> value and returns the value.
>
> ?

This sounds good.

>> >  
>> >    (with-current-buffer (notmuch-json-buffer jp)
>> >      (or
>> > @@ -474,11 +476,22 @@ Otherwise, returns the value."
>> >  	       (notmuch-json-partial-pos jp) nil
>> >  	       (notmuch-json-partial-state jp) nil)
>> >  	 ;; Parse the value
>> > -	 (let* ((json-object-type 'plist)
>> > -		(json-array-type 'list)
>> > -		(json-false nil))
>> > +	 (let ((json-object-type 'plist)
>> > +	       (json-array-type 'list)
>> > +	       (json-false nil))
>> >  	   (json-read)))))))
>> >  
>> > +(defun notmuch-json-eof (jp)
>> > +  "Signal a json-error if there is more input in JP's buffer.
>> 
>> Would `data' be better than `input' (to distinguish allowed whitespace
>> from disallowed data)?
>
> Ah, yes.
>
>   "Signal a json-error if there is more data in JP's buffer.
>
> Moves point to the beginning of any trailing data or to the end
> of the buffer if there is only trailing whitespace."
>
> ?

This is good too.

>> > +Moves point to the beginning of any trailing garbage or to the
>> > +end of the buffer if there is no trailing garbage."
>> > +
>> > +  (with-current-buffer (notmuch-json-buffer jp)
>> > +    (skip-chars-forward " \t\r\n")
>> > +    (unless (eobp)
>> > +      (signal 'json-error (list "Trailing garbage following JSON data")))))
>> > +
>> >  (provide 'notmuch-lib)
>> >  
>> >  ;; Local Variables:
>> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> > index 2a09a98..fabb7c0 100644
>> > --- a/emacs/notmuch.el
>> > +++ b/emacs/notmuch.el
>> > @@ -702,28 +702,29 @@ non-authors is found, assume that all of the authors match."
>> >  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
>> >        (insert padding))))
>> >  
>> > -(defun notmuch-search-insert-field (field format result)
>> > +(defun notmuch-search-insert-field (field format-string result)
>> >    (cond
>> >     ((string-equal field "date")
>> > -    (insert (propertize (format format (plist-get result :date_relative))
>> > +    (insert (propertize (format format-string (plist-get result :date_relative))
>> >  			'face 'notmuch-search-date)))
>> >     ((string-equal field "count")
>> > -    (insert (propertize (format format (format "[%s/%s]"
>> > -					       (plist-get result :matched)
>> > -					       (plist-get result :total)))
>> > +    (insert (propertize (format format-string
>> > +				(format "[%s/%s]" (plist-get result :matched)
>> > +					(plist-get result :total)))
>> >  			'face 'notmuch-search-count)))
>> >     ((string-equal field "subject")
>> > -    (insert (propertize (format format (plist-get result :subject))
>> > +    (insert (propertize (format format-string (plist-get result :subject))
>> >  			'face 'notmuch-search-subject)))
>> >  
>> >     ((string-equal field "authors")
>> > -    (notmuch-search-insert-authors format (plist-get result :authors)))
>> > +    (notmuch-search-insert-authors format-string (plist-get result :authors)))
>> >  
>> >     ((string-equal field "tags")
>> > -    (insert
>> > -     (format format (propertize
>> > -		     (mapconcat 'identity (plist-get result :tags) " ")
>> > -		     'font-lock-face 'notmuch-tag-face))))))
>> > +    ;; Ignore format-string here because notmuch-search-set-tags
>> > +    ;; depends on the format of this
>> > +    (insert (concat "(" (propertize
>> > +			 (mapconcat 'identity (plist-get result :tags) " ")
>> > +			 'font-lock-face 'notmuch-tag-face) ")")))))
>> >  
>> >  (defun notmuch-search-show-result (result)
>> >    ;; Ignore excluded matches
>> > @@ -731,8 +732,8 @@ non-authors is found, assume that all of the authors match."
>> >      (let ((beg (point-max)))
>> >        (save-excursion
>> >  	(goto-char beg)
>> > -	(dolist (format notmuch-search-result-format)
>> > -	  (notmuch-search-insert-field (car format) (cdr format) result))
>> > +	(dolist (spec notmuch-search-result-format)
>> > +	  (notmuch-search-insert-field (car spec) (cdr spec) result))
>> >  	(insert "\n")
>> >  	(notmuch-search-color-line beg (point) (plist-get result :tags))
>> >  	(put-text-property beg (point) 'notmuch-search-thread-id
>> > @@ -790,11 +791,8 @@ non-authors is found, assume that all of the authors match."
>> >  		     (otherwise (notmuch-search-show-result result)))))
>> >  		((end)
>> >  		 ;; Any trailing data is unexpected
>> > -		 (with-current-buffer parse-buf
>> > -		   (skip-chars-forward " \t\r\n")
>> > -		   (if (eobp)
>> > -		       (setq done t)
>> > -		     (signal 'json-error nil)))))
>> > +		 (notmuch-json-eof notmuch-search-json-parser)
>> > +		 (setq done t)))
>> 
>> I think this used to only set `done' if there was not trailing data but
>> now does so anyway?
>
> It still won't set done if there's trailing data because
> notmuch-json-eof will signal an error, which will unwind to the
> condition-case (which will then consume said trailing data and done
> will get set on the next time through the loop).


Ah! I had overlooked that. It all looks good now.

Best wishes

Mark

[PATCH v3 0/9] JSON-based search-mode

[Austin Clements]

This fixes the docstings for notmuch-json-read and notmuch-json-eof as
suggested by Mark.  No other changes.

[PATCH v3 1/9] emacs: Clean up notmuch-search-show-result

[Austin Clements]

This simplifies the code and makes it no longer cubic in the number of
result fields.
---
 emacs/notmuch.el |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c6236db..8ad0b68 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -707,29 +707,30 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field date count authors subject tags)
+(defun notmuch-search-insert-field (field format-string date count authors subject tags)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) date)
+    (insert (propertize (format format-string date)
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) count)
+    (insert (propertize (format format-string count)
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format (cdr (assoc field notmuch-search-result-format)) subject)
+    (insert (propertize (format format-string subject)
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors (cdr (assoc field notmuch-search-result-format)) authors))
+    (notmuch-search-insert-authors format-string authors))
 
    ((string-equal field "tags")
+    ;; Ignore format-string here because notmuch-search-set-tags
+    ;; depends on the format of this
     (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
 
 (defun notmuch-search-show-result (date count authors subject tags)
-  (let ((fields) (field))
-    (setq fields (mapcar 'car notmuch-search-result-format))
-    (loop for field in fields
-	  do (notmuch-search-insert-field field date count authors subject tags)))
+  (dolist (spec notmuch-search-result-format)
+    (notmuch-search-insert-field (car spec) (cdr spec)
+				 date count authors subject tags))
   (insert "\n"))
 
 (defun notmuch-search-process-filter (proc string)
-- 
1.7.10

[PATCH v3 2/9] emacs: Separate search line parsing and display

[Austin Clements]

Previously, much of the display of search lines was done in the same
function that parsed the CLI's output.  Now the parsing function only
parses, and notmuch-search-show-result fully inserts the search result
in the search buffer.
---
 emacs/notmuch.el |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8ad0b68..746d0cb 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -727,11 +727,19 @@ non-authors is found, assume that all of the authors match."
     ;; depends on the format of this
     (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (date count authors subject tags)
-  (dolist (spec notmuch-search-result-format)
-    (notmuch-search-insert-field (car spec) (cdr spec)
-				 date count authors subject tags))
-  (insert "\n"))
+(defun notmuch-search-show-result (thread-id date count authors subject tags)
+  ;; Ignore excluded matches
+  (unless (eq (aref count 1) ?0)
+    (let ((beg (point))
+	  (tags-str (mapconcat 'identity tags " ")))
+      (dolist (spec notmuch-search-result-format)
+	(notmuch-search-insert-field (car spec) (cdr spec)
+				     date count authors subject tags-str))
+      (insert "\n")
+      (notmuch-search-color-line beg (point) tags)
+      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
+      (put-text-property beg (point) 'notmuch-search-authors authors)
+      (put-text-property beg (point) 'notmuch-search-subject subject))))
 
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
@@ -759,17 +767,10 @@ non-authors is found, assume that all of the authors match."
 		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
 			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
-		      ;; We currently just throw away excluded matches.
-		      (unless (eq (aref count 1) ?0)
-			(let ((beg (point)))
-			  (notmuch-search-show-result date count authors subject tags)
-			  (notmuch-search-color-line beg (point) tag-list)
-			  (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-			  (put-text-property beg (point) 'notmuch-search-authors authors)
-			  (put-text-property beg (point) 'notmuch-search-subject subject)
-			  (when (string= thread-id notmuch-search-target-thread)
-			    (set 'found-target beg)
-			    (set 'notmuch-search-target-thread "found"))))
+		      (when (string= thread-id notmuch-search-target-thread)
+			(set 'found-target (point))
+			(set 'notmuch-search-target-thread "found"))
+		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.10

[PATCH v3 3/9] emacs: Helper for reporting search parsing errors

[Austin Clements]

This removes the last bit of direct output from the parsing function.
With the parser now responsible solely for parsing, we can swap it out
for another parser.
---
 emacs/notmuch.el |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 746d0cb..f952fa8 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -741,6 +741,11 @@ non-authors is found, assume that all of the authors match."
       (put-text-property beg (point) 'notmuch-search-authors authors)
       (put-text-property beg (point) 'notmuch-search-subject subject))))
 
+(defun notmuch-search-show-error (string &rest objects)
+  (insert "Error: Unexpected output from notmuch search:\n")
+  (insert (apply #'format string objects))
+  (insert "\n"))
+
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
   (let ((buffer (process-buffer proc))
@@ -766,7 +771,8 @@ non-authors is found, assume that all of the authors match."
 			   (tag-list (if tags (save-match-data (split-string tags)))))
 		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
-			  (insert (concat "Error: Unexpected output from notmuch search:\n" (substring string line (match-beginning 1)) "\n")))
+			  (notmuch-search-show-error
+			   (substring string line (match-beginning 1))))
 		      (when (string= thread-id notmuch-search-target-thread)
 			(set 'found-target (point))
 			(set 'notmuch-search-target-thread "found"))
-- 
1.7.10

[PATCH v3 4/9] emacs: Move search-target logic to `notmuch-search-show-result'

[Austin Clements]

This is a simpler place to do this, since we can avoid any point
motion and hence any save-excursions in
`notmuch-search-process-filter', which in turn lets us put all of the
search-target logic outside of any save-excursions.

`notmuch-search-show-{result,error}' are now responsible for their own
point motion.

`notmuch-search-process-filter' could use some reindentation after
this, but we're about to rewrite it entirely, so we won't bother.
---
 emacs/notmuch.el |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f952fa8..5bf01ca 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -730,29 +730,34 @@ non-authors is found, assume that all of the authors match."
 (defun notmuch-search-show-result (thread-id date count authors subject tags)
   ;; Ignore excluded matches
   (unless (eq (aref count 1) ?0)
-    (let ((beg (point))
+    (let ((beg (point-max))
 	  (tags-str (mapconcat 'identity tags " ")))
-      (dolist (spec notmuch-search-result-format)
-	(notmuch-search-insert-field (car spec) (cdr spec)
-				     date count authors subject tags-str))
-      (insert "\n")
-      (notmuch-search-color-line beg (point) tags)
-      (put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-      (put-text-property beg (point) 'notmuch-search-authors authors)
-      (put-text-property beg (point) 'notmuch-search-subject subject))))
+      (save-excursion
+	(goto-char beg)
+	(dolist (spec notmuch-search-result-format)
+	  (notmuch-search-insert-field (car spec) (cdr spec)
+				       date count authors subject tags-str))
+	(insert "\n")
+	(notmuch-search-color-line beg (point) tags)
+	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
+	(put-text-property beg (point) 'notmuch-search-authors authors)
+	(put-text-property beg (point) 'notmuch-search-subject subject))
+      (when (string= thread-id notmuch-search-target-thread)
+	(setq notmuch-search-target-thread "found")
+	(goto-char beg)))))
 
 (defun notmuch-search-show-error (string &rest objects)
-  (insert "Error: Unexpected output from notmuch search:\n")
-  (insert (apply #'format string objects))
-  (insert "\n"))
+  (save-excursion
+    (goto-char (point-max))
+    (insert "Error: Unexpected output from notmuch search:\n")
+    (insert (apply #'format string objects))
+    (insert "\n")))
 
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
-  (let ((buffer (process-buffer proc))
-	(found-target nil))
+  (let ((buffer (process-buffer proc)))
     (if (buffer-live-p buffer)
 	(with-current-buffer buffer
-	  (save-excursion
 	    (let ((line 0)
 		  (more t)
 		  (inhibit-read-only t)
@@ -769,13 +774,9 @@ non-authors is found, assume that all of the authors match."
 			   (subject (match-string 5 string))
 			   (tags (match-string 6 string))
 			   (tag-list (if tags (save-match-data (split-string tags)))))
-		      (goto-char (point-max))
 		      (if (/= (match-beginning 1) line)
 			  (notmuch-search-show-error
 			   (substring string line (match-beginning 1))))
-		      (when (string= thread-id notmuch-search-target-thread)
-			(set 'found-target (point))
-			(set 'notmuch-search-target-thread "found"))
 		      (notmuch-search-show-result thread-id date count authors subject tag-list)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
@@ -784,8 +785,6 @@ non-authors is found, assume that all of the authors match."
 		  (if (< line (length string))
 		      (setq notmuch-search-process-filter-data (substring string line)))
 		  ))))
-	  (if found-target
-	      (goto-char found-target)))
       (delete-process proc))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
-- 
1.7.10

[PATCH v3 5/9] emacs: Pass plist to `notmuch-search-show-result'

[Austin Clements]

Rather than passing lots of arguments and then further passing those
to `notmuch-search-insert-field', pass a plist containing all of the
search result information.  This plist is compatible with the JSON
format search results.
---
 emacs/notmuch.el |   65 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 5bf01ca..dfeaf35 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -707,42 +707,47 @@ non-authors is found, assume that all of the authors match."
 	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
       (insert padding))))
 
-(defun notmuch-search-insert-field (field format-string date count authors subject tags)
+(defun notmuch-search-insert-field (field format-string result)
   (cond
    ((string-equal field "date")
-    (insert (propertize (format format-string date)
+    (insert (propertize (format format-string (plist-get result :date_relative))
 			'face 'notmuch-search-date)))
    ((string-equal field "count")
-    (insert (propertize (format format-string count)
+    (insert (propertize (format format-string
+				(format "[%s/%s]" (plist-get result :matched)
+					(plist-get result :total)))
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format format-string subject)
+    (insert (propertize (format format-string (plist-get result :subject))
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors format-string authors))
+    (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
     ;; Ignore format-string here because notmuch-search-set-tags
     ;; depends on the format of this
-    (insert (concat "(" (propertize tags 'font-lock-face 'notmuch-tag-face) ")")))))
+    (insert (concat "(" (propertize
+			 (mapconcat 'identity (plist-get result :tags) " ")
+			 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (thread-id date count authors subject tags)
+(defun notmuch-search-show-result (result)
   ;; Ignore excluded matches
-  (unless (eq (aref count 1) ?0)
-    (let ((beg (point-max))
-	  (tags-str (mapconcat 'identity tags " ")))
+  (unless (= (plist-get result :matched) 0)
+    (let ((beg (point-max)))
       (save-excursion
 	(goto-char beg)
 	(dolist (spec notmuch-search-result-format)
-	  (notmuch-search-insert-field (car spec) (cdr spec)
-				       date count authors subject tags-str))
+	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
-	(notmuch-search-color-line beg (point) tags)
-	(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
-	(put-text-property beg (point) 'notmuch-search-authors authors)
-	(put-text-property beg (point) 'notmuch-search-subject subject))
-      (when (string= thread-id notmuch-search-target-thread)
+	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-thread-id
+			   (concat "thread:" (plist-get result :thread)))
+	(put-text-property beg (point) 'notmuch-search-authors
+			   (plist-get result :authors))
+	(put-text-property beg (point) 'notmuch-search-subject
+			   (plist-get result :subject)))
+      (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
 
@@ -766,18 +771,24 @@ non-authors is found, assume that all of the authors match."
 	      (while more
 		(while (and (< line (length string)) (= (elt string line) ?\n))
 		  (setq line (1+ line)))
-		(if (string-match "^\\(thread:[0-9A-Fa-f]*\\) \\([^][]*\\) \\(\\[[0-9/]*\\]\\) \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
+		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
 		    (let* ((thread-id (match-string 1 string))
-			   (date (match-string 2 string))
-			   (count (match-string 3 string))
-			   (authors (match-string 4 string))
-			   (subject (match-string 5 string))
-			   (tags (match-string 6 string))
-			   (tag-list (if tags (save-match-data (split-string tags)))))
-		      (if (/= (match-beginning 1) line)
+			   (tags-str (match-string 7 string))
+			   (result (list :thread thread-id
+					 :date_relative (match-string 2 string)
+					 :matched (string-to-number
+						   (match-string 3 string))
+					 :total (string-to-number
+						 (match-string 4 string))
+					 :authors (match-string 5 string)
+					 :subject (match-string 6 string)
+					 :tags (if tags-str
+						   (save-match-data
+						     (split-string tags-str))))))
+		      (if (/= (match-beginning 0) line)
 			  (notmuch-search-show-error
-			   (substring string line (match-beginning 1))))
-		      (notmuch-search-show-result thread-id date count authors subject tag-list)
+			   (substring string line (match-beginning 0))))
+		      (notmuch-search-show-result result)
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.10

[PATCH v3 6/9] test: New test for incremental search output parsing

[Austin Clements]

This advises the search process filter to make it process one
character at a time in order to test the pessimal case for incremental
search output parsing.

The text parser fails this test because it gets tricked into thinking
a parenthetical remark in a subject is the tag list.
---
 test/emacs       |   11 +++++++++++
 test/test-lib.el |    8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/test/emacs b/test/emacs
index e9f954c..293b12a 100755
--- a/test/emacs
+++ b/test/emacs
@@ -35,6 +35,17 @@ test_emacs '(notmuch-search "tag:inbox")
 	    (test-output)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
 
+test_begin_subtest "Incremental parsing of search results"
+test_subtest_known_broken
+test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
+	    (ad-activate 'notmuch-search-process-filter)
+	    (notmuch-search \"tag:inbox\")
+	    (notmuch-test-wait)
+	    (ad-disable-advice 'notmuch-search-process-filter 'around 'pessimal)
+	    (ad-activate 'notmuch-search-process-filter)
+	    (test-output)"
+test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
+
 test_begin_subtest "Navigation of notmuch-hello to search results"
 test_emacs '(notmuch-hello)
 	    (goto-char (point-min))
diff --git a/test/test-lib.el b/test/test-lib.el
index 6271da2..5dd6271 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -89,6 +89,14 @@ nothing."
 (add-hook-counter 'notmuch-hello-mode-hook)
 (add-hook-counter 'notmuch-hello-refresh-hook)
 
+(defadvice notmuch-search-process-filter (around pessimal activate disable)
+  "Feed notmuch-search-process-filter one character at a time."
+  (let ((string (ad-get-arg 1)))
+    (loop for char across string
+	  do (progn
+	       (ad-set-arg 1 (char-to-string char))
+	       ad-do-it))))
+
 (defmacro notmuch-test-run (&rest body)
   "Evaluate a BODY of test expressions and output the result."
   `(with-temp-buffer
-- 
1.7.10

[PATCH v3 7/9] emacs: Implement an incremental JSON parser

[Austin Clements]

This parser is designed to read streaming JSON whose structure is
known to the caller.  Like a typical JSON parsing interface, it
provides a function to read a complete JSON value from the input.
However, it extends this with an additional function that
requires the next value in the input to be a compound value and
descends into it, allowing its elements to be read one at a time
or further descended into.  Both functions can return 'retry to
indicate that not enough input is available.

The parser supports efficient partial parsing, so there's no need to
frame the input for correctness or performance.

The bulk of the parsing is still done by Emacs' json.el, so any
improvements or optimizations to that will benefit the incremental
parser as well.

Currently only descending into JSON lists is supported because that's
all we need, but support for descending into JSON objects can be added
in the future.
---
 emacs/notmuch-lib.el |  197 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 197 insertions(+)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index c829df3..aa25513 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -23,6 +23,7 @@
 
 (require 'mm-view)
 (require 'mm-decode)
+(require 'json)
 (eval-when-compile (require 'cl))
 
 (defvar notmuch-command "notmuch"
@@ -296,6 +297,202 @@ was called."
 (defvar notmuch-show-process-crypto nil)
 (make-variable-buffer-local 'notmuch-show-process-crypto)
 
+;; Incremental JSON parsing
+
+(defun notmuch-json-create-parser (buffer)
+  "Return a streaming JSON parser that consumes input from BUFFER.
+
+This parser is designed to read streaming JSON whose structure is
+known to the caller.  Like a typical JSON parsing interface, it
+provides a function to read a complete JSON value from the input.
+However, it extends this with an additional function that
+requires the next value in the input to be a compound value and
+descends into it, allowing its elements to be read one at a time
+or further descended into.  Both functions can return 'retry to
+indicate that not enough input is available.
+
+The parser always consumes input from BUFFER's point.  Hence, the
+caller is allowed to delete and data before point and may
+resynchronize after an error by moving point."
+
+  (list buffer
+	;; Terminator stack: a stack of characters that indicate the
+	;; end of the compound values enclosing point
+	'()
+	;; Next: One of
+	;; * 'expect-value if the next token must be a value, but a
+	;;   value has not yet been reached
+	;; * 'value if point is at the beginning of a value
+	;; * 'expect-comma if the next token must be a comma
+	'expect-value
+	;; Allow terminator: non-nil if the next token may be a
+	;; terminator
+	nil
+	;; Partial parse position: If state is 'value, a marker for
+	;; the position of the partial parser or nil if no partial
+	;; parsing has happened yet
+	nil
+	;; Partial parse state: If state is 'value, the current
+	;; `parse-partial-sexp' state
+	nil))
+
+(defmacro notmuch-json-buffer (jp) `(first ,jp))
+(defmacro notmuch-json-term-stack (jp) `(second ,jp))
+(defmacro notmuch-json-next (jp) `(third ,jp))
+(defmacro notmuch-json-allow-term (jp) `(fourth ,jp))
+(defmacro notmuch-json-partial-pos (jp) `(fifth ,jp))
+(defmacro notmuch-json-partial-state (jp) `(sixth ,jp))
+
+(defvar notmuch-json-syntax-table
+  (let ((table (make-syntax-table)))
+    ;; The standard syntax table is what we need except that "." needs
+    ;; to have word syntax instead of punctuation syntax.
+    (modify-syntax-entry ?. "w" table)
+    table)
+  "Syntax table used for incremental JSON parsing.")
+
+(defun notmuch-json-scan-to-value (jp)
+  ;; Helper function that consumes separators, terminators, and
+  ;; whitespace from point.  Returns nil if it successfully reached
+  ;; the beginning of a value, 'end if it consumed a terminator, or
+  ;; 'retry if not enough input was available to reach a value.  Upon
+  ;; nil return, (notmuch-json-next jp) is always 'value.
+
+  (if (eq (notmuch-json-next jp) 'value)
+      ;; We're already at a value
+      nil
+    ;; Drive the state toward 'expect-value
+    (skip-chars-forward " \t\r\n")
+    (or (when (eobp) 'retry)
+	;; Test for the terminator for the current compound
+	(when (and (notmuch-json-allow-term jp)
+		   (eq (char-after) (car (notmuch-json-term-stack jp))))
+	  ;; Consume it and expect a comma or terminator next
+	  (forward-char)
+	  (setf (notmuch-json-term-stack jp) (cdr (notmuch-json-term-stack jp))
+		(notmuch-json-next jp) 'expect-comma
+		(notmuch-json-allow-term jp) t)
+	  'end)
+	;; Test for a separator
+	(when (eq (notmuch-json-next jp) 'expect-comma)
+	  (when (/= (char-after) ?,)
+	    (signal 'json-readtable-error (list "expected ','")))
+	  ;; Consume it, switch to 'expect-value, and disallow a
+	  ;; terminator
+	  (forward-char)
+	  (skip-chars-forward " \t\r\n")
+	  (setf (notmuch-json-next jp) 'expect-value
+		(notmuch-json-allow-term jp) nil)
+	  ;; We moved point, so test for eobp again and fall through
+	  ;; to the next test if there's more input
+	  (when (eobp) 'retry))
+	;; Next must be 'expect-value and we know this isn't
+	;; whitespace, EOB, or a terminator, so point must be on a
+	;; value
+	(progn
+	  (assert (eq (notmuch-json-next jp) 'expect-value))
+	  (setf (notmuch-json-next jp) 'value)
+	  nil))))
+
+(defun notmuch-json-begin-compound (jp)
+  "Parse the beginning of a compound value and traverse inside it.
+
+Returns 'retry if there is insufficient input to parse the
+beginning of the compound.  If this is able to parse the
+beginning of a compound, it moves point past the token that opens
+the compound and returns t.  Later calls to `notmuch-json-read'
+will return the compound's elements.
+
+Entering JSON objects is currently unimplemented."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    ;; Disallow terminators
+    (setf (notmuch-json-allow-term jp) nil)
+    (or (notmuch-json-scan-to-value jp)
+	(if (/= (char-after) ?\[)
+	    (signal 'json-readtable-error (list "expected '['"))
+	  (forward-char)
+	  (push ?\] (notmuch-json-term-stack jp))
+	  ;; Expect a value or terminator next
+	  (setf (notmuch-json-next jp) 'expect-value
+		(notmuch-json-allow-term jp) t)
+	  t))))
+
+(defun notmuch-json-read (jp)
+  "Parse the value at point in JP's buffer.
+
+Returns 'retry if there is insufficient input to parse a complete
+JSON value (though it may still move point over separators or
+whitespace).  If the parser is currently inside a compound value
+and the next token ends the list or object, this moves point just
+past the terminator and returns 'end.  Otherwise, this moves
+point to just past the end of the value and returns the value."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    (or
+     ;; Get to a value state
+     (notmuch-json-scan-to-value jp)
+
+     ;; Can we parse a complete value?
+     (let ((complete
+	    (if (looking-at "[-+0-9tfn]")
+		;; This is a number or a keyword, so the partial
+		;; parser isn't going to help us because a truncated
+		;; number or keyword looks like a complete symbol to
+		;; it.  Look for something that clearly ends it.
+		(save-excursion
+		  (skip-chars-forward "^]},: \t\r\n")
+		  (not (eobp)))
+
+	      ;; We're looking at a string, object, or array, which we
+	      ;; can partial parse.  If we just reached the value, set
+	      ;; up the partial parser.
+	      (when (null (notmuch-json-partial-state jp))
+		(setf (notmuch-json-partial-pos jp) (point-marker)))
+
+	      ;; Extend the partial parse until we either reach EOB or
+	      ;; get the whole value
+	      (save-excursion
+		(let ((pstate
+		       (with-syntax-table notmuch-json-syntax-table
+			 (parse-partial-sexp
+			  (notmuch-json-partial-pos jp) (point-max) 0 nil
+			  (notmuch-json-partial-state jp)))))
+		  ;; A complete value is available if we've reached
+		  ;; depth 0 or less and encountered a complete
+		  ;; subexpression.
+		  (if (and (<= (first pstate) 0) (third pstate))
+		      t
+		    ;; Not complete.  Update the partial parser state
+		    (setf (notmuch-json-partial-pos jp) (point-marker)
+			  (notmuch-json-partial-state jp) pstate)
+		    nil))))))
+
+       (if (not complete)
+	   'retry
+	 ;; We have a value.  Reset the partial parse state and expect
+	 ;; a comma or terminator after the value.
+	 (setf (notmuch-json-next jp) 'expect-comma
+	       (notmuch-json-allow-term jp) t
+	       (notmuch-json-partial-pos jp) nil
+	       (notmuch-json-partial-state jp) nil)
+	 ;; Parse the value
+	 (let ((json-object-type 'plist)
+	       (json-array-type 'list)
+	       (json-false nil))
+	   (json-read)))))))
+
+(defun notmuch-json-eof (jp)
+  "Signal a json-error if there is more data in JP's buffer.
+
+Moves point to the beginning of any trailing data or to the end
+of the buffer if there is only trailing whitespace."
+
+  (with-current-buffer (notmuch-json-buffer jp)
+    (skip-chars-forward " \t\r\n")
+    (unless (eobp)
+      (signal 'json-error (list "Trailing garbage following JSON data")))))
+
 (provide 'notmuch-lib)
 
 ;; Local Variables:
-- 
1.7.10

[PATCH v3 8/9] emacs: Switch from text to JSON format for search results

[Austin Clements]

The JSON format eliminates the complex escaping issues that have
plagued the text search format.  This uses the incremental JSON parser
so that, like the text parser, it can output search results
incrementally.

This slows down the parser by about ~4X, but puts us in a good
position to optimize either by improving the JSON parser (evidence
suggests this can reduce the overhead to ~40% over the text format) or
by switching to S-expressions (evidence suggests this will more than
double performance over the text parser).  [1]

This also fixes the incremental search parsing test.

This has one minor side-effect on search result formatting.
Previously, the date field was always padded to a fixed width of 12
characters because of how the text parser's regexp was written.  The
JSON format doesn't do this.  We could pad it out in Emacs before
formatting it, but, since all of the other fields are variable width,
we instead fix notmuch-search-result-format to take the variable-width
field and pad it out.  For users who have customized this variable,
we'll mention in the NEWS how to fix this slight format change.

[1] id:"20110720205007.GB21316@mit.edu"
---
 emacs/notmuch.el |  110 +++++++++++++++++++++++++++++++-----------------------
 test/emacs       |    1 -
 2 files changed, 64 insertions(+), 47 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index dfeaf35..fabb7c0 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -60,7 +60,7 @@
 (require 'notmuch-message)
 
 (defcustom notmuch-search-result-format
-  `(("date" . "%s ")
+  `(("date" . "%12s ")
     ("count" . "%-7s ")
     ("authors" . "%-20s ")
     ("subject" . "%s ")
@@ -557,17 +557,14 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
-(defvar notmuch-search-process-filter-data nil
-  "Data that has not yet been processed.")
-(make-variable-buffer-local 'notmuch-search-process-filter-data)
-
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
 	(status (process-status proc))
 	(exit-status (process-exit-status proc))
 	(never-found-target-thread nil))
-    (if (memq status '(exit signal))
+    (when (memq status '(exit signal))
+	(kill-buffer (process-get proc 'parse-buf))
 	(if (buffer-live-p buffer)
 	    (with-current-buffer buffer
 	      (save-excursion
@@ -577,8 +574,6 @@ This function advances the next thread when finished."
 		  (if (eq status 'signal)
 		      (insert "Incomplete search results (search process was killed).\n"))
 		  (when (eq status 'exit)
-		    (if notmuch-search-process-filter-data
-			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
 		    (insert "End of search results.")
 		    (unless (= exit-status 0)
 		      (insert (format " (process returned %d)" exit-status)))
@@ -758,45 +753,59 @@ non-authors is found, assume that all of the authors match."
     (insert (apply #'format string objects))
     (insert "\n")))
 
+(defvar notmuch-search-process-state nil
+  "Parsing state of the search process filter.")
+
+(defvar notmuch-search-json-parser nil
+  "Incremental JSON parser for the search process filter.")
+
 (defun notmuch-search-process-filter (proc string)
   "Process and filter the output of \"notmuch search\""
-  (let ((buffer (process-buffer proc)))
-    (if (buffer-live-p buffer)
-	(with-current-buffer buffer
-	    (let ((line 0)
-		  (more t)
-		  (inhibit-read-only t)
-		  (string (concat notmuch-search-process-filter-data string)))
-	      (setq notmuch-search-process-filter-data nil)
-	      (while more
-		(while (and (< line (length string)) (= (elt string line) ?\n))
-		  (setq line (1+ line)))
-		(if (string-match "^thread:\\([0-9A-Fa-f]*\\) \\([^][]*\\) \\[\\([0-9]*\\)/\\([0-9]*\\)\\] \\([^;]*\\); \\(.*\\) (\\([^()]*\\))$" string line)
-		    (let* ((thread-id (match-string 1 string))
-			   (tags-str (match-string 7 string))
-			   (result (list :thread thread-id
-					 :date_relative (match-string 2 string)
-					 :matched (string-to-number
-						   (match-string 3 string))
-					 :total (string-to-number
-						 (match-string 4 string))
-					 :authors (match-string 5 string)
-					 :subject (match-string 6 string)
-					 :tags (if tags-str
-						   (save-match-data
-						     (split-string tags-str))))))
-		      (if (/= (match-beginning 0) line)
-			  (notmuch-search-show-error
-			   (substring string line (match-beginning 0))))
-		      (notmuch-search-show-result result)
-		      (set 'line (match-end 0)))
-		  (set 'more nil)
-		  (while (and (< line (length string)) (= (elt string line) ?\n))
-		    (setq line (1+ line)))
-		  (if (< line (length string))
-		      (setq notmuch-search-process-filter-data (substring string line)))
-		  ))))
-      (delete-process proc))))
+  (let ((results-buf (process-buffer proc))
+	(parse-buf (process-get proc 'parse-buf))
+	(inhibit-read-only t)
+	done)
+    (if (not (buffer-live-p results-buf))
+	(delete-process proc)
+      (with-current-buffer parse-buf
+	;; Insert new data
+	(save-excursion
+	  (goto-char (point-max))
+	  (insert string)))
+      (with-current-buffer results-buf
+	(while (not done)
+	  (condition-case nil
+	      (case notmuch-search-process-state
+		((begin)
+		 ;; Enter the results list
+		 (if (eq (notmuch-json-begin-compound
+			  notmuch-search-json-parser) 'retry)
+		     (setq done t)
+		   (setq notmuch-search-process-state 'result)))
+		((result)
+		 ;; Parse a result
+		 (let ((result (notmuch-json-read notmuch-search-json-parser)))
+		   (case result
+		     ((retry) (setq done t))
+		     ((end) (setq notmuch-search-process-state 'end))
+		     (otherwise (notmuch-search-show-result result)))))
+		((end)
+		 ;; Any trailing data is unexpected
+		 (notmuch-json-eof notmuch-search-json-parser)
+		 (setq done t)))
+	    (json-error
+	     ;; Do our best to resynchronize and ensure forward
+	     ;; progress
+	     (notmuch-search-show-error
+	      "%s"
+	      (with-current-buffer parse-buf
+		(let ((bad (buffer-substring (line-beginning-position)
+					     (line-end-position))))
+		  (forward-line)
+		  bad))))))
+	;; Clear out what we've parsed
+	(with-current-buffer parse-buf
+	  (delete-region (point-min) (point)))))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
   "Add/remove tags from all messages in current search buffer.
@@ -899,10 +908,19 @@ Other optional parameters are used as follows:
 	(let ((proc (start-process
 		     "notmuch-search" buffer
 		     notmuch-command "search"
+		     "--format=json"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
-		     query)))
+		     query))
+	      ;; Use a scratch buffer to accumulate partial output.
+	      ;; This buffer will be killed by the sentinel, which
+	      ;; should be called no matter how the process dies.
+	      (parse-buf (generate-new-buffer " *notmuch search parse*")))
+	  (set (make-local-variable 'notmuch-search-process-state) 'begin)
+	  (set (make-local-variable 'notmuch-search-json-parser)
+	       (notmuch-json-create-parser parse-buf))
+	  (process-put proc 'parse-buf parse-buf)
 	  (set-process-sentinel proc 'notmuch-search-process-sentinel)
 	  (set-process-filter proc 'notmuch-search-process-filter)
 	  (set-process-query-on-exit-flag proc nil))))
diff --git a/test/emacs b/test/emacs
index 293b12a..afe35ba 100755
--- a/test/emacs
+++ b/test/emacs
@@ -36,7 +36,6 @@ test_emacs '(notmuch-search "tag:inbox")
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-search-tag-inbox
 
 test_begin_subtest "Incremental parsing of search results"
-test_subtest_known_broken
 test_emacs "(ad-enable-advice 'notmuch-search-process-filter 'around 'pessimal)
 	    (ad-activate 'notmuch-search-process-filter)
 	    (notmuch-search \"tag:inbox\")
-- 
1.7.10

[PATCH v3 9/9] News for JSON-based search

[Austin Clements]

---
 NEWS |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/NEWS b/NEWS
index d29ec5b..a1a6e93 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,23 @@ Maildir tag synchronization
   messages (typically causing new messages to not receive the "unread"
   tag).
 
+Emacs Interface
+---------------
+
+Search now uses the JSON format internally
+
+  This should address problems with unusual characters in authors and
+  subject lines that could confuse the old text-based search parser.
+
+The date shown in search results is no longer padded before applying
+user-specified formatting
+
+  Previously, the date in the search results was padded to fixed width
+  before being formatted with `notmuch-search-result-format`.  It is
+  no longer padded.  The default format has been updated, but if
+  you've customized this variable, you may have to change your date
+  format from `"%s "` to `"%12s "`.
+
 Notmuch 0.13.2 (2012-06-02)
 ===========================
 
-- 
1.7.10

Re: [PATCH v3 0/9] JSON-based search-mode

[Mark Walters]

On Mon, 09 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This fixes the docstings for notmuch-json-read and notmuch-json-eof as
> suggested by Mark.  No other changes.

This is a definite +1 from me: 

It all seems to work perfectly, and fixes a definite bug (and makes it
easy to fix some other bugs when changing tags in search results etc)

Moreover I modified notmuch-pick to use the incremental parser and it
also works fine. Pick parses the output from notmuch-show so this
provides good evidence that the parser can parse both of the cli json
outputs properly (I believe it will parse any JSON but the fact that it
parses both outputs that we use is a good sign).

Best wishes

Mark

Re: [PATCH v3 0/9] JSON-based search-mode

[Tomi Ollila]

On Tue, Jul 10 2012, Austin Clements <amdragon@MIT.EDU> wrote:

> This fixes the docstings for notmuch-json-read and notmuch-json-eof as
> suggested by Mark.  No other changes.

+1

Tomi

Re: [PATCH v3 1/9] emacs: Clean up notmuch-search-show-result

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> This simplifies the code and makes it no longer cubic in the number of
> result fields.
> ---

series pushed.

d

[PATCH v4 0/8] emacs: JSON-based search cleanups

[Austin Clements]

This version fixes several bugs found in the previous version.  I
replaced the insert-before-markers trick in
notmuch-search-update-result with direct point manipulation.  This
fixes the problem with authors getting unhidden when a result is
updated with point after the authors on the line (since it no longer
deletes the region with the invisibility overlay).  I also removed the
scrolling hack (which was partially necessitated by
insert-before-markers), so that archiving the last visible result will
properly scroll the buffer instead of jumping point to the middle of
the visible window.  As a result, the window may scroll when updating
a multiline result, however, it will scroll to show the entire result
(unlike an earlier version where it scrolled to cut off the result
because of an interaction with insert-before-markers).  Finally, I
fixed notmuch-search-last-thread so that it behaves like it did before
when there are no results, rather than failing with an obscure error.

I also updated the customize documentation and NEWS to indicate that
multiline search results are considered experimental.

Diff from v3:

diff --git a/NEWS b/NEWS
index 7b33f0d..7b1f36c 100644
--- a/NEWS
+++ b/NEWS
@@ -25,7 +25,7 @@ The formatting of tags in search results can now be customized
   `notmuch-search-result-format` would usually break tagging from
   search-mode.  We no longer make assumptions about the format.
 
-Multi-line search result formats are now supported
+Experimental support for multi-line search result formats
 
   It is now possible to embed newlines in
   `notmuch-search-result-format` to make individual search results
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ec760dc..fd1836f 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -70,12 +70,12 @@
 For example:
 	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
 					     \(\"subject\" . \"%s\"\)\)\)
-Line breaks are permitted in format strings.  Note that a line
-break at the end of an \"authors\" field will get elided if the
-authors list is long; place it instead at the beginning of the
-following field.  To enter a line break when setting this
-variable with setq, use \\n.  To enter a line break in customize,
-press \\[quoted-insert] C-j."
+Line breaks are permitted in format strings (though this is
+currently experimental).  Note that a line break at the end of an
+\"authors\" field will get elided if the authors list is long;
+place it instead at the beginning of the following field.  To
+enter a line break when setting this variable with setq, use \\n.
+To enter a line break in customize, press \\[quoted-insert] C-j."
   :type '(alist :key-type (string) :value-type (string))
   :group 'notmuch-search)
 
@@ -310,7 +310,8 @@ For a mouse binding, return nil."
   (interactive)
   (goto-char (point-max))
   (forward-line -2)
-  (goto-char (notmuch-search-result-beginning)))
+  (let ((beg (notmuch-search-result-beginning)))
+    (when beg (goto-char beg))))
 
 (defun notmuch-search-first-thread ()
   "Select the first thread in the search results."
@@ -599,30 +600,31 @@ This function advances the next thread when finished."
 
 (defun notmuch-search-update-result (result &optional pos)
   "Replace the result object of the thread at POS (or point) by
-RESULT and redraw it."
+RESULT and redraw it.
+
+This will keep point in a reasonable location.  However, if there
+are enclosing save-excursions and the saved point is in the
+result being updated, the point will be restored to the beginning
+of the result."
   (let ((start (notmuch-search-result-beginning pos))
 	(end (notmuch-search-result-end pos))
 	(init-point (point))
-	(init-start (window-start))
 	(inhibit-read-only t))
     ;; Delete the current thread
     (delete-region start end)
     ;; Insert the updated thread
     (notmuch-search-show-result result start)
-    ;; There may have been markers pointing into the text we just
-    ;; replaced.  For the most part, there's nothing we can do about
-    ;; this, but we can fix markers that were at point (which includes
-    ;; point itself and any save-excursions for which point hasn't
-    ;; moved) by re-inserting the text that should come before point
-    ;; before markers.
+    ;; If point was inside the old result, make an educated guess
+    ;; about where to place it now.  Unfortunately, this won't work
+    ;; with save-excursion (or any other markers that would be nice to
+    ;; preserve, such as the window start), but there's nothing we can
+    ;; do about that without a way to retrieve markers in a region.
     (when (and (>= init-point start) (<= init-point end))
       (let* ((new-end (notmuch-search-result-end start))
 	     (new-point (if (= init-point end)
 			    new-end
 			  (min init-point (- new-end 1)))))
-	(insert-before-markers (delete-and-extract-region start new-point))))
-    ;; We also may have shifted the window scroll.  Fix it.
-    (set-window-start (selected-window) init-start)))
+	(goto-char new-point)))))
 
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"

[PATCH v4 1/8] emacs: Record thread search result object in a text property

[Austin Clements]

This also provides utility functions for working with this text
property that get its value, find its start, and find its end.
---
 emacs/notmuch.el |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index fabb7c0..ef18927 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -401,6 +401,32 @@ Complete list of currently available key bindings:
 	mode-name "notmuch-search")
   (setq buffer-read-only t))
 
+(defun notmuch-search-get-result (&optional pos)
+  "Return the result object for the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (get-text-property (or pos (point)) 'notmuch-search-result))
+
+(defun notmuch-search-result-beginning (&optional pos)
+  "Return the point at the beginning of the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (when (notmuch-search-get-result pos)
+    ;; We pass 1+point because previous-single-property-change starts
+    ;; searching one before the position we give it.
+    (previous-single-property-change (1+ (or pos (point)))
+				     'notmuch-search-result nil (point-min))))
+
+(defun notmuch-search-result-end (&optional pos)
+  "Return the point at the end of the thread at POS (or point).
+
+The returned point will be just after the newline character that
+ends the result line.  If there is no thread at POS (or point),
+returns nil"
+  (when (notmuch-search-get-result pos)
+    (next-single-property-change (or pos (point)) 'notmuch-search-result
+				 nil (point-max))))
+
 (defun notmuch-search-properties-in-region (property beg end)
   (save-excursion
     (let ((output nil)
@@ -736,6 +762,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-result result)
 	(put-text-property beg (point) 'notmuch-search-thread-id
 			   (concat "thread:" (plist-get result :thread)))
 	(put-text-property beg (point) 'notmuch-search-authors
-- 
1.7.10

[PATCH v4 2/8] emacs: Use text properties instead of overlays for tag coloring

[Austin Clements]

Previously, tag-based search result highlighting was done by creating
an overlay over each search result.  However, overlays have annoying
front- and rear-advancement semantics that make it difficult to
manipulate text at their boundaries, which the next patch will do.
They also have performance problems (creating an overlay is linear in
the number of overlays between point and the new overlay, making
highlighting a search buffer quadratic in the number of results).

Text properties have neither problem.  However, text properties make
it more difficult to apply multiple faces since, unlike with overlays,
a given character can only have a single 'face text property.  Hence,
we introduce a utility function that combines faces into any existing
'face text properties.

Using this utility function, it's straightforward to apply all of the
appropriate tag faces in notmuch-search-color-line.
---
 emacs/notmuch-lib.el |   15 +++++++++++++++
 emacs/notmuch.el     |   21 +++++++--------------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index aa25513..30db58f 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -269,6 +269,21 @@ current buffer, if possible."
   (loop for (key value . rest) on plist by #'cddr
 	collect (cons (intern (substring (symbol-name key) 1)) value)))
 
+(defun notmuch-combine-face-text-property (start end face)
+  "Combine FACE into the 'face text property between START and END.
+
+This function combines FACE with any existing faces between START
+and END.  Attributes specified by FACE take precedence over
+existing attributes.  FACE must be a face name (a symbol or
+string), a property list of face attributes, or a list of these."
+
+  (let ((pos start))
+    (while (< pos end)
+      (let ((cur (get-text-property pos 'face))
+	    (next (next-single-property-change pos 'face nil end)))
+	(put-text-property pos next 'face (cons face cur))
+	(setq pos next)))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ef18927..82c148d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -633,20 +633,13 @@ foreground and blue background."
 
 (defun notmuch-search-color-line (start end line-tag-list)
   "Colorize lines in `notmuch-show' based on tags."
-  ;; Create the overlay only if the message has tags which match one
-  ;; of those specified in `notmuch-search-line-faces'.
-  (let (overlay)
-    (mapc (lambda (elem)
-	    (let ((tag (car elem))
-		  (attributes (cdr elem)))
-	      (when (member tag line-tag-list)
-		(when (not overlay)
-		  (setq overlay (make-overlay start end)))
-		;; Merge the specified properties with any already
-		;; applied from an earlier match.
-		(overlay-put overlay 'face
-			     (append (overlay-get overlay 'face) attributes)))))
-	  notmuch-search-line-faces)))
+  (mapc (lambda (elem)
+	  (let ((tag (car elem))
+		(attributes (cdr elem)))
+	    (when (member tag line-tag-list)
+	      (notmuch-combine-face-text-property start end attributes))))
+	;; Reverse the list so earlier entries take precedence
+	(reverse notmuch-search-line-faces)))
 
 (defun notmuch-search-author-propertize (authors)
   "Split `authors' into matching and non-matching authors and
-- 
1.7.10

[PATCH v4 3/8] emacs: Update tags by rewriting the search result line in place

[Austin Clements]

Now that we keep the full thread result object, we can refresh a
result after any changes by simply deleting and reconstructing the
result line from scratch.

A convenient side-effect of this wholesale replacement is that search
now re-applies notmuch-search-line-faces when tags change.
---
 emacs/notmuch.el |   61 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 82c148d..6995482 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	    (error (buffer-substring beg end))
 	    ))))))
 
-(defun notmuch-search-set-tags (tags)
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (forward-char)
-    (let ((beg (point))
-	  (inhibit-read-only t))
-      (re-search-forward ")")
-      (backward-char)
-      (let ((end (point)))
-	(delete-region beg end)
-	(insert (propertize (mapconcat  'identity tags " ")
-			    'face 'notmuch-tag-face))))))
-
-(defun notmuch-search-get-tags ()
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (let ((beg (+ (point) 1)))
-      (re-search-forward ")")
-      (let ((end (- (point) 1)))
-	(split-string (buffer-substring-no-properties beg end))))))
+(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)))
+
+(defun notmuch-search-get-tags (&optional pos)
+  (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
   (save-excursion
@@ -583,6 +567,34 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
+(defun notmuch-search-update-result (result &optional pos)
+  "Replace the result object of the thread at POS (or point) by
+RESULT and redraw it.
+
+This will keep point in a reasonable location.  However, if there
+are enclosing save-excursions and the saved point is in the
+result being updated, the point will be restored to the beginning
+of the result."
+  (let ((start (notmuch-search-result-beginning pos))
+	(end (notmuch-search-result-end pos))
+	(init-point (point))
+	(inhibit-read-only t))
+    ;; Delete the current thread
+    (delete-region start end)
+    ;; Insert the updated thread
+    (notmuch-search-show-result result start)
+    ;; If point was inside the old result, make an educated guess
+    ;; about where to place it now.  Unfortunately, this won't work
+    ;; with save-excursion (or any other markers that would be nice to
+    ;; preserve, such as the window start), but there's nothing we can
+    ;; do about that without a way to retrieve markers in a region.
+    (when (and (>= init-point start) (<= init-point end))
+      (let* ((new-end (notmuch-search-result-end start))
+	     (new-point (if (= init-point end)
+			    new-end
+			  (min init-point (- new-end 1)))))
+	(goto-char new-point)))))
+
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
@@ -745,10 +757,11 @@ non-authors is found, assume that all of the authors match."
 			 (mapconcat 'identity (plist-get result :tags) " ")
 			 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (result)
+(defun notmuch-search-show-result (result &optional pos)
+  "Insert RESULT at POS or the end of the buffer if POS is null."
   ;; Ignore excluded matches
   (unless (= (plist-get result :matched) 0)
-    (let ((beg (point-max)))
+    (let ((beg (or pos (point-max))))
       (save-excursion
 	(goto-char beg)
 	(dolist (spec notmuch-search-result-format)
-- 
1.7.10

[PATCH v4 4/8] emacs: Use result text properties for search result iteration

[Austin Clements]

This simplifies the traversal of regions of results and eliminates the
need for save-excursions (which tend to get in the way of maintaining
point when we make changes to the buffer).  It also fixes some strange
corner cases in the old line-based code where results that bordered
the region but were not included in it could be affected by region
commands.  Coincidentally, this also essentially enables multi-line
search result formats; the only remaining non-multi-line-capable
functions are notmuch-search-{next,previous}-thread, which are only
used for interactive navigation.
---
 emacs/notmuch.el |   78 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 6995482..f79d004 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -69,7 +69,13 @@
 	date, count, authors, subject, tags
 For example:
 	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
-					     \(\"subject\" . \"%s\"\)\)\)"
+					     \(\"subject\" . \"%s\"\)\)\)
+Line breaks are permitted in format strings (though this is
+currently experimental).  Note that a line break at the end of an
+\"authors\" field will get elided if the authors list is long;
+place it instead at the beginning of the following field.  To
+enter a line break when setting this variable with setq, use \\n.
+To enter a line break in customize, press \\[quoted-insert] C-j."
   :type '(alist :key-type (string) :value-type (string))
   :group 'notmuch-search)
 
@@ -427,17 +433,40 @@ returns nil"
     (next-single-property-change (or pos (point)) 'notmuch-search-result
 				 nil (point-max))))
 
+(defun notmuch-search-foreach-result (beg end function)
+  "Invoke FUNCTION for each result between BEG and END.
+
+FUNCTION should take one argument.  It will be applied to the
+character position of the beginning of each result that overlaps
+the region between points BEG and END.  As a special case, if (=
+BEG END), FUNCTION will be applied to the result containing point
+BEG."
+
+  (lexical-let ((pos (notmuch-search-result-beginning beg))
+		;; End must be a marker in case function changes the
+		;; text.
+		(end (copy-marker end))
+		;; Make sure we examine at least one result, even if
+		;; (= beg end).
+		(first t))
+    ;; We have to be careful if the region extends beyond the results.
+    ;; In this case, pos could be null or there could be no result at
+    ;; pos.
+    (while (and pos (or (< pos end) first))
+      (when (notmuch-search-get-result pos)
+	(funcall function pos))
+      (setq pos (notmuch-search-result-end pos)
+	    first nil))))
+;; Unindent the function argument of notmuch-search-foreach-result so
+;; the indentation of callers doesn't get out of hand.
+(put 'notmuch-search-foreach-result 'lisp-indent-function 2)
+
 (defun notmuch-search-properties-in-region (property beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (beginning-of-line)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (cons (get-text-property (point) property) output))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(push (get-text-property pos property) output)))
+    output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
@@ -517,28 +546,21 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
   (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (append output (notmuch-search-get-tags)))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(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)))
     (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
-    (save-excursion
-      (let ((last-line (line-number-at-pos end))
-	    (max-line (- (line-number-at-pos (point-max)) 2)))
-	(goto-char beg)
-	(while (<= (line-number-at-pos) (min last-line max-line))
-	  (notmuch-search-set-tags
-	   (notmuch-update-tags (notmuch-search-get-tags) tag-changes))
-	  (forward-line))))))
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(notmuch-search-set-tags
+	 (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
+	 pos)))))
 
 (defun notmuch-search-tag (&optional tag-changes)
   "Change tags for the currently selected thread or region.
-- 
1.7.10

[PATCH v4 5/8] emacs: Replace other search text properties with result property

[Austin Clements]

Since the result object contains everything that the other text
properties recorded, we can remove the other text properties and
simply look in the plist of the appropriate result object.
---
 emacs/notmuch.el |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f79d004..7180b9d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -465,16 +465,18 @@ BEG."
   (let (output)
     (notmuch-search-foreach-result beg end
       (lambda (pos)
-	(push (get-text-property pos property) output)))
+	(push (plist-get (notmuch-search-get-result pos) property) output)))
     output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
-  (get-text-property (point) 'notmuch-search-thread-id))
+  (let ((thread (plist-get (notmuch-search-get-result) :thread)))
+    (when thread (concat "thread:" thread))))
 
 (defun notmuch-search-find-thread-id-region (beg end)
   "Return a list of threads for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-thread-id beg end))
+  (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"
@@ -482,19 +484,19 @@ BEG."
 
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
-  (get-text-property (point) 'notmuch-search-authors))
+  (plist-get (notmuch-search-get-result) :authors))
 
 (defun notmuch-search-find-authors-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-authors beg end))
+  (notmuch-search-properties-in-region :authors beg end))
 
 (defun notmuch-search-find-subject ()
   "Return the subject for the current thread"
-  (get-text-property (point) 'notmuch-search-subject))
+  (plist-get (notmuch-search-get-result) :subject))
 
 (defun notmuch-search-find-subject-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-subject beg end))
+  (notmuch-search-properties-in-region :subject beg end))
 
 (defun notmuch-search-show-thread ()
   "Display the currently selected thread."
@@ -790,13 +792,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
-	(put-text-property beg (point) 'notmuch-search-result result)
-	(put-text-property beg (point) 'notmuch-search-thread-id
-			   (concat "thread:" (plist-get result :thread)))
-	(put-text-property beg (point) 'notmuch-search-authors
-			   (plist-get result :authors))
-	(put-text-property beg (point) 'notmuch-search-subject
-			   (plist-get result :subject)))
+	(put-text-property beg (point) 'notmuch-search-result result))
       (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
-- 
1.7.10

[PATCH v4 6/8] emacs: Allow custom tags formatting

[Austin Clements]

Previously we ignored any notmuch-search-result-format customizations
for tag formatting because we needed to be able to parse back in the
result line and update the tags in place.  We no longer do either of
these things, so we can allow customization of this format.

(Coincidentally, previously we still allowed too much customization of
the tags format, since moving it earlier on the line or removing it
from the line would interfere with the tagging mechanism.  There is
now no problem with doing such things.)
---
 emacs/notmuch.el |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 7180b9d..d092528 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -775,11 +775,9 @@ non-authors is found, assume that all of the authors match."
     (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
-    ;; Ignore format-string here because notmuch-search-set-tags
-    ;; depends on the format of this
-    (insert (concat "(" (propertize
-			 (mapconcat 'identity (plist-get result :tags) " ")
-			 'font-lock-face 'notmuch-tag-face) ")")))))
+    (let ((tags-str (mapconcat 'identity (plist-get result :tags) " ")))
+      (insert (propertize (format format-string tags-str)
+			  'face 'notmuch-tag-face))))))
 
 (defun notmuch-search-show-result (result &optional pos)
   "Insert RESULT at POS or the end of the buffer if POS is null."
-- 
1.7.10

[PATCH v4 7/8] emacs: Fix navigation of multi-line search result formats

[Austin Clements]

At this point, the only remaining functions that don't support
multi-line search result formats are the thread navigation functions.
This patch fixes that by rewriting them in terms of
notmuch-search-result-{beginning,end}.

This changes the behavior of notmuch-search-previous-thread slightly
so that if point isn't at the beginning of a result, it first moves
point to the beginning of the result.
---
 emacs/notmuch.el |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index d092528..fd1836f 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -293,18 +293,25 @@ For a mouse binding, return nil."
 (defun notmuch-search-next-thread ()
   "Select the next thread in the search results."
   (interactive)
-  (forward-line 1))
+  (when (notmuch-search-get-result (notmuch-search-result-end))
+    (goto-char (notmuch-search-result-end))))
 
 (defun notmuch-search-previous-thread ()
   "Select the previous thread in the search results."
   (interactive)
-  (forward-line -1))
+  (if (notmuch-search-get-result)
+      (unless (bobp)
+	(goto-char (notmuch-search-result-beginning (- (point) 1))))
+    ;; We must be past the end; jump to the last result
+    (notmuch-search-last-thread)))
 
 (defun notmuch-search-last-thread ()
   "Select the last thread in the search results."
   (interactive)
   (goto-char (point-max))
-  (forward-line -2))
+  (forward-line -2)
+  (let ((beg (notmuch-search-result-beginning)))
+    (when beg (goto-char beg))))
 
 (defun notmuch-search-first-thread ()
   "Select the first thread in the search results."
-- 
1.7.10

[PATCH v4 8/8] News for search cleanups

[Austin Clements]

---
 NEWS |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/NEWS b/NEWS
index a1a6e93..7b1f36c 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,20 @@ Maildir tag synchronization
 Emacs Interface
 ---------------
 
+Search results now get re-colored when tags are updated
+
+The formatting of tags in search results can now be customized
+
+  Previously, attempting to change the format of tags in
+  `notmuch-search-result-format` would usually break tagging from
+  search-mode.  We no longer make assumptions about the format.
+
+Experimental support for multi-line search result formats
+
+  It is now possible to embed newlines in
+  `notmuch-search-result-format` to make individual search results
+  span multiple lines.
+
 Search now uses the JSON format internally
 
   This should address problems with unusual characters in authors and
-- 
1.7.10

Re: [PATCH v4 0/8] emacs: JSON-based search cleanups

[Austin Clements]

This series was supposed to be in-reply-to
id:"1342306940-7499-1-git-send-email-amdragon@mit.edu".

On Sat, 21 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This version fixes several bugs found in the previous version.  I
> replaced the insert-before-markers trick in
> notmuch-search-update-result with direct point manipulation.  This
> fixes the problem with authors getting unhidden when a result is
> updated with point after the authors on the line (since it no longer
> deletes the region with the invisibility overlay).  I also removed the
> scrolling hack (which was partially necessitated by
> insert-before-markers), so that archiving the last visible result will
> properly scroll the buffer instead of jumping point to the middle of
> the visible window.  As a result, the window may scroll when updating
> a multiline result, however, it will scroll to show the entire result
> (unlike an earlier version where it scrolled to cut off the result
> because of an interaction with insert-before-markers).  Finally, I
> fixed notmuch-search-last-thread so that it behaves like it did before
> when there are no results, rather than failing with an obscure error.
>
> I also updated the customize documentation and NEWS to indicate that
> multiline search results are considered experimental.

Re: [PATCH v4 0/8] emacs: JSON-based search cleanups

[Mark Walters]

This version looks good to me +1

Best wishes

Mark

On Sat, 21 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This version fixes several bugs found in the previous version.  I
> replaced the insert-before-markers trick in
> notmuch-search-update-result with direct point manipulation.  This
> fixes the problem with authors getting unhidden when a result is
> updated with point after the authors on the line (since it no longer
> deletes the region with the invisibility overlay).  I also removed the
> scrolling hack (which was partially necessitated by
> insert-before-markers), so that archiving the last visible result will
> properly scroll the buffer instead of jumping point to the middle of
> the visible window.  As a result, the window may scroll when updating
> a multiline result, however, it will scroll to show the entire result
> (unlike an earlier version where it scrolled to cut off the result
> because of an interaction with insert-before-markers).  Finally, I
> fixed notmuch-search-last-thread so that it behaves like it did before
> when there are no results, rather than failing with an obscure error.
>
> I also updated the customize documentation and NEWS to indicate that
> multiline search results are considered experimental.
>
> Diff from v3:
>
> diff --git a/NEWS b/NEWS
> index 7b33f0d..7b1f36c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -25,7 +25,7 @@ The formatting of tags in search results can now be customized
>    `notmuch-search-result-format` would usually break tagging from
>    search-mode.  We no longer make assumptions about the format.
>  
> -Multi-line search result formats are now supported
> +Experimental support for multi-line search result formats
>  
>    It is now possible to embed newlines in
>    `notmuch-search-result-format` to make individual search results
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index ec760dc..fd1836f 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -70,12 +70,12 @@
>  For example:
>  	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
>  					     \(\"subject\" . \"%s\"\)\)\)
> -Line breaks are permitted in format strings.  Note that a line
> -break at the end of an \"authors\" field will get elided if the
> -authors list is long; place it instead at the beginning of the
> -following field.  To enter a line break when setting this
> -variable with setq, use \\n.  To enter a line break in customize,
> -press \\[quoted-insert] C-j."
> +Line breaks are permitted in format strings (though this is
> +currently experimental).  Note that a line break at the end of an
> +\"authors\" field will get elided if the authors list is long;
> +place it instead at the beginning of the following field.  To
> +enter a line break when setting this variable with setq, use \\n.
> +To enter a line break in customize, press \\[quoted-insert] C-j."
>    :type '(alist :key-type (string) :value-type (string))
>    :group 'notmuch-search)
>  
> @@ -310,7 +310,8 @@ For a mouse binding, return nil."
>    (interactive)
>    (goto-char (point-max))
>    (forward-line -2)
> -  (goto-char (notmuch-search-result-beginning)))
> +  (let ((beg (notmuch-search-result-beginning)))
> +    (when beg (goto-char beg))))
>  
>  (defun notmuch-search-first-thread ()
>    "Select the first thread in the search results."
> @@ -599,30 +600,31 @@ This function advances the next thread when finished."
>  
>  (defun notmuch-search-update-result (result &optional pos)
>    "Replace the result object of the thread at POS (or point) by
> -RESULT and redraw it."
> +RESULT and redraw it.
> +
> +This will keep point in a reasonable location.  However, if there
> +are enclosing save-excursions and the saved point is in the
> +result being updated, the point will be restored to the beginning
> +of the result."
>    (let ((start (notmuch-search-result-beginning pos))
>  	(end (notmuch-search-result-end pos))
>  	(init-point (point))
> -	(init-start (window-start))
>  	(inhibit-read-only t))
>      ;; Delete the current thread
>      (delete-region start end)
>      ;; Insert the updated thread
>      (notmuch-search-show-result result start)
> -    ;; There may have been markers pointing into the text we just
> -    ;; replaced.  For the most part, there's nothing we can do about
> -    ;; this, but we can fix markers that were at point (which includes
> -    ;; point itself and any save-excursions for which point hasn't
> -    ;; moved) by re-inserting the text that should come before point
> -    ;; before markers.
> +    ;; If point was inside the old result, make an educated guess
> +    ;; about where to place it now.  Unfortunately, this won't work
> +    ;; with save-excursion (or any other markers that would be nice to
> +    ;; preserve, such as the window start), but there's nothing we can
> +    ;; do about that without a way to retrieve markers in a region.
>      (when (and (>= init-point start) (<= init-point end))
>        (let* ((new-end (notmuch-search-result-end start))
>  	     (new-point (if (= init-point end)
>  			    new-end
>  			  (min init-point (- new-end 1)))))
> -	(insert-before-markers (delete-and-extract-region start new-point))))
> -    ;; We also may have shifted the window scroll.  Fix it.
> -    (set-window-start (selected-window) init-start)))
> +	(goto-char new-point)))))
>  
>  (defun notmuch-search-process-sentinel (proc msg)
>    "Add a message to let user know when \"notmuch search\" exits"

Re: [PATCH v4 0/8] emacs: JSON-based search cleanups

[Jameson Graef Rollins]

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

On Sat, Jul 21 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This version fixes several bugs found in the previous version.  I
> replaced the insert-before-markers trick in
> notmuch-search-update-result with direct point manipulation.  This
> fixes the problem with authors getting unhidden when a result is
> updated with point after the authors on the line (since it no longer
> deletes the region with the invisibility overlay).  I also removed the
> scrolling hack (which was partially necessitated by
> insert-before-markers), so that archiving the last visible result will
> properly scroll the buffer instead of jumping point to the middle of
> the visible window.  As a result, the window may scroll when updating
> a multiline result, however, it will scroll to show the entire result
> (unlike an earlier version where it scrolled to cut off the result
> because of an interaction with insert-before-markers).  Finally, I
> fixed notmuch-search-last-thread so that it behaves like it did before
> when there are no results, rather than failing with an obscure error.
>
> I also updated the customize documentation and NEWS to indicate that
> multiline search results are considered experimental.

This seems to fix the minor issues with the previous series, and seems
to be behaving great.  The changes and the documentation fixes look good
to me.

jamie.

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

Re: [PATCH v4 0/8] emacs: JSON-based search cleanups

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> This version fixes several bugs found in the previous version.  

pushed, 

d

Re: [PATCH v4 7/8] emacs: Fix navigation of multi-line search result formats

[Jani Nikula]

On Sat, 21 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> At this point, the only remaining functions that don't support
> multi-line search result formats are the thread navigation functions.
> This patch fixes that by rewriting them in terms of
> notmuch-search-result-{beginning,end}.
>
> This changes the behavior of notmuch-search-previous-thread slightly
> so that if point isn't at the beginning of a result, it first moves
> point to the beginning of the result.

Hi Austin, bisecting suggests this patch (committed as 5d0883e) breaks
notmuch-show-archive-thread-then-next and subsequently
notmuch-show-advance-and-archive for me. When showing the last thread in
the search results, notmuch-show-archive-thread-then-next used to exit
to the end of the search buffer. Now it redisplays the current
thread. When I hit SPC to cruise through mails, there's no indication
that I've reached the end of search results, other than the same thread
being displayed over and over.

BR,
Jani.


> ---
>  emacs/notmuch.el |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index d092528..fd1836f 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -293,18 +293,25 @@ For a mouse binding, return nil."
>  (defun notmuch-search-next-thread ()
>    "Select the next thread in the search results."
>    (interactive)
> -  (forward-line 1))
> +  (when (notmuch-search-get-result (notmuch-search-result-end))
> +    (goto-char (notmuch-search-result-end))))
>  
>  (defun notmuch-search-previous-thread ()
>    "Select the previous thread in the search results."
>    (interactive)
> -  (forward-line -1))
> +  (if (notmuch-search-get-result)
> +      (unless (bobp)
> +	(goto-char (notmuch-search-result-beginning (- (point) 1))))
> +    ;; We must be past the end; jump to the last result
> +    (notmuch-search-last-thread)))
>  
>  (defun notmuch-search-last-thread ()
>    "Select the last thread in the search results."
>    (interactive)
>    (goto-char (point-max))
> -  (forward-line -2))
> +  (forward-line -2)
> +  (let ((beg (notmuch-search-result-beginning)))
> +    (when beg (goto-char beg))))
>  
>  (defun notmuch-search-first-thread ()
>    "Select the first thread in the search results."
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: fix a bug introduced by the recent search cleanups.

[Mark Walters]

In commit 5d0883e the function notmuch-search-next-thread was changed.
In particular it only goes to the next message if there is a next
message. This breaks notmuch-show-archive-thread-then-next. Fix this
by going to the "next" message whenever we are on a current message.
---

> On Sat, 21 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> At this point, the only remaining functions that don't support
>> multi-line search result formats are the thread navigation functions.
>> This patch fixes that by rewriting them in terms of
>> notmuch-search-result-{beginning,end}.
>>
>> This changes the behavior of notmuch-search-previous-thread slightly
>> so that if point isn't at the beginning of a result, it first moves
>> point to the beginning of the result.
>
> Hi Austin, bisecting suggests this patch (committed as 5d0883e) breaks
> notmuch-show-archive-thread-then-next and subsequently
> notmuch-show-advance-and-archive for me. When showing the last thread in
> the search results, notmuch-show-archive-thread-then-next used to exit
> to the end of the search buffer. Now it redisplays the current
> thread. When I hit SPC to cruise through mails, there's no indication
> that I've reached the end of search results, other than the same thread
> being displayed over and over.

This might be the correct fix but I am not certain. It doesn't instantly
break for me! (It looks more similar to what happened before the parent
patch.)

Best wishes

Mark


 emacs/notmuch.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index fd1836f..d2d82a9 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -293,7 +293,7 @@ For a mouse binding, return nil."
 (defun notmuch-search-next-thread ()
   "Select the next thread in the search results."
   (interactive)
-  (when (notmuch-search-get-result (notmuch-search-result-end))
+  (when (notmuch-search-get-result)
     (goto-char (notmuch-search-result-end))))
 
 (defun notmuch-search-previous-thread ()
-- 
1.7.9.1

Re: [PATCH] emacs: fix a bug introduced by the recent search cleanups.

[Jani Nikula]

On Thu, 02 Aug 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> In commit 5d0883e the function notmuch-search-next-thread was changed.
> In particular it only goes to the next message if there is a next
> message. This breaks notmuch-show-archive-thread-then-next. Fix this
> by going to the "next" message whenever we are on a current message.

Works for me,
J.


> ---
>
>> On Sat, 21 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>>> At this point, the only remaining functions that don't support
>>> multi-line search result formats are the thread navigation functions.
>>> This patch fixes that by rewriting them in terms of
>>> notmuch-search-result-{beginning,end}.
>>>
>>> This changes the behavior of notmuch-search-previous-thread slightly
>>> so that if point isn't at the beginning of a result, it first moves
>>> point to the beginning of the result.
>>
>> Hi Austin, bisecting suggests this patch (committed as 5d0883e) breaks
>> notmuch-show-archive-thread-then-next and subsequently
>> notmuch-show-advance-and-archive for me. When showing the last thread in
>> the search results, notmuch-show-archive-thread-then-next used to exit
>> to the end of the search buffer. Now it redisplays the current
>> thread. When I hit SPC to cruise through mails, there's no indication
>> that I've reached the end of search results, other than the same thread
>> being displayed over and over.
>
> This might be the correct fix but I am not certain. It doesn't instantly
> break for me! (It looks more similar to what happened before the parent
> patch.)
>
> Best wishes
>
> Mark
>
>
>  emacs/notmuch.el |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index fd1836f..d2d82a9 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -293,7 +293,7 @@ For a mouse binding, return nil."
>  (defun notmuch-search-next-thread ()
>    "Select the next thread in the search results."
>    (interactive)
> -  (when (notmuch-search-get-result (notmuch-search-result-end))
> +  (when (notmuch-search-get-result)
>      (goto-char (notmuch-search-result-end))))
>  
>  (defun notmuch-search-previous-thread ()
> -- 
> 1.7.9.1

Re: [PATCH] emacs: fix a bug introduced by the recent search cleanups.

[Austin Clements]

On Thu, 02 Aug 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> In commit 5d0883e the function notmuch-search-next-thread was changed.
> In particular it only goes to the next message if there is a next
> message. This breaks notmuch-show-archive-thread-then-next. Fix this
> by going to the "next" message whenever we are on a current message.
> ---
>
>> On Sat, 21 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>>> At this point, the only remaining functions that don't support
>>> multi-line search result formats are the thread navigation functions.
>>> This patch fixes that by rewriting them in terms of
>>> notmuch-search-result-{beginning,end}.
>>>
>>> This changes the behavior of notmuch-search-previous-thread slightly
>>> so that if point isn't at the beginning of a result, it first moves
>>> point to the beginning of the result.
>>
>> Hi Austin, bisecting suggests this patch (committed as 5d0883e) breaks
>> notmuch-show-archive-thread-then-next and subsequently
>> notmuch-show-advance-and-archive for me. When showing the last thread in
>> the search results, notmuch-show-archive-thread-then-next used to exit
>> to the end of the search buffer. Now it redisplays the current
>> thread. When I hit SPC to cruise through mails, there's no indication
>> that I've reached the end of search results, other than the same thread
>> being displayed over and over.
>
> This might be the correct fix but I am not certain. It doesn't instantly
> break for me! (It looks more similar to what happened before the parent
> patch.)

LGTM.  Technically this is still different from the old code, since it
won't move forward once point is on the "End of search results" line.  I
can't imagine anything actually depending on that (maybe something that
loops until eobp?), but if we want to be paranoid, we could

(if (notmuch-search-get-result)
    (goto-char (notmuch-search-result-end)
  (forward-line 1)))

> Best wishes
>
> Mark
>
>
>  emacs/notmuch.el |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index fd1836f..d2d82a9 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -293,7 +293,7 @@ For a mouse binding, return nil."
>  (defun notmuch-search-next-thread ()
>    "Select the next thread in the search results."
>    (interactive)
> -  (when (notmuch-search-get-result (notmuch-search-result-end))
> +  (when (notmuch-search-get-result)
>      (goto-char (notmuch-search-result-end))))
>  
>  (defun notmuch-search-previous-thread ()
> -- 
> 1.7.9.1

Re: [PATCH] emacs: fix a bug introduced by the recent search cleanups.

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> In commit 5d0883e the function notmuch-search-next-thread was changed.
> In particular it only goes to the next message if there is a next
> message. This breaks notmuch-show-archive-thread-then-next. Fix this
> by going to the "next" message whenever we are on a current message.

Pushed, 

d