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

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

[Enrico Scholz]

Hi,

wdired seems to misbehave when 'wdired-use-interactive-rename' is
active:

1. create scratch directory with a file

   mkdir /tmp/test
   cd /tmp/test
   touch foo.c

2. start emacs

   LC_ALL=C emacs -Q -nw

3. set option above

   M-: (setq wdired-use-interactive-rename t)

4. go into the folder

   C-x C-f /tmp/test

   emacs will show

   | /tmp/test:
   | total used in directory 0 available 4023272
   | drwxrwxr-x.  2 ensc ensc  60 Jul 16 15:16 .
   | drwxrwxrwt. 18 root root 600 Jul 16 15:17 ..
   | -rw-rw-r--.  1 ensc ensc   0 Jul 16 14:58 foo.c

5. enter wdired mode

   C-x C-q

6. replace 'foo' with 'test'; e.g.

   test M-d

7. commit it

   C-c C-c


---> emacs asks

| Move `c' to `test.c'? [Type yn!q or C-h]

or

| Move `.' to `test.c'? [Type yn!q or C-h]

(seems to differ slightly when repeating step 6).  Buffer content is
malformed too (first two lines are merged, or whitespace between time
and filename is removed).


Confirming with 'y' will make the operation fail because the requested
source file does not exist.


Without the interactive rename, things seem to be fine.




Enrico
----------------------------
In GNU Emacs 26.1 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 2.24.32)
 of 2018-05-31 built on koji-builder4.intern.sigma-chemnitz.de
Windowing system distributor 'Fedora Project', version 11.0.11906000

Configured using:
 'configure --build=x86_64-redhat-linux-gnu
 --host=x86_64-redhat-linux-gnu --program-prefix=
 --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
 --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
 --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
 --libexecdir=/usr/libexec --localstatedir=/var
 --sharedstatedir=/var/lib --mandir=/usr/share/man
 --infodir=/usr/share/info --with-dbus --with-gif --with-jpeg --with-png
 --with-rsvg --with-tiff --with-xft --with-xpm --with-x-toolkit=gtk
 --with-gpm=no --with-modules build_alias=x86_64-redhat-linux-gnu
 host_alias=x86_64-redhat-linux-gnu 'CFLAGS=-DMAIL_USE_LOCKF -O2 -g
 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong
 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
 -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection'
 LDFLAGS=-Wl,-z,relro
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND DBUS GSETTINGS NOTIFY ACL
LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK2 X11 MODULES THREADS LCMS2

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

[Stephen Berman]

On Mon, 16 Jul 2018 15:28:29 +0200 Enrico Scholz <enrico.scholz@ensc.de> wrote:

> Hi,
>
> wdired seems to misbehave when 'wdired-use-interactive-rename' is
> active:
>
> 1. create scratch directory with a file
>
>    mkdir /tmp/test
>    cd /tmp/test
>    touch foo.c
>
> 2. start emacs
>
>    LC_ALL=C emacs -Q -nw
>
> 3. set option above
>
>    M-: (setq wdired-use-interactive-rename t)
>
> 4. go into the folder
>
>    C-x C-f /tmp/test
>
>    emacs will show
>
>    | /tmp/test:
>    | total used in directory 0 available 4023272
>    | drwxrwxr-x.  2 ensc ensc  60 Jul 16 15:16 .
>    | drwxrwxrwt. 18 root root 600 Jul 16 15:17 ..
>    | -rw-rw-r--.  1 ensc ensc   0 Jul 16 14:58 foo.c
>
> 5. enter wdired mode
>
>    C-x C-q
>
> 6. replace 'foo' with 'test'; e.g.
>
>    test M-d
>
> 7. commit it
>
>    C-c C-c
>
>
> ---> emacs asks
>
> | Move `c' to `test.c'? [Type yn!q or C-h]
>
> or
>
> | Move `.' to `test.c'? [Type yn!q or C-h]
>
> (seems to differ slightly when repeating step 6).  Buffer content is
> malformed too (first two lines are merged, or whitespace between time
> and filename is removed).
>
>
> Confirming with 'y' will make the operation fail because the requested
> source file does not exist.
>
>
> Without the interactive rename, things seem to be fine.

