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

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