* [PATCH] emacs: tweak error buffer handling @ 2012-12-22 20:49 Mark Walters 2012-12-25 21:05 ` Tomi Ollila 0 siblings, 1 reply; 16+ messages in thread From: Mark Walters @ 2012-12-22 20:49 UTC (permalink / raw) To: notmuch view-mode-enter changed between emacs 23 and emacs 24: the current code makes the error buffer disappear in emacs 24 on quitting it (ie pressing q) but this just kills the buffer without closing the split window in emacs 23. This patch makes the error buffer window disappear in emacs 23 too. Since the view-mode-enter function changed we have to test for version and do the correct thing in each case. --- This seems to work but I have only tested on 23.4 and 24.2 Best wishes Mark emacs/notmuch-lib.el | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el index 77a591d..0407f8a 100644 --- a/emacs/notmuch-lib.el +++ b/emacs/notmuch-lib.el @@ -324,15 +324,17 @@ the user dismisses it." (let ((buf (get-buffer-create "*Notmuch errors*"))) (with-current-buffer buf - (view-mode-enter nil #'kill-buffer) + (pop-to-buffer buf) + (view-mode-enter (when (< emacs-major-version 24) + (cons (selected-window) (cons nil t))) + #'kill-buffer) (let ((inhibit-read-only t)) (goto-char (point-max)) (unless (bobp) (insert "\n")) (insert msg) (unless (bolp) - (insert "\n")))) - (pop-to-buffer buf))) + (insert "\n")))))) (defun notmuch-check-async-exit-status (proc msg) "If PROC exited abnormally, pop up an error buffer and signal an error. -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: tweak error buffer handling 2012-12-22 20:49 [PATCH] emacs: tweak error buffer handling Mark Walters @ 2012-12-25 21:05 ` Tomi Ollila 2012-12-26 22:27 ` Mark Walters 0 siblings, 1 reply; 16+ messages in thread From: Tomi Ollila @ 2012-12-25 21:05 UTC (permalink / raw) To: Mark Walters, notmuch On Sat, Dec 22 2012, Mark Walters <markwalters1009@gmail.com> wrote: > view-mode-enter changed between emacs 23 and emacs 24: the current > code makes the error buffer disappear in emacs 24 on quitting it (ie > pressing q) but this just kills the buffer without closing the split > window in emacs 23. > > This patch makes the error buffer window disappear in emacs 23 > too. Since the view-mode-enter function changed we have to test for > version and do the correct thing in each case. > --- > > This seems to work but I have only tested on 23.4 and 24.2 I run emacs 23.1.1 to get the documentation of view-mode-enter there. So, this patch instructs to delete WINDOW when exiting view mode... Documentation of pop-to-buffer says: "Select buffer BUFFER-OR-NAME in some window, preferably a different one." What if pop-up-windows's value is nil -- the content of current window is replaced with this view stuff -- and when exiting view mode, the window will be deleted ? What happens with emacs 24 in this case ? Tomi > Best wishes > > Mark > > > > emacs/notmuch-lib.el | 8 +++++--- > 1 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el > index 77a591d..0407f8a 100644 > --- a/emacs/notmuch-lib.el > +++ b/emacs/notmuch-lib.el > @@ -324,15 +324,17 @@ the user dismisses it." > > (let ((buf (get-buffer-create "*Notmuch errors*"))) > (with-current-buffer buf > - (view-mode-enter nil #'kill-buffer) > + (pop-to-buffer buf) > + (view-mode-enter (when (< emacs-major-version 24) > + (cons (selected-window) (cons nil t))) > + #'kill-buffer) > (let ((inhibit-read-only t)) > (goto-char (point-max)) > (unless (bobp) > (insert "\n")) > (insert msg) > (unless (bolp) > - (insert "\n")))) > - (pop-to-buffer buf))) > + (insert "\n")))))) > > (defun notmuch-check-async-exit-status (proc msg) > "If PROC exited abnormally, pop up an error buffer and signal an error. > -- > 1.7.9.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: tweak error buffer handling 2012-12-25 21:05 ` Tomi Ollila @ 2012-12-26 22:27 ` Mark Walters 2012-12-27 23:04 ` Austin Clements 0 siblings, 1 reply; 16+ messages in thread From: Mark Walters @ 2012-12-26 22:27 UTC (permalink / raw) To: Tomi Ollila, notmuch On Tue, 25 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote: > On Sat, Dec 22 2012, Mark Walters <markwalters1009@gmail.com> wrote: > >> view-mode-enter changed between emacs 23 and emacs 24: the current >> code makes the error buffer disappear in emacs 24 on quitting it (ie >> pressing q) but this just kills the buffer without closing the split >> window in emacs 23. >> >> This patch makes the error buffer window disappear in emacs 23 >> too. Since the view-mode-enter function changed we have to test for >> version and do the correct thing in each case. >> --- >> >> This seems to work but I have only tested on 23.4 and 24.2 > > I run emacs 23.1.1 to get the documentation of view-mode-enter > there. So, this patch instructs to delete WINDOW when exiting > view mode... > > Documentation of pop-to-buffer says: > > "Select buffer BUFFER-OR-NAME in some window, preferably a different one." > > What if pop-up-windows's value is nil -- the content of current window > is replaced with this view stuff -- and when exiting view mode, the > window will be deleted ? What happens with emacs 24 in this case ? Hi You are quite right there are problems here under emacs 23: if you already have a split window when the error occurs in one part the error is displayed in the other window and then on exit that (previously existing) window is closed. What do people think should happen on an error? I, personally, don't like taking over an existing window, and Jamie liked some of the errors (eg non-fatal `locked database' tagging errors) to be shown in the mini-buffer. I also think it is going to be difficult to get this right: emacs 23 and 24 are different and there are also some user configuration variable that affect what happens. Best wishes Mark > > Tomi > >> Best wishes >> >> Mark >> >> >> >> emacs/notmuch-lib.el | 8 +++++--- >> 1 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el >> index 77a591d..0407f8a 100644 >> --- a/emacs/notmuch-lib.el >> +++ b/emacs/notmuch-lib.el >> @@ -324,15 +324,17 @@ the user dismisses it." >> >> (let ((buf (get-buffer-create "*Notmuch errors*"))) >> (with-current-buffer buf >> - (view-mode-enter nil #'kill-buffer) >> + (pop-to-buffer buf) >> + (view-mode-enter (when (< emacs-major-version 24) >> + (cons (selected-window) (cons nil t))) >> + #'kill-buffer) >> (let ((inhibit-read-only t)) >> (goto-char (point-max)) >> (unless (bobp) >> (insert "\n")) >> (insert msg) >> (unless (bolp) >> - (insert "\n")))) >> - (pop-to-buffer buf))) >> + (insert "\n")))))) >> >> (defun notmuch-check-async-exit-status (proc msg) >> "If PROC exited abnormally, pop up an error buffer and signal an error. >> -- >> 1.7.9.1 >> >> _______________________________________________ >> notmuch mailing list >> notmuch@notmuchmail.org >> http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: tweak error buffer handling 2012-12-26 22:27 ` Mark Walters @ 2012-12-27 23:04 ` Austin Clements 2012-12-28 9:03 ` Mark Walters 0 siblings, 1 reply; 16+ messages in thread From: Austin Clements @ 2012-12-27 23:04 UTC (permalink / raw) To: Mark Walters; +Cc: Tomi Ollila, notmuch Quoth Mark Walters on Dec 26 at 10:27 pm: > > On Tue, 25 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote: > > On Sat, Dec 22 2012, Mark Walters <markwalters1009@gmail.com> wrote: > > > >> view-mode-enter changed between emacs 23 and emacs 24: the current > >> code makes the error buffer disappear in emacs 24 on quitting it (ie > >> pressing q) but this just kills the buffer without closing the split > >> window in emacs 23. > >> > >> This patch makes the error buffer window disappear in emacs 23 > >> too. Since the view-mode-enter function changed we have to test for > >> version and do the correct thing in each case. > >> --- > >> > >> This seems to work but I have only tested on 23.4 and 24.2 > > > > I run emacs 23.1.1 to get the documentation of view-mode-enter > > there. So, this patch instructs to delete WINDOW when exiting > > view mode... > > > > Documentation of pop-to-buffer says: > > > > "Select buffer BUFFER-OR-NAME in some window, preferably a different one." > > > > What if pop-up-windows's value is nil -- the content of current window > > is replaced with this view stuff -- and when exiting view mode, the > > window will be deleted ? What happens with emacs 24 in this case ? > > Hi > > You are quite right there are problems here under emacs 23: if you > already have a split window when the error occurs in one part the error > is displayed in the other window and then on exit that (previously > existing) window is closed. > > What do people think should happen on an error? I, personally, don't > like taking over an existing window, and Jamie liked some of the errors > (eg non-fatal `locked database' tagging errors) to be shown in the > mini-buffer. > > I also think it is going to be difficult to get this right: emacs 23 and > 24 are different and there are also some user configuration variable > that affect what happens. How about showing all errors in the minibuffer (which could simply mean calling (error ...) and letting the Emacs top-level show it in the mini-buffer)? We could log the verbose error details (like stdout) to some other buffer that we don't automatically show, but instead simply reference from the minibuffer message. This would be more in line with how Emacs typically handles errors, and would make the details available to the user without flooding them with the details. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: tweak error buffer handling 2012-12-27 23:04 ` Austin Clements @ 2012-12-28 9:03 ` Mark Walters 2012-12-28 12:44 ` David Bremner 0 siblings, 1 reply; 16+ messages in thread From: Mark Walters @ 2012-12-28 9:03 UTC (permalink / raw) To: Austin Clements; +Cc: Tomi Ollila, notmuch Austin Clements <amdragon@MIT.EDU> writes: > Quoth Mark Walters on Dec 26 at 10:27 pm: >> >> On Tue, 25 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote: >> > On Sat, Dec 22 2012, Mark Walters <markwalters1009@gmail.com> wrote: >> > >> >> view-mode-enter changed between emacs 23 and emacs 24: the current >> >> code makes the error buffer disappear in emacs 24 on quitting it (ie >> >> pressing q) but this just kills the buffer without closing the split >> >> window in emacs 23. >> >> >> >> This patch makes the error buffer window disappear in emacs 23 >> >> too. Since the view-mode-enter function changed we have to test for >> >> version and do the correct thing in each case. >> >> --- >> >> >> >> This seems to work but I have only tested on 23.4 and 24.2 >> > >> > I run emacs 23.1.1 to get the documentation of view-mode-enter >> > there. So, this patch instructs to delete WINDOW when exiting >> > view mode... >> > >> > Documentation of pop-to-buffer says: >> > >> > "Select buffer BUFFER-OR-NAME in some window, preferably a different one." >> > >> > What if pop-up-windows's value is nil -- the content of current window >> > is replaced with this view stuff -- and when exiting view mode, the >> > window will be deleted ? What happens with emacs 24 in this case ? >> >> Hi >> >> You are quite right there are problems here under emacs 23: if you >> already have a split window when the error occurs in one part the error >> is displayed in the other window and then on exit that (previously >> existing) window is closed. >> >> What do people think should happen on an error? I, personally, don't >> like taking over an existing window, and Jamie liked some of the errors >> (eg non-fatal `locked database' tagging errors) to be shown in the >> mini-buffer. >> >> I also think it is going to be difficult to get this right: emacs 23 and >> 24 are different and there are also some user configuration variable >> that affect what happens. > > How about showing all errors in the minibuffer (which could simply > mean calling (error ...) and letting the Emacs top-level show it in > the mini-buffer)? We could log the verbose error details (like > stdout) to some other buffer that we don't automatically show, but > instead simply reference from the minibuffer message. This would be > more in line with how Emacs typically handles errors, and would make > the details available to the user without flooding them with the > details. Hi That sounds great. In an ideal world we could behave slightly differently for fatal errors but the common errors are likely to be transient (locked database errors) so getting these right (which this would do) seems the important step. My view is that we should push a fix like the above for 0.15. What do other people think? Best wishes Mark ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: tweak error buffer handling 2012-12-28 9:03 ` Mark Walters @ 2012-12-28 12:44 ` David Bremner 2012-12-28 19:48 ` [PATCH] emacs: Use the minibuffer for CLI error reporting Austin Clements 0 siblings, 1 reply; 16+ messages in thread From: David Bremner @ 2012-12-28 12:44 UTC (permalink / raw) To: Mark Walters, Austin Clements; +Cc: notmuch Mark Walters <markwalters1009@gmail.com> writes: > Austin Clements <amdragon@MIT.EDU> writes: >> How about showing all errors in the minibuffer (which could simply >> mean calling (error ...) and letting the Emacs top-level show it in >> the mini-buffer)? We could log the verbose error details (like >> stdout) to some other buffer that we don't automatically show, but >> instead simply reference from the minibuffer message > > That sounds great. In an ideal world we could behave slightly > differently for fatal errors but the common errors are likely to be > transient (locked database errors) so getting these right (which this > would do) seems the important step. > > My view is that we should push a fix like the above for 0.15. What do > other people think? Sounds reasonable. I have to admit the new reporting is a bit annoying for transient messages. d ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] emacs: Use the minibuffer for CLI error reporting 2012-12-28 12:44 ` David Bremner @ 2012-12-28 19:48 ` Austin Clements 2012-12-29 18:00 ` Mark Walters 2013-01-03 0:50 ` [PATCH v2] " Austin Clements 0 siblings, 2 replies; 16+ messages in thread From: Austin Clements @ 2012-12-28 19:48 UTC (permalink / raw) To: notmuch 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). --- emacs/notmuch-lib.el | 92 ++++++++++++++++++++++++++++---------------------- emacs/notmuch.el | 9 +++-- test/emacs | 11 +++--- test/emacs-show | 6 ++-- 4 files changed, 67 insertions(+), 51 deletions(-) diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el index 77a591d..3baab97 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." + + (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))))) + (basic-msg (format "%s exited with status %s" + (car command) exit-status))) + (when (and (null err) (or (null output) (equal output ""))) + ;; We have no details to speak of. Mimic `process-lines'. + (notmuch-logged-error basic-msg)) + (let ((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)) + "Output:\n" + (if (and output (not (equal output ""))) + output + "(none)")))) + (if err + ;; We have an error message straight from the CLI. + (notmuch-logged-error err extra) + ;; We only have combined output from the CLI; don't inundate + ;; the user with it. + (notmuch-logged-error basic-msg extra))))))) (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..8e0a4fd 100755 --- a/test/emacs +++ b/test/emacs @@ -862,18 +862,19 @@ 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 \"*Notmuch errors*\" - (test-output \"ERROR\")))" -test_expect_equal "$(cat OUTPUT ERROR)" "\ + (with-current-buffer \"*Messages*\" + (test-output \"MESSAGES\")) + (test-output))" +test_expect_equal "$(cat OUTPUT MESSAGES)" "\ 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" test_done diff --git a/test/emacs-show b/test/emacs-show index ebf530b..ae9459d 100755 --- a/test/emacs-show +++ b/test/emacs-show @@ -177,10 +177,12 @@ test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\")) (test-output) (with-current-buffer \"*Notmuch errors*\" (test-output \"ERROR\")))" +sed -i -e 's/^\[.*\]$/[XXX]/' 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: +[XXX] This is an error +Command: $PWD/notmuch_fail show --format\\=json --format-version\\=1 --exclude\\=false \\' \\* \\' +Exit status: 1 Output: This is output" -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: Use the minibuffer for CLI error reporting 2012-12-28 19:48 ` [PATCH] emacs: Use the minibuffer for CLI error reporting Austin Clements @ 2012-12-29 18:00 ` Mark Walters 2013-01-03 0:44 ` Austin Clements 2013-01-03 0:50 ` [PATCH v2] " Austin Clements 1 sibling, 1 reply; 16+ messages in thread From: Mark Walters @ 2012-12-29 18:00 UTC (permalink / raw) To: Austin Clements, notmuch On Fri, 28 Dec 2012, 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). I like this approach but have some queries below. > --- > emacs/notmuch-lib.el | 92 ++++++++++++++++++++++++++++---------------------- > emacs/notmuch.el | 9 +++-- > test/emacs | 11 +++--- > test/emacs-show | 6 ++-- > 4 files changed, 67 insertions(+), 51 deletions(-) > > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el > index 77a591d..3baab97 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." It might be worth commenting that since this signals an error it does not "return"; I found the code in notmuch-check-exit-status rather confusing until I realised that. > + > + (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))))) > + (basic-msg (format "%s exited with status %s" > + (car command) exit-status))) > + (when (and (null err) (or (null output) (equal output ""))) > + ;; We have no details to speak of. Mimic `process-lines'. This means that if err and output are null we give a minimal error message and we don't log the command line that fails. Perhaps the `when' clause could be omitted so we get the extra information from below? > + (notmuch-logged-error basic-msg)) > + (let ((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)) > + "Output:\n" > + (if (and output (not (equal output ""))) > + output > + "(none)")))) > + (if err > + ;; We have an error message straight from the CLI. > + (notmuch-logged-error err extra) > + ;; We only have combined output from the CLI; don't inundate > + ;; the user with it. > + (notmuch-logged-error basic-msg extra))))))) Also, depending how the above gets changed, would it be worth pulling the let clause before the cond clause, and subsuming some of the when/if/else logic into the cond? This has the nice side effect that the reader expects cond clauses to stop after the first match so the fact that notmuch-check-exit-status signals an error would not matter when reading this code. I think a command line would be useful in almost all cases (in the error buffer). If you decide to always supply that then your error message might want tweaking as it would always have extra information in the error buffer. Finally, and this is only a thought, I wonder if the mechanism can be tweaked to provide debug information along these lines for all notmuch commands whether or not they succeed: something like if notmuch-debug-commands is set or there is an error? Incidentally do you have good ways to test this code (ie see what it does in each case)? My hackish experiments suggested the async errors were less useful than the sync ones but maybe that is just an inherent limitation of the emacs async mechanisms. Best wishes Mark > > (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..8e0a4fd 100755 > --- a/test/emacs > +++ b/test/emacs > @@ -862,18 +862,19 @@ 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 \"*Notmuch errors*\" > - (test-output \"ERROR\")))" > -test_expect_equal "$(cat OUTPUT ERROR)" "\ > + (with-current-buffer \"*Messages*\" > + (test-output \"MESSAGES\")) > + (test-output))" > +test_expect_equal "$(cat OUTPUT MESSAGES)" "\ > 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" > > > test_done > diff --git a/test/emacs-show b/test/emacs-show > index ebf530b..ae9459d 100755 > --- a/test/emacs-show > +++ b/test/emacs-show > @@ -177,10 +177,12 @@ test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\")) > (test-output) > (with-current-buffer \"*Notmuch errors*\" > (test-output \"ERROR\")))" > +sed -i -e 's/^\[.*\]$/[XXX]/' 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: > +[XXX] > This is an error > +Command: $PWD/notmuch_fail show --format\\=json --format-version\\=1 --exclude\\=false \\' \\* \\' > +Exit status: 1 > Output: > This is output" > > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: Use the minibuffer for CLI error reporting 2012-12-29 18:00 ` Mark Walters @ 2013-01-03 0:44 ` Austin Clements 2013-01-03 23:21 ` David Bremner 0 siblings, 1 reply; 16+ messages in thread From: Austin Clements @ 2013-01-03 0:44 UTC (permalink / raw) To: Mark Walters, notmuch On Sat, 29 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote: > On Fri, 28 Dec 2012, 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). > > I like this approach but have some queries below. > >> --- >> emacs/notmuch-lib.el | 92 ++++++++++++++++++++++++++++---------------------- >> emacs/notmuch.el | 9 +++-- >> test/emacs | 11 +++--- >> test/emacs-show | 6 ++-- >> 4 files changed, 67 insertions(+), 51 deletions(-) >> >> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el >> index 77a591d..3baab97 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." > > It might be worth commenting that since this signals an error it does > not "return"; I found the code in notmuch-check-exit-status rather > confusing until I realised that. Done. >> + >> + (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))))) >> + (basic-msg (format "%s exited with status %s" >> + (car command) exit-status))) >> + (when (and (null err) (or (null output) (equal output ""))) >> + ;; We have no details to speak of. Mimic `process-lines'. > > This means that if err and output are null we give a minimal error message and we > don't log the command line that fails. Perhaps the `when' clause could > be omitted so we get the extra information from below? Good point. v2 basically follows your suggestion of removing the `when'. >> + (notmuch-logged-error basic-msg)) >> + (let ((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)) >> + "Output:\n" >> + (if (and output (not (equal output ""))) >> + output >> + "(none)")))) >> + (if err >> + ;; We have an error message straight from the CLI. >> + (notmuch-logged-error err extra) >> + ;; We only have combined output from the CLI; don't inundate >> + ;; the user with it. >> + (notmuch-logged-error basic-msg extra))))))) > > Also, depending how the above gets changed, would it be worth pulling the let > clause before the cond clause, and subsuming some of the when/if/else > logic into the cond? This has the nice side effect that the reader > expects cond clauses to stop after the first match so the fact that > notmuch-check-exit-status signals an error would not matter when reading > this code. I think removing the `when' simplifies this enough. I'd rather not lift the let outside the cond because the process of getting the error message is nontrivial and would be a waste in the common case of a success exit code. > I think a command line would be useful in almost all cases (in the error > buffer). If you decide to always supply that then your error message > might want tweaking as it would always have extra information in the > error buffer. I'm not too worried about always having the reference to the errors buffer. My hope is that most cases will provide an error file (search is a notable exception and may be worth fixing), in which case it's going to provide that reference regardless. > Finally, and this is only a thought, I wonder if the mechanism can be > tweaked to provide debug information along these lines for all notmuch > commands whether or not they succeed: something like if > notmuch-debug-commands is set or there is an error? Sounds like a good follow-up patch. Though currently the code is full of direct call-process and process-lines calls, so it would take a little (worthwhile) effort to consolidate these. > Incidentally do you have good ways to test this code (ie see what it > does in each case)? My hackish experiments suggested the async errors > were less useful than the sync ones but maybe that is just an inherent > limitation of the emacs async mechanisms. I'm not sure I can do much beyond what's in the patch. v2 improves it a bit to be more thorough, so now both tests systematically collect the buffer, the errors buffer, and messages. See what you think. Async errors are harder, since it's 2013 and Emacs still provides no means to separate stdout from stderr for async processes. The official way to do this is to fire up a shell running the command and have the shell redirect stderr. This may be worthwhile for search since it would give us better error messages and eliminate the crazy resynchronization we have to do to deal with errors embedded in the output, but that's for another patch. > Best wishes > > Mark > > > > > > >> >> (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..8e0a4fd 100755 >> --- a/test/emacs >> +++ b/test/emacs >> @@ -862,18 +862,19 @@ 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 \"*Notmuch errors*\" >> - (test-output \"ERROR\")))" >> -test_expect_equal "$(cat OUTPUT ERROR)" "\ >> + (with-current-buffer \"*Messages*\" >> + (test-output \"MESSAGES\")) >> + (test-output))" >> +test_expect_equal "$(cat OUTPUT MESSAGES)" "\ >> 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" >> >> >> test_done >> diff --git a/test/emacs-show b/test/emacs-show >> index ebf530b..ae9459d 100755 >> --- a/test/emacs-show >> +++ b/test/emacs-show >> @@ -177,10 +177,12 @@ test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\")) >> (test-output) >> (with-current-buffer \"*Notmuch errors*\" >> (test-output \"ERROR\")))" >> +sed -i -e 's/^\[.*\]$/[XXX]/' 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: >> +[XXX] >> This is an error >> +Command: $PWD/notmuch_fail show --format\\=json --format-version\\=1 --exclude\\=false \\' \\* \\' >> +Exit status: 1 >> Output: >> This is output" >> >> -- >> 1.7.10.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] emacs: Use the minibuffer for CLI error reporting 2013-01-03 0:44 ` Austin Clements @ 2013-01-03 23:21 ` David Bremner 0 siblings, 0 replies; 16+ messages in thread From: David Bremner @ 2013-01-03 23:21 UTC (permalink / raw) To: Austin Clements, Mark Walters, notmuch Austin Clements <amdragon@MIT.EDU> writes: > > Async errors are harder, since it's 2013 and Emacs still provides no > means to separate stdout from stderr for async processes. The official > way to do this is to fire up a shell running the command and have the > shell redirect stderr. This may be worthwhile for search since it would > give us better error messages and eliminate the crazy resynchronization > we have to do to deal with errors embedded in the output, but that's for > another patch. Nothing to do with this patch per se. but it might be sensible to add top level convenience arguments to the notmuch command to redirect stdout and stderr. d ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] emacs: Use the minibuffer for CLI error reporting 2012-12-28 19:48 ` [PATCH] emacs: Use the minibuffer for CLI error reporting Austin Clements 2012-12-29 18:00 ` Mark Walters @ 2013-01-03 0:50 ` Austin Clements 2013-01-03 21:47 ` [PATCH v3] " Austin Clements 1 sibling, 1 reply; 16+ messages in thread From: Austin Clements @ 2013-01-03 0:50 UTC (permalink / raw) To: notmuch 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). --- 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..c7bd150 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] +/tmp/nmtest/tmp.emacs/notmuch_fail exited with status 1 +command: /tmp/nmtest/tmp.emacs/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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3] emacs: Use the minibuffer for CLI error reporting 2013-01-03 0:50 ` [PATCH v2] " Austin Clements @ 2013-01-03 21:47 ` Austin Clements 2013-01-03 22:48 ` Mark Walters ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Austin Clements @ 2013-01-03 21:47 UTC (permalink / raw) To: notmuch 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). --- 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting 2013-01-03 21:47 ` [PATCH v3] " Austin Clements @ 2013-01-03 22:48 ` Mark Walters 2013-01-06 21:12 ` Austin Clements 2013-01-05 21:33 ` Tomi Ollila 2013-01-07 2:54 ` David Bremner 2 siblings, 1 reply; 16+ messages in thread From: Mark Walters @ 2013-01-03 22:48 UTC (permalink / raw) To: Austin Clements, notmuch On Thu, 03 Jan 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). > --- > > This version fixes two hard-coded paths in the tests. This version looks good to me but I have one query we may like to consider. At the moment notmuch-call-notmuch-process returns the stderr mixed with stdout and we might like to separate that out (particularly as the error message lists stderr and stdout separately and in this case it all gets called stdout). I attach a patch that does this: I am not really familiar with this so I just took Austin's code from notmuch-call-notmuch-json. Austin: obviously feel free to fold this into your patch if you think appropriate. Best wishes Mark From b73395c8efb57111bd4de281797004747de6c2ed Mon Sep 17 00:00:00 2001 From: Mark Walters <markwalters1009@gmail.com> Date: Thu, 3 Jan 2013 22:25:02 +0000 Subject: [PATCH] tweak notmuch-call-notmuch-process --- emacs/notmuch.el | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/emacs/notmuch.el b/emacs/notmuch.el index c98a4fe..4f7ee2c 100644 --- a/emacs/notmuch.el +++ b/emacs/notmuch.el @@ -540,9 +540,13 @@ If notmuch exits with a non-zero status, output from the process will appear in a buffer named \"*Notmuch errors*\" and an error will be signaled." (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))))) + (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)) + (delete-file err-file))))) (defun notmuch-search-set-tags (tags &optional pos) (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags))) -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting 2013-01-03 22:48 ` Mark Walters @ 2013-01-06 21:12 ` Austin Clements 0 siblings, 0 replies; 16+ messages in thread From: Austin Clements @ 2013-01-06 21:12 UTC (permalink / raw) To: Mark Walters, notmuch On Thu, 03 Jan 2013, Mark Walters <markwalters1009@gmail.com> wrote: > On Thu, 03 Jan 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). >> --- >> >> This version fixes two hard-coded paths in the tests. > > > This version looks good to me but I have one query we may like to > consider. > > At the moment notmuch-call-notmuch-process returns the stderr mixed with > stdout and we might like to separate that out (particularly as the error > message lists stderr and stdout separately and in this case it all gets > called stdout). This sounds like a good idea to me, though I'd rather do it as a separate patch. Your patch looks good to me (modulo commit message, obviously). Care to roll an official patch? As I mentioned elsewhere, there are several direct calls to notmuch that don't go through any of this error handling (for example, `notmuch-call-notmuch-process' is only used in 'notmuch-tag'). It would be great if Someone (TM) cleaned this up. > I attach a patch that does this: I am not really familiar with this so I > just took Austin's code from notmuch-call-notmuch-json. > > Austin: obviously feel free to fold this into your patch if you think > appropriate. > > Best wishes > > Mark > > From b73395c8efb57111bd4de281797004747de6c2ed Mon Sep 17 00:00:00 2001 > From: Mark Walters <markwalters1009@gmail.com> > Date: Thu, 3 Jan 2013 22:25:02 +0000 > Subject: [PATCH] tweak notmuch-call-notmuch-process > > --- > emacs/notmuch.el | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/emacs/notmuch.el b/emacs/notmuch.el > index c98a4fe..4f7ee2c 100644 > --- a/emacs/notmuch.el > +++ b/emacs/notmuch.el > @@ -540,9 +540,13 @@ If notmuch exits with a non-zero status, output from the process > will appear in a buffer named \"*Notmuch errors*\" and an error > will be signaled." > (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))))) > + (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)) > + (delete-file err-file))))) > > (defun notmuch-search-set-tags (tags &optional pos) > (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags))) > -- > 1.7.9.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting 2013-01-03 21:47 ` [PATCH v3] " Austin Clements 2013-01-03 22:48 ` Mark Walters @ 2013-01-05 21:33 ` Tomi Ollila 2013-01-07 2:54 ` David Bremner 2 siblings, 0 replies; 16+ messages in thread From: Tomi Ollila @ 2013-01-05 21:33 UTC (permalink / raw) To: Austin Clements, notmuch 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting 2013-01-03 21:47 ` [PATCH v3] " Austin Clements 2013-01-03 22:48 ` Mark Walters 2013-01-05 21:33 ` Tomi Ollila @ 2013-01-07 2:54 ` David Bremner 2 siblings, 0 replies; 16+ messages in thread From: David Bremner @ 2013-01-07 2:54 UTC (permalink / raw) To: Austin Clements, notmuch Austin Clements <amdragon@MIT.EDU> writes: > 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. pushed. d ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-01-07 2:54 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-22 20:49 [PATCH] emacs: tweak error buffer handling Mark Walters 2012-12-25 21:05 ` Tomi Ollila 2012-12-26 22:27 ` Mark Walters 2012-12-27 23:04 ` Austin Clements 2012-12-28 9:03 ` Mark Walters 2012-12-28 12:44 ` David Bremner 2012-12-28 19:48 ` [PATCH] emacs: Use the minibuffer for CLI error reporting Austin Clements 2012-12-29 18:00 ` Mark Walters 2013-01-03 0:44 ` Austin Clements 2013-01-03 23:21 ` David Bremner 2013-01-03 0:50 ` [PATCH v2] " Austin Clements 2013-01-03 21:47 ` [PATCH v3] " Austin Clements 2013-01-03 22:48 ` Mark Walters 2013-01-06 21:12 ` Austin Clements 2013-01-05 21:33 ` Tomi Ollila 2013-01-07 2:54 ` David Bremner
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).