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

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

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