bug#12394: 24.2.50; wdired: error when wdired-use-interactive-rename is non-nil

[Michael Heerdegen]

Please describe exactly what actions triggered the bug, and
the precise symptoms of the bug. If you can, give a recipe
starting from `emacs -Q':

Hello,

when you have set `wdired-use-interactive-rename' to non-nil, and you
add something to the front of a filename when in wdired-mode, you get
an error after hitting C-c C-c. Here is a recipe for emacs -Q:

- dired a directory where you are allowed to change file names
- M-: (progn (require 'wdired) (setq wdired-use-interactive-rename t)) RET
- M-x wdired-change-to-wdired-mode
- go to any file
- prepend any string to its name
- C-c C-c

This will fail either with an error, or an attempt to rename a
non-existent file (whose name is a substring of the file you actually
wanted to rename).

Here is a backtrace where I got an error:

Debugger entered--Lisp error: (args-out-of-range 0 0)
replace-match("newlines.el" t 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))
(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))
(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)) (forward-line -1))
(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)) (forward-line -1)))
(let ((done nil) 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)) (forward-line -1))))
(save-excursion (goto-char (point-max)) (forward-line -1) (let ((done nil) 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)) (forward-line -1)))))
wdired-search-and-rename("/home/micha/today/Test/newlines.el" "/home/micha/today/Test/znewlines.el")
(if wdired-use-interactive-rename (wdired-search-and-rename file-ori file-new) (require (quote dired-aux)) (condition-case err (let ((dired-backup-overwrite nil)) (dired-rename-file file-ori file-new overwrite)) (error (setq errors (1+ errors)) (dired-log (concat "Rename `" file-ori "' to `" file-new "' failed:\n%s\n") err))))
(let ((file-ori (car rename))) (if wdired-use-interactive-rename (wdired-search-and-rename file-ori file-new) (require (quote dired-aux)) (condition-case err (let ((dired-backup-overwrite nil)) (dired-rename-file file-ori file-new overwrite)) (error (setq errors (1+ errors)) (dired-log (concat "Rename `" file-ori "' to `" file-new "' failed:\n%s\n") err)))))
(cond ((rassoc file-new renames) (error "Trying to rename 2 files to the same name")) ((assoc file-new renames) (setq residue (cons rename residue))) ((and (assoc file-new residue) (file-exists-p file-new)) (if (or progress renames) (setq residue (cons rename residue)) (message "Circular renaming: using temporary file name") (let ((tmp (make-temp-name file-new))) (setq renames (cons (cons (car rename) tmp) renames)) (setq residue (cons (cons tmp file-new) residue))))) (t (setq progress t) (let ((file-ori (car rename))) (if wdired-use-interactive-rename (wdired-search-and-rename file-ori file-new) (require (quote dired-aux)) (condition-case err (let ((dired-backup-overwrite nil)) (dired-rename-file file-ori file-new overwrite)) (error (setq errors (1+ errors)) (dired-log (concat "Rename `" file-ori "' to `" file-new "' failed:\n%s\n") err)))))))
(let* ((rename (car (prog1 renames (setq renames (cdr renames))))) (file-new (cdr rename))) (cond ((rassoc file-new renames) (error "Trying to rename 2 files to the same name")) ((assoc file-new renames) (setq residue (cons rename residue))) ((and (assoc file-new residue) (file-exists-p file-new)) (if (or progress renames) (setq residue (cons rename residue)) (message "Circular renaming: using temporary file name") (let ((tmp (make-temp-name file-new))) (setq renames (cons (cons ... tmp) renames)) (setq residue (cons (cons tmp file-new) residue))))) (t (setq progress t) (let ((file-ori (car rename))) (if wdired-use-interactive-rename (wdired-search-and-rename file-ori file-new) (require (quote dired-aux)) (condition-case err (let (...) (dired-rename-file file-ori file-new overwrite)) (error (setq errors ...) (dired-log ... err))))))))
(while (or renames (prog1 (setq renames residue) (setq progress nil) (setq residue nil))) (let* ((rename (car (prog1 renames (setq renames (cdr renames))))) (file-new (cdr rename))) (cond ((rassoc file-new renames) (error "Trying to rename 2 files to the same name")) ((assoc file-new renames) (setq residue (cons rename residue))) ((and (assoc file-new residue) (file-exists-p file-new)) (if (or progress renames) (setq residue (cons rename residue)) (message "Circular renaming: using temporary file name") (let ((tmp ...)) (setq renames (cons ... renames)) (setq residue (cons ... residue))))) (t (setq progress t) (let ((file-ori (car rename))) (if wdired-use-interactive-rename (wdired-search-and-rename file-ori file-new) (require (quote dired-aux)) (condition-case err (let ... ...) (error ... ...))))))))
(let ((residue nil) (progress nil) (errors 0) (overwrite (or (not wdired-confirm-overwrite) 1))) (while (or renames (prog1 (setq renames residue) (setq progress nil) (setq residue nil))) (let* ((rename (car (prog1 renames (setq renames ...)))) (file-new (cdr rename))) (cond ((rassoc file-new renames) (error "Trying to rename 2 files to the same name")) ((assoc file-new renames) (setq residue (cons rename residue))) ((and (assoc file-new residue) (file-exists-p file-new)) (if (or progress renames) (setq residue (cons rename residue)) (message "Circular renaming: using temporary file name") (let (...) (setq renames ...) (setq residue ...)))) (t (setq progress t) (let ((file-ori ...)) (if wdired-use-interactive-rename (wdired-search-and-rename file-ori file-new) (require ...) (condition-case err ... ...))))))) errors)
wdired-do-renames((("/home/micha/today/Test/newlines.el" . "/home/micha/today/Test/znewlines.el")))
(+ errors (wdired-do-renames files-renamed))
(setq errors (+ errors (wdired-do-renames files-renamed)))
(progn (setq errors (+ errors (wdired-do-renames files-renamed))))
(if files-renamed (progn (setq errors (+ errors (wdired-do-renames files-renamed)))))
(let ((changes nil) (errors 0) files-deleted files-renamed some-file-names-unchanged file-old file-new tmp-value) (save-excursion (if (and wdired-allow-to-redirect-links (fboundp (quote make-symbolic-link))) (progn (setq tmp-value (wdired-do-symlink-changes)) (setq errors (cdr tmp-value)) (setq changes (car tmp-value)))) (if (and wdired-allow-to-change-permissions (boundp (quote wdired-col-perm))) (progn (setq tmp-value (wdired-do-perm-changes)) (setq errors (+ errors (cdr tmp-value))) (setq changes (or changes (car tmp-value))))) (goto-char (point-max)) (while (not (bobp)) (setq file-old (wdired-get-filename nil t)) (if file-old (progn (setq file-new (wdired-get-filename)) (if (equal file-new file-old) (setq some-file-names-unchanged t) (setq changes t) (if (not file-new) (setq files-deleted ...) (setq files-renamed ...))))) (forward-line -1))) (if files-renamed (progn (setq errors (+ errors (wdired-do-renames files-renamed))))) (if changes (progn (if (and (stringp dired-directory) (not (file-directory-p dired-directory)) (null some-file-names-unchanged) (= (length files-renamed) 1)) (progn (setq dired-directory (cdr (car files-renamed))))) (revert-buffer)) (let ((inhibit-read-only t)) (remove-text-properties (point-min) (point-max) (quote (old-name nil end-name nil old-link nil end-link nil end-perm nil old-perm nil perm-changed nil))) (message "(No changes to be performed)"))) (if files-deleted (progn (wdired-flag-for-deletion files-deleted))) (if (> errors 0) (progn (dired-log-summary (format "%d rename actions failed" errors) nil))))
wdired-finish-edit()
call-interactively(wdired-finish-edit nil nil)

