[O] 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]

> Solution: adjust in between the before and after change functions.
> Patch below.  I think there shouldn't be performance problems, although
> it does add yet another flag to replace_range (by the way, I noticed
> that the MARKERS flags is never 0, so it's redundant).  I tested with
> the attached bug-23917-match-data-buffer-modhook.el.

Looks pretty good, indeed.  More specifically, looks like the better fix.
2 comments:
- I'd take advantage of this change to replace the 0/1 booleans with
  false/true [but that's just my preference of bikeshed color).
- maybe we can even adjust_match_data in every call to replace_range
  rather than just in the one from Freplace_match.
Thanks,


        Stefan


> From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <address@hidden>
> Date: Wed, 20 Jul 2016 20:15:14 -0400
> Subject: [PATCH v1] Adjust match data before calling after-change-funs

> * src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
> true.  Update all callers except Freplace_match to pass 0 for the new
> parameter.
> * src/search.c (update_search_regs): New function, extracted from
> Freplace_match.
> (Freplace_match): Remove match data adjustment code, pass 1 for
> ADJUST_MATCH_DATA to replace_range instead.
> ---
>  src/cmds.c    |  2 +-
>  src/editfns.c |  6 +++---
>  src/insdel.c  | 10 ++++++++--
>  src/lisp.h    |  4 +++-
>  src/search.c  | 50 +++++++++++++++++++++++++++++---------------------
>  5 files changed, 44 insertions(+), 28 deletions(-)

> diff --git a/src/cmds.c b/src/cmds.c
> index 1e44ddd..4003d8b 100644
> --- a/src/cmds.c
> +++ b/src/cmds.c
> @@ -447,7 +447,7 @@ internal_self_insert (int c, EMACS_INT n)
>         string = concat2 (string, tem);
>       }
 
> -      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1);
> +      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1, 0);
>        Fforward_char (make_number (n));
>      }
>    else if (n > 1)
> diff --git a/src/editfns.c b/src/editfns.c
> index 412745d..32c8bec 100644
> --- a/src/editfns.c
> +++ b/src/editfns.c
> @@ -3192,7 +3192,7 @@ DEFUN ("subst-char-in-region", Fsubst_char_in_region,
>             /* replace_range is less efficient, because it moves the gap,
>                but it handles combining correctly.  */
>             replace_range (pos, pos + 1, string,
> -                          0, 0, 1);
> +                          0, 0, 1, 0);
>             pos_byte_next = CHAR_TO_BYTE (pos);
>             if (pos_byte_next > pos_byte)
>               /* Before combining happened.  We should not increment
> @@ -3405,7 +3405,7 @@ DEFUN ("translate-region-internal", 
> Ftranslate_region_internal,
>                 /* This is less efficient, because it moves the gap,
>                    but it should handle multibyte characters correctly.  */
>                 string = make_multibyte_string ((char *) str, 1, str_len);
> -               replace_range (pos, pos + 1, string, 1, 0, 1);
> +               replace_range (pos, pos + 1, string, 1, 0, 1, 0);
>                 len = str_len;
>               }
>             else
> @@ -3446,7 +3446,7 @@ DEFUN ("translate-region-internal", 
> Ftranslate_region_internal,
>               {
>                 string = Fmake_string (make_number (1), val);
>               }
> -           replace_range (pos, pos + len, string, 1, 0, 1);
> +           replace_range (pos, pos + len, string, 1, 0, 1, 0);
>             pos_byte += SBYTES (string);
>             pos += SCHARS (string);
>             cnt += SCHARS (string);
> diff --git a/src/insdel.c b/src/insdel.c
> index 4ad1074..fc3f19f 100644
> --- a/src/insdel.c
> +++ b/src/insdel.c
> @@ -1268,7 +1268,9 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t 
> from_byte,
>  /* Replace the text from character positions FROM to TO with NEW,
>     If PREPARE, call prepare_to_modify_buffer.
>     If INHERIT, the newly inserted text should inherit text properties
> -   from the surrounding non-deleted text.  */
> +   from the surrounding non-deleted text.
> +   If ADJUST_MATCH_DATA, then adjust the match data before calling
> +   signal_after_change.  */
 
>  /* Note that this does not yet handle markers quite right.
>     Also it needs to record a single undo-entry that does a replacement
> @@ -1279,7 +1281,8 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t 
> from_byte,
 
>  void
>  replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
> -            bool prepare, bool inherit, bool markers)
> +               bool prepare, bool inherit, bool markers,
> +               bool adjust_match_data)
>  {
>    ptrdiff_t inschars = SCHARS (new);
>    ptrdiff_t insbytes = SBYTES (new);
> @@ -1426,6 +1429,9 @@ replace_range (ptrdiff_t from, ptrdiff_t to, 
> Lisp_Object new,
>    MODIFF++;
>    CHARS_MODIFF = MODIFF;
 
> +  if (adjust_match_data)
> +    update_search_regs (from, to, from + SCHARS (new));
> +
>    signal_after_change (from, nchars_del, GPT - from);
>    update_compositions (from, GPT, CHECK_BORDER);
>  }
> diff --git a/src/lisp.h b/src/lisp.h
> index 6a98adb..25f811e 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -3516,7 +3516,7 @@ extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, 
> ptrdiff_t,
>                                ptrdiff_t, ptrdiff_t);
>  extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
>                                      ptrdiff_t, ptrdiff_t);
> -extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, 
> bool);
> +extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, 
> bool, bool);
>  extern void replace_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t,
>                            const char *, ptrdiff_t, ptrdiff_t, bool);
>  extern void syms_of_insdel (void);
> @@ -3994,6 +3994,8 @@ extern Lisp_Object make_temp_name (Lisp_Object, bool);
>  /* Defined in search.c.  */
>  extern void shrink_regexp_cache (void);
>  extern void restore_search_regs (void);
> +extern void update_search_regs (ptrdiff_t oldstart,
> +                                ptrdiff_t oldend, ptrdiff_t newend);
>  extern void record_unwind_save_match_data (void);
>  struct re_registers;
>  extern struct re_pattern_buffer *compile_pattern (Lisp_Object,
> diff --git a/src/search.c b/src/search.c
> index 5c949ad..1b82c94 100644
> --- a/src/search.c
> +++ b/src/search.c
> @@ -2727,8 +2727,15 @@ DEFUN ("replace-match", Freplace_match, 
> Sreplace_match, 1, 5, 0,
 
>    /* Replace the old text with the new in the cleanest possible way.  */
>    replace_range (search_regs.start[sub], search_regs.end[sub],
> -              newtext, 1, 0, 1);
> +                 newtext, 1, 0, 1, 1);
>    newpoint = search_regs.start[sub] + SCHARS (newtext);
> +  /* Update saved data to match adjustment made by replace_range.  */
> +  {
> +    ptrdiff_t change = newpoint - sub_end;
> +    if (sub_start >= sub_end)
> +      sub_start += change;
> +    sub_end += change;
> +  }
 
>    if (case_action == all_caps)
>      Fupcase_region (make_number (search_regs.start[sub]),
> @@ -2742,26 +2749,6 @@ DEFUN ("replace-match", Freplace_match, 
> Sreplace_match, 1, 5, 0,
>        || search_regs.num_regs != num_regs)
>      error ("Match data clobbered by buffer modification hooks");
 
> -  /* Adjust search data for this change.  */
> -  {
> -    ptrdiff_t oldend = search_regs.end[sub];
> -    ptrdiff_t oldstart = search_regs.start[sub];
> -    ptrdiff_t change = newpoint - search_regs.end[sub];
> -    ptrdiff_t i;
> -
> -    for (i = 0; i < search_regs.num_regs; i++)
> -      {
> -     if (search_regs.start[i] >= oldend)
> -       search_regs.start[i] += change;
> -     else if (search_regs.start[i] > oldstart)
> -       search_regs.start[i] = oldstart;
> -     if (search_regs.end[i] >= oldend)
> -       search_regs.end[i] += change;
> -     else if (search_regs.end[i] > oldstart)
> -       search_regs.end[i] = oldstart;
> -      }
> -  }
> -
>    /* Put point back where it was in the text.  */
>    if (opoint <= 0)
>      TEMP_SET_PT (opoint + ZV);
> @@ -3102,6 +3089,27 @@ restore_search_regs (void)
>      }
>  }
 
> +/* Called from replace-match via replace_range.  */
> +void
> +update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
> +{
> +  /* Adjust search data for this change.  */
> +  ptrdiff_t change = newend - oldend;
> +  ptrdiff_t i;
> +
> +  for (i = 0; i < search_regs.num_regs; i++)
> +    {
> +      if (search_regs.start[i] >= oldend)
> +        search_regs.start[i] += change;
> +      else if (search_regs.start[i] > oldstart)
> +        search_regs.start[i] = oldstart;
> +      if (search_regs.end[i] >= oldend)
> +        search_regs.end[i] += change;
> +      else if (search_regs.end[i] > oldstart)
> +        search_regs.end[i] = oldstart;
> +    }
> +}
> +
>  static void
>  unwind_set_match_data (Lisp_Object list)
>  {
> -- 
> 2.8.0