[PATCH 0/7] Improve Emacs CLI error handling

[PATCH 0/7] Improve Emacs CLI error handling

[Austin Clements]

This remixes the error handling patches from the schema versioning
series [1] and adds tests for error handling.  Beyond generally
improving our error handling in Emacs, with this series in place, the
schema versioning series will be much simpler.

[1] id:1354416002-3557-1-git-send-email-amdragon@mit.edu

[PATCH 1/7] emacs: Centralize notmuch command error handling

[Austin Clements]

This provides library functions for unified handling of errors from
the notmuch CLI.  Follow-up patches will convert some scattered error
handling to use this and add error handling where we currently ignore
errors.
---
 emacs/notmuch-lib.el |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 9c4ee71..851098f 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -316,6 +316,59 @@ 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))
+	(insert msg)
+	(unless (bolp)
+	  (insert "\n"))
+	(goto-char (point-min))))
+    (pop-to-buffer buf)))
+
+(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.
+
+If EXIT-STATUS is non-zero, pop up a notmuch error buffer
+describing the error and signal an Elisp error.  EXIT-STATUS must
+be a number indicating the exit status code of a process or a
+string describing the signal that terminated the process (such as
+returned by `call-process').  COMMAND must be a list giving the
+command and its arguments.  OUTPUT, if provided, is a string
+giving the output of command.  ERR-FILE, if provided, is the name
+of a file containing the error output of command.  OUTPUT and the
+contents of ERR-FILE will be included in the error message."
+
+  ;; This is implemented as a cond to make it easy to expand.
+  (cond
+   ((eq exit-status 0) t)
+   (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))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
-- 
1.7.10.4

[PATCH 2/7] emacs: Use unified error handling in notmuch-call-notmuch-process

[Austin Clements]

This makes notmuch-call-notmuch-process use the unified CLI error
handling, which basically refines the error handling this function
already did.
---
 emacs/notmuch.el |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f9454d8..e25b54e 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -538,17 +538,10 @@ If BARE is set then do not prefix with \"thread:\""
 
 Output from the process will be presented to the user as an error
 and will also appear in a buffer named \"*Notmuch errors*\"."
-  (let ((error-buffer (get-buffer-create "*Notmuch errors*")))
-    (with-current-buffer error-buffer
-	(erase-buffer))
-    (if (eq (apply 'call-process notmuch-command nil error-buffer nil args) 0)
-	(point)
-      (progn
-	(with-current-buffer error-buffer
-	  (let ((beg (point-min))
-		(end (- (point-max) 1)))
-	    (error (buffer-substring beg end))
-	    ))))))
+  (with-temp-buffer
+    (let ((status (apply #'call-process notmuch-command nil t nil args)))
+      (notmuch-check-exit-status status (cons notmuch-command args)
+				 (buffer-string)))))
 
 (defun notmuch-search-set-tags (tags &optional pos)
   (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
-- 
1.7.10.4

[PATCH 3/7] emacs: Factor out synchronous notmuch JSON invocations

[Austin Clements]

Previously this code was duplicated between show and reply.  This
factors out synchronously invoking notmuch and parsing the output as
JSON.
---
 emacs/notmuch-lib.el   |   14 ++++++++++++++
 emacs/notmuch-mua.el   |    8 +-------
 emacs/notmuch-query.el |   11 ++---------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 851098f..4b71116 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -369,6 +369,20 @@ contents of ERR-FILE will be included in the error message."
     ;; Mimic `process-lines'
     (error "%s exited with status %s" (car command) exit-status))))
 
