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

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

[Juri Linkov]

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

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"

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

[Eli Zaretskii]

> 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
> 
> 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?

P.S. Juri, in the future, could you please make sure the version of
Emacs where you discovered this appears in the bug report, either in
the Subject or in the body?

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

[Juri Linkov]

> P.S. Juri, in the future, could you please make sure the version of
> Emacs where you discovered this appears in the bug report, either in
> the Subject or in the body?

The problem is that I don't know what version to report:
I discover these bugs in 27.0 in master, but I believe the same
bugs exist also in 26.0, and should be fixed in the release branch.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,  30280@debbugs.gnu.org
> Date: Mon, 29 Jan 2018 23:40:52 +0200
> 
> > P.S. Juri, in the future, could you please make sure the version of
> > Emacs where you discovered this appears in the bug report, either in
> > the Subject or in the body?
> 
> The problem is that I don't know what version to report:
> I discover these bugs in 27.0 in master, but I believe the same
> bugs exist also in 26.0, and should be fixed in the release branch.

To avoid the dilemma, how about if you run the release branch from now
until it is released?

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

[Basil L. Contovounesios]

[-- 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

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

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

2018-01-30 18:53 GMT+00:00 Basil L. Contovounesios <contovob@tcd.ie>:

>
> 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 think you're right that (b) is not necessary. I think I did it that way
originally because I was doing everything with the buffer name, so really
it was just an oversight.​

-- 
https://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 1675 bytes --]

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

[Juri Linkov]

> 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?

Thanks, I confirm this is the right thing to do and your patch fixes
the reported issue.

> 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?

Such changes are always welcome as patches separate from the actual fixes,
i.e. when not amalgamating bug fixes and code beautification/refactoring
in the same patch.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Reuben Thomas <rrt@sc3d.org>,  <30280@debbugs.gnu.org>
> Date: Wed, 31 Jan 2018 23:44:43 +0200
> 
> > 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?
> 
> Thanks, I confirm this is the right thing to do and your patch fixes
> the reported issue.

Then please push them to the emacs-26 branch, and thanks.

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

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-lisp-simple.el-async-shell-command-shell-command-Fix.patch --]
[-- Type: text/x-diff, Size: 1916 bytes --]

From b5167407b464a13968af600ac3d09f13d52e6d26 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 3 Feb 2018 13:52:03 +0000
Subject: * lisp/simple.el (async-shell-command, shell-command): Fix grammar

---
 lisp/simple.el | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index e2deceb4c2..b7ad6ebd79 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3351,15 +3351,15 @@ async-shell-command
 The output appears in the buffer `*Async Shell Command*'.
 That buffer is in shell mode.
 
-You can configure `async-shell-command-buffer' to specify what to do in
-case when `*Async Shell Command*' buffer is already taken by another
+You can configure `async-shell-command-buffer' to specify what to do
+when the `*Async Shell Command*' buffer is already taken by another
 running shell command.  To run COMMAND without displaying the output
 in a window you can configure `display-buffer-alist' to use the action
 `display-buffer-no-window' for the buffer `*Async Shell Command*'.
 
 In Elisp, you will often be better served by calling `start-process'
-directly, since it offers more control and does not impose the use of a
-shell (with its need to quote arguments)."
+directly, since it offers more control and does not impose the use of
+a shell (with its need to quote arguments)."
   (interactive
    (list
     (read-shell-command "Async shell command: " nil nil
@@ -3428,8 +3428,8 @@ shell-command
 specifies the value of ERROR-BUFFER.
 
 In Elisp, you will often be better served by calling `call-process' or
