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

[Allen Li <vianchielfaura@gmail.com>]

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