unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#24340: insert-file-contents calls before-change-functions too late
@ 2016-08-30 18:42 Stefan Monnier
  2016-08-30 19:10 ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2016-08-30 18:42 UTC (permalink / raw)
  To: 24340

Package: Emacs
Version: 25.1.50


Recipe:

    emacs -Q lisp/subr.el -f auto-revert-mode-hook
    M-: (defun sm-foo (&rest args) (message "b-c-f: %S size=%S" args (buffer-size))) RET
    M-: (add-hook 'before-change-functions #'sm-foo nil t) RET

then

    zile lisp/subr.el
    <modify the file somehow, then save it>

finally check *Messages*: you should see that the first message will say
something like "b-c-f: (...) NNN" where NNN is *smaller* than the
original buffer size.  IOW b-c-f is called after some part of the buffer
has already been removed.

Indeed, the bug is in Finsert_file_contents where we do:

      if (same_at_end != same_at_start)
	{
	  invalidate_buffer_caches (current_buffer,
				    BYTE_TO_CHAR (same_at_start),
				    same_at_end_charpos);
	  del_range_byte (same_at_start, same_at_end, 0);
      [...]
      /* This binding is to avoid ask-user-about-supersession-threat
	 being called in insert_from_buffer (via in
	 prepare_to_modify_buffer).  */
      specbind (intern ("buffer-file-name"), Qnil);
      insert_from_buffer (XBUFFER (conversion_buffer),
			  same_at_start_charpos, inserted_chars, 0);

The call to del_range_byte has arg prepare=0 so it doesn't run b-c-f.
Instead b-c-f is called from insert_from_buffer, which is too late since
the deletion already took place.

This is also the source of the known bug that b-c-f is not called at all
if insert-file-contents ends up only deleting text (since in that case
del_range_byte is called but insert_from_buffer isn't).

I include a suggested patch (which also would have the consequence that
we could get rid of the `prepare' argument of del_range_byte).


        Stefan


diff --git a/src/fileio.c b/src/fileio.c
index bf6bf62..55331d9 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -3839,6 +3839,7 @@ by calling `format-decode', which see.  */)
       if (! giveup_match_end)
 	{
 	  ptrdiff_t temp;
+          ptrdiff_t this_count = SPECPDL_INDEX ();
 
 	  /* We win!  We can handle REPLACE the optimized way.  */
 
@@ -3868,13 +3869,15 @@ by calling `format-decode', which see.  */)
 	  beg_offset += same_at_start - BEGV_BYTE;
 	  end_offset -= ZV_BYTE - same_at_end;
 
+          specbind (intern ("buffer-file-name"), Qnil);
 	  invalidate_buffer_caches (current_buffer,
 				    BYTE_TO_CHAR (same_at_start),
 				    same_at_end_charpos);
-	  del_range_byte (same_at_start, same_at_end, 0);
+	  del_range_byte (same_at_start, same_at_end, true);
 	  /* Insert from the file at the proper position.  */
 	  temp = BYTE_TO_CHAR (same_at_start);
 	  SET_PT_BOTH (temp, same_at_start);
+          unbind_to (this_count, Qnil);
 
 	  /* If display currently starts at beginning of line,
 	     keep it that way.  */
@@ -3979,10 +3982,11 @@ by calling `format-decode', which see.  */)
 	  /* Truncate the buffer to the size of the file.  */
 	  if (same_at_start != same_at_end)
 	    {
+              specbind (intern ("buffer-file-name"), Qnil);
 	      invalidate_buffer_caches (current_buffer,
 					BYTE_TO_CHAR (same_at_start),
 					BYTE_TO_CHAR (same_at_end));
-	      del_range_byte (same_at_start, same_at_end, 0);
+	      del_range_byte (same_at_start, same_at_end, true);
 	    }
 	  inserted = 0;
 
@@ -4030,12 +4034,23 @@ by calling `format-decode', which see.  */)
 	 we are taking from the decoded string.  */
       inserted -= (ZV_BYTE - same_at_end) + (same_at_start - BEGV_BYTE);
 
+      /* This binding is to avoid ask-user-about-supersession-threat
+	 being called in insert_from_buffer or del_range_bytes (via in
+	 prepare_to_modify_buffer).
+         AFAICT this is mostly needed because we change the buffer
+         before we set current_buffer->modtime, but if the file is modified
+         while we read from it, even if we set current_buffer->modtime first we
+         could still end up calling ask-user-about-supersession-threat
+         which wouldn't make much sense.  */
+      specbind (intern ("buffer-file-name"), Qnil);
       if (same_at_end != same_at_start)
 	{
+          /* FIXME: Shouldn't invalidate_buffer_caches be called by
+             del_range_byte or even directly by prepare_to_modify_buffer?  */
 	  invalidate_buffer_caches (current_buffer,
 				    BYTE_TO_CHAR (same_at_start),
 				    same_at_end_charpos);
-	  del_range_byte (same_at_start, same_at_end, 0);
+	  del_range_byte (same_at_start, same_at_end, true);
 	  temp = GPT;
 	  eassert (same_at_start == GPT_BYTE);
 	  same_at_start = GPT_BYTE;
@@ -4056,10 +4071,6 @@ by calling `format-decode', which see.  */)
 				   same_at_start + inserted - BEGV_BYTE
 				  + BUF_BEG_BYTE (XBUFFER (conversion_buffer)))
 	   - same_at_start_charpos);
-      /* This binding is to avoid ask-user-about-supersession-threat
-	 being called in insert_from_buffer (via in
-	 prepare_to_modify_buffer).  */
-      specbind (intern ("buffer-file-name"), Qnil);
       insert_from_buffer (XBUFFER (conversion_buffer),
 			  same_at_start_charpos, inserted_chars, 0);
       /* Set `inserted' to the number of inserted characters.  */





^ permalink raw reply related	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-30 18:42 bug#24340: insert-file-contents calls before-change-functions too late Stefan Monnier
@ 2016-08-30 19:10 ` Eli Zaretskii
  2016-08-30 21:21   ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2016-08-30 19:10 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 24340

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Tue, 30 Aug 2016 14:42:19 -0400
> 
>       if (same_at_end != same_at_start)
> 	{
> 	  invalidate_buffer_caches (current_buffer,
> 				    BYTE_TO_CHAR (same_at_start),
> 				    same_at_end_charpos);
> 	  del_range_byte (same_at_start, same_at_end, 0);
>       [...]
>       /* This binding is to avoid ask-user-about-supersession-threat
> 	 being called in insert_from_buffer (via in
> 	 prepare_to_modify_buffer).  */
>       specbind (intern ("buffer-file-name"), Qnil);
>       insert_from_buffer (XBUFFER (conversion_buffer),
> 			  same_at_start_charpos, inserted_chars, 0);
> 
> The call to del_range_byte has arg prepare=0 so it doesn't run b-c-f.
> Instead b-c-f is called from insert_from_buffer, which is too late since
> the deletion already took place.
> 
> This is also the source of the known bug that b-c-f is not called at all
> if insert-file-contents ends up only deleting text (since in that case
> del_range_byte is called but insert_from_buffer isn't).
> 
> I include a suggested patch (which also would have the consequence that
> we could get rid of the `prepare' argument of del_range_byte).

Thanks for the patch, but I don't want to make such pervasive changes
for this problem.  I thought we agreed that a single call to
before-change hooks for the entire buffer would be an okay solution
for this scenario.  What you suggest is exactly the slippery path
which I was afraid of: a lot of tricky/kludgey changes whose effect we
don't really understand, because we have no tests for the affected
functionality.

One comment to your FIXME comment:

> +          /* FIXME: Shouldn't invalidate_buffer_caches be called by
> +             del_range_byte or even directly by prepare_to_modify_buffer?  */

prepare_to_modify_buffer already calls invalidate_buffer_caches.  The
direct call here is because del_range_byte is called with its last
argument zero.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-30 19:10 ` Eli Zaretskii
@ 2016-08-30 21:21   ` Stefan Monnier
  2016-08-31 14:22     ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2016-08-30 21:21 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 24340

> Thanks for the patch, but I don't want to make such pervasive changes
> for this problem.

It was for another problem (the fact that b-c-f was not called at all in
a corner case).

This is a problem (it breaks the promise that b-c-f covers all
following changes) that shows up everytime insert-file-contents is
called with a non-nil `replace' argument, which is a completely
different situation.  It's a common case.

> I thought we agreed that a single call to before-change hooks for the
> entire buffer would be an okay solution for this scenario.

Kind of, but I think it'd be a kludge compared to the patch I submitted.
This said, if you insist on fixing it some other way, that's fine by me.

It's just that this problem is a lot more common than the one we knew
about, so I think it makes it more important to fix it sooner rather
than later.

> What you suggest is exactly the slippery path which I was afraid of:
> a lot of tricky/kludgey changes whose effect we don't really
> understand, because we have no tests for the affected functionality.

Its effect is to call prepare_to_modify_buffer one more time in
a function which already calls it moments later.  Seems safer than about
90% of the changes I installed in the past.

>> +          /* FIXME: Shouldn't invalidate_buffer_caches be called by
>> +             del_range_byte or even directly by prepare_to_modify_buffer?  */
> prepare_to_modify_buffer already calls invalidate_buffer_caches.
> The direct call here is because del_range_byte is called with its last
> argument zero.

OMG, indeed, so my patch makes even more sense and is even cleaner.
See new patch below, which actually simplifies the code.

I agree that the let-binding is a bit ugly, but that's the kludge we use
currently, so my patch doesn't make it much uglier in this respect.
A cleaner way to handle this issue might be to introduce a new
buffer-local variable (like inhibit-file-supersession-check) which we'd
let-bind to t around the whole function (either inside
insert-file-contents when `replace' is non-nil, or around
revert-buffer's call to insert-file-contents).  But for now, I can live
with the current kludge.


        Stefan


diff --git a/src/fileio.c b/src/fileio.c
index bf6bf62..a63deee 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -3839,6 +3839,7 @@ by calling `format-decode', which see.  */)
       if (! giveup_match_end)
 	{
 	  ptrdiff_t temp;
+          ptrdiff_t this_count = SPECPDL_INDEX ();
 
 	  /* We win!  We can handle REPLACE the optimized way.  */
 
@@ -3868,13 +3869,19 @@ by calling `format-decode', which see.  */)
 	  beg_offset += same_at_start - BEGV_BYTE;
 	  end_offset -= ZV_BYTE - same_at_end;
 
-	  invalidate_buffer_caches (current_buffer,
-				    BYTE_TO_CHAR (same_at_start),
-				    same_at_end_charpos);
-	  del_range_byte (same_at_start, same_at_end, 0);
+          /* This binding is to avoid ask-user-about-supersession-threat
+	     being called in insert_from_buffer or del_range_bytes (via
+	     prepare_to_modify_buffer).
+             AFAICT we could avoid ask-user-about-supersession-threat by setting
+             current_buffer->modtime earlier, but we could still end up calling
+             ask-user-about-supersession-threat if the file is modified while
+             we read it, so we bind buffer-file-name instead.  */
+          specbind (intern ("buffer-file-name"), Qnil);
+	  del_range_byte (same_at_start, same_at_end);
 	  /* Insert from the file at the proper position.  */
 	  temp = BYTE_TO_CHAR (same_at_start);
 	  SET_PT_BOTH (temp, same_at_start);
+          unbind_to (this_count, Qnil);
 
 	  /* If display currently starts at beginning of line,
 	     keep it that way.  */
@@ -3979,10 +3986,9 @@ by calling `format-decode', which see.  */)
 	  /* Truncate the buffer to the size of the file.  */
 	  if (same_at_start != same_at_end)
 	    {
-	      invalidate_buffer_caches (current_buffer,
-					BYTE_TO_CHAR (same_at_start),
-					BYTE_TO_CHAR (same_at_end));
-	      del_range_byte (same_at_start, same_at_end, 0);
+              /* See previous specbind for the reason behind this.  */
+              specbind (intern ("buffer-file-name"), Qnil);
+	      del_range_byte (same_at_start, same_at_end);
 	    }
 	  inserted = 0;
 
@@ -4030,12 +4036,11 @@ by calling `format-decode', which see.  */)
 	 we are taking from the decoded string.  */
       inserted -= (ZV_BYTE - same_at_end) + (same_at_start - BEGV_BYTE);
 
+      /* See previous specbind for the reason behind this.  */
+      specbind (intern ("buffer-file-name"), Qnil);
       if (same_at_end != same_at_start)
 	{
-	  invalidate_buffer_caches (current_buffer,
-				    BYTE_TO_CHAR (same_at_start),
-				    same_at_end_charpos);
-	  del_range_byte (same_at_start, same_at_end, 0);
+	  del_range_byte (same_at_start, same_at_end);
 	  temp = GPT;
 	  eassert (same_at_start == GPT_BYTE);
 	  same_at_start = GPT_BYTE;
@@ -4056,10 +4061,6 @@ by calling `format-decode', which see.  */)
 				   same_at_start + inserted - BEGV_BYTE
 				  + BUF_BEG_BYTE (XBUFFER (conversion_buffer)))
 	   - same_at_start_charpos);
-      /* This binding is to avoid ask-user-about-supersession-threat
-	 being called in insert_from_buffer (via in
-	 prepare_to_modify_buffer).  */
-      specbind (intern ("buffer-file-name"), Qnil);
       insert_from_buffer (XBUFFER (conversion_buffer),
 			  same_at_start_charpos, inserted_chars, 0);
       /* Set `inserted' to the number of inserted characters.  */
diff --git a/src/fns.c b/src/fns.c
index 31f0fd2..d3f9a6b 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3192,7 +3192,7 @@ into shorter lines.  */)
   SET_PT_BOTH (XFASTINT (beg), ibeg);
   insert (encoded, encoded_length);
   SAFE_FREE ();
-  del_range_byte (ibeg + encoded_length, iend + encoded_length, 1);
+  del_range_byte (ibeg + encoded_length, iend + encoded_length);
 
   /* If point was outside of the region, restore it exactly; else just
      move to the beginning of the region.  */
diff --git a/src/insdel.c b/src/insdel.c
index 5d3884b..ed914ec 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -1690,7 +1690,7 @@ del_range_1 (ptrdiff_t from, ptrdiff_t to, bool prepare, bool ret_string)
 /* Like del_range_1 but args are byte positions, not char positions.  */
 
 void
-del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte, bool prepare)
+del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte)
 {
   ptrdiff_t from, to;
 
@@ -1706,23 +1706,22 @@ del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte, bool prepare)
   from = BYTE_TO_CHAR (from_byte);
   to = BYTE_TO_CHAR (to_byte);
 
