bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer)

[Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

> Stefan, any ideas for a better fix, or other comments?  AFAIU, the
> root cause of the bug was that we are using integers instead of
> markers there, and that is because bug#31492 needs to preserve the
> original match-start position when the search string is a regexp that
> matches an empty string (in which case the match-start marker moves
> together with the match-end marker instead of staying put).

> diff --git a/lisp/replace.el b/lisp/replace.el
> index eeac734..6a3f223 100644
> --- a/lisp/replace.el
> +++ b/lisp/replace.el
> @@ -2642,8 +2642,11 @@ replace-match-maybe-edit
>    (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)
> +  ;; 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))))

(nth 1 (match-data)) == (match-end 0), no?

Hmm... so here we're throwing away all the subgroup info and keeping
only the start/end, right?  It's probably OK, indeed, but I think the
comment should clarify that (and should clarify that we (well,
presumably the undo feature) need the match end in addition to the
match beginning).

Also here it's not obvious which match-data is returned by (match-data).
IIUC it's the match data as adjusted by `replace-match`.

Which makes me wonder why we don't change `replace-match` so it's also
careful to preserve the match beginning just like it preserves the match
end.

>    ;; `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)))

and (nth 0 match-data) == (match-beginning 0), no?

So, I tried the patch below, which makes sense to my superficial
understanding of the problem, but it apparently doesn't fix the problem
in the OP's recipe, so I'm clearly missing something.

In any case the C part of the change seems right (if I may say so
myself :-), at least when `i==0` and more generally when `i` denotes
a subgroup which contains the subgroup that `replace-match` has
replaced.


        Stefan


diff --git a/lisp/replace.el b/lisp/replace.el
index 6b06e48c384..acf6b8a4652 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2642,13 +2642,9 @@ 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)))
+  (when backward (goto-char (match-beginning 0)))
   noedit)
 
 (defvar replace-update-post-hook nil
diff --git a/src/search.c b/src/search.c
index 692d8488049..0c5e81a245d 100644
--- a/src/search.c
+++ b/src/search.c
@@ -3142,9 +3142,11 @@ update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
 
   for (i = 0; i < search_regs.num_regs; i++)
     {
-      if (search_regs.start[i] >= oldend)
+      if (search_regs.start[i] <= oldstart)
+        ;
+      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;