From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/5] emacs: Use async process helper for search
In-Reply-To: <1368851472-5382-4-git-send-email-amdragon@mit.edu>
References: <1368851472-5382-1-git-send-email-amdragon@mit.edu>
	<1368851472-5382-4-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.14+255~gff3cc55 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 18 May 2013 08:14:37 +0100
Message-ID: <87d2sopxnm.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


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