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

[Basil L. Contovounesios]

0001-Fix-deferred-display-of-async-shell-command-buffers.patch
Description: Text Data

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

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

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