Subject: bug#24340: closed (Re: bug#24340: insert-file-contents calls before-change-functions too late)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 03 Oct 2016 13:49:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
24340: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=24340
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#24340: insert-file-contents calls before-change-functions too late
From: Stefan Monnier <[email-masked]>
To: Eli Zaretskii <[email-masked]>
Cc: [email-masked]
Date: Mon, 03 Oct 2016 09:48:14 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

> 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

Subject: insert-file-contents calls before-change-functions too late
From: Stefan Monnier <[email-masked]>
To: [email-masked]
Date: Tue, 30 Aug 2016 14:42:19 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

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