From: Tomi Ollila Date: Sat, 5 Jan 2013 21:33:08 +0000 (+0200) Subject: Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=22d2a9c1afb1106c8dd560572d221ad9189b1309;p=notmuch-archives.git Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting --- diff --git a/fc/59563151a7ceecf01558537edf2faeaa6f72ac b/fc/59563151a7ceecf01558537edf2faeaa6f72ac new file mode 100644 index 000000000..be49aff36 --- /dev/null +++ b/fc/59563151a7ceecf01558537edf2faeaa6f72ac @@ -0,0 +1,310 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id C4AD0431FAF + for ; Sat, 5 Jan 2013 13:33:19 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id hvKnUtqLYREZ for ; + Sat, 5 Jan 2013 13:33:18 -0800 (PST) +Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) + by olra.theworths.org (Postfix) with ESMTP id 4E268431FAE + for ; Sat, 5 Jan 2013 13:33:18 -0800 (PST) +Received: from guru.guru-group.fi (localhost [IPv6:::1]) + by guru.guru-group.fi (Postfix) with ESMTP id E94AB10007D; + Sat, 5 Jan 2013 23:33:08 +0200 (EET) +From: Tomi Ollila +To: Austin Clements , notmuch@notmuchmail.org +Subject: Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting +In-Reply-To: <1357249669-9706-1-git-send-email-amdragon@mit.edu> +References: <1357174222-25132-1-git-send-email-amdragon@mit.edu> + <1357249669-9706-1-git-send-email-amdragon@mit.edu> +User-Agent: Notmuch/0.14+236~ge6d0259 (http://notmuchmail.org) Emacs/24.2.1 + (x86_64-unknown-linux-gnu) +X-Face: HhBM'cA~ +MIME-Version: 1.0 +Content-Type: text/plain +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Sat, 05 Jan 2013 21:33:19 -0000 + +On Thu, Jan 03 2013, Austin Clements 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