The bug is in wdired-search-and-rename (called when
wdired-use-interactive-rename is non-nil) and is triggered by leaving
part of the original file name unmodified (in the above case, the
extension '.c' was not altered), which leaves the dired-filename text
property, which dired-move-to-filename, called by
wdired-search-and-rename, finds and moves point there, causing the
subsequent search for the file name to fail.  The patch below fixes this
bug, according to my tests, but maybe I overlooked some corner cases.
Can you try it and see if it works for you?

Steve Berman

diff --git a/lisp/wdired.el b/lisp/wdired.el
index bb60e77776..968aac0149 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -543,19 +543,41 @@ 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))
+                    ;; If part of filename-ori is unmodified,
+                    ;; dired-move-to-filename moves point there, which
+                    ;; causes the search to fail (bug#32173).
+                    ;; Removing the dired-filename text property
+                    ;; prevents this (the text property is added again
+                    ;; when renaming succeeds).
+		    (remove-text-properties
+		     (line-beginning-position) (line-end-position)
+		     '(dired-filename nil))
+		    (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))
+              ;; If user quits before renaming succeeds, restore the
+              ;; dired-filename text property.
+	      (when failed
+		(beginning-of-line)
+		(let ((beg (re-search-forward
+			    directory-listing-before-filename-regexp
+			    (line-end-position) t))
+		      (end (dired-move-to-end-of-filename))
+		      (inhibit-read-only t))
+		  (add-text-properties beg end '(dired-filename t)))))
 	  (forward-line -1))))))
 
 ;; marks a list of files for deletion

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

[Eli Zaretskii]

> From: Enrico Scholz <enrico.scholz@ensc.de>
> Date: Mon, 16 Jul 2018 15:28:29 +0200
> 
> wdired seems to misbehave when 'wdired-use-interactive-rename' is
> active:
> 
> 1. create scratch directory with a file
> 
>    mkdir /tmp/test
>    cd /tmp/test
>    touch foo.c
> 
> 2. start emacs
> 
>    LC_ALL=C emacs -Q -nw
> 
> 3. set option above
> 
>    M-: (setq wdired-use-interactive-rename t)
> 
> 4. go into the folder
> 
>    C-x C-f /tmp/test
> 
>    emacs will show
> 
>    | /tmp/test:
>    | total used in directory 0 available 4023272
>    | drwxrwxr-x.  2 ensc ensc  60 Jul 16 15:16 .
>    | drwxrwxrwt. 18 root root 600 Jul 16 15:17 ..
>    | -rw-rw-r--.  1 ensc ensc   0 Jul 16 14:58 foo.c
> 
> 5. enter wdired mode
> 
>    C-x C-q
> 
> 6. replace 'foo' with 'test'; e.g.
> 
>    test M-d
> 
> 7. commit it
> 
>    C-c C-c
> 
> 
> ---> emacs asks
> 
> | Move `c' to `test.c'? [Type yn!q or C-h]
> 
> or
> 
> | Move `.' to `test.c'? [Type yn!q or C-h]
> 
> (seems to differ slightly when repeating step 6).  Buffer content is
> malformed too (first two lines are merged, or whitespace between time
> and filename is removed).

It looks like the code expects you to delete the entire original name
and then type the new name from scratch, it doesn't expect to see part
of the old file name unaltered.

Does the patch below give good result?

