Subject: bug#78777: 30.1; insert-file-contents should not set buffer-file-name to nil

From: Stefan Monnier <[email-masked]>
Date: Wed, 18 Jun 2025 15:49:07 -0400

> In article <[email-masked]>, Stefan Monnier
> <[email-masked]> writes:
> [...]
>>     (Finsert_file_contents): On replacing, temporarily bind
>>     buffer-file-name to Qnil before calling insert_from_buffer.
>
>> diff --git a/src/fileio.c b/src/fileio.c
>> --- a/src/fileio.c
>> +++ b/src/fileio.c
>> @@ -4359,8 +4359,12 @@
>>        inserted_chars
>>  	= (buf_bytepos_to_charpos (XBUFFER (conversion_buffer),
>>  				   same_at_start + inserted)
>>  	   - 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.  */
>>        inserted = PT - temp;
>> ```
>
>> I'm not sure exactly what was the motivation for the change.
>> I mean, the comment explains that the purpose is to silence a potential
>> `ask-user-about-supersession-threat`, but I don't know why we should
>> care about such a possibility and why it would be right to silence
>> it here.
> I don't remember, sorry.

AFAICT this is used only when REPLACE is non-nil but I wonder whether it
should be limited also to the case where VISIT is non-nil.

E.g. after removing the three `specbind (Qbuffer_file_name", Qnil)` we
have in the current code (which are the descendants of the above
`specbind`), I see that I can get a "changed on disk; really edit"
prompt if I do the following:

    % emacs -Q src/regex-emacs.c &
    % echo foo >>src/regex-emacs.c
    in Emacs do: M-x revert-buffer RET

which is indeed undesirable and might be the original reason for
commit d07af40d8826.

But if I use the patch below to restrict the `specbind` to the case
where VISIT is non-nil, then the above recipe still works as it should.

>> Nor do I understand why it was added to the code path where coding
>> conversion was needed but not to the other one.
>
> ??? It seems that the code above is where coding conversion is not
> needed.

I don't understand: the patch (still quoted above) touches code which
refers to `buf_bytepos_to_charpos` and `conversion_buffer`, so it's
clearly a part of the code which had to deal with coding conversion.

I'm not a great fan of this `specbind` and maybe we could/should
inhibit the ask-supersession prompt in a more direct way, but I think
the patch below is an improvement (and should fix the "lsp-mode +
reformatter" case that triggered the current bug report).

Any objection to pushing it to `master`?

        Stefan
diff --git a/src/fileio.c b/src/fileio.c
index 95bfa979d13..73c97b2999e 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4568,14 +4568,19 @@ DEFUN ("insert-file-contents", Finsert_file_contents, Sinsert_file_contents,
 	  beg_offset += same_at_start - BEGV_BYTE;
 	  end_offset -= ZV_BYTE - same_at_end;
 
-          /* 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 (Qbuffer_file_name, Qnil);
+          if (!NILP (visit))
+            /* 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).
+	       This prompt makes no sense if we're VISITing the file,
+	       since the insertion makes the buffer *more* like the file
+	       rather than the reverse.
+               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 (Qbuffer_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);
@@ -4685,7 +4690,8 @@ DEFUN ("insert-file-contents", Finsert_file_contents, Sinsert_file_contents,
 	  if (same_at_start != same_at_end)
 	    {
               /* See previous specbind for the reason behind this.  */
-              specbind (Qbuffer_file_name, Qnil);
+              if (!NILP (visit))
+        	specbind (Qbuffer_file_name, Qnil);
 	      del_range_byte (same_at_start, same_at_end);
 	    }
 	  inserted = 0;
@@ -4735,7 +4741,8 @@ DEFUN ("insert-file-contents", Finsert_file_contents, Sinsert_file_contents,
       inserted -= (ZV_BYTE - same_at_end) + (same_at_start - BEGV_BYTE);
 
       /* See previous specbind for the reason behind this.  */
-      specbind (Qbuffer_file_name, Qnil);
+      if (!NILP (visit))
+        specbind (Qbuffer_file_name, Qnil);
       if (same_at_end != same_at_start)
 	{
 	  del_range_byte (same_at_start, same_at_end);