bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

emacs -Q

< C-M-% \b RET foo RET TAB TAB TAB
U ; undo all replacements
;; Nothing is undid :-(

--8<-----------------------------cut here---------------start------------->8---
commit f1ee02d0c0bef4534c895559eb53b1ac0ecfb752
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Fri May 18 22:19:18 2018 +0900

    Fix corner case in query-replace-regexp undo
    
    * lisp/replace.el (perform-replace): Handle the case of a regexp
    not having printable characters (Bug#31492).
    
    * test/lisp/replace-tests.el (query-replace-undo-bug31492): Add test.

diff --git a/lisp/replace.el b/lisp/replace.el
index 3503b656d9..526a40d230 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2692,7 +2692,9 @@ perform-replace
                                    (setq real-match-data
                                          (save-excursion
                                            (goto-char (match-beginning 0))
-                                           (looking-at search-string)
+                                           (if (/= (match-beginning 0) (match-end 0))
+					       (looking-at search-string)
+					     (looking-back search-string (- (point) (length search-string))))
                                            (match-data t (nth 2 elt)))
                                          noedit
                                          (replace-match-maybe-edit
diff --git a/test/lisp/replace-tests.el b/test/lisp/replace-tests.el
index 40a1a31cf7..5c0ec2aa1c 100644
--- a/test/lisp/replace-tests.el
+++ b/test/lisp/replace-tests.el
@@ -399,5 +399,25 @@ replace-tests--query-replace-undo
       ;; After undo text must be the same.
       (should (string= text (buffer-string))))))
 
+(ert-deftest query-replace-undo-bug31492 ()
+  "Test for https://debbugs.gnu.org/31492 ."
+  (let ((text "a\nb\nc\n")
+        (count 0)
+        (inhibit-message t))
+    (with-temp-buffer
+      (insert text)
+      (goto-char 1)
+      (cl-letf (((symbol-function 'read-event)
+                 (lambda (&rest args)
+                   (cl-incf count)
+                   (let ((val (pcase count
+                                ((or 1 2) ?\s) ; replace current and go next
+                                (3 ?U) ; undo-all
+                                (_ ?q)))) ; exit
+                     val))))
+        (perform-replace "^\\|\b\\|$" "foo" t t nil))
+      ;; After undo text must be the same.
+      (should (string= text (buffer-string))))))
+
 
 ;;; replace-tests.el ends here
--8<-----------------------------cut here---------------end--------------->8---

In GNU Emacs 26.1 (build 6, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2018-05-18 built on calancha-pc
Repository revision: 73bc6f8693fcbb98b41ee67ab35a4dd8c3940355

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 18 May 2018 22:27:36 +0900
> 
> emacs -Q
> 
> < C-M-% \b RET foo RET TAB TAB TAB
> U ; undo all replacements
> ;; Nothing is undid :-(

I'm probably missing something, because I cannot reproduce the problem
(and therefore don't understand the solution).  When I repeat what you
show above, I get "replaced 0 occurrences", and therefore there's
nothing to undo in the first place...

I guess the explanation is in what does "<" mean and, more
importantly, what did you actually type instead of "\b".

Or maybe some command is missing before the recipe begins?

TIA

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

On Fri, 18 May 2018, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Fri, 18 May 2018 22:27:36 +0900
>>
>> emacs -Q
>>
>> < C-M-% \b RET foo RET TAB TAB TAB
>> U ; undo all replacements
>> ;; Nothing is undid :-(
>
> I'm probably missing something, because I cannot reproduce the problem
> I guess the explanation is in what does "<" mean and, more
'<' stands for `beginning-of-buffer'.  Then you have text to replace in 
the *scratch* buffer.  Otherwise you at the end of the buffer and my
regexp cannot match.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 18 May 2018 23:22:13 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 31492@debbugs.gnu.org
> 
> >> emacs -Q
> >>
> >> < C-M-% \b RET foo RET TAB TAB TAB
> >> U ; undo all replacements
> >> ;; Nothing is undid :-(
> >
> > I'm probably missing something, because I cannot reproduce the problem
> > I guess the explanation is in what does "<" mean and, more
> '<' stands for `beginning-of-buffer'.  Then you have text to replace in 
> the *scratch* buffer.  Otherwise you at the end of the buffer and my
> regexp cannot match.

Sorry, I'm still in the dark.  After "emacs -Q", the current buffer is
*scratch*, and in that buffer, typing '<' inserts that character into
the buffer, it doesn't invoke beginning-of-buffer.  And 'U' doesn't
undo, either.  These are self-inserting characters in Lisp Interaction
mode.

What am I missing?

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

On Fri, 18 May 2018, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Fri, 18 May 2018 23:22:13 +0900 (JST)
>> cc: Tino Calancha <tino.calancha@gmail.com>, 31492@debbugs.gnu.org
>>
>>>> emacs -Q
>>>>
>>>> < C-M-% \b RET foo RET TAB TAB TAB
>>>> U ; undo all replacements
>>>> ;; Nothing is undid :-(
>>>
>> '<' stands for `beginning-of-buffer'.
> Sorry, I'm still in the dark. Typing '<' inserts that character
Opps... Sorry!  That should read 'M-<'.
And the TAB's should read SPC.

The correct recipe is:

M-<
C-M-% \b RET foo RET SPC SPC
U
;; All 'foo' keep there :-(

This happen because the regexp "\b" has any printable character.
If we try instead:

M-<
C-M-% is\b RET foo RET SPC SPC
U
;; Now all 'foo' are gone :-)

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sat, 19 May 2018 10:46:10 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 31492@debbugs.gnu.org
> 
> The correct recipe is:
> 
> M-<
> C-M-% \b RET foo RET SPC SPC
> U
> ;; All 'foo' keep there :-(

Thanks, I see the problem now.

However, isn't the root cause in replace--push-stack?  The relevant
element of the replacement stack (whose structure, btw, seems not to
be documented anywhere), is (4 4 *scratch*), whereas I'd expect to see
(1 4 *scratch) instead, because the replacement was at position 1;
then setting match-data from this would DTRT.

IOW, I'm afraid the looking-back solution is ad-hoc, and might not
work in general, because the real problem is elsewhere.  WDYT?

> This happen because the regexp "\b" has any printable character.

Why does it matter for \b to match a non-empty string?  The undo-all
command matches text against the _replacement_ string, not against the
original search string.  And the replacement string, "foo", is not
empty.  The problem here, AFAICT, is that we are looking for it in the
wrong place, and that happens because the replacement stack tells us
to look at position 4 instead of position 1.  Right?

> If we try instead:
> 
> M-<
> C-M-% is\b RET foo RET SPC SPC
> U
> ;; Now all 'foo' are gone :-)

Yes, because in this case the replacement stack has the correct data.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I see the problem now.
>
> However, isn't the root cause in replace--push-stack?
I am not sure; there is something else (see below).

> The relevant
> element of the replacement stack (whose structure, btw, seems not to
> be documented anywhere), is (4 4 *scratch*), whereas I'd expect to see
> (1 4 *scratch) instead, because the replacement was at position 1;
> then setting match-data from this would DTRT.
Yes, that is the logic.  The thing is, for some unknown reason to me,
the reported match-data is inexact when there are no printable chars
in the regexp (maybe it's expected and I am wrong on my assumptions).

> IOW, I'm afraid the looking-back solution is ad-hoc, and might not
> work in general, because the real problem is elsewhere.  WDYT?
Look what happen in these examples:

(with-temp-buffer
  (insert "foo")
  (goto-char 1)
  (progn (re-search-forward "o$" nil t)
	 (save-match-data (replace-match "oZZZ"))
	 (list (point) (match-beginning 0) (match-end 0))))
=> (7 3 7)

;; Now, a regexp with no printable chars

(with-temp-buffer
  (insert "foo")
  (goto-char 1)
  (progn (re-search-forward "$" nil t)
	 (save-match-data (replace-match "ZZZ"))
	 (list (point) (match-beginning 0) (match-end 0))))
=> (7 7 7)
;; If this would be (7 4 7), then we could use `looking-at'; we are to
;; the right of the replacement, then we use `looking-back'.
  

;; But the match was at 4 not at 7
(with-temp-buffer
  (insert "foo")
  (goto-char 1)
  (progn (re-search-forward "$" nil t)
	 (list (point) (match-beginning 0) (match-end 0))))
=> (4 4 4)
;; We lost the information about the beginning of the match when
;; the regexps lack of printable characters.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 31492@debbugs.gnu.org
> Date: Sat, 19 May 2018 23:28:35 +0900
> 
> > The relevant
> > element of the replacement stack (whose structure, btw, seems not to
> > be documented anywhere), is (4 4 *scratch*), whereas I'd expect to see
> > (1 4 *scratch) instead, because the replacement was at position 1;
> > then setting match-data from this would DTRT.
> Yes, that is the logic.  The thing is, for some unknown reason to me,
> the reported match-data is inexact when there are no printable chars
> in the regexp (maybe it's expected and I am wrong on my assumptions).

The reason for that is that match-data is recorded as markers, and so
the positions move if text is inserted.  In your example, the position
of $ moved due to insertion, so the marker's position was updated as
part of the replacement.

> (with-temp-buffer
>   (insert "foo")
>   (goto-char 1)
>   (progn (re-search-forward "$" nil t)
> 	 (save-match-data (replace-match "ZZZ"))
> 	 (list (point) (match-beginning 0) (match-end 0))))
> => (7 7 7)
> ;; If this would be (7 4 7), then we could use `looking-at'; we are to
> ;; the right of the replacement, then we use `looking-back'.
>   
> 
> ;; But the match was at 4 not at 7
> (with-temp-buffer
>   (insert "foo")
>   (goto-char 1)
>   (progn (re-search-forward "$" nil t)
> 	 (list (point) (match-beginning 0) (match-end 0))))
> => (4 4 4)

Right, and so I submit that the problem is where the replacement stack
is updated: it should account for these subtleties and adjust the
stack positions accordingly, since it has the opportunity to look at
the match position before the matched text is replaced.  It is IMO
suboptimal to make these adjustments where the stack is used, because
you've lost the information about the actual match point, and you are
deducing it using heuristics, which I'm not sure is 100% reliable.

Thanks.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> > The relevant
>> > element of the replacement stack (whose structure, btw, seems not to
>> > be documented anywhere), is (4 4 *scratch*), whereas I'd expect to see
>> > (1 4 *scratch) instead, because the replacement was at position 1;
>> > then setting match-data from this would DTRT.
>> Yes, that is the logic.  The thing is, for some unknown reason to me,
>> the reported match-data is inexact when there are no printable chars
>> in the regexp (maybe it's expected and I am wrong on my assumptions).
>
> The reason for that is that match-data is recorded as markers, and so
> the positions move if text is inserted.  In your example, the position
> of $ moved due to insertion, so the marker's position was updated as
> part of the replacement.
Yeah, I have noticed that this afternoon.

> Right, and so I submit that the problem is where the replacement stack
> is updated: it should account for these subtleties and adjust the
> stack positions accordingly, since it has the opportunity to look at
> the match position before the matched text is replaced.
I have a different approach:
If the problem arise from using markers istead of integers, then how
about we just use integers?

Please take a look:
--8<-----------------------------cut here---------------start------------->8---
commit cf13cb2dff040571b289e2ba8dcc0008394ffe3d
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Sun May 20 20:43:30 2018 +0900

    Fix corner case in query-replace-regexp undo
    
    This commit fixes Bug#31492.
    * lisp/replace.el(replace-save-match-data): New macro.
    (replace-match-maybe-edit): Preserve match data.
    
    * test/lisp/replace-tests.el (query-replace-undo-bug31492): Add test.

diff --git a/lisp/replace.el b/lisp/replace.el
index 3503b656d9..7d49b977fd 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2184,6 +2184,16 @@ replace-match-data
 		  new)))
       (match-data integers reuse t)))
 
+(defmacro replace-save-match-data (&rest body)
+  "Like `save-match-data', but it uses integers instead of markers.
+The value returned is the value of the last form in BODY."
+  (declare (indent 0) (debug t))
+  (let ((match (gensym "match-data")))
+    `(let ((,match (match-data 'integers)))
+       (unwind-protect
+           ,@body
+         (set-match-data ,match)))))
+
 (defun replace-match-maybe-edit (newtext fixedcase literal noedit match-data
                                  &optional backward)
   "Make a replacement with `replace-match', editing `\\?'.
@@ -2213,7 +2223,7 @@ replace-match-maybe-edit
                                   nil match-data match-data))))
 	    noedit nil)))
   (set-match-data match-data)
-  (replace-match newtext fixedcase literal)
+  (replace-save-match-data (replace-match newtext fixedcase literal))
   ;; `replace-match' leaves point at the end of the replacement text,
   ;; so move point to the beginning when replacing backward.
   (when backward (goto-char (nth 0 match-data)))
diff --git a/test/lisp/replace-tests.el b/test/lisp/replace-tests.el
index 40a1a31cf7..40ee838e67 100644
--- a/test/lisp/replace-tests.el
+++ b/test/lisp/replace-tests.el
@@ -399,5 +399,25 @@ replace-tests--query-replace-undo
       ;; After undo text must be the same.
       (should (string= text (buffer-string))))))
 
+(ert-deftest query-replace-undo-bug31492 ()
+  "Test for https://debbugs.gnu.org/31492 ."
+  (let ((text "a\nb\nc\n")
+        (count 0)
+        (inhibit-message t))
+    (with-temp-buffer
+      (insert text)
+      (goto-char 1)
+      (cl-letf (((symbol-function 'read-event)
+                 (lambda (&rest args)
+                   (cl-incf count)
+                   (let ((val (pcase count
+                                ((or 1 2) ?\s) ; replace current and go next
+                                (3 ?U) ; undo-all
+                                (_ ?q)))) ; exit
+                     val))))
+        (perform-replace "^\\|\b\\|$" "foo" t t nil))
+      ;; After undo text must be the same.
+      (should (string= text (buffer-string))))))
+
 
 ;;; replace-tests.el ends here

--8<-----------------------------cut here---------------end--------------->8---

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 31492@debbugs.gnu.org
> Date: Sun, 20 May 2018 20:51:10 +0900
> 
> If the problem arise from using markers istead of integers, then how
> about we just use integers?

Does this work with '^' and 'C-e', if they change text before a match
that was already replaced and recorded?

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

On Sun, 20 May 2018, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Cc: 31492@debbugs.gnu.org
>> Date: Sun, 20 May 2018 20:51:10 +0900
>>
>> If the problem arise from using markers istead of integers, then how
>> about we just use integers?
>
> Does this work with '^' and 'C-e', if they change text before a match
> that was already replaced and recorded?
I don't understand.  Do you have a recipe?

BTW, I found another issue in this feature.
I will open another report.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 20 May 2018 21:06:21 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 31492@debbugs.gnu.org
> 
> >> If the problem arise from using markers istead of integers, then how
> >> about we just use integers?
> >
> > Does this work with '^' and 'C-e', if they change text before a match
> > that was already replaced and recorded?
> I don't understand.  Do you have a recipe?

Not a recipe, an idea: I'm bothered by the possibility of the saved
positions becoming outdated if something changes text before the saved
positions.  Markers would move with the text, but positions won't.

The question is: is such a situation possible?

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

On Sun, 20 May 2018, Eli Zaretskii wrote:

> Not a recipe, an idea: I'm bothered by the possibility of the saved
> positions becoming outdated if something changes text before the saved
> positions.  Markers would move with the text, but positions won't.
>
> The question is: is such a situation possible?
Everything is possible (except, possibly, that I find a new job).

You know what they say: In the risk is the pleasure. Otherwise we
can use the first patch; the one with the test:
(/= (match-beginning 0) (match-end 0))

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 20 May 2018 22:46:05 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 31492@debbugs.gnu.org
> 
> > The question is: is such a situation possible?
> Everything is possible (except, possibly, that I find a new job).

Actually, I know a few things that are even less possible than that.

> You know what they say: In the risk is the pleasure. Otherwise we
> can use the first patch; the one with the test:
> (/= (match-beginning 0) (match-end 0))

No, I like the 2nd one better.

Thanks.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:


>> we can use the first patch; the one with the test:
>> (/= (match-beginning 0) (match-end 0))
>
> No, I like the 2nd one better.
Following patch is equivalent to the 2nd and its 1-liner
(excluded clarification comments):
--8<-----------------------------cut here---------------start------------->8---
commit 565c30caa8d5b015fd34446bd8900c55b2f67544
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Mon May 21 10:46:50 2018 +0900

    Fix corner case in query-replace-regexp undo
    
    This commit fixes Bug#31492.
    * lisp/replace.el (replace-match-maybe-edit): Preserve match data.
    
    * test/lisp/replace-tests.el (query-replace-undo-bug31492): Add test.

diff --git a/lisp/replace.el b/lisp/replace.el
index 3503b656d9..a17dd19b0d 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2214,6 +2214,10 @@ replace-match-maybe-edit
 	    noedit nil)))
   (set-match-data match-data)
   (replace-match newtext fixedcase literal)
+  ;; `query-replace' undo feature needs the beginning of the match position,
+  ;; but `replace-match' may change it, for instance, with a regexp like "^".
+  ;; Ensure that this function preserves the match data (Bug#31492).
+  (set-match-data match-data)
   ;; `replace-match' leaves point at the end of the replacement text,
   ;; so move point to the beginning when replacing backward.
   (when backward (goto-char (nth 0 match-data)))
diff --git a/test/lisp/replace-tests.el b/test/lisp/replace-tests.el
index 40a1a31cf7..40ee838e67 100644
--- a/test/lisp/replace-tests.el
+++ b/test/lisp/replace-tests.el
@@ -399,5 +399,25 @@ replace-tests--query-replace-undo
       ;; After undo text must be the same.
       (should (string= text (buffer-string))))))
 
