[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)

2016-07-26 Thread npostavs
tags 23917 fixed
close 23917 25.1
quit

Eli Zaretskii  writes:

>> From: npost...@users.sourceforge.net
>> Cc: 23...@debbugs.gnu.org,  nljlistb...@gmail.com,  jwieg...@gmail.com,  
>> rpl...@gmail.com,  monn...@iro.umontreal.ca,  alex.ben...@linaro.org
>> Date: Thu, 21 Jul 2016 21:08:43 -0400
>> 
>> I made the same adjustments to the saved sub_start and sub_end
>> variables, but I had a mistake in that adjustment which caused the false
>> positives.  Fixed in the attached v2 patch.  We could just drop the
>> check, though I've already found it useful to catch bugs
>> (https://github.com/joaotavora/yasnippet/issues/720).
>> 
>> If I drop the checks (see attached v3 patch), then after following the
>> bug#23869 recipe, I get:
>> 
>> ## -*- Octave -*-
>> -module(bug).
>> -export([identity/1, is_even/1, size/1, reverse/1]).
>
> OK, let's wait for a few days to give time to the people who were
> affected by the issue to test the patch, and if no new issues come up,
> please push the version with the error code to emacs-25.

Since they've now reported that the new patch is working well, I've
pushed it as e1def617.







[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)

2016-07-26 Thread npostavs
Eli Zaretskii  writes:

>> From: npost...@users.sourceforge.net
>> Cc: 23...@debbugs.gnu.org,  nljlistb...@gmail.com,  jwieg...@gmail.com,  
>> rpl...@gmail.com,  monn...@iro.umontreal.ca,  alex.ben...@linaro.org
>> Date: Wed, 20 Jul 2016 23:00:59 -0400
>> 
>> > Please also make sure bug#23869 is still fixed after this.
>> 
>> Following the recipe in
>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23869#11 gives me 'Lisp
>> error: (error "Match data clobbered by buffer modification hooks")',
>> that indicates it's still fixed, right?
>
> Yes, but I thought we want to remove the error-out code.  Since we now
> protect ourselves from clobbered data, we don't need that extra
> protection, and I think leaving it in place will cause false positives
> (as a few people already reported).  That's because the adjustment of
> the search registers in the new function you introduce will itself
> trigger the error message, won't it?

I made the same adjustments to the saved sub_start and sub_end
variables, but I had a mistake in that adjustment which caused the false
positives.  Fixed in the attached v2 patch.  We could just drop the
check, though I've already found it useful to catch bugs
(https://github.com/joaotavora/yasnippet/issues/720).

If I drop the checks (see attached v3 patch), then after following the
bug#23869 recipe, I get:

## -*- Octave -*-
-module(bug).
-export([identity/1, is_even/1, size/1, reverse/1]).

>From 3e8f9b9f89108bcebf04e06e41a5ce2c719c6ad2 Mon Sep 17 00:00:00 2001
From: Noam Postavsky 
Date: Wed, 20 Jul 2016 20:15:14 -0400
Subject: [PATCH v2] Adjust match data before calling after-change-funs

* src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
true call update_search_regs.  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  | 52 ++--
 5 files changed, 45 insertions(+), 29 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 

[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)

2016-07-26 Thread npostavs
Eli Zaretskii  writes:

>> From: npost...@users.sourceforge.net
>> Cc: Eli Zaretskii ,  23...@debbugs.gnu.org,  
>> nljlistb...@gmail.com,  jwieg...@gmail.com,  rpl...@gmail.com,  
>> alex.ben...@linaro.org
>> Date: Wed, 20 Jul 2016 20:56:28 -0400
>> 
>> >> Maybe there's a misunderstanding.  What Noam suggested was just to
>> >> move the code which adjusts search_regs.start[i] and .end[i] to before
>> >> the call to replace_range.
>> >
>> > Oh, right, that's also an option.  It might suffer from another problem,
>> > which is that the match-data will be broken while the
>> > before-change-functions are run, so if there's a save-match-data there
>> > we're back to square one.
>> 
>> 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.
>
> Thanks.
>
> Please also make sure bug#23869 is still fixed after this.

Following the recipe in
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23869#11 gives me 'Lisp
error: (error "Match data clobbered by buffer modification hooks")',
that indicates it's still fixed, right?





[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)

2016-07-26 Thread npostavs
Eli Zaretskii  writes:

>> From: Stefan Monnier 
>> Cc: rpl...@gmail.com,  23...@debbugs.gnu.org,  alex.ben...@linaro.org,  
>> jwieg...@gmail.com,  nljlistb...@gmail.com
>> Date: Tue, 19 Jul 2016 00:48:19 -0400
>> 
>> > 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.
>
> I think we are mis-communicating.  I mean the following scenario:
>
> Before call to replace_range in replace-match:
>
>|---|---|--||
>s1 e1   s2e2   EOB
>
> (s1, e1, etc. are the start and end of the corresponding
> sub-expressions.)
>
> After the call to replace_range in replace-match:
>
>|-|---|--||
>s1   e1   s2e2   EOB
>
> IOW, the 1st sub-expression got replaced with a much shorter text,
> which made EOB be smaller than the original beginning and end of the
> 2nd sub-expression.  There's nothing bogus with this, is there?  The
> user will expect to get match-data adjusted as shown in the second
> diagram, and that's what she will really get -- unless there are
> buffer-modification hooks that use save-match-data.  In the latter
> case, what the user will get instead is this:
>
>|-|---|--||
>s1   EOB
> e1
>   s2
>   e2
>
> and that is even before the adjustment code kicks in and makes
> "adjustments" with an incorrect adjustment value, which is computed as
>
>   newpoint = search_regs.start[sub] + SCHARS (newtext);
>   [...]
>   ptrdiff_t change = newpoint - search_regs.end[sub];
>
> and so will use the new EOB as search_regs.end[sub], instead of the
> correct original value of e1 from the first diagram above.
>
> IOW, the call to save-match-data in a buffer-modification hook
> _disrupts_ the normal operation of replace-match in this case, by
> indirectly sabotaging the adjustment of match data after the
> replacement.

Is it not possible to adjust the match data *before* calling buffer
modification hooks?  Seems to me the root of the problem is that buffer
modification hooks get to see this invalid intermediate state where the
match data is out of sync with the buffer.





[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)

2016-07-26 Thread npostavs
Stefan Monnier  writes:

>> Maybe there's a misunderstanding.  What Noam suggested was just to
>> move the code which adjusts search_regs.start[i] and .end[i] to before
>> the call to replace_range.
>
> Oh, right, that's also an option.  It might suffer from another problem,
> which is that the match-data will be broken while the
> before-change-functions are run, so if there's a save-match-data there
> we're back to square one.

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.

>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,
    

[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)

2016-07-26 Thread Noam Postavsky
On Wed, Jul 20, 2016 at 9:47 PM, Stefan Monnier
 wrote:

> - maybe we can even adjust_match_data in every call to replace_range
>   rather than just in the one from Freplace_match.

That would be simpler, though I wasn't sure if it would be correct.





[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)

2016-07-26 Thread Nicolas Petton
Eli Zaretskii  writes:

> OK, let's wait for a few days to give time to the people who were
> affected by the issue to test the patch, and if no new issues come up,
> please push the version with the error code to emacs-25.

I'll wait until Monday for the first release candidate to give time to
this patch to be applied to emacs-25.

Nico


signature.asc
Description: PGP signature


[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)

2016-07-22 Thread Robert Pluim
Eli Zaretskii  writes:

>> From: npost...@users.sourceforge.net
>> Cc: 23...@debbugs.gnu.org,  nljlistb...@gmail.com,  jwieg...@gmail.com,  
>> rpl...@gmail.com,  monn...@iro.umontreal.ca,  alex.ben...@linaro.org
>> Date: Thu, 21 Jul 2016 21:08:43 -0400
>> 
>> I made the same adjustments to the saved sub_start and sub_end
>> variables, but I had a mistake in that adjustment which caused the false
>> positives.  Fixed in the attached v2 patch.  We could just drop the
>> check, though I've already found it useful to catch bugs
>> (https://github.com/joaotavora/yasnippet/issues/720).
>> 
>> If I drop the checks (see attached v3 patch), then after following the
>> bug#23869 recipe, I get:
>> 
>> ## -*- Octave -*-
>> -module(bug).
>> -export([identity/1, is_even/1, size/1, reverse/1]).
>
> OK, let's wait for a few days to give time to the people who were
> affected by the issue to test the patch, and if no new issues come up,
> please push the version with the error code to emacs-25.
>

Patch v2 fixes 'emacs -Q' and my normal capture templates, and I'm
using the patched emacs for this email. I'll keep running with it for
the next few days.

Regards

Robert





[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)

2016-07-22 Thread Eli Zaretskii
> From: npost...@users.sourceforge.net
> Cc: 23...@debbugs.gnu.org,  nljlistb...@gmail.com,  jwieg...@gmail.com,  
> rpl...@gmail.com,  monn...@iro.umontreal.ca,  alex.ben...@linaro.org
> Date: Thu, 21 Jul 2016 21:08:43 -0400
> 
> I made the same adjustments to the saved sub_start and sub_end
> variables, but I had a mistake in that adjustment which caused the false
> positives.  Fixed in the attached v2 patch.  We could just drop the
> check, though I've already found it useful to catch bugs
> (https://github.com/joaotavora/yasnippet/issues/720).
> 
> If I drop the checks (see attached v3 patch), then after following the
> bug#23869 recipe, I get:
> 
> ## -*- Octave -*-
> -module(bug).
> -export([identity/1, is_even/1, size/1, reverse/1]).

OK, let's wait for a few days to give time to the people who were
affected by the issue to test the patch, and if no new issues come up,
please push the version with the error code to emacs-25.

Thanks.





[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)

2016-07-21 Thread Eli Zaretskii
> From: npost...@users.sourceforge.net
> Cc: 23...@debbugs.gnu.org,  nljlistb...@gmail.com,  jwieg...@gmail.com,  
> rpl...@gmail.com,  monn...@iro.umontreal.ca,  alex.ben...@linaro.org
> Date: Wed, 20 Jul 2016 23:00:59 -0400
> 
> > Please also make sure bug#23869 is still fixed after this.
> 
> Following the recipe in
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23869#11 gives me 'Lisp
> error: (error "Match data clobbered by buffer modification hooks")',
> that indicates it's still fixed, right?

Yes, but I thought we want to remove the error-out code.  Since we now
protect ourselves from clobbered data, we don't need that extra
protection, and I think leaving it in place will cause false positives
(as a few people already reported).  That's because the adjustment of
the search registers in the new function you introduce will itself
trigger the error message, won't it?

Perhaps we should error out only if the number of registers changed,
because that can never be valid, I think.





[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)

2016-07-20 Thread Stefan Monnier
>> - maybe we can even adjust_match_data in every call to replace_range
>> rather than just in the one from Freplace_match.
> That would be simpler, though I wasn't sure if it would be correct.

I think it's definitely not an option for emacs-25.  But maybe we can
try it on master.


Stefan





[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)

2016-07-20 Thread Eli Zaretskii
> From: npost...@users.sourceforge.net
> Cc: Eli Zaretskii ,  23...@debbugs.gnu.org,  
> nljlistb...@gmail.com,  jwieg...@gmail.com,  rpl...@gmail.com,  
> alex.ben...@linaro.org
> Date: Wed, 20 Jul 2016 20:56:28 -0400
> 
> >> Maybe there's a misunderstanding.  What Noam suggested was just to
> >> move the code which adjusts search_regs.start[i] and .end[i] to before
> >> the call to replace_range.
> >
> > Oh, right, that's also an option.  It might suffer from another problem,
> > which is that the match-data will be broken while the
> > before-change-functions are run, so if there's a save-match-data there
> > we're back to square one.
> 
> 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.

Thanks.

Please also make sure bug#23869 is still fixed after this.





[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)

2016-07-20 Thread 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 
> 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);
>

[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)

2016-07-20 Thread Stefan Monnier
> Maybe there's a misunderstanding.  What Noam suggested was just to
> move the code which adjusts search_regs.start[i] and .end[i] to before
> the call to replace_range.

Oh, right, that's also an option.  It might suffer from another problem,
which is that the match-data will be broken while the
before-change-functions are run, so if there's a save-match-data there
we're back to square one.

Admittedly, before-change-functions is used less often, so it might be
good enough.


Stefan





[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)

2016-07-20 Thread Eli Zaretskii
> From: Stefan Monnier 
> Cc: rpl...@gmail.com,  23...@debbugs.gnu.org,  alex.ben...@linaro.org,  
> jwieg...@gmail.com,  nljlistb...@gmail.com,  npost...@users.sourceforge.net
> Date: Wed, 20 Jul 2016 14:19:59 -0400
> 
> > Is it OK to adjust the match data before actually making the
> > replacement?  If so, I think it's a simpler solution.
> >> PS: I can think of one (theoretical) other/better way to fix this
> >> problem: move the match-data adjustment so it's done within
> >> replace_range before running the after-change-functions.
> > Isn't that almost the same as what Noam suggested?
> 
> Yes, it's the same.  And yes, I like the idea, but I just don't know
> what it would look like as a patch.  I have the impression that it could
> prove either expensive in CPU time and backward incompatible
> (e.g. adjust markers for every buffer modification), or require
> extensive code surgery and/or breaking some abstractions.
> 
> This is just an impression, tho.  I think it'd definitely be the better
> solution, so it's worth investigating anyway, if only for "master" rather
> than for "emacs-25".

Maybe there's a misunderstanding.  What Noam suggested was just to
move the code which adjusts search_regs.start[i] and .end[i] to before
the call to replace_range.  The above values are not markers, and no
other markers are involved, so I'm not sure which markers did you
allude to.  Or why it would be CPU intensive.

What did I miss?





[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)

2016-07-20 Thread Stefan Monnier
> Is it OK to adjust the match data before actually making the
> replacement?  If so, I think it's a simpler solution.
>> PS: I can think of one (theoretical) other/better way to fix this
>> problem: move the match-data adjustment so it's done within
>> replace_range before running the after-change-functions.
> Isn't that almost the same as what Noam suggested?

Yes, it's the same.  And yes, I like the idea, but I just don't know
what it would look like as a patch.  I have the impression that it could
prove either expensive in CPU time and backward incompatible
(e.g. adjust markers for every buffer modification), or require
extensive code surgery and/or breaking some abstractions.

This is just an impression, tho.  I think it'd definitely be the better
solution, so it's worth investigating anyway, if only for "master" rather
than for "emacs-25".


Stefan





[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)

2016-07-20 Thread Eli Zaretskii
> From: Stefan Monnier 
> Cc: rpl...@gmail.com,  23...@debbugs.gnu.org,  alex.ben...@linaro.org,  
> jwieg...@gmail.com,  nljlistb...@gmail.com
> Date: Tue, 19 Jul 2016 21:50:07 -0400
> 
> > Do we care that using save-match-data in every call to replace-match
> > might mean a performance hit?
> 
> I do but:
> - to be honest, it's probably lost in the noise.
> - if we copy search_regs.start and search_regs.end with something like
>   alloca+memcpy (instead of calling Fmatch_data), the cost should be even more
>   lost in the noise.  Especially if you consider that the current code
>   already loops through the match-data to adjust it.
> - it's the best fix we've found so far.

What about Noam's suggestion:

> Is it not possible to adjust the match data *before* calling buffer
> modification hooks?  Seems to me the root of the problem is that buffer
> modification hooks get to see this invalid intermediate state where the
> match data is out of sync with the buffer.

Is it OK to adjust the match data before actually making the
replacement?  If so, I think it's a simpler solution.

> PS: I can think of one (theoretical) other/better way to fix this
> problem: move the match-data adjustment so it's done within
> replace_range before running the after-change-functions.

Isn't that almost the same as what Noam suggested?





[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)

2016-07-19 Thread Stefan Monnier
> Do we care that using save-match-data in every call to replace-match
> might mean a performance hit?

I do but:
- to be honest, it's probably lost in the noise.
- if we copy search_regs.start and search_regs.end with something like
  alloca+memcpy (instead of calling Fmatch_data), the cost should be even more
  lost in the noise.  Especially if you consider that the current code
  already loops through the match-data to adjust it.
- it's the best fix we've found so far.

> If it will, then this will again punish
> most of the users for the benefit of those few who (1) have
> buffer-modification hooks, and (2) those hooks call save-match-data.

I think the combination of 1 and 2 is actually pretty frequent.


Stefan


PS: I can think of one (theoretical) other/better way to fix this
problem: move the match-data adjustment so it's done within
replace_range before running the after-change-functions.  I think
this would be very satisfactory, since it would mean that the Elisp
code would always see the valid match-data (whereas currently the
after-change-functions get passed not-yet-adjusted match-data), so
save-match-data wouldn't mess it up.  But I fear this would require
much larger changes (and might involve a heavier performance cost as
well).





[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)

2016-07-19 Thread Eli Zaretskii
> From: Stefan Monnier 
> Cc: rpl...@gmail.com,  23...@debbugs.gnu.org,  alex.ben...@linaro.org,  
> jwieg...@gmail.com,  nljlistb...@gmail.com
> Date: Tue, 19 Jul 2016 12:03:51 -0400
> 
> I guess the next best thing is:
> - copy search_regs.start and search_regs.end before calling replace_range.
> - use that copy when adjusting the match data.
> Or equivalently, use save-match-data.  IOW go back to your original patch.
> Duh!

Do we care that using save-match-data in every call to replace-match
might mean a performance hit?  If it will, then this will again punish
most of the users for the benefit of those few who (1) have
buffer-modification hooks, and (2) those hooks call save-match-data.





[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)

2016-07-19 Thread Stefan Monnier
> Before call to replace_range in replace-match:
>
>|---|---|--||
>s1 e1   s2e2   EOB
>
> (s1, e1, etc. are the start and end of the corresponding
> sub-expressions.)
>
> After the call to replace_range in replace-match:
>
>|-|---|--||
>s1   e1   s2e2   EOB

Ah, right, now I see my confusion, thank you.

So, the data is within bounds before replace_range but after bounds
afterwards and the subsequent adjustments should fix it, but an
intervening save-match-data will mess it up.

Hmm... indeed, the adjustment isn't working correctly in this case.

I don't think we can safely change the way save-match-data works, so
I guess the next best thing is:
- copy search_regs.start and search_regs.end before calling replace_range.
- use that copy when adjusting the match data.
Or equivalently, use save-match-data.  IOW go back to your original patch.
Duh!


Stefan





[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)

2016-07-19 Thread Eli Zaretskii
> From: Stefan Monnier 
> Cc: rpl...@gmail.com,  23...@debbugs.gnu.org,  alex.ben...@linaro.org,  
> jwieg...@gmail.com,  nljlistb...@gmail.com
> Date: Tue, 19 Jul 2016 00:48:19 -0400
> 
> > 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.

I think we are mis-communicating.  I mean the following scenario:

Before call to replace_range in replace-match:

   |---|---|--||
   s1 e1   s2e2   EOB

(s1, e1, etc. are the start and end of the corresponding
sub-expressions.)

After the call to replace_range in replace-match:

   |-|---|--||
   s1   e1   s2e2   EOB

IOW, the 1st sub-expression got replaced with a much shorter text,
which made EOB be smaller than the original beginning and end of the
2nd sub-expression.  There's nothing bogus with this, is there?  The
user will expect to get match-data adjusted as shown in the second
diagram, and that's what she will really get -- unless there are
buffer-modification hooks that use save-match-data.  In the latter
case, what the user will get instead is this:

   |-|---|--||
   s1   EOB
e1
s2
e2

and that is even before the adjustment code kicks in and makes
"adjustments" with an incorrect adjustment value, which is computed as

  newpoint = search_regs.start[sub] + SCHARS (newtext);
  [...]
  ptrdiff_t change = newpoint - search_regs.end[sub];

and so will use the new EOB as search_regs.end[sub], instead of the
correct original value of e1 from the first diagram above.

IOW, the call to save-match-data in a buffer-modification hook
_disrupts_ the normal operation of replace-match in this case, by
indirectly sabotaging the adjustment of match data after the
replacement.

Am I missing something?

> And in any case, that's what has been happening for ever and has
> proved safe enough.

So you are saying that if a bug has been happening "for ever", it
doesn't have to be fixed?  (I disagree about "safe enough": the amount
of bug reports in our data base that are not reproducible and about
whose reasons we have no clear idea is non-negligible, so we don't
really know whether this particular issue caused trouble in the past
or not.)

> >> 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".

See above: the match data for any sub-expressions beyond the one that
shrunk too much is now bogus.  Thus "losing".

> >> 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.

We don't only solve bugs that cause crashes, do we?  When I debugged
the crash in bug#23869, I found and tried to solve the root cause of
it, not just the symptom that caused the assertion violation.  I still
think we should strive to solve the root cause.

As for the significance of the problem, I hope you will reconsider
this after reading the above description of the scenario I have in
mind.  (Or tell me where I am wrong.)

> > 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? 

[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)

2016-07-19 Thread Eli Zaretskii
> From: Stefan Monnier 
> Cc: Robert Pluim ,  23...@debbugs.gnu.org,  
> alex.ben...@linaro.org,  jwieg...@gmail.com,  nljlistb...@gmail.com
> Date: Mon, 18 Jul 2016 20:58:35 -0400
> 
> > I think this change performed by save-match-data is harmless: the old
> > value (62) was not valid any more anyway.
> 
> In this particular case, yes.  But only in this case, because (a)
> there's actually only one sub-expression, and (b) it ends exactly at
> EOB.

Moreover, if the value of 62 was left alone by save-match-data, the
adjustment code in replace-match would have fixed it.  That's what
that code is all about: it fixes the match data due to changes to the
buffer made by replacing the old text by the new.  Any attempts to
"fix" those values under that code's feet are not helpful; quite the
contrary.

> > 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?

What really bothers me is that by just loosening the conditions under
which we signal an error we defeat _valid_ code, which did use
save-match-data, and yet the match data still ends up being
clobbered.  I don't mind making the test more loose so that invalid
programs have a longer rope to hang themselves, but valid programs
should not be failed, IMO.

> > 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.

> > This said, I don't fully understand what's going on: bug#23869 reported
> > a crash, but AFAICT the match-data here is only used to adjust
> > search_regs which seems like it wouldn't cause a crash, even if the new
> > values are bogus.

> 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.


> > > -   '((save-match-data-internal (match-data)))
> > > +   '((save-match-data-internal (match-data 'integers)))
> > 
> > That looks risky.
> 
> Then how about manually doing the equivalent of save-match-data around
> the call to replace_range, calling match-data with non-nil argument?

