bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Gabriel]

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

Severity: bug

Steps:
1) emacs -Q from master branch (3adc1e7f379)
2) C-u M-& "ls"
   Error: shell-command: Wrong type argument: stringp, (4)

Cause:
When called interactively, `async-shell-command' pass argument
OUTPUT-BUFFER as `current-prefix-arg' to `shell-command'.  As per
docstring of `shell-command':

  "If OUTPUT-BUFFER is not a buffer and not nil (which happens
  interactively when the prefix argument is given), insert the output in
  current buffer after point leaving mark after it.  This cannot be done
  asynchronously."

Patch:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Handle-current-prefix-arg-in-async-shell-command.patch --]
[-- Type: text/x-diff, Size: 1205 bytes --]

From 8016742cfd1d59cf3082afeb1cf77b9a12b9b251 Mon Sep 17 00:00:00 2001
From: Gabriel do Nascimento Ribeiro <gabriel376@hotmail.com>
Date: Wed, 10 May 2023 19:41:05 -0300
Subject: [PATCH 1/1] Handle current-prefix-arg in async-shell-command.

* lisp/simple.el (async-shell-command): Properly handle
  current-prefix-arg.  When called interactively, defaults to nil,
  otherwise, check if it's not a number before passing to
  `shell-command'.
---
 lisp/simple.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 58517dd81f9..ba832581955 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4499,10 +4499,12 @@ async-shell-command
 				((eq major-mode 'dired-mode)
 				 (dired-get-filename nil t)))))
 			  (and filename (file-relative-name filename))))
-    current-prefix-arg
+    nil
     shell-command-default-error-buffer))
   (unless (string-match "&[ \t]*\\'" command)
     (setq command (concat command " &")))
+  (when (and output-buffer (numberp output-buffer))
+    (error "Invalid output buffer"))
   (shell-command command output-buffer error-buffer))
 
 (declare-function comint-output-filter "comint" (process string))
-- 
2.34.1


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


---
Gabriel

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Eli Zaretskii]

> From: Gabriel <gabriel376@hotmail.com>
> Date: Wed, 10 May 2023 20:04:24 -0300
> 
> Steps:
> 1) emacs -Q from master branch (3adc1e7f379)
> 2) C-u M-& "ls"
>    Error: shell-command: Wrong type argument: stringp, (4)
> 
> Cause:
> When called interactively, `async-shell-command' pass argument
> OUTPUT-BUFFER as `current-prefix-arg' to `shell-command'.  As per
> docstring of `shell-command':
> 
>   "If OUTPUT-BUFFER is not a buffer and not nil (which happens
>   interactively when the prefix argument is given), insert the output in
>   current buffer after point leaving mark after it.  This cannot be done
>   asynchronously."

Yes, this bug was there since the day async-shell-command was added to
Emacs.

> diff --git a/lisp/simple.el b/lisp/simple.el
> index 58517dd81f9..ba832581955 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -4499,10 +4499,12 @@ async-shell-command
>  				((eq major-mode 'dired-mode)
>  				 (dired-get-filename nil t)))))
>  			  (and filename (file-relative-name filename))))
> -    current-prefix-arg
> +    nil
>      shell-command-default-error-buffer))
>    (unless (string-match "&[ \t]*\\'" command)
>      (setq command (concat command " &")))
> +  (when (and output-buffer (numberp output-buffer))
> +    (error "Invalid output buffer"))
>    (shell-command command output-buffer error-buffer))

Thanks, but I don't understand why we need the error message.  Isn't
it enough to pass nil as 2nd argument to shell-command?

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Gabriel]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I don't understand why we need the error message.  Isn't
> it enough to pass nil as 2nd argument to shell-command?

The error message is just for noninteractive cases where a caller passes
a numeric argument as the second argument of `async-shell-command'.  For
interactive cases, a nil value will be always passed to `shell-command'.

Example:
    Before patch:
        (async-shell-command "ls" 1) => error: (wrong-type-argument stringp 1)
    After patch:
        (async-shell-command "ls" 1) => error: (error "Invalid output buffer")

Here is a patch without the error message, in case it's preferable:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ignore-current-prefix-arg-in-async-shell-command.patch --]
[-- Type: text/x-diff, Size: 912 bytes --]

From 60f901126d5c883cc1c1d60a2b2b71ad3524f065 Mon Sep 17 00:00:00 2001
From: Gabriel do Nascimento Ribeiro <gabriel376@hotmail.com>
Date: Sat, 13 May 2023 20:31:24 -0300
Subject: [PATCH 1/1] Ignore current-prefix-arg in async-shell-command.

* lisp/simple.el (async-shell-command): Ignore current-prefix-arg and
  always pass nil to second argument of `shell-command'.
---
 lisp/simple.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 73c2dfa365d..98278ef2a18 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4499,7 +4499,7 @@ async-shell-command
 				((eq major-mode 'dired-mode)
 				 (dired-get-filename nil t)))))
 			  (and filename (file-relative-name filename))))
-    current-prefix-arg
+    nil
     shell-command-default-error-buffer))
   (unless (string-match "&[ \t]*\\'" command)
     (setq command (concat command " &")))
-- 
2.34.1


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


---
Gabriel

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Eli Zaretskii]

> From: Gabriel <gabriel376@hotmail.com>
> Date: Sat, 13 May 2023 20:35:51 -0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but I don't understand why we need the error message.  Isn't
> > it enough to pass nil as 2nd argument to shell-command?
> 
> The error message is just for noninteractive cases where a caller passes
> a numeric argument as the second argument of `async-shell-command'.

I think the error signaled by shell-command is sufficient in the
non-interactive case.  We don't usually try detecting
wrong-type-argument errors in non-interactive use, we leave it to the
underlying primitives to detect and report.

> Example:
>     Before patch:
>         (async-shell-command "ls" 1) => error: (wrong-type-argument stringp 1)
>     After patch:
>         (async-shell-command "ls" 1) => error: (error "Invalid output buffer")

I see nothing wrong with the "before" version, it gives an accurate
description of the problem for that case.

> Here is a patch without the error message, in case it's preferable:

Thanks, installed on the emacs-29 branch, and closing the bug.

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Eli Zaretskii]

> Cc: 63432-done@debbugs.gnu.org
> Date: Sun, 14 May 2023 09:28:34 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Thanks, installed on the emacs-29 branch, and closing the bug.

Btw, I've noticed that the ERROR-BUFFER argument is ignored by
async-shell-command, and OUTPUT-BUFFER is not documented.  So I've now
fixed the doc string to correct these deficiencies.

(I wonder why was ERROR-BUFFER at all provided in this function in the
first place, since the underlying shell-command cannot support it.
Maybe we should have an advertised-calling-convention there which
excludes it?)

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Juri Linkov]

> Btw, I've noticed that the ERROR-BUFFER argument is ignored by
> async-shell-command, and OUTPUT-BUFFER is not documented.  So I've now
> fixed the doc string to correct these deficiencies.
>
> (I wonder why was ERROR-BUFFER at all provided in this function in the
> first place, since the underlying shell-command cannot support it.
> Maybe we should have an advertised-calling-convention there which
> excludes it?)

Keeping the same function signatures for shell-command and async-shell-command
would allow to implement the error-buffer support for the latter in future.

bug#63432: 30.0.50; Handle current-prefix-arg in async-shell-command

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: gabriel376@hotmail.com,  Stefan Monnier <monnier@iro.umontreal.ca>,
>   63432@debbugs.gnu.org
> Date: Mon, 15 May 2023 20:17:00 +0300
> 
> > Btw, I've noticed that the ERROR-BUFFER argument is ignored by
> > async-shell-command, and OUTPUT-BUFFER is not documented.  So I've now
> > fixed the doc string to correct these deficiencies.
> >
> > (I wonder why was ERROR-BUFFER at all provided in this function in the
> > first place, since the underlying shell-command cannot support it.
> > Maybe we should have an advertised-calling-convention there which
> > excludes it?)
> 
> Keeping the same function signatures for shell-command and async-shell-command
> would allow to implement the error-buffer support for the latter in future.

That's fine, but then it would have been better to mention this in a
comment.  Because otherwise it just looks like a copy-pasta error
(after one wastes some time to figure out that it is basically
unused).