-`start-process' directly, since it offers more control and does not impose
-the use of a shell (with its need to quote arguments)."
+`start-process' directly, since they offer more control and do not
+impose the use of a shell (with its need to quote arguments)."
 
   (interactive
    (list
-- 
2.15.1


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


Eli Zaretskii <eliz@gnu.org> writes:

>> Thanks, I confirm this is the right thing to do and your patch fixes
>> the reported issue.
>
> Then please push them to the emacs-26 branch, and thanks.

Perhaps I can sneak in the attached doc fixes as well.

-- 
Basil

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

[Juri Linkov]

>>> Thanks, I confirm this is the right thing to do and your patch fixes
>>> the reported issue.
>>
>> Then please push them to the emacs-26 branch, and thanks.

Done.

> Perhaps I can sneak in the attached doc fixes as well.

Thanks, this is pushed as well.

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

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Fix-failing-test-for-bug-30280.patch --]
[-- Type: text/x-diff, Size: 3786 bytes --]

From 871cc1bd0b4ef13b759814e4f32a644463e887d7 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Fri, 20 Apr 2018 15:45:06 +0100
Subject: [PATCH 1/2] Fix failing test for bug#30280

* test/lisp/simple-tests.el
(simple-tests-async-shell-command-30280): Fix failing test.
---
 test/lisp/simple-tests.el | 59 ++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index 7a10df2058..678d9b9385 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -521,30 +521,51 @@ simple-test-undo-with-switched-buffer
     (do-auto-fill)
     (should (string-equal (buffer-string) "foo bar"))))
 