Here are some more alternatives for dealing with this issue:

 (1) Make match-data use integers instead of markers by default when a
 call to replace-match is in progress, i.e. when match-data is
 called from some buffer-modification hook triggered by
 replace-match

 (2) Don't signal an error, even if match data seems clobbered, if the
 new value of point is valid and either (a) there's only one
 search register, or (b) the adjustment value is zero (i.e. the
 registers will be left unchanged).

I like (1) better than (2) because (1) will let valid programs avoid
clobbering match data, but maybe it's too risky for emacs-25.  Also,
is it plausible that some buffer-modification hook will edit the
buffer in the save-match-data forms, and expect the match data to be
adjusted, or is this something that a buffer-modification hook should
never do?  If valid programs can do this, then (1) is probably not a
good idea.





[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)

2016-07-18 Thread Stefan Monnier
> 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





[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)

2016-07-18 Thread Eli Zaretskii
> From: Stefan Monnier 
> Cc: Robert Pluim ,  23...@debbugs.gnu.org,  
> alex.ben...@linaro.org,  jwieg...@gmail.com,  nljlistb...@gmail.com
> Date: Mon, 18 Jul 2016 20:58:35 -0400
> 
> > In the case in point, a single character at EOB (= 62) was deleted,
> > which made EOB be 61, one less than its previous value.  When
> > save-match-data was called from within a hook set up by Org, it tried
> > to record the end of the sub-expression as 62, but set-marker silently
> > changed that to 61.  That "corrected" value was subsequently restored
> > when save-match-data was exited, whereas replace-match expected to see
> > the original value of 62, and therefore barfed.
> 
> I think this change performed by save-match-data is harmless: the old
> value (62) was not valid any more anyway.

