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

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

[Madhu]

[-- Attachment #1: Type: Text/Plain, Size: 871 bytes --]

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)

[-- Attachment #2: 0001-lisp-wdired.el-wdired-change-to-wdired-mode-call-fon.patch --]
[-- Type: Text/X-Patch, Size: 897 bytes --]

From 05c8405a30a36098c55e4f31a1ec339719ccbcb3 Mon Sep 17 00:00:00 2001
From: Madhu <enometh@net.meer>
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

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Madhu <enometh@meer.net> writes:

> (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.

Thanks for reporting this here.  I CC Juri Linkov.

> (However there still seems to be a boostrap related problem with
> "Match data clobbered by buffer modification hooks" when wdired is
> first loaded)

Could you please post a recipe and a backtrace for this second problem?

> ---
>  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 \

Yip.  When we do this (guess we don't have a choice), my preferred
solution would be to hook this into the corresponding isearch function.
Because calling `font-lock-ensure' can be really slow in large dired
buffers (several seconds).  Or we manage to rewrite things so that the
work is done on the fly in some way.


Michael.

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

[Madhu]

[-- Attachment #1: Type: Text/Plain, Size: 604 bytes --]

No problem,
*  Michael Heerdegen <michael_heerdegen@web.de> <87seug85hh.fsf@web.de>
Wrote on Wed, 04 Sep 2024 05:25:46 +0200
> Could you please post a recipe and a backtrace for this second problem?

```
emacs -Q -l f.el
```

produces the backtrace in the file backtrace.txt with the match-data
clobbered error. At this point (i.e. after the error occurs) if I do
"M-x load-library wdired", then a subsequent replace-regexp operation
succeeds.  However in a long running emacs I believe I still see the
match-data clobbered error crops up again, I'm keeping a lookout on
what might be triggering that.



[-- Attachment #2: backtrace.txt --]
[-- Type: Text/Plain, Size: 564 bytes --]

Debugger entered--Lisp error: (error "Match data clobbered by buffer modification hooks")
  replace-match("bar" nil nil)
  replace-match-maybe-edit("bar" nil nil nil (170 173 #<buffer test-foo>) nil)
  perform-replace("foo" "bar" nil t nil nil nil nil nil nil nil)
  replace-regexp("foo" "bar")
  eval-buffer(#<buffer  *load*> nil "/dev/shm/f.el" nil t)  ; Reading at buffer position 359
  load-with-code-conversion("/dev/shm/f.el" "/dev/shm/f.el" nil t)
  load("/dev/shm/f.el" nil t)
  command-line-1(("-l" "/dev/shm/f.el"))
  command-line()
  normal-top-level()

[-- Attachment #3: f.el --]
[-- Type: Text/Plain, Size: 383 bytes --]

(setq $test-foo-dir "/tmp/test-foo/")
(ignore-errors (make-directory $test-foo-dir))
(ignore-errors
  (loop for i below 40
	for target = (format "/foo/%d" i)
	for link-name = (format "%s%d" $test-foo-dir i)
	do (make-symbolic-link target link-name)))
(dired $test-foo-dir)
(wdired-change-to-wdired-mode)
(load-library "wdired")
(toggle-debug-on-error t)
(replace-regexp "foo" "bar")

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

[Madhu]

*  Madhu <enometh@meer.net> <20240904.142831.1583368143272051709.enometh@meer.net>
Wrote on Wed, 04 Sep 2024 14:28:31 +0530 (IST)
> produces the backtrace in the file backtrace.txt with the match-data
> clobbered error. At this point (i.e. after the error occurs) if I do
> "M-x load-library wdired", then a subsequent replace-regexp operation
> succeeds.  However in a long running emacs I believe I still see the
> match-data clobbered error crops up again, I'm keeping a lookout on
> what might be triggering that.

I think this error goes away with an ;;;###autoload cookie for
wdired--before-change-fn in wdired.el

(The actual sequence of events is obscure)

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

[Juri Linkov]

>> +  (font-lock-ensure)
>>    (run-mode-hooks 'wdired-mode-hook)
>>    (message "%s" (substitute-command-keys
>>  		 "Press \\[wdired-finish-edit] when finished \
>
> Yip.  When we do this (guess we don't have a choice), my preferred
> solution would be to hook this into the corresponding isearch function.
> Because calling `font-lock-ensure' can be really slow in large dired
> buffers (several seconds).  Or we manage to rewrite things so that the
> work is done on the fly in some way.

So you prefer to slow down only when the user types C-s?
This is possible by adding a local hook in
wdired-change-to-wdired-mode:

  (add-hook 'isearch-mode-hook #'font-lock-ensure nil t)

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juri Linkov <juri@linkov.net> writes:

> So you prefer to slow down only when the user types C-s?
> This is possible by adding a local hook in
> wdired-change-to-wdired-mode:
>
>   (add-hook 'isearch-mode-hook #'font-lock-ensure nil t)

A step back: I now tried to reproduce the recipe, but I only see the
clobbered match data error (randomly) - I don't see only the visible
buffer portion operated on.  With other words: I can't reproduce the
issue, at least not with the "visible buffer portion" interpretation,
and I don't see that we would need to call `font-lock-ensure' at all.

Second: I'm confused.  Apparently, when `dired-isearch-filenames-mode'
is on, why do `search-forward-regexp' and `replace-regexp' behave
differently?  `search-forward-regexp' does find matches outside of file
names that `replace-regexp' ignores.


Michael.

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

[Madhu]

*  Michael Heerdegen <michael_heerdegen@web.de> <874j6uz4d8.fsf@web.de>
Wrote on Thu, 05 Sep 2024 14:12:19 +0200

apparently the autoload cookie fix that i suggested to wdired.el
wdired--before-change-fn (and recompiling) only gets rid of the match
data clobbered error when I have already applied the earlier
font-lock-ensure patch.

also the recipe is flawed, I apologize. [i didn't (require 'cl-lib)
which is required for emacs -Q]

But if you are in a dired buffer looking at /tmp/test-foo with 40
entries of symlinks of the form n -> foo/n This is the behaviour I get
on emacs -q:

1. C-x C-q
2. M-x rep-reg "foo" "bar" RET
;; get a  match data clobbered  error (q), C-c C-k to revert dired
3. M-x load-library "dired" RET  ;;makes the match-data clobbered error go away
4. C-x C-q
5. M-x rep-reg "foo" "bar" RET

I get "Replaced 19 occurrences" and the point ends up on the last
entry, the remaining entries have not been replaced.

I confirmed this twice before posting

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

[Juri Linkov]

>> So you prefer to slow down only when the user types C-s?
>> This is possible by adding a local hook in
>> wdired-change-to-wdired-mode:
>>
>>   (add-hook 'isearch-mode-hook #'font-lock-ensure nil t)
>
> A step back: I now tried to reproduce the recipe, but I only see the
> clobbered match data error (randomly) - I don't see only the visible
> buffer portion operated on.  With other words: I can't reproduce the
> issue, at least not with the "visible buffer portion" interpretation,
> and I don't see that we would need to call `font-lock-ensure' at all.

Maybe this is reproducible only on very long Dired buffers?

> Second: I'm confused.  Apparently, when `dired-isearch-filenames-mode'
> is on, why do `search-forward-regexp' and `replace-regexp' behave
> differently?  `search-forward-regexp' does find matches outside of file
> names that `replace-regexp' ignores.

`replace-regexp' uses Isearch functions,
`search-forward-regexp' is a core function that doesn't use Isearch.

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juri Linkov <juri@linkov.net> writes:

> Maybe this is reproducible only on very long Dired buffers?

I tried in a buffer with over 19,000 files.  I should have experienced a
problem (no matches found, nothing changed) if font-lock would be
related.  But every function involved always found matches hundreds of
screens below the current position.  I reloaded the buffer after each
experiment to be sure only the currently visited area was ever displayed.

OTOH, I did see the match-data issue occur.  Maybe this is the only
reason of our problems.  I would focus on trying to understand that
problem.


> > Second: I'm confused.  Apparently, when `dired-isearch-filenames-mode'
> > is on, why do `search-forward-regexp' and `replace-regexp' behave
> > differently?  `search-forward-regexp' does find matches outside of file
> > names that `replace-regexp' ignores.
>
> `replace-regexp' uses Isearch functions,
> `search-forward-regexp' is a core function that doesn't use Isearch.

It's a not-so-nice inconsistency, but ok...


Michael.

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> > Maybe this is reproducible only on very long Dired buffers?

After following the recipe literally, I could reproduce that thing, too.

Maybe this issue occurring depends on what exactly is replaced - symlink
targets.  In this case, (font-lock-ensure) does make a
difference.

Yesterday I had experimented with replacing in symlink names - in that
case, the whole buffer had been considered.


Michael.

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

[Juri Linkov]

>> > Maybe this is reproducible only on very long Dired buffers?
>
> After following the recipe literally, I could reproduce that thing, too.
>
> Maybe this issue occurring depends on what exactly is replaced - symlink
> targets.  In this case, (font-lock-ensure) does make a
> difference.
>
> Yesterday I had experimented with replacing in symlink names - in that
> case, the whole buffer had been considered.

I tried this:

(dired "/dev/char")

M-: (buffer-substring (- (point-max) 2) (- (point-max) 1))
=> #("7" 0 1 (fontified nil invisible dired-hide-details-link))

M-> ;; (end-of-buffer)

M-: (buffer-substring (- (point-max) 2) (- (point-max) 1))
=> #("7" 0 1 (face default dired-symlink-filename t fontified t invisible dired-hide-details-link))

And indeed, after going to the end of the Dired buffer
the last file gets an additional property `dired-symlink-filename'
used by Isearch/Replace.

Also noticed that doing the first replacement always raises an error:

Debugger entered--Lisp error: (error "Match data clobbered by buffer modification hooks")
  replace-match("!" nil nil)
  replace-match-maybe-edit("!" nil nil nil (672 673 #<buffer char>) nil)
  perform-replace("7" "!" t t nil nil nil nil nil nil nil)
  query-replace-regexp("7" "!" nil nil nil nil nil)
  funcall-interactively(query-replace-regexp "7" "!" nil nil nil nil nil)
  command-execute(query-replace-regexp)

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

Juri Linkov <juri@linkov.net> writes:

> M-: (buffer-substring (- (point-max) 2) (- (point-max) 1))
> => #("7" 0 1 (fontified nil invisible dired-hide-details-link))
>
> M-> ;; (end-of-buffer)
>
> M-: (buffer-substring (- (point-max) 2) (- (point-max) 1))
> => #("7" 0 1 (face default dired-symlink-filename t fontified t
> invisible dired-hide-details-link))
>
> And indeed, after going to the end of the Dired buffer
> the last file gets an additional property `dired-symlink-filename'
> used by Isearch/Replace.

So, the properties we use are of different types: some are already
attached by `dired-insert-set-properties', others later by font-lock
(see `dired-font-lock-keywords').  AFAIR this is because some tests are
more expensive than others (e.g. the test whether a link is broken) and
are intentionally delayed.

Would something like this be good?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Bug-73018.patch --]
[-- Type: text/x-diff, Size: 1100 bytes --]

From c5ef8c8db7d8b2ab03998a0f3f2a1b820ff24e2a Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Mon, 9 Sep 2024 16:46:13 +0200
Subject: [PATCH] WIP: Bug#73018

---
 lisp/dired-aux.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index d79ec342435..77dde7147bc 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3740,8 +3740,12 @@ dired-isearch-search-filenames
 The returned function narrows the search to match the search string
 only as part of a file name enclosed by the text property `dired-filename'.
 It's intended to override the default search function."
-  (isearch-search-fun-in-text-property
-   (funcall orig-fun) '(dired-filename dired-symlink-filename)))
+  (let ((search-fun
+         (isearch-search-fun-in-text-property
+          (funcall orig-fun) '(dired-filename dired-symlink-filename))))
+    (lambda (&rest args)
+      (font-lock-ensure)
+      (apply search-fun args))))

 ;;;###autoload
 (defun dired-isearch-filenames ()
--
2.39.2


[-- Attachment #3: Type: text/plain, Size: 1153 bytes --]


A related question is whether everybody always wants to search in
symlink targets when isearching file names in dired... I don't.  Would
it be worth to add an option for that?  Currently the properties are
just hardcoded.

Then, in the above patch we could make the `font-lock-ensure' call
depend on the value of that option.


> Also noticed that doing the first replacement always raises an error:
>
> Debugger entered--Lisp error: (error "Match data clobbered by buffer
> modification hooks")
>   replace-match("!" nil nil)
>   replace-match-maybe-edit("!" nil nil nil (672 673 #<buffer char>) nil)
>   perform-replace("7" "!" t t nil nil nil nil nil nil nil)
>   query-replace-regexp("7" "!" nil nil nil nil nil)
>   funcall-interactively(query-replace-regexp "7" "!" nil nil nil nil nil)
>   command-execute(query-replace-regexp)

Do I interpret the code in replace_match correctly: this error doesn't
even mean the match data has been clobbered - only that modification
hooks called searching functions?  I don't know what the referenced
search_regs.num_regs exactly contains.  But we already seem to ensure
not to clobber match data.


Michael.

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

[Juri Linkov]

> Would something like this be good?
>
> @@ -3740,8 +3740,12 @@ dired-isearch-search-filenames
> -  (isearch-search-fun-in-text-property
> -   (funcall orig-fun) '(dired-filename dired-symlink-filename)))
> +  (let ((search-fun
> +         (isearch-search-fun-in-text-property
> +          (funcall orig-fun) '(dired-filename dired-symlink-filename))))
> +    (lambda (&rest args)
> +      (font-lock-ensure)
> +      (apply search-fun args))))

This will call 'font-lock-ensure' for every search hit?
Wouldn't it be better to call 'font-lock-ensure' only once
at the beginning of the search?

> A related question is whether everybody always wants to search in
> symlink targets when isearching file names in dired... I don't.  Would
> it be worth to add an option for that?  Currently the properties are
> just hardcoded.
>
> Then, in the above patch we could make the `font-lock-ensure' call
> depend on the value of that option.

Maybe not an option, but just a simple variable?

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juri Linkov <juri@linkov.net> writes:

> > Would something like this be good?
> >
> > @@ -3740,8 +3740,12 @@ dired-isearch-search-filenames
> > -  (isearch-search-fun-in-text-property
> > -   (funcall orig-fun) '(dired-filename dired-symlink-filename)))
> > +  (let ((search-fun
> > +         (isearch-search-fun-in-text-property
> > +          (funcall orig-fun) '(dired-filename dired-symlink-filename))))
> > +    (lambda (&rest args)
> > +      (font-lock-ensure)
> > +      (apply search-fun args))))
>
> This will call 'font-lock-ensure' for every search hit?

Right - this is the wrong place to add it.

The idea was to make the search function itself know that it has to care
about font-locking.  We could still save the information in the function
by using an oclosure, though.

Michael.

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

[Juri Linkov]

>> Also noticed that doing the first replacement always raises an error:
>>
>> Debugger entered--Lisp error: (error "Match data clobbered by buffer
>> modification hooks")
>>   replace-match("!" nil nil)
>>   replace-match-maybe-edit("!" nil nil nil (672 673 #<buffer char>) nil)
>>   perform-replace("7" "!" t t nil nil nil nil nil nil nil)
>>   query-replace-regexp("7" "!" nil nil nil nil nil)
>>   funcall-interactively(query-replace-regexp "7" "!" nil nil nil nil nil)
>>   command-execute(query-replace-regexp)
>
> Do I interpret the code in replace_match correctly: this error doesn't
> even mean the match data has been clobbered - only that modification
> hooks called searching functions?  I don't know what the referenced
> search_regs.num_regs exactly contains.  But we already seem to ensure
> not to clobber match data.

It fails in emacs-30, but not in emacs-29.
So this is a regression.

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

[Juri Linkov]

>>> Also noticed that doing the first replacement always raises an error:
>>>
>>> Debugger entered--Lisp error: (error "Match data clobbered by buffer
>>> modification hooks")
>>>   replace-match("!" nil nil)
>>>   replace-match-maybe-edit("!" nil nil nil (672 673 #<buffer char>) nil)
>>>   perform-replace("7" "!" t t nil nil nil nil nil nil nil)
>>>   query-replace-regexp("7" "!" nil nil nil nil nil)
>>>   funcall-interactively(query-replace-regexp "7" "!" nil nil nil nil nil)
>>>   command-execute(query-replace-regexp)
>>
>> Do I interpret the code in replace_match correctly: this error doesn't
>> even mean the match data has been clobbered - only that modification
>> hooks called searching functions?  I don't know what the referenced
>> search_regs.num_regs exactly contains.  But we already seem to ensure
>> not to clobber match data.
>
> It fails in emacs-30, but not in emacs-29.
> So this is a regression.

This is caused by commit 63588775fcb, so Cc-ing Stefan.

But probably this commit just exposed the problem
that existed before?

Anyway here is 100% reproducible recipe:

0. emacs -Q
1. 'C-x C-q' in a Dired with symlinks
2. 'C-M-% some_part_of_symlink RET anything RET'
3. 'y'

=> (error "Match data clobbered by buffer modification hooks")

This happens only for the first replacement after `emacs -Q`.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> This is caused by commit 63588775fcb, so Cc-ing Stefan.
>
> But probably this commit just exposed the problem
> that existed before?

I think you're on to something.

> => (error "Match data clobbered by buffer modification hooks")

This comes from

    if (search_regs.num_regs != num_regs)
      error ("Match data clobbered by buffer modification hooks");

But this test is not doing what it is intended to do: it doesn't check
whether the match data has changed.  It just checks whether the size of
the arrays we have allocated to hold the match data has changed.

I got to that conclusion after narrowing down the origin of the error
and finding out that the problem is that `search_regs.num_regs` is set
to 13 before the first call to `wdired--restore-properties` but to
23 afterwards.

We should probably use something like

    ptrdiff_t
    search_regs_last_reg (void)
    {
      ptrdiff_t i = search_regs.num_regs - 1;
      while (i >= 0 && search_regs.start[i] < 0)
        i--;
      return i;
    }

instead of `search_regs.num_regs`.


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> We should probably use something like
>
>     ptrdiff_t
>     search_regs_last_reg (void)
>     {
>       ptrdiff_t i = search_regs.num_regs - 1;
>       while (i >= 0 && search_regs.start[i] < 0)
>         i--;
>       return i;
>     }
>
> instead of `search_regs.num_regs`.

And maybe we should consider changing `struct re_search` to keep track
of the last non-nil subgroup.  Currently, things like `match-data` loop
through all the elements of `search_regs` whose size depends on the
size (in number of subgroups) of the largest regexp we have ever matched
so far, rather than the actual number of subgroups currently in use, so
there's some wasted work there.  Maybe the cost of maintaining the "last
reg" would pay off.


        Stefan

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

[Eli Zaretskii]

> Cc: Michael Heerdegen <michael_heerdegen@web.de>, Madhu <enometh@meer.net>,
>  73018@debbugs.gnu.org
> Date: Tue, 10 Sep 2024 09:27:31 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > We should probably use something like
> >
> >     ptrdiff_t
> >     search_regs_last_reg (void)
> >     {
> >       ptrdiff_t i = search_regs.num_regs - 1;
> >       while (i >= 0 && search_regs.start[i] < 0)
> >         i--;
> >       return i;
> >     }
> >
> > instead of `search_regs.num_regs`.
> 
> And maybe we should consider changing `struct re_search` to keep track
> of the last non-nil subgroup.  Currently, things like `match-data` loop
> through all the elements of `search_regs` whose size depends on the
> size (in number of subgroups) of the largest regexp we have ever matched
> so far, rather than the actual number of subgroups currently in use, so
> there's some wasted work there.  Maybe the cost of maintaining the "last
> reg" would pay off.

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?

TIA

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > We should probably use something like
>> >
>> >     ptrdiff_t
>> >     search_regs_last_reg (void)
>> >     {
>> >       ptrdiff_t i = search_regs.num_regs - 1;
>> >       while (i >= 0 && search_regs.start[i] < 0)
>> >         i--;
>> >       return i;
>> >     }
>> >
>> > instead of `search_regs.num_regs`.
>> 
>> And maybe we should consider changing `struct re_search` to keep track
>> of the last non-nil subgroup.  Currently, things like `match-data` loop
>> through all the elements of `search_regs` whose size depends on the
>> size (in number of subgroups) of the largest regexp we have ever matched
>> so far, rather than the actual number of subgroups currently in use, so
>> there's some wasted work there.  Maybe the cost of maintaining the "last
>> reg" would pay off.
>
> 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.  Since `search_regs.num_regs` keeps
track only of the size of the array (rather than the part of it
currently in use), and the array is never shrunk, it changes *very* few
times in an Emacs session, so this check we have triggers
*extremely* rarely.

The "correct" change I suggest above would definitely not be good for
the release branch since it will likely catch more cases (and thus
introduce regressions where code used to "work" but now signals an
error).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: juri@linkov.net,  michael_heerdegen@web.de,  enometh@meer.net,
>   73018@debbugs.gnu.org
> Date: Sun, 15 Sep 2024 09:04:40 -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.  Frankly, I'd rather revert the offending change which
caused the regression and let Emacs 30 live with bug#65451, which it
was supposed to fix.

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?

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

[Madhu]

[-- Attachment #1: Type: Text/Plain, Size: 250 bytes --]


fwiw in a build on master with the earlier patch i posted (which turns
on font-lock-mode, admittedly overkill), and the autoload cookie patch
(attached) and a fresh compile I do not seem to trigger the
replace-match error (with the wdired recipe)



[-- Attachment #2: 0001-lisp-wdired.el-wdired-before-change-fn-autoload-cook.patch --]
[-- Type: Text/X-Patch, Size: 691 bytes --]

From 41e4505815f58ee69255801a55dc07394f729930 Mon Sep 17 00:00:00 2001
From: Madhu <enometh@net.meer>
Date: Wed, 4 Sep 2024 15:05:26 +0530
Subject: [PATCH] lisp/wdired.el: (wdired--before-change-fn): autoload cookie
 prevents a "match data clobbered" error

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

diff --git a/lisp/wdired.el b/lisp/wdired.el
index dd8b8640a89..6f1f7b06e35 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -306,6 +306,7 @@ wdired--self-insert
 
 (put 'wdired--self-insert 'delete-selection 'delete-selection-uses-region-p)
 
+;;;###autoload
 (defun wdired--before-change-fn (beg end)
   (save-match-data
     (save-excursion
-- 
2.46.0.27.gfa3b914457

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

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Madhu <enometh@meer.net> writes:

> fwiw in a build on master with the earlier patch i posted (which turns
> on font-lock-mode, admittedly overkill), and the autoload cookie patch
> (attached) and a fresh compile I do not seem to trigger the
> replace-match error (with the wdired recipe)

Do you have an explanation for why this should make a difference?

Because: all references to `wdired--before-change-fn' are in "wdired"
itself, and whenever the function is called, "wdired" is necessarily
already loaded.

Michael.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Do you have an explanation for why this should make a difference?

The bug shows up if the `search_regs.start/end` happen to be resized
(grown) during the modification hooks (and only if that happens when
running those hooks inside `replace_match`).

Those arrays are resized only when we use a regexp that has more groups
than the "largest" regexp we've encountered so far.  So the problem
disappears as soon as we use the "largest" regexp time before we use it
from within a change hook run from within `replace_match`.


        Stefan

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > 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 <eliz@gnu.org>
    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 <npostavs@gmail.com>
    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 <npostavs@gmail.com>
    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).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: michael_heerdegen@web.de,  enometh@meer.net,  Ihor Radchenko
>  <yantar92@posteo.net>,  73018@debbugs.gnu.org,  juri@linkov.net
> 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 <eliz@gnu.org>
>     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 <npostavs@gmail.com>
>     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 <npostavs@gmail.com>
>     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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

> bug#65451, sigh).  I did install the change you suggested on master.
> And with that, I'm closing this bug.

BTW, for `master` I think a better change is to fix the check.
E.g. with the patch below.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: search.patch --]
[-- Type: text/x-diff, Size: 2836 bytes --]

diff --git a/src/search.c b/src/search.c
index 24b1ee6bd3f..718d23b919e 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2356,6 +2356,18 @@ DEFUN ("posix-search-forward", Fposix_search_forward, Sposix_search_forward, 1,
 {
   return search_command (regexp, bound, noerror, count, 1, true, true);
 }
+
+/* Return the number of search regs currently used.
+   FIXME: Maybe we should keep this as a field in `search_regs` rather
+   than (re)computing it dynamically.  */
+static ptrdiff_t
+search_regs_num_used (void)
+{
+  ptrdiff_t last = search_regs.num_regs - 1;
+  while (last >= 0 && search_regs.start[last] < 0)
+    last--;
+  return last + 1;
+}
 \f
 DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
        doc: /* Replace text matched by last search with NEWTEXT.
@@ -2759,11 +2771,22 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
     }
 
   newpoint = sub_start + SCHARS (newtext);
+  ptrdiff_t num_used = search_regs_num_used ();
 
   /* Replace the old text with the new in the cleanest possible way.  */
   replace_range (sub_start, sub_end, newtext, 1, 0, 1, true, true);
   signal_after_change (sub_start, sub_end - sub_start, SCHARS (newtext));
 
+  if (search_regs_num_used () != num_used)
+    /* 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.  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. */
+    error ("Match data clobbered by buffer modification hooks");
+
   if (case_action == all_caps)
     Fupcase_region (make_fixnum (search_regs.start[sub]),
 		    make_fixnum (newpoint),
@@ -2869,7 +2892,8 @@ DEFUN ("match-data", Fmatch_data, Smatch_data, 0, 3, 0,
 {
   Lisp_Object tail, prev;
   Lisp_Object *data;
-  ptrdiff_t i, len;
+  ptrdiff_t i, used = search_regs_num_used () + 1;
+  ptrdiff_t len = used * 2;
 
   if (!NILP (reseat))
     for (tail = reuse; CONSP (tail); tail = XCDR (tail))
@@ -2885,10 +2909,9 @@ DEFUN ("match-data", Fmatch_data, Smatch_data, 0, 3, 0,
   prev = Qnil;
 
   USE_SAFE_ALLOCA;
-  SAFE_NALLOCA (data, 1, 2 * search_regs.num_regs + 1);
+  SAFE_NALLOCA (data, 1, 2 * used + 1);
 
-  len = 0;
-  for (i = 0; i < search_regs.num_regs; i++)
+  for (i = 0; i < used; i++)
     {
       ptrdiff_t start = search_regs.start[i];
       if (start >= 0)
@@ -2913,8 +2936,6 @@ DEFUN ("match-data", Fmatch_data, Smatch_data, 0, 3, 0,
 	  else
 	    /* last_thing_searched must always be Qt, a buffer, or Qnil.  */
 	    emacs_abort ();
-
-	  len = 2 * i + 2;
 	}
       else
 	data[2 * i] = data[2 * i + 1] = Qnil;

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: michael_heerdegen@web.de,  enometh@meer.net,  yantar92@posteo.net,
>   73018-done@debbugs.gnu.org,  juri@linkov.net
> Date: Sun, 22 Sep 2024 23:43:03 -0400
> 
> > bug#65451, sigh).  I did install the change you suggested on master.
> > And with that, I'm closing this bug.
> 
> BTW, for `master` I think a better change is to fix the check.
> E.g. with the patch below.

Feel free to install, if you are satisfied with this.