bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Allen Li]

When you use * or ? in dired-do-shell-command without the intent for
Dired to replace it,  you are prompted with

  Confirm--do you mean to use `*' as a wildcard?

This message is confusing, because there are lots of ways for * to be
passed to the shell without globbing.  I am also more familiar with
the term globbing than wildcard, which makes it doubly confusing, for
example if I run

  find ? -name '*.txt'

I get the message:

  Confirm--do you mean to use `*' as a wildcard?

What the question is really asking is, should * be passed to the shell
directly, as whether or not it is interpreted as a glob is determined
by the shell and the quoting rules in question.

I think this confirmation message should be removed entirely.

1. The edge case that it is trying to protect against is not very common.
2. There is no reasonable behavior that the user could expect from
this edge case.
3. The documentation string clearly describes how * and ? are interpreted.
4. The confirmation message is not very informative and is possible misleading.
5. This confirmation message shows up every time an advanced user
wants to run any command containing * or ?, e.g. for find, grep, sed,
or many other tools.
6. The confirmation message is not even shown consistently.  For
example it is shown for

  find ? -name '*.txt'

but not for

  find * -name '*.txt'

Thus, it isn't even useful for protecting against some hypothetical
unwanted behavior.

In GNU Emacs 25.3.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.19)
 of 2017-09-16 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-modules
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong
 -fno-plt' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Allen Li]

I have included two patches.  The first is to fix the documentation
string which I encountered while reading the code, the second is for
my proposed solution of removing the confirmation message.

Subject: [PATCH 1/2] Clarify dired-do-shell-command docstring

The docstring seemed to imply that if * and ? were used together, *
would take priority.  However, it is explicitly checked that * and ?
are not used together.

* lisp/dired-aux.el (dired-do-shell-command): Fix docstring
---
 lisp/dired-aux.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index f1f7cf0b0e..57eb216231 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -686,13 +686,15 @@ dired-do-shell-command
 If there is a `*' in COMMAND, surrounded by whitespace, this runs
 COMMAND just once with the entire file list substituted there.

-If there is no `*', but there is a `?' in COMMAND, surrounded by
-whitespace, or a `\\=`?\\=`' this runs COMMAND on each file
-individually with the file name substituted for `?' or `\\=`?\\=`'.
+If there is a `?' in COMMAND, surrounded by whitespace, or a
+`\\=`?\\=`' this runs COMMAND on each file individually with the
+file name substituted for `?' or `\\=`?\\=`'.

 Otherwise, this runs COMMAND on each file individually with the
 file name added at the end of COMMAND (separated by a space).

