* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' @ 2018-07-16 13:28 Enrico Scholz 2018-07-18 16:23 ` Stephen Berman 2018-07-20 19:44 ` Eli Zaretskii 0 siblings, 2 replies; 14+ messages in thread From: Enrico Scholz @ 2018-07-16 13:28 UTC (permalink / raw) To: 32173 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-16 13:28 bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' Enrico Scholz @ 2018-07-18 16:23 ` Stephen Berman 2018-07-20 19:44 ` Eli Zaretskii 1 sibling, 0 replies; 14+ messages in thread From: Stephen Berman @ 2018-07-18 16:23 UTC (permalink / raw) To: Enrico Scholz; +Cc: 32173 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-16 13:28 bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' Enrico Scholz 2018-07-18 16:23 ` Stephen Berman @ 2018-07-20 19:44 ` Eli Zaretskii 2018-07-21 10:48 ` Stephen Berman 1 sibling, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2018-07-20 19:44 UTC (permalink / raw) To: Enrico Scholz; +Cc: 32173 > 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-20 19:44 ` Eli Zaretskii @ 2018-07-21 10:48 ` Stephen Berman 2018-07-21 12:06 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Stephen Berman @ 2018-07-21 10:48 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 32173, Enrico Scholz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-21 10:48 ` Stephen Berman @ 2018-07-21 12:06 ` Eli Zaretskii 2018-07-21 12:19 ` Eli Zaretskii 2018-07-21 23:38 ` Stephen Berman 0 siblings, 2 replies; 14+ messages in thread From: Eli Zaretskii @ 2018-07-21 12:06 UTC (permalink / raw) To: Stephen Berman; +Cc: 32173, enrico.scholz > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-21 12:06 ` Eli Zaretskii @ 2018-07-21 12:19 ` Eli Zaretskii 2018-07-21 23:38 ` Stephen Berman 1 sibling, 0 replies; 14+ messages in thread From: Eli Zaretskii @ 2018-07-21 12:19 UTC (permalink / raw) To: stephen.berman; +Cc: 32173, enrico.scholz > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-21 12:06 ` Eli Zaretskii 2018-07-21 12:19 ` Eli Zaretskii @ 2018-07-21 23:38 ` Stephen Berman 2018-07-26 7:54 ` Stephen Berman 2018-07-27 7:09 ` Eli Zaretskii 1 sibling, 2 replies; 14+ messages in thread From: Stephen Berman @ 2018-07-21 23:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 32173, enrico.scholz 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-21 23:38 ` Stephen Berman @ 2018-07-26 7:54 ` Stephen Berman 2018-07-26 17:23 ` Eli Zaretskii 2018-07-27 7:09 ` Eli Zaretskii 1 sibling, 1 reply; 14+ messages in thread From: Stephen Berman @ 2018-07-26 7:54 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 32173, enrico.scholz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-26 7:54 ` Stephen Berman @ 2018-07-26 17:23 ` Eli Zaretskii 0 siblings, 0 replies; 14+ messages in thread From: Eli Zaretskii @ 2018-07-26 17:23 UTC (permalink / raw) To: Stephen Berman; +Cc: 32173, enrico.scholz > 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... ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-21 23:38 ` Stephen Berman 2018-07-26 7:54 ` Stephen Berman @ 2018-07-27 7:09 ` Eli Zaretskii 2018-07-27 18:15 ` Stephen Berman 1 sibling, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2018-07-27 7:09 UTC (permalink / raw) To: Stephen Berman; +Cc: 32173, enrico.scholz > 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-27 7:09 ` Eli Zaretskii @ 2018-07-27 18:15 ` Stephen Berman 2018-07-27 20:59 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Stephen Berman @ 2018-07-27 18:15 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 32173, enrico.scholz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-27 18:15 ` Stephen Berman @ 2018-07-27 20:59 ` Eli Zaretskii 2018-07-28 23:21 ` Stephen Berman 0 siblings, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2018-07-27 20:59 UTC (permalink / raw) To: Stephen Berman; +Cc: 32173, enrico.scholz > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-27 20:59 ` Eli Zaretskii @ 2018-07-28 23:21 ` Stephen Berman 2018-08-08 10:04 ` Stephen Berman 0 siblings, 1 reply; 14+ messages in thread From: Stephen Berman @ 2018-07-28 23:21 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 32173, enrico.scholz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' 2018-07-28 23:21 ` Stephen Berman @ 2018-08-08 10:04 ` Stephen Berman 0 siblings, 0 replies; 14+ messages in thread From: Stephen Berman @ 2018-08-08 10:04 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 32173-done, enrico.scholz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-08-08 10:04 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-16 13:28 bug#32173: 26.1; wdired: broken 'wdired-use-interactive-rename' Enrico Scholz 2018-07-18 16:23 ` Stephen Berman 2018-07-20 19:44 ` Eli Zaretskii 2018-07-21 10:48 ` Stephen Berman 2018-07-21 12:06 ` Eli Zaretskii 2018-07-21 12:19 ` Eli Zaretskii 2018-07-21 23:38 ` Stephen Berman 2018-07-26 7:54 ` Stephen Berman 2018-07-26 17:23 ` Eli Zaretskii 2018-07-27 7:09 ` Eli Zaretskii 2018-07-27 18:15 ` Stephen Berman 2018-07-27 20:59 ` Eli Zaretskii 2018-07-28 23:21 ` Stephen Berman 2018-08-08 10:04 ` Stephen Berman
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.