bug#35564: [PATCH v3] Tweak dired warning about "wildcard" characters

[Noam Postavsky <npostavs@gmail.com>]

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

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

>> 2. dired-aux.el already contains some logic to detect isolated
>>    characters

Yes, this was kind of bugging me, so here's a patch to reuse that logic
(it applies on top of yours).  While doing this I noticed that we
earlier missed that `*` is not considered isolated, unlike `?`.

Assuming no problems, I'll just push your two patches followed by mine
(I considered squashing, but probably that makes things harder to
follow in this case).


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 10740 bytes --]

From 52af7efc5e0c673b24e09991db275112212f3fbf Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 27 Jun 2019 19:15:56 -0400
Subject: [PATCH 3/3] Dedup dired-aux isolated char searching

* lisp/dired-aux.el (dired-isolated-string-re): Use explicitly
numbered groups.
(dired--star-or-qmark-p): Add START parameter.  Make sure to return
the first isolated match.
(dired--no-subst-prompt): Operate on a list of positions rather than
searching again for isolated chars.
(dired--isolated-char-p): Remove.
(dired--need-confirm-positions): New function.
(dired-do-shell-command): Use it.
* test/lisp/dired-aux-tests.el (dired-test-isolated-char-p): Remove.
(dired-test-highlight-metachar): Adjust to new functions.  Make sure
that `*` isn't considered isolated.
---
 lisp/dired-aux.el            | 108 +++++++++++++++++++------------------------
 test/lisp/dired-aux-tests.el |  31 ++++++-------
 2 files changed, 62 insertions(+), 77 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 079e4f102f..e716b2b42c 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -60,60 +60,55 @@ (defun dired-isolated-string-re (string)
 of a string followed/prefixed with an space.
 The regexp capture the preceding blank, STRING and the following blank as
 the groups 1, 2 and 3 respectively."
-  (format "\\(\\`\\|[ \t]\\)\\(%s\\)\\([ \t]\\|\\'\\)" string))
+  (format "\\(?1:\\`\\|[ \t]\\)\\(?2:%s\\)\\(?3:[ \t]\\|\\'\\)" string))
 