-  if (prepare)
-    {
-      ptrdiff_t old_from = from, old_to = Z - to;
-      ptrdiff_t range_length = to - from;
-      prepare_to_modify_buffer (from, to, &from);
-      to = from + range_length;
-
-      if (old_from != from)
-	from_byte = CHAR_TO_BYTE (from);
-      if (to > ZV)
-	{
-	  to = ZV;
-	  to_byte = ZV_BYTE;
-	}
-      else if (old_to == Z - to)
-	to_byte = CHAR_TO_BYTE (to);
-    }
+  {
+    ptrdiff_t old_from = from, old_to = Z - to;
+    ptrdiff_t range_length = to - from;
+    prepare_to_modify_buffer (from, to, &from);
+    to = from + range_length;
+
+    if (old_from != from)
+      from_byte = CHAR_TO_BYTE (from);
+    if (to > ZV)
+      {
+	to = ZV;
+	to_byte = ZV_BYTE;
+      }
+    else if (old_to == Z - to)
+      to_byte = CHAR_TO_BYTE (to);
+  }
 
   del_range_2 (from, from_byte, to, to_byte, 0);
   signal_after_change (from, to - from, 0);
diff --git a/src/lisp.h b/src/lisp.h
index 97c8d9f..b757e7b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3514,7 +3514,7 @@ extern void insert_from_string_before_markers (Lisp_Object, ptrdiff_t,
 					       ptrdiff_t, bool);
 extern void del_range (ptrdiff_t, ptrdiff_t);
 extern Lisp_Object del_range_1 (ptrdiff_t, ptrdiff_t, bool, bool);
