From: Austin Clements <amdragon@MIT.EDU>
To: notmuch@notmuchmail.org
Subject: [PATCH v2 0/5] Make Emacs search use sexp format
Date: Thu, 30 May 2013 13:13:31 -0400
Message-Id: <1369934016-22308-1-git-send-email-amdragon@mit.edu>
Cc: tomi.ollila@iki.fi
Precedence: list

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)