I located the problem in `wdired-search-and-rename':

(defun wdired-search-and-rename (filename-ori filename-new)
(save-excursion
(goto-char (point-max))
(forward-line -1)
(let ((done nil)
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) ;; <--------------- here
(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))
(forward-line -1))))))

It is an error to use `dired-move-to-filename' in this context.
`dired-move-to-filename' looks for text with the `dired-filename' text
property. But this is wrong if you have added text to the beginning
of a filename, because then, the first appearance of the
`dired-filename' text property is not the beginning of the edited
filename. In the backtrace above, the `search-forward' fails, and
thus `replace-match' uses invalid match-data.


Fix: The following patch works for me:

[Michael Heerdegen]

Michael Heerdegen <michael_...@web.de> writes:

> when you have set `wdired-use-interactive-rename' to non-nil, and you
> add something to the front of a filename when in wdired-mode, you get
> an error after hitting C-c C-c.

But wait. There are many more problems with
`wdired-use-interactive-rename' non-nil.

All in emacs -Q:

1. When I hit ! at the prompt after hitting C-c C-c to perform changes,
this has not the desired effect. Emacs still asks me one time for every
remaining renaming action.

2. If I hit n when Emacs prompts, I get this error:

dired-create-files: No buffer named *Dired log*

3. I also noticed that `wdired-use-interactive-rename' non-nil is not
usable at all when circular renamings are to be done. You end up with
files renamed to a temporary name. E.g. try to switch the names of two
files, and see what you get.

