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

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

[Austin Clements]

This series implements an incremental S-expression parser and switches
search over to it.  To simplify things, it also implements better
handing of stderr for asynchronous processes so we don't have to
handle errors embedded in the S-expression stream.

[PATCH 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 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 |   73 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 59b1ce3..a543471 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,68 @@ 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)
+	 (proc (apply #'start-process name buffer
+		      "sh" "-c"
+		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
+		      notmuch-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-unless-debug 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
+       ;; Don't signal an error from a sentinel
+       (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

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

[Mark Walters]

Hi

I am working my way through this set. 

On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> 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 |   73 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 59b1ce3..a543471 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,

I think I would prefer that this patch is split here. The stuff above is
"obviously correct" and could go in independently of the rest.
 
> @@ -469,6 +472,68 @@ 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)
> +	 (proc (apply #'start-process name buffer
> +		      "sh" "-c"
> +		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
> +		      notmuch-command err-file args)))

I would like some other people to look at this carefully as I won't spot
errors in quoting or shell side effects or whether PATH is the same as
emacs would use etc.

> +    (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-unless-debug 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
> +       ;; Don't signal an error from a sentinel

I found this comment confusing as I was thinking of signal in the
non-technical sense of "tell the user".

Best wishes

Mark
> +       (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

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

[Tomi Ollila]

On Tue, May 21 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> Hi
>
> I am working my way through this set. 
>
> On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> 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 |   73 +++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 69 insertions(+), 4 deletions(-)
>>
>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> index 59b1ce3..a543471 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,
>
> I think I would prefer that this patch is split here. The stuff above is
> "obviously correct" and could go in independently of the rest.
>  
>> @@ -469,6 +472,68 @@ 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)
>> +	 (proc (apply #'start-process name buffer
>> +		      "sh" "-c"
>> +		      "ERR=\"$1\"; shift; exec \"$0\" \"$@\" 2>\"$ERR\""
>> +		      notmuch-command err-file args)))
>
> I would like some other people to look at this carefully as I won't spot
> errors in quoting or shell side effects or whether PATH is the same as
> emacs would use etc.

The variable expansion for sell works as desired, but PATH question is
interesting:

I tried:

setq exec-path (cons "/bii" exec-path))
(shell-command "echo $PATH")

