From: Stefan Monnier <monnier@iro.umontreal.ca>
Date: Sat, 18 Nov 2023 21:34:38 +0000 (-0500)
Subject: (update_search_regs): Install better fix for bug#67124
X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=47b497b4dac91e5ea56102018223bdeb5e21a93b;p=emacs.git

(update_search_regs): Install better fix for bug#67124

The recent fix for the bug in `replace-match-maybe-edit`
was basically a refinement of a previously installed workaround,
whereas the bug was really in `update_search_regs`.

* src/search.c (update_search_regs): Improve handling of `start` positions.
* lisp/replace.el (replace-match-maybe-edit): Remove workaround.
* test/src/search-tests.el (search-test--replace-match-update-data): New test.
---

diff --git a/lisp/replace.el b/lisp/replace.el
index 7fec54ecb27..ac677db2feb 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2642,13 +2642,6 @@ passed in.  If LITERAL is set, no checking is done, anyway."
 	    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 beginning of the match position
-  ;; (bug#31492).  But we need to avoid clobbering the end of the match with
-  ;; the original match-end position, since `replace-match' could have made
-  ;; that incorrect or even invalid (bug#67124).
-  (set-match-data (list (car match-data) (nth 1 (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/src/search.c b/src/search.c
index 692d8488049..2996d32fca1 100644
--- a/src/search.c
+++ b/src/search.c
@@ -3140,11 +3140,25 @@ update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
   ptrdiff_t change = newend - oldend;
   ptrdiff_t i;
 
+  /* When replacing subgroup 3 in a match for regexp '\(\)\(\(\)\)\(\)'
+     start[i] should ideally stay unchanged for all but i=4 and end[i]
+     should move for all but i=1.
+     We don't have enough info here to distinguish those different subgroups
+     (except for subgroup 0), so instead we lean towards leaving the start[i]s
+     unchanged and towards moving the end[i]s.  */
+
   for (i = 0; i < search_regs.num_regs; i++)
     {
-      if (search_regs.start[i] >= oldend)
+      if (search_regs.start[i] <= oldstart)
+        /* If the subgroup that 'replace-match' is modifying encloses the
+           subgroup 'i', then its 'start' position should stay unchanged.
+           That's always true for subgroup 0.
+           For other subgroups it depends on details we don't have, so
+           we optimistically assume that it also holds for them.  */
+        ;
+      else if (search_regs.start[i] >= oldend)
         search_regs.start[i] += change;
-      else if (search_regs.start[i] > oldstart)
+      else
         search_regs.start[i] = oldstart;
       if (search_regs.end[i] >= oldend)
         search_regs.end[i] += change;
diff --git a/test/src/search-tests.el b/test/src/search-tests.el
index 293a715f5dc..32dc8a72a86 100644
--- a/test/src/search-tests.el
+++ b/test/src/search-tests.el
@@ -39,4 +39,42 @@
 	  (replace-match "bcd"))
       (should (= (point) 10)))))
 
+(ert-deftest search-test--replace-match-update-data ()
+  (with-temp-buffer
+    (pcase-dolist (`(,pre ,post) '(("" "")
+                                   ("a" "")
+                                   ("" "b")
+                                   ("a" "b")))
+      (erase-buffer)
+      (insert "hello ")
+      (save-excursion (insert pre post " world"))
+      (should (looking-at
+               (concat "\\(\\)" pre "\\(\\)\\(\\(\\)\\)\\(\\)" post "\\(\\)")))
+      (let* ((beg0 (match-beginning 0))
+             (beg4 (+ beg0 (length pre)))
+             (end4 (+ beg4 (length "BOO")))
+             (end0 (+ end4 (length post))))
+        (replace-match "BOO" t t nil 4)
+        (should (equal (match-beginning 0) beg0))
+        (should (equal (match-beginning 1) beg0))
+        (should (equal (match-beginning 2) beg4))
+        (should (equal (match-beginning 3) beg4))
+        (should (equal (match-beginning 4) beg4))
+        (should (equal (match-end 6) end0))
+        (should (equal (match-end 5) end4))
+        (should (equal (match-end 4) end4))
+        (should (equal (match-end 3) end4))
+        (should (equal (match-end 0) end0))
+        ;; `update_search_regs' doesn't have enough information to get
+        ;; the ones below correctly in all cases.
+        (when (> (length post) 0)
+          (should (equal (match-beginning 6) end0)))
+        (when (> (length pre) 0)
+          (should (equal (match-end 1) beg0)))
+        ;; `update_search_regs' doesn't have enough information to get
+        ;; the ones below correctly at all.
+        ;;(should (equal (match-beginning 5) end4))
+        ;;(should (equal (match-end 2) beg4))
+        ))))
+
 ;;; search-tests.el ends here