-extern void del_range_byte (ptrdiff_t, ptrdiff_t, bool);
+extern void del_range_byte (ptrdiff_t, ptrdiff_t);
 extern void del_range_both (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t, bool);
 extern Lisp_Object del_range_2 (ptrdiff_t, ptrdiff_t,
 				ptrdiff_t, ptrdiff_t, bool);





^ permalink raw reply related	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-30 21:21   ` Stefan Monnier
@ 2016-08-31 14:22     ` Eli Zaretskii
  2016-08-31 14:48       ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2016-08-31 14:22 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 24340

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 24340@debbugs.gnu.org
> Date: Tue, 30 Aug 2016 17:21:38 -0400
> 
> > Thanks for the patch, but I don't want to make such pervasive changes
> > for this problem.
> 
> It was for another problem (the fact that b-c-f was not called at all in
> a corner case).
> 
> This is a problem (it breaks the promise that b-c-f covers all
> following changes) that shows up everytime insert-file-contents is
> called with a non-nil `replace' argument, which is a completely
> different situation.  It's a common case.

Then you misunderstood me.  My suggestion was meant for any use of
insert-file-contents with REPLACE non-nil.

> > I thought we agreed that a single call to before-change hooks for the
> > entire buffer would be an okay solution for this scenario.
> 
> Kind of, but I think it'd be a kludge compared to the patch I submitted.
> This said, if you insist on fixing it some other way, that's fine by me.

I'm sorry, but I must insist.  The change I proposed is the only one
in insert-file-contents for that use case that I'm prepared to
consider in the current situation.  (We could also leave this bug
open, until such time as a more thorough refactoring is done of the
related functionalities.)  Deeper changes are exactly the can of worms
that I don't want to open to fix just this one use case, especially
since Emacs 25.2 will almost certainly be branched from what now is
the master branch.  Such deeper changes were what Alan proposed in the
first place (with a similar patch), and I already said I didn't want
to do that.

> Its effect is to call prepare_to_modify_buffer one more time in
> a function which already calls it moments later.  Seems safer than about
> 90% of the changes I installed in the past.

I disagree about the safety.  The various things that
prepare_to_modify_buffer does is a hodge-podge of unrelated jobs that
were lumped there for "histerical raisins".  Just look at this list of
what it does:

 . bitches if the buffer is read-only
 . signals an undoable change
 . sets the buffer's redisplay flag
 . optionally preserves a buffer position by holding it in a pointer
 . locks the file
 . saves the text of selected region
 . calls before-change-functions and modification hooks stored in text
   properties
 . sets deactivate-mark non-nil

Doing this in the delete portions of insert-file-contents will send
ripples everywhere in Emacs, and I don't want to risk that without
being able to test that everything still works as before.  Each one of
these jobs should be carefully analyzed, decided whether we want to do
it for each chunk of text we change, and separate controls designed
and implemented for each of them we want to be able to control
independently of the others.  Anything else is just kludging patch
upon patch and hoping they will somehow DTRT.

> I agree that the let-binding is a bit ugly, but that's the kludge we use
> currently, so my patch doesn't make it much uglier in this respect.
> A cleaner way to handle this issue might be to introduce a new
> buffer-local variable (like inhibit-file-supersession-check) which we'd
> let-bind to t around the whole function

I think a cleaner way is to change the model of how we partition
piecemeal changes, when we signal the changes and when don't, when we
ask the user about supersession-threat, etc.  The current model is
fundamentally flawed, if we want to use the buffer-change hooks in the
ways that emerged from these discussions.  E.g., locking the file
should clearly be decoupled from signaling a change, because you
generally lock the file only once for a batch of changes, and the same
for the read-only checks.

Thanks.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-31 14:22     ` Eli Zaretskii
@ 2016-08-31 14:48       ` Stefan Monnier
  2016-08-31 15:03         ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2016-08-31 14:48 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 24340

> I'm sorry, but I must insist.  The change I proposed is the only one
> in insert-file-contents for that use case that I'm prepared to
> consider in the current situation.  (We could also leave this bug
> open, until such time as a more thorough refactoring is done of the
> related functionalities.)  Deeper changes are exactly the can of worms
> that I don't want to open to fix just this one use case, especially
> since Emacs 25.2 will almost certainly be branched from what now is
> the master branch.  Such deeper changes were what Alan proposed in the
> first place (with a similar patch), and I already said I didn't want
> to do that.

Right: I do not intend for my patch to go to 25.2.  But for Emacs-26,
it seems there's plenty of time to find and fix any possible fallout.

> I think a cleaner way is to change the model of how we partition
> piecemeal changes, when we signal the changes and when don't, when we
> ask the user about supersession-threat, etc.  The current model is
> fundamentally flawed, if we want to use the buffer-change hooks in the
> ways that emerged from these discussions.

All I care about is for any change to the buffer to be announced by
a prior b-c-f.


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-31 14:48       ` Stefan Monnier
@ 2016-08-31 15:03         ` Eli Zaretskii
  2016-08-31 17:50           ` Stefan Monnier
  2016-10-02  1:11           ` Stefan Monnier
  0 siblings, 2 replies; 11+ messages in thread
From: Eli Zaretskii @ 2016-08-31 15:03 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 24340

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 24340@debbugs.gnu.org
> Date: Wed, 31 Aug 2016 10:48:09 -0400
> 
> > I'm sorry, but I must insist.  The change I proposed is the only one
> > in insert-file-contents for that use case that I'm prepared to
> > consider in the current situation.  (We could also leave this bug
> > open, until such time as a more thorough refactoring is done of the
> > related functionalities.)  Deeper changes are exactly the can of worms
> > that I don't want to open to fix just this one use case, especially
> > since Emacs 25.2 will almost certainly be branched from what now is
> > the master branch.  Such deeper changes were what Alan proposed in the
> > first place (with a similar patch), and I already said I didn't want
> > to do that.
> 
> Right: I do not intend for my patch to go to 25.2.  But for Emacs-26,
> it seems there's plenty of time to find and fix any possible fallout.

Since I believe master will be used for 25.2, we cannot currently push
anything that must wait for 26.

> > I think a cleaner way is to change the model of how we partition
> > piecemeal changes, when we signal the changes and when don't, when we
> > ask the user about supersession-threat, etc.  The current model is
> > fundamentally flawed, if we want to use the buffer-change hooks in the
> > ways that emerged from these discussions.
> 
> All I care about is for any change to the buffer to be announced by
> a prior b-c-f.

My proposed simple change satisfies this requirement, albeit the
effect is less optimal.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-31 15:03         ` Eli Zaretskii
@ 2016-08-31 17:50           ` Stefan Monnier
  2016-10-02  1:11           ` Stefan Monnier
  1 sibling, 0 replies; 11+ messages in thread
