6b/a717938896a4a59079b7c4ce0042315d2a1d15

   1 Return-Path: <amdragon@mit.edu>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 1421C431FAF\r
   6         for <notmuch@notmuchmail.org>; Wed,  2 Jan 2013 16:44:22 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id 9dSF8q5n+qsL for <notmuch@notmuchmail.org>;\r
  16         Wed,  2 Jan 2013 16:44:21 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-3.mit.edu (DMZ-MAILSEC-SCANNER-3.MIT.EDU\r
  18         [18.9.25.14])\r
  19         by olra.theworths.org (Postfix) with ESMTP id CEFF4431FAE\r
  20         for <notmuch@notmuchmail.org>; Wed,  2 Jan 2013 16:44:20 -0800 (PST)\r
  21 X-AuditID: 1209190e-b7fa16d000001402-74-50e4d4632a33\r
  22 Received: from mailhub-auth-4.mit.edu ( [18.7.62.39])\r
  23         by dmz-mailsec-scanner-3.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id BA.40.05122.364D4E05; Wed,  2 Jan 2013 19:44:19 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id r030iIHx023009; \r
  27         Wed, 2 Jan 2013 19:44:18 -0500\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id r030iGKR005149\r
  32         (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT);\r
  33         Wed, 2 Jan 2013 19:44:17 -0500 (EST)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1TqYuu-0001T1-7V; Wed, 02 Jan 2013 19:44:16 -0500\r
  37 From: Austin Clements <amdragon@MIT.EDU>\r
  38 To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org\r
  39 Subject: Re: [PATCH] emacs: Use the minibuffer for CLI error reporting\r
  40 In-Reply-To: <87r4m820yh.fsf@qmul.ac.uk>\r
  41 References: <87wqw2pcqs.fsf@zancas.localnet>\r
  42         <1356724088-26032-1-git-send-email-amdragon@mit.edu>\r
  43         <87r4m820yh.fsf@qmul.ac.uk>\r
  44 User-Agent: Notmuch/0.14+236~gf64406d (http://notmuchmail.org) Emacs/23.4.1\r
  45         (i486-pc-linux-gnu)\r
  46 Date: Wed, 02 Jan 2013 19:44:16 -0500\r
  47 Message-ID: <874nizksdb.fsf@awakening.csail.mit.edu>\r
  48 MIME-Version: 1.0\r
  49 Content-Type: text/plain; charset=us-ascii\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFupileLIzCtJLcpLzFFi42IRYrdT102+8iTA4M8seYsbrd2MFnv2eVms\r
  52         nstjcf3mTGYHFo+7p7k8ds66y+7xbNUtZo8th94zB7BEcdmkpOZklqUW6dslcGWcW/yYqeBV\r
  53         TMXF/edZGhhne3QxcnJICJhITH3eywhhi0lcuLeerYuRi0NIYB+jROfOuVDOekaJZSemQzkX\r
  54         mCR+nfrCBOEsYZToPn+HGaSfTUBDYtv+5WCzRARcJZ5++wwWZxYwlNgy7S07iC0s4Cax6NFq\r
  55         sBpOoPqbuxrA4kICtRLbuj+zgtiiAvESz+99YwGxWQRUJV69fAK0jIODF+jWP+cCQcK8AoIS\r
  56         J2c+YYEYryVx499LpgmMgrOQpGYhSS1gZFrFKJuSW6Wbm5iZU5yarFucnJiXl1qka6yXm1mi\r
  57         l5pSuokRFMycknw7GL8eVDrEKMDBqMTDu6LmSYAQa2JZcWXuIUZJDiYlUd78i0AhvqT8lMqM\r
  58         xOKM+KLSnNTiQ4wSHMxKIrzXc4ByvCmJlVWpRfkwKWkOFiVx3ispN/2FBNITS1KzU1MLUotg\r
  59         sjIcHEoSvFMvAzUKFqWmp1akZeaUIKSZODhBhvMADa8BqeEtLkjMLc5Mh8ifYtTlaHh54ymj\r
  60         EEtefl6qlDjvYpAiAZCijNI8uDmwJPSKURzoLWHeJpAqHmACg5v0CmgJE9CSV28egywpSURI\r
  61         STUwysjX9D5j95gbsHLukdK7O4K4DivuP805jdNF+oTxrAvH5/bs+zJTZ+b707udN7UVS81Y\r
  62         G8h8cd2R/AXR17P/9J9U/huxU2j3vAUePn031RkWb9pr9yzZ5dzdt+vWTExl+ibUwmazPm/X\r
  63         ked5i9wKnl1/+mW3RAmX0HHRWP0pjYt0OJRLT9e/Oq/EUpyRaKjFXFScCAAongfTHQMAAA==\r
  64 X-BeenThere: notmuch@notmuchmail.org\r
  65 X-Mailman-Version: 2.1.13\r
  66 Precedence: list\r
  67 List-Id: "Use and development of the notmuch mail system."\r
  68         <notmuch.notmuchmail.org>\r
  69 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  70         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  71 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  72 List-Post: <mailto:notmuch@notmuchmail.org>\r
  73 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  74 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  75         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  76 X-List-Received-Date: Thu, 03 Jan 2013 00:44:22 -0000\r
  77 \r
  78 On Sat, 29 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  79 > On Fri, 28 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:\r
  80 >> We recently switched to popping up a buffer to report CLI errors, but\r
  81 >> this was too intrusive, especially for transient errors and especially\r
  82 >> since we made fewer things ignore errors.  This patch changes this to\r
  83 >> display a basic error message in the minibuffer (using Emacs' usual\r
  84 >> error handling path) and, if there are additional details, to log\r
  85 >> these to a separate error buffer and reference the error buffer from\r
  86 >> the minibuffer message.  This is more in line with how Emacs typically\r
  87 >> handles errors, but makes the details available to the user without\r
  88 >> flooding them with the details.\r
  89 >>\r
  90 >> Given this split, we pare down the basic message and make it more\r
  91 >> user-friendly, and also make the verbose message even more detailed\r
  92 >> (and more debugging-oriented).\r
  93 >\r
  94 > I like this approach but have some queries below.\r
  95 >\r
  96 >> ---\r
  97 >>  emacs/notmuch-lib.el |   92 ++++++++++++++++++++++++++++----------------------\r
  98 >>  emacs/notmuch.el     |    9 +++--\r
  99 >>  test/emacs           |   11 +++---\r
 100 >>  test/emacs-show      |    6 ++--\r
 101 >>  4 files changed, 67 insertions(+), 51 deletions(-)\r
 102 >>\r
 103 >> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el\r
 104 >> index 77a591d..3baab97 100644\r
 105 >> --- a/emacs/notmuch-lib.el\r
 106 >> +++ b/emacs/notmuch-lib.el\r
 107 >> @@ -316,23 +316,28 @@ string), a property list of face attributes, or a list of these."\r
 108 >>      (put-text-property pos next 'face (cons face cur))\r
 109 >>      (setq pos next)))))\r
 110 >>  \r
 111 >> -(defun notmuch-pop-up-error (msg)\r
 112 >> -  "Pop up an error buffer displaying MSG.\r
 113 >> -\r
 114 >> -This will accumulate error messages in the errors buffer until\r
 115 >> -the user dismisses it."\r
 116 >> -\r
 117 >> -  (let ((buf (get-buffer-create "*Notmuch errors*")))\r
 118 >> -    (with-current-buffer buf\r
 119 >> -      (view-mode-enter nil #'kill-buffer)\r
 120 >> -      (let ((inhibit-read-only t))\r
 121 >> -    (goto-char (point-max))\r
 122 >> -    (unless (bobp)\r
 123 >> -      (insert "\n"))\r
 124 >> -    (insert msg)\r
 125 >> +(defun notmuch-logged-error (msg &optional extra)\r
 126 >> +  "Log MSG and EXTRA to *Notmuch errors* and signal MSG.\r
 127 >> +\r
 128 >> +This logs MSG and EXTRA to the *Notmuch errors* buffer and\r
 129 >> +signals MSG as an error.  If EXTRA is non-nil, text referring the\r
 130 >> +user to the *Notmuch errors* buffer will be appended to the\r
 131 >> +signaled error."\r
 132 >\r
 133 > It might be worth commenting that since this signals an error it does\r
 134 > not "return"; I found the code in notmuch-check-exit-status rather\r
 135 > confusing until I realised that.\r
 136 \r
 137 Done.\r
 138 \r
 139 >> +\r
 140 >> +  (with-current-buffer (get-buffer-create "*Notmuch errors*")\r
 141 >> +    (goto-char (point-max))\r
 142 >> +    (unless (bobp)\r
 143 >> +      (newline))\r
 144 >> +    (save-excursion\r
 145 >> +      (insert "[" (current-time-string) "]\n" msg)\r
 146 >> +      (unless (bolp)\r
 147 >> +    (newline))\r
 148 >> +      (when extra\r
 149 >> +    (insert extra)\r
 150 >>      (unless (bolp)\r
 151 >> -      (insert "\n"))))\r
 152 >> -    (pop-to-buffer buf)))\r
 153 >> +      (newline)))))\r
 154 >> +  (error "%s" (concat msg (when extra\r
 155 >> +                        " (see *Notmuch errors* for more details)"))))\r
 156 >>  \r
 157 >>  (defun notmuch-check-async-exit-status (proc msg)\r
 158 >>    "If PROC exited abnormally, pop up an error buffer and signal an error.\r
 159 >> @@ -363,35 +368,40 @@ contents of ERR-FILE will be included in the error message."\r
 160 >>    (cond\r
 161 >>     ((eq exit-status 0) t)\r
 162 >>     ((eq exit-status 20)\r
 163 >> -    (notmuch-pop-up-error "Error: Version mismatch.\r
 164 >> +    (notmuch-logged-error "notmuch CLI version mismatch\r
 165 >>  Emacs requested an older output format than supported by the notmuch CLI.\r
 166 >> -You may need to restart Emacs or upgrade your notmuch Emacs package.")\r
 167 >> -    (error "notmuch CLI version mismatch"))\r
 168 >> +You may need to restart Emacs or upgrade your notmuch Emacs package."))\r
 169 >>     ((eq exit-status 21)\r
 170 >> -    (notmuch-pop-up-error "Error: Version mismatch.\r
 171 >> +    (notmuch-logged-error "notmuch CLI version mismatch\r
 172 >>  Emacs requested a newer output format than supported by the notmuch CLI.\r
 173 >> -You may need to restart Emacs or upgrade your notmuch package.")\r
 174 >> -    (error "notmuch CLI version mismatch"))\r
 175 >> +You may need to restart Emacs or upgrade your notmuch package."))\r
 176 >>     (t\r
 177 >> -    (notmuch-pop-up-error\r
 178 >> -     (concat\r
 179 >> -      (format "Error invoking notmuch.  %s exited with %s%s.\n"\r
 180 >> -          (mapconcat #'identity command " ")\r
 181 >> -          ;; Signal strings look like "Terminated", hence the\r
 182 >> -          ;; colon.\r
 183 >> -          (if (integerp exit-status) "status " "signal: ")\r
 184 >> -          exit-status)\r
 185 >> -      (when err-file\r
 186 >> -    (concat "Error:\n"\r
 187 >> -            (with-temp-buffer\r
 188 >> -              (insert-file-contents err-file)\r
 189 >> -              (if (eobp)\r
 190 >> -                  "(no error output)\n"\r
 191 >> -                (buffer-string)))))\r
 192 >> -      (when (and output (not (equal output "")))\r
 193 >> -    (format "Output:\n%s" output))))\r
 194 >> -    ;; Mimic `process-lines'\r
 195 >> -    (error "%s exited with status %s" (car command) exit-status))))\r
 196 >> +    (let ((err (when err-file\r
 197 >> +             (with-temp-buffer\r
 198 >> +               (insert-file-contents err-file)\r
 199 >> +               (unless (eobp)\r
 200 >> +                 (buffer-string)))))\r
 201 >> +      (basic-msg (format "%s exited with status %s"\r
 202 >> +                         (car command) exit-status)))\r
 203 >> +      (when (and (null err) (or (null output) (equal output "")))\r
 204 >> +    ;; We have no details to speak of.  Mimic `process-lines'.\r
 205 >\r
 206 > This means that if err and output are null we give a minimal error message and we\r
 207 > don't log the command line that fails. Perhaps the `when' clause could\r
 208 > be omitted so we get the extra information from below?\r
 209 \r
 210 Good point.  v2 basically follows your suggestion of removing the\r
 211 `when'.\r
 212 \r
 213 >> +    (notmuch-logged-error basic-msg))\r
 214 >> +      (let ((extra\r
 215 >> +         (concat\r
 216 >> +          "Command: " (mapconcat #'shell-quote-argument command " ") "\n"\r
 217 >> +          (if (integerp exit-status)\r
 218 >> +              (format "Exit status: %s\n" exit-status)\r
 219 >> +            (format "Exit signal: %s\n" exit-status))\r
 220 >> +          "Output:\n"\r
 221 >> +          (if (and output (not (equal output "")))\r
 222 >> +              output\r
 223 >> +            "(none)"))))\r
 224 >> +    (if err\r
 225 >> +        ;; We have an error message straight from the CLI.\r
 226 >> +        (notmuch-logged-error err extra)\r
 227 >> +      ;; We only have combined output from the CLI; don't inundate\r
 228 >> +      ;; the user with it.\r
 229 >> +      (notmuch-logged-error basic-msg extra)))))))\r
 230 >\r
 231 > Also, depending how the above gets changed, would it be worth pulling the let\r
 232 > clause before the cond clause, and subsuming some of the when/if/else\r
 233 > logic into the cond? This has the nice side effect that the reader\r
 234 > expects cond clauses to stop after the first match so the fact that\r
 235 > notmuch-check-exit-status signals an error would not matter when reading\r
 236 > this code.\r
 237 \r
 238 I think removing the `when' simplifies this enough.  I'd rather not lift\r
 239 the let outside the cond because the process of getting the error\r
 240 message is nontrivial and would be a waste in the common case of a\r
 241 success exit code.\r
 242 \r
 243 > I think a command line would be useful in almost all cases (in the error\r
 244 > buffer). If you decide to always supply that then your error message\r
 245 > might want tweaking as it would always have extra information in the\r
 246 > error buffer.\r
 247 \r
 248 I'm not too worried about always having the reference to the errors\r
 249 buffer.  My hope is that most cases will provide an error file (search\r
 250 is a notable exception and may be worth fixing), in which case it's\r
 251 going to provide that reference regardless.\r
 252 \r
 253 > Finally, and this is only a thought, I wonder if the mechanism can be\r
 254 > tweaked to provide debug information along these lines for all notmuch\r
 255 > commands whether or not they succeed: something like if\r
 256 > notmuch-debug-commands is set or there is an error? \r
 257 \r
 258 Sounds like a good follow-up patch.  Though currently the code is full\r
 259 of direct call-process and process-lines calls, so it would take a\r
 260 little (worthwhile) effort to consolidate these.\r
 261 \r
 262 > Incidentally do you have good ways to test this code (ie see what it\r
 263 > does in each case)? My hackish experiments suggested the async errors\r
 264 > were less useful than the sync ones but maybe that is just an inherent\r
 265 > limitation of the emacs async mechanisms.\r
 266 \r
 267 I'm not sure I can do much beyond what's in the patch.  v2 improves it a\r
 268 bit to be more thorough, so now both tests systematically collect the\r
 269 buffer, the errors buffer, and messages.  See what you think.\r
 270 \r
 271 Async errors are harder, since it's 2013 and Emacs still provides no\r
 272 means to separate stdout from stderr for async processes.  The official\r
 273 way to do this is to fire up a shell running the command and have the\r
 274 shell redirect stderr.  This may be worthwhile for search since it would\r
 275 give us better error messages and eliminate the crazy resynchronization\r
 276 we have to do to deal with errors embedded in the output, but that's for\r
 277 another patch.\r
 278 \r
 279 > Best wishes\r
 280 >\r
 281 > Mark\r
 282 >\r
 283 >\r
 284 >\r
 285 >\r
 286 >\r
 287 >\r
 288 >>  \r
 289 >>  (defun notmuch-call-notmuch-json (&rest args)\r
 290 >>    "Invoke `notmuch-command' with `args' and return the parsed JSON output.\r
 291 >> diff --git a/emacs/notmuch.el b/emacs/notmuch.el\r
 292 >> index 63387a2..c98a4fe 100644\r
 293 >> --- a/emacs/notmuch.el\r
 294 >> +++ b/emacs/notmuch.el\r
 295 >> @@ -654,11 +654,14 @@ of the result."\r
 296 >>                  ;; showing the search buffer\r
 297 >>                  (when (or (= exit-status 20) (= exit-status 21))\r
 298 >>                    (kill-buffer))\r
 299 >> -                (condition-case nil\r
 300 >> +                (condition-case err\r
 301 >>                      (notmuch-check-async-exit-status proc msg)\r
 302 >>                    ;; Suppress the error signal since strange\r
 303 >> -                  ;; things happen if a sentinel signals.\r
 304 >> -                  (error (throw 'return nil)))\r
 305 >> +                  ;; things happen if a sentinel signals.  Mimic\r
 306 >> +                  ;; the top-level's handling of error messages.\r
 307 >> +                  (error\r
 308 >> +                   (message "%s" (second err))\r
 309 >> +                   (throw 'return nil)))\r
 310 >>                  (if (and atbob\r
 311 >>                           (not (string= notmuch-search-target-thread "found")))\r
 312 >>                      (set 'never-found-target-thread t)))))\r
 313 >> diff --git a/test/emacs b/test/emacs\r
 314 >> index 6b18968..8e0a4fd 100755\r
 315 >> --- a/test/emacs\r
 316 >> +++ b/test/emacs\r
 317 >> @@ -862,18 +862,19 @@ exit 1\r
 318 >>  EOF\r
 319 >>  chmod a+x notmuch_fail\r
 320 >>  test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))\r
 321 >> +           (with-current-buffer \"*Messages*\" (erase-buffer))\r
 322 >>             (notmuch-search \"tag:inbox\")\r
 323 >>             (notmuch-test-wait)\r
 324 >> -           (test-output)\r
 325 >> -           (with-current-buffer \"*Notmuch errors*\"\r
 326 >> -              (test-output \"ERROR\")))"\r
 327 >> -test_expect_equal "$(cat OUTPUT ERROR)" "\\r
 328 >> +           (with-current-buffer \"*Messages*\"\r
 329 >> +              (test-output \"MESSAGES\"))\r
 330 >> +           (test-output))"\r
 331 >> +test_expect_equal "$(cat OUTPUT MESSAGES)" "\\r
 332 >>  Error: Unexpected output from notmuch search:\r
 333 >>  This is output\r
 334 >>  Error: Unexpected output from notmuch search:\r
 335 >>  This is an error\r
 336 >>  End of search results.\r
 337 >> -Error invoking notmuch.  $PWD/notmuch_fail search --format=json --format-version=1 --sort=newest-first tag:inbox exited with status 1."\r
 338 >> +$PWD/notmuch_fail exited with status 1"\r
 339 >>  \r
 340 >>  \r
 341 >>  test_done\r
 342 >> diff --git a/test/emacs-show b/test/emacs-show\r
 343 >> index ebf530b..ae9459d 100755\r
 344 >> --- a/test/emacs-show\r
 345 >> +++ b/test/emacs-show\r
 346 >> @@ -177,10 +177,12 @@ test_emacs "(let ((notmuch-command \"$PWD/notmuch_fail\"))\r
 347 >>             (test-output)\r
 348 >>             (with-current-buffer \"*Notmuch errors*\"\r
 349 >>                (test-output \"ERROR\")))"\r
 350 >> +sed -i -e 's/^\[.*\]$/[XXX]/' ERROR\r
 351 >>  test_expect_equal "$(cat OUTPUT ERROR)" "\\r
 352 >> -Error invoking notmuch.  $PWD/notmuch_fail show --format=json --format-version=1 --exclude=false ' * ' exited with status 1.\r
 353 >> -Error:\r
 354 >> +[XXX]\r
 355 >>  This is an error\r
 356 >> +Command: $PWD/notmuch_fail show --format\\=json --format-version\\=1 --exclude\\=false \\' \\* \\'\r
 357 >> +Exit status: 1\r
 358 >>  Output:\r
 359 >>  This is output"\r
 360 >>  \r
 361 >> -- \r
 362 >> 1.7.10.4\r