and "/bii" was not in the head of the output the shell-command executed
(which is interesting... found this text:

    The value of “PATH” is used by emacs when you are running a shell in
    emacs, similar to when you are using a shell in a terminal.

    The exec-path is used by emacs itself to find programs it needs for its
    features, such as spell checking, file compression, compiling, grep,
    diff, etc.

 (in http://ergoemacs.org/emacs/emacs_env_var_paths.htm )

and then:

  Generally, you should not modify exec-path directly. Instead, ensure that
  your PATH environment variable is set appropriately before starting
  Emacs. Trying to modify exec-path independently of PATH can lead to
  confusing results. 

 (in http://www.gnu.org/software/emacs/manual/html_node/elisp/Subprocess-Creation.html))

... the --stderr option in the RFC patch I just sent 
( id:1369161750-12342-1-git-send-email-tomi.ollila@iki.fi ) could solve
quite a few problems here :D

Tomi

[PATCH 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

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

[Mark Walters]

I have only very briefly looked at this: it seems to not quite apply to
master (one fix up see below)

Also, as far as I can see condition-case-unless-debug (used in patch
2/5) is emacs 24 only.

Best wishes

Mark

On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> 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))

This line is 
		       (message "%s" (second err))
in master.


> -		       (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

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

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>> -		      (error
>> -		       (message "%s" (error-message-string err))
>
> This line is 
> 		       (message "%s" (second err))
> in master.
>

This is a seperate patch sent to the list recently.

d

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

[Austin Clements]

Quoth Mark Walters on May 18 at  8:14 am:
> 
> I have only very briefly looked at this: it seems to not quite apply to
> master (one fix up see below)
> 
> Also, as far as I can see condition-case-unless-debug (used in patch
> 2/5) is emacs 24 only.

Indeed.  I'll just use condition-case in v2 (unless we want to
introduce a compatibility macro).

I really wish the Emacs documentation included what version functions
were introduced in.  I couldn't even *find* a copy of the non-latest
documentation; I wound up downloading a tarball of Emacs 23 and
building the documentation myself.

> Best wishes
> 
> Mark
> 
> On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > 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))
> 
> This line is 
> 		       (message "%s" (second err))
> in master.

Sorry, I thought these patches were independent from the ones I'd sent
earlier, but obviously not.  It should apply now that David has
applied the other patch.

> > -		       (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

[PATCH 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 |  212 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 213 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..1b7cf64
--- /dev/null
+++ b/emacs/notmuch-parser.el
@@ -0,0 +1,212 @@
+;; 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 (buffer)
+  "Return a streaming S-expression parser that reads from BUFFER.
+
+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 BUFFER's point.  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
+	  nil
+	  ;; 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))
+
+(defun notmuch-sexp-read (sp)
+  "Consume and return the value at point in SP's 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."
+
+  (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.
+	       (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."
+
+  (with-current-buffer (notmuch-sexp--buffer sp)
+    (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."
+
+  (with-current-buffer (notmuch-sexp--buffer sp)
+    (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 consume 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 (current-buffer)))
+    (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

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

[Mark Walters]

Hi

This patch looks good to me. Some minor comments below.

Best wishes

Mark


On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> 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 |  212 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 213 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..1b7cf64
> --- /dev/null
> +++ b/emacs/notmuch-parser.el
> @@ -0,0 +1,212 @@
> +;; 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 (buffer)
> +  "Return a streaming S-expression parser that reads from BUFFER.
> +
> +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 BUFFER's point.  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
> +	  nil
> +	  ;; 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))

Why the double hyphen --? Is it a name-space or some convention?


> +(defun notmuch-sexp-read (sp)
> +  "Consume and return the value at point in SP's 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."
> +
> +  (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.
> +	       (read (current-buffer))

Why is good for read to signal the error rather than us doing it?

> +	     ;; 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."
> +
> +  (with-current-buffer (notmuch-sexp--buffer sp)
> +    (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."
> +
> +  (with-current-buffer (notmuch-sexp--buffer sp)
> +    (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 consume an S-expression list from the current

consumes

> +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 (current-buffer)))
> +    (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

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

[Austin Clements]

On Tue, 21 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Hi
>
> This patch looks good to me. Some minor comments below.

Some minor replies below.

In building some other code on top of this, I found an interesting (but
easy to fix) interface bug.  Currently, the interface is designed as if
it doesn't matter what buffer these functions are called from, however,
because they move point and expect this point motion to persist, it's
actually not safe to call this interface unless the caller is in the
right buffer anyway.  For example, if the buffer is selected in a
window, the with-current-buffer in the parser functions will actually
move a *temporary* point, meaning that the only way the caller can
discover the new point is to first select the buffer for itself.  I can
think of two solutions: 1) maintain our own mark for the parser's
current position or 2) tweak the doc strings and code so that it reads
from the current buffer.  1 keeps the interface the way it's currently
documented, but complicates the parser implementation and interface and
doesn't simplify the caller.  2 simplifies the parser and it turns out
all callers already satisfy the requirement.

> Best wishes
>
> Mark
>
>
> On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> 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 |  212 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 213 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..1b7cf64
>> --- /dev/null
>> +++ b/emacs/notmuch-parser.el
>> @@ -0,0 +1,212 @@
>> +;; 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 (buffer)
>> +  "Return a streaming S-expression parser that reads from BUFFER.
>> +
>> +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 BUFFER's point.  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
>> +	  nil
>> +	  ;; 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))
>
> Why the double hyphen --? Is it a name-space or some convention?

More specifically, this seems to be the most common Elisp convention for
indicating private symbols.

>> +(defun notmuch-sexp-read (sp)
>> +  "Consume and return the value at point in SP's 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."
>> +
>> +  (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.
>> +	       (read (current-buffer))
>
> Why is good for read to signal the error rather than us doing it?

This ensures the syntax error handling and signal behavior of
notmuch-sexp-read is identical in every way to a regular read call.
Maybe the comment should read "Let read signal the error like we do in
all other code paths."?

>> +	     ;; 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."
>> +
>> +  (with-current-buffer (notmuch-sexp--buffer sp)
>> +    (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."
>> +
>> +  (with-current-buffer (notmuch-sexp--buffer sp)
>> +    (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 consume an S-expression list from the current
>
> consumes

Oops, yes.

>> +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 (current-buffer)))
>> +    (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

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

[Mark Walters]

Hi

On Wed, 22 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> On Tue, 21 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>> Hi
>>
>> This patch looks good to me. Some minor comments below.
>
> Some minor replies below.
>
> In building some other code on top of this, I found an interesting (but
> easy to fix) interface bug.  Currently, the interface is designed as if
> it doesn't matter what buffer these functions are called from, however,
> because they move point and expect this point motion to persist, it's
> actually not safe to call this interface unless the caller is in the
> right buffer anyway.  For example, if the buffer is selected in a
> window, the with-current-buffer in the parser functions will actually
> move a *temporary* point, meaning that the only way the caller can
> discover the new point is to first select the buffer for itself.  I can
> think of two solutions: 1) maintain our own mark for the parser's
> current position or 2) tweak the doc strings and code so that it reads
> from the current buffer.  1 keeps the interface the way it's currently
> documented, but complicates the parser implementation and interface and
> doesn't simplify the caller.  2 simplifies the parser and it turns out
> all callers already satisfy the requirement.

I am confused by this: the docs strings for json/sexp-parse-partial-list
both say something like "Parse a partial JSON list from current buffer"?
Or do you mean the with-current-buffer in notmuch-search-process-filter?

>> On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>>> 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 |  212 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 213 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..1b7cf64
>>> --- /dev/null
>>> +++ b/emacs/notmuch-parser.el
>>> @@ -0,0 +1,212 @@
>>> +;; 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 (buffer)
>>> +  "Return a streaming S-expression parser that reads from BUFFER.
>>> +
>>> +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 BUFFER's point.  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
>>> +	  nil
>>> +	  ;; 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))
>>
>> Why the double hyphen --? Is it a name-space or some convention?
>
> More specifically, this seems to be the most common Elisp convention for
> indicating private symbols.

Ok. If we are keeping the json parser it might be worth making it follow
the same convention but as it is purely internal it's probably not worth
bothering.

>
>>> +(defun notmuch-sexp-read (sp)
>>> +  "Consume and return the value at point in SP's 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."
>>> +
>>> +  (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.
>>> +	       (read (current-buffer))
>>
>> Why is good for read to signal the error rather than us doing it?
>
> This ensures the syntax error handling and signal behavior of
> notmuch-sexp-read is identical in every way to a regular read call.
> Maybe the comment should read "Let read signal the error like we do in
> all other code paths."?

Yes that would be good: or perhaps "like it does in all other code
paths".

Best wishes 

Mark

>
>>> +	     ;; 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."
>>> +
>>> +  (with-current-buffer (notmuch-sexp--buffer sp)
>>> +    (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."
>>> +
>>> +  (with-current-buffer (notmuch-sexp--buffer sp)
>>> +    (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 consume an S-expression list from the current
>>
>> consumes
>
> Oops, yes.
>
>>> +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 (current-buffer)))
>>> +    (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

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

[Austin Clements]

Quoth Mark Walters on May 25 at  9:59 am:
> 
> Hi
> 
> On Wed, 22 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > On Tue, 21 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> >> Hi
> >>
> >> This patch looks good to me. Some minor comments below.
> >
> > Some minor replies below.
> >
> > In building some other code on top of this, I found an interesting (but
> > easy to fix) interface bug.  Currently, the interface is designed as if
> > it doesn't matter what buffer these functions are called from, however,
> > because they move point and expect this point motion to persist, it's
> > actually not safe to call this interface unless the caller is in the
> > right buffer anyway.  For example, if the buffer is selected in a
> > window, the with-current-buffer in the parser functions will actually
> > move a *temporary* point, meaning that the only way the caller can
> > discover the new point is to first select the buffer for itself.  I can
> > think of two solutions: 1) maintain our own mark for the parser's
> > current position or 2) tweak the doc strings and code so that it reads
> > from the current buffer.  1 keeps the interface the way it's currently
> > documented, but complicates the parser implementation and interface and
> > doesn't simplify the caller.  2 simplifies the parser and it turns out
> > all callers already satisfy the requirement.
> 
> I am confused by this: the docs strings for json/sexp-parse-partial-list
> both say something like "Parse a partial JSON list from current buffer"?
> Or do you mean the with-current-buffer in notmuch-search-process-filter?

I was referring to the lower level parser, which effectively has the
same requirement but isn't documented to and has code that pointlessly
tries to track the parsing buffer (I consider
json/sexp-parse-partial-list to be a helper).  In fact, one reason the
lower level parser didn't choke is because right now we only use it
through json/sexp-parse-partial-list, which requires that it be called
from the right buffer.

> >> On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> >>> 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 |  212 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 213 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..1b7cf64
> >>> --- /dev/null
> >>> +++ b/emacs/notmuch-parser.el
> >>> @@ -0,0 +1,212 @@
> >>> +;; 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 (buffer)
> >>> +  "Return a streaming S-expression parser that reads from BUFFER.
> >>> +
> >>> +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 BUFFER's point.  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
> >>> +	  nil
> >>> +	  ;; 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))
> >>
> >> Why the double hyphen --? Is it a name-space or some convention?
> >
> > More specifically, this seems to be the most common Elisp convention for
> > indicating private symbols.
> 
> Ok. If we are keeping the json parser it might be worth making it follow
> the same convention but as it is purely internal it's probably not worth
> bothering.

Yeah, it is purely internal.  Also, I was planning to remove the JSON
parser (or maybe move it somewhere else?  I feel bad deleting that
much perfectly functional code, though of course git will keep it in
perpetuity).

> >>> +(defun notmuch-sexp-read (sp)
> >>> +  "Consume and return the value at point in SP's 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."
> >>> +
> >>> +  (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.
> >>> +	       (read (current-buffer))
> >>
> >> Why is good for read to signal the error rather than us doing it?
> >
> > This ensures the syntax error handling and signal behavior of
> > notmuch-sexp-read is identical in every way to a regular read call.
> > Maybe the comment should read "Let read signal the error like we do in
> > all other code paths."?
> 
> Yes that would be good: or perhaps "like it does in all other code
> paths".

Sure.

> Best wishes 
> 
> Mark
> 
> >
> >>> +	     ;; 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."
> >>> +
> >>> +  (with-current-buffer (notmuch-sexp--buffer sp)
> >>> +    (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."
> >>> +
> >>> +  (with-current-buffer (notmuch-sexp--buffer sp)
> >>> +    (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 consume an S-expression list from the current
> >>
> >> consumes
> >
> > Oops, yes.
> >
> >>> +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 (current-buffer)))
> >>> +    (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:

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

[Mark Walters]

Austin Clements <amdragon@MIT.EDU> writes:

> Quoth Mark Walters on May 25 at  9:59 am:
>> 
>> Hi
>> 
>> On Wed, 22 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> > On Tue, 21 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>> >> Hi
>> >>
>> >> This patch looks good to me. Some minor comments below.
>> >
>> > Some minor replies below.
>> >
>> > In building some other code on top of this, I found an interesting (but
>> > easy to fix) interface bug.  Currently, the interface is designed as if
>> > it doesn't matter what buffer these functions are called from, however,
>> > because they move point and expect this point motion to persist, it's
>> > actually not safe to call this interface unless the caller is in the
>> > right buffer anyway.  For example, if the buffer is selected in a
>> > window, the with-current-buffer in the parser functions will actually
>> > move a *temporary* point, meaning that the only way the caller can
>> > discover the new point is to first select the buffer for itself.  I can
>> > think of two solutions: 1) maintain our own mark for the parser's
>> > current position or 2) tweak the doc strings and code so that it reads
>> > from the current buffer.  1 keeps the interface the way it's currently
>> > documented, but complicates the parser implementation and interface and
>> > doesn't simplify the caller.  2 simplifies the parser and it turns out
>> > all callers already satisfy the requirement.
>> 
>> I am confused by this: the docs strings for json/sexp-parse-partial-list
>> both say something like "Parse a partial JSON list from current buffer"?
>> Or do you mean the with-current-buffer in notmuch-search-process-filter?
>
> I was referring to the lower level parser, which effectively has the
> same requirement but isn't documented to and has code that pointlessly
> tries to track the parsing buffer (I consider
> json/sexp-parse-partial-list to be a helper).  In fact, one reason the
> lower level parser didn't choke is because right now we only use it
> through json/sexp-parse-partial-list, which requires that it be called
> from the right buffer.

Right I think I see. I am definitely happy with insisting on being
called from the correct buffer in the low level code.

>> >> On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> >>> 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 |  212 +++++++++++++++++++++++++++++++++++++++++++++++
>> >>>  2 files changed, 213 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..1b7cf64
>> >>> --- /dev/null
>> >>> +++ b/emacs/notmuch-parser.el
>> >>> @@ -0,0 +1,212 @@
>> >>> +;; 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 (buffer)
>> >>> +  "Return a streaming S-expression parser that reads from BUFFER.
>> >>> +
>> >>> +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 BUFFER's point.  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
>> >>> +	  nil
>> >>> +	  ;; 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))
>> >>
>> >> Why the double hyphen --? Is it a name-space or some convention?
>> >
>> > More specifically, this seems to be the most common Elisp convention for
>> > indicating private symbols.
>> 
>> Ok. If we are keeping the json parser it might be worth making it follow
>> the same convention but as it is purely internal it's probably not worth
>> bothering.
>
> Yeah, it is purely internal.  Also, I was planning to remove the JSON
> parser (or maybe move it somewhere else?  I feel bad deleting that
> much perfectly functional code, though of course git will keep it in
> perpetuity).

Yes it does seem a shame to lose it. It would be nice to find a better
home for it then hiding in a git tree.

Best wishes

Mark


>> >>> +(defun notmuch-sexp-read (sp)
>> >>> +  "Consume and return the value at point in SP's 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."
>> >>> +
>> >>> +  (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.
>> >>> +	       (read (current-buffer))
>> >>
>> >> Why is good for read to signal the error rather than us doing it?
>> >
>> > This ensures the syntax error handling and signal behavior of
>> > notmuch-sexp-read is identical in every way to a regular read call.
>> > Maybe the comment should read "Let read signal the error like we do in
>> > all other code paths."?
>> 
>> Yes that would be good: or perhaps "like it does in all other code
>> paths".
>
> Sure.
>
>> Best wishes 
>> 
>> Mark
>> 
>> >
>> >>> +	     ;; 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."
>> >>> +
>> >>> +  (with-current-buffer (notmuch-sexp--buffer sp)
>> >>> +    (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."
>> >>> +
>> >>> +  (with-current-buffer (notmuch-sexp--buffer sp)
>> >>> +    (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 consume an S-expression list from the current
>> >>
>> >> consumes
>> >
>> > Oops, yes.
>> >
>> >>> +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 (current-buffer)))
>> >>> +    (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:

[PATCH 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 5/5] emacs: Use streaming S-expr parser for search

[Mark Walters]

This patch and patch 3/5 look good to me. Do you intend to take out the
async json parser immediately after this goes in? If not then I think it
would be good to tweak the arguments/error handling in the json parser
so that it matches that in this sexp parser.

Best wishes

Mark

On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> 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 0/5] Make Emacs search use sexp format

[Mark Walters]

This whole series looks good to me (modulo the minor comments I have
made). The tests all pass. Additionally it was very simple to modify
notmuch-pick to use the new parser and that also seemed to work.

Incidentally it seems that notmuch-show still uses the JSON format so we
may want to switch that over too.

Best wishes

Mark


On Sat, 18 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> This series implements an incremental S-expression parser and switches
> search over to it.  To simplify things, it also implements better
> handing of stderr for asynchronous processes so we don't have to
> handle errors embedded in the S-expression stream.