From: Stefan Monnier @ 2016-08-31 17:50 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 24340

> Since I believe master will be used for 25.2,

[ Making an effort not to comment on that ;-)  ]

> we cannot currently push anything that must wait for 26.

I can wait for this issue to clear up.

>> > I think a cleaner way is to change the model of how we partition
>> > piecemeal changes, when we signal the changes and when don't, when we
>> > ask the user about supersession-threat, etc.  The current model is
>> > fundamentally flawed, if we want to use the buffer-change hooks in the
>> > ways that emerged from these discussions.
>> All I care about is for any change to the buffer to be announced by
>> a prior b-c-f.
> My proposed simple change satisfies this requirement, albeit the
> effect is less optimal.

Yes, as I said, I prefer my patch, but your suggestion is acceptable
as well.


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-08-31 15:03         ` Eli Zaretskii
  2016-08-31 17:50           ` Stefan Monnier
@ 2016-10-02  1:11           ` Stefan Monnier
  2016-10-02  7:19             ` Eli Zaretskii
  1 sibling, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2016-10-02  1:11 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 24340

> Since I believe master will be used for 25.2, we cannot currently push
> anything that must wait for 26.

Now that master has been labeled "for 26.1", can we push a patch to it?
I'd happily push my patch to it, but if you prefer a different approach,
feel free to push another patch there instead.


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-10-02  1:11           ` Stefan Monnier
@ 2016-10-02  7:19             ` Eli Zaretskii
  2016-10-03 13:48               ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2016-10-02  7:19 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 24340

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 24340@debbugs.gnu.org
> Date: Sat, 01 Oct 2016 21:11:34 -0400
> 
> > Since I believe master will be used for 25.2, we cannot currently push
> > anything that must wait for 26.
> 
> Now that master has been labeled "for 26.1", can we push a patch to it?
> I'd happily push my patch to it, but if you prefer a different approach,
> feel free to push another patch there instead.

