Date: Sat, 18 May 2013 16:13:57 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH 3/5] emacs: Use async process helper for search
Message-ID: <20130518201356.GC5999@mit.edu>
References: <1368851472-5382-1-git-send-email-amdragon@mit.edu>
	<1368851472-5382-4-git-send-email-amdragon@mit.edu>
	<87d2sopxnm.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87d2sopxnm.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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