On Thu, Jan 03 2013, Austin Clements <amdragon at 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 at notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch
