From 0d75f94112fe586a904a46e9833a84d6028b7f47 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Thu, 3 Jan 2013 19:44:16 +1900 Subject: [PATCH] Re: [PATCH] emacs: Use the minibuffer for CLI error reporting --- 6b/a717938896a4a59079b7c4ce0042315d2a1d15 | 362 ++++++++++++++++++++++ 1 file changed, 362 insertions(+) create mode 100644 6b/a717938896a4a59079b7c4ce0042315d2a1d15 diff --git a/6b/a717938896a4a59079b7c4ce0042315d2a1d15 b/6b/a717938896a4a59079b7c4ce0042315d2a1d15 new file mode 100644 index 000000000..539386597 --- /dev/null +++ b/6b/a717938896a4a59079b7c4ce0042315d2a1d15 @@ -0,0 +1,362 @@ +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 1421C431FAF + for ; Wed, 2 Jan 2013 16:44:22 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 9dSF8q5n+qsL for ; + Wed, 2 Jan 2013 16:44:21 -0800 (PST) +Received: from dmz-mailsec-scanner-3.mit.edu (DMZ-MAILSEC-SCANNER-3.MIT.EDU + [18.9.25.14]) + by olra.theworths.org (Postfix) with ESMTP id CEFF4431FAE + for ; Wed, 2 Jan 2013 16:44:20 -0800 (PST) +X-AuditID: 1209190e-b7fa16d000001402-74-50e4d4632a33 +Received: from mailhub-auth-4.mit.edu ( [18.7.62.39]) + by dmz-mailsec-scanner-3.mit.edu (Symantec Messaging Gateway) with SMTP + id BA.40.05122.364D4E05; Wed, 2 Jan 2013 19:44:19 -0500 (EST) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id r030iIHx023009; + Wed, 2 Jan 2013 19:44:18 -0500 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id r030iGKR005149 + (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); + Wed, 2 Jan 2013 19:44:17 -0500 (EST) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80) + (envelope-from ) + id 1TqYuu-0001T1-7V; Wed, 02 Jan 2013 19:44:16 -0500 +From: Austin Clements +To: Mark Walters , notmuch@notmuchmail.org +Subject: Re: [PATCH] emacs: Use the minibuffer for CLI error reporting +In-Reply-To: <87r4m820yh.fsf@qmul.ac.uk> +References: <87wqw2pcqs.fsf@zancas.localnet> + <1356724088-26032-1-git-send-email-amdragon@mit.edu> + <87r4m820yh.fsf@qmul.ac.uk> +User-Agent: Notmuch/0.14+236~gf64406d (http://notmuchmail.org) Emacs/23.4.1 + (i486-pc-linux-gnu) +Date: Wed, 02 Jan 2013 19:44:16 -0500 +Message-ID: <874nizksdb.fsf@awakening.csail.mit.edu> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFupileLIzCtJLcpLzFFi42IRYrdT102+8iTA4M8seYsbrd2MFnv2eVms + nstjcf3mTGYHFo+7p7k8ds66y+7xbNUtZo8th94zB7BEcdmkpOZklqUW6dslcGWcW/yYqeBV + TMXF/edZGhhne3QxcnJICJhITH3eywhhi0lcuLeerYuRi0NIYB+jROfOuVDOekaJZSemQzkX + mCR+nfrCBOEsYZToPn+HGaSfTUBDYtv+5WCzRARcJZ5++wwWZxYwlNgy7S07iC0s4Cax6NFq + sBpOoPqbuxrA4kICtRLbuj+zgtiiAvESz+99YwGxWQRUJV69fAK0jIODF+jWP+cCQcK8AoIS + J2c+YYEYryVx499LpgmMgrOQpGYhSS1gZFrFKJuSW6Wbm5iZU5yarFucnJiXl1qka6yXm1mi + l5pSuokRFMycknw7GL8eVDrEKMDBqMTDu6LmSYAQa2JZcWXuIUZJDiYlUd78i0AhvqT8lMqM + xOKM+KLSnNTiQ4wSHMxKIrzXc4ByvCmJlVWpRfkwKWkOFiVx3ispN/2FBNITS1KzU1MLUotg + sjIcHEoSvFMvAzUKFqWmp1akZeaUIKSZODhBhvMADa8BqeEtLkjMLc5Mh8ifYtTlaHh54ymj + EEtefl6qlDjvYpAiAZCijNI8uDmwJPSKURzoLWHeJpAqHmACg5v0CmgJE9CSV28egywpSURI + STUwysjX9D5j95gbsHLukdK7O4K4DivuP805jdNF+oTxrAvH5/bs+zJTZ+b707udN7UVS81Y + G8h8cd2R/AXR17P/9J9U/huxU2j3vAUePn031RkWb9pr9yzZ5dzdt+vWTExl+ibUwmazPm/X + ked5i9wKnl1/+mW3RAmX0HHRWP0pjYt0OJRLT9e/Oq/EUpyRaKjFXFScCAAongfTHQMAAA== +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: Thu, 03 Jan 2013 00:44:22 -0000 + +On Sat, 29 Dec 2012, Mark Walters wrote: +> 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. + +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 -- 2.26.2