+`*' and `?' cannot be used together.
+
 `*' and `?' when not surrounded by whitespace nor `\\=`' have no special
 significance for `dired-do-shell-command', and are passed through
 normally to the shell, but you must confirm first.
-- 
2.15.0

Subject: [PATCH 2/2] Remove confirmation when using * or ? for the shell

These confirmation messages are misleading, do not trigger for all
cases, and obstruct many commands that use * or ?, like find, sed,
grep, etc.

* lisp/dired-aux.el (dired-do-shell-command): Remove substitution mark
  confirmation
---
 lisp/dired-aux.el | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 57eb216231..15bb3173b7 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -744,20 +744,10 @@ dired-do-shell-command
                            (dired--star-or-qmark-p res str))
                  (setq res (replace-match "" t t res 2)))
                (string-match regexp res))))
-  (let* ((on-each (not (dired--star-or-qmark-p command "*" 'keep)))
- (no-subst (not (dired--star-or-qmark-p command "?" 'keep)))
-         ;; Get confirmation for wildcards that may have been meant
-         ;; to control substitution of a file name or the file name list.
-         (ok (cond ((not (or on-each no-subst))
-             (error "You can not combine `*' and `?' substitution marks"))
-            ((need-confirm-p command "*")
-             (y-or-n-p (format-message
-        "Confirm--do you mean to use `*' as a wildcard? ")))
-            ((need-confirm-p command "?")
-             (y-or-n-p (format-message
-        "Confirm--do you mean to use `?' as a wildcard? ")))
-            (t))))
-    (when ok
+    (let* ((on-each (not (dired--star-or-qmark-p command "*" 'keep)))
+           (no-subst (not (dired--star-or-qmark-p command "?" 'keep))))
+      (unless (or on-each no-subst)
+        (error "You can not combine `*' and `?' substitution marks"))
       (if on-each
    (dired-bunch-files (- 10000 (length command))
                       (lambda (&rest files)
@@ -766,7 +756,7 @@ dired-do-shell-command
                       nil file-list)
  ;; execute the shell command
  (dired-run-shell-command
- (dired-shell-stuff-it command file-list nil arg)))))))
+ (dired-shell-stuff-it command file-list nil arg))))))

 ;; Might use {,} for bash or csh:
 (defvar dired-mark-prefix ""
-- 
2.15.0

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Michael Heerdegen]

Allen Li <vianchielfaura@gmail.com> writes:

> I have included two patches.  The first is to fix the documentation
> string which I encountered while reading the code, the second is for
> my proposed solution of removing the confirmation message.

I think this would also solve Bug#28969 that I had reported some time
ago.  I wanted to try your patch but...

> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
> index 57eb216231..15bb3173b7 100644
> --- a/lisp/dired-aux.el
> +++ b/lisp/dired-aux.el
> @@ -744,20 +744,10 @@ dired-do-shell-command
>                             (dired--star-or-qmark-p res str))
>                   (setq res (replace-match "" t t res 2)))
>                 (string-match regexp res))))
> -  (let* ((on-each (not (dired--star-or-qmark-p command "*" 'keep)))
> - (no-subst (not (dired--star-or-qmark-p command "?" 'keep)))

Here seems to be a problem with leading whitespace in the first line -
applying this patch fails for me.


Michael.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Eli Zaretskii]

> From: Allen Li <vianchielfaura@gmail.com>
> Date: Sun, 26 Nov 2017 23:16:59 -0800
> 
> When you use * or ? in dired-do-shell-command without the intent for
> Dired to replace it,  you are prompted with
> 
>   Confirm--do you mean to use `*' as a wildcard?
> 
> This message is confusing, because there are lots of ways for * to be
> passed to the shell without globbing.  I am also more familiar with
> the term globbing than wildcard, which makes it doubly confusing, for
> example if I run
> 
>   find ? -name '*.txt'
> 
> I get the message:
> 
>   Confirm--do you mean to use `*' as a wildcard?
> 
> What the question is really asking is, should * be passed to the shell
> directly, as whether or not it is interpreted as a glob is determined
> by the shell and the quoting rules in question.

Actually, AFAICT the command wants to ask this:

  Are you sure you want `*' to be passed to the shell?

(as opposed to letting dired-do-shell-command interpret `*' as
described in the doc string).

> I think this confirmation message should be removed entirely.
> 
> 1. The edge case that it is trying to protect against is not very common.
> 2. There is no reasonable behavior that the user could expect from
> this edge case.
> 3. The documentation string clearly describes how * and ? are interpreted.
> 4. The confirmation message is not very informative and is possible misleading.
> 5. This confirmation message shows up every time an advanced user
> wants to run any command containing * or ?, e.g. for find, grep, sed,
> or many other tools.
> 6. The confirmation message is not even shown consistently.  For
> example it is shown for
> 
>   find ? -name '*.txt'
> 
> but not for
> 
>   find * -name '*.txt'
> 
> Thus, it isn't even useful for protecting against some hypothetical
> unwanted behavior.

Tino added this confirmation last July, so I will let him defend his
change.

If we want to remove this confirmation, now is the time, because it
wasn't yet released with any Emacs version.  Once this confirmation is
out at large, it will be much harder to remove it, as that would be an
incompatible change.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Allen Li <vianchielfaura@gmail.com>
>> 
>>   find ? -name '*.txt'
>> 
>> but not for
>> 
>>   find * -name '*.txt'
>> 
>> Thus, it isn't even useful for protecting against some hypothetical
>> unwanted behavior.
>
> Tino added this confirmation last July, so I will let him defend his
> change.
>
> If we want to remove this confirmation, now is the time, because it
> wasn't yet released with any Emacs version.  Once this confirmation is
> out at large, it will be much harder to remove it, as that would be an
> incompatible change.
Thanks for your report Allen!

FWIW the confirmation was added by RMS in 2002
(eab9ed67eb50bab4fc736058a799508d544606a0).
See also commits:
commit e52c37fad057b29d68c51cf3a70b2e0d94f656cb
commit edb8d73e62552cf2f95cbf871050913862dc5f18

My commit "Ask confirmation for all suspicious wildcards"
(6e39940adba7b96dfe520caa52a1b92a1a84a84f)
extends that confirmation to cover Bug#27496.

Before 6e39940adb, not all `?' or `*' were checked for
being between white spaces:
! echo ./? RET ; This ask confirmation
! echo ./? ? RET ; This doesn't

After 6e39940adb, all occurrences of `?' or `*' are checked for
being between white spaces; the user is asked confirmation if
any of them are not surrounded by whites.
I made this change for consistency: I thought it has sense to
check all `?' `*' occurrences, not just one.

In fact, this change causes that you are prompted in your snippet:
! find * -name '*.txt' RET 
;; After (before) 6e39940adb you are (not) prompted.

Compare with the following:
! find ? -name '*.txt' RET 
;; After and before 6e39940adb you are prompted.

Your second patch disables the confirmation prompts that have
being around since 2002.  Since the source of this bug report
seems to be 6e39940adb, I would rather revert just this commit.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Allen Li]

On Mon, Nov 27, 2017 at 7:50 PM, Tino Calancha <tino.calancha@gmail.com> wrote:
> FWIW the confirmation was added by RMS in 2002
> (eab9ed67eb50bab4fc736058a799508d544606a0).
> See also commits:
> commit e52c37fad057b29d68c51cf3a70b2e0d94f656cb
> commit edb8d73e62552cf2f95cbf871050913862dc5f18
>
> My commit "Ask confirmation for all suspicious wildcards"
> (6e39940adba7b96dfe520caa52a1b92a1a84a84f)
> extends that confirmation to cover Bug#27496.
>
> Before 6e39940adb, not all `?' or `*' were checked for
> being between white spaces:
> ! echo ./? RET ; This ask confirmation
> ! echo ./? ? RET ; This doesn't
>
> After 6e39940adb, all occurrences of `?' or `*' are checked for
> being between white spaces; the user is asked confirmation if
> any of them are not surrounded by whites.
> I made this change for consistency: I thought it has sense to
> check all `?' `*' occurrences, not just one.
>
> In fact, this change causes that you are prompted in your snippet:
> ! find * -name '*.txt' RET
> ;; After (before) 6e39940adb you are (not) prompted.
>
> Compare with the following:
> ! find ? -name '*.txt' RET
> ;; After and before 6e39940adb you are prompted.
>
> Your second patch disables the confirmation prompts that have
> being around since 2002.  Since the source of this bug report
> seems to be 6e39940adb, I would rather revert just this commit.

I see.  I actually do not have your commit yet (on 25.3.1), so I am
encountering the inconsistent behavior that you fixed.

However, your commit doesn't solve the problem that the message is
misleading; unfortunately by making the prompt appear consistently the
message is now even more confusing.

Before (right now on 25.3.1), this did not show a prompt:

  find * -name '*.txt'

Now it does show a prompt:

  Confirm--do you mean to use `*' as a wildcard?

I would have no idea what this means.  Does it mean that Dired is
going to replace the *?  Does it mean that Dired is going to replace
the ’*.txt’?  Or perhaps both?  What am I confirming exactly?

The second issue is that the prompt is very annoying for advanced
users; I have filed a second bug for this (#29190).

Since your commit fixes the inconsistency problem, that's one less
reason for my advocating to remove the confirmation.  If we can make
the message less confusing and add an option to disable the prompt, I
would be happy.

However, I think writing a useful confirmation prompt for this is
hard; hopefully someone has a good idea.

One idea would be what Eli suggested:

  Are you sure you want `*' to be passed to the shell?

However, what if the command contains both `*' and `?'?

  find * -name ’*.txt’ -o -name ’x??’
  Are you sure you want `*' to be passed to the shell?
  (Does that include the `?' or not?)

What if the user typos the intended substitution character?

  find *x -name ’*.txt’ -o -name ’x??’
  Are you sure you want `*' to be passed to the shell?
  (Which one?)

Since I was not confident that a good message could be written, I
suggested removing the confirmation.

Also, I am not sure what this is supposed to be protecting against.
It seems more useful to confirm when dired-do-shell-command is going
to replace * or ? rather than when it is not.  If the user did not
read the documentation string, the user would most likely expect these
characters to be passed to the shell.  If the user did read the
documentation string, the prompt would only be an annoyance.

The original commit by RMS (eab9ed67eb50bab4fc736058a799508d544606a0)
does not provide a reason for the confirmation.  If I were to hazard a
guess, the behavior of the command was changed, so the prompt was
added to warn users accustomed to the old behavior.  However, it is
now 15 years since; I don’t think there’s any value keeping the
confirmation around for its original (?) purpose.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 29465@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, rms@gnu.org
> Date: Tue, 28 Nov 2017 12:50:52 +0900
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Allen Li <vianchielfaura@gmail.com>
> >> 
> >>   find ? -name '*.txt'
> >> 
> >> but not for
> >> 
> >>   find * -name '*.txt'
> >> 
> >> Thus, it isn't even useful for protecting against some hypothetical
> >> unwanted behavior.
> >
> > Tino added this confirmation last July, so I will let him defend his
> > change.
> >
> > If we want to remove this confirmation, now is the time, because it
> > wasn't yet released with any Emacs version.  Once this confirmation is
> > out at large, it will be much harder to remove it, as that would be an
> > incompatible change.
> Thanks for your report Allen!
> 
> FWIW the confirmation was added by RMS in 2002
> (eab9ed67eb50bab4fc736058a799508d544606a0).
> See also commits:
> commit e52c37fad057b29d68c51cf3a70b2e0d94f656cb
> commit edb8d73e62552cf2f95cbf871050913862dc5f18
> 
> My commit "Ask confirmation for all suspicious wildcards"
> (6e39940adba7b96dfe520caa52a1b92a1a84a84f)
> extends that confirmation to cover Bug#27496.

Oops, I apologize for my faulty forensic job.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Eli Zaretskii]

> From: Allen Li <vianchielfaura@gmail.com>
> Date: Tue, 28 Nov 2017 00:25:17 -0800
> Cc: 29465@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, rms@gnu.org
> 
> Since your commit fixes the inconsistency problem, that's one less
> reason for my advocating to remove the confirmation.  If we can make
> the message less confusing and add an option to disable the prompt, I
> would be happy.

I think clarifying the prompt and adding an option is indeed the way
forward.

> However, I think writing a useful confirmation prompt for this is
> hard; hopefully someone has a good idea.
> 
> One idea would be what Eli suggested:
> 
>   Are you sure you want `*' to be passed to the shell?
> 
> However, what if the command contains both `*' and `?'?

Then the prompt should say that:

   Are you sure you want `*' and `?' to be passed to the shell?

> Since I was not confident that a good message could be written, I
> suggested removing the confirmation.

I think we should try to make the prompt more clear, it cannot be that
hard.  Removing the prompt will introduce backward incompatibility
with what Emacs was doing for the past 15 years, so it's a worse
alternative, IMO.

> Also, I am not sure what this is supposed to be protecting against.
> It seems more useful to confirm when dired-do-shell-command is going
> to replace * or ? rather than when it is not.  If the user did not
> read the documentation string, the user would most likely expect these
> characters to be passed to the shell.  If the user did read the
> documentation string, the prompt would only be an annoyance.
> 
> The original commit by RMS (eab9ed67eb50bab4fc736058a799508d544606a0)
> does not provide a reason for the confirmation.

You need to look up relevant discussions on Emacs mailing lists around
the date of the commit.  In this case, read this thread:

  http://lists.gnu.org/archive/html/emacs-devel/2002-01/msg00233.html

and also the original bug report and its followups:

  http://lists.gnu.org/archive/html/bug-gnu-emacs/2002-01/msg00230.html

> If I were to hazard a guess, the behavior of the command was
> changed, so the prompt was added to warn users accustomed to the old
> behavior.

No, it was a bug report about a potentially risky feature, where a
user mistyping a command could have their files wiped out or cause
some other grave accident.

> However, it is now 15 years since; I don’t think there’s any value
> keeping the confirmation around for its original (?) purpose.

The syntax of the shell commands supported by dired-do-shell-command
and its features regarding '*' and '?' are still very complicated, as
they were back then.  Just the doc string describing the behavior is
so long it can scare.  So I don't see how the time that has passed is
of relevance here.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Allen Li]

On Tue, Nov 28, 2017 at 8:26 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> I think we should try to make the prompt more clear, it cannot be that
> hard.  Removing the prompt will introduce backward incompatibility
> with what Emacs was doing for the past 15 years, so it's a worse
> alternative, IMO.

I don't care personally, if there's an option to disable it, but I am
not convinced for the sake of the betterment of Emacs.  I don't see
how removing the prompt introduces backward incompatibility issues
unless someone has a keyboard macro saved that includes a "y" to
handle this case (which I suppose is a possible use case, but I think
we have to draw the line somewhere; would improving Emacs performance
be a breaking change because someone was relying on a certain function
call taking at least a certain amount of time?).  This is changing
interactive usage, not API usage.

> You need to look up relevant discussions on Emacs mailing lists around
> the date of the commit.  In this case, read this thread:
>
>   http://lists.gnu.org/archive/html/emacs-devel/2002-01/msg00233.html
>
> and also the original bug report and its followups:
>
>   http://lists.gnu.org/archive/html/bug-gnu-emacs/2002-01/msg00230.html
>
>> If I were to hazard a guess, the behavior of the command was
>> changed, so the prompt was added to warn users accustomed to the old
>> behavior.
>
> No, it was a bug report about a potentially risky feature, where a
> user mistyping a command could have their files wiped out or cause
> some other grave accident.

Thanks for that.  But the confirmation prompt doesn't actually address
said bug.  Mistyping `M-! rm *' instead of `! rm *' will still wipe
out your files with no confirmation.  I don't accept that `! rm *""',
the case covered by the confirmation prompt, is a common typo.

Basically, I'm not convinced that this is protecting against a
common case of user error.  I would appreciate it if someone
could provide a concrete example of such a user error (or ideally
two or three examples).

> The syntax of the shell commands supported by dired-do-shell-command
> and its features regarding '*' and '?' are still very complicated, as
> they were back then.  Just the doc string describing the behavior is
> so long it can scare.  So I don't see how the time that has passed is
> of relevance here.

If dired-do-shell-command is complicated enough to scare, as you
say, I think presenting the user with an ambiguous confirmation
prompt scares even more.  The confirmation prompt implies that
the user is doing something dangerous, when in all likelihood the
user is doing nothing of the sort.  Again, I would like to see a
concrete example of a user error this prompt actually protects
against.

With all of that said, I am fine with adding an option, since I can
just set it and be on my merry way.  But I think Emacs would benefit
from not having this prompt (and I would be happy to be convinced
otherwise).

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Drew Adams]

I have not been following this thread, so apologies if
this doesn't help or isn't very relevant.

IF we feel it helps a user to prompt about something,
and IF we feel there is a possibility that some users
might not understand the prompt, in spite of our best
efforts to come up with a good prompt, and IF we feel
that understanding the prompt is important, THEN the
doc string should make clear whatever it is that it
is important that users understand about that prompting.

It's quite possible for a user not to understand even
a good prompt.  S?he should be able to get the point
by doing `C-h f', in that case.

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Eli Zaretskii]

> Date: Tue, 28 Nov 2017 20:20:38 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 29465@debbugs.gnu.org, Tino Calancha <tino.calancha@gmail.com>
> 
> IF we feel it helps a user to prompt about something,
> and IF we feel there is a possibility that some users
> might not understand the prompt, in spite of our best
> efforts to come up with a good prompt, and IF we feel
> that understanding the prompt is important, THEN the
> doc string should make clear whatever it is that it
> is important that users understand about that prompting.
> 
> It's quite possible for a user not to understand even
> a good prompt.  S?he should be able to get the point
> by doing `C-h f', in that case.

The doc string already attempts to do that:

  `*' and `?' when not surrounded by whitespace nor `\\=`' have no special
  significance for `dired-do-shell-command', and are passed through
  normally to the shell, but you must confirm first.

We could make the intent of the confirmation even more clear, e.g.

  `*' and `?' when not surrounded by whitespace nor `\\=`' have no special
  significance for `dired-do-shell-command', and are passed through
  normally to the shell, but you must confirm first, to avoid
  inadvertently passing a wildcard to a shell command, which would cause
  that command to act on more files than you intended.

Is anything else needed to make this prompt's intent more clear?

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Drew Adams]

> > IF we feel it helps a user to prompt about something,
> > and IF we feel there is a possibility that some users
> > might not understand the prompt, in spite of our best
> > efforts to come up with a good prompt, and IF we feel
> > that understanding the prompt is important, THEN the
> > doc string should make clear whatever it is that it
> > is important that users understand about that prompting.
> >
> > It's quite possible for a user not to understand even
> > a good prompt.  S?he should be able to get the point
> > by doing `C-h f', in that case.
> 
> The doc string already attempts to do that:
> 
> `*' and `?' when not surrounded by whitespace nor `\\=`' have...

  `*' and `?', unless surrounded by whitespace or `\\=', have...

is easier to understand, I think.

> We could make the intent of the confirmation even more clear, e.g.
> 
>   `*' and `?' when not surrounded by whitespace nor `\\=`' have no
>   special
>   significance for `dired-do-shell-command', and are passed through
>   normally to the shell, but you must confirm first, to avoid
>   inadvertently passing a wildcard to a shell command, which would
>   cause that command to act on more files than you intended.

Please consider splitting that in two: "...to the shell.  But..."

> Is anything else needed to make this prompt's intent more clear?

That seems good enough for the doc string.  I don't have
a suggestion for the prompt itself.  (I don't think it's
super clear, though.)

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Allen Li]

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

On Fri, Dec 1, 2017 at 12:36 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Tue, 28 Nov 2017 20:20:38 -0800 (PST)
>> From: Drew Adams <drew.adams@oracle.com>
>> Cc: 29465@debbugs.gnu.org, Tino Calancha <tino.calancha@gmail.com>
>>
>> IF we feel it helps a user to prompt about something,
>> and IF we feel there is a possibility that some users
>> might not understand the prompt, in spite of our best
>> efforts to come up with a good prompt, and IF we feel
>> that understanding the prompt is important, THEN the
>> doc string should make clear whatever it is that it
>> is important that users understand about that prompting.
>>
>> It's quite possible for a user not to understand even
>> a good prompt.  S?he should be able to get the point
>> by doing `C-h f', in that case.
>
> The doc string already attempts to do that:
>
>   `*' and `?' when not surrounded by whitespace nor `\\=`' have no special
>   significance for `dired-do-shell-command', and are passed through
>   normally to the shell, but you must confirm first.
>
> We could make the intent of the confirmation even more clear, e.g.
>
>   `*' and `?' when not surrounded by whitespace nor `\\=`' have no special
>   significance for `dired-do-shell-command', and are passed through
>   normally to the shell, but you must confirm first, to avoid
>   inadvertently passing a wildcard to a shell command, which would cause
>   that command to act on more files than you intended.
>
> Is anything else needed to make this prompt's intent more clear?

I made some small changes to the docstring and I added an option for
disabling the prompt, in two separate patches against master.  I have
attached the patches.

Since I don't have a good idea for the prompt text itself, I fixed
these two issues first.

Aside: is there a recommended way of formatting and sending patches?
What's easiest for me is using git format-patch and then attaching the
files, but I don't know if Emacs maintainers prefer anything specific
(e.g. mail readers that don't support MIME attachments?)

[-- Attachment #2: 0001-Clarify-dired-do-shell-command-docstring.patch --]
[-- Type: text/x-patch, Size: 1582 bytes --]

From 54b01741bd61dd023d9cef12f2c1fb2890d85990 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sun, 26 Nov 2017 23:21:34 -0800
Subject: [PATCH 1/2] Clarify dired-do-shell-command docstring

The docstring seemed to imply that if * and ? were used together, *
would take priority.  However, it is explicitly checked that * and ?
are not used together.

* lisp/dired-aux.el (dired-do-shell-command): Fix docstring
---
 lisp/dired-aux.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index f1f7cf0b0e..57eb216231 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -686,13 +686,15 @@ dired-do-shell-command
 If there is a `*' in COMMAND, surrounded by whitespace, this runs
 COMMAND just once with the entire file list substituted there.
 
-If there is no `*', but there is a `?' in COMMAND, surrounded by
-whitespace, or a `\\=`?\\=`' this runs COMMAND on each file
-individually with the file name substituted for `?' or `\\=`?\\=`'.
+If there is a `?' in COMMAND, surrounded by whitespace, or a
+`\\=`?\\=`' this runs COMMAND on each file individually with the
+file name substituted for `?' or `\\=`?\\=`'.
 
 Otherwise, this runs COMMAND on each file individually with the
 file name added at the end of COMMAND (separated by a space).
 
+`*' and `?' cannot be used together.
+
 `*' and `?' when not surrounded by whitespace nor `\\=`' have no special
 significance for `dired-do-shell-command', and are passed through
 normally to the shell, but you must confirm first.
-- 
2.15.1


[-- Attachment #3: 0002-Add-option-for-controlling-dired-do-shell-command-pr.patch --]
[-- Type: text/x-patch, Size: 3480 bytes --]

From 0a71662774adfb8480644c4c736b8d3f04bc9a02 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Fri, 1 Dec 2017 22:22:53 -0800
Subject: [PATCH 2/2] Add option for controlling dired-do-shell-command prompt

* doc/emacs/dired.texi (Shell Commands in Dired): Document option
* lisp/dired-aux.el (dired-confirm-shell-command): Add option
(dired-do-shell-command): Check option before prompting
---
 doc/emacs/dired.texi |  4 +++-
 lisp/dired-aux.el    | 26 +++++++++++++++++++-------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/doc/emacs/dired.texi b/doc/emacs/dired.texi
index 9348ef5042..4c0826e1a3 100644
--- a/doc/emacs/dired.texi
+++ b/doc/emacs/dired.texi
@@ -866,6 +866,7 @@ Shell Commands in Dired
 @findex dired-do-shell-command
 @kindex ! @r{(Dired)}
 @kindex X @r{(Dired)}
+@vindex dired-confirm-shell-command
 The Dired command @kbd{!} (@code{dired-do-shell-command}) reads a
 shell command string in the minibuffer, and runs that shell command on
 one or more files.  The files that the shell command operates on are
@@ -902,7 +903,8 @@ Shell Commands in Dired
 If you want to use @samp{*} as a shell wildcard with whitespace around
 it, write @samp{*""}.  In the shell, this is equivalent to @samp{*};
 but since the @samp{*} is not surrounded by whitespace, Dired does not
-treat it specially.
+treat it specially.  Emacs will prompt for confirmation if you do
+this, unless @code{dired-confirm-shell-command} is @code{nil}.
 
 @item
 Otherwise, if the command string contains @samp{?} surrounded by
diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 57eb216231..7bd3aa12a2 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -649,6 +649,16 @@ dired-read-shell-command
       (dired-mark-pop-up nil 'shell files
 			 'read-shell-command prompt nil nil))))
 
+;;;###autoload
+(defcustom dired-confirm-shell-command t
+  "Whether to prompt for confirmation for Dired shell commands.
+If t, prompt"
+  :type '(choice (const :tag "No restrictions" nil)
+		 (const :tag "When point is on a file name initially, search file names" dwim)
+		 (const :tag "Always search in file names" t))
+  :group 'dired
+  :version "26.0")
+
 ;;;###autoload
 (defun dired-do-async-shell-command (command &optional arg file-list)
   "Run a shell command COMMAND on the marked files asynchronously.
@@ -737,13 +747,15 @@ dired-do-shell-command
       files)))
   (cl-flet ((need-confirm-p
              (cmd str)
-             (let ((res cmd)
-                   (regexp (regexp-quote str)))
-               ;; Drop all ? and * surrounded by spaces and `?`.
-               (while (and (string-match regexp res)
-                           (dired--star-or-qmark-p res str))
-                 (setq res (replace-match "" t t res 2)))
-               (string-match regexp res))))
+             (when
+                 dired-confirm-shell-command
+               (let ((res cmd)
+                     (regexp (regexp-quote str)))
+                 ;; Drop all ? and * surrounded by spaces and `?`.
+                 (while (and (string-match regexp res)
+                             (dired--star-or-qmark-p res str))
+                   (setq res (replace-match "" t t res 2)))
+                 (string-match regexp res)))))
   (let* ((on-each (not (dired--star-or-qmark-p command "*" 'keep)))
 	 (no-subst (not (dired--star-or-qmark-p command "?" 'keep)))
          ;; Get confirmation for wildcards that may have been meant
-- 
2.15.1

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Tino Calancha]

Allen Li <vianchielfaura@gmail.com> writes:

Thank you for the new patches Allen!
I don't have strong opinions on this thread; probably because
I am already in Christmass mode or something...  Anyway, I think
you guys are discussing pretty well the thing!

I have just two comments in the second patch:
> +;;;###autoload
> +(defcustom dired-confirm-shell-command t
> +  "Whether to prompt for confirmation for Dired shell commands.
> +If t, prompt"
> +  :type '(choice (const :tag "No restrictions" nil)
> +		 (const :tag "When point is on a file name initially, search file names" dwim)
> +		 (const :tag "Always search in file names" t))
> +  :group 'dired
> +  :version "26.0")
               ^^^^

* Version must be 26.1
* The :type looks unrelated with the option.
Maybe better something like this:

:type '(choice (const :tag "Ask confirmation" t)
		 (const :tag "Never ask confirmation" nil))     
> +
> -               (string-match regexp res))))
> +             (when
> +                 dired-confirm-shell-command
> +               (let ((res cmd)
You might put the option in the same line as `when', i.e.:
(when dired-confirm-shell-command

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Allen Li]

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

On Fri, Dec 1, 2017 at 11:32 PM, Tino Calancha <tino.calancha@gmail.com> wrote:
> Allen Li <vianchielfaura@gmail.com> writes:
>
> Thank you for the new patches Allen!
> I don't have strong opinions on this thread; probably because
> I am already in Christmass mode or something...  Anyway, I think
> you guys are discussing pretty well the thing!
>
> I have just two comments in the second patch:
>> +;;;###autoload
>> +(defcustom dired-confirm-shell-command t
>> +  "Whether to prompt for confirmation for Dired shell commands.
>> +If t, prompt"
>> +  :type '(choice (const :tag "No restrictions" nil)
>> +              (const :tag "When point is on a file name initially, search file names" dwim)
>> +              (const :tag "Always search in file names" t))
>> +  :group 'dired
>> +  :version "26.0")
>                ^^^^
>
> * Version must be 26.1
> * The :type looks unrelated with the option.
> Maybe better something like this:
>
> :type '(choice (const :tag "Ask confirmation" t)
>                  (const :tag "Never ask confirmation" nil))
>> +
>> -               (string-match regexp res))))
>> +             (when
>> +                 dired-confirm-shell-command
>> +               (let ((res cmd)
> You might put the option in the same line as `when', i.e.:
> (when dired-confirm-shell-command

Thanks, attached new second patch.

[-- Attachment #2: 0002-Add-option-for-controlling-dired-do-shell-command-pr.patch --]
[-- Type: text/x-patch, Size: 3417 bytes --]

From 3a6861552d2a49e052b76b10d5b77b9ce2eed016 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Fri, 1 Dec 2017 22:22:53 -0800
Subject: [PATCH 2/2] Add option for controlling dired-do-shell-command prompt

* doc/emacs/dired.texi (Shell Commands in Dired): Document option
* lisp/dired-aux.el (dired-confirm-shell-command): Add option
(dired-do-shell-command): Check option before prompting
---
 doc/emacs/dired.texi |  4 +++-
 lisp/dired-aux.el    | 24 +++++++++++++++++-------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/doc/emacs/dired.texi b/doc/emacs/dired.texi
index 9348ef5042..4c0826e1a3 100644
--- a/doc/emacs/dired.texi
+++ b/doc/emacs/dired.texi
@@ -866,6 +866,7 @@ Shell Commands in Dired
 @findex dired-do-shell-command
 @kindex ! @r{(Dired)}
 @kindex X @r{(Dired)}
+@vindex dired-confirm-shell-command
 The Dired command @kbd{!} (@code{dired-do-shell-command}) reads a
 shell command string in the minibuffer, and runs that shell command on
 one or more files.  The files that the shell command operates on are
@@ -902,7 +903,8 @@ Shell Commands in Dired
 If you want to use @samp{*} as a shell wildcard with whitespace around
 it, write @samp{*""}.  In the shell, this is equivalent to @samp{*};
 but since the @samp{*} is not surrounded by whitespace, Dired does not
-treat it specially.
+treat it specially.  Emacs will prompt for confirmation if you do
+this, unless @code{dired-confirm-shell-command} is @code{nil}.
 
 @item
 Otherwise, if the command string contains @samp{?} surrounded by
diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 57eb216231..c9f240dd46 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -649,6 +649,15 @@ dired-read-shell-command
       (dired-mark-pop-up nil 'shell files
 			 'read-shell-command prompt nil nil))))
 
+;;;###autoload
+(defcustom dired-confirm-shell-command t
+  "Whether to prompt for confirmation for ‘dired-do-shell-command’.
+If t, prompt for confirmation if the command contains potentially
+dangerous characters.  If nil, never prompt for confirmation."
+  :type 'boolean
+  :group 'dired
+  :version "26.1")
+
 ;;;###autoload
 (defun dired-do-async-shell-command (command &optional arg file-list)
   "Run a shell command COMMAND on the marked files asynchronously.
@@ -737,13 +746,14 @@ dired-do-shell-command
       files)))
   (cl-flet ((need-confirm-p
              (cmd str)
-             (let ((res cmd)
-                   (regexp (regexp-quote str)))
-               ;; Drop all ? and * surrounded by spaces and `?`.
-               (while (and (string-match regexp res)
-                           (dired--star-or-qmark-p res str))
-                 (setq res (replace-match "" t t res 2)))
-               (string-match regexp res))))
+             (when dired-confirm-shell-command
+               (let ((res cmd)
+                     (regexp (regexp-quote str)))
+                 ;; Drop all ? and * surrounded by spaces and `?`.
+                 (while (and (string-match regexp res)
+                             (dired--star-or-qmark-p res str))
+                   (setq res (replace-match "" t t res 2)))
+                 (string-match regexp res)))))
   (let* ((on-each (not (dired--star-or-qmark-p command "*" 'keep)))
 	 (no-subst (not (dired--star-or-qmark-p command "?" 'keep)))
          ;; Get confirmation for wildcards that may have been meant
-- 
2.15.1

bug#29465: 25.3; Confusing message for dired-do-shell-command substitution

[Lars Ingebrigtsen]

Allen Li <vianchielfaura@gmail.com> writes:

> Thanks, attached new second patch.

Thanks; pushed to Emacs 29 now (with some changes).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no