[PATCH v3 0/6] Make Emacs search use sexp format

[PATCH v3 0/6] Make Emacs search use sexp format

[Austin Clements]

This is v3 of id:1369934016-22308-1-git-send-email-amdragon@mit.edu.
This tweaks the shell invocation as suggested by Tomi and fixes two
comment typos pointed out by Mark.  It also adds a NEWS patch.  I'm
going to go ahead and mark this ready because of Tomi's and Mark's
reviews of v2.

The diff from v2 follows

diff --git a/NEWS b/NEWS
index 80abd97..2a4bde6 100644
--- a/NEWS
+++ b/NEWS
@@ -66,6 +66,17 @@ notmuch-vim, but of course that is their decision.
 Emacs Interface
 ---------------
 
+Better handling of errors in search buffers
+
+  Instead of interleaving errors in search result buffers, search mode
+  now reports errors in the minibuffer.
+
+Faster search results
+
+  Communication between search mode and the notmuch CLI is now more
+  efficient because it uses the CLI's S-expression support.  As a
+  result, search mode should now fill search buffers faster.
+
 No Emacs 22 support
 
   The Emacs 22 support added late 2010 was sufficient only for a short
diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 180f63d..a206808 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -497,8 +497,8 @@ status."
 	 (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\""
+		      "/bin/sh" "-c"
+		      "exec 2>\"$1\"; shift; exec \"$0\" \"$@\""
 		      command err-file args)))
     (process-put proc 'err-file err-file)
     (process-put proc 'sub-sentinel sentinel)
@@ -533,8 +533,8 @@ status."
 	    (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.
+       ;; Emacs behaves strangely if an error escapes from a sentinel,
+       ;; so turn errors into messages.
        (message "%s" (error-message-string err))))
     (ignore-errors (delete-file err-file))))
 

[PATCH v3 1/6] 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 v3 2/6] 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..a206808 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
+		      "/bin/sh" "-c"
+		      "exec 2>\"$1\"; shift; exec \"$0\" \"$@\""
+		      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 an error escapes from a sentinel,
+       ;; so turn 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 v3 3/6] 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 v3 4/6] 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 v3 5/6] 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

[PATCH v3 6/6] News for S-expression support in Emacs search mode

[Austin Clements]

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

diff --git a/NEWS b/NEWS
index 80abd97..2a4bde6 100644
--- a/NEWS
+++ b/NEWS
@@ -66,6 +66,17 @@ notmuch-vim, but of course that is their decision.
 Emacs Interface
 ---------------
 
+Better handling of errors in search buffers
+
+  Instead of interleaving errors in search result buffers, search mode
+  now reports errors in the minibuffer.
+
+Faster search results
+
+  Communication between search mode and the notmuch CLI is now more
+  efficient because it uses the CLI's S-expression support.  As a
+  result, search mode should now fill search buffers faster.
+
 No Emacs 22 support
 
   The Emacs 22 support added late 2010 was sufficient only for a short
-- 
1.7.10.4

Re: [PATCH v3 0/6] Make Emacs search use sexp format

[Tomi Ollila]

On Sat, Jun 01 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> This is v3 of id:1369934016-22308-1-git-send-email-amdragon@mit.edu.
> This tweaks the shell invocation as suggested by Tomi and fixes two
> comment typos pointed out by Mark.  It also adds a NEWS patch.  I'm
> going to go ahead and mark this ready because of Tomi's and Mark's
> reviews of v2.

+1

Tomi