In this particular case, yes.  But only in this case, because (a)
there's actually only one sub-expression, and (b) it ends exactly at
EOB.

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,
should the buffer-modification hooks use save-match-data.

> 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?

> 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.

> This said, I don't fully understand what's going on: bug#23869 reported
> a crash, but AFAICT the match-data here is only used to adjust
> search_regs which seems like it wouldn't cause a crash, even if the new
> values are bogus.

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.

> > -   '((save-match-data-internal (match-data)))
> > +   '((save-match-data-internal (match-data 'integers)))
> 
> That looks risky.

Then how about manually doing the equivalent of save-match-data around
the call to replace_range, calling match-data with non-nil argument?





[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)

2016-07-18 Thread Stefan Monnier
> In the case in point, a single character at EOB (= 62) was deleted,
> which made EOB be 61, one less than its previous value.  When
> save-match-data was called from within a hook set up by Org, it tried
> to record the end of the sub-expression as 62, but set-marker silently
> changed that to 61.  That "corrected" value was subsequently restored
> when save-match-data was exited, whereas replace-match expected to see
> the original value of 62, and therefore barfed.

I think this change performed by save-match-data is harmless: the old
value (62) was not valid any more anyway.

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.

Or maybe instead of signaling an error, we could simply skip the "Adjust
search data for this change".  I like the idea of signaling an error, as
a debugging help, but maybe for emacs-25 we should go for something less
intrusive after all?