+\f
+;;; Shell command.
+
 (ert-deftest simple-tests-async-shell-command-30280 ()
   "Test for https://debbugs.gnu.org/30280 ."
-  :expected-result :failed
   (let* ((async-shell-command-buffer 'new-buffer)
          (async-shell-command-display-buffer nil)
-         (str "*Async Shell Command*")
-         (buffers-name
-          (cl-loop repeat 2
-                   collect (buffer-name
-                            (generate-new-buffer str))))
+         (base "name")
+         (first (buffer-name (generate-new-buffer base)))
+         (second (generate-new-buffer-name base))
+         ;; `save-window-excursion' doesn't restore frame configurations.
+         (pop-up-frames nil)
          (inhibit-message t))
-    (mapc #'kill-buffer buffers-name)
-    (async-shell-command
-     (format "%s -Q -batch -eval '(progn (sleep-for 3600) (message \"foo\"))'"
-             invocation-name))
-    (async-shell-command
-     (format "%s -Q -batch -eval '(progn (sleep-for 1) (message \"bar\"))'"
-             invocation-name))
-    (let ((buffers (mapcar #'get-buffer buffers-name))
-          (processes (mapcar #'get-buffer-process buffers-name)))
-      (unwind-protect
-          (should (memq (cadr buffers) (mapcar #'window-buffer (window-list))))
-        (mapc #'delete-process processes)
-        (mapc #'kill-buffer buffers)))))
+    ;; Let `shell-command' create the buffer as needed.
+    (kill-buffer first)
+    (unwind-protect
+        (save-window-excursion
+          ;; One command has no output, the other does.
+          ;; Removing the -eval argument also yields no output, but
+          ;; then both commands exit simultaneously when
+          ;; `accept-process-output' is called on the second command.
+          (dolist (form '("(sleep-for 8)" "(message \"\")"))
+            (async-shell-command (format "%s -Q -batch -eval '%s'"
+                                         invocation-name form)
+                                 first))
+          ;; First command should neither have nor display output.
+          (let* ((buffer (get-buffer first))
+                 (process (get-buffer-process buffer)))
+            (should (buffer-live-p buffer))
+            (should process)
+            (should (zerop (buffer-size buffer)))
+            (should (not (get-buffer-window buffer))))
+          ;; Second command should both have and display output.
+          (let* ((buffer (get-buffer second))
+                 (process (get-buffer-process buffer)))
+            (should (buffer-live-p buffer))
+            (should process)
+            (should (accept-process-output process 4 nil t))
+            (should (> (buffer-size buffer) 0))
+            (should (get-buffer-window buffer))))
+      (dolist (name (list first second))
+        (let* ((buffer (get-buffer name))
+               (process (and buffer (get-buffer-process buffer))))
+          (when process (delete-process process))
+          (when buffer (kill-buffer buffer)))))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Minor-shell-command-simplifications.patch --]
[-- Type: text/x-diff, Size: 7049 bytes --]

From 8743148a16480f12923dbaecdefbc641b64d7f0a Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 6 May 2018 16:41:01 +0100
Subject: [PATCH 2/2] Minor shell-command simplifications

* lisp/simple.el (shell-command): Use call-process-shell-command,
start-process-shell-command, and file-attribute-size.  Keep track of
output-buffer only by its object, not by its name. (bug#30280)
---
 lisp/simple.el | 72 +++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 39 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index a0a6898e17..7958a3b134 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3400,6 +3400,8 @@ async-shell-command
     (setq command (concat command " &")))
   (shell-command command output-buffer error-buffer))
 
+(declare-function comint-output-filter "comint" (process string))
+
 (defun shell-command (command &optional output-buffer error-buffer)
   "Execute string COMMAND in inferior shell; display output, if any.
 With prefix argument, insert the COMMAND's output at point.
@@ -3477,12 +3479,11 @@ shell-command
 	       (not (or (bufferp output-buffer)  (stringp output-buffer))))
 	  ;; Output goes in current buffer.
 	  (let ((error-file
-		 (if error-buffer
-		     (make-temp-file
-		      (expand-file-name "scor"
-					(or small-temporary-file-directory
-					    temporary-file-directory)))
-		   nil)))
+                 (and error-buffer
+                      (make-temp-file
+                       (expand-file-name "scor"
+                                         (or small-temporary-file-directory
+                                             temporary-file-directory))))))
 	    (barf-if-buffer-read-only)
 	    (push-mark nil t)
 	    ;; We do not use -f for csh; we will not support broken use of
@@ -3490,24 +3491,22 @@ shell-command
 	    ;; "if ($?prompt) exit" before things which are not useful
 	    ;; non-interactively.  Besides, if someone wants their other
 	    ;; aliases for shell commands then they can still have them.
-	    (call-process shell-file-name nil
-			  (if error-file
-			      (list t error-file)
-			    t)
-			  nil shell-command-switch command)
+            (call-process-shell-command command nil (if error-file
+                                                        (list t error-file)
+                                                      t))
 	    (when (and error-file (file-exists-p error-file))
-	      (if (< 0 (nth 7 (file-attributes error-file)))
-		  (with-current-buffer (get-buffer-create error-buffer)
-		    (let ((pos-from-end (- (point-max) (point))))
-		      (or (bobp)
-			  (insert "\f\n"))
-		      ;; Do no formatting while reading error file,
-		      ;; because that can run a shell command, and we
-		      ;; don't want that to cause an infinite recursion.
-		      (format-insert-file error-file nil)
-		      ;; Put point after the inserted errors.
-		      (goto-char (- (point-max) pos-from-end)))
-		    (display-buffer (current-buffer))))
+              (when (< 0 (file-attribute-size (file-attributes error-file)))
+                (with-current-buffer (get-buffer-create error-buffer)
+                  (let ((pos-from-end (- (point-max) (point))))
+                    (or (bobp)
+                        (insert "\f\n"))
+                    ;; Do no formatting while reading error file,
+                    ;; because that can run a shell command, and we
+                    ;; don't want that to cause an infinite recursion.
+                    (format-insert-file error-file nil)
+                    ;; Put point after the inserted errors.
+                    (goto-char (- (point-max) pos-from-end)))
+                  (display-buffer (current-buffer))))
 	      (delete-file error-file))
 	    ;; This is like exchange-point-and-mark, but doesn't
 	    ;; activate the mark.  It is cleaner to avoid activation,
@@ -3525,13 +3524,11 @@ shell-command
 	      ;; Command ending with ampersand means asynchronous.
               (let* ((buffer (get-buffer-create
                               (or output-buffer "*Async Shell Command*")))
-                     (bname (buffer-name buffer))
-                     (directory default-directory)
-                     proc)
+                     (proc (get-buffer-process buffer))
+                     (directory default-directory))
 		;; Remove the ampersand.
 		(setq command (substring command 0 (match-beginning 0)))
 		;; Ask the user what to do with already running process.
-		(setq proc (get-buffer-process buffer))
 		(when proc
 		  (cond
 		   ((eq async-shell-command-buffer 'confirm-kill-process)
@@ -3542,35 +3539,32 @@ shell-command
 		   ((eq async-shell-command-buffer 'confirm-new-buffer)
 		    ;; If will create a new buffer, query first.
 		    (if (yes-or-no-p "A command is running in the default buffer.  Use a new buffer? ")
-                        (setq buffer (generate-new-buffer bname))
+                        (setq buffer (generate-new-buffer (buffer-name buffer)))
 		      (error "Shell command in progress")))
 		   ((eq async-shell-command-buffer 'new-buffer)
 		    ;; It will create a new buffer.
-                    (setq buffer (generate-new-buffer bname)))
+                    (setq buffer (generate-new-buffer (buffer-name buffer))))
 		   ((eq async-shell-command-buffer 'confirm-rename-buffer)
 		    ;; If will rename the buffer, query first.
 		    (if (yes-or-no-p "A command is running in the default buffer.  Rename it? ")
-			(progn
-			  (with-current-buffer buffer
-			    (rename-uniquely))
-                          (setq buffer (get-buffer-create bname)))
+                        (with-current-buffer buffer
+                          (rename-uniquely))
 		      (error "Shell command in progress")))
 		   ((eq async-shell-command-buffer 'rename-buffer)
 		    ;; It will rename the buffer.
 		    (with-current-buffer buffer
-		      (rename-uniquely))
-                    (setq buffer (get-buffer-create bname)))))
+                      (rename-uniquely)))))
 		(with-current-buffer buffer
                   (shell-command--save-pos-or-erase)
 		  (setq default-directory directory)
-		  (setq proc (start-process "Shell" buffer shell-file-name
-					    shell-command-switch command))
+                  (setq proc
+                        (start-process-shell-command "Shell" buffer command))
 		  (setq mode-line-process '(":%s"))
 		  (require 'shell) (shell-mode)
-		  (set-process-sentinel proc 'shell-command-sentinel)
+                  (set-process-sentinel proc #'shell-command-sentinel)
 		  ;; Use the comint filter for proper handling of
 		  ;; carriage motion (see comint-inhibit-carriage-motion).
-		  (set-process-filter proc 'comint-output-filter)
+                  (set-process-filter proc #'comint-output-filter)
                   (if async-shell-command-display-buffer
                       ;; Display buffer immediately.
                       (display-buffer buffer '(nil (allow-no-window . t)))
-- 
2.17.0


[-- Attachment #3: Type: text/plain, Size: 547 bytes --]


I recently noticed that a test for an expected failure was added around
the time of this bug report[1: ea8c0e1b9e].

[1: ea8c0e1b9e]: 2018-01-29 22:31:50 +0900
  * test/lisp/simple-tests.el (simple-tests-async-shell-command-30280): Add test
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ea8c0e1b9eaa6651919fb4e039e3fcb5a1fa73db

I attach two patches.  The first tries to make this test succeed in
accordance with the resulting bugfix.  The second suggests some
simplifications to the logic in shell-command.  WDYT?

Thanks,

-- 
Basil

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

[Tino Calancha]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I attach two patches.  The first tries to make this test succeed in
> accordance with the resulting bugfix.  The second suggests some
> simplifications to the logic in shell-command.  WDYT?

Basil, thank you for fixing that test!

Concerning the 2nd patch; it adds a bug when
`async-shell-command-buffer' is
confirm-rename-buffer
;; or
rename-buffer

Following is a recipe:

emacs -Q -eval "(setq async-shell-command-buffer 'rename-buffer)" \
-eval '(async-shell-command "while true; do echo foo; sleep 3;done")' \
-eval '(async-shell-command "while true; do echo bar; sleep 3;done")'

;; Now execute the following form:
(cdr 
  (delq nil
     (mapcar (lambda (b) (if (string-match "\*Async Shell Command\*" (buffer-name b)) (buffer-name b)))(buffer-list))))
=> nil
;; It shouldn't be nil: there must be 2 buffers matching '*Async Shell Command*'.
;; Here we are using 1 buffer for 2 shell processes.

;; The original code binds bname below so that we can create the new
;; buffer with the proper name:
(let* ((buffer (get-buffer-create
                (or output-buffer "*Async Shell Command*")))
       (bname (buffer-name buffer))
       (directory default-directory)
       proc)

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

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Fix-failing-test-for-bug-30280.patch --]
[-- Type: text/x-diff, Size: 3786 bytes --]

From 77f70d788631c57823ab18fe7ae1b58f2583b400 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Fri, 20 Apr 2018 15:45:06 +0100
Subject: [PATCH 1/2] Fix failing test for bug#30280

* test/lisp/simple-tests.el
(simple-tests-async-shell-command-30280): Fix failing test.
---
 test/lisp/simple-tests.el | 59 ++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index 7a10df2058..678d9b9385 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -521,30 +521,51 @@ simple-test-undo-with-switched-buffer
     (do-auto-fill)
     (should (string-equal (buffer-string) "foo bar"))))
 
+\f
+;;; Shell command.
+
 (ert-deftest simple-tests-async-shell-command-30280 ()
   "Test for https://debbugs.gnu.org/30280 ."
-  :expected-result :failed
   (let* ((async-shell-command-buffer 'new-buffer)
          (async-shell-command-display-buffer nil)
-         (str "*Async Shell Command*")
-         (buffers-name
-          (cl-loop repeat 2
-                   collect (buffer-name
-                            (generate-new-buffer str))))
+         (base "name")
+         (first (buffer-name (generate-new-buffer base)))
+         (second (generate-new-buffer-name base))
+         ;; `save-window-excursion' doesn't restore frame configurations.
+         (pop-up-frames nil)
          (inhibit-message t))
-    (mapc #'kill-buffer buffers-name)
-    (async-shell-command
-     (format "%s -Q -batch -eval '(progn (sleep-for 3600) (message \"foo\"))'"
-             invocation-name))
-    (async-shell-command
-     (format "%s -Q -batch -eval '(progn (sleep-for 1) (message \"bar\"))'"
-             invocation-name))
-    (let ((buffers (mapcar #'get-buffer buffers-name))
-          (processes (mapcar #'get-buffer-process buffers-name)))
-      (unwind-protect
-          (should (memq (cadr buffers) (mapcar #'window-buffer (window-list))))
-        (mapc #'delete-process processes)
-        (mapc #'kill-buffer buffers)))))
+    ;; Let `shell-command' create the buffer as needed.
+    (kill-buffer first)
+    (unwind-protect
+        (save-window-excursion
+          ;; One command has no output, the other does.
+          ;; Removing the -eval argument also yields no output, but
+          ;; then both commands exit simultaneously when
+          ;; `accept-process-output' is called on the second command.
+          (dolist (form '("(sleep-for 8)" "(message \"\")"))
+            (async-shell-command (format "%s -Q -batch -eval '%s'"
+                                         invocation-name form)
+                                 first))
+          ;; First command should neither have nor display output.
+          (let* ((buffer (get-buffer first))
+                 (process (get-buffer-process buffer)))
+            (should (buffer-live-p buffer))
+            (should process)
+            (should (zerop (buffer-size buffer)))
+            (should (not (get-buffer-window buffer))))
+          ;; Second command should both have and display output.
+          (let* ((buffer (get-buffer second))
+                 (process (get-buffer-process buffer)))
+            (should (buffer-live-p buffer))
+            (should process)
+            (should (accept-process-output process 4 nil t))
+            (should (> (buffer-size buffer) 0))
+            (should (get-buffer-window buffer))))
+      (dolist (name (list first second))
+        (let* ((buffer (get-buffer name))
+               (process (and buffer (get-buffer-process buffer))))
+          (when process (delete-process process))
+          (when buffer (kill-buffer buffer)))))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Minor-shell-command-simplifications.patch --]
[-- Type: text/x-diff, Size: 5546 bytes --]

From ad179cfbcbe81d8dd98e8d0dc1aeeeabf3f7aee8 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 9 May 2018 12:47:43 +0100
Subject: [PATCH 2/2] Minor shell-command simplifications

* lisp/simple.el (shell-command): Use call-process-shell-command,
start-process-shell-command, and file-attribute-size. (bug#30280)
---
 lisp/simple.el | 58 ++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index a0a6898e17..57e70a8d15 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3400,6 +3400,8 @@ async-shell-command
     (setq command (concat command " &")))
   (shell-command command output-buffer error-buffer))
 
+(declare-function comint-output-filter "comint" (process string))
+
 (defun shell-command (command &optional output-buffer error-buffer)
   "Execute string COMMAND in inferior shell; display output, if any.
 With prefix argument, insert the COMMAND's output at point.
@@ -3477,12 +3479,11 @@ shell-command
 	       (not (or (bufferp output-buffer)  (stringp output-buffer))))
 	  ;; Output goes in current buffer.
 	  (let ((error-file
-		 (if error-buffer
-		     (make-temp-file
-		      (expand-file-name "scor"
-					(or small-temporary-file-directory
-					    temporary-file-directory)))
-		   nil)))
+                 (and error-buffer
+                      (make-temp-file
+                       (expand-file-name "scor"
+                                         (or small-temporary-file-directory
+                                             temporary-file-directory))))))
 	    (barf-if-buffer-read-only)
 	    (push-mark nil t)
 	    ;; We do not use -f for csh; we will not support broken use of
@@ -3490,24 +3491,22 @@ shell-command
 	    ;; "if ($?prompt) exit" before things which are not useful
 	    ;; non-interactively.  Besides, if someone wants their other
 	    ;; aliases for shell commands then they can still have them.
-	    (call-process shell-file-name nil
-			  (if error-file
-			      (list t error-file)
-			    t)
-			  nil shell-command-switch command)
+            (call-process-shell-command command nil (if error-file
+                                                        (list t error-file)
+                                                      t))
 	    (when (and error-file (file-exists-p error-file))
-	      (if (< 0 (nth 7 (file-attributes error-file)))
-		  (with-current-buffer (get-buffer-create error-buffer)
-		    (let ((pos-from-end (- (point-max) (point))))
-		      (or (bobp)
-			  (insert "\f\n"))
-		      ;; Do no formatting while reading error file,
-		      ;; because that can run a shell command, and we
-		      ;; don't want that to cause an infinite recursion.
-		      (format-insert-file error-file nil)
-		      ;; Put point after the inserted errors.
-		      (goto-char (- (point-max) pos-from-end)))
-		    (display-buffer (current-buffer))))
+              (when (< 0 (file-attribute-size (file-attributes error-file)))
+                (with-current-buffer (get-buffer-create error-buffer)
+                  (let ((pos-from-end (- (point-max) (point))))
+                    (or (bobp)
+                        (insert "\f\n"))
+                    ;; Do no formatting while reading error file,
+                    ;; because that can run a shell command, and we
+                    ;; don't want that to cause an infinite recursion.
+                    (format-insert-file error-file nil)
+                    ;; Put point after the inserted errors.
+                    (goto-char (- (point-max) pos-from-end)))
+                  (display-buffer (current-buffer))))
 	      (delete-file error-file))
 	    ;; This is like exchange-point-and-mark, but doesn't
 	    ;; activate the mark.  It is cleaner to avoid activation,
@@ -3526,12 +3525,11 @@ shell-command
               (let* ((buffer (get-buffer-create
                               (or output-buffer "*Async Shell Command*")))
                      (bname (buffer-name buffer))
-                     (directory default-directory)
-                     proc)
+                     (proc (get-buffer-process buffer))
+                     (directory default-directory))
 		;; Remove the ampersand.
 		(setq command (substring command 0 (match-beginning 0)))
 		;; Ask the user what to do with already running process.
-		(setq proc (get-buffer-process buffer))
 		(when proc
 		  (cond
 		   ((eq async-shell-command-buffer 'confirm-kill-process)
@@ -3563,14 +3561,14 @@ shell-command
 		(with-current-buffer buffer
                   (shell-command--save-pos-or-erase)
 		  (setq default-directory directory)
-		  (setq proc (start-process "Shell" buffer shell-file-name
-					    shell-command-switch command))
+                  (setq proc
+                        (start-process-shell-command "Shell" buffer command))
 		  (setq mode-line-process '(":%s"))
 		  (require 'shell) (shell-mode)
-		  (set-process-sentinel proc 'shell-command-sentinel)
+                  (set-process-sentinel proc #'shell-command-sentinel)
 		  ;; Use the comint filter for proper handling of
 		  ;; carriage motion (see comint-inhibit-carriage-motion).
-		  (set-process-filter proc 'comint-output-filter)
+                  (set-process-filter proc #'comint-output-filter)
                   (if async-shell-command-display-buffer
                       ;; Display buffer immediately.
                       (display-buffer buffer '(nil (allow-no-window . t)))
-- 
2.17.0


[-- Attachment #3: Type: text/plain, Size: 616 bytes --]


Tino Calancha <tino.calancha@gmail.com> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> I attach two patches.  The first tries to make this test succeed in
>> accordance with the resulting bugfix.  The second suggests some
>> simplifications to the logic in shell-command.  WDYT?
>
> Basil, thank you for fixing that test!
>
> Concerning the 2nd patch; it adds a bug when
> `async-shell-command-buffer' is
> confirm-rename-buffer
> ;; or
> rename-buffer

Right, that was silly of me; thanks for catching it.
I reattach the patches with the buffer name changes removed.

Thanks again,

-- 
Basil

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

[Tino Calancha]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I reattach the patches with the buffer name changes removed.
Thank you Basil!

IMHO this patch looks OK.
I have two minor comments.

I)
> +(declare-function comint-output-filter "comint" (process string))
> +
What is the purpose of this? AFICT no warning is shown when compiling
the file.
* We require `shell.el' inside `shell-coomand'.
* `shell.el' requires `comint.el'.
Is the purpose to serve as documentation? In that case I don't think we
need it (the prefix 'comint-' already makes obvious where this function
belongs to).

II)
It's better to keep consistent with the indentation of the function you
are modifying:  here, `shell-command' is indenting with TAB.

Tip:
You can see the tabs searching them with:
C-s C-q C-I
or you can persistenly highlight them with:
M-s h r C-I RET RET

For instance, here you are changing:
1) ' ---> #'
;; and
2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)

Please, do not change 2).
>  		  ;; Use the comint filter for proper handling of
>  		  ;; carriage motion (see comint-inhibit-carriage-motion).
> -		  (set-process-filter proc 'comint-output-filter)
> +                  (set-process-filter proc #'comint-output-filter)

>                    (if async-shell-command-display-buffer
>                        ;; Display buffer immediately.
>                        (display-buffer buffer '(nil (allow-no-window . t)))

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

[Noam Postavsky]

On 9 May 2018 at 09:57, Tino Calancha <tino.calancha@gmail.com> wrote:

> II)
> It's better to keep consistent with the indentation of the function you
> are modifying

> For instance, here you are changing:
> 1) ' ---> #'
> ;; and
> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)

AFAIK, current policy is that it's fine to change tabs to spaces when
the line is being modified anyway.

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

[Tino Calancha]

On Wed, 9 May 2018, Noam Postavsky wrote:

> On 9 May 2018 at 09:57, Tino Calancha <tino.calancha@gmail.com> wrote:
>
>> II)
>> It's better to keep consistent with the indentation of the function you
>> are modifying
>
>> For instance, here you are changing:
>> 1) ' ---> #'
>> ;; and
>> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)
>
> AFAIK, current policy is that it's fine to change tabs to spaces when
> the line is being modified anyway.
Thank you.  I didn't know it.
IMO, not very nice to make a 'Frankestein' indented function, but ... OK.

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

[Basil L. Contovounesios]

Tino Calancha <tino.calancha@gmail.com> writes:

> I have two minor comments.
>
> I)
>> +(declare-function comint-output-filter "comint" (process string))
>> +
> What is the purpose of this? AFICT no warning is shown when compiling
> the file.

On my end, removing the function declaration and invoking 'make' results
in the following output:

...
make[2]: Entering directory '/home/blc/.local/src/emacs/lisp'
  ELC      ../lisp/simple.elc
Reloading stale loaddefs.el
Loading /home/blc/.local/src/emacs/lisp/loaddefs.el (source)...

In end of data:
simple.el:9030:1:Warning: the function ‘comint-output-filter’ is not known to
    be defined.
...

Note that this warning is only emitted when comint-output-filter is
#'-quoted.  This is in line with the usual behaviour of the
byte-compiler that I am accustomed to, i.e. I don't see anything out of
the ordinary here.

> * We require `shell.el' inside `shell-coomand'.
> * `shell.el' requires `comint.el'.

Yes, I understand that using comint-output-filter at this point in the
program is kosher, but the byte-compiler evidently does not.

> Is the purpose to serve as documentation? In that case I don't think we
> need it (the prefix 'comint-' already makes obvious where this function
> belongs to).

No, the only intention is to pacify the byte-compiler.

> II)
> It's better to keep consistent with the indentation of the function you
> are modifying:  here, `shell-command' is indenting with TAB.
>
> Tip:
> You can see the tabs searching them with:
> C-s C-q C-I
> or you can persistenly highlight them with:
> M-s h r C-I RET RET

Thanks for the tip.  I personally prefer to use [global-]whitespace-mode
with a whitespace-style setting which includes (face tab-mark).
I additionally avoid accidentally committing tabs by configuring the git
option core.whitespace to include tab-in-indent.

> For instance, here you are changing:
> 1) ' ---> #'
> ;; and
> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)
>
> Please, do not change 2).

I have no strong feeling on this; I was merely going along with the
(emacs-lisp-mode . ((indent-tabs-mode . nil))) setting in the project's
toplevel dir-locals-file, as well what I had inferred to be accepted
policy (as Noam mentions in a separate email) from following Emacs
development for the last couple of years.

If it weren't for the above and the fact that most everything I have
come across in the Emacs tree has Frankindentation (including the target
function shell-command), I would be more inclined to remain consistent
with the surrounding source.

Let me know if it's still a problem and I'll gladly resend the patches
with indent-tabs-mode enabled.

Thanks again for your help and feedback,

-- 
Basil

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

[Tino Calancha]

[-- Attachment #1: Type: text/plain, Size: 3215 bytes --]


Thank you Basil for your prompt response!

Your patches LGTM.
Let's wait few more days if some people want to make further comments.
Otherwise, I will push them in your name into the master branch.

On Wed, 9 May 2018, Basil L. Contovounesios wrote:

>> I)
>>> +(declare-function comint-output-filter "comint" (process string))
>>> +
>> What is the purpose of this? AFICT no warning is shown when compiling
>> the file.
>
> On my end, removing the function declaration and invoking 'make' results
> in the following output:
>
> ...
> make[2]: Entering directory '/home/blc/.local/src/emacs/lisp'
>  ELC      ../lisp/simple.elc
> Reloading stale loaddefs.el
> Loading /home/blc/.local/src/emacs/lisp/loaddefs.el (source)...
>
> In end of data:
> simple.el:9030:1:Warning: the function ‘comint-output-filter’ is not known to
>    be defined.
> ...
>
> Note that this warning is only emitted when comint-output-filter is
> #'-quoted.  This is in line with the usual behaviour of the
> byte-compiler that I am accustomed to, i.e. I don't see anything out of
> the ordinary here.
Oh, I see.  I overlooked the fact that we changed `quote' with `function'.
With:
make bootstrap
there is no warning.
I am OK with clearing the warning in all cases.


>> * We require `shell.el' inside `shell-coomand'.
>> * `shell.el' requires `comint.el'.
>
> Yes, I understand that using comint-output-filter at this point in the
> program is kosher, but the byte-compiler evidently does not.
>
>> Is the purpose to serve as documentation? In that case I don't think we
>> need it (the prefix 'comint-' already makes obvious where this function
>> belongs to).
>
> No, the only intention is to pacify the byte-compiler.
>
>> II)
>> It's better to keep consistent with the indentation of the function you
>> are modifying:  here, `shell-command' is indenting with TAB.
>>
>> Tip:
>> You can see the tabs searching them with:
>> C-s C-q C-I
>> or you can persistenly highlight them with:
>> M-s h r C-I RET RET
>
> Thanks for the tip.  I personally prefer to use [global-]whitespace-mode
> with a whitespace-style setting which includes (face tab-mark).
> I additionally avoid accidentally committing tabs by configuring the git
> option core.whitespace to include tab-in-indent.
>
>> For instance, here you are changing:
>> 1) ' ---> #'
>> ;; and
>> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)
>>
>> Please, do not change 2).
>
> I have no strong feeling on this; I was merely going along with the
> (emacs-lisp-mode . ((indent-tabs-mode . nil))) setting in the project's
> toplevel dir-locals-file, as well what I had inferred to be accepted
> policy (as Noam mentions in a separate email) from following Emacs
> development for the last couple of years.
>
> If it weren't for the above and the fact that most everything I have
> come across in the Emacs tree has Frankindentation (including the target
> function shell-command), I would be more inclined to remain consistent
> with the surrounding source.
>
> Let me know if it's still a problem and I'll gladly resend the patches
> with indent-tabs-mode enabled.
It's OK, Frankenstein is a glorious novel after all; I recommend anyone to 
revisit such classical masterpiece :-)