[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> From: nljlistb...@gmail.com (N. Jackson)
> Cc: 23...@debbugs.gnu.org,  Eli Zaretskii ,  
> jwieg...@gmail.com,  rpl...@gmail.com,  monn...@iro.umontreal.ca,  
> alex.ben...@linaro.org
> Date: Fri, 22 Jul 2016 21:42:08 -0300
> 
> Both the v2 and the v3 patch work for me with all my Org captures under
> my full config, and with the simple recipe under `emacs -Q".
> 
> I'm not familiar with the details of the bug or the patches, but turning
> off checks doesn't sound right, so I've stayed with the v2 patch.
> 
> I am now running the v2 patch as my every day Emacs.

Thanks.

Noam, I think this means your v2 patch can be pushed to emacs-25,
let's say by end of today.

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 10:19 +0200 on Thursday 2016-07-21, Robert Pluim wrote:
>
> nljlistb...@gmail.com (N. Jackson) writes:
>
>> At 20:56 -0400 on Wednesday 2016-07-20, npost...@users.sourceforge.net wrote:
>>>
>>> From: Noam Postavsky 
>>> Subject: [PATCH v1] Adjust match data before calling after-change-funs
>>
>> It puzzles me that your patch doesn't work for the emacs -Q recipe but
>> does work for my normal configuration, so much so that I suspected that
>> I had made a mistake, but I have reset and reapplied the patch three
>> times and I continue to see the same results.
>
> You're not alone: this patch doesn't fix the issue for me either with
> emacs -Q or with my normal capture templates.

FWIW, taking into account that I needed to rebuild Org Mode using the
patched Emacs, I redid my tests of Noam's patch and got the same
puzzling results as before. That is the patch *does* fix the problem for
me with my normal caputure templates, but it does not fix the problem
with the simple recipe in emacs -Q.

N.

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 10:08 +0200 on Thursday 2016-07-21, Robert Pluim wrote:
>>
> nljlistb...@gmail.com (N. Jackson) writes:
>
>> At 21:09 +0300 on Monday 2016-07-18, Eli Zaretskii wrote:
>>
>>> diff --git a/lisp/subr.el b/lisp/subr.el
>>> index e9e19d3..1bb1cb3 100644
>>> --- a/lisp/subr.el
>>> +++ b/lisp/subr.el
>>> @@ -3466,7 +3466,7 @@ save-match-data
>>>;; if you need to recompile all the Lisp files using interpreted code.
>>>(declare (indent 0) (debug t))
>>>(list 'let
>>> -   '((save-match-data-internal (match-data)))
>>> +   '((save-match-data-internal (match-data 'integers)))
>>> (list 'unwind-protect
>>>   (cons 'progn body)
>>>   ;; It is safe to free (evaporate) markers immediately here,
>>
>> FWIW on my system applying this patch does not resolve the org-capture
>> issue. I'm testing with org-20160718 from GNU Elpa and latest Emacs 25
>> branch from the git (Repository revision: 
>> 4157159a37b43712440da91a45a6d5f71eb96e8a).
>
> save-match-data is a macro. Did you recompile org with the modified
> emacs?

Thanks Robert. I failed to take that into account.

After re-applying Eli's patch above and then rebuilding Org Mode from
GNU Elpa (now org-20160719), the org-capture match-data-clobbered
error/abort no longer occurs, neither with the simple recipe in emacs -Q
nor with my own capture templates with my full config loaded.

Sorry for the noise.

[For completeness, I should say that I then removed the patch and
rebuilt org-20160719 again, and confirmed that it does trigger the bug
in both emacs -Q and with my configuration.]

N.

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Robert Pluim]

nljlistb...@gmail.com (N. Jackson) writes:

> At 20:56 -0400 on Wednesday 2016-07-20, npost...@users.sourceforge.net wrote:
>>
>> From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
>> From: Noam Postavsky 
>> 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.
> FWIW on my system applying this patch only partially resolves the
> org-capture issue. I'm testing with org-20160718 from GNU Elpa and
> latest Emacs 25 branch from the git (Repository revision:
> 4157159a37b43712440da91a45a6d5f71eb96e8a).
>
> The patch successfully eliminates the match-data-clobbered error/abort
> during org-capture with all my capture templates when I have my entire
> config loaded, but with a minimal recipe from emacs -Q the org-capture
> match-data-clobbered error still occurs.
>
> The minimal recipe I'm testing with is similar to that posted by Robert
> Pluim on 2016-07-18, specifically
>
>   src/emacs -Q
>
>   M-: (custom-set-variables '(package-selected-packages (quote 
> (org-20160718 RET
>   M-x package-initialize RET
>
>   C-x C-f ; find file.
>   C-S-backspace   ; kill-whole-line.
>   ~/.notes RET; Open the file expected by default capture 
> template.
>   M-x org-mode RET; put the buffer into Org Mode.
>   M-x org-capture RET t ; Run the default "Task" capture template bound to 
> the t key.
>
> With your patch I still get the error: 
>
>   org-capture: Capture template ‘t’: Match data clobbered by buffer 
> modification hooks
>
> .
>
> It puzzles me that your patch doesn't work for the emacs -Q recipe but
> does work for my normal configuration, so much so that I suspected that
> I had made a mistake, but I have reset and reapplied the patch three
> times and I continue to see the same results.

You're not alone: this patch doesn't fix the issue for me either with
emacs -Q or with my normal capture templates.

Regards

Robert

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Robert Pluim]

nljlistb...@gmail.com (N. Jackson) writes:

> At 21:09 +0300 on Monday 2016-07-18, Eli Zaretskii wrote:
>
>> diff --git a/lisp/subr.el b/lisp/subr.el
>> index e9e19d3..1bb1cb3 100644
>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -3466,7 +3466,7 @@ save-match-data
>>;; if you need to recompile all the Lisp files using interpreted code.
>>(declare (indent 0) (debug t))
>>(list 'let
>> -'((save-match-data-internal (match-data)))
>> +'((save-match-data-internal (match-data 'integers)))
>>  (list 'unwind-protect
>>(cons 'progn body)
>>;; It is safe to free (evaporate) markers immediately here,
>
> FWIW on my system applying this patch does not resolve the org-capture
> issue. I'm testing with org-20160718 from GNU Elpa and latest Emacs 25
> branch from the git (Repository revision: 
> 4157159a37b43712440da91a45a6d5f71eb96e8a).
>
> With these versions of Org and Emacs and your patch applied, with a
> recipe similar to that posted by Robert Pluim on 2016-07-18,
> specifically
>
>   src/emacs -Q
>
>   M-: (custom-set-variables '(package-selected-packages (quote 
> (org-20160718 RET
>   M-x package-initialize RET
>
>   C-x C-f ; find file.
>   C-S-backspace   ; kill-whole-line.
>   ~/.notes RET; Open the file expected by default capture 
> template.
>   M-x org-mode RET; put the buffer into Org Mode.
>   M-x org-capture RET t ; Run the default "Task" capture template bound to 
> the t key.
>
> I get the error: 
>
>   org-capture: Capture template ‘t’: Match data clobbered by buffer 
> modification hooks

save-match-data is a macro. Did you recompile org with the modified
emacs?

That patch works for me when using that version of org uncompiled.

Regards

Robert

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 20:56 -0400 on Wednesday 2016-07-20, npost...@users.sourceforge.net wrote:
>
> From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky 
> 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 

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 21:09 +0300 on Monday 2016-07-18, Eli Zaretskii wrote:

> My suggestion to fix this is below.  I ask for opinions on (1) whether
> this looks like TRT, (2) whether it is safe enough for emacs-25, and
> (3) whether someone has better ideas.  If someone thinks I've
> misunderstood the issue, don't hesitate to explain why, because
> frankly it feels very strange to find bugs that seem to have existed
> since 1990.
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index e9e19d3..1bb1cb3 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -3466,7 +3466,7 @@ save-match-data
>;; if you need to recompile all the Lisp files using interpreted code.
>(declare (indent 0) (debug t))
>(list 'let
> - '((save-match-data-internal (match-data)))
> + '((save-match-data-internal (match-data 'integers)))
>   (list 'unwind-protect
> (cons 'progn body)
> ;; It is safe to free (evaporate) markers immediately here,

FWIW on my system applying this patch does not resolve the org-capture
issue. I'm testing with org-20160718 from GNU Elpa and latest Emacs 25
branch from the git (Repository revision: 
4157159a37b43712440da91a45a6d5f71eb96e8a).

With these versions of Org and Emacs and your patch applied, with a
recipe similar to that posted by Robert Pluim on 2016-07-18,
specifically

  src/emacs -Q

  M-: (custom-set-variables '(package-selected-packages (quote 
(org-20160718 RET
  M-x package-initialize RET

  C-x C-f   ; find file.
  C-S-backspace ; kill-whole-line.
  ~/.notes RET  ; Open the file expected by default capture template.
  M-x org-mode RET  ; put the buffer into Org Mode.
  M-x org-capture RET t ; Run the default "Task" capture template bound to the 
t key.

I get the error: 

  org-capture: Capture template ‘t’: Match data clobbered by buffer 
modification hooks

. I also get a similar error with your patch and my full configuration
loaded and using my own capture templates:

  org-capture: Capture abort: (error Match data clobbered by buffer 
modification hooks)

.

The results above are same as I get if I do not apply your patch.

[On the other hand, with the same version of Org as above and Emacs from the
25.0.95 tarball, I do not see these error.]

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> Date: Fri, 08 Jul 2016 20:03:35 +0300
> From: Eli Zaretskii 
> Cc: 23...@debbugs.gnu.org
> 
> > which already does save-match-data. If I globally disable the org
> > element cache by (setq org-element-use-cache nil) the issue
> > disappears, so now I'm confused as to what's going on.
> 
> Load the file where this function lives as a .el file, and when the
> watchpoint triggers, show the results of "xbacktrace".

Actually, we might be looking in the wrong direction: since some of
these functions do use save-match-data, the search registers might be
legitimately changed and restored several times.  So I think you need
to type "continue" and see what other parts of code change the match
data, all the way until the call to replace_range returns.  Once we've
seen all those changes, with backtraces for each of them, we will
probably see the culprit.

It is still advisable to load the involved files as *.el files, as the
results of "xbacktrace" will then be more detailed.

Thanks.

[O] bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> From: Robert Pluim 
> Cc: 23...@debbugs.gnu.org
> Date: Fri, 08 Jul 2016 17:40:42 +0200
> 
> org-element--cache-after-change is:
> 
> (defun org-element--cache-after-change (beg end pre)
>   "Update buffer modifications for current buffer.
> BEG and END are the beginning and end of the range of changed
> text, and the length in bytes of the pre-change text replaced by
> that range.  See `after-change-functions' for more information."
>   (when (org-element--cache-active-p)
> (org-with-wide-buffer
>  (goto-char beg)
>  (beginning-of-line)
>  (save-match-data
>(let ((top (point))
>(bottom (save-excursion (goto-char end) (line-end-position
>;; Determine if modified area needs to be extended, according
>;; to both previous and current state.  We make a special
>;; case for headline editing: if a headline is modified but
>;; not removed, do not extend.
>(when (case org-element--cache-change-warning
>((t) t)
>(headline
> (not (and (org-with-limited-levels (org-at-heading-p))
>   (= (line-end-position) bottom
>(otherwise
> (let ((case-fold-search t))
>   (re-search-forward
>org-element--cache-sensitive-re bottom t
>  ;; Effectively extend modified area.
>  (org-with-limited-levels
>   (setq top (progn (goto-char top)
>(when (outline-previous-heading) (forward-line))
>(point)))
>   (setq bottom (progn (goto-char bottom)
>   (if (outline-next-heading) (1- (point))
> (point))
>;; Store synchronization request.
>(let ((offset (- end beg pre)))
>  (org-element--cache-submit-request top (- bottom offset) offset)
> ;; Activate a timer to process the request during idle time.
> (org-element--cache-set-timer (current-buffer
> 
> which already does save-match-data. If I globally disable the org
> element cache by (setq org-element-use-cache nil) the issue
> disappears, so now I'm confused as to what's going on.

Load the file where this function lives as a .el file, and when the
watchpoint triggers, show the results of "xbacktrace".