I'm okay with your patch only if you promise to be there for fixing
any fallout.  It scares me quite a lot, especially the fact that it
fixes a single use case by making broad changes across the board.

Once again, my suggestion is instead to announce a change in the
entire buffer, as that is what revert-buffer is all about.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-10-02  7:19             ` Eli Zaretskii
@ 2016-10-03 13:48               ` Stefan Monnier
  2016-10-03 14:13                 ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2016-10-03 13:48 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 24340-done

> I'm okay with your patch only if you promise to be there for fixing
> any fallout.  It scares me quite a lot, especially the fact that it
> fixes a single use case by making broad changes across the board.

Great, installed,


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#24340: insert-file-contents calls before-change-functions too late
  2016-10-03 13:48               ` Stefan Monnier
@ 2016-10-03 14:13                 ` Eli Zaretskii
  0 siblings, 0 replies; 11+ messages in thread
From: Eli Zaretskii @ 2016-10-03 14:13 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 24340

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 24340-done@debbugs.gnu.org
> Date: Mon, 03 Oct 2016 09:48:14 -0400
> 
> > I'm okay with your patch only if you promise to be there for fixing
> > any fallout.  It scares me quite a lot, especially the fact that it
> > fixes a single use case by making broad changes across the board.
> 
> Great, installed,

Thank you very much for considering my opinions.





^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-10-03 14:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-30 18:42 bug#24340: insert-file-contents calls before-change-functions too late Stefan Monnier
2016-08-30 19:10 ` Eli Zaretskii
2016-08-30 21:21   ` Stefan Monnier
2016-08-31 14:22     ` Eli Zaretskii
2016-08-31 14:48       ` Stefan Monnier
2016-08-31 15:03         ` Eli Zaretskii
2016-08-31 17:50           ` Stefan Monnier
2016-10-02  1:11           ` Stefan Monnier
2016-10-02  7:19             ` Eli Zaretskii
2016-10-03 13:48               ` Stefan Monnier
2016-10-03 14:13                 ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).