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

[Eli Zaretskii <eliz@gnu.org>]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Gabriele Nicolardi <gabriele@medialab.sissa.it>,  67124@debbugs.gnu.org
> Date: Thu, 16 Nov 2023 10:35:26 -0500
> 
> > 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.
> 
> Hmm... not sure what happened, because I just tried it again and it does
> seem to fix the OP's problem.

Are you sure?

> So I suggest the patch below (adjusted to the new code on `master`).
> 
> 
>         Stefan
> 
> 
> diff --git a/lisp/replace.el b/lisp/replace.el
> index 7fec54ecb27..acf6b8a4652 100644
> --- a/lisp/replace.el
> +++ b/lisp/replace.el
> @@ -2642,16 +2642,9 @@ replace-match-maybe-edit
>  	    noedit nil)))
>    (set-match-data match-data)

Why do we still need this line?

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

Why this unrelated change? is something wrong with the original code?

> --- a/src/search.c
> +++ b/src/search.c
> @@ -3142,9 +3142,16 @@ 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)
> +        /* 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.

I've read this part of the comment many times, and I still don't
understand what you are trying to convey there, and thus don't
understand the new 'if' clause you added.  In particular, how come
this was never something brought to our attention? the comment seems
to imply that replace-match was somehow badly broken since forever.

And please don't use Markdown's markup in our code.

Still confused...