From: Tomi Ollila <tomi.ollila@iki.fi>
To: Mark Walters <markwalters1009@gmail.com>,
	Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/5] emacs: Utilities to manage asynchronous notmuch
	processes
In-Reply-To: <87li782yy6.fsf@qmul.ac.uk>
References: <1368851472-5382-1-git-send-email-amdragon@mit.edu>
	<1368851472-5382-3-git-send-email-amdragon@mit.edu>
	<87li782yy6.fsf@qmul.ac.uk>
User-Agent: Notmuch/0.15.2+115~g12cf6af (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Tue, 21 May 2013 21:44:12 +0300
Message-ID: <m2txlwkwar.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Tue, May 21 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> Hi
>
> I am working my way through this set.=20
>
> 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)"))))
>>=20=20
>> -(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 erro=
r.
>>=20=20
>>  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-comma=
nd proc))
>> +				 nil err-file))))
>>=20=20
>>  (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 err=
or.
>> @@ -448,7 +451,7 @@ You may need to restart Emacs or upgrade your notmuc=
h package."))
>>  	))))
>>=20=20
>>  (defun notmuch-call-notmuch-json (&rest args)
>> -  "Invoke `notmuch-command' with `args' and return the parsed JSON outp=
ut.
>> +  "Invoke `notmuch-command' with ARGS and return the parsed JSON output.
>>=20=20
>>  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.
>=20=20
>> @@ -469,6 +472,68 @@ an error."
>>  	      (json-read)))
>>  	(delete-file err-file)))))
>>=20=20
>> +(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=3D\"$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 =E2=80=9CPATH=E2=80=9D is used by emacs when you are runni=
ng 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.=20

 (in http://www.gnu.org/software/emacs/manual/html_node/elisp/Subprocess-Cr=
eation.html))

... the --stderr option in the RFC patch I just sent=20
( id:1369161750-12342-1-git-send-email-tomi.ollila@iki.fi ) could solve
quite a few problems here :D

Tomi