bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename'

[Stephen Berman]

On Sat, 21 Jul 2018 15:06:19 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: Enrico Scholz <enrico.scholz@ensc.de>,  32173@debbugs.gnu.org
>> Date: Sat, 21 Jul 2018 12:48:57 +0200
>> 
>> AFAICT this patch avoids the bug and is simpler than the fix I proposed
>> (https://lists.gnu.org/archive/html/bug-gnu-emacs/2018-07/msg00602.html).
>> But with the above patch, if the user types C-g when prompted to make
>> the replacement, the file name is left partly or wholely without the
>> dired-filename text property.  I'm not sure if that's a problem, that's
>> why in my patch I restored the property.  I note the current buggy code
>> has the same issue.
>
> Right.  But I think we had better did this more thoroughly, so I think
> your solution (which I somehow managed to miss) is better.  Please
> wait for a few days and push to emacs-26 if no problems are reported
> with your patch.

Thanks, but...

On Sat, 21 Jul 2018 15:19:36 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

> Btw, what happens in the non-interactive rename case, wrt the
> dired-filename property?  If the renamed file is left with part of it
> covered by that property, we may have a broader problem in wdired.el.

That's a good question (which didn't occur to me).  With
wdired-use-interactive-rename nil (the default), a partially edited
filename is indeed only partly covered by the dired-filename property,
but as soon as you type C-c C-c or C-x C-s the change is saved and the
buffer returns to dired-mode, which makes the whole file name
propertized again.  So that's no problem.  However, there could be a
problem before saving the change if some function looks for the
dired-filename property -- and in fact, there is such a function:
dired-isearch-filenames in dired-aux.el.  And indeed, you can use this
in wdired-mode after editing file names but before saving the changes,
and then the search will fail if the search string includes characters
now lacking the dired-filename property.

The only way I could think of to avoid this is to restore the text
property via after-change-functions, as in the patch below.  I'm not so
confident that this is the best approach, but it seems to work, in that
AFAICT it fixes the bug with non-nil wdired-use-interactive-rename and
also handles the non-interactive case, allowing dired-isearch-filenames
to function as expected.  Maybe there's a less heavy-handed way to get
this, but none has occurred to me.

It was also necessary to move the invocation of
wdired-change-to-dired-mode in wdired-finish-edit to after the
invocation of wdired-do-renames, since calling it before meant the
buffer was in dired-mode, which doesn't use the after change function,
so typing C-g on being prompted to accept the change would have left a
partially unpropertized file name.  (The invocation of
wdired-change-to-dired-mode also has to be before the invocation of
revert-buffer in wdired-finish-edit to avoid using wdired-revert, which
changes to dired-mode and then back to wdired-mode.)

Finally, a consequence of moving wdired-change-to-dired-mode is that
with typing C-g with non-nil wdired-use-interactive-rename leaves the
buffer in wdired-mode, instead of returning to dired-mode as it
currently does.  To keep the current behavior I wrapped an extra call to
wdired-change-to-dired-mode in unwind-protect in
wdired-search-and-rename.

Steve Berman


diff --git a/lisp/wdired.el b/lisp/wdired.el
index bb60e77776..63202bbed9 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -255,6 +255,7 @@ wdired-change-to-wdired-mode
   (setq buffer-read-only nil)
   (dired-unadvertise default-directory)
   (add-hook 'kill-buffer-hook 'wdired-check-kill-buffer nil t)
+  (add-hook 'after-change-functions 'wdired--restore-dired-filename-prop nil t)
   (setq major-mode 'wdired-mode)
   (setq mode-name "Editable Dired")
   (setq revert-buffer-function 'wdired-revert)
@@ -363,6 +364,7 @@ wdired-change-to-dired-mode
   (setq mode-name "Dired")
   (dired-advertise)
   (remove-hook 'kill-buffer-hook 'wdired-check-kill-buffer t)
+  (remove-hook 'after-change-functions 'wdired--restore-dired-filename-prop t)
   (set (make-local-variable 'revert-buffer-function) 'dired-revert))
 
 
@@ -381,7 +383,6 @@ wdired-abort-changes
 (defun wdired-finish-edit ()
   "Actually rename files based on your editing in the Dired buffer."
   (interactive)
-  (wdired-change-to-dired-mode)
   (let ((changes nil)
        (errors 0)
        files-deleted
@@ -423,6 +424,7 @@ wdired-finish-edit
        (forward-line -1)))
     (when files-renamed
       (setq errors (+ errors (wdired-do-renames files-renamed))))
+    (wdired-change-to-dired-mode)
     (if changes
        (progn
          ;; If we are displaying a single file (rather than the
@@ -543,19 +545,23 @@ wdired-search-and-rename
     (goto-char (point-max))
     (forward-line -1)
     (let ((done nil)
+          (failed t)
          curr-filename)
       (while (and (not done) (not (bobp)))
         (setq curr-filename (wdired-get-filename nil t))
         (if (equal curr-filename filename-ori)
-            (progn
-              (setq done t)
-              (let ((inhibit-read-only t))
-                (dired-move-to-filename)
-                (search-forward (wdired-get-filename t) nil t)
-                (replace-match (file-name-nondirectory filename-ori) t t))
-              (dired-do-create-files-regexp
-               (function dired-rename-file)
-               "Move" 1 ".*" filename-new nil t))
+            (unwind-protect
+                (progn
+                  (setq done t)
+                  (let ((inhibit-read-only t))
+                    (dired-move-to-filename)
+                    (search-forward (wdired-get-filename t) nil t)
+                    (replace-match (file-name-nondirectory filename-ori) t t))
+                  (dired-do-create-files-regexp
+                   (function dired-rename-file)
+                   "Move" 1 ".*" filename-new nil t)
+                  (setq failed nil))
+              (when failed (wdired-change-to-dired-mode)))
          (forward-line -1))))))
 
 ;; marks a list of files for deletion
@@ -586,6 +592,22 @@ wdired-check-kill-buffer
        (not (y-or-n-p "Buffer changed. Discard changes and kill buffer? ")))
       (error "Error")))
 
+;; Added to after-change-functions in wdired-change-to-wdired-mode to
+;; ensure that, on editing a file name, new characters get the
+;; dired-filename text property, which allows functions that look for
+;; this property (e.g. dired-isearch-filenames) to work in wdired-mode
+;; and also avoids an error with non-nil wdired-use-interactive-rename
+;; (bug#32173).
+(defun wdired--restore-dired-filename-prop (beg end _len)
+  (save-match-data
+    (save-excursion
+      (beginning-of-line)
+      (when (re-search-forward directory-listing-before-filename-regexp
+                               (line-end-position) t)
+        (setq beg (point)
+              end (line-end-position))
+        (put-text-property beg end 'dired-filename t)))))
+
 (defun wdired-next-line (arg)
   "Move down lines then position at filename or the current column.
 See `wdired-use-dired-vertical-movement'.  Optional prefix ARG