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

[Stefan Monnier]

[ Sorry for my late reply, somehow I missed this reply of yours when
  you sent it :-( ]

>> (nth 1 (match-data)) == (match-end 0), no?
> No, because the former normally returns a marker, whereas the latter
> returns a number.  And here the difference is crucial.

It returns a different representation of the same number, agreed.
But we pass that to `set-match-data` which won't keep that difference,
will it?

>> 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).
> The comment before the patched part (you can see its end in the patch)
> says so, no?

It says that we need to adjust (match-beginning 0), while keeping
(match-end 0) but it doesn't say that it's OK that we throw away
the rest.
Probably not important, tho, you're right.

>> 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`.
> Yes, and that's the root cause here: replace-match updates match-data,
> but the original code then clobbered it by overwriting it with the
> match-data _before_ the replace-match call.

I think the comment should mention it.

>> 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.
> AFAIU, it's a general issue with markers:

But the match data adjusted by `replace-match` is not made of markers.
It's plain integers which are manually adjusted (by the code which
I changed in my patch).

> when you have both match-beginning and match-end at the same buffer
> position (because the matched text is an empty string, as when the
> search regexp is \b or similar, then replace-match moves them both to
> the end of the match, instead of leaving one of them at the beginning
> of the match.

I agree if they were markers that's what would happen.  But instead the
behavior we see is because the code of `update_search_regs` mimics the
behavior of "insert-before" markers both for start and end boundaries
of subgroups.  My patch tries to make it more careful.

>> >    ;; `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?
> See above: not exactly.

I believe the numerical value of (nth 0 match-data) will be the same at
this point as that of (nth 0 (match-data)) because we just passed that
very value to `set-match-data`, and that is always equal
(numerically) to (match-beginning 0).
Since the only thing we do with that value is pass it immediately to
`goto-char`, it makes no difference if it's a marker or an integer, no?

What am I missing?

>> 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.
> I don't understand the fix, so cannot help you here ;-)

:-(


        Stefan