* bug#49124: Wdired doesn't like re-search-forward/replace-match
@ 2021-06-20 0:33 Eduardo Ochs
2021-06-20 1:28 ` Michael Heerdegen
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Ochs @ 2021-06-20 0:33 UTC (permalink / raw)
To: 49124
Here's how to see the bug in action. Define `foo' by executing this
defun:
(defun foo (s e)
"Replace all `a's by `b's in the region."
(interactive "r")
(save-excursion
(save-restriction
(narrow-to-region s e)
(goto-char (point-min))
(while (re-search-forward "a" nil 'noerror)
(replace-match "b" 'fixedcase 'literal)))))
and run this to create a directory /tmp/foo with some scratch files:
rm -Rv /tmp/foo/
mkdir /tmp/foo/
cd /tmp/foo/
touch aaaa
touch aaaaa
touch aaaaaa
Visit /tmp/foo/ in dired mode, and run `M-x
wdired-change-to-wdired-mode' to switch to wdired mode. Mark a region
with two "aa"s in the middle of one of the file names, and run `M-x
foo'. The first "a" will be changed to a "b" and `foo' will abort with
the error message "Text is read-only" - not good. Leave wdired with
`C-c C-c'. The "a" that was changed to a "b" will be reverted back to
an "a", and wdired will display the message "(No changes to be
performed)" - not good again.
Tested with this version of Emacs:
GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.5, cairo version 1.16.0) of 2021-06-08
on a Debian box, with:
~/bigsrc/emacs28/src/emacs \
-T emacs28 -fg bisque -bg black -fn 6x13 \
-Q ~/TODO
I told Emacs to ignore the local variables list in my ~/TODO file.
Cheers,
Eduardo Ochs
http://angg.twu.net/#eev
edrx at irc.libera.chat
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#49124: Wdired doesn't like re-search-forward/replace-match
2021-06-20 0:33 bug#49124: Wdired doesn't like re-search-forward/replace-match Eduardo Ochs
@ 2021-06-20 1:28 ` Michael Heerdegen
2021-06-20 1:45 ` Eduardo Ochs
0 siblings, 1 reply; 7+ messages in thread
From: Michael Heerdegen @ 2021-06-20 1:28 UTC (permalink / raw)
To: Eduardo Ochs; +Cc: 49124
Eduardo Ochs <eduardoochs@gmail.com> writes:
> Here's how to see the bug in action. Define `foo' by executing this
> defun:
>
> (defun foo (s e)
> "Replace all `a's by `b's in the region."
> (interactive "r")
> (save-excursion
> (save-restriction
> (narrow-to-region s e)
> (goto-char (point-min))
> (while (re-search-forward "a" nil 'noerror)
> (replace-match "b" 'fixedcase 'literal)))))
> [...]
I can reproduce the issue. The culprit seems to be `narrow-to-region'
which seems to confuse the functions wdired now installs in the before
and/or after change hooks (they expect at least complete lines) --
because this version:
(defun foo (s e)
"Replace all `a's by `b's in the region."
(interactive "r")
(save-excursion
(save-restriction
;; (narrow-to-region s e)
(goto-char s)
(while (re-search-forward "a" e 'noerror)
(replace-match "b" 'fixedcase 'literal)))))
works as expected.
I guess we should just temporarily `widen' in these functions.
Michael.
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#49124: Wdired doesn't like re-search-forward/replace-match
2021-06-20 1:28 ` Michael Heerdegen
@ 2021-06-20 1:45 ` Eduardo Ochs
2021-06-20 2:12 ` Michael Heerdegen
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Ochs @ 2021-06-20 1:45 UTC (permalink / raw)
To: Michael Heerdegen; +Cc: 49124
On Sat, 19 Jun 2021 at 22:28, Michael Heerdegen
<michael_heerdegen@web.de> wrote:
>
> Eduardo Ochs <eduardoochs@gmail.com> writes:
>
> > Here's how to see the bug in action. Define `foo' by executing this
> > defun:
> >
> > (defun foo (s e)
> > "Replace all `a's by `b's in the region."
> > (interactive "r")
> > (save-excursion
> > (save-restriction
> > (narrow-to-region s e)
> > (goto-char (point-min))
> > (while (re-search-forward "a" nil 'noerror)
> > (replace-match "b" 'fixedcase 'literal)))))
> > [...]
>
> I can reproduce the issue. The culprit seems to be `narrow-to-region'
> which seems to confuse the functions wdired now installs in the before
> and/or after change hooks (they expect at least complete lines) --
> because this version:
>
> (defun foo (s e)
> "Replace all `a's by `b's in the region."
> (interactive "r")
> (save-excursion
> (save-restriction
> ;; (narrow-to-region s e)
> (goto-char s)
> (while (re-search-forward "a" e 'noerror)
> (replace-match "b" 'fixedcase 'literal)))))
>
> works as expected.
>
> I guess we should just temporarily `widen' in these functions.
>
> Michael.
Hi Michael,
Is there a simple way to write a macro called, say,
`wdired-with-narrow-to-filename' that would narrow the buffer to the
editable portion of current line, evaluate some code, and on exit it
would tell wdired to reread the filename in that line, knowing that it
it may have been changed?
Cheers & possibly thanks in advance =),
Eduardo Ochs
http://angg.twu.net/#eev
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#49124: Wdired doesn't like re-search-forward/replace-match
2021-06-20 1:45 ` Eduardo Ochs
@ 2021-06-20 2:12 ` Michael Heerdegen
2021-06-21 13:11 ` Lars Ingebrigtsen
0 siblings, 1 reply; 7+ messages in thread
From: Michael Heerdegen @ 2021-06-20 2:12 UTC (permalink / raw)
To: Eduardo Ochs; +Cc: 49124
Eduardo Ochs <eduardoochs@gmail.com> writes:
> Is there a simple way to write a macro called, say,
> `wdired-with-narrow-to-filename' that would narrow the buffer to the
> editable portion of current line, evaluate some code, and on exit it
> would tell wdired to reread the filename in that line, knowing that it
> it may have been changed?
The answer to that question is much harder than to fix the underlying
problem in wdired (which should be quite easy).
AFAIU we just need a (save-restriction (widen) ...) wrapper for the code
of `wdired--before-change-fn' and `wdired--restore-properties'. You can
try that in your instance, e.g. using an advice.
Regards,
Michael.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#49124: Wdired doesn't like re-search-forward/replace-match
2021-06-20 2:12 ` Michael Heerdegen
@ 2021-06-21 13:11 ` Lars Ingebrigtsen
2021-06-21 21:59 ` Michael Heerdegen
0 siblings, 1 reply; 7+ messages in thread
From: Lars Ingebrigtsen @ 2021-06-21 13:11 UTC (permalink / raw)
To: Michael Heerdegen; +Cc: 49124, Eduardo Ochs
Michael Heerdegen <michael_heerdegen@web.de> writes:
> AFAIU we just need a (save-restriction (widen) ...) wrapper for the code
> of `wdired--before-change-fn' and `wdired--restore-properties'. You can
> try that in your instance, e.g. using an advice.
I guess you're suggesting the change below? (It looks big, but it's
mostly whitespace changes because of the `save-restriction'.)
Eduardo, can you try the patch and see whether it fixes the problem
you're seeing?
diff --git a/lisp/wdired.el b/lisp/wdired.el
index 22c1cebe13..fd549bac32 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -297,26 +297,28 @@ wdired--self-insert
(defun wdired--before-change-fn (beg end)
(save-match-data
(save-excursion
- ;; Make sure to process entire lines.
- (goto-char end)
- (setq end (line-end-position))
- (goto-char beg)
- (forward-line 0)
-
- (while (< (point) end)
- (unless (wdired--line-preprocessed-p)
+ (save-restriction
+ (widen)
+ ;; Make sure to process entire lines.
+ (goto-char end)
+ (setq end (line-end-position))
+ (goto-char beg)
+ (forward-line 0)
+
+ (while (< (point) end)
+ (unless (wdired--line-preprocessed-p)
+ (with-silent-modifications
+ (put-text-property (point) (1+ (point)) 'front-sticky t)
+ (wdired--preprocess-files)
+ (when wdired-allow-to-change-permissions
+ (wdired--preprocess-perms))
+ (when (fboundp 'make-symbolic-link)
+ (wdired--preprocess-symlinks))))
+ (forward-line))
+ (when (eobp)
(with-silent-modifications
- (put-text-property (point) (1+ (point)) 'front-sticky t)
- (wdired--preprocess-files)
- (when wdired-allow-to-change-permissions
- (wdired--preprocess-perms))
- (when (fboundp 'make-symbolic-link)
- (wdired--preprocess-symlinks))))
- (forward-line))
- (when (eobp)
- (with-silent-modifications
- ;; Is this good enough? Assumes no extra white lines from dired.
- (put-text-property (1- (point-max)) (point-max) 'read-only t))))))
+ ;; Is this good enough? Assumes no extra white lines from dired.
+ (put-text-property (1- (point-max)) (point-max) 'read-only t)))))))
(defun wdired-isearch-filter-read-only (beg end)
"Skip matches that have a read-only property."
@@ -700,47 +702,49 @@ wdired-check-kill-buffer
(defun wdired--restore-properties (beg end _len)
(save-match-data
(save-excursion
- (let ((lep (line-end-position))
- (used-F (dired-check-switches
- dired-actual-switches
- "F" "classify")))
- ;; Deleting the space between the link name and the arrow (a
- ;; noop) also deletes the end-name property, so restore it.
- (when (and (save-excursion
- (re-search-backward dired-permission-flags-regexp nil t)
- (looking-at "l"))
- (get-text-property (1- (point)) 'dired-filename)
- (not (get-text-property (point) 'dired-filename))
- (not (get-text-property (point) 'end-name)))
+ (save-restriction
+ (widen)
+ (let ((lep (line-end-position))
+ (used-F (dired-check-switches
+ dired-actual-switches
+ "F" "classify")))
+ ;; Deleting the space between the link name and the arrow (a
+ ;; noop) also deletes the end-name property, so restore it.
+ (when (and (save-excursion
+ (re-search-backward dired-permission-flags-regexp nil t)
+ (looking-at "l"))
+ (get-text-property (1- (point)) 'dired-filename)
+ (not (get-text-property (point) 'dired-filename))
+ (not (get-text-property (point) 'end-name)))
(put-text-property (point) (1+ (point)) 'end-name t))
- (beginning-of-line)
- (when (re-search-forward
- directory-listing-before-filename-regexp lep t)
- (setq beg (point)
- end (if (or
- ;; If the file is a symlink, put the
- ;; dired-filename property only on the link
- ;; name. (Using (file-symlink-p
- ;; (dired-get-filename)) fails in
- ;; wdired-mode, bug#32673.)
- (and (re-search-backward
- dired-permission-flags-regexp nil t)
- (looking-at "l")
- ;; macOS and Ultrix adds "@" to the end
- ;; of symlinks when using -F.
- (if (and used-F
- dired-ls-F-marks-symlinks)
- (re-search-forward "@? -> " lep t)
- (search-forward " -> " lep t)))
- ;; When dired-listing-switches includes "F"
- ;; or "classify", don't treat appended
- ;; indicator characters as part of the file
- ;; name (bug#34915).
- (and used-F
- (re-search-forward "[*/@|=>]$" lep t)))
- (goto-char (match-beginning 0))
- lep))
- (put-text-property beg end 'dired-filename t))))))
+ (beginning-of-line)
+ (when (re-search-forward
+ directory-listing-before-filename-regexp lep t)
+ (setq beg (point)
+ end (if (or
+ ;; If the file is a symlink, put the
+ ;; dired-filename property only on the link
+ ;; name. (Using (file-symlink-p
+ ;; (dired-get-filename)) fails in
+ ;; wdired-mode, bug#32673.)
+ (and (re-search-backward
+ dired-permission-flags-regexp nil t)
+ (looking-at "l")
+ ;; macOS and Ultrix adds "@" to the end
+ ;; of symlinks when using -F.
+ (if (and used-F
+ dired-ls-F-marks-symlinks)
+ (re-search-forward "@? -> " lep t)
+ (search-forward " -> " lep t)))
+ ;; When dired-listing-switches includes "F"
+ ;; or "classify", don't treat appended
+ ;; indicator characters as part of the file
+ ;; name (bug#34915).
+ (and used-F
+ (re-search-forward "[*/@|=>]$" lep t)))
+ (goto-char (match-beginning 0))
+ lep))
+ (put-text-property beg end 'dired-filename t)))))))
(defun wdired-next-line (arg)
"Move down lines then position at filename or the current column.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply related [flat|nested] 7+ messages in thread* bug#49124: Wdired doesn't like re-search-forward/replace-match
2021-06-21 13:11 ` Lars Ingebrigtsen
@ 2021-06-21 21:59 ` Michael Heerdegen
2021-07-19 17:04 ` Lars Ingebrigtsen
0 siblings, 1 reply; 7+ messages in thread
From: Michael Heerdegen @ 2021-06-21 21:59 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 49124, Eduardo Ochs
Lars Ingebrigtsen <larsi@gnus.org> writes:
> I guess you're suggesting the change below?
Yes, exactly, thanks (hoping we have added all necessary kinds of
"excursions" to that functions now).
Regards,
Michael.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#49124: Wdired doesn't like re-search-forward/replace-match
2021-06-21 21:59 ` Michael Heerdegen
@ 2021-07-19 17:04 ` Lars Ingebrigtsen
0 siblings, 0 replies; 7+ messages in thread
From: Lars Ingebrigtsen @ 2021-07-19 17:04 UTC (permalink / raw)
To: Michael Heerdegen; +Cc: 49124, Eduardo Ochs
Michael Heerdegen <michael_heerdegen@web.de> writes:
>> I guess you're suggesting the change below?
>
> Yes, exactly, thanks (hoping we have added all necessary kinds of
> "excursions" to that functions now).
There was no response from Eduardo, but I went ahead and applied the
patch anyway, and I'm now closing the bug report.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-19 17:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-20 0:33 bug#49124: Wdired doesn't like re-search-forward/replace-match Eduardo Ochs
2021-06-20 1:28 ` Michael Heerdegen
2021-06-20 1:45 ` Eduardo Ochs
2021-06-20 2:12 ` Michael Heerdegen
2021-06-21 13:11 ` Lars Ingebrigtsen
2021-06-21 21:59 ` Michael Heerdegen
2021-07-19 17:04 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).