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)