-(defun dired--star-or-qmark-p (string match &optional keep)
+(defun dired--star-or-qmark-p (string match &optional keep start)
   "Return non-nil if STRING contains isolated MATCH or `\\=`?\\=`'.
 MATCH should be the strings \"?\", `\\=`?\\=`', \"*\" or nil.  The latter
 means STRING contains either \"?\" or `\\=`?\\=`' or \"*\".
 If optional arg KEEP is non-nil, then preserve the match data.  Otherwise,
 this function changes it and saves MATCH as the second match group.
+START is the position to start matching from.
 
 Isolated means that MATCH is surrounded by spaces or at the beginning/end
 of STRING followed/prefixed with an space.  A match to `\\=`?\\=`',
 isolated or not, is also valid."
-  (let ((regexps (list (dired-isolated-string-re (if match (regexp-quote match) "[*?]")))))
+  (let ((regexp (dired-isolated-string-re (if match (regexp-quote match) "[*?]"))))
     (when (or (null match) (equal match "?"))
-      (setq regexps (append (list "\\(\\)\\(`\\?`\\)\\(\\)") regexps)))
-    (cl-some (lambda (x)
-               (funcall (if keep #'string-match-p #'string-match) x string))
-             regexps)))
-
-(defun dired--isolated-char-p (command pos)
-  "Assert whether the character at POS is isolated within COMMAND.
-A character is isolated if:
-- it is surrounded by whitespace, the start of the command, or
-  the end of the command,
-- it is surrounded by `\\=`' characters."
-  (let ((start (max 0 (1- pos)))
-        (char (string (aref command pos))))
-    (and (string-match
-          (rx (or (seq (or bos blank)
-                       (group-n 1 (literal char))
-                       (or eos blank))
-                  (seq ?` (group-n 1 (literal char)) ?`)))
-          command start)
-         (= pos (match-beginning 1)))))
-
-(defun dired--highlight-nosubst-char (command char)
-  "Highlight occurences of CHAR that are not isolated in COMMAND.
-These occurences will not be substituted; they will be sent as-is
-to the shell, which may interpret them as wildcards."
-  (save-match-data
-    (let ((highlighted (substring-no-properties command))
-          (pos 0))
-      (while (string-match (regexp-quote char) command pos)
-        (let ((start (match-beginning 0))
-              (end (match-end 0)))
-          (unless (dired--isolated-char-p command start)
-            (add-face-text-property start end 'warning nil highlighted))
-          (setq pos end)))
-      highlighted)))
-
-(defun dired--no-subst-prompt (command char)
-  (let ((highlighted-command (dired--highlight-nosubst-char command char))
-        (prompt "Confirm--the highlighted characters will not be substituted:"))
-    (format-message "%s\n%s\nProceed?" prompt highlighted-command)))
+      (cl-callf concat regexp "\\|\\(?1:\\)\\(?2:`\\?`\\)\\(?3:\\)"))
+    (funcall (if keep #'string-match-p #'string-match) regexp string start)))
+
+(defun dired--need-confirm-positions (command string)
+  "Search for non-isolated matches of STRING in COMMAND.
+Return a list of positions that match STRING, but would not be
+considered \"isolated\" by `dired--star-or-qmark-p'."
+  (cl-assert (= (length string) 1))
+  (let ((start 0)
+        (isolated-char-positions nil)
+        (confirm-positions nil)
+        (regexp (regexp-quote string)))
+    ;; Collect all ? and * surrounded by spaces and `?`.
+    (while (dired--star-or-qmark-p command string nil start)
+      (push (cons (match-beginning 2) (match-end 2))
+            isolated-char-positions)
+      (setq start (match-end 2)))
+    ;; Now collect any remaining ? and *.
+    (setq start 0)
+    (while (string-match regexp command start)
+      (unless (cl-member (match-beginning 0) isolated-char-positions
+                         :test (lambda (pos match)
+                                 (<= (car match) pos (cdr match))))
+        (push (match-beginning 0) confirm-positions))
+      (setq start (match-end 0)))
+    confirm-positions))
+
+(defun dired--no-subst-prompt (command char-positions)
+  (let ((highlighted (substring-no-properties command)))
+    (dolist (pos char-positions)
+      (add-face-text-property pos (1+ pos) 'warning nil highlighted))
+    (format-message "\
+Confirm--the highlighted characters will not be substituted:\n%s
+Proceed?" highlighted)))
 
 ;;;###autoload
 (defun dired-diff (file &optional switches)
@@ -779,26 +774,19 @@ (defun dired-do-shell-command (command &optional arg file-list)
       (dired-read-shell-command "! on %s: " current-prefix-arg files)
       current-prefix-arg
       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))))
   (let* ((on-each (not (dired--star-or-qmark-p command "*" 'keep)))
 	 (no-subst (not (dired--star-or-qmark-p command "?" 'keep)))
+         (confirmations nil)
          ;; 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 (dired--no-subst-prompt command "*")))
-	           ((need-confirm-p command "?")
-	            (y-or-n-p (dired--no-subst-prompt command "?")))
-	           (t))))
+         (ok (cond
+              ((not (or on-each no-subst))
+               (error "You can not combine `*' and `?' substitution marks"))
+              ((setq confirmations (dired--need-confirm-positions command "*"))
+               (y-or-n-p (dired--no-subst-prompt confirmations command)))
+              ((setq confirmations (dired--need-confirm-positions command "?"))
+               (y-or-n-p (dired--no-subst-prompt command confirmations)))
+              (t))))
     (cond ((not ok) (message "Command canceled"))
           (t
            (if on-each
@@ -809,7 +797,7 @@ (defun dired-do-shell-command (command &optional arg file-list)
 	                          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 ""
diff --git a/test/lisp/dired-aux-tests.el b/test/lisp/dired-aux-tests.el
index 80b6393931..3f4bfffaf6 100644
--- a/test/lisp/dired-aux-tests.el
+++ b/test/lisp/dired-aux-tests.el
@@ -114,34 +114,31 @@ (ert-deftest dired-test-bug30624 ()
         (mapc #'delete-file `(,file1 ,file2))
         (kill-buffer buf)))))
 
-(ert-deftest dired-test-isolated-char-p ()
-  (should (dired--isolated-char-p "?" 0))
-  (should (dired--isolated-char-p "? " 0))
-  (should (dired--isolated-char-p " ?" 1))
-  (should (dired--isolated-char-p " ? " 1))
-  (should (dired--isolated-char-p "foo bar ? baz" 8))
-  (should (dired--isolated-char-p "foo -i`?`" 7))
-  (should-not (dired--isolated-char-p "foo `bar`?" 9))
-  (should-not (dired--isolated-char-p "foo 'bar?'" 8))
-  (should-not (dired--isolated-char-p "foo bar?baz" 7))
-  (should-not (dired--isolated-char-p "foo bar?" 7)))
-
 (ert-deftest dired-test-highlight-metachar ()
   "Check that non-isolated meta-characters are highlighted"
   (let* ((command "sed -r -e 's/oo?/a/' -e 's/oo?/a/' ? `?`")
-         (result (dired--highlight-nosubst-char command "?")))
+         (prompt (dired--no-subst-prompt
+                  command
+                  (dired--need-confirm-positions command "?")))
+         (result (and (string-match (regexp-quote command) prompt)
+                      (match-string 0 prompt))))
     (should-not (text-property-not-all 1 14 'face nil result))
     (should (equal 'warning (get-text-property 15 'face result)))
     (should-not (text-property-not-all 16 28 'face nil result))
     (should (equal 'warning (get-text-property 29 'face result)))
     (should-not (text-property-not-all 30 39 'face nil result)))
-  (let* ((command "sed -e 's/o*/a/' -e 's/o*/a/'")
-         (result (dired--highlight-nosubst-char command "*")))
+  ;; Note that `?` is considered isolated, but `*` is not.
+  (let* ((command "sed -e 's/o*/a/' -e 's/o`*` /a/'")
+         (prompt (dired--no-subst-prompt
+                  command
+                  (dired--need-confirm-positions command "*")))
+         (result (and (string-match (regexp-quote command) prompt)
+                      (match-string 0 prompt))))
     (should-not (text-property-not-all 1 10 'face nil result))
     (should (equal 'warning (get-text-property 11 'face result)))
     (should-not (text-property-not-all 12 23 'face nil result))
-    (should (equal 'warning (get-text-property 24 'face result)))
-    (should-not (text-property-not-all 25 29 'face nil result))))
+    (should (equal 'warning (get-text-property 25 'face result)))
+    (should-not (text-property-not-all 26 32 'face nil result))))
 
 (provide 'dired-aux-tests)
 ;; dired-aux-tests.el ends here
-- 
2.11.0