GNU bug report logs - #49124
Wdired doesn't like re-search-forward/replace-match

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49124 in the body.
You can then email your comments to 49124 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eduardo Ochs <eduardoochs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Wdired doesn't like re-search-forward/replace-match
Date: Sat, 19 Jun 2021 21:33:37 -0300

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

Message #8 received at 49124 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Eduardo Ochs <eduardoochs <at> gmail.com>
Cc: 49124 <at> debbugs.gnu.org
Subject: Re: bug#49124: Wdired doesn't like re-search-forward/replace-match
Date: Sun, 20 Jun 2021 03:28:56 +0200

Eduardo Ochs <eduardoochs <at> 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.

Message #11 received at 49124 <at> debbugs.gnu.org (full text, mbox):

From: Eduardo Ochs <eduardoochs <at> gmail.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 49124 <at> debbugs.gnu.org
Subject: Re: bug#49124: Wdired doesn't like re-search-forward/replace-match
Date: Sat, 19 Jun 2021 22:45:25 -0300

On Sat, 19 Jun 2021 at 22:28, Michael Heerdegen
<michael_heerdegen <at> web.de> wrote:
>
> Eduardo Ochs <eduardoochs <at> 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

Message #14 received at 49124 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Eduardo Ochs <eduardoochs <at> gmail.com>
Cc: 49124 <at> debbugs.gnu.org
Subject: Re: bug#49124: Wdired doesn't like re-search-forward/replace-match
Date: Sun, 20 Jun 2021 04:12:37 +0200

Eduardo Ochs <eduardoochs <at> 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.

Message #17 received at 49124 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 49124 <at> debbugs.gnu.org, Eduardo Ochs <eduardoochs <at> gmail.com>
Subject: Re: bug#49124: Wdired doesn't like re-search-forward/replace-match
Date: Mon, 21 Jun 2021 15:11:50 +0200

Michael Heerdegen <michael_heerdegen <at> 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

Message #22 received at 49124 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 49124 <at> debbugs.gnu.org, Eduardo Ochs <eduardoochs <at> gmail.com>
Subject: Re: bug#49124: Wdired doesn't like re-search-forward/replace-match
Date: Mon, 21 Jun 2021 23:59:27 +0200

Lars Ingebrigtsen <larsi <at> 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.

Message #27 received at 49124 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 49124 <at> debbugs.gnu.org, Eduardo Ochs <eduardoochs <at> gmail.com>
Subject: Re: bug#49124: Wdired doesn't like re-search-forward/replace-match
Date: Mon, 19 Jul 2021 19:04:09 +0200

Michael Heerdegen <michael_heerdegen <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.