This said, I don't fully understand what's going on: bug#23869 reported
a crash, but AFAICT the match-data here is only used to adjust
search_regs which seems like it wouldn't cause a crash, even if the new
values are bogus.  So maybe signaling an error is important because the
crash happens further down.

> - '((save-match-data-internal (match-data)))
> + '((save-match-data-internal (match-data 'integers)))

That looks risky.


Stefan





[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)

2016-07-18 Thread Eli Zaretskii
> From: John Wiegley 
> Cc: Robert Pluim ,  Stefan Monnier 
> ,  23...@debbugs.gnu.org,  alex.ben...@linaro.org,  
> nljlistb...@gmail.com
> Date: Mon, 18 Jul 2016 12:04:11 -0700
> 
> > Eli Zaretskii  writes:
> 
> > 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.
> 
> I didn't even know match-data took arguments, so I defer to your judgment on
> this issue, Eli.

Neither did I, but I've read the code through which I stepped ;-)





[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)

2016-07-18 Thread John Wiegley
> Eli Zaretskii  writes:

> 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.

I didn't even know match-data took arguments, so I defer to your judgment on
this issue, Eli.

-- 
John Wiegley  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com  60E1 46C4 BD1A 7AC1 4BA2


signature.asc
Description: PGP signature


[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)

