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

From: Stefan Monnier <[email-masked]>
Date: Thu, 16 Nov 2023 13:23:48 -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?

Well, since it didn't first time I tried, no, I'm not sure.
But the more I look at it, the more I'm sure that it was just a pilot
error first time around.

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

IIUC this line is there because the code between the original regexp
match and the `replace-match` itself is large and thus likely to mess
the 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)))
>
> Why this unrelated change? is something wrong with the original code?

No, indeed, it's unrelated and not really better.

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

`replace-match` goes through the trouble of trying to keep the
match-data valid by moving the subgroup positions.

The way it does it currently is sometimes flat out incorrect and other
times debatable.  For end positions, we do:

      if (search_regs.end[i] >= oldend)
        search_regs.end[i] += change;
      else if (search_regs.end[i] > oldstart)
        search_regs.end[i] = oldstart;

which means that positions beyond (or at the end of) the change are
moved appropriately, positions before (or at the beginning of) the change
are left untouched and positions strictly within the change are moved to
the beginning of the change.
If the position is both at start and end at the same time, we move them,
assuming that they're more at the end than at the start.  For subgroup
0, this is definitely correct.  For other subgroups, it depends on the
inclusion or relative position of that subgroup with the subgroup that
`replace-match` is replacing.  E.g. if I matched
"\\(\\)\\(\\(\\)\\)\\(\\)" and I replace subgroup 3 with "hi", then the
ideal behavior would be to move the end of subgroups 0, 2, 3, and 4 but
not 1.

For start positions, we do the same:

      if (search_regs.start[i] >= oldend)
        search_regs.start[i] += change;
      else if (search_regs.start[i] > oldstart)
        search_regs.start[i] = oldstart;

When oldstart < oldend this works OK, but when they're equal, moving the
start position is definitely wrong for subgroup 0 and is often wrong for
others as well.  In the above regexp example, we should ideally move the
start position of subgroup 4 only and we should leave the other
ones unchanged.

`update_search_regs` doesn't have enough information to figure out which
subgroup is before/after/around/within some other subgroup and it isn't
even told which subgroup is being updated by `replace-match`, so the
best it can do is to either move them all or move none.  The current
code moves them all, which is always wrong for start[0].
My patch intends to make the other choice so it will get it wrong for
start[4] in the above example (just like the current code gets it wrong
for end[1]) but it will get it right for the other ones.

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

Oops, thanks for catching it.

        Stefan