From: Tomi Ollila <tomi.ollila@iki.fi>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting
In-Reply-To: <1357249669-9706-1-git-send-email-amdragon@mit.edu>
References: <1357174222-25132-1-git-send-email-amdragon@mit.edu>
	<1357249669-9706-1-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.14+236~ge6d0259 (http://notmuchmail.org) Emacs/24.2.1
	(x86_64-unknown-linux-gnu)
Date: Sat, 05 Jan 2013 23:33:08 +0200
Message-ID: <m238yfjox7.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Thu, Jan 03 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> We recently switched to popping up a buffer to report CLI errors, but
> this was too intrusive, especially for transient errors and especially
> since we made fewer things ignore errors.  This patch changes this to
> display a basic error message in the minibuffer (using Emacs' usual
> error handling path) and, if there are additional details, to log
> these to a separate error buffer and reference the error buffer from
> the minibuffer message.  This is more in line with how Emacs typically
> handles errors, but makes the details available to the user without
> flooding them with the details.
>
> Given this split, we pare down the basic message and make it more
> user-friendly, and also make the verbose message even more detailed
> (and more debugging-oriented).
> ---

LGTM.

Mark's suggestion could go to a separate patch -- this one
doesn't (seem to?) make things worse compared what it is before
applying. 

If things are currently irritatively borken due to notmuch emitting
more errors then Someone(tm) should make a proper patch following
Mark's suggestion.

Tomi

>
> This version fixes two hard-coded paths in the tests.
>
>  emacs/notmuch-lib.el |   92 ++++++++++++++++++++++++++++----------------------
>  emacs/notmuch.el     |    9 +++--
>  test/emacs           |   19 ++++++++---
>  test/emacs-show      |   26 ++++++++++----
>  4 files changed, 90 insertions(+), 56 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 77a591d..d78bcf8 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -316,23 +316,28 @@ string), a property list of face attributes, or a list of these."
>  	(put-text-property pos next 'face (cons face cur))
>  	(setq pos next)))))
>  
> -(defun notmuch-pop-up-error (msg)
> -  "Pop up an error buffer displaying MSG.
> -
> -This will accumulate error messages in the errors buffer until
> -the user dismisses it."
> -
> -  (let ((buf (get-buffer-create "*Notmuch errors*")))
> -    (with-current-buffer buf
> -      (view-mode-enter nil #'kill-buffer)
> -      (let ((inhibit-read-only t))
> -	(goto-char (point-max))
> -	(unless (bobp)
> -	  (insert "\n"))
> -	(insert msg)
> +(defun notmuch-logged-error (msg &optional extra)
> +  "Log MSG and EXTRA to *Notmuch errors* and signal MSG.
> +
> +This logs MSG and EXTRA to the *Notmuch errors* buffer and
> +signals MSG as an error.  If EXTRA is non-nil, text referring the
> +user to the *Notmuch errors* buffer will be appended to the
> +signaled error.  This function does not return."
> +
> +  (with-current-buffer (get-buffer-create "*Notmuch errors*")
> +    (goto-char (point-max))
> +    (unless (bobp)
> +      (newline))
> +    (save-excursion
> +      (insert "[" (current-time-string) "]\n" msg)
> +      (unless (bolp)
> +	(newline))
> +      (when extra
> +	(insert extra)
>  	(unless (bolp)
> -	  (insert "\n"))))
> -    (pop-to-buffer buf)))
> +	  (newline)))))
> +  (error "%s" (concat msg (when extra
> +			    " (see *Notmuch errors* for more details)"))))
>  
>  (defun notmuch-check-async-exit-status (proc msg)
>    "If PROC exited abnormally, pop up an error buffer and signal an error.
> @@ -363,35 +368,40 @@ contents of ERR-FILE will be included in the error message."
>    (cond
>     ((eq exit-status 0) t)
>     ((eq exit-status 20)
> -    (notmuch-pop-up-error "Error: Version mismatch.
> +    (notmuch-logged-error "notmuch CLI version mismatch
>  Emacs requested an older output format than supported by the notmuch CLI.
> -You may need to restart Emacs or upgrade your notmuch Emacs package.")
> -    (error "notmuch CLI version mismatch"))
> +You may need to restart Emacs or upgrade your notmuch Emacs package."))
>     ((eq exit-status 21)
> -    (notmuch-pop-up-error "Error: Version mismatch.
> +    (notmuch-logged-error "notmuch CLI version mismatch
>  Emacs requested a newer output format than supported by the notmuch CLI.
> -You may need to restart Emacs or upgrade your notmuch package.")
> -    (error "notmuch CLI version mismatch"))
> +You may need to restart Emacs or upgrade your notmuch package."))
>     (t
> -    (notmuch-pop-up-error
> -     (concat
> -      (format "Error invoking notmuch.  %s exited with %s%s.\n"
> -	      (mapconcat #'identity command " ")
> -	      ;; Signal strings look like "Terminated", hence the
> -	      ;; colon.
> -	      (if (integerp exit-status) "status " "signal: ")
> -	      exit-status)
> -      (when err-file
> -	(concat "Error:\n"
> -		(with-temp-buffer
> -		  (insert-file-contents err-file)
> -		  (if (eobp)
> -		      "(no error output)\n"
> -		    (buffer-string)))))
> -      (when (and output (not (equal output "")))
> -	(format "Output:\n%s" output))))
> -    ;; Mimic `process-lines'
> -    (error "%s exited with status %s" (car command) exit-status))))
> +    (let* ((err (when err-file
> +		  (with-temp-buffer
> +		    (insert-file-contents err-file)
> +		    (unless (eobp)
> +		      (buffer-string)))))
> +	   (extra
> +	    (concat
> +	     "command: " (mapconcat #'shell-quote-argument command " ") "\n"
> +	     (if (integerp exit-status)
> +		 (format "exit status: %s\n" exit-status)
> +	       (format "exit signal: %s\n" exit-status))
> +	     (when err
> +	       (concat "stderr:\n" err))
> +	     (when output
> +	       (concat "stdout:\n" output)))))
> +	(if err
> +	    ;; We have an error message straight from the CLI.
> +	    (notmuch-logged-error
> +	     (replace-regexp-in-string "\\s $" "" err) extra)
> +	  ;; We only have combined output from the CLI; don't inundate
> +	  ;; the user with it.  Mimic `process-lines'.
> +	  (notmuch-logged-error (format "%s exited with status %s"
> +					(car command) exit-status)
> +				extra))
> +	;; `notmuch-logged-error' does not return.
> +	))))
>  
>  (defun notmuch-call-notmuch-json (&rest args)
>    "Invoke `notmuch-command' with `args' and return the parsed JSON output.
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 63387a2..c98a4fe 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -654,11 +654,14 @@ of the result."
>  		    ;; showing the search buffer
>  		    (when (or (= exit-status 20) (= exit-status 21))
>  		      (kill-buffer))
> -		    (condition-case nil
> +		    (condition-case err
>  			(notmuch-check-async-exit-status proc msg)
>  		      ;; Suppress the error signal since strange
> -		      ;; things happen if a sentinel signals.
> -		      (error (throw 'return nil)))
> +		      ;; things happen if a sentinel signals.  Mimic
> +		      ;; the top-level's handling of error messages.
> +		      (error
> +		       (message "%s" (second err))
> +		       (throw 'return nil)))
>  		    (if (and atbob
>  			     (not (string= notmuch-search-target-thread "found")))
>  			(set 'never-found-target-thread t)))))
> diff --git a/test/emacs b/test/emacs
> index 6b18968..f033bdf 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -862,18 +862,27 @@ exit 1
>  EOF
>  chmod a+x notmuch_fail
>  test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
> +	       (with-current-buffer \"*Messages*\" (erase-buffer))
>  	       (notmuch-search \"tag:inbox\")
>  	       (notmuch-test-wait)
> -	       (test-output)
> +	       (with-current-buffer \"*Messages*\"
> +		  (test-output \"MESSAGES\"))
>  	       (with-current-buffer \"*Notmuch errors*\"
> -		  (test-output \"ERROR\")))"
> -test_expect_equal "$(cat OUTPUT ERROR)" "\
> +		  (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
>  Error: Unexpected output from notmuch search:
>  This is an error
>  End of search results.
> -Error invoking notmuch.  $PWD/notmuch_fail search --format=json --format-version=1 --sort=newest-first tag:inbox exited with status 1."
> -
> +---
> +$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
> +exit status: 1"
>  
>  test_done
> diff --git a/test/emacs-show b/test/emacs-show
> index ebf530b..9f2ccb0 100755
> --- a/test/emacs-show
> +++ b/test/emacs-show
> @@ -172,16 +172,28 @@ exit 1
>  EOF
>  chmod a+x notmuch_fail
>  test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
> -	       (ignore-errors (notmuch-show \"*\"))
> +	       (with-current-buffer \"*Messages*\" (erase-buffer))
> +	       (condition-case err
> +		   (notmuch-show \"*\")
> +		 (error (message \"%s\" (second err))))
>  	       (notmuch-test-wait)
> -	       (test-output)
> +	       (with-current-buffer \"*Messages*\"
> +		  (test-output \"MESSAGES\"))
>  	       (with-current-buffer \"*Notmuch errors*\"
> -		  (test-output \"ERROR\")))"
> -test_expect_equal "$(cat OUTPUT ERROR)" "\
> -Error invoking notmuch.  $PWD/notmuch_fail show --format=json --format-version=1 --exclude=false ' * ' exited with status 1.
> -Error:
> +		  (test-output \"ERROR\"))
> +	       (test-output))"
> +sed -i -e 's/^\[.*\]$/[XXX]/' ERROR
> +test_expect_equal "$(cat OUTPUT; echo ---; cat MESSAGES; echo ---; cat ERROR)" "\
> +---
> +This is an error (see *Notmuch errors* for more details)
> +---
> +[XXX]
>  This is an error
> -Output:
> +command: $PWD/notmuch_fail show --format\\=json --format-version\\=1 --exclude\\=false \\' \\* \\'
> +exit status: 1
> +stderr:
> +This is an error
> +stdout:
>  This is output"
>  
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch