bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

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

Function dired-read-shell-command was accepting an empty input. This is
a bug because it then passes the empty input as a legitimate command
which is silently processed (in error) by dired, eventually to return a
completion message.

The attached patch fixes that, and also validates the entered command as
a valid shell executable.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[-- Attachment #2: dired-aux-read-shell-command.patch --]
[-- Type: text/x-diff, Size: 1530 bytes --]

diff --git a/dired-aux.el b/dired-aux.el
index 1c10990..7f5775c 100644
--- a/dired-aux.el
+++ b/dired-aux.el
@@ -674,15 +674,22 @@ This normally reads using `read-shell-command', but if the
 offer a smarter default choice of shell command."
   (minibuffer-with-setup-hook
       (lambda ()
-	(setq-local dired-aux-files files)
-	(setq-local minibuffer-default-add-function
-		    #'minibuffer-default-add-dired-shell-commands))
+        (setq-local dired-aux-files files)
+        (setq-local minibuffer-default-add-function
+                    #'minibuffer-default-add-dired-shell-commands))
     (setq prompt (format prompt (dired-mark-prompt arg files)))
-    (if (functionp 'dired-guess-shell-command)
-	(dired-mark-pop-up nil 'shell files
-			   'dired-guess-shell-command prompt files)
-      (dired-mark-pop-up nil 'shell files
-			 'read-shell-command prompt nil nil))))
+    (let (command)
+      (setq command
+        (if (functionp 'dired-guess-shell-command)
+          (dired-mark-pop-up nil 'shell files
+                             'dired-guess-shell-command prompt files)
+         (dired-mark-pop-up nil 'shell files
+                            'read-shell-command prompt nil nil)))
+      (when (string-empty-p command)
+        (user-error "No command entered. Nothing to do!"))
+      (unless (executable-find command)
+        (user-error "Not a valid command!"))
+      command)))

 ;;;###autoload
 (defun dired-do-async-shell-command (command &optional arg file-list)

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> Date: Tue, 27 Apr 2021 15:02:43 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> 
> Function dired-read-shell-command was accepting an empty input. This is
> a bug because it then passes the empty input as a legitimate command
> which is silently processed (in error) by dired, eventually to return a
> completion message.

I'm not sure we want to disallow this.  (Wasn't there a similar
discussion recently?)

> validates the entered command as a valid shell executable.

This is certainly a mistake: a valid shell command doesn't have to be
a file that executable-find is able to find.  Shells are known to
support all kinds of internal and magic commands.

Thanks.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-27 22:19, Eli Zaretskii wrote:
> > Date: Tue, 27 Apr 2021 15:02:43 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> >
> > Function dired-read-shell-command was accepting an empty input. This is
> > a bug because it then passes the empty input as a legitimate command
> > which is silently processed (in error) by dired, eventually to return a
> > completion message.
>
> I'm not sure we want to disallow this.

Well, that's not anything for me to respond to.

>  (Wasn't there a similar discussion recently?)

I don't follow the mailing lists, so I have no idea.

> > validates the entered command as a valid shell executable.
>
> This is certainly a mistake: a valid shell command doesn't have to be
> a file that executable-find is able to find.  Shells are known to
> support all kinds of internal and magic commands.

My testing indicates otherwise.

>
> Thanks.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> Date: Tue, 27 Apr 2021 15:32:53 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: 48072@debbugs.gnu.org
> 
> > > validates the entered command as a valid shell executable.
> >
> > This is certainly a mistake: a valid shell command doesn't have to be
> > a file that executable-find is able to find.  Shells are known to
> > support all kinds of internal and magic commands.
> 
> My testing indicates otherwise.

Would you care to share the details of that testing, please?

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 05:22, Eli Zaretskii wrote:
> > Date: Tue, 27 Apr 2021 15:32:53 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> > Cc: 48072@debbugs.gnu.org
> >
> > > > validates the entered command as a valid shell executable.
> > >
> > > This is certainly a mistake: a valid shell command doesn't have to be
> > > a file that executable-find is able to find.  Shells are known to
> > > support all kinds of internal and magic commands.
> >
> > My testing indicates otherwise.
>
> Would you care to share the details of that testing, please?

I evaluated shell-command-to-string for shell aliases and shell
functions, all of which returned error of type 'command not found'. That
would indicate that the class of 'internal commands' aren't valid in the
current state of emacs.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Kévin Le Gouguec]

Boruch Baum <boruch_baum@gmx.com> writes:

>> > > This is certainly a mistake: a valid shell command doesn't have to be
>> > > a file that executable-find is able to find.  Shells are known to
>> > > support all kinds of internal and magic commands.
>> >
>> > My testing indicates otherwise.
>>
>> Would you care to share the details of that testing, please?
>
> I evaluated shell-command-to-string for shell aliases and shell
> functions, all of which returned error of type 'command not found'. That
> would indicate that the class of 'internal commands' aren't valid in the
> current state of emacs.

Shell builtins seem to work though:

M-: (shell-command-to-string "type echo")
"echo is a shell builtin
"
M-: (shell-command-to-string "which type")
"which: no type in (…)
"

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 08:19, Kévin Le Gouguec wrote:
> Shell builtins seem to work though:

Oooo. Thanks. Function shell-command-to-string is using a
non-interactive shell (echo $- results in no 'i") while function shell
isn't.

Now I definitely need to re-think my position for the diredc package,
and possibly also for this bug report. For dired{,c} use, the judgment
for me is to weigh OTOH the common (my experience) case of uninstalled
programs appearing in the default lists for "&" and "!", versus OTOH the
rare (never in my experience) use of "&" and "!" for shell built-ins.
For diredc the call is easier in favor of limiting '&' and '!' because
diredc has a shell pop-up window "'" with midnight-commander type variables
($d1 $d2 $f1 $f2 $t1 $t2) for the current directories, files, and marked
files.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 08:19, Kévin Le Gouguec wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
> Shell builtins seem to work though:

I could restrict the check to the preparation of list of completion
candidates for the defaults put into the mini-buffer history (already
done in diredc, as an advice around function dired-guess-default), and
give dired users feedback when a command returns an error condition (on
this week's plan anyway).

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Michael Albinus]

Boruch Baum <boruch_baum@gmx.com> writes:

Hi,

> The attached patch fixes that, and also validates the entered command as
> a valid shell executable.
>
> +      (unless (executable-find command)
> +        (user-error "Not a valid command!"))

This doesn't check the remote case.

Best regards, Michael.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  48072@debbugs.gnu.org
> Date: Wed, 28 Apr 2021 08:19:31 +0200
> 
> Boruch Baum <boruch_baum@gmx.com> writes:
> 
> >> > > This is certainly a mistake: a valid shell command doesn't have to be
> >> > > a file that executable-find is able to find.  Shells are known to
> >> > > support all kinds of internal and magic commands.
> >> >
> >> > My testing indicates otherwise.
> >>
> >> Would you care to share the details of that testing, please?
> >
> > I evaluated shell-command-to-string for shell aliases and shell
> > functions, all of which returned error of type 'command not found'. That
> > would indicate that the class of 'internal commands' aren't valid in the
> > current state of emacs.
> 
> Shell builtins seem to work though:

Yes, I meant builtins when I said "internal commands".  Sorry for my
sloppy wording.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> Date: Wed, 28 Apr 2021 05:50:54 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 48072@debbugs.gnu.org
> 
> I could restrict the check to the preparation of list of completion
> candidates for the defaults put into the mini-buffer history (already
> done in diredc, as an advice around function dired-guess-default), and
> give dired users feedback when a command returns an error condition (on
> this week's plan anyway).

I'm not sure I understand what you are suggesting.  Do you mean set up
the completion candidates so that they would only include executable
files found on the system, but allow users also to type commands that
are not among the completion candidates?  I think this could be
confusing, and I don't think we have a precedent for such a behavior
elsewhere.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 13:03, Michael Albinus wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
> >
> > +      (unless (executable-find command)
> > +        (user-error "Not a valid command!"))
>
> This doesn't check the remote case.

Good point. It's an area of dired I'm weak in  because I don't use it.
How about the following (with the update diredc regex):

      (unless (executable-find
                (if (string-match "^ *\\([^ ]+\\) " command)
                  (substring command (match-beginning 1) (match-end 1))
                 command)
                (file-remote-p file))
        (user-error "Not a valid command!"))

The snippet is currently in three separate points in diredc with slight
differences (defsubst called-for). For preparing the completion
candidates, the snippet is there at function
diredc--advice--shell-guess-fallback.

For the post-input check (`diredc--advice--dired-read-shell-command',
and with a tweak, also `diredc-do-async-shell-command'), how about this
for checking that the command is valid for all selected file:

      (unless (executable-find
                (if (string-match "^ *\\([^ ]+\\) " command)
                  (substring command (match-beginning 1) (match-end 1))
                 command)
                (let ((f files)
                       remote-found)
                  (while (and f (not (setq remote-found (file-remote-p (pop f))))))
                  remote-found))
        (user-error "Not a valid command!"))


ref: https://github.com/Boruch-Baum/emacs-diredc

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Michael Albinus]

Boruch Baum <boruch_baum@gmx.com> writes:

Hi Boruch,

>> > +      (unless (executable-find command)
>> > +        (user-error "Not a valid command!"))
>>
>> This doesn't check the remote case.
>
> Good point. It's an area of dired I'm weak in  because I don't use it.
> How about the following (with the update diredc regex):
>
>       (unless (executable-find
>                 (if (string-match "^ *\\([^ ]+\\) " command)
>                   (substring command (match-beginning 1) (match-end 1))
>                  command)
>                 (file-remote-p file))
>         (user-error "Not a valid command!"))

Instead of (file-remote-p file) you could always use t. If
default-directory is local, it doesn't hurt.

Best regards, Michael.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 14:13, Michael Albinus wrote:
> Instead of (file-remote-p file) you could always use t. If
> default-directory is local, it doesn't hurt.

Thanks for the tip and input. Much appreciated.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 14:58, Eli Zaretskii wrote:
> > Date: Wed, 28 Apr 2021 05:50:54 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> > Cc: Eli Zaretskii <eliz@gnu.org>, 48072@debbugs.gnu.org
> >
> > I could restrict the check to the preparation of list of completion
> > candidates for the defaults put into the mini-buffer history (already
> > done in diredc, as an advice around function dired-guess-default), and
> > give dired users feedback when a command returns an error condition (on
> > this week's plan anyway).
>
> I'm not sure I understand what you are suggesting.  Do you mean set up
> the completion candidates so that they would only include executable
> files found on the system, but allow users also to type commands that
> are not among the completion candidates?  I think this could be
> confusing, and I don't think we have a precedent for such a behavior
> elsewhere.

I don't understand what your point of confusion is, but it's your call,
of course. Your position has emacs offering users impossible choices,
choices guaranteed to fail.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> Date: Wed, 28 Apr 2021 08:49:52 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: kevin.legouguec@gmail.com, 48072@debbugs.gnu.org
> 
> On 2021-04-28 14:58, Eli Zaretskii wrote:
> > > Date: Wed, 28 Apr 2021 05:50:54 -0400
> > > From: Boruch Baum <boruch_baum@gmx.com>
> > > Cc: Eli Zaretskii <eliz@gnu.org>, 48072@debbugs.gnu.org
> > >
> > > I could restrict the check to the preparation of list of completion
> > > candidates for the defaults put into the mini-buffer history (already
> > > done in diredc, as an advice around function dired-guess-default), and
> > > give dired users feedback when a command returns an error condition (on
> > > this week's plan anyway).
> >
> > I'm not sure I understand what you are suggesting.  Do you mean set up
> > the completion candidates so that they would only include executable
> > files found on the system, but allow users also to type commands that
> > are not among the completion candidates?  I think this could be
> > confusing, and I don't think we have a precedent for such a behavior
> > elsewhere.
> 
> I don't understand what your point of confusion is, but it's your call,
> of course. Your position has emacs offering users impossible choices,
> choices guaranteed to fail.

I'm asking you to help me understand the details of restricting the
check that you are proposing.  Once I understand that, I could make up
my mind about the proposal.  Right now I simply don't understand it.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 16:03, Eli Zaretskii wrote:
> > Date: Wed, 28 Apr 2021 08:49:52 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> > Cc: kevin.legouguec@gmail.com, 48072@debbugs.gnu.org
> >
> > On 2021-04-28 14:58, Eli Zaretskii wrote:
> > > > Date: Wed, 28 Apr 2021 05:50:54 -0400
> > > > From: Boruch Baum <boruch_baum@gmx.com>
> > > > Cc: Eli Zaretskii <eliz@gnu.org>, 48072@debbugs.gnu.org
> > > >
> > > > I could restrict the check to the preparation of list of completion
> > > > candidates for the defaults put into the mini-buffer history (already
> > > > done in diredc, as an advice around function dired-guess-default), and
> > > > give dired users feedback when a command returns an error condition (on
> > > > this week's plan anyway).
> > >
> > > I'm not sure I understand what you are suggesting.  Do you mean set up
> > > the completion candidates so that they would only include executable
> > > files found on the system, but allow users also to type commands that
> > > are not among the completion candidates?  I think this could be
> > > confusing, and I don't think we have a precedent for such a behavior
> > > elsewhere.
> >
> > I don't understand what your point of confusion is, but it's your call,
> > of course. Your position has emacs offering users impossible choices,
> > choices guaranteed to fail.
>
> I'm asking you to help me understand the details of restricting the
> check that you are proposing.  Once I understand that, I could make up
> my mind about the proposal.  Right now I simply don't understand it.

You had that idea correct in your prior paragraph, where your wrote:

   "set up the completion candidates so that they would only include
   executable files found on the system, but allow users also to type
   commands that are not among the completion candidates"

What I can add is:

1) the completion candidates would be the subset of elements of the emacs
   variables `dired-guess-shell-alist-user' and
   `dired-guess-shell-alist-default' (diredc adds `diredc-shell-guess-fallback')
   which satisfy function `executable-find';

2) the user would face no constraint whatsoever in what could be entered.

2.1) I have mixed feelings about this, because for asynchronous operations
     any typographic error silently fails and yields a misleading message
     that can easily be interpreted as a successful completion.

2.1.1) Try the following in a vanilla dired buffer: Navigate POINT to a file,
       let's say 'bar', and press '&' for the async command. Then type in some
       garbage command, let's say 'foo', and <RET>. The response I get is a
       message in the mini-buffer: "foo bar&wait: finished." (BTW, I haven't
       figured out where that message is being generated; anyone's help would be
       appreciated; I would like to see if it can report errors).

2.1.2) It would be nice to get feedback from dired power users about how they
       use the '!' and '&' commands in practice. My experience and observation
       is that even power users use those commands for straightforward
       operations and use shell built-ins from either a shell buffer or a
       shell-script buffer. (note that some built-ins also have corresponding
       executables, eg '[').

2.1.3) My gut feeling is that everyone will appreciate getting feedback on an
       invalid command, and no-one would mind not being able to use shell
       built-ins. Thus, I prefer having the constraint.

2.2) The constraint as written only checks the initial atom of the command line,
     so it wouldn't catch "sort ? | uniqqq".

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> Date: Wed, 28 Apr 2021 11:01:44 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: kevin.legouguec@gmail.com, 48072@debbugs.gnu.org
> 
> > > > I'm not sure I understand what you are suggesting.  Do you mean set up
> > > > the completion candidates so that they would only include executable
> > > > files found on the system, but allow users also to type commands that
> > > > are not among the completion candidates?  I think this could be
> > > > confusing, and I don't think we have a precedent for such a behavior
> > > > elsewhere.
> 
> You had that idea correct in your prior paragraph, where your wrote:
> 
>    "set up the completion candidates so that they would only include
>    executable files found on the system, but allow users also to type
>    commands that are not among the completion candidates"

Then I already stated my opinion about this above.  I wonder what do
others think about such a feature.

> 2.1.1) Try the following in a vanilla dired buffer: Navigate POINT to a file,
>        let's say 'bar', and press '&' for the async command. Then type in some
>        garbage command, let's say 'foo', and <RET>. The response I get is a
>        message in the mini-buffer: "foo bar&wait: finished." (BTW, I haven't
>        figured out where that message is being generated; anyone's help would be
>        appreciated; I would like to see if it can report errors).

I think the message comes from process.c:status_notify, which is
called when the process is deleted after it exits.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

[Continued]

2.3) Though what we're considering is how undesirable it would be to
     restrict a user from using shell built-ins, what I feel would be
     much more useful and natural for users would be to be enable them
     to use their personal shell aliases and functions. Under the
     current dired implementation this is impossible. In diredc, it is
     possible for persistent asynchronous commands (the default for '&')
     because diredc opens an interactive shell buffer and uses function
     comint-send-input instead of using emacs shell-* commands. This
     would be an argument for me not to perform the constraint check as
     currently written because function executable-find wouldn't find
     them.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Boruch Baum]

On 2021-04-28 18:13, Eli Zaretskii wrote:
> > Date: Wed, 28 Apr 2021 11:01:44 -0400
> > From: Boruch Baum <boruch_baum@gmx.com>
> > Cc: kevin.legouguec@gmail.com, 48072@debbugs.gnu.org
> >
> > 2.1.1) Try the following in a vanilla dired buffer: Navigate POINT to a file,
> >        let's say 'bar', and press '&' for the async command. Then type in some
> >        garbage command, let's say 'foo', and <RET>. The response I get is a
> >        message in the mini-buffer: "foo bar&wait: finished." (BTW, I haven't
> >        figured out where that message is being generated; anyone's help would be
> >        appreciated; I would like to see if it can report errors).
>
> I think the message comes from process.c:status_notify, which is
> called when the process is deleted after it exits.

Thanks. It was frustrating not being able to find it.

1) Is there a way to make it user-extensible?

2) Can it report some indication of STDERR or shell variable '$?' ?

3) The '&wait' shouldn't be reported. It's just confusing cruft to a user.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Eli Zaretskii]

> Date: Wed, 28 Apr 2021 11:21:36 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: kevin.legouguec@gmail.com, 48072@debbugs.gnu.org
> 
> > I think the message comes from process.c:status_notify, which is
> > called when the process is deleted after it exits.
> 
> Thanks. It was frustrating not being able to find it.
> 
> 1) Is there a way to make it user-extensible?
> 
> 2) Can it report some indication of STDERR or shell variable '$?' ?
> 
> 3) The '&wait' shouldn't be reported. It's just confusing cruft to a user.

I guess you can define your own sentinel function, and then do there
what you want.  For STDERR, you could redirect it as you see fit.

bug#48072: 28.0.50: dired-read-shell-command: handle empty input properly [PATCH]

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> > I think the message comes from process.c:status_notify, which is
>> > called when the process is deleted after it exits.
>>
>> Thanks. It was frustrating not being able to find it.
>>
>> 1) Is there a way to make it user-extensible?
>>
>> 2) Can it report some indication of STDERR or shell variable '$?' ?
>>
>> 3) The '&wait' shouldn't be reported. It's just confusing cruft to a user.
>
> I guess you can define your own sentinel function, and then do there
> what you want.

The default mechanism to hide this message is to take #'ignore as
sentinel :-)

> For STDERR, you could redirect it as you see fit.

Yes, although I must confess there are limitations for STDERR for remote
processes.

Best regards, Michael.