To sum up, setting `wdired-use-interactive-rename' to a non-nil value
breaks many many things.

I also think that the questions you would get when performing circular
renamings would confuse many users, if we would get it work.

In general, IMHO `wdired-use-interactive-rename' is not very useful at
all. I never used it, and the fact that all these problems were never
reported by anyone before indicates that not many people are using it.

I suggest to remove it completely. Instead, it would be better to
ask the user in a way similar to e.g. C in dired, like

Really perform these renamings?

a -> b
b -> a
file-x -> file-y
...

But I'm open to other suggestions. Any opinions?


Thanks,

Michael.

[Marcin Borkowski]

On 2012-09-09, at 17:19, Michael Heerdegen <michael_...@web.de> wrote:

> Please describe exactly what actions triggered the bug, and
> the precise symptoms of the bug. If you can, give a recipe
> starting from `emacs -Q':
>
> Hello,
>

> when you have set `wdired-use-interactive-rename' to non-nil, and you
> add something to the front of a filename when in wdired-mode, you get

> an error after hitting C-c C-c. Here is a recipe for emacs -Q:
>
> - dired a directory where you are allowed to change file names
> - M-: (progn (require 'wdired) (setq wdired-use-interactive-rename t)) RET
> - M-x wdired-change-to-wdired-mode
> - go to any file
> - prepend any string to its name
> - C-c C-c
>
> This will fail either with an error, or an attempt to rename a
> non-existent file (whose name is a substring of the file you actually
> wanted to rename).

Confirmed in GNU Emacs 25.1.50.1.

--
mb

[Marcin Borkowski]

On 2012-09-11, at 21:40, Michael Heerdegen <michael_...@web.de> wrote:

> Michael Heerdegen <michael_...@web.de> writes:
>
>> when you have set `wdired-use-interactive-rename' to non-nil, and you
>> add something to the front of a filename when in wdired-mode, you get
>> an error after hitting C-c C-c.
>

> But wait. There are many more problems with
> `wdired-use-interactive-rename' non-nil.
>
> All in emacs -Q:
>
> 1. When I hit ! at the prompt after hitting C-c C-c to perform changes,
> this has not the desired effect. Emacs still asks me one time for every
> remaining renaming action.

Could not reproduce in GNU Emacs 25.1.50.1 - got an error even before
the prompt.

> In general, IMHO `wdired-use-interactive-rename' is not very useful at
> all. I never used it, and the fact that all these problems were never
> reported by anyone before indicates that not many people are using it.
>
> I suggest to remove it completely. Instead, it would be better to
> ask the user in a way similar to e.g. C in dired, like
>
> Really perform these renamings?
>
> a -> b
> b -> a
> file-x -> file-y
> ...
>
> But I'm open to other suggestions. Any opinions?

+1.

> Thanks,
>
> Michael.

--
mb

[martin rudalics]

>> In general, IMHO `wdired-use-interactive-rename' is not very useful at
>> all. I never used it, and the fact that all these problems were never
>> reported by anyone before indicates that not many people are using it.
>>
>> I suggest to remove it completely.

IIUC this was broken by the introduction of the 'dired-filename' text
property. There's hardly anything reasonable we can do about this.
That property should be front-sticky but wdired removes all text
properties it creates before doing the renaming so this seems hardly
reliable.

So we probably should obsolete ‘wdired-use-interactive-rename’ and not
call ‘wdired-search-and-rename’ any more in ‘wdired-do-renames’ in the
Emacs-25 branch.

>> Instead, it would be better to
>> ask the user in a way similar to e.g. C in dired, like
>>
>> Really perform these renamings?
>>
>> a -> b
>> b -> a
>> file-x -> file-y
>> ...
>>
>> But I'm open to other suggestions. Any opinions?
>
> +1.

Sounds TRT to me too.

martin