>
> The diff from v2 follows
>
> diff --git a/NEWS b/NEWS
> index 80abd97..2a4bde6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -66,6 +66,17 @@ notmuch-vim, but of course that is their decision.
>  Emacs Interface
>  ---------------
>  
> +Better handling of errors in search buffers
> +
> +  Instead of interleaving errors in search result buffers, search mode
> +  now reports errors in the minibuffer.
> +
> +Faster search results
> +
> +  Communication between search mode and the notmuch CLI is now more
> +  efficient because it uses the CLI's S-expression support.  As a
> +  result, search mode should now fill search buffers faster.
> +
>  No Emacs 22 support
>  
>    The Emacs 22 support added late 2010 was sufficient only for a short
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 180f63d..a206808 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -497,8 +497,8 @@ status."
>  	 (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\""
> +		      "/bin/sh" "-c"
> +		      "exec 2>\"$1\"; shift; exec \"$0\" \"$@\""
>  		      command err-file args)))
>      (process-put proc 'err-file err-file)
>      (process-put proc 'sub-sentinel sentinel)
> @@ -533,8 +533,8 @@ status."
>  	    (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.
> +       ;; Emacs behaves strangely if an error escapes from a sentinel,
> +       ;; so turn errors into messages.
>         (message "%s" (error-message-string err))))
>      (ignore-errors (delete-file err-file))))
>  
>
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 0/6] Make Emacs search use sexp format

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> This is v3 of id:1369934016-22308-1-git-send-email-amdragon@mit.edu.
> This tweaks the shell invocation as suggested by Tomi and fixes two
> comment typos pointed out by Mark.  It also adds a NEWS patch.  I'm
> going to go ahead and mark this ready because of Tomi's and Mark's
> reviews of v2.

The first 5 I pushed. The NEWS patch has a conflict.

d

Re: [PATCH v3 0/6] Make Emacs search use sexp format

[Jameson Graef Rollins]

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

On Sat, Jun 01 2013, David Bremner <david@tethera.net> wrote:
> Austin Clements <amdragon@MIT.EDU> writes:
>
>> This is v3 of id:1369934016-22308-1-git-send-email-amdragon@mit.edu.
>> This tweaks the shell invocation as suggested by Tomi and fixes two
>> comment typos pointed out by Mark.  It also adds a NEWS patch.  I'm
>> going to go ahead and mark this ready because of Tomi's and Mark's
>> reviews of v2.
>
> The first 5 I pushed. The NEWS patch has a conflict.

I'm very happy to see the long-coming sexp handling working here.  Good
work, folks, particularly to Austin for getting the awesome asynchronous
processing stuff working.  Searches are now definitely noticeably
faster.

I am, however, seeing a couple of issues that we might want to address.

* Killing a search buffer that is still in the process of being filled
  causes errors to be thrown.  I'm seeing both of the following
  intermittently:

[Sun Jun  2 08:26:40 2013]
notmuch exited with status killed
command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
exit signal: killed

[Sun Jun  2 08:32:26 2013]
notmuch exited with status hangup
command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
exit signal: hangup

  This is somewhat understandable, as the notmuch binary exits with an
  error if it hasn't finished dumping the output, but given how common
  this particular scenario is I think we should try to avoid throwing
  errors in this circumstance.  I wonder if we shouldn't just modify the
  binary to not return non-zero if it was manually killed while
  processing the output, or at least special-case the particular error
  caused by manually killing the search.

* The next thing I'm seeing is this:

Opening input file: no such file or directory, /home/jrollins/tmp/nmerr5390CAY

  I'm not exactly sure what causes this error, but it looks to me like
  the temporary error file was removed before we were finished with it.

* Finally, something happened that caused *12,000* of the following lines
  to be sent to the *Notmuch errors* buffer:

A Xapian exception occurred performing query: The revision being read has been discarded - you should call Xapian::Database::reopen() and retry the operation

  Again, this was related to killing a search buffer that was still
  being filled. I'm pretty sure the database was not modified during
  this process.

Let me know if I can help provide any more info.

jamie.

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

Re: [PATCH v3 0/6] Make Emacs search use sexp format

[Austin Clements]

On Sun, 02 Jun 2013, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Sat, Jun 01 2013, David Bremner <david@tethera.net> wrote:
>> Austin Clements <amdragon@MIT.EDU> writes:
>>
>>> This is v3 of id:1369934016-22308-1-git-send-email-amdragon@mit.edu.
>>> This tweaks the shell invocation as suggested by Tomi and fixes two
>>> comment typos pointed out by Mark.  It also adds a NEWS patch.  I'm
>>> going to go ahead and mark this ready because of Tomi's and Mark's
>>> reviews of v2.
>>
>> The first 5 I pushed. The NEWS patch has a conflict.
>
> I'm very happy to see the long-coming sexp handling working here.  Good
> work, folks, particularly to Austin for getting the awesome asynchronous
> processing stuff working.  Searches are now definitely noticeably
> faster.
>
> I am, however, seeing a couple of issues that we might want to address.
>
> * Killing a search buffer that is still in the process of being filled
>   causes errors to be thrown.  I'm seeing both of the following
>   intermittently:
>
> [Sun Jun  2 08:26:40 2013]
> notmuch exited with status killed
> command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
> exit signal: killed
>
> [Sun Jun  2 08:32:26 2013]
> notmuch exited with status hangup
> command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
> exit signal: hangup
>
>   This is somewhat understandable, as the notmuch binary exits with an
>   error if it hasn't finished dumping the output, but given how common
>   this particular scenario is I think we should try to avoid throwing
>   errors in this circumstance.  I wonder if we shouldn't just modify the
>   binary to not return non-zero if it was manually killed while
>   processing the output, or at least special-case the particular error
>   caused by manually killing the search.

