bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Tino Calancha]

emacs -Q -eval "(setq async-shell-command-buffer 'new-buffer)"

I) M-&: i=0; while true; do echo $i; i=$((i+1)); sleep 1;done RET
;; Repeating the command will not prompt for confirmation
II) M-&: M-p RET ; create buffer "*Async Shell Command*<2>".
III) M-:(let ((dir (expand-file-name default-directory)))
           (find-file (concat "/sudo::") dir)) RET
IV) M-&: M-p RET
;; Received prompt to kill buffer "*Async Shell Command*".

In IV) the machine is the same as before and the original user
still is the owner of the Emacs session: i would expect IV) create
the buffer "*Async Shell Command*<3>" without any prompt.
Even if i set `async-shell-command-buffer' to 'new-buffer in
/root/.emacs i receive the prompt.


In GNU Emacs 25.1.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.21.5)
  of 2016-09-08
Repository revision: ba5d32398ba42fcf14ad5242d95c68133981744f

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Tino Calancha]

On Thu, 8 Sep 2016, Tino Calancha wrote:

> III) M-:(let ((dir (expand-file-name default-directory)))
>          (find-file (concat "/sudo::") dir)) RET
There is a typo: It should read as follows:
III) M-:(let ((dir (expand-file-name default-directory)))
           (find-file (concat "/sudo::" dir))) RET

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Michael Albinus]

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

Hi Tino,

> emacs -Q -eval "(setq async-shell-command-buffer 'new-buffer)"
>
> I) M-&: i=0; while true; do echo $i; i=$((i+1)); sleep 1;done RET
> ;; Repeating the command will not prompt for confirmation
> II) M-&: M-p RET ; create buffer "*Async Shell Command*<2>".
> III) M-:(let ((dir (expand-file-name default-directory)))
>           (find-file (concat "/sudo::") dir)) RET
> IV) M-&: M-p RET
> ;; Received prompt to kill buffer "*Async Shell Command*".
>
> In IV) the machine is the same as before and the original user
> still is the owner of the Emacs session: i would expect IV) create
> the buffer "*Async Shell Command*<3>" without any prompt.
> Even if i set `async-shell-command-buffer' to 'new-buffer in
> /root/.emacs i receive the prompt.

You are right, Tramp's handler for `shell-command' ignores
`async-shell-command-buffer'. The reason for this is the following
comment in `tramp-handle-shell-command':

--8<---------------cut here---------------start------------->8---
    ;; Check whether there is another process running.  Tramp does not
    ;; support 2 (asynchronous) processes in parallel.
--8<---------------cut here---------------end--------------->8---