+(ert-deftest query-replace-undo-bug31492 ()
+  "Test for https://debbugs.gnu.org/31492 ."
+  (let ((text "a\nb\nc\n")
+        (count 0)
+        (inhibit-message t))
+    (with-temp-buffer
+      (insert text)
+      (goto-char 1)
+      (cl-letf (((symbol-function 'read-event)
+                 (lambda (&rest args)
+                   (cl-incf count)
+                   (let ((val (pcase count
+                                ((or 1 2) ?\s) ; replace current and go next
+                                (3 ?U) ; undo-all
+                                (_ ?q)))) ; exit
+                     val))))
+        (perform-replace "^\\|\b\\|$" "foo" t t nil))
+      ;; After undo text must be the same.
+      (should (string= text (buffer-string))))))
+
 
 ;;; replace-tests.el ends here

--8<-----------------------------cut here---------------end--------------->8---

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 31492@debbugs.gnu.org
> Date: Mon, 21 May 2018 10:51:43 +0900
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> 
> >> we can use the first patch; the one with the test:
> >> (/= (match-beginning 0) (match-end 0))
> >
> > No, I like the 2nd one better.
> Following patch is equivalent to the 2nd and its 1-liner
> (excluded clarification comments):

Thanks, LGTM.  If no comments emerge in a couple of days, please push.

bug#31492: 26.1; query-replace-regexp undo fails in regexps w/o printable chars

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Cc: 31492@debbugs.gnu.org
>> Date: Mon, 21 May 2018 10:51:43 +0900
>> Following patch is equivalent to the 2nd and its 1-liner
>> (excluded clarification comments):
>
> Thanks, LGTM.  If no comments emerge in a couple of days, please push.
Pushed into master branch as 'Fix corner case in query-replace-regexp
undo'
(bab73230d1be1fe394b7269c1365ef6fb1a5d9b3)