[PATCH v2 0/5] Make Emacs search use sexp format

[PATCH v2 0/5] Make Emacs search use sexp format

[Austin Clements]

This is v2 of id:1368851472-5382-1-git-send-email-amdragon@mit.edu.
The most substantial change from v1 is that the streaming S-expression
parser now requires the caller to invoke it from the appropriate
buffer and no longer attempts to track the buffer itself.  For subtle
reasons arising from per-window points, the only *correct* way to use
the interface before required the caller to invoke it from the
appropriate buffer anyway (or risk losing track of what had been
parsed).  The only place that currently invokes the streaming
S-expression parser already satisfied this requirement.

I decided *not* to use --stderr to redirect stderr.  As discussed on
IRC, --stderr causes serious problems for wrapper scripts, which
either have to handle --stderr themselves or risk mixing their own
stderr with stdout (e.g., errors from ssh) or, worse, redirecting
notmuch's stderr to a (world-readable) file on a *remote* machine.  I
did fix the exec-path problem that Tomi pointed out, so
notmuch-command will continue to be searched for in exec-path, like it
currently is.

The diff from v1 is below, with whitespace changes because of
re-indentation in the S-expression parser.

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index a543471..180f63d 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -493,10 +493,13 @@ status."
   (let* ((err-file (make-temp-file "nmerr"))
 	 ;; Use a pipe
 	 (process-connection-type nil)
+	 ;; Find notmuch using Emacs' `exec-path'
+	 (command (or (executable-find notmuch-command)
+		      (error "command not found: %s" notmuch-command)))
 	 (proc (apply #'start-process name buffer
 		      "sh" "-c"
 		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
-		      notmuch-command err-file args)))
+		      command err-file args)))
     (process-put proc 'err-file err-file)
     (process-put proc 'sub-sentinel sentinel)
     (process-put proc 'real-command (cons notmuch-command args))
@@ -507,7 +510,7 @@ status."
   (let ((err-file (process-get proc 'err-file))
 	(sub-sentinel (process-get proc 'sub-sentinel))
 	(real-command (process-get proc 'real-command)))
-    (condition-case-unless-debug err
+    (condition-case err
 	(progn
 	  ;; Invoke the sub-sentinel, if any
 	  (when sub-sentinel
@@ -530,7 +533,8 @@ status."
 	    (when warnings
 	      (notmuch-logged-error (car warnings) (cdr warnings)))))
       (error
-       ;; Don't signal an error from a sentinel
+       ;; Emacs behaves strangely if error an error escapes from a
+       ;; sentinel, so turns errors into messages.
        (message "%s" (error-message-string err))))
     (ignore-errors (delete-file err-file))))
 
diff --git a/emacs/notmuch-parser.el b/emacs/notmuch-parser.el
index 1b7cf64..d59c0e1 100644
--- a/emacs/notmuch-parser.el
+++ b/emacs/notmuch-parser.el
@@ -21,8 +21,8 @@
 
 (require 'cl)
 
-(defun notmuch-sexp-create-parser (buffer)
-  "Return a streaming S-expression parser that reads from BUFFER.
+(defun notmuch-sexp-create-parser ()
+  "Return a new streaming S-expression parser.
 
 This parser is designed to incrementally read an S-expression
 whose structure is known to the caller.  Like a typical
@@ -33,12 +33,11 @@ input to be a list 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 any data before point and may
-resynchronize after an error by moving point."
+The parser always consumes input from point in the current
+buffer.  Hence, the caller is allowed to delete any data before
+point and may resynchronize after an error by moving point."
 
   (vector 'notmuch-sexp-parser
-	  buffer
 	  ;; List depth
 	  0
 	  ;; Partial parse position marker
@@ -46,13 +45,12 @@ resynchronize after an error by moving point."
 	  ;; Partial parse state
 	  nil))
 
-(defmacro notmuch-sexp--buffer (sp)        `(aref ,sp 1))
-(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 2))
-(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 3))
-(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 4))
+(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 1))
+(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 2))
+(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 3))
 
 (defun notmuch-sexp-read (sp)
-  "Consume and return the value at point in SP's buffer.
+  "Consume and return the value at point in the current buffer.
 
 Returns 'retry if there is insufficient input to parse a complete
 value (though it may still move point over whitespace).  If the
@@ -61,14 +59,13 @@ list, 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-sexp--buffer sp)
   (skip-chars-forward " \n\r\t")
   (cond ((eobp) 'retry)
 	((= (char-after) ?\))
 	 ;; We've reached the end of a list
 	 (if (= (notmuch-sexp--depth sp) 0)
 	     ;; .. but we weren't in a list.  Let read signal the
-	       ;; error.
+	     ;; error to be consistent with all other code paths.
 	     (read (current-buffer))
 	   ;; Go up a level and return an end token
 	   (decf (notmuch-sexp--depth sp))
@@ -124,7 +121,7 @@ returns the value."
 		   'retry))
 	     (end-of-file
 	      (goto-char start)
-		'retry)))))))
+	      'retry))))))
 
 (defun notmuch-sexp-begin-list (sp)
   "Parse the beginning of a list value and enter the list.
@@ -136,7 +133,6 @@ returns t.  Later calls to `notmuch-sexp-read' will return the
 elements inside the list.  If the input in buffer is not the
 beginning of a list, throw invalid-read-syntax."
 
-  (with-current-buffer (notmuch-sexp--buffer sp)
   (skip-chars-forward " \n\r\t")
   (cond ((eobp) 'retry)
 	((= (char-after) ?\()
@@ -146,7 +142,7 @@ beginning of a list, throw invalid-read-syntax."
 	(t
 	 ;; Skip over the bad character like `read' does
 	 (forward-char)
-	   (signal 'invalid-read-syntax (list (string (char-before))))))))
+	 (signal 'invalid-read-syntax (list (string (char-before)))))))
 
 (defun notmuch-sexp-eof (sp)
   "Signal an error if there is more data in SP's buffer.
@@ -154,10 +150,9 @@ beginning of a list, throw invalid-read-syntax."
 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-sexp--buffer sp)
   (skip-chars-forward " \n\r\t")
   (unless (eobp)
-      (error "Trailing garbage following expression"))))
+    (error "Trailing garbage following expression")))
 
 (defvar notmuch-sexp--parser nil
   "The buffer-local notmuch-sexp-parser instance.
@@ -170,7 +165,7 @@ Used by `notmuch-sexp-parse-partial-list'.")
 (defun notmuch-sexp-parse-partial-list (result-function result-buffer)
   "Incrementally parse an S-expression list from the current buffer.
 
-This function consume an S-expression list from the current
+This function consumes an S-expression list from the current
 buffer, applying RESULT-FUNCTION in RESULT-BUFFER to each
 complete value in the list.  It operates incrementally and should
 be called whenever the input buffer has been extended with
@@ -180,7 +175,7 @@ move point in the input buffer."
   ;; Set up the initial state
   (unless (local-variable-p 'notmuch-sexp--parser)
     (set (make-local-variable 'notmuch-sexp--parser)
-	 (notmuch-sexp-create-parser (current-buffer)))
+	 (notmuch-sexp-create-parser))
     (set (make-local-variable 'notmuch-sexp--state) 'begin))
   (let (done)
     (while (not done)

[PATCH v2 1/5] test: Remove extraneous Emacs error handling test

[Austin Clements]

We now check error handling more carefully in the last test in
test/emacs and we're about to add more error handling tests.  (This
was also a strange place for this test, since it had nothing to do
with large search buffers.)
---
 test/emacs-large-search-buffer |   11 -----------
 1 file changed, 11 deletions(-)

diff --git a/test/emacs-large-search-buffer b/test/emacs-large-search-buffer
index 9dcbef5..8b1251f 100755
--- a/test/emacs-large-search-buffer
+++ b/test/emacs-large-search-buffer
@@ -29,15 +29,4 @@ test_emacs '(notmuch-search "*")
 sed -i -e s',  *, ,g' -e 's/xxx*/[BLOB]/g' OUTPUT
 test_expect_equal_file OUTPUT EXPECTED
 
-test_begin_subtest "Ensure that emacs doesn't drop error messages"
-test_emacs '(notmuch-search "--this-option-does-not-exist")
-	    (notmuch-test-wait)
-	    (test-output)'
-cat <<EOF >EXPECTED
-Error: Unexpected output from notmuch search:
-Unrecognized option: --this-option-does-not-exist
-End of search results.
-EOF
-test_expect_equal_file OUTPUT EXPECTED
-
 test_done
-- 
1.7.10.4

[PATCH v2 2/5] emacs: Utilities to manage asynchronous notmuch processes

[Austin Clements]

This provides a new notmuch-lib utility to start an asynchronous
notmuch process that handles redirecting of stderr and checking of the
exit status.  This is similar to `notmuch-call-notmuch-json', but for
asynchronous processes (and it leaves output processing to the
caller).
---
 emacs/notmuch-lib.el |   77 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 59b1ce3..180f63d 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -383,18 +383,21 @@ signaled error.  This function does not return."
   (error "%s" (concat msg (when extra
 			    " (see *Notmuch errors* for more details)"))))
 
-(defun notmuch-check-async-exit-status (proc msg)
+(defun notmuch-check-async-exit-status (proc msg &optional command err-file)
   "If PROC exited abnormally, pop up an error buffer and signal an error.
 
 This is a wrapper around `notmuch-check-exit-status' for
 asynchronous process sentinels.  PROC and MSG must be the
-arguments passed to the sentinel."
+arguments passed to the sentinel.  COMMAND and ERR-FILE, if
+provided, are passed to `notmuch-check-exit-status'.  If COMMAND
+is not provided, it is taken from `process-command'."
   (let ((exit-status
 	 (case (process-status proc)
 	   ((exit) (process-exit-status proc))
 	   ((signal) msg))))
     (when exit-status
-      (notmuch-check-exit-status exit-status (process-command proc)))))
+      (notmuch-check-exit-status exit-status (or command (process-command proc))
+				 nil err-file))))
 
 (defun notmuch-check-exit-status (exit-status command &optional output err-file)
   "If EXIT-STATUS is non-zero, pop up an error buffer and signal an error.
@@ -448,7 +451,7 @@ You may need to restart Emacs or upgrade your notmuch package."))
 	))))
 
 (defun notmuch-call-notmuch-json (&rest args)
-  "Invoke `notmuch-command' with `args' and return the parsed JSON output.
+  "Invoke `notmuch-command' with ARGS and return the parsed JSON output.
 
 The returned output will represent objects using property lists
 and arrays as lists.  If notmuch exits with a non-zero status,
@@ -469,6 +472,72 @@ an error."
 	      (json-read)))
 	(delete-file err-file)))))
 
+(defun notmuch-start-notmuch (name buffer sentinel &rest args)
+  "Start and return an asynchronous notmuch command.
+
+This starts and returns an asynchronous process running
+`notmuch-command' with ARGS.  The exit status is checked via
+`notmuch-check-async-exit-status'.  Output written to stderr is
+redirected and displayed when the process exits (even if the
+process exits successfully).  NAME and BUFFER are the same as in
+`start-process'.  SENTINEL is a process sentinel function to call
+when the process exits, or nil for none.  The caller must *not*
+invoke `set-process-sentinel' directly on the returned process,
+as that will interfere with the handling of stderr and the exit
+status."
+
+  ;; There is no way (as of Emacs 24.3) to capture stdout and stderr
+  ;; separately for asynchronous processes, or even to redirect stderr
+  ;; to a file, so we use a trivial shell wrapper to send stderr to a
+  ;; temporary file and clean things up in the sentinel.
+  (let* ((err-file (make-temp-file "nmerr"))
+	 ;; Use a pipe
+	 (process-connection-type nil)
+	 ;; Find notmuch using Emacs' `exec-path'
+	 (command (or (executable-find notmuch-command)
+		      (error "command not found: %s" notmuch-command)))
+	 (proc (apply #'start-process name buffer
+		      "sh" "-c"
+		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
+		      command err-file args)))
+    (process-put proc 'err-file err-file)
+    (process-put proc 'sub-sentinel sentinel)
+    (process-put proc 'real-command (cons notmuch-command args))
+    (set-process-sentinel proc #'notmuch-start-notmuch-sentinel)
+    proc))
+
+(defun notmuch-start-notmuch-sentinel (proc event)
+  (let ((err-file (process-get proc 'err-file))
+	(sub-sentinel (process-get proc 'sub-sentinel))
+	(real-command (process-get proc 'real-command)))
+    (condition-case err
+	(progn
+	  ;; Invoke the sub-sentinel, if any
+	  (when sub-sentinel
+	    (funcall sub-sentinel proc event))
+	  ;; Check the exit status.  This will signal an error if the
+	  ;; exit status is non-zero.
+	  (notmuch-check-async-exit-status proc event real-command err-file)
+	  ;; If that didn't signal an error, then any error output was
+	  ;; really warning output.  Show warnings, if any.
+	  (let ((warnings
+		 (with-temp-buffer
+		   (unless (= (second (insert-file-contents err-file)) 0)
+		     (end-of-line)
+		     ;; Show first line; stuff remaining lines in the
+		     ;; errors buffer.
+		     (let ((l1 (buffer-substring (point-min) (point))))
+		       (skip-chars-forward "\n")
+		       (cons l1 (unless (eobp)
+				  (buffer-substring (point) (point-max)))))))))
+	    (when warnings
+	      (notmuch-logged-error (car warnings) (cdr warnings)))))
+      (error
+       ;; Emacs behaves strangely if error an error escapes from a
+       ;; sentinel, so turns errors into messages.
+       (message "%s" (error-message-string err))))
+    (ignore-errors (delete-file err-file))))
+
 ;; This variable is used only buffer local, but it needs to be
 ;; declared globally first to avoid compiler warnings.
 (defvar notmuch-show-process-crypto nil)
-- 
1.7.10.4

[PATCH v2 3/5] emacs: Use async process helper for search

[Austin Clements]

Previously, search started the async notmuch process directly.  Now,
it uses `notmuch-start-notmuch'.  This simplifies the process sentinel
a bit and means that we no longer have to worry about errors
interleaved with the JSON output.

We also update the tests of Emacs error handling, since the error
output is now separated from the search results buffer.
---
 emacs/notmuch.el |   19 +++++--------------
 test/emacs       |   36 ++++++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 4c1a6ca..b8d9c44 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -653,15 +653,8 @@ of the result."
 		    ;; For version mismatch, there's no point in
 		    ;; showing the search buffer
 		    (when (or (= exit-status 20) (= exit-status 21))
-		      (kill-buffer))
-		    (condition-case err
-			(notmuch-check-async-exit-status proc msg)
-		      ;; Suppress the error signal since strange
-		      ;; things happen if a sentinel signals.  Mimic
-		      ;; the top-level's handling of error messages.
-		      (error
-		       (message "%s" (error-message-string err))
-		       (throw 'return nil)))
+		      (kill-buffer)
+		      (throw 'return nil))
 		    (if (and atbob
 			     (not (string= notmuch-search-target-thread "found")))
 			(set 'never-found-target-thread t)))))
@@ -938,10 +931,9 @@ Other optional parameters are used as follows:
       (erase-buffer)
       (goto-char (point-min))
       (save-excursion
-	(let ((proc (start-process
-		     "notmuch-search" buffer
-		     notmuch-command "search"
-		     "--format=json" "--format-version=1"
+	(let ((proc (notmuch-start-notmuch
+		     "notmuch-search" buffer #'notmuch-search-process-sentinel
+		     "search" "--format=json" "--format-version=1"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
@@ -951,7 +943,6 @@ Other optional parameters are used as follows:
 	      ;; should be called no matter how the process dies.
 	      (parse-buf (generate-new-buffer " *notmuch search parse*")))
 	  (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))))
     (run-hooks 'notmuch-search-hook)))
diff --git a/test/emacs b/test/emacs
index f033bdf..d38ae8c 100755
--- a/test/emacs
+++ b/test/emacs
@@ -853,11 +853,10 @@ test_expect_success "Rendering HTML mail with images" \
     'cat OUTPUT && grep -q smiley OUTPUT'
 
 
-test_begin_subtest "Search handles subprocess errors"
+test_begin_subtest "Search handles subprocess error exit codes"
 cat > notmuch_fail <<EOF
 #!/bin/sh
 echo This is output
-echo This is an error >&2
 exit 1
 EOF
 chmod a+x notmuch_fail
@@ -874,8 +873,6 @@ sed -i -e 's/^\[.*\]$/[XXX]/' ERROR
 test_expect_equal "$(cat OUTPUT; echo ---; cat MESSAGES; echo ---; cat ERROR)" "\
 Error: Unexpected output from notmuch search:
 This is output
-Error: Unexpected output from notmuch search:
-This is an error
 End of search results.
 ---
 $PWD/notmuch_fail exited with status 1 (see *Notmuch errors* for more details)
@@ -885,4 +882,35 @@ $PWD/notmuch_fail exited with status 1
 command: $PWD/notmuch_fail search --format\=json --format-version\=1 --sort\=newest-first tag\:inbox
 exit status: 1"
 
+test_begin_subtest "Search handles subprocess warnings"
+cat > notmuch_fail <<EOF
+#!/bin/sh
+echo This is output
+echo This is a warning >&2
+echo This is another warning >&2
+exit 0
+EOF
+chmod a+x notmuch_fail
+test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
+	       (with-current-buffer \"*Messages*\" (erase-buffer))
+	       (with-current-buffer \"*Notmuch errors*\" (erase-buffer))
+	       (notmuch-search \"tag:inbox\")
+	       (notmuch-test-wait)
+	       (with-current-buffer \"*Messages*\"
+		  (test-output \"MESSAGES\"))
+	       (with-current-buffer \"*Notmuch errors*\"
+		  (test-output \"ERROR\"))
+	       (test-output))"
+sed -i -e 's/^\[.*\]$/[XXX]/' ERROR
+test_expect_equal "$(cat OUTPUT; echo ---; cat MESSAGES; echo ---; cat ERROR)" "\
+Error: Unexpected output from notmuch search:
+This is output
+End of search results.
+---
+This is a warning (see *Notmuch errors* for more details)
+---
+[XXX]
+This is a warning
+This is another warning"
+
 test_done
-- 
1.7.10.4

[PATCH v2 4/5] emacs: Streaming S-expression parser

[Austin Clements]

This provides the same interface as the streaming JSON parser, but
reads S-expressions incrementally.  The only difference is that the
`notmuch-sexp-parse-partial-list' helper does not handle interleaved
error messages (since we now have the ability to separate these out at
the invocation level), so it no longer takes an error function and
does not need to do the horrible resynchronization that the JSON
parser had to.

Some implementation improvements have been made over the JSON parser.
This uses a vector instead of a list for the parser data structure,
since this allows faster access to elements (and modern versions of
Emacs handle storage of small vectors efficiently).  Private functions
follow the "prefix--name" convention.  And the implementation is much
simpler overall because S-expressions are much easier to parse.
---
 emacs/Makefile.local    |    1 +
 emacs/notmuch-parser.el |  207 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 208 insertions(+)
 create mode 100644 emacs/notmuch-parser.el

diff --git a/emacs/Makefile.local b/emacs/Makefile.local
index 456700a..a910aff 100644
--- a/emacs/Makefile.local
+++ b/emacs/Makefile.local
@@ -3,6 +3,7 @@
 dir := emacs
 emacs_sources := \
 	$(dir)/notmuch-lib.el \
+	$(dir)/notmuch-parser.el \
 	$(dir)/notmuch.el \
 	$(dir)/notmuch-query.el \
 	$(dir)/notmuch-show.el \
diff --git a/emacs/notmuch-parser.el b/emacs/notmuch-parser.el
new file mode 100644
index 0000000..d59c0e1
--- /dev/null
+++ b/emacs/notmuch-parser.el
@@ -0,0 +1,207 @@
+;; notmuch-parser.el --- streaming S-expression parser
+;;
+;; Copyright © Austin Clements
+;;
+;; This file is part of Notmuch.
+;;
+;; Notmuch is free software: you can redistribute it and/or modify it
+;; under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+;;
+;; Notmuch is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+;;
+;; You should have received a copy of the GNU General Public License
+;; along with Notmuch.  If not, see <http://www.gnu.org/licenses/>.
+;;
+;; Authors: Austin Clements <aclements@csail.mit.edu>
+
+(require 'cl)
+
+(defun notmuch-sexp-create-parser ()
+  "Return a new streaming S-expression parser.
+
+This parser is designed to incrementally read an S-expression
+whose structure is known to the caller.  Like a typical
+S-expression parsing interface, it provides a function to read a
+complete S-expression from the input.  However, it extends this
+with an additional function that requires the next value in the
+input to be a list 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 point in the current
+buffer.  Hence, the caller is allowed to delete any data before
+point and may resynchronize after an error by moving point."
+
+  (vector 'notmuch-sexp-parser
+	  ;; List depth
+	  0
+	  ;; Partial parse position marker
+	  nil
+	  ;; Partial parse state
+	  nil))
+
+(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 1))
+(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 2))
+(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 3))
+
+(defun notmuch-sexp-read (sp)
+  "Consume and return the value at point in the current buffer.
+
+Returns 'retry if there is insufficient input to parse a complete
+value (though it may still move point over whitespace).  If the
+parser is currently inside a list and the next token ends the
+list, 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."
+
+  (skip-chars-forward " \n\r\t")
+  (cond ((eobp) 'retry)
+	((= (char-after) ?\))
+	 ;; We've reached the end of a list
+	 (if (= (notmuch-sexp--depth sp) 0)
+	     ;; .. but we weren't in a list.  Let read signal the
+	     ;; error to be consistent with all other code paths.
+	     (read (current-buffer))
+	   ;; Go up a level and return an end token
+	   (decf (notmuch-sexp--depth sp))
+	   (forward-char)
+	   'end))
+	((= (char-after) ?\()
+	 ;; We're at the beginning of a list.  If we haven't started
+	 ;; a partial parse yet, attempt to read the list in its
+	 ;; entirety.  If this fails, or we've started a partial
+	 ;; parse, extend the partial parse to figure out when we
+	 ;; have a complete list.
+	 (catch 'return
+	   (when (null (notmuch-sexp--partial-state sp))
+	     (let ((start (point)))
+	       (condition-case nil
+		   (throw 'return (read (current-buffer)))
+		 (end-of-file (goto-char start)))))
+	   ;; Extend the partial parse
+	   (let (is-complete)
+	     (save-excursion
+	       (let* ((new-state (parse-partial-sexp
+				  (or (notmuch-sexp--partial-pos sp) (point))
+				  (point-max) 0 nil
+				  (notmuch-sexp--partial-state sp)))
+		      ;; A complete value is available if we've
+		      ;; reached depth 0.
+		      (depth (first new-state)))
+		 (assert (>= depth 0))
+		 (if (= depth 0)
+		     ;; Reset partial parse state
+		     (setf (notmuch-sexp--partial-state sp) nil
+			   (notmuch-sexp--partial-pos sp) nil
+			   is-complete t)
+		   ;; Update partial parse state
+		   (setf (notmuch-sexp--partial-state sp) new-state
+			 (notmuch-sexp--partial-pos sp) (point-marker)))))
+	     (if is-complete
+		 (read (current-buffer))
+	       'retry))))
+	(t
+	 ;; Attempt to read a non-compound value
+	 (let ((start (point)))
+	   (condition-case nil
+	       (let ((val (read (current-buffer))))
+		 ;; We got what looks like a complete read, but if
+		 ;; we reached the end of the buffer in the process,
+		 ;; we may not actually have all of the input we
+		 ;; need (unless it's a string, which is delimited).
+		 (if (or (stringp val) (not (eobp)))
+		     val
+		   ;; We can't be sure the input was complete
+		   (goto-char start)
+		   'retry))
+	     (end-of-file
+	      (goto-char start)
+	      'retry))))))
+
+(defun notmuch-sexp-begin-list (sp)
+  "Parse the beginning of a list value and enter the list.
+
+Returns 'retry if there is insufficient input to parse the
+beginning of the list.  If this is able to parse the beginning of
+a list, it moves point past the token that opens the list and
+returns t.  Later calls to `notmuch-sexp-read' will return the
+elements inside the list.  If the input in buffer is not the
+beginning of a list, throw invalid-read-syntax."
+
+  (skip-chars-forward " \n\r\t")
+  (cond ((eobp) 'retry)
+	((= (char-after) ?\()
+	 (forward-char)
+	 (incf (notmuch-sexp--depth sp))
+	 t)
+	(t
+	 ;; Skip over the bad character like `read' does
+	 (forward-char)
+	 (signal 'invalid-read-syntax (list (string (char-before)))))))
+
+(defun notmuch-sexp-eof (sp)
+  "Signal an error if there is more data in SP's buffer.
+
+Moves point to the beginning of any trailing data or to the end
+of the buffer if there is only trailing whitespace."
+
+  (skip-chars-forward " \n\r\t")
+  (unless (eobp)
+    (error "Trailing garbage following expression")))
+
+(defvar notmuch-sexp--parser nil
+  "The buffer-local notmuch-sexp-parser instance.
+
+Used by `notmuch-sexp-parse-partial-list'.")
+
+(defvar notmuch-sexp--state nil
+  "The buffer-local `notmuch-sexp-parse-partial-list' state.")
+
+(defun notmuch-sexp-parse-partial-list (result-function result-buffer)
+  "Incrementally parse an S-expression list from the current buffer.
+
+This function consumes an S-expression list from the current
+buffer, applying RESULT-FUNCTION in RESULT-BUFFER to each
+complete value in the list.  It operates incrementally and should
+be called whenever the input buffer has been extended with
+additional data.  The caller just needs to ensure it does not
+move point in the input buffer."
+
+  ;; Set up the initial state
+  (unless (local-variable-p 'notmuch-sexp--parser)
+    (set (make-local-variable 'notmuch-sexp--parser)
+	 (notmuch-sexp-create-parser))
+    (set (make-local-variable 'notmuch-sexp--state) 'begin))
+  (let (done)
+    (while (not done)
+      (case notmuch-sexp--state
+	(begin
+	 ;; Enter the list
+	 (if (eq (notmuch-sexp-begin-list notmuch-sexp--parser) 'retry)
+	     (setq done t)
+	   (setq notmuch-sexp--state 'result)))
+	(result
+	 ;; Parse a result
+	 (let ((result (notmuch-sexp-read notmuch-sexp--parser)))
+	   (case result
+	     (retry (setq done t))
+	     (end   (setq notmuch-sexp--state 'end))
+	     (t     (with-current-buffer result-buffer
+		      (funcall result-function result))))))
+	(end
+	 ;; Any trailing data is unexpected
+	 (notmuch-sexp-eof notmuch-sexp--parser)
+	 (setq done t)))))
+  ;; Clear out what we've parsed
+  (delete-region (point-min) (point)))
+
+(provide 'notmuch-parser)
+
+;; Local Variables:
+;; byte-compile-warnings: (not cl-functions)
+;; End:
-- 
1.7.10.4

[PATCH v2 5/5] emacs: Use streaming S-expr parser for search

[Austin Clements]

In addition to being the Right Thing to do, this noticeably improves
the time taken to display the first page of search results, since it's
roughly an order of magnitude faster than the JSON parser.
Interestingly, it does *not* significantly improve the time to
completely fill a large search buffer because for large search
buffers, the cost of creating author invisibility overlays and
inserting text (which slows down with more overlays) dominates.
However, the time required to display the first page of results is
generally more important to the user experience.
---
 emacs/notmuch.el |   13 +++----------
 test/emacs       |   10 +++-------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index b8d9c44..5a8c957 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -58,6 +58,7 @@
 (require 'notmuch-hello)
 (require 'notmuch-maildir-fcc)
 (require 'notmuch-message)
+(require 'notmuch-parser)
 
 (defcustom notmuch-search-result-format
   `(("date" . "%12s ")
@@ -809,13 +810,6 @@ 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 ((results-buf (process-buffer proc))
@@ -829,8 +823,7 @@ non-authors is found, assume that all of the authors match."
 	(save-excursion
 	  (goto-char (point-max))
 	  (insert string))
-	(notmuch-json-parse-partial-list 'notmuch-search-show-result
-					 'notmuch-search-show-error
+	(notmuch-sexp-parse-partial-list 'notmuch-search-show-result
 					 results-buf)))))
 
 (defun notmuch-search-tag-all (&optional tag-changes)
@@ -933,7 +926,7 @@ Other optional parameters are used as follows:
       (save-excursion
 	(let ((proc (notmuch-start-notmuch
 		     "notmuch-search" buffer #'notmuch-search-process-sentinel
-		     "search" "--format=json" "--format-version=1"
+		     "search" "--format=sexp" "--format-version=1"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
diff --git a/test/emacs b/test/emacs
index d38ae8c..7d42abf 100755
--- a/test/emacs
+++ b/test/emacs
@@ -856,7 +856,7 @@ test_expect_success "Rendering HTML mail with images" \
 test_begin_subtest "Search handles subprocess error exit codes"
 cat > notmuch_fail <<EOF
 #!/bin/sh
-echo This is output
+echo '()'
 exit 1
 EOF
 chmod a+x notmuch_fail
@@ -871,21 +871,19 @@ test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
 	       (test-output))"
 sed -i -e 's/^\[.*\]$/[XXX]/' ERROR
 test_expect_equal "$(cat OUTPUT; echo ---; cat MESSAGES; echo ---; cat ERROR)" "\
-Error: Unexpected output from notmuch search:
-This is output
 End of search results.
 ---
 $PWD/notmuch_fail exited with status 1 (see *Notmuch errors* for more details)
 ---
 [XXX]
 $PWD/notmuch_fail exited with status 1
-command: $PWD/notmuch_fail search --format\=json --format-version\=1 --sort\=newest-first tag\:inbox
+command: $PWD/notmuch_fail search --format\=sexp --format-version\=1 --sort\=newest-first tag\:inbox
 exit status: 1"
 
 test_begin_subtest "Search handles subprocess warnings"
 cat > notmuch_fail <<EOF
 #!/bin/sh
-echo This is output
+echo '()'
 echo This is a warning >&2
 echo This is another warning >&2
 exit 0
@@ -903,8 +901,6 @@ test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
 	       (test-output))"
 sed -i -e 's/^\[.*\]$/[XXX]/' ERROR
 test_expect_equal "$(cat OUTPUT; echo ---; cat MESSAGES; echo ---; cat ERROR)" "\
-Error: Unexpected output from notmuch search:
-This is output
 End of search results.
 ---
 This is a warning (see *Notmuch errors* for more details)
-- 
1.7.10.4

Re: [PATCH v2 2/5] emacs: Utilities to manage asynchronous notmuch processes

[Tomi Ollila]

Austin Clements <amdragon@MIT.EDU> writes:

> This provides a new notmuch-lib utility to start an asynchronous
> notmuch process that handles redirecting of stderr and checking of the
> exit status.  This is similar to `notmuch-call-notmuch-json', but for
> asynchronous processes (and it leaves output processing to the
> caller).
> ---
>  emacs/notmuch-lib.el |   77 +++++++++++++++++++++++++++++++++++++++++++++++---

...

> +(defun notmuch-start-notmuch (name buffer sentinel &rest args)
> +  "Start and return an asynchronous notmuch command.
> +
> +This starts and returns an asynchronous process running
> +`notmuch-command' with ARGS.  The exit status is checked via
> +`notmuch-check-async-exit-status'.  Output written to stderr is
> +redirected and displayed when the process exits (even if the
> +process exits successfully).  NAME and BUFFER are the same as in
> +`start-process'.  SENTINEL is a process sentinel function to call
> +when the process exits, or nil for none.  The caller must *not*
> +invoke `set-process-sentinel' directly on the returned process,
> +as that will interfere with the handling of stderr and the exit
> +status."
> +
> +  ;; There is no way (as of Emacs 24.3) to capture stdout and stderr
> +  ;; separately for asynchronous processes, or even to redirect stderr
> +  ;; to a file, so we use a trivial shell wrapper to send stderr to a
> +  ;; temporary file and clean things up in the sentinel.
> +  (let* ((err-file (make-temp-file "nmerr"))
> +	 ;; Use a pipe
> +	 (process-connection-type nil)
> +	 ;; Find notmuch using Emacs' `exec-path'
> +	 (command (or (executable-find notmuch-command)
> +		      (error "command not found: %s" notmuch-command)))
> +	 (proc (apply #'start-process name buffer
> +		      "sh" "-c"

I'd suggest "/bin/sh".


> +		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""

An alternative to the above ...

"exec 2>\"$1\"; shift; exec \"$0\" \"$@\""

... which one is better is a matter of quar^H^H^H^H^H taste :D

Everything else in this patch series looks good to me (as far as
I understood -- test passed and works as expected).


Tomi

> +		      command err-file args)))
> +    (process-put proc 'err-file err-file)
> +    (process-put proc 'sub-sentinel sentinel)
> +    (process-put proc 'real-command (cons notmuch-command args))
> +    (set-process-sentinel proc #'notmuch-start-notmuch-sentinel)
> +    proc))
> +

Re: [PATCH v2 0/5] Make Emacs search use sexp format

[Mark Walters]

This version LGTM +1. I will let you and Tomi decide which shell magic
to go with.

One very trivial comment on a comment below

Best wishes

Mark

Austin Clements <amdragon@MIT.EDU> writes:

> This is v2 of id:1368851472-5382-1-git-send-email-amdragon@mit.edu.
> The most substantial change from v1 is that the streaming S-expression
> parser now requires the caller to invoke it from the appropriate
> buffer and no longer attempts to track the buffer itself.  For subtle
> reasons arising from per-window points, the only *correct* way to use
> the interface before required the caller to invoke it from the
> appropriate buffer anyway (or risk losing track of what had been
> parsed).  The only place that currently invokes the streaming
> S-expression parser already satisfied this requirement.
>
> I decided *not* to use --stderr to redirect stderr.  As discussed on
> IRC, --stderr causes serious problems for wrapper scripts, which
> either have to handle --stderr themselves or risk mixing their own
> stderr with stdout (e.g., errors from ssh) or, worse, redirecting
> notmuch's stderr to a (world-readable) file on a *remote* machine.  I
> did fix the exec-path problem that Tomi pointed out, so
> notmuch-command will continue to be searched for in exec-path, like it
> currently is.
>
> The diff from v1 is below, with whitespace changes because of
> re-indentation in the S-expression parser.
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index a543471..180f63d 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -493,10 +493,13 @@ status."
>    (let* ((err-file (make-temp-file "nmerr"))
>  	 ;; Use a pipe
>  	 (process-connection-type nil)
> +	 ;; Find notmuch using Emacs' `exec-path'
> +	 (command (or (executable-find notmuch-command)
> +		      (error "command not found: %s" notmuch-command)))
>  	 (proc (apply #'start-process name buffer
>  		      "sh" "-c"
>  		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
> -		      notmuch-command err-file args)))
> +		      command err-file args)))
>      (process-put proc 'err-file err-file)
>      (process-put proc 'sub-sentinel sentinel)
>      (process-put proc 'real-command (cons notmuch-command args))
> @@ -507,7 +510,7 @@ status."
>    (let ((err-file (process-get proc 'err-file))
>  	(sub-sentinel (process-get proc 'sub-sentinel))
>  	(real-command (process-get proc 'real-command)))
> -    (condition-case-unless-debug err
> +    (condition-case err
>  	(progn
>  	  ;; Invoke the sub-sentinel, if any
>  	  (when sub-sentinel
> @@ -530,7 +533,8 @@ status."
>  	    (when warnings
>  	      (notmuch-logged-error (car warnings) (cdr warnings)))))
>        (error
> -       ;; Don't signal an error from a sentinel
> +       ;; Emacs behaves strangely if error an error escapes from a
> +       ;; sentinel, so turns errors into messages.

2 typos: "error an error" and turns should be turn.

>         (message "%s" (error-message-string err))))
>      (ignore-errors (delete-file err-file))))
>  
> diff --git a/emacs/notmuch-parser.el b/emacs/notmuch-parser.el
> index 1b7cf64..d59c0e1 100644
> --- a/emacs/notmuch-parser.el
> +++ b/emacs/notmuch-parser.el
> @@ -21,8 +21,8 @@
>  
>  (require 'cl)
>  
> -(defun notmuch-sexp-create-parser (buffer)
> -  "Return a streaming S-expression parser that reads from BUFFER.
> +(defun notmuch-sexp-create-parser ()
> +  "Return a new streaming S-expression parser.
>  
>  This parser is designed to incrementally read an S-expression
>  whose structure is known to the caller.  Like a typical
> @@ -33,12 +33,11 @@ input to be a list 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 any data before point and may
> -resynchronize after an error by moving point."
> +The parser always consumes input from point in the current
> +buffer.  Hence, the caller is allowed to delete any data before
> +point and may resynchronize after an error by moving point."
>  
>    (vector 'notmuch-sexp-parser
> -	  buffer
>  	  ;; List depth
>  	  0
>  	  ;; Partial parse position marker
> @@ -46,13 +45,12 @@ resynchronize after an error by moving point."
>  	  ;; Partial parse state
>  	  nil))
>  
> -(defmacro notmuch-sexp--buffer (sp)        `(aref ,sp 1))
> -(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 2))
> -(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 3))
> -(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 4))
> +(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 1))
> +(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 2))
> +(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 3))
>  
>  (defun notmuch-sexp-read (sp)
> -  "Consume and return the value at point in SP's buffer.
> +  "Consume and return the value at point in the current buffer.
>  
>  Returns 'retry if there is insufficient input to parse a complete
>  value (though it may still move point over whitespace).  If the
> @@ -61,14 +59,13 @@ list, 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-sexp--buffer sp)
>    (skip-chars-forward " \n\r\t")
>    (cond ((eobp) 'retry)
>  	((= (char-after) ?\))
>  	 ;; We've reached the end of a list
>  	 (if (= (notmuch-sexp--depth sp) 0)
>  	     ;; .. but we weren't in a list.  Let read signal the
> -	       ;; error.
> +	     ;; error to be consistent with all other code paths.
>  	     (read (current-buffer))
>  	   ;; Go up a level and return an end token
>  	   (decf (notmuch-sexp--depth sp))
> @@ -124,7 +121,7 @@ returns the value."
>  		   'retry))
>  	     (end-of-file
>  	      (goto-char start)
> -		'retry)))))))
> +	      'retry))))))
>  
>  (defun notmuch-sexp-begin-list (sp)
>    "Parse the beginning of a list value and enter the list.
> @@ -136,7 +133,6 @@ returns t.  Later calls to `notmuch-sexp-read' will return the
>  elements inside the list.  If the input in buffer is not the
>  beginning of a list, throw invalid-read-syntax."
>  
> -  (with-current-buffer (notmuch-sexp--buffer sp)
>    (skip-chars-forward " \n\r\t")
>    (cond ((eobp) 'retry)
>  	((= (char-after) ?\()
> @@ -146,7 +142,7 @@ beginning of a list, throw invalid-read-syntax."
>  	(t
>  	 ;; Skip over the bad character like `read' does
>  	 (forward-char)
> -	   (signal 'invalid-read-syntax (list (string (char-before))))))))
> +	 (signal 'invalid-read-syntax (list (string (char-before)))))))
>  
>  (defun notmuch-sexp-eof (sp)
>    "Signal an error if there is more data in SP's buffer.
> @@ -154,10 +150,9 @@ beginning of a list, throw invalid-read-syntax."
>  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-sexp--buffer sp)
>    (skip-chars-forward " \n\r\t")
>    (unless (eobp)
> -      (error "Trailing garbage following expression"))))
> +    (error "Trailing garbage following expression")))
>  
>  (defvar notmuch-sexp--parser nil
>    "The buffer-local notmuch-sexp-parser instance.
> @@ -170,7 +165,7 @@ Used by `notmuch-sexp-parse-partial-list'.")
>  (defun notmuch-sexp-parse-partial-list (result-function result-buffer)
>    "Incrementally parse an S-expression list from the current buffer.
>  
> -This function consume an S-expression list from the current
> +This function consumes an S-expression list from the current
>  buffer, applying RESULT-FUNCTION in RESULT-BUFFER to each
>  complete value in the list.  It operates incrementally and should
>  be called whenever the input buffer has been extended with
> @@ -180,7 +175,7 @@ move point in the input buffer."
>    ;; Set up the initial state
>    (unless (local-variable-p 'notmuch-sexp--parser)
>      (set (make-local-variable 'notmuch-sexp--parser)
> -	 (notmuch-sexp-create-parser (current-buffer)))
> +	 (notmuch-sexp-create-parser))
>      (set (make-local-variable 'notmuch-sexp--state) 'begin))
>    (let (done)
>      (while (not done)

Re: [PATCH v2 2/5] emacs: Utilities to manage asynchronous notmuch processes

[Austin Clements]

On Fri, 31 May 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Austin Clements <amdragon@MIT.EDU> writes:
>
>> This provides a new notmuch-lib utility to start an asynchronous
>> notmuch process that handles redirecting of stderr and checking of the
>> exit status.  This is similar to `notmuch-call-notmuch-json', but for
>> asynchronous processes (and it leaves output processing to the
>> caller).
>> ---
>>  emacs/notmuch-lib.el |   77 +++++++++++++++++++++++++++++++++++++++++++++++---
>
> ...
>
>> +(defun notmuch-start-notmuch (name buffer sentinel &rest args)
>> +  "Start and return an asynchronous notmuch command.
>> +
>> +This starts and returns an asynchronous process running
>> +`notmuch-command' with ARGS.  The exit status is checked via
>> +`notmuch-check-async-exit-status'.  Output written to stderr is
>> +redirected and displayed when the process exits (even if the
>> +process exits successfully).  NAME and BUFFER are the same as in
>> +`start-process'.  SENTINEL is a process sentinel function to call
>> +when the process exits, or nil for none.  The caller must *not*
>> +invoke `set-process-sentinel' directly on the returned process,
>> +as that will interfere with the handling of stderr and the exit
>> +status."
>> +
>> +  ;; There is no way (as of Emacs 24.3) to capture stdout and stderr
>> +  ;; separately for asynchronous processes, or even to redirect stderr
>> +  ;; to a file, so we use a trivial shell wrapper to send stderr to a
>> +  ;; temporary file and clean things up in the sentinel.
>> +  (let* ((err-file (make-temp-file "nmerr"))
>> +	 ;; Use a pipe
>> +	 (process-connection-type nil)
>> +	 ;; Find notmuch using Emacs' `exec-path'
>> +	 (command (or (executable-find notmuch-command)
>> +		      (error "command not found: %s" notmuch-command)))
>> +	 (proc (apply #'start-process name buffer
>> +		      "sh" "-c"
>
> I'd suggest "/bin/sh".

Done.

>> +		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
>
> An alternative to the above ...
>
> "exec 2>\"$1\"; shift; exec \"$0\" \"$@\""
>
> ... which one is better is a matter of quar^H^H^H^H^H taste :D

I like it.  Done.

> Everything else in this patch series looks good to me (as far as
> I understood -- test passed and works as expected).
>
>
> Tomi
>
>> +		      command err-file args)))
>> +    (process-put proc 'err-file err-file)
>> +    (process-put proc 'sub-sentinel sentinel)
>> +    (process-put proc 'real-command (cons notmuch-command args))
>> +    (set-process-sentinel proc #'notmuch-start-notmuch-sentinel)
>> +    proc))
>> +

Re: [PATCH v2 0/5] Make Emacs search use sexp format

[Austin Clements]

On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> This version LGTM +1. I will let you and Tomi decide which shell magic
> to go with.
>
> One very trivial comment on a comment below
>
> Best wishes
>
> Mark
>
> Austin Clements <amdragon@MIT.EDU> writes:
>
>> This is v2 of id:1368851472-5382-1-git-send-email-amdragon@mit.edu.
>> The most substantial change from v1 is that the streaming S-expression
>> parser now requires the caller to invoke it from the appropriate
>> buffer and no longer attempts to track the buffer itself.  For subtle
>> reasons arising from per-window points, the only *correct* way to use
>> the interface before required the caller to invoke it from the
>> appropriate buffer anyway (or risk losing track of what had been
>> parsed).  The only place that currently invokes the streaming
>> S-expression parser already satisfied this requirement.
>>
>> I decided *not* to use --stderr to redirect stderr.  As discussed on
>> IRC, --stderr causes serious problems for wrapper scripts, which
>> either have to handle --stderr themselves or risk mixing their own
>> stderr with stdout (e.g., errors from ssh) or, worse, redirecting
>> notmuch's stderr to a (world-readable) file on a *remote* machine.  I
>> did fix the exec-path problem that Tomi pointed out, so
>> notmuch-command will continue to be searched for in exec-path, like it
>> currently is.
>>
>> The diff from v1 is below, with whitespace changes because of
>> re-indentation in the S-expression parser.
>>
>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> index a543471..180f63d 100644
>> --- a/emacs/notmuch-lib.el
>> +++ b/emacs/notmuch-lib.el
>> @@ -493,10 +493,13 @@ status."
>>    (let* ((err-file (make-temp-file "nmerr"))
>>  	 ;; Use a pipe
>>  	 (process-connection-type nil)
>> +	 ;; Find notmuch using Emacs' `exec-path'
>> +	 (command (or (executable-find notmuch-command)
>> +		      (error "command not found: %s" notmuch-command)))
>>  	 (proc (apply #'start-process name buffer
>>  		      "sh" "-c"
>>  		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
>> -		      notmuch-command err-file args)))
>> +		      command err-file args)))
>>      (process-put proc 'err-file err-file)
>>      (process-put proc 'sub-sentinel sentinel)
>>      (process-put proc 'real-command (cons notmuch-command args))
>> @@ -507,7 +510,7 @@ status."
>>    (let ((err-file (process-get proc 'err-file))
>>  	(sub-sentinel (process-get proc 'sub-sentinel))
>>  	(real-command (process-get proc 'real-command)))
>> -    (condition-case-unless-debug err
>> +    (condition-case err
>>  	(progn
>>  	  ;; Invoke the sub-sentinel, if any
>>  	  (when sub-sentinel
>> @@ -530,7 +533,8 @@ status."
>>  	    (when warnings
>>  	      (notmuch-logged-error (car warnings) (cdr warnings)))))
>>        (error
>> -       ;; Don't signal an error from a sentinel
>> +       ;; Emacs behaves strangely if error an error escapes from a
>> +       ;; sentinel, so turns errors into messages.
>
> 2 typos: "error an error" and turns should be turn.

Oof.  I must have been sleepy when I wrote that.  Fixed.

>>         (message "%s" (error-message-string err))))
>>      (ignore-errors (delete-file err-file))))
>>  
>> diff --git a/emacs/notmuch-parser.el b/emacs/notmuch-parser.el
>> index 1b7cf64..d59c0e1 100644
>> --- a/emacs/notmuch-parser.el
>> +++ b/emacs/notmuch-parser.el
>> @@ -21,8 +21,8 @@
>>  
>>  (require 'cl)
>>  
>> -(defun notmuch-sexp-create-parser (buffer)
>> -  "Return a streaming S-expression parser that reads from BUFFER.
>> +(defun notmuch-sexp-create-parser ()
>> +  "Return a new streaming S-expression parser.
>>  
>>  This parser is designed to incrementally read an S-expression
>>  whose structure is known to the caller.  Like a typical
>> @@ -33,12 +33,11 @@ input to be a list 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 any data before point and may
>> -resynchronize after an error by moving point."
>> +The parser always consumes input from point in the current
>> +buffer.  Hence, the caller is allowed to delete any data before
>> +point and may resynchronize after an error by moving point."
>>  
>>    (vector 'notmuch-sexp-parser
>> -	  buffer
>>  	  ;; List depth
>>  	  0
>>  	  ;; Partial parse position marker
>> @@ -46,13 +45,12 @@ resynchronize after an error by moving point."
>>  	  ;; Partial parse state
>>  	  nil))
>>  
>> -(defmacro notmuch-sexp--buffer (sp)        `(aref ,sp 1))
>> -(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 2))
>> -(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 3))
>> -(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 4))
>> +(defmacro notmuch-sexp--depth (sp)         `(aref ,sp 1))
>> +(defmacro notmuch-sexp--partial-pos (sp)   `(aref ,sp 2))
>> +(defmacro notmuch-sexp--partial-state (sp) `(aref ,sp 3))
>>  
>>  (defun notmuch-sexp-read (sp)
>> -  "Consume and return the value at point in SP's buffer.
>> +  "Consume and return the value at point in the current buffer.
>>  
>>  Returns 'retry if there is insufficient input to parse a complete
>>  value (though it may still move point over whitespace).  If the
>> @@ -61,14 +59,13 @@ list, 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-sexp--buffer sp)
>>    (skip-chars-forward " \n\r\t")
>>    (cond ((eobp) 'retry)
>>  	((= (char-after) ?\))
>>  	 ;; We've reached the end of a list
>>  	 (if (= (notmuch-sexp--depth sp) 0)
>>  	     ;; .. but we weren't in a list.  Let read signal the
>> -	       ;; error.
>> +	     ;; error to be consistent with all other code paths.
>>  	     (read (current-buffer))
>>  	   ;; Go up a level and return an end token
>>  	   (decf (notmuch-sexp--depth sp))
>> @@ -124,7 +121,7 @@ returns the value."
>>  		   'retry))
>>  	     (end-of-file
>>  	      (goto-char start)
>> -		'retry)))))))
>> +	      'retry))))))
>>  
>>  (defun notmuch-sexp-begin-list (sp)
>>    "Parse the beginning of a list value and enter the list.
>> @@ -136,7 +133,6 @@ returns t.  Later calls to `notmuch-sexp-read' will return the
>>  elements inside the list.  If the input in buffer is not the
>>  beginning of a list, throw invalid-read-syntax."
>>  
>> -  (with-current-buffer (notmuch-sexp--buffer sp)
>>    (skip-chars-forward " \n\r\t")
>>    (cond ((eobp) 'retry)
>>  	((= (char-after) ?\()
>> @@ -146,7 +142,7 @@ beginning of a list, throw invalid-read-syntax."
>>  	(t
>>  	 ;; Skip over the bad character like `read' does
>>  	 (forward-char)
>> -	   (signal 'invalid-read-syntax (list (string (char-before))))))))
>> +	 (signal 'invalid-read-syntax (list (string (char-before)))))))
>>  
>>  (defun notmuch-sexp-eof (sp)
>>    "Signal an error if there is more data in SP's buffer.
>> @@ -154,10 +150,9 @@ beginning of a list, throw invalid-read-syntax."
>>  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-sexp--buffer sp)
>>    (skip-chars-forward " \n\r\t")
>>    (unless (eobp)
>> -      (error "Trailing garbage following expression"))))
>> +    (error "Trailing garbage following expression")))
>>  
>>  (defvar notmuch-sexp--parser nil
>>    "The buffer-local notmuch-sexp-parser instance.
>> @@ -170,7 +165,7 @@ Used by `notmuch-sexp-parse-partial-list'.")
>>  (defun notmuch-sexp-parse-partial-list (result-function result-buffer)
>>    "Incrementally parse an S-expression list from the current buffer.
>>  
>> -This function consume an S-expression list from the current
>> +This function consumes an S-expression list from the current
>>  buffer, applying RESULT-FUNCTION in RESULT-BUFFER to each
>>  complete value in the list.  It operates incrementally and should
>>  be called whenever the input buffer has been extended with
>> @@ -180,7 +175,7 @@ move point in the input buffer."
>>    ;; Set up the initial state
>>    (unless (local-variable-p 'notmuch-sexp--parser)
>>      (set (make-local-variable 'notmuch-sexp--parser)
>> -	 (notmuch-sexp-create-parser (current-buffer)))
>> +	 (notmuch-sexp-create-parser))
>>      (set (make-local-variable 'notmuch-sexp--state) 'begin))
>>    (let (done)
>>      (while (not done)