Well, this comment is more than 8 years old, and it is not true anymore
(I've just tested). I don't remember when this was fixed, but so what ...

However, I'm kind of reluctant to fix this in
`tramp-handle-shell-command'. The respective code in `shell-command'
spans over ~40 lines, and I don't believe Tramp shall simply copy those
lines (and other details not handled in Tramp yet). It's even
questionable that Tramp shall offer an own handler for `shell-command'.

The reason why Tramp does this is the use of `shell-file-name' and
`shell-command-switch'. They keep host local values, for remote
connections other values are needed. It is a long standing request, that
Tramp shall offer connection local variables, which carry different
values for different remote hosts. If we would have such a mechanism,
`shell-command' could use `process-file' and `start-file-process', and it
would not need to call a file name handler anymore.

And this error would go away.

Comments?

Best regards, Michael.

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Tino Calancha]

On Sun, 11 Sep 2016, Michael Albinus wrote:

Hi Michael,
thank you for your comprehensive answer.

> Well, this comment is more than 8 years old, and it is not true anymore
> (I've just tested). I don't remember when this was fixed, but so what 
...
Great!  It's very good to heard that in principle tramp could support
>1 async processes.

> However, I'm kind of reluctant to fix this in
> `tramp-handle-shell-command'. The respective code in `shell-command'
> spans over ~40 lines, and I don't believe Tramp shall simply copy those
> lines (and other details not handled in Tramp yet). It's even
> questionable that Tramp shall offer an own handler for `shell-command'.
It might has sense to refactor that part into a new function (see patch 
below).
The tramp could use this new function which just use the variable
`async-shell-command-buffer'.  Other things like 
`shell-command-dont-erase-buffer'
can perfectly be ignored by tramp: they are still not well established.

> The reason why Tramp does this is the use of `shell-file-name' and
> `shell-command-switch'. They keep host local values, for remote
> connections other values are needed. It is a long standing request, that
> Tramp shall offer connection local variables, which carry different
> values for different remote hosts. If we would have such a mechanism,
> `shell-command' could use `process-file' and `start-file-process', and 
it
> would not need to call a file name handler anymore.
>
> And this error would go away.
That sounds like the ultimate solution.
A partial solution could be to allow running >1 async commands as root
in the local machine _only_.  Then, `shell-file-name' and
`shell-command-switch' are the same.
Sometimes i need to execute more than 1 process with root priviledges in
my local machine: i do this using several terminals.
Running all the processes inside Emacs would be nicer.


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From ea08797362e9f4e02746e12229af7f160f7b4251 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Sun, 11 Sep 2016 20:49:56 +0900
Subject: [PATCH] shell-command: Refactor buffer creation for async cmd

* lisp/simple.el (async-shell-command-handle-multi-process):
New defun; handle the creation of a new asynchronous shell command
according with 'async-shell-command-buffer'.
(shell-command): Use it.
---
  lisp/simple.el | 91 
++++++++++++++++++++++++++++++++++++----------------------
  1 file changed, 57 insertions(+), 34 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 04a525c..3d0c579 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3311,6 +3311,59 @@ async-shell-command
      (setq command (concat command " &")))
    (shell-command command output-buffer error-buffer))

+(defun async-shell-command-handle-multi-process (proc buffer 
output-buffer)
+  "Handle > 1 async shell commands according with 
`async-shell-command-buffer'.
+PROC is the process with buffer *Async Shell Command*.
+BUFFER is the buffer associated to the new async shell command.
+OUTPUT-BUFFER, if non-nil, says to put the output in some other buffer.
+Return BUFFER."
+  (pcase async-shell-command-buffer
+    ('confirm-kill-process
+      ;; If will kill a process, query first.
+      (if (yes-or-no-p
+           "A command is running in the default buffer.  Kill it? ")
+          (kill-process proc)
+        (error "Shell command in progress")))
+    ('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
+                        (or (and (or (bufferp output-buffer)
+                                     (stringp output-buffer))
+                                 (buffer-name output-buffer))
+                            "*Async Shell Command*")))
+        (error "Shell command in progress")))
+    ('new-buffer
+      ;; It will create a new buffer.
+      (setq buffer (generate-new-buffer
+                    (or (and (or (bufferp output-buffer)
+                                 (stringp output-buffer))
+                             (buffer-name output-buffer))
+                        "*Async Shell Command*"))))
+    ('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
+                          (or (and (or (bufferp output-buffer)
+                                       (stringp output-buffer))
+                                   output-buffer)
+                              "*Async Shell Command*"))))
+        (error "Shell command in progress")))
+    ('rename-buffer
+      ;; It will rename the buffer.
+      (with-current-buffer buffer
+        (rename-uniquely))
+      (setq buffer (get-buffer-create
+                    (or (and (or (bufferp output-buffer)
+                                 (stringp output-buffer))
+                             output-buffer)
+                        "*Async Shell Command*"))))) buffer)
+
  (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.
@@ -3442,40 +3495,10 @@ shell-command
  		(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)
-		    ;; If will kill a process, query first.
-		    (if (yes-or-no-p "A command is running in the default 
buffer.  Kill it? ")
-			(kill-process proc)
-		      (error "Shell command in progress")))
-		   ((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
-				      (or (and (bufferp output-buffer) 
(buffer-name output-buffer))
-					  output-buffer "*Async Shell 
Command*")))
-		      (error "Shell command in progress")))
-		   ((eq async-shell-command-buffer 'new-buffer)
-		    ;; It will create a new buffer.
-		    (setq buffer (generate-new-buffer
-				  (or (and (bufferp output-buffer) 
(buffer-name output-buffer))
-				      output-buffer "*Async Shell 
Command*"))))
-		   ((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
-					(or output-buffer "*Async Shell 
Command*"))))
-		      (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
-				  (or output-buffer "*Async Shell 
Command*"))))))
+                (when proc
+                  (setq buffer
+                        (async-shell-command-handle-multi-process
+                         proc output-buffer buffer)))
  		(with-current-buffer buffer
  		  (display-buffer buffer '(nil (allow-no-window . t)))
                    (shell-command--save-pos-or-erase)
-- 
2.9.3

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

In GNU Emacs 25.1.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.21.5)
  of 2016-09-11 built on calancha-pc
Repository revision: 5fd1f7f931163ddf04f0ba0c362840fc91fba54a

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Michael Albinus]

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

> Hi Michael,

Hi Tino,

>> However, I'm kind of reluctant to fix this in
>> `tramp-handle-shell-command'. The respective code in `shell-command'
>> spans over ~40 lines, and I don't believe Tramp shall simply copy those
>> lines (and other details not handled in Tramp yet). It's even
>> questionable that Tramp shall offer an own handler for `shell-command'.
> It might has sense to refactor that part into a new function (see
> patch below).
> The tramp could use this new function which just use the variable
> `async-shell-command-buffer'.  Other things like

This could be done, of course. But it would add additional complexity to
simple.el, although a better solution could be implemented.

> `shell-command-dont-erase-buffer'
> can perfectly be ignored by tramp: they are still not well established.

Mid-term somebody else will write another bug report for this. If we're
going to fix this bug, it shall cover this aspect as well.

>> The reason why Tramp does this is the use of `shell-file-name' and
>> `shell-command-switch'. They keep host local values, for remote
>> connections other values are needed. It is a long standing request, that
>> Tramp shall offer connection local variables, which carry different
>> values for different remote hosts. If we would have such a mechanism,
>> `shell-command' could use `process-file' and `start-file-process',
>> and 
> it
>> would not need to call a file name handler anymore.
>>
>> And this error would go away.
> That sounds like the ultimate solution.

Yes, I'd like to go this way.

> A partial solution could be to allow running >1 async commands as root
> in the local machine _only_.  Then, `shell-file-name' and
> `shell-command-switch' are the same.
> Sometimes i need to execute more than 1 process with root priviledges in
> my local machine: i do this using several terminals.
> Running all the processes inside Emacs would be nicer.

It would be impossible to explain why it is restricted to just this
use case. And people would blame us.

No, I would like to take the opportunity to introduce connection-local
variables. First it needs a proper API.

Best regards, Michael.

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Tino Calancha]

Hi Michael,

thanks for your replay.

On Mon, 12 Sep 2016, Michael Albinus wrote:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> Hi Michael,
>
> Hi Tino,
>
>>> However, I'm kind of reluctant to fix this in
>>> `tramp-handle-shell-command'. The respective code in `shell-command'
>>> spans over ~40 lines, and I don't believe Tramp shall simply copy those
>>> lines (and other details not handled in Tramp yet). It's even
>>> questionable that Tramp shall offer an own handler for `shell-command'.
>> It might has sense to refactor that part into a new function (see
>> patch below).
>> The tramp could use this new function which just use the variable
>> `async-shell-command-buffer'.  Other things like
>
> This could be done, of course. But it would add additional complexity to
> simple.el, although a better solution could be implemented.
>
>> `shell-command-dont-erase-buffer'
>> can perfectly be ignored by tramp: they are still not well established.
>
> Mid-term somebody else will write another bug report for this. If we're
> going to fix this bug, it shall cover this aspect as well.
That's true.  Maybe even myself after 6 months :-)


>>> The reason why Tramp does this is the use of `shell-file-name' and
>>> `shell-command-switch'. They keep host local values, for remote
>>> connections other values are needed. It is a long standing request, that
>>> Tramp shall offer connection local variables, which carry different
>>> values for different remote hosts. If we would have such a mechanism,
>>> `shell-command' could use `process-file' and `start-file-process',
>>> and
>> it
>>> would not need to call a file name handler anymore.
>>>
>>> And this error would go away.
>> That sounds like the ultimate solution.
>
> Yes, I'd like to go this way.
>
>> A partial solution could be to allow running >1 async commands as root
>> in the local machine _only_.  Then, `shell-file-name' and
>> `shell-command-switch' are the same.
>> Sometimes i need to execute more than 1 process with root priviledges in
>> my local machine: i do this using several terminals.
>> Running all the processes inside Emacs would be nicer.
>
> It would be impossible to explain why it is restricted to just this
> use case. And people would blame us.
> No, I would like to take the opportunity to introduce connection-local
> variables. First it needs a proper API.
It's OK, i respect your opinion.  You are talking as the developer of the
code, it's normal you only want to looks perfect.  I would say the same
if i were coding in tramp.
In my case, i talk just as a simple tramp user: i would be _super_ 
happy if i could get your ultimate solution, but i would be _mild_ happy
if i could get at least the partial solution.

Regards,
Tino

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Tino,

>>> The reason why Tramp does this is the use of `shell-file-name' and
>>> `shell-command-switch'. They keep host local values, for remote
>>> connections other values are needed. It is a long standing request, that
>>> Tramp shall offer connection local variables, which carry different
>>> values for different remote hosts. If we would have such a mechanism,
>>> `shell-command' could use `process-file' and `start-file-process',
>>> and
>> it
>>> would not need to call a file name handler anymore.
>>>
>>> And this error would go away.
>> That sounds like the ultimate solution.
>
> Yes, I'd like to go this way.

I've implemented this as much this bug is concerned. The final solution
(do not use any file name handler for shell-command) is not implemented
yet, but this is not target of this bug report.

Pls check, whether the bug is fixed for you.

Best regards, Michael.

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Tino Calancha]

Thank you Michael!
I just tested it now and I confirm it is fixed.

On Fri, 22 Mar 2019, Michael Albinus wrote:

> I've implemented this as much this bug is concerned. The final solution
> (do not use any file name handler for shell-command) is not implemented
> yet, but this is not target of this bug report.
>
> Pls check, whether the bug is fixed for you.
>
> Best regards, Michael.

bug#24394: 25.1.50; (find-file "/sudo::") ignores async-shell-command-buffer settings

[Michael Albinus]

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

Hi Tino,

> Thank you Michael!
> I just tested it now and I confirm it is fixed.

Thanks, I'm closing the bug.

Best regards, Michael.