From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 0/5] Make Emacs search use sexp format
In-Reply-To: <87y5aug4gi.fsf@qmul.ac.uk>
References: <1369934016-22308-1-git-send-email-amdragon@mit.edu>
	<87y5aug4gi.fsf@qmul.ac.uk>
User-Agent: Notmuch/0.15.2+133~gf9ff9fd (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Fri, 31 May 2013 19:43:47 -0400
Message-ID: <87wqqehg0c.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: tomi.ollila@iki.fi
Precedence: list

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)