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

[Stefan Monnier]

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