Your assessment is correct, of course.  The right place to fix this is
in Emacs, not the CLI (the CLI *can't* do anything about this, since it
gets killed by a signal).  Probably we should do something different in
the sentinel if the search process's buffer is no longer live.  Clearly
we should suppress the status error for the signal, but I think we still
should report anything that appeared in err-file because it may be
relevant to why the user killed the buffer (e.g., maybe a notmuch
wrapper was blocked on something).

> * The next thing I'm seeing is this:
>
> Opening input file: no such file or directory, /home/jrollins/tmp/nmerr5390CAY
>
>   I'm not exactly sure what causes this error, but it looks to me like
>   the temporary error file was removed before we were finished with it.

This one's pretty awesome (and I think is a bug in Emacs).  At a high
level, the sentinel is getting run twice and since the first call
deletes the error file, the second call fails.  At a low level, what
causes this is fascinating.

1) You kill the search buffer.  This invokes kill_buffer_processes,
   which sends a SIGHUP to notmuch, but doesn't do anything else.
   Meanwhile, the notmuch search process has printed some more output,
   but Emacs hasn't consumed it yet (this is critical).

2) Emacs gets a SIGCHLD from the dying notmuch process, which invokes
   handle_child_signal, which sets the new process status, but can't do
   anything else because it's a signal handler.

3) Emacs returns to its idle loop, which calls status_notify, which sees
   that the notmuch process has a new status.  This is where things get
   interesting.

3.1) Emacs guarantees that it will run process filters on any unconsumed
     output before running the process sentinel, so status_notify calls
     read_process_output, which consumes the final output and calls
     notmuch-search-process-filter.

3.1.1) notmuch-search-process-filter contains code to check if the
       search buffer is still alive and, since it's not, it calls
       delete-process.

3.1.1.1) delete-process correctly sees that the process is already dead
         and doesn't try to send another signal, *but* it still modifies
         the status to "killed".  To deal with the new status, it calls
         status_notify.  Dun dun dun.  We've seen this function before.

3.1.1.1.1) The *recursive* status_notify invocation sees that the
           process has a new status and doesn't have any more output to
           consume, so it invokes our sentinel and returns.

