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

From: Eli Zaretskii <[email-masked]>
Date: Sat, 18 Nov 2023 12:18:12 +0200

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

Please include at least some, if not all, of this extended explanation
in the comments there.

And I think you can install your changes on master.

Thanks.