2016-07-18 Thread Eli Zaretskii
> From: Robert Pluim 
> CC: 23...@debbugs.gnu.org, Alex Bennée
>  , jwieg...@gmail.com, nljlistb...@gmail.com
> Date: Mon, 18 Jul 2016 14:24:59 +0200
> 
> (I'm moving this discussion to the bug, let me know if that's not OK)

Thanks.

> Make sure that you have org-20160704 from elpa.
> 
> # emacs -Q
> 
> ;evaluate the following 
> (custom-set-variables
>  '(package-selected-packages
>(quote
> (org-20160704
> (package-initialize)
> 
> ; Now do:
> 
> C-x C-f ~/.notes
> M-x org-mode
> M-x org-capture
> t
> 
> ; This should result in:
> Capture template ‘t’: Match data clobbered by buffer modification
> hooks

Thanks again.

It turns out the validation added in 3a9d6296, to solve bug#23869,
exposed a very serious bug we seem to have always had with
save-match-data called from buffer-modification hooks.

The basic problem is that set-marker restricts the buffer position
passed as its argument to valid limits, between 1 and EOB.  So if you
call set-marker with a value beyond EOB, the marker's position will
silently set to EOB.

Now, when buffer-modification hooks run due to changes made by
replace-match, the replacement has already been done.  If that
replacement shrinks the buffer, then when save-match-data is called,
and calls match-data, the latter will see the new (smaller) value of
EOB.  By default, match-data records the data in markers it creates
for beginning and end of each matched sub-expression.  So the result
is that beginning and/or end of any sub-expression that was beyond the
new EOB will be recorded as EOB.  Then restoring this saved match data
will clobber the data of those sub-expressions.

In the case in point, a single character at EOB (= 62) was deleted,
which made EOB be 61, one less than its previous value.  When
save-match-data was called from within a hook set up by Org, it tried
to record the end of the sub-expression as 62, but set-marker silently
changed that to 61.  That "corrected" value was subsequently restored
when save-match-data was exited, whereas replace-match expected to see
the original value of 62, and therefore barfed.

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,





[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)

2016-07-18 Thread Eric S Fraga
On Monday, 18 Jul 2016 at 14:50, Kaushal Modi wrote:
> @Eric Would it be possible for you to provide a recipe to recreate
> this issue from an emacs -Q session?

With emacs-snapshot (aka 25.0.95.1), and the following emacs-minimal.el
file:

--8<---cut here---start->8---
(add-to-list 'load-path "~/git/org-mode/lisp")
(require 'org)
(setq org-capture-templates '(("t" "todo" entry (file+datetree 
"/tmp/tasks.org") "* TODO %^{Task} %^G\nSCHEDULED: %t\n%i%?")))
--8<---cut here---end--->8---

the following sequence:

--8<---cut here---start->8---
emacs -Q -l emacs-minimal.el
M-x org-capture RET t this is a test RET testing RET
--8<---cut here---end--->8---

gives

--8<---cut here---start->8---
Debugger entered--Lisp error: (error "Capture template ‘t’: Match data 
clobbered by buffer modification hooks")
  signal(error ("Capture template ‘t’: Match data clobbered by buffer 
modification hooks"))
  error("Capture template `%s': %s" "t" "Match data clobbered by buffer 
modification hooks")
  (condition-case error (org-capture-place-template (equal (car 
(org-capture-get :target)) (quote function))) ((error quit) (if (and 
(buffer-base-buffer (current-buffer)) (string-match "\\`CAPTURE-" 
(buffer-name))) (kill-buffer (current-buffer))) (set-window-configuration 
(org-capture-get :return-to-wconf)) (error "Capture template `%s': %s" 
(org-capture-get :key) (nth 1 error
  (if (equal goto 0) (org-capture-insert-template-here) (condition-case error 
(org-capture-place-template (equal (car (org-capture-get :target)) (quote 
function))) ((error quit) (if (and (buffer-base-buffer (current-buffer)) 
(string-match "\\`CAPTURE-" (buffer-name))) (kill-buffer (current-buffer))) 
(set-window-configuration (org-capture-get :return-to-wconf)) (error "Capture 
template `%s': %s" (org-capture-get :key) (nth 1 error (if (and 
(derived-mode-p (quote org-mode)) (org-capture-get :clock-in)) (condition-case 
nil (progn (if (org-clock-is-active) (org-capture-put :interrupted-clock 
(copy-marker org-clock-marker))) (org-clock-in) (set (make-local-variable 
(quote org-capture-clock-was-started)) t)) (error "Could not start the clock in 
this capture buffer"))) (if (org-capture-get :immediate-finish) 
(org-capture-finalize)))
  (cond ((equal entry "C") (customize-variable (quote org-capture-templates))) 
((equal entry "q") (user-error "Abort")) (t (org-capture-set-plist entry) 
(org-capture-get-template) (org-capture-put :original-buffer orig-buf 
:original-file (or (buffer-file-name orig-buf) (and (featurep (quote dired)) 
(car (rassq orig-buf dired-buffers :original-file-nondirectory (and 
(buffer-file-name orig-buf) (file-name-nondirectory (buffer-file-name 
orig-buf))) :annotation annotation :initial initial :return-to-wconf 
(current-window-configuration) :default-time (or org-overriding-default-time 
(org-current-time))) (org-capture-set-target-location) (condition-case error 
(org-capture-put :template (org-capture-fill-template)) ((error quit) (if 
(get-buffer "*Capture*") (kill-buffer "*Capture*")) (error "Capture abort: %s" 
error))) (setq org-capture-clock-keep (org-capture-get :clock-keep)) (if (equal 
goto 0) (org-capture-insert-template-here) (condition-case error 
(org-capture-place-template (equal (car (org-capture-get :target)) (quote 
function))) ((error quit) (if (and (buffer-base-buffer ...) (string-match 
"\\`CAPTURE-" ...)) (kill-buffer (current-buffer))) (set-window-configuration 
(org-capture-get :return-to-wconf)) (error "Capture template `%s': %s" 
(org-capture-get :key) (nth 1 error (if (and (derived-mode-p (quote 
org-mode)) (org-capture-get :clock-in)) (condition-case nil (progn (if 
(org-clock-is-active) (org-capture-put :interrupted-clock ...)) (org-clock-in) 
(set (make-local-variable ...) t)) (error "Could not start the clock in this 
capture buffer"))) (if (org-capture-get :immediate-finish) 
(org-capture-finalize)
  (let* ((orig-buf (current-buffer)) (annotation (if (and (boundp (quote 
org-capture-link-is-already-stored)) org-capture-link-is-already-stored) 
(plist-get org-store-link-plist :annotation) (condition-case nil (progn 
(org-store-link nil)) (error nil (entry (or org-capture-entry 
(org-capture-select-template keys))) initial) (setq initial (or 
org-capture-initial (and (org-region-active-p) (buffer-substring (point) 
(mark) (if (stringp initial) (progn (remove-text-properties 0 (length 
initial) (quote (read-only t)) initial))) (if (stringp annotation) (progn 
(remove-text-properties 0 (length annotation) (quote (read-only t)) 
annotation))) (cond ((equal entry "C") (customize-variable (quote 
org-capture-templates))) ((equal entry "q") (user-error "Abort")) (t 
(org-capture-set-plist entry) (org-capture-get-template) (org-capture-put 
:original-buffer orig-buf :original-file (or 

[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)

2016-07-18 Thread Kaushal Modi
I use org-capture heavily, but I do not use a template that causes this
issue.

This issue was posted once again today on the org mailing list and I would
vote for this to be made a blocking bug too.

One useful update we get from Eric Fraga (
http://thread.gmane.org/gmane.emacs.orgmode/108289 ) is that this bug is
seen only after 25.0.94 pretest.

@Eric Would it be possible for you to provide a recipe to recreate this
issue from an emacs -Q session?
-- 

Kaushal Modi


[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)

2016-07-18 Thread Robert Pluim
(I'm moving this discussion to the bug, let me know if that's not OK)

Eli Zaretskii  writes:

>> From: Alex Bennée 
>> Cc: Eli Zaretskii , "N. Jackson" , 
>> emacs-de...@gnu.org
>> Date: Sun, 17 Jul 2016 18:28:36 +0100
>> 
>> I've just uninstalled the ELPA installed org and run into the same
>> problem with the bundled version. It certainly doesn't happen with all
>> my capture templates but this particular entry with %a and %c does
>> trigger the error. From my *Messages:
>> 
>> Clipboard pasted as level 2 subtree
>> org-capture: Capture template ‘g’: Match data clobbered by buffer 
>> modification hooks
>> "/home/alex/src/emacs/install/share/emacs/25.0.95/lisp/org/org.elc"
>
> Can you come up with a reproducible recipe, starting with "emacs -Q",
> and then loading everything required for the reproduction?  Otherwise,
> I don't see how this problem could be resolved, given the sadly small
> number of people on board who are capable of debugging such problems.
>

Make sure that you have org-20160704 from elpa.

# emacs -Q

;evaluate the following 
(custom-set-variables
 '(package-selected-packages
   (quote
(org-20160704
(package-initialize)

; Now do:

C-x C-f ~/.notes
M-x org-mode
M-x org-capture
t

; This should result in:
Capture template ‘t’: Match data clobbered by buffer modification
hooks

I've tried to follow who clobbers it via GDB, but I keep getting
lost. For me it's always search_regs.end[sub] that has the unexpected
value.

Regards

Robert