bug#24340: insert-file-contents calls before-change-functions too late

[Stefan Monnier]

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