diff --git a/lisp/wdired.el b/lisp/wdired.el
index bb60e77..13005cb 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -550,7 +550,11 @@ wdired-search-and-rename
             (progn
               (setq done t)
               (let ((inhibit-read-only t))
-                (dired-move-to-filename)
+                ;; Can't use dired-move-to-filename, because editing
+                ;; the file names could have left the 'dired-filename'
+                ;; property only on part of the file name.
+                (re-search-forward directory-listing-before-filename-regexp
+                                   (line-end-position) t)
                 (search-forward (wdired-get-filename t) nil t)
                 (replace-match (file-name-nondirectory filename-ori) t t))
               (dired-do-create-files-regexp

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

[Stephen Berman]

On Fri, 20 Jul 2018 22:44:31 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Enrico Scholz <enrico.scholz@ensc.de>
>> Date: Mon, 16 Jul 2018 15:28:29 +0200
>> 
>> wdired seems to misbehave when 'wdired-use-interactive-rename' is
>> active:
[...]
> It looks like the code expects you to delete the entire original name
> and then type the new name from scratch, it doesn't expect to see part
> of the old file name unaltered.
>
> Does the patch below give good result?
>
> diff --git a/lisp/wdired.el b/lisp/wdired.el
> index bb60e77..13005cb 100644
> --- a/lisp/wdired.el
> +++ b/lisp/wdired.el
> @@ -550,7 +550,11 @@ wdired-search-and-rename
>              (progn
>                (setq done t)
>                (let ((inhibit-read-only t))
> -                (dired-move-to-filename)
> +                ;; Can't use dired-move-to-filename, because editing
> +                ;; the file names could have left the 'dired-filename'
> +                ;; property only on part of the file name.
> +                (re-search-forward directory-listing-before-filename-regexp
> +                                   (line-end-position) t)
>                  (search-forward (wdired-get-filename t) nil t)
>                  (replace-match (file-name-nondirectory filename-ori) t t))
>                (dired-do-create-files-regexp

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.

Steve Berman

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

[Eli Zaretskii]

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

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

[Eli Zaretskii]

> Date: Sat, 21 Jul 2018 15:06:19 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 32173@debbugs.gnu.org, enrico.scholz@ensc.de
> 
> > 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.

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.

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

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

[Stephen Berman]

On Sun, 22 Jul 2018 01:38:44 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:

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

Just pinging in case this has fallen under your radar.  No sweat if you
haven't had time to review it or are waiting for a reaction from the OP.

Steve Berman

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: enrico.scholz@ensc.de,  32173@debbugs.gnu.org
> Date: Thu, 26 Jul 2018 09:54:10 +0200
> 
> Just pinging in case this has fallen under your radar.

Don't worry, it didn't.  It's been a busy week...

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: enrico.scholz@ensc.de,  32173@debbugs.gnu.org
> Date: Sun, 22 Jul 2018 01:38:44 +0200
> 
> > 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.

Thanks.  I think we should install your original and safer patch on
the release branch, and this more thorough fix on master.  WDYT?

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

[Stephen Berman]

On Fri, 27 Jul 2018 10:09:25 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: enrico.scholz@ensc.de,  32173@debbugs.gnu.org
>> Date: Sun, 22 Jul 2018 01:38:44 +0200
>> 
>> > 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.
[...]
> Thanks.  I think we should install your original and safer patch on
> the release branch, and this more thorough fix on master.  WDYT?

Sounds reasonable.  Should we give the OP a bit longer to react or
should I just go ahead and commit the fixes (in any case, I may not be
able to until tomorrow or Sunday)?

I also wrote three tests, two for the bug with non-nil
wdired-use-interactive-rename, one where the edit is finished and one
where it's aborted, and one test for unfinished edits (it might be nice
to have a variant of the latter that uses dired-isearch-filenames, but I
don't see any straightforward way to emulate isearch in the test
environment).  The first two tests are suitable for both fixes, but the
third test only succeeds with the fix intended for master, so I use the
:expected-result keyword in the test definition.  But should I install
the test file on each branch as part of the commit with the respective
fix (which won't be merged from release to master), or should I make a
separate commit of the test file just to the release branch and let it
be merged to master?

Steve Berman

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

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: enrico.scholz@ensc.de,  32173@debbugs.gnu.org
> Date: Fri, 27 Jul 2018 20:15:38 +0200
> 
> > Thanks.  I think we should install your original and safer patch on
> > the release branch, and this more thorough fix on master.  WDYT?
> 
> Sounds reasonable.  Should we give the OP a bit longer to react or
> should I just go ahead and commit the fixes (in any case, I may not be
> able to until tomorrow or Sunday)?

I think by then we will have waited long enough.

> I also wrote three tests, two for the bug with non-nil
> wdired-use-interactive-rename, one where the edit is finished and one
> where it's aborted, and one test for unfinished edits (it might be nice
> to have a variant of the latter that uses dired-isearch-filenames, but I
> don't see any straightforward way to emulate isearch in the test
> environment).  The first two tests are suitable for both fixes, but the
> third test only succeeds with the fix intended for master, so I use the
> :expected-result keyword in the test definition.  But should I install
> the test file on each branch as part of the commit with the respective
> fix (which won't be merged from release to master), or should I make a
> separate commit of the test file just to the release branch and let it
> be merged to master?

You can commit the tests to the emacs-26 branch and let it be merged.

Thanks.

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

[Stephen Berman]

On Fri, 27 Jul 2018 23:59:19 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: enrico.scholz@ensc.de,  32173@debbugs.gnu.org
>> Date: Fri, 27 Jul 2018 20:15:38 +0200
>> 
>> > Thanks.  I think we should install your original and safer patch on
>> > the release branch, and this more thorough fix on master.  WDYT?
>> 
>> Sounds reasonable.  Should we give the OP a bit longer to react or
>> should I just go ahead and commit the fixes (in any case, I may not be
>> able to until tomorrow or Sunday)?
>
> I think by then we will have waited long enough.
>
>> I also wrote three tests, two for the bug with non-nil
>> wdired-use-interactive-rename, one where the edit is finished and one
>> where it's aborted, and one test for unfinished edits (it might be nice
>> to have a variant of the latter that uses dired-isearch-filenames, but I
>> don't see any straightforward way to emulate isearch in the test
>> environment).  The first two tests are suitable for both fixes, but the
>> third test only succeeds with the fix intended for master, so I use the
>> :expected-result keyword in the test definition.  But should I install
>> the test file on each branch as part of the commit with the respective
>> fix (which won't be merged from release to master), or should I make a
>> separate commit of the test file just to the release branch and let it
>> be merged to master?
>
> You can commit the tests to the emacs-26 branch and let it be merged.
>
> Thanks.

I committed the fixes and the tests.  I'll wait another couple of days
to see if the OP responds, and then close the bug.

Steve Berman

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

[Stephen Berman]

On Sun, 29 Jul 2018 01:21:25 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Fri, 27 Jul 2018 23:59:19 +0300 Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman@gmx.net>
>>> Cc: enrico.scholz@ensc.de,  32173@debbugs.gnu.org
>>> Date: Fri, 27 Jul 2018 20:15:38 +0200
>>> 
>>> > Thanks.  I think we should install your original and safer patch on
>>> > the release branch, and this more thorough fix on master.  WDYT?
>>> 
>>> Sounds reasonable.  Should we give the OP a bit longer to react or
>>> should I just go ahead and commit the fixes (in any case, I may not be
>>> able to until tomorrow or Sunday)?
>>
>> I think by then we will have waited long enough.
>>
>>> I also wrote three tests, two for the bug with non-nil
>>> wdired-use-interactive-rename, one where the edit is finished and one
>>> where it's aborted, and one test for unfinished edits (it might be nice
>>> to have a variant of the latter that uses dired-isearch-filenames, but I
>>> don't see any straightforward way to emulate isearch in the test
>>> environment).  The first two tests are suitable for both fixes, but the
>>> third test only succeeds with the fix intended for master, so I use the
>>> :expected-result keyword in the test definition.  But should I install
>>> the test file on each branch as part of the commit with the respective
>>> fix (which won't be merged from release to master), or should I make a
>>> separate commit of the test file just to the release branch and let it
>>> be merged to master?
>>
>> You can commit the tests to the emacs-26 branch and let it be merged.
>>
>> Thanks.
>
> I committed the fixes and the tests.  I'll wait another couple of days
> to see if the OP responds, and then close the bug.

Done (better later than never).

Steve Berman