3.2) The outer status_notify call (which we're still in) is now done
     flushing pending process output, so it *also* invokes our sentinel.

It might be that the answer is to just remove the delete-process call
from the filter.  It seems completely redundant (and racy) with Emacs'
automatic SIGHUP'ing.

> * Finally, something happened that caused *12,000* of the following lines
>   to be sent to the *Notmuch errors* buffer:
>
> A Xapian exception occurred performing query: The revision being read has been discarded - you should call Xapian::Database::reopen() and retry the operation
>
>   Again, this was related to killing a search buffer that was still
>   being filled. I'm pretty sure the database was not modified during
>   this process.

I have no insight on this one.  My best guess is that this has nothing
to do with this change except that this change makes these warnings
visible rather than burying them somewhere down in the search results
buffer.

> Let me know if I can help provide any more info.
>
> jamie.

Re: [PATCH v3 0/6] Make Emacs search use sexp format

[Jameson Graef Rollins]

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

On Mon, Jun 03 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> * Killing a search buffer that is still in the process of being filled
>>   causes errors to be thrown.  I'm seeing both of the following
>>   intermittently:
>>
>> [Sun Jun  2 08:26:40 2013]
>> notmuch exited with status killed
>> command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
>> exit signal: killed
>>
>> [Sun Jun  2 08:32:26 2013]
>> notmuch exited with status hangup
>> command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
>> exit signal: hangup
>>
>>   This is somewhat understandable, as the notmuch binary exits with an
>>   error if it hasn't finished dumping the output, but given how common
>>   this particular scenario is I think we should try to avoid throwing
>>   errors in this circumstance.  I wonder if we shouldn't just modify the
>>   binary to not return non-zero if it was manually killed while
>>   processing the output, or at least special-case the particular error
>>   caused by manually killing the search.
>
> Your assessment is correct, of course.  The right place to fix this is
> in Emacs, not the CLI (the CLI *can't* do anything about this, since it
> gets killed by a signal).  Probably we should do something different in
> the sentinel if the search process's buffer is no longer live.  Clearly
> we should suppress the status error for the signal, but I think we still
> should report anything that appeared in err-file because it may be
> relevant to why the user killed the buffer (e.g., maybe a notmuch
> wrapper was blocked on something).

That seems like a reasonable approach to me, to suppress the error but
continue to report in *Notmuch errors* buffer.

>> * The next thing I'm seeing is this:
>>
>> Opening input file: no such file or directory, /home/jrollins/tmp/nmerr5390CAY
>>
>>   I'm not exactly sure what causes this error, but it looks to me like
>>   the temporary error file was removed before we were finished with it.
>
> This one's pretty awesome (and I think is a bug in Emacs).  At a high
> level, the sentinel is getting run twice and since the first call
> deletes the error file, the second call fails.  At a low level, what
> causes this is fascinating.
>
> 1) You kill the search buffer.  This invokes kill_buffer_processes,
>    which sends a SIGHUP to notmuch, but doesn't do anything else.
>    Meanwhile, the notmuch search process has printed some more output,
>    but Emacs hasn't consumed it yet (this is critical).
>
> 2) Emacs gets a SIGCHLD from the dying notmuch process, which invokes
>    handle_child_signal, which sets the new process status, but can't do
>    anything else because it's a signal handler.
>
> 3) Emacs returns to its idle loop, which calls status_notify, which sees
>    that the notmuch process has a new status.  This is where things get
>    interesting.
>
> 3.1) Emacs guarantees that it will run process filters on any unconsumed
>      output before running the process sentinel, so status_notify calls
>      read_process_output, which consumes the final output and calls
>      notmuch-search-process-filter.
>
> 3.1.1) notmuch-search-process-filter contains code to check if the
>        search buffer is still alive and, since it's not, it calls
>        delete-process.
>
> 3.1.1.1) delete-process correctly sees that the process is already dead
>          and doesn't try to send another signal, *but* it still modifies
>          the status to "killed".  To deal with the new status, it calls
>          status_notify.  Dun dun dun.  We've seen this function before.
>
> 3.1.1.1.1) The *recursive* status_notify invocation sees that the
>            process has a new status and doesn't have any more output to
>            consume, so it invokes our sentinel and returns.
>
> 3.2) The outer status_notify call (which we're still in) is now done
>      flushing pending process output, so it *also* invokes our sentinel.
>
> It might be that the answer is to just remove the delete-process call
> from the filter.  It seems completely redundant (and racy) with Emacs'
> automatic SIGHUP'ing.

Wow, awesome detective work.  As mentioned on IRC, this suggestion of
Austin's does seem to fix the problem:

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 5a8c957..975ef2b 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -817,7 +817,7 @@ non-authors is found, assume that all of the authors match."
        (inhibit-read-only t)
        done)
     (if (not (buffer-live-p results-buf))
-       (delete-process proc)
+       t
       (with-current-buffer parse-buf
        ;; Insert new data
        (save-excursion


I'm not sure if this is the ultimate solution, but it does cause the
missing tmp file errors to go away.

>> * Finally, something happened that caused *12,000* of the following lines
>>   to be sent to the *Notmuch errors* buffer:
>>
>> A Xapian exception occurred performing query: The revision being read has been discarded - you should call Xapian::Database::reopen() and retry the operation
>>
>>   Again, this was related to killing a search buffer that was still
>>   being filled. I'm pretty sure the database was not modified during
>>   this process.
>
> I have no insight on this one.  My best guess is that this has nothing
> to do with this change except that this change makes these warnings
> visible rather than burying them somewhere down in the search results
> buffer.

Yeah, I suspected as much as well.

jamie.

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