bug#30280: async-shell-command-display-buffer doesn't work anymore

["Basil L. Contovounesios" <contovob@tcd.ie>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Fix-deferred-display-of-async-shell-command-buffers.patch --]
[-- Type: text/x-diff, Size: 2474 bytes --]

From e55856682b1ff85e8743da04965e12a82e763689 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 30 Jan 2018 16:18:14 +0000
Subject: [PATCH] Fix deferred display of async shell-command buffers

* lisp/simple.el (shell-command): Display async shell buffer on
process output for every, not just first, command invocation.  Check
buffer liveness, not name, before displaying. (bug#30213, bug#30280)
---
 lisp/simple.el | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 3ac6b86381..e2deceb4c2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3547,14 +3547,20 @@ shell-command
 		  ;; carriage motion (see comint-inhibit-carriage-motion).
 		  (set-process-filter proc 'comint-output-filter)
                   (if async-shell-command-display-buffer
+                      ;; Display buffer immediately.
                       (display-buffer buffer '(nil (allow-no-window . t)))
-                    (add-function :before (process-filter proc)
-                                  (lambda (process _string)
-                                    (let ((buf (process-buffer process)))
-                                      (when (and (zerop (buffer-size buf))
-                                                 (string= (buffer-name buf)
-                                                          bname))
-                                        (display-buffer buf))))))))
+                    ;; Defer displaying buffer until first process output.
+                    ;; Use disposable named advice so that the buffer is
+                    ;; displayed at most once per process lifetime.
+                    (let ((nonce (make-symbol "nonce")))
+                      (add-function :before (process-filter proc)
+                                    (lambda (proc _string)
+                                      (let ((buf (process-buffer proc)))
+                                        (when (buffer-live-p buf)
+                                          (remove-function (process-filter proc)
+                                                           nonce)
+                                          (display-buffer buf))))
+                                    `((name . ,nonce)))))))
 	    ;; Otherwise, command is executed synchronously.
 	    (shell-command-on-region (point) (point) command
 				     output-buffer nil error-buffer)))))))
-- 
2.15.1


[-- Attachment #2: Type: text/plain, Size: 2683 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Juri Linkov <juri@linkov.net>
>> Date: Mon, 29 Jan 2018 00:20:30 +0200
>> 
>> 0. emacs -Q
>> 1. Eval: (setq async-shell-command-buffer 'new-buffer
>>                async-shell-command-display-buffer nil)
>> 2. M-& sleep 3600
>> 3. M-& sleep 3; echo "xyzzy"
>> 
>> At the step 2 “*Async Shell Command*” is not shown because of no output,
>> this is ok.
>> 
>> But at the step 3 “*Async Shell Command*<2>” is not shown after 3 seconds
>> when the output arrives, this is a regression.
>> 
>> It seems it is caused by commit#9f4f130 from bug#28997

Commit 9f4f130 indeed touches the relevant code, but the bug was
introduced along with the async-shell-command-display-buffer feature.

>> PS: Also it doesn't work with less official configuration:
>> 0. emacs -Q
>> 1. Eval: (setq async-shell-command-display-buffer nil)
>>          (add-hook 'shell-mode-hook 'rename-uniquely)
>> 2. M-& sleep 3600
>> 3. M-& sleep 3; echo "xyzzy"
>
> Thanks for reporting this.  Basil, can you take a look, please?

I am taking the liberty of CCing Reuben because I believe this report
can be merged with bug#30213.  As justification, a recap and correction
of my diagnosis and solution from that report follows.

The async-shell-command-display-buffer toggle determines whether the
output buffer is displayed immediately or in the process filter, as the
latter should only be executed on process output.  The guard in the
process filter, however, has always required that the buffer (a) be
empty; and (b) have the same name as the original output buffer.

I assume the reasoning behind (a) is that we only want the output buffer
to be displayed when the process first outputs something, and not every
time the process filter is called.  As mentioned in bug#30213, though,
the empty buffer check is The Wrong Thing when
shell-command-dont-erase-buffer is non-nil, i.e. when we want to reuse a
non-empty output buffer left behind by an old shell-command invocation.
AIUI, the process filter advice needs a way to distinguish between the
first time it receives process output and all its subsequent triggers,
irrespective of buffer contents.

I'm not entirely sure what the case for (b) is.  Usually process filters
need only check that their process buffer is live before acting on it;
surely that is also The Right Thing in this case?  The main issue with
(b) is that it does not take into account the various flavours of
async-shell-command-buffer under which the output buffer is renamed, as
demonstrated in this bug report.  I think the simplest fix for (b) would
be something like


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: foo.diff --]
[-- Type: text/x-diff, Size: 761 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 3ac6b86381..4f6708dbc1 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3552,8 +3552,7 @@ shell-command
                                   (lambda (process _string)
                                     (let ((buf (process-buffer process)))
                                       (when (and (zerop (buffer-size buf))
-                                                 (string= (buffer-name buf)
-                                                          bname))
+                                                 (eq buf buffer))
                                         (display-buffer buf))))))))
 	    ;; Otherwise, command is executed synchronously.
 	    (shell-command-on-region (point) (point) command

[-- Attachment #4: Type: text/plain, Size: 825 bytes --]


but I still don't see why we are forgoing a liveness check in favour of
this.

I attach a patch which addresses both bugs.  Its solution for (a) is to
make the advice disposable, i.e. it removes itself from the process
filter after it has fulfilled its purpose of displaying the output
buffer.  A syntactically simpler implementation of this could use a
plain boolean switch instead of removing the advice, but IMO the latter
is semantically more sound (and possibly more performant in subsequent
invocations of the process filter, though this should be irrelevant).

WDYT?

P.S. Would patch(es) for aesthetic changes to the rest of shell-command
(such as removing redundant variables, inverting the condition of the
massive if-then-else to reduce indentation, etc.) be welcome?  If so,
where should I send them?

-- 
Basil