+(defun notmuch-call-notmuch-json (&rest args)
+  "Invoke `notmuch-command' with `args' and return the parsed JSON output.
+
+The returned output will represent objects using property lists
+and arrays as lists."
+
+  (with-temp-buffer
+    (apply #'call-process notmuch-command nil (list t nil) nil args)
+    (goto-char (point-min))
+    (let ((json-object-type 'plist)
+	  (json-array-type 'list)
+	  (json-false 'nil))
+      (json-read))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 408b49e..ac2d29e 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -158,13 +158,7 @@ list."
     (setq args (append args (list query-string)))
 
     ;; Get the reply object as JSON, and parse it into an elisp object.
-    (with-temp-buffer
-      (apply 'call-process (append (list notmuch-command nil (list t nil) nil) args))
-      (goto-char (point-min))
-      (let ((json-object-type 'plist)
-	    (json-array-type 'list)
-	    (json-false 'nil))
-	(setq reply (json-read))))
+    (setq reply (apply #'notmuch-call-notmuch-json args))
 
     ;; Extract the original message to simplify the following code.
     (setq original (plist-get reply :original))
diff --git a/emacs/notmuch-query.el b/emacs/notmuch-query.el
index d66baea..e7e3520 100644
--- a/emacs/notmuch-query.el
+++ b/emacs/notmuch-query.el
@@ -29,18 +29,11 @@ A thread is a forest or list of trees. A tree is a two element
 list where the first element is a message, and the second element
 is a possibly empty forest of replies.
 "
-  (let  ((args '("show" "--format=json"))
-	 (json-object-type 'plist)
-	 (json-array-type 'list)
-	 (json-false 'nil))
+  (let ((args '("show" "--format=json")))
     (if notmuch-show-process-crypto
 	(setq args (append args '("--decrypt"))))
     (setq args (append args search-terms))
-    (with-temp-buffer
-      (progn
-	(apply 'call-process (append (list notmuch-command nil (list t nil) nil) args))
-	(goto-char (point-min))
-	(json-read)))))
+    (apply #'notmuch-call-notmuch-json args)))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; Mapping functions across collections of messages.
-- 
1.7.10.4

[PATCH 4/7] emacs: Improve error handling for notmuch-call-notmuch-json

[Austin Clements]

This checks for non-zero exit status from JSON CLI calls and pops up
an error buffer with stderr and stdout.  A consequence of this is that
show and reply now handle errors, rather than ignoring them.
---
 emacs/notmuch-lib.el |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 4b71116..9222de8 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -373,15 +373,23 @@ contents of ERR-FILE will be included in the error message."
   "Invoke `notmuch-command' with `args' and return the parsed JSON output.
 
 The returned output will represent objects using property lists
-and arrays as lists."
+and arrays as lists.  If notmuch exits with a non-zero status,
+this will pop up a buffer containing notmuch's output and signal
+an error."
 
   (with-temp-buffer
-    (apply #'call-process notmuch-command nil (list t nil) nil args)
-    (goto-char (point-min))
-    (let ((json-object-type 'plist)
-	  (json-array-type 'list)
-	  (json-false 'nil))
-      (json-read))))
+    (let ((err-file (make-temp-file "nmerr")))
+      (unwind-protect
+	  (let ((status (apply #'call-process
+			       notmuch-command nil (list t err-file) nil args)))
+	    (notmuch-check-exit-status status (cons notmuch-command args)
+				       (buffer-string) err-file)
+	    (goto-char (point-min))
+	    (let ((json-object-type 'plist)
+		  (json-array-type 'list)
+		  (json-false 'nil))
+	      (json-read)))
+	(delete-file err-file)))))
 
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
-- 
1.7.10.4

[PATCH 5/7] test: Test show's handling of subprocess errors

[Austin Clements]

---
 test/emacs-show |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/emacs-show b/test/emacs-show
index b670abf..c67c6a4 100755
--- a/test/emacs-show
+++ b/test/emacs-show
@@ -163,4 +163,26 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 
+test_begin_subtest "Show handles subprocess errors"
+cat > notmuch_fail <<EOF
+#!/bin/sh
+echo This is output
+echo This is an error >&2
+exit 1
+EOF
+chmod a+x notmuch_fail
+test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
+	       (ignore-errors (notmuch-show \"*\"))
+	       (notmuch-test-wait)
+	       (test-output)
+	       (with-current-buffer \"*Notmuch errors*\"
+		  (test-output \"ERROR\")))"
+test_expect_equal "$(cat OUTPUT ERROR)" "\
+Error invoking notmuch.  /tmp/nmtest/tmp.emacs-show/notmuch_fail show --format=json --exclude=false ' * ' exited with status 1.
+Error:
+This is an error
+Output:
+This is output"
+
+
 test_done
-- 
1.7.10.4

[PATCH 6/7] emacs: Use unified error handling in search

[Austin Clements]

This slightly changes the output of an existing test since we now
report non-zero exits with a pop-up buffer instead of at the end of
the search results.
---
 emacs/notmuch-lib.el           |   13 +++++++++++++
 emacs/notmuch.el               |   13 ++++++++-----
 test/emacs-large-search-buffer |    2 +-
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 9222de8..cf61635 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -332,6 +332,19 @@ the user dismisses it."
 	(goto-char (point-min))))
     (pop-to-buffer buf)))
 
+(defun notmuch-check-async-exit-status (proc msg)
+  "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."
+  (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)))))
+
 (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.
 
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index e25b54e..c20de13 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -637,6 +637,7 @@ of the result."
 	(exit-status (process-exit-status proc))
 	(never-found-target-thread nil))
     (when (memq status '(exit signal))
+      (catch 'return
 	(kill-buffer (process-get proc 'parse-buf))
 	(if (buffer-live-p buffer)
 	    (with-current-buffer buffer
@@ -647,17 +648,19 @@ of the result."
 		  (if (eq status 'signal)
 		      (insert "Incomplete search results (search process was killed).\n"))
 		  (when (eq status 'exit)
-		    (insert "End of search results.")
-		    (unless (= exit-status 0)
-		      (insert (format " (process returned %d)" exit-status)))
-		    (insert "\n")
+		    (insert "End of search results.\n")
+		    (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)))
 		    (if (and atbob
 			     (not (string= notmuch-search-target-thread "found")))
 			(set 'never-found-target-thread t)))))
 	      (when (and never-found-target-thread
 		       notmuch-search-target-line)
 		  (goto-char (point-min))
-		  (forward-line (1- notmuch-search-target-line))))))))
+		  (forward-line (1- notmuch-search-target-line)))))))))
 
 (defcustom notmuch-search-line-faces '(("unread" :weight bold)
 				       ("flagged" :foreground "blue"))
diff --git a/test/emacs-large-search-buffer b/test/emacs-large-search-buffer
index 678328d..9dcbef5 100755
--- a/test/emacs-large-search-buffer
+++ b/test/emacs-large-search-buffer
@@ -36,7 +36,7 @@ test_emacs '(notmuch-search "--this-option-does-not-exist")
 cat <<EOF >EXPECTED
 Error: Unexpected output from notmuch search:
 Unrecognized option: --this-option-does-not-exist
-End of search results. (process returned 1)
+End of search results.
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.10.4

[PATCH 7/7] test: Test search's handling of subprocess errors

[Austin Clements]

---
 test/emacs |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/test/emacs b/test/emacs
index 5403930..88b062c 100755
--- a/test/emacs
+++ b/test/emacs
@@ -853,4 +853,27 @@ test_expect_success "Rendering HTML mail with images" \
     'cat OUTPUT && grep -q smiley OUTPUT'
 
 
+test_begin_subtest "Search handles subprocess errors"
+cat > notmuch_fail <<EOF
+#!/bin/sh
+echo This is output
+echo This is an error >&2
+exit 1
+EOF
+chmod a+x notmuch_fail
+test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
+	       (notmuch-search \"tag:inbox\")
+	       (notmuch-test-wait)
+	       (test-output)
+	       (with-current-buffer \"*Notmuch errors*\"
+		  (test-output \"ERROR\")))"
+test_expect_equal "$(cat OUTPUT 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.  /tmp/nmtest/tmp.emacs/notmuch_fail search --format=json --sort=newest-first tag:inbox exited with status 1."
+
+
 test_done
-- 
1.7.10.4

Re: [PATCH 2/7] emacs: Use unified error handling in notmuch-call-notmuch-process

[Mark Walters]

On Sat, 15 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes notmuch-call-notmuch-process use the unified CLI error
> handling, which basically refines the error handling this function
> already did.
> ---
>  emacs/notmuch.el |   15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index f9454d8..e25b54e 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -538,17 +538,10 @@ If BARE is set then do not prefix with \"thread:\""
>  
>  Output from the process will be presented to the user as an error
>  and will also appear in a buffer named \"*Notmuch errors*\"."

This comment looks like it is not true (but wasn't true before
either). I think output will only be presented to the user if notmuch
exits with a non-zero status?

> -  (let ((error-buffer (get-buffer-create "*Notmuch errors*")))
> -    (with-current-buffer error-buffer
> -	(erase-buffer))
> -    (if (eq (apply 'call-process notmuch-command nil error-buffer nil args) 0)
> -	(point)
> -      (progn
> -	(with-current-buffer error-buffer
> -	  (let ((beg (point-min))
> -		(end (- (point-max) 1)))
> -	    (error (buffer-substring beg end))
> -	    ))))))
> +  (with-temp-buffer
> +    (let ((status (apply #'call-process notmuch-command nil t nil args)))
> +      (notmuch-check-exit-status status (cons notmuch-command args)
> +				 (buffer-string)))))
>  

Would it be worth separating stderr and stdout here? (Quite plausibly it
isn't.)

Best wishes

Mark


>  (defun notmuch-search-set-tags (tags &optional pos)
>    (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
> -- 
> 1.7.10.4

Re: [PATCH 6/7] emacs: Use unified error handling in search

[Mark Walters]

On Sat, 15 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This slightly changes the output of an existing test since we now
> report non-zero exits with a pop-up buffer instead of at the end of
> the search results.
> ---
>  emacs/notmuch-lib.el           |   13 +++++++++++++
>  emacs/notmuch.el               |   13 ++++++++-----
>  test/emacs-large-search-buffer |    2 +-
>  3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 9222de8..cf61635 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -332,6 +332,19 @@ the user dismisses it."
>  	(goto-char (point-min))))
>      (pop-to-buffer buf)))
>  
> +(defun notmuch-check-async-exit-status (proc msg)
> +  "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."
> +  (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)))))
> +
>  (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.
>  
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index e25b54e..c20de13 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -637,6 +637,7 @@ of the result."
>  	(exit-status (process-exit-status proc))
>  	(never-found-target-thread nil))
>      (when (memq status '(exit signal))
> +      (catch 'return
>  	(kill-buffer (process-get proc 'parse-buf))
>  	(if (buffer-live-p buffer)
>  	    (with-current-buffer buffer
> @@ -647,17 +648,19 @@ of the result."
>  		  (if (eq status 'signal)
>  		      (insert "Incomplete search results (search process was killed).\n"))
>  		  (when (eq status 'exit)
> -		    (insert "End of search results.")
> -		    (unless (= exit-status 0)
> -		      (insert (format " (process returned %d)" exit-status)))
> -		    (insert "\n")
> +		    (insert "End of search results.\n")
> +		    (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)))

A total triviality (mostly just to check that I am understanding these
parts): is the "err" after the condition-case used/needed?

Best wishes

Mark
>  		    (if (and atbob
>  			     (not (string= notmuch-search-target-thread "found")))
>  			(set 'never-found-target-thread t)))))
>  	      (when (and never-found-target-thread
>  		       notmuch-search-target-line)
>  		  (goto-char (point-min))
> -		  (forward-line (1- notmuch-search-target-line))))))))
> +		  (forward-line (1- notmuch-search-target-line)))))))))
>  
>  (defcustom notmuch-search-line-faces '(("unread" :weight bold)
>  				       ("flagged" :foreground "blue"))
> diff --git a/test/emacs-large-search-buffer b/test/emacs-large-search-buffer
> index 678328d..9dcbef5 100755
> --- a/test/emacs-large-search-buffer
> +++ b/test/emacs-large-search-buffer
> @@ -36,7 +36,7 @@ test_emacs '(notmuch-search "--this-option-does-not-exist")
>  cat <<EOF >EXPECTED
>  Error: Unexpected output from notmuch search:
>  Unrecognized option: --this-option-does-not-exist
> -End of search results. (process returned 1)
> +End of search results.
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> -- 
> 1.7.10.4

Re: [PATCH 7/7] test: Test search's handling of subprocess errors

[Mark Walters]

On Sat, 15 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> ---
>  test/emacs |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/test/emacs b/test/emacs
> index 5403930..88b062c 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -853,4 +853,27 @@ test_expect_success "Rendering HTML mail with images" \
>      'cat OUTPUT && grep -q smiley OUTPUT'
>  
>  
> +test_begin_subtest "Search handles subprocess errors"
> +cat > notmuch_fail <<EOF
> +#!/bin/sh
> +echo This is output
> +echo This is an error >&2
> +exit 1
> +EOF
> +chmod a+x notmuch_fail
> +test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))
> +	       (notmuch-search \"tag:inbox\")
> +	       (notmuch-test-wait)
> +	       (test-output)
> +	       (with-current-buffer \"*Notmuch errors*\"
> +		  (test-output \"ERROR\")))"
> +test_expect_equal "$(cat OUTPUT 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.  /tmp/nmtest/tmp.emacs/notmuch_fail search --format=json --sort=newest-first tag:inbox exited with status 1."

The filename above is hardcoded so fails for me, and the same for patch
5/7 (modulo this the tests would both pass)

Best wishes 

Mark


 

> +
> +
>  test_done
> -- 
> 1.7.10.4

Re: [PATCH 1/7] emacs: Centralize notmuch command error handling

[Tomi Ollila]

On Sat, Dec 15 2012, Austin Clements <amdragon@MIT.EDU> wrote:

> This provides library functions for unified handling of errors from
> the notmuch CLI.  Follow-up patches will convert some scattered error
> handling to use this and add error handling where we currently ignore
> errors.
> ---

The series looks pretty good, some hardcoded patchs Mark noticed
and it looks to me (after viewing id:8738z7hd6x.fsf@qmul.ac.uk
that err -> nil could be done.

Would (goto-char (point-min)) be needed before (insert msg) in
this patch -- what If user has moved cursor while viewing old
error messages but not pressed q (dismissed) on the buffer. 
Also, what about (unless (eobp) (insert "\n")) to add empty
line between error messages ?

Tomi

>  emacs/notmuch-lib.el |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 9c4ee71..851098f 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -316,6 +316,59 @@ 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))
> +	(insert msg)
> +	(unless (bolp)
> +	  (insert "\n"))
> +	(goto-char (point-min))))
> +    (pop-to-buffer buf)))
> +
> +(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.
> +
> +If EXIT-STATUS is non-zero, pop up a notmuch error buffer
> +describing the error and signal an Elisp error.  EXIT-STATUS must
> +be a number indicating the exit status code of a process or a
> +string describing the signal that terminated the process (such as
> +returned by `call-process').  COMMAND must be a list giving the
> +command and its arguments.  OUTPUT, if provided, is a string
> +giving the output of command.  ERR-FILE, if provided, is the name
> +of a file containing the error output of command.  OUTPUT and the
> +contents of ERR-FILE will be included in the error message."
> +
> +  ;; This is implemented as a cond to make it easy to expand.
> +  (cond
> +   ((eq exit-status 0) t)
> +   (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))))
> +
>  ;; Compatibility functions for versions of emacs before emacs 23.
>  ;;
>  ;; Both functions here were copied from emacs 23 with the following copyright:
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/7] emacs: Centralize notmuch command error handling

[Austin Clements]

On Sat, 15 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Sat, Dec 15 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>
>> This provides library functions for unified handling of errors from
>> the notmuch CLI.  Follow-up patches will convert some scattered error
>> handling to use this and add error handling where we currently ignore
>> errors.
>> ---
>
> The series looks pretty good, some hardcoded patchs Mark noticed
> and it looks to me (after viewing id:8738z7hd6x.fsf@qmul.ac.uk
> that err -> nil could be done.

Fixed.

> Would (goto-char (point-min)) be needed before (insert msg) in
> this patch -- what If user has moved cursor while viewing old
> error messages but not pressed q (dismissed) on the buffer. 

Good idea.  Actually, I had intended to *append* the error to the buffer
so it would be in chronological order.  I tweaked things a bit so
messages should always be appended and point will be left at the end of
the error log.

> Also, what about (unless (eobp) (insert "\n")) to add empty
> line between error messages ?

Done.

> Tomi

Re: [PATCH 2/7] emacs: Use unified error handling in notmuch-call-notmuch-process

[Austin Clements]

On Sat, 15 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> On Sat, 15 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> This makes notmuch-call-notmuch-process use the unified CLI error
>> handling, which basically refines the error handling this function
>> already did.
>> ---
>>  emacs/notmuch.el |   15 ++++-----------
>>  1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> index f9454d8..e25b54e 100644
>> --- a/emacs/notmuch.el
>> +++ b/emacs/notmuch.el
>> @@ -538,17 +538,10 @@ If BARE is set then do not prefix with \"thread:\""
>>  
>>  Output from the process will be presented to the user as an error
>>  and will also appear in a buffer named \"*Notmuch errors*\"."
>
> This comment looks like it is not true (but wasn't true before
> either). I think output will only be presented to the user if notmuch
> exits with a non-zero status?

Fixed.

>> -  (let ((error-buffer (get-buffer-create "*Notmuch errors*")))
>> -    (with-current-buffer error-buffer
>> -	(erase-buffer))
>> -    (if (eq (apply 'call-process notmuch-command nil error-buffer nil args) 0)
>> -	(point)
>> -      (progn
>> -	(with-current-buffer error-buffer
>> -	  (let ((beg (point-min))
>> -		(end (- (point-max) 1)))
>> -	    (error (buffer-substring beg end))
>> -	    ))))))
>> +  (with-temp-buffer
>> +    (let ((status (apply #'call-process notmuch-command nil t nil args)))
>> +      (notmuch-check-exit-status status (cons notmuch-command args)
>> +				 (buffer-string)))))
>>  
>
> Would it be worth separating stderr and stdout here? (Quite plausibly it
> isn't.)

Actually I think it's better to not separate stdout and stderr when we
can avoid it, since stdout may give context for what's on stderr.  When
we're trying to interpret stdout it's important to separate it, but
that's not the case here.

> Best wishes
>
> Mark
>
>
>>  (defun notmuch-search-set-tags (tags &optional pos)
>>    (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
>> -- 
>> 1.7.10.4

Re: [PATCH 6/7] emacs: Use unified error handling in search

[Austin Clements]

On Sat, 15 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> On Sat, 15 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> This slightly changes the output of an existing test since we now
>> report non-zero exits with a pop-up buffer instead of at the end of
>> the search results.
>> ---
>>  emacs/notmuch-lib.el           |   13 +++++++++++++
>>  emacs/notmuch.el               |   13 ++++++++-----
>>  test/emacs-large-search-buffer |    2 +-
>>  3 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> index 9222de8..cf61635 100644
>> --- a/emacs/notmuch-lib.el
>> +++ b/emacs/notmuch-lib.el
>> @@ -332,6 +332,19 @@ the user dismisses it."
>>  	(goto-char (point-min))))
>>      (pop-to-buffer buf)))
>>  
>> +(defun notmuch-check-async-exit-status (proc msg)
>> +  "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."
>> +  (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)))))
>> +
>>  (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.
>>  
>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> index e25b54e..c20de13 100644
>> --- a/emacs/notmuch.el
>> +++ b/emacs/notmuch.el
>> @@ -637,6 +637,7 @@ of the result."
>>  	(exit-status (process-exit-status proc))
>>  	(never-found-target-thread nil))
>>      (when (memq status '(exit signal))
>> +      (catch 'return
>>  	(kill-buffer (process-get proc 'parse-buf))
>>  	(if (buffer-live-p buffer)
>>  	    (with-current-buffer buffer
>> @@ -647,17 +648,19 @@ of the result."
>>  		  (if (eq status 'signal)
>>  		      (insert "Incomplete search results (search process was killed).\n"))
>>  		  (when (eq status 'exit)
>> -		    (insert "End of search results.")
>> -		    (unless (= exit-status 0)
>> -		      (insert (format " (process returned %d)" exit-status)))
>> -		    (insert "\n")
>> +		    (insert "End of search results.\n")
>> +		    (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)))
>
> A total triviality (mostly just to check that I am understanding these
> parts): is the "err" after the condition-case used/needed?

It's not used or needed.  Syntactically you need something there, of
course, so I changed it to nil as Tomi suggested, which causes
condition-case to simply not bind the error value to anything.

> Best wishes
>
> Mark
>>  		    (if (and atbob
>>  			     (not (string= notmuch-search-target-thread "found")))
>>  			(set 'never-found-target-thread t)))))
>>  	      (when (and never-found-target-thread
>>  		       notmuch-search-target-line)
>>  		  (goto-char (point-min))
>> -		  (forward-line (1- notmuch-search-target-line))))))))
>> +		  (forward-line (1- notmuch-search-target-line)))))))))
>>  
>>  (defcustom notmuch-search-line-faces '(("unread" :weight bold)
>>  				       ("flagged" :foreground "blue"))
>> diff --git a/test/emacs-large-search-buffer b/test/emacs-large-search-buffer
>> index 678328d..9dcbef5 100755
>> --- a/test/emacs-large-search-buffer
>> +++ b/test/emacs-large-search-buffer
>> @@ -36,7 +36,7 @@ test_emacs '(notmuch-search "--this-option-does-not-exist")
>>  cat <<EOF >EXPECTED
>>  Error: Unexpected output from notmuch search:
>>  Unrecognized option: --this-option-does-not-exist
>> -End of search results. (process returned 1)
>> +End of search results.
>>  EOF
>>  test_expect_equal_file OUTPUT EXPECTED
>>  
>> -- 
>> 1.7.10.4