From 9b377c05d7c686dd7ccbba0803e75e8354a417d5 Mon Sep 17 00:00:00 2001 From: Mark Walters Date: Sat, 29 Dec 2012 18:00:06 +0000 Subject: [PATCH] Re: [PATCH] emacs: Use the minibuffer for CLI error reporting --- d2/d3a9f15ca927e94f11c000765724cf0a1a8388 | 329 ++++++++++++++++++++++ 1 file changed, 329 insertions(+) create mode 100644 d2/d3a9f15ca927e94f11c000765724cf0a1a8388 diff --git a/d2/d3a9f15ca927e94f11c000765724cf0a1a8388 b/d2/d3a9f15ca927e94f11c000765724cf0a1a8388 new file mode 100644 index 000000000..2e31fb7a3 --- /dev/null +++ b/d2/d3a9f15ca927e94f11c000765724cf0a1a8388 @@ -0,0 +1,329 @@ +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 78E08431FB6 + for ; Sat, 29 Dec 2012 10:00:20 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -1.098 +X-Spam-Level: +X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5 + tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001, + NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] 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 Qk6IuyG2vCsj for ; + Sat, 29 Dec 2012 10:00:19 -0800 (PST) +Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6]) + (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id A3780431FAF + for ; Sat, 29 Dec 2012 10:00:18 -0800 (PST) +Received: from smtp.qmul.ac.uk ([138.37.6.40]) + by mail2.qmul.ac.uk with esmtp (Exim 4.71) + (envelope-from ) + id 1Tp0hj-00031n-B4; Sat, 29 Dec 2012 18:00:15 +0000 +Received: from 94.197.9.104.threembb.co.uk ([94.197.9.104] helo=localhost) + by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69) + (envelope-from ) + id 1Tp0hc-00021Y-9f; Sat, 29 Dec 2012 18:00:15 +0000 +From: Mark Walters +To: Austin Clements , notmuch@notmuchmail.org +Subject: Re: [PATCH] emacs: Use the minibuffer for CLI error reporting +In-Reply-To: <1356724088-26032-1-git-send-email-amdragon@mit.edu> +References: <87wqw2pcqs.fsf@zancas.localnet> + <1356724088-26032-1-git-send-email-amdragon@mit.edu> +User-Agent: Notmuch/0.14+236~g1d0044f (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Sat, 29 Dec 2012 18:00:06 +0000 +Message-ID: <87r4m820yh.fsf@qmul.ac.uk> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Sender-Host-Address: 94.197.9.104 +X-QM-SPAM-Info: Sender has good ham record. :) +X-QM-Body-MD5: b41ddfec0532531c5a658b929c786d0a (of first 20000 bytes) +X-SpamAssassin-Score: -1.8 +X-SpamAssassin-SpamBar: - +X-SpamAssassin-Report: The QM spam filters have analysed this message to + determine if it is + spam. We require at least 5.0 points to mark a message as spam. + This message scored -1.8 points. + Summary of the scoring: + * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, + * medium trust + * [138.37.6.40 listed in list.dnswl.org] + * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail + provider * (markwalters1009[at]gmail.com) + * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay + * domain + * 0.5 AWL AWL: From: address is in the auto white-list +X-QM-Scan-Virus: ClamAV says the message is clean +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, 29 Dec 2012 18:00:20 -0000 + +On Fri, 28 Dec 2012, 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). + +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 -- 2.26.2