Subject: bug#73018: closed (Re: bug#73018: 31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 21 Sep 2024 09:35:02 +0000

Your bug report

#73018: 31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
73018: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73018
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#73018: 31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer
From: Eli Zaretskii <[email-masked]>
To: Stefan Monnier <[email-masked]>
Cc: [email-masked], [email-masked], [email-masked], [email-masked], [email-masked]
Date: Sat, 21 Sep 2024 12:34:12 +0300
List-Id: <debbugs-submit.debbugs.gnu.org>

> From: Stefan Monnier <[email-masked]>
> Cc: [email-masked],  [email-masked],  Ihor Radchenko
>  <[email-masked]>,  [email-masked],  [email-masked]
> Date: Tue, 17 Sep 2024 14:52:57 -0400
> 
> >> > Since this is a regression in Emacs 30, I'd like to solve it on the
> >> > release branch.  Can you suggest the safest fix you can come up with
> >> > for that purpose?
> >> 
> >> Oh, yes: just remove the check.
> >
> > Whoa!  We had that check there for 9 years, and it was introduced to
> > avoid crashes (see bug#23869), so removing it now, during a pretest,
> > is scary.
> 
> Here's the story I see:
> In response to that bug, you proposed to add:
> 
>     /* Last line of defense, in case search registers were actually not
>        saved (because someone else already occupied the save slots).  */
>     if (search_regs.start[sub] != sub_start
>         || search_regs.end[sub] != sub_end)
>       error ("Match data clobbered by buffer modification hooks");
> 
> In the end, you added:
> 
>     commit 3a9d6296b35e5317c497674d5725eb52699bd3b8
>     Author: Eli Zaretskii <[email-masked]>
>     Date:   Mon Jul 4 18:34:40 2016 +0300
>     
>         Avoid crashes when buffer modification hooks clobber match data
>         
>         * src/search.c (Freplace_match): Error out if buffer modification
>         hooks triggered by buffer changes in replace_range, upcase-region,
>         and upcase-initials-region clobber the match data needed to be
>         adjusted for the replacement.  (Bug#23869)
>     
>     diff --git a/src/search.c b/src/search.c
>     --- a/src/search.c
>     +++ b/src/search.c
>     @@ -2699,0 +2707,5 @@
>     +  if (search_regs.start[sub] != sub_start
>     +      || search_regs.end[sub] != sub_end
>     +      || search_regs.num_regs != num_regs)
>     +    error ("Match data clobbered by buffer modification hooks");
> 
> A bit later we dropped the start/end part (for a reason I'm not sure is
> valid, since change hooks that modify the buffer should be disallowed,
> I think):
> 
>     commit 487498e497f8c6b6303bd5feeac83a5bcc2315af
>     Author: Noam Postavsky <[email-masked]>
>     Date:   Sun May 16 15:19:57 2021 +0200
>     
>         Remove unreliable test for match data clobbering
>         
>         * src/search.c (Freplace_match): Don't test for change in search_regs
>         start and end, this is unreliable if change hooks modify text earlier
>         in the buffer (bug#35264).
>     
>     diff --git a/src/search.c b/src/search.c
>     --- a/src/search.c
>     +++ b/src/search.c
>     @@ -2739,10 +2738,10 @@
>        /* The replace_range etc. functions can trigger modification hooks
>           (see signal_before_change and signal_after_change).  Try to error
>           out if these hooks clobber the match data since clobbering can
>     -     result in confusing bugs.  Although this sanity check does not
>     -     catch all possible clobberings, it should catch many of them.  */
>     -  if (! (search_regs.num_regs == num_regs
>     -	 && search_regs.start[sub] == newstart
>     -	 && search_regs.end[sub] == newpoint))
>     +     result in confusing bugs.  We used to check for changes in
>     +     search_regs start and end, but that fails if modification hooks
>     +     remove or add text earlier in the buffer, so just check num_regs
>     +     now. */
>     +  if (search_regs.num_regs != num_regs)
>          error ("Match data clobbered by buffer modification hooks");
> 
> So the check that remains is one that wasn't even present originally.
> 
> Also, IIUC the origin of the crash in bug#23869 is that we did:
> 
>        /* Adjust search data for this change.  */
>        {
>          ptrdiff_t oldend = search_regs.end[sub];
> 
> after running the change functions (i.e. at a time where
> `search_regs.end[sub]` might not hold the same match data and hence
> might be -1, leading to the crash).
> 
> This code is different now.  The only place where we use something like
> `search_regs.end[sub]` once it's possibly-clobbered is:
> 
>       if (case_action == all_caps)
>         Fupcase_region (make_fixnum (search_regs.start[sub]),
>     		    make_fixnum (newpoint),
>     		    Qnil);
>       else if (case_action == cap_initial)
>         Fupcase_initials_region (make_fixnum (search_regs.start[sub]),
>     			     make_fixnum (newpoint), Qnil);
> 
> both of whose functions should not crash just because they're called
> with a -1.  So I think the original crash should not happen nowadays,
> and this is because the "Adjust search data" part of the code was
> completely rewritten by:
> 
>     commit 66f95e0dabf750e9d2eff59b2bb6e593618cd48a
>     Author: Noam Postavsky <[email-masked]>
>     Date:   Wed Jul 20 20:15:14 2016 -0400
>     
>         Adjust match data before calling after-change-funs
>         
>         It's important to adjust the match data in between calling
>         before-change-functions and after-change-functions, so that buffer
>         change hooks will always see match-data consistent with buffer content.
>         (Bug #23917)
>         
>         * src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
>         true call update_search_regs.  Update all callers (except
>         Freplace_match) to pass 0 for the new parameter.
>         * src/search.c (update_search_regs): New function, extracted from
>         Freplace_match.
>         (Freplace_match): Remove match data adjustment code, pass 1 for
>         ADJUST_MATCH_DATA to replace_range instead.
> 
> > And I don't think I understand how a single line you moved in
> > 63588775fcb could cause this check to signal an error in the scenario
> > of this bug.  Can you explain?
> 
> The line-move caused the modification hooks to be run at a different
> moment: we used to run them *after* the if+error check whereas now we
> run them before.  The problem can probably be triggered in the old code
> as well if `case_action` is given a different value (in which case the
> `Fupcase_region` may also run the hooks, thus potentially causing the
> same change to the size of the `search_regs.start/end` arrays before the
> if+error check).

Thanks for the analysis.  Call me a coward, but I don't want to make
this change on the release branch.  Instead, I reverted the search.c
part of the 63588775fcb commit there (which will re-introduce
bug#65451, sigh).  I did install the change you suggested on master.

And with that, I'm closing this bug.

Subject: 31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer
From: Madhu <[email-masked]>
To: [email-masked]
Date: Wed, 04 Sep 2024 08:03:21 +0530 (IST)
List-Id: <debbugs-submit.debbugs.gnu.org>

In GNU Emacs 31.0.50 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.18.0, Xaw3d scroll bars)
Repository revision: f1e29506822739208e5706b733cfd713c5f37cfd

Ref: https://lists.gnu.org/archive/html/emacs-devel/2024-09/msg00071.html

On carrying out the following steps

```
mkdir /dev/shm/test-foo -pv
for i in $(seq 1 40); do ln -sv /foo/$i /dev/shm/test-foo; done

(dired "/dev/shm/test-foo")
(wdired-change-to-wdired-mode)
(replace-regexp "foo" "bar")
```

It is seen that only the files in the visible portion of the buffer
are affeceted by the replace-regexp. The attached patch implements the
suggestion in
https://lists.gnu.org/archive/html/emacs-devel/2024-09/msg00079.html
and appears to fix the problem.

(However there still seems to be a boostrap related problem with
"Match data clobbered by buffer modification hooks" when wdired is
first loaded)
>From 05c8405a30a36098c55e4f31a1ec339719ccbcb3 Mon Sep 17 00:00:00 2001
From: Madhu <[email-masked]>
Date: Wed, 4 Sep 2024 06:55:44 +0530
Subject: [PATCH] * lisp/wdired.el: (wdired-change-to-wdired-mode): call
 font-lock-ensure  so replace-regexp with wdired-search-replace-filenames t
 works on the whole buffer.

---
 lisp/wdired.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/wdired.el b/lisp/wdired.el
index 4b6a9c14b20..dd8b8640a89 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -264,6 +264,7 @@ wdired-change-to-wdired-mode
   ;; hidden partly, so we remove filename invisibility spec
   ;; temporarily to ensure filenames are visible for editing.
   (dired-filename-update-invisibility-spec)
+  (font-lock-ensure)
   (run-mode-hooks 'wdired-mode-hook)
   (message "%s" (substitute-command-keys
 		 "Press \\[wdired-finish-edit] when finished \
-- 
2.46.0.27.gfa3b914457