bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier <monnier@iro.umontreal.ca>]

> The more general problem is when there's at least one more
> sub-expression, whose start and/or end are after the new EOB.
> Those sub-expression's data will be completely bogus after the
> adjustment,

If they were after the EOB, they were already bogus to start with.
So there's really not much harm moving them around.  And in any case,
that's what has been happening for ever and has proved safe enough.

>> So I think a safe fix is to try and relax the check we added to
>> replace-match so it doesn't get all worked up when something ≥ EOB gets
>> changed to something else that's also ≥ EOB.
> And lose the other sub-expressions in a more general case?  Really?

I'm not sure what you mean by "losing sub-expressions".  But yes,
I think the behavior of save-match-data you describe is not
a real problem.  Arguably if save-match-data moves positions from
outside BEGV...ZV to inside it it's a problem.  But if it moves them
from outside BEG...Z to inside it, I think it's perfectly fine.

>> Or maybe instead of signaling an error, we could simply skip the "Adjust
>> search data for this change".
> That would still sweep the problem under the carpet, leaving the match
> data bogus, so I don't like doing that.

Maybe I'm not 100% satisfied with the behavior either, but I don't think
it's a significant problem and I don't think it'd cause the crash we saw
in bug#23869.

> The crash in bug#23869 was due to this:
>
>   newpoint = search_regs.start[sub] + SCHARS (newtext);
>   [...]
>   /* Now move point "officially" to the start of the inserted replacement.  */
>   move_if_not_intangible (newpoint);  <<<<<<<<<<<<<<<<<<<<<<<
>
> because due to clobbering, newpoint became -1.

Ah, I see.

Then maybe another fix is to compute newpoint before we call
replace_range, so it uses search_regs.start[sub] before the
*-change-functions can mess it up.  IOW:

    @@ -2726,9 +2726,9 @@ since only regular expressions have distinguished subexpressions.  */)
       unsigned  num_regs = search_regs.num_regs;
     
       /* Replace the old text with the new in the cleanest possible way.  */
    +  newpoint = search_regs.start[sub] + SCHARS (newtext);
       replace_range (search_regs.start[sub], search_regs.end[sub],
     		 newtext, 1, 0, 1);
    -  newpoint = search_regs.start[sub] + SCHARS (newtext);
     
       if (case_action == all_caps)
         Fupcase_region (make_number (search_regs.start[sub]),

Would that be sufficient to avoid the crash?  Why not?


        Stefan