GNU bug report logs - #78777
30.1; insert-file-contents should not set buffer-file-name to nil

Previous Next

To reply to this bug, email your comments to 78777 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.1; insert-file-contents should not set buffer-file-name to nil
Date: Thu, 12 Jun 2025 17:58:34 +0100

When investigating https://github.com/emacs-lsp/lsp-mode/issues/4782 and
https://github.com/purcell/emacs-reformatter/issues/63, I've discovered
the following bug in insert-file-contents.

Reproduction:

1. echo "hello world" > ~/test.txt
2. emacs -q ~/test.txt
3. M-x eval-expression RET (add-hook 'after-change-functions (lambda (&rest _) (message "buffer-file-name: %s, current-buffer: %s" (buffer-file-name) (current-buffer))) nil t) RET
4. Type something into the buffer
5. M-x eval-expression RET (insert-file-contents (buffer-file-name) nil nil nil t) RET
6. Switch to the *Messages* buffer and observe the buffer file name is nil.

Expectation:

insert-file-contents should not set buffer-file-name to nil

Message #8 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1;
 insert-file-contents should not set buffer-file-name to nil
Date: Thu, 12 Jun 2025 21:19:47 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Thu, 12 Jun 2025 17:58:34 +0100
> 
> 
> When investigating https://github.com/emacs-lsp/lsp-mode/issues/4782 and
> https://github.com/purcell/emacs-reformatter/issues/63, I've discovered
> the following bug in insert-file-contents.
> 
> Reproduction:
> 
> 1. echo "hello world" > ~/test.txt
> 2. emacs -q ~/test.txt
> 3. M-x eval-expression RET (add-hook 'after-change-functions (lambda (&rest _) (message "buffer-file-name: %s, current-buffer: %s" (buffer-file-name) (current-buffer))) nil t) RET
> 4. Type something into the buffer
> 5. M-x eval-expression RET (insert-file-contents (buffer-file-name) nil nil nil t) RET
> 6. Switch to the *Messages* buffer and observe the buffer file name is nil.
> 
> Expectation:
> 
> insert-file-contents should not set buffer-file-name to nil

I don't think it does.  (buffer-file-name) returns nil when the call
is evaluated in the minibuffer, and I thin that's what you see.  After
performing the above recipe the buffer-filename of the buffer that
visits test.txt remains to be "test.txt", at least in my testing.  So
the only evidence that it's set to nil are the messages logged in
*Messages*, and they are about a different buffer.

Message #11 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Thu, 12 Jun 2025 20:34:30 +0100

[Message part 1 (text/plain, inline)]

On Thu, Jun 12, 2025 at 7:19 PM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> > Date: Thu, 12 Jun 2025 17:58:34 +0100
> >
> >
> > When investigating https://github.com/emacs-lsp/lsp-mode/issues/4782 and
> > https://github.com/purcell/emacs-reformatter/issues/63, I've discovered
> > the following bug in insert-file-contents.
> >
> > Reproduction:
> >
> > 1. echo "hello world" > ~/test.txt
> > 2. emacs -q ~/test.txt
> > 3. M-x eval-expression RET (add-hook 'after-change-functions (lambda
> (&rest _) (message "buffer-file-name: %s, current-buffer: %s"
> (buffer-file-name) (current-buffer))) nil t) RET
> > 4. Type something into the buffer
> > 5. M-x eval-expression RET (insert-file-contents (buffer-file-name) nil
> nil nil t) RET
> > 6. Switch to the *Messages* buffer and observe the buffer file name is
> nil.
> >
> > Expectation:
> >
> > insert-file-contents should not set buffer-file-name to nil
>
> I don't think it does.  (buffer-file-name) returns nil when the call
> is evaluated in the minibuffer, and I thin that's what you see.  After
> performing the above recipe the buffer-filename of the buffer that
> visits test.txt remains to be "test.txt", at least in my testing.  So
> the only evidence that it's set to nil are the messages logged in
> *Messages*, and they are about a different buffer.
>

When a lisp expression is evaluated in the minibuffer, the current buffer
is the file buffer, not the minibuffer. You can easily verify this with M-x
eval-expression RET (current-buffer)  RET.

The crux of the matter is the lambda added to after-change-function.
`insert-file-contents` calls `signals_after_change` (fileio.c line 5007 on
master), which will run the after-change-functions. It is at this moment
the buffer-file-name is nil.

In addition, the following will NOT result in insert-file-contents
temporarily setting the buffer-file-name of the current buffer to nil:

(let ((coding-system-for-read 'no-conversion)
      (coding-system-for-write 'no-conversion))
  (insert-file-contents (buffer-file-name) nil nil nil t))

Which suggests some of the code conversion logic in the C function is
erroneously setting the current buffer's file-name to nil, and then running
the after-change-functions before resetting it.

[Message part 2 (text/html, inline)]

Message #14 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sat, 14 Jun 2025 17:51:22 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Thu, 12 Jun 2025 20:34:30 +0100
> Cc: 78777 <at> debbugs.gnu.org
> 
> On Thu, Jun 12, 2025 at 7:19 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>  > From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
>  > Date: Thu, 12 Jun 2025 17:58:34 +0100
>  > 
>  > 
>  > When investigating https://github.com/emacs-lsp/lsp-mode/issues/4782 and
>  > https://github.com/purcell/emacs-reformatter/issues/63, I've discovered
>  > the following bug in insert-file-contents.
>  > 
>  > Reproduction:
>  > 
>  > 1. echo "hello world" > ~/test.txt
>  > 2. emacs -q ~/test.txt
>  > 3. M-x eval-expression RET (add-hook 'after-change-functions (lambda (&rest _) (message
>  "buffer-file-name: %s, current-buffer: %s" (buffer-file-name) (current-buffer))) nil t) RET
>  > 4. Type something into the buffer
>  > 5. M-x eval-expression RET (insert-file-contents (buffer-file-name) nil nil nil t) RET
>  > 6. Switch to the *Messages* buffer and observe the buffer file name is nil.
>  > 
>  > Expectation:
>  > 
>  > insert-file-contents should not set buffer-file-name to nil
> 
>  I don't think it does.  (buffer-file-name) returns nil when the call
>  is evaluated in the minibuffer, and I thin that's what you see.  After
>  performing the above recipe the buffer-filename of the buffer that
>  visits test.txt remains to be "test.txt", at least in my testing.  So
>  the only evidence that it's set to nil are the messages logged in
>  *Messages*, and they are about a different buffer.
> 
> When a lisp expression is evaluated in the minibuffer, the current buffer is the file buffer, not the minibuffer.
> You can easily verify this with M-x eval-expression RET (current-buffer)  RET.
> 
> The crux of the matter is the lambda added to after-change-function. `insert-file-contents` calls
> `signals_after_change` (fileio.c line 5007 on master), which will run the after-change-functions. It is at this
> moment the buffer-file-name is nil.
> 
> In addition, the following will NOT result in insert-file-contents temporarily setting the buffer-file-name of the
> current buffer to nil:
> 
> (let ((coding-system-for-read 'no-conversion)
>       (coding-system-for-write 'no-conversion))
>   (insert-file-contents (buffer-file-name) nil nil nil t))
> 
> Which suggests some of the code conversion logic in the C function is erroneously setting the current
> buffer's file-name to nil, and then running the after-change-functions before resetting it.

I think your after-change-function is called not due to
insert-file-contents inserting the text from the file, but from the
functions that decode the file's text.  To see if this is true, make
your after-change-function show the name of the current buffer.  If
I'm right, you will see something like " *code conversion works" as
the name of the buffer, in which case indeed buffer-filename is
expected to be nil, and I don't see a bug here.

Message #17 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 15:45:02 +0100

[Message part 1 (text/plain, inline)]

> I think your after-change-function is called not due to
> insert-file-contents inserting the text from the file, but from the
> functions that decode the file's text.  To see if this is true, make
> your after-change-function show the name of the current buffer.  If
> I'm right, you will see something like " *code conversion works" as
> the name of the buffer, in which case indeed buffer-filename is
> expected to be nil, and I don't see a bug here.
>

 I have suspected the same as the only call to set a buffer's name to nil
is found in the implementation of insert-file-contents near the code
conversion buffer, but that's a red herring. As you can see from my report,
the after-change-function does contain a call to (current-buffer), and the
message prints out the file buffer's name correctly.

Curiously, if you save the file between step 4 and 5 from my reproduction
instruction, the buffer-file-name is printed correctly.

Hope this helps.

[Message part 2 (text/html, inline)]

Message #20 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 18:00:12 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Sun, 15 Jun 2025 15:45:02 +0100
> Cc: 78777 <at> debbugs.gnu.org
> 
>  I think your after-change-function is called not due to
>  insert-file-contents inserting the text from the file, but from the
>  functions that decode the file's text.  To see if this is true, make
>  your after-change-function show the name of the current buffer.  If
>  I'm right, you will see something like " *code conversion works" as
>  the name of the buffer, in which case indeed buffer-filename is
>  expected to be nil, and I don't see a bug here.
> 
>  I have suspected the same as the only call to set a buffer's name to nil is found in the implementation of
> insert-file-contents near the code conversion buffer, but that's a red herring. As you can see from my
> report, the after-change-function does contain a call to (current-buffer), and the message prints out the file
> buffer's name correctly.

That's not what I see.  The only call to after-change-functions that
insert-file-contents does is not done in your recipe: if I set a
breakpoint there, it is never hit.

So I have no doubt that what you see is an unrelated call to
after-change-functions.  The evidence that current-buffer returns the
buffer name you expect is not enough: the actual current buffer can be
different, and I saw it in GDB.

So, given what I see under a debugger, there's no bug here.

Why did you think insert-file-contents should at all call
after-change-functions in this scenario?

Message #23 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 16:17:43 +0100

[Message part 1 (text/plain, inline)]

>
> That's not what I see.  The only call to after-change-functions that
> insert-file-contents does is not done in your recipe: if I set a
> breakpoint there, it is never hit.
>
> So I have no doubt that what you see is an unrelated call to
> after-change-functions.  The evidence that current-buffer returns the
> buffer name you expect is not enough: the actual current buffer can be
> different, and I saw it in GDB.
>
> So, given what I see under a debugger, there's no bug here.
>
> Why did you think insert-file-contents should at all call
> after-change-functions in this scenario?
>

I was never able to set up GDB as I'm on a Mac, so I actually am not sure
if the signal_after_change call is made directly inside
insert-file-contents. Looking at it again, it can't possibly be called
there as that line is under the notfound label, which is only jumped to
when the file descriptor is invalid.

It is possible that some other Emacs internal machinery set the current
buffer-file-name to nil temporarily and invoke signal_after_change after
insert-file-contents has replaced the buffer content, but I have not been
able to identify where. What is sure is a lone call to insert-file-contents
runs the after-change-functions.

[Message part 2 (text/html, inline)]

Message #26 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 18:35:44 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Sun, 15 Jun 2025 16:17:43 +0100
> Cc: 78777 <at> debbugs.gnu.org
> 
>  That's not what I see.  The only call to after-change-functions that
>  insert-file-contents does is not done in your recipe: if I set a
>  breakpoint there, it is never hit.
> 
>  So I have no doubt that what you see is an unrelated call to
>  after-change-functions.  The evidence that current-buffer returns the
>  buffer name you expect is not enough: the actual current buffer can be
>  different, and I saw it in GDB.
> 
>  So, given what I see under a debugger, there's no bug here.
> 
>  Why did you think insert-file-contents should at all call
>  after-change-functions in this scenario?
> 
> I was never able to set up GDB as I'm on a Mac, so I actually am not sure if the signal_after_change call is
> made directly inside insert-file-contents. Looking at it again, it can't possibly be called there as that line is
> under the notfound label, which is only jumped to when the file descriptor is invalid.
> 
> It is possible that some other Emacs internal machinery set the current buffer-file-name to nil temporarily and
> invoke signal_after_change after insert-file-contents has replaced the buffer content, but I have not been
> able to identify where. What is sure is a lone call to insert-file-contents runs the after-change-functions.

Yes, and that lone call is never made in your scenario.

So I'm no longer sure what problem are we discussing here.

Message #29 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 16:40:42 +0100

[Message part 1 (text/plain, inline)]

> Yes, and that lone call is never made in your scenario.
>
> So I'm no longer sure what problem are we discussing here.
>

The lone call to insert-file-contents, as in step 5 of my reproduction.
Calling insert-file-contents after typing in a file buffer runs the
after-change-functions hooks. When these hooks are run, the current
buffer's file name is seen to have been temporarily set to nil, which
should not be the case.

Is this clear?

[Message part 2 (text/html, inline)]

Message #32 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 18:59:46 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Sun, 15 Jun 2025 16:40:42 +0100
> Cc: 78777 <at> debbugs.gnu.org
> 
>  Yes, and that lone call is never made in your scenario.
> 
>  So I'm no longer sure what problem are we discussing here.
> 
> The lone call to insert-file-contents, as in step 5 of my reproduction. Calling insert-file-contents after typing in
> a file buffer runs the after-change-functions hooks. When these hooks are run, the current buffer's file name
> is seen to have been temporarily set to nil, which should not be the case.
> 
> Is this clear?

I understand what you are saying, but not why you think it's a bug.
Once again, the call to after-change-functions in this scenario is
done not from insert-file-contents, but from the function that decodes
the text read from the file.  That function runs in a temporary
buffer, which has no associated file name.

The Subject of this bug says "insert-file-contents should not set
buffer-file-name yo nil", but what I found that insert-file-contents
doesn't set buffer-file-name to nil.  It's an illusion created by the
way you use after-change-functions.

Message #35 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sun, 15 Jun 2025 17:30:47 +0100

[Message part 1 (text/plain, inline)]

>
> I understand what you are saying, but not why you think it's a bug.
> Once again, the call to after-change-functions in this scenario is
> done not from insert-file-contents, but from the function that decodes
> the text read from the file.  That function runs in a temporary
> buffer, which has no associated file name.
>

Because the docstring of after-change-functions says the hooks are called
with the current buffer, and the current buffer at the time
insert-file-contents is called, is the file buffer, not the code conversion
buffer. The message printed out also indicated that the current buffer at
the time the after-change-functions lambda is called is indeed the file
buffer, not the code conversion buffer. The file buffer's buffer-file-name
should not be set to nil by insert-file-contents or any function it
calls. As far as I know, only `insert-file-contents` exhibits this
undocumented behavior for any functions that change buffer contents.

The guarantee of after-change-functions being called with the current
buffer isn't very useful if crucial variables of the current buffer such as
the buffer file name cannot also be guaranteed to remain the same. This use
case is illustrated by the tickets I linked to in my original report. There
exist many file formatter packages that replace the content of the current
buffer with a temporary file with the formatted file content inside a
before-save-hook due to slowness of replace-buffer-content. When these
packages are used in combination with lsp-mode, which has a function added
to after-change-functions that needs access to the current buffer's file
name in order to synchronize the document between the editor and the
language server.

[Message part 2 (text/html, inline)]

Message #38 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Mon, 16 Jun 2025 12:09:14 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Sun, 15 Jun 2025 17:30:47 +0100
> Cc: 78777 <at> debbugs.gnu.org
> 
>  I understand what you are saying, but not why you think it's a bug.
>  Once again, the call to after-change-functions in this scenario is
>  done not from insert-file-contents, but from the function that decodes
>  the text read from the file.  That function runs in a temporary
>  buffer, which has no associated file name.
> 
> Because the docstring of after-change-functions says the hooks are called with the current buffer, and the
> current buffer at the time insert-file-contents is called, is the file buffer, not the code conversion buffer. The
> message printed out also indicated that the current buffer at the time the after-change-functions lambda is
> called is indeed the file buffer, not the code conversion buffer. The file buffer's buffer-file-name should not
> be set to nil by insert-file-contents or any function it calls. As far as I know, only `insert-file-contents` exhibits
> this undocumented behavior for any functions that change buffer contents.

OK, I see what happens.  insert-file-contents intentionally binds
buffer-file-name temporarily to nil when it is called with REPLACE
non-nil, during the deletion or insertion from the code-conversion
buffer.  It does that to avoid prompting the user via
ask-user-about-supersession-threat, which in this case will confuse.

So this is a feature, an intentional behavior.

> The guarantee of after-change-functions being called with the current buffer isn't very useful if crucial
> variables of the current buffer such as the buffer file name cannot also be guaranteed to remain the same.
> This use case is illustrated by the tickets I linked to in my original report. There exist many file formatter
> packages that replace the content of the current buffer with a temporary file with the formatted file content
> inside a before-save-hook due to slowness of replace-buffer-content. When these packages are used in
> combination with lsp-mode, which has a function added to after-change-functions that needs access to the
> current buffer's file name in order to synchronize the document between the editor and the language server.

I suggest that buffer-modification hooks use buffer-file-truename
instead, it will provide the correct file name in this (and other)
cases.

Message #41 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Mon, 16 Jun 2025 13:19:08 +0100

[Message part 1 (text/plain, inline)]

On Mon, Jun 16, 2025 at 10:09 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> > Date: Sun, 15 Jun 2025 17:30:47 +0100
> > Cc: 78777 <at> debbugs.gnu.org
> >
> >  I understand what you are saying, but not why you think it's a bug.
> >  Once again, the call to after-change-functions in this scenario is
> >  done not from insert-file-contents, but from the function that decodes
> >  the text read from the file.  That function runs in a temporary
> >  buffer, which has no associated file name.
> >
> > Because the docstring of after-change-functions says the hooks are
> called with the current buffer, and the
> > current buffer at the time insert-file-contents is called, is the file
> buffer, not the code conversion buffer. The
> > message printed out also indicated that the current buffer at the time
> the after-change-functions lambda is
> > called is indeed the file buffer, not the code conversion buffer. The
> file buffer's buffer-file-name should not
> > be set to nil by insert-file-contents or any function it calls. As far
> as I know, only `insert-file-contents` exhibits
> > this undocumented behavior for any functions that change buffer contents.
>
> OK, I see what happens.  insert-file-contents intentionally binds
> buffer-file-name temporarily to nil when it is called with REPLACE
> non-nil, during the deletion or insertion from the code-conversion
> buffer.  It does that to avoid prompting the user via
> ask-user-about-supersession-threat, which in this case will confuse.
>
> So this is a feature, an intentional behavior.
>

This is one silly hack. If the intention is to suppress some file lock
interactive functions, isn't the Emacs convention to use some kind of
inhibit-* variable? In my case, I don't even use file locks, I've
set create-lockfiles permanantly to nil, there's no reason that any file
lock related functions should be run at all.

Can userlock--ask-user-about-supersession-threat be modified to check for
create-lockfiles and a newly introduced inhibit-file-lock internal variable
before running userlock--check-content-unchanged?

This change also seems to have been introduced to before-change-functions
are called at the right time
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2016-08/msg01029.html>,
perhaps we should ask Stefan what the best thing to do here is.


>
> > The guarantee of after-change-functions being called with the current
> buffer isn't very useful if crucial
> > variables of the current buffer such as the buffer file name cannot also
> be guaranteed to remain the same.
> > This use case is illustrated by the tickets I linked to in my original
> report. There exist many file formatter
> > packages that replace the content of the current buffer with a temporary
> file with the formatted file content
> > inside a before-save-hook due to slowness of replace-buffer-content.
> When these packages are used in
> > combination with lsp-mode, which has a function added to
> after-change-functions that needs access to the
> > current buffer's file name in order to synchronize the document between
> the editor and the language server.
>
> I suggest that buffer-modification hooks use buffer-file-truename
> instead, it will provide the correct file name in this (and other)
> cases.
>

This is not ideal. The Language Server Protocol does not specify how to
handle symlinks, there is no guarantee that file content changes to the
source of a symlink will also change the target, which as far as the
language server is concerned, can very well be just 2 regular files.

In addition, the expectation is that unless the user has explicitly changed
the buffer-file-name by renaming the buffer and/or file,
buffer-file-name should remain unmodified at all times from the user's
perspective. Examining buffer-file-truename should only be used as a crutch
to look up the target of a file symlink quickly without hitting the disk.
Both of the above scenarios are not what's happening here.
insert-file-content is simply writing to a buffer and
after-change-functions are simply reacting to changes in the buffer, not
the file on disk.

[Message part 2 (text/html, inline)]

Message #44 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Mon, 16 Jun 2025 16:58:43 +0300

> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Mon, 16 Jun 2025 13:19:08 +0100
> Cc: 78777 <at> debbugs.gnu.org
> 
>  OK, I see what happens.  insert-file-contents intentionally binds
>  buffer-file-name temporarily to nil when it is called with REPLACE
>  non-nil, during the deletion or insertion from the code-conversion
>  buffer.  It does that to avoid prompting the user via
>  ask-user-about-supersession-threat, which in this case will confuse.
> 
>  So this is a feature, an intentional behavior.
> 
> This is one silly hack.

Thank you for your kind words.  Any hope of convincing you to use
kinder language?

> If the intention is to suppress some file lock interactive functions, isn't the Emacs
> convention to use some kind of inhibit-* variable? In my case, I don't even use file locks, I've set
> create-lockfiles permanantly to nil, there's no reason that any file lock related functions should be run at all.
> 
> Can userlock--ask-user-about-supersession-threat be modified to check for create-lockfiles and a newly
> introduced inhibit-file-lock internal variable before running userlock--check-content-unchanged? 

ask-user-about-supersession-threat is not about file locks, it's about
discovering that the file was modified outside of this Emacs session
(perhaps by another Emacs session or some other system command).

> This change also seems to have been introduced to before-change-functions are called at the right time,
> perhaps we should ask Stefan what the best thing to do here is.

Perhaps.

>  I suggest that buffer-modification hooks use buffer-file-truename
>  instead, it will provide the correct file name in this (and other)
>  cases.
> 
> This is not ideal. The Language Server Protocol does not specify how to handle symlinks, there is no
> guarantee that file content changes to the source of a symlink will also change the target, which as far as the
> language server is concerned, can very well be just 2 regular files.

An alternative is to go back to using replace-buffer-content.  If its
default operation is (or could be) too slow, you should be able to
avoid that by passing zero for the MAX-SECS argument.

> In addition, the expectation is that unless the user has explicitly changed the buffer-file-name by renaming
> the buffer and/or file, buffer-file-name should remain unmodified at all times from the user's perspective.
> Examining buffer-file-truename should only be used as a crutch to look up the target of a file symlink quickly
> without hitting the disk. Both of the above scenarios are not what's happening here. insert-file-content is
> simply writing to a buffer and after-change-functions are simply reacting to changes in the buffer, not the file
> on disk.

Can you explain what exactly is the problem with buffer-file-name
being nil in the cases where you found that?  If the problem is that a
function placed on the after-change-functions hook doesn't expect
that, it should be easy to amend the function to deal with the
problem, no?  IOW, would it solve the problem if the after-change hook
simply did nothing when buffer-file-name is not a valid file name?  If
not, why not?

Message #47 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: Kenichi Handa <handa <at> gnu.org>, Eli Zaretskii <eliz <at> gnu.org>,
 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Mon, 16 Jun 2025 11:14:20 -0400

> Can userlock--ask-user-about-supersession-threat be modified to check for
> create-lockfiles and a newly introduced inhibit-file-lock internal variable
> before running userlock--check-content-unchanged?
>
> This change also seems to have been introduced to before-change-functions
> are called at the right time
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2016-08/msg01029.html>,
> perhaps we should ask Stefan what the best thing to do here is.

No, if you look at the diff, you'll see that this just moved the
`specbind`.  It was introduced earlier by:

```diff
commit d07af40d882673ccb30c267a617e673c22a85ee4
Author: Kenichi Handa <handa <at> gnu.org>
Date:   Mon Oct 23 12:40:32 2006 +0000

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

Nor do I understand why it was added to the code path where coding
conversion was needed but not to the other one.

I see that back in 2016 I apparently understood the situation a bit
better (or at least I thought) because while moving the `specbind`
I managed to improve the comment with:

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

but it's still insufficient for me to understand now what's going on.
Handa?


        Stefan

Message #50 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: "K. Handa" <handa <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: handa <at> gnu.org, eliz <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Wed, 18 Jun 2025 23:23:16 +0900

Hi,

In article <jwvtt4fag1z.fsf-monnier+emacs <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca> 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.

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

---
K. Handa
handa <at> gnu.org

Message #53 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: Kenichi Handa <handa <at> gnu.org>, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Wed, 18 Jun 2025 15:24:42 -0400

> When investigating https://github.com/emacs-lsp/lsp-mode/issues/4782 and
> https://github.com/purcell/emacs-reformatter/issues/63, I've discovered
> the following bug in insert-file-contents.
[...]
> insert-file-contents should not set buffer-file-name to nil

I have not tracked lsp-mode's code closely enough to fully understand
what's going on, but IIUC from the above bug reports,
`lsp-diagnostics--request-pull-diagnostics` is executed (indirectly)
from `before/after-change-functions`.

While I can agree that binding `buffer-file-name` to nil within
`insert-file-contents` is a bug, I think lsp-mode would do well to
change its code so it does not run things like
`lsp-diagnostics--request-pull-diagnostics` directly from
`before/after-change-functions`, because those hooks are a bit like
POSIX signal handlers, i.e. you should do as little work in there as
possible.  IOW, lsp-mode should try to just record the change there and
move all the rest of the processing to some other place, such as
`post-command-hook` or a timer.

This would not only circumvent the current problem but also have further
benefits, typically for operations which run `after-change-functions`
many times, where lsp-mode could combine the corresponding LSP requests.


        Stefan

Message #56 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "K. Handa" <handa <at> gnu.org>
Cc: eliz <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Wed, 18 Jun 2025 15:49:07 -0400

[Message part 1 (text/plain, inline)]

> In article <jwvtt4fag1z.fsf-monnier+emacs <at> gnu.org>, Stefan Monnier
> <monnier <at> iro.umontreal.ca> 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

[insert-file-contents.patch (text/x-diff, inline)]

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

Message #59 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: handa <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Thu, 19 Jun 2025 08:30:52 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: eliz <at> gnu.org,  wyuenho <at> gmail.com,  78777 <at> debbugs.gnu.org
> Date: Wed, 18 Jun 2025 15:49:07 -0400
> 
> 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`?

TBH, I don't like this, because it just makes the issue more subtle.

IMO, if we want to solve this cleanly, we should introduce a variable
which, when bound, causes ask-user-about-supersession-threat to do
nothing and return.  Then insert-file-contents could bind that
variable instead of buffer-file-name.  Alternatively, the test for
this new variable could be in prepare_to_modify_buffer, here:

  if (!NILP (BVAR (base_buffer, file_truename))
      /* Make binding buffer-file-name to nil effective.  */
      && !NILP (BVAR (base_buffer, filename))
      && SAVE_MODIFF >= MODIFF)
    Flock_file (BVAR (base_buffer, file_truename));

As yet another alternative, we could add to the test above some other
internal knob C variable that insert-file-contents could use, so that
the above condition is not satisfied.

IMO, any of these alternatives is better than your proposal, because
it solves the problem completely, not partially, and because it
doesn't run any risks of regressions due to the VISIT case which until
now did not matter.

Message #62 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: handa <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Thu, 19 Jun 2025 10:56:03 -0400

>> 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`?
>
> TBH, I don't like this, because it just makes the issue more subtle.

Does it?  To me it makes the "inhibit ask-supersession" more clear:
`ask-supersession` is about warning when a buffer modification risks
creating a "fork" of the file, but when VISIT is non-nil
`insert-file-contents` actually erases local changes and brings the
buffer in-sync with the file so it cannot risk creating a fork (on the
contrary, it tends to undo a fork if there was one).

> IMO, if we want to solve this cleanly, we should introduce a variable
> which, when bound, causes ask-user-about-supersession-threat to do
> nothing and return.  Then insert-file-contents could bind that
> variable instead of buffer-file-name.  Alternatively, the test for
> this new variable could be in prepare_to_modify_buffer, here:
>
>   if (!NILP (BVAR (base_buffer, file_truename))
>       /* Make binding buffer-file-name to nil effective.  */
>       && !NILP (BVAR (base_buffer, filename))
>       && SAVE_MODIFF >= MODIFF)
>     Flock_file (BVAR (base_buffer, file_truename));
>
> As yet another alternative, we could add to the test above some other
> internal knob C variable that insert-file-contents could use, so that
> the above condition is not satisfied.

Ah, you're talking about something else: I'm talking about "when should
we inhibit ask-supersession" whereas you're talking about "how".

> IMO, any of these alternatives is better than your proposal, because
> it solves the problem completely, not partially, and because it
> doesn't run any risks of regressions due to the VISIT case which until
> now did not matter.

IOW, at least in my mind, the code as it stands is mysterious, whereas
with the `!NILP (visit)` it makes perfect sense.  This is orthogonal to
whether we want to inhibit ask-supersession by binding
`buffer-file-name` or via some other (new) mechanism.


        Stefan

Message #65 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: handa <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Thu, 19 Jun 2025 18:05:18 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: handa <at> gnu.org,  wyuenho <at> gmail.com,  78777 <at> debbugs.gnu.org
> Date: Thu, 19 Jun 2025 10:56:03 -0400
> 
> Ah, you're talking about something else: I'm talking about "when should
> we inhibit ask-supersession" whereas you're talking about "how".

Yes, because I see no reason to change the "when".  We had zero
problems with it since it was introduced long ago.

> > IMO, any of these alternatives is better than your proposal, because
> > it solves the problem completely, not partially, and because it
> > doesn't run any risks of regressions due to the VISIT case which until
> > now did not matter.
> 
> IOW, at least in my mind, the code as it stands is mysterious, whereas
> with the `!NILP (visit)` it makes perfect sense.  This is orthogonal to
> whether we want to inhibit ask-supersession by binding
> `buffer-file-name` or via some other (new) mechanism.

But then this bug report is not the right place to discuss your
suggestions, right?

Message #68 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: handa <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Thu, 19 Jun 2025 13:10:30 -0400

>> Ah, you're talking about something else: I'm talking about "when should
>> we inhibit ask-supersession" whereas you're talking about "how".
> Yes, because I see no reason to change the "when".  We had zero
> problems with it since it was introduced long ago.

Oh, well for me the reason is: to make the code understandable.

Also, I think the code as it stands is wrong (which is part of the
reason why it's hard to understand) in the reformatter-case
(where we (well, they) use `insert-file-contents` to REPLACE the
buffer's content with that of another file (which, in that case,
contains a reformatted version of the buffer's contents)), because it
will fail to prompt the user about the change you're performing
this way.  E.g.

- Start with:

      % src/emacs -Q BUGS &
      % echo foo >>BUGS

- In the Emacs session type:

      a

  Notice how Emacs prompts about "changed on disk, really edit".
  Hit `C-g` so we don't actually edit the buffer.

- In the Emacs session do:

      M-: (insert-file-contents "README" nil nil nil t) RET

  Notice how Emacs just blindly changed the contents of the buffer
  without prompting about "changed on disk, really edit".

I think this last step is an error, we should be prompted just as we are
with any other buffer modification that diverges from the file's contents.

>> > IMO, any of these alternatives is better than your proposal, because
>> > it solves the problem completely, not partially, and because it
>> > doesn't run any risks of regressions due to the VISIT case which until
>> > now did not matter.
>> 
>> IOW, at least in my mind, the code as it stands is mysterious, whereas
>> with the `!NILP (visit)` it makes perfect sense.  This is orthogonal to
>> whether we want to inhibit ask-supersession by binding
>> `buffer-file-name` or via some other (new) mechanism.
>
> But then this bug report is not the right place to discuss your
> suggestions, right?

Except that it's where I discovered the problem and that it happens to
make the OP's problem disappear.  IOW, we have now 3 ways to fix the
PS's problem, all of which have other benefits.  I don't think we need
to choose: we should apply all three (except that the third needs to
be done in `lsp-mode` so it's not under our control).


        Stefan

Message #71 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: handa <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Thu, 19 Jun 2025 13:48:46 -0400

> Except that it's where I discovered the problem and that it happens to
> make the OP's problem disappear.  IOW, we have now 3 ways to fix the
> PS's problem, all of which have other benefits.  I don't think we need
  ^^^^
  OP's

[ Not sure what happened here.  ]


        Stefan

Message #74 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: handa <at> gnu.org, wyuenho <at> gmail.com, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Fri, 20 Jun 2025 09:41:13 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: handa <at> gnu.org,  wyuenho <at> gmail.com,  78777 <at> debbugs.gnu.org
> Date: Thu, 19 Jun 2025 13:10:30 -0400
> 
> >> Ah, you're talking about something else: I'm talking about "when should
> >> we inhibit ask-supersession" whereas you're talking about "how".
> > Yes, because I see no reason to change the "when".  We had zero
> > problems with it since it was introduced long ago.
> 
> Oh, well for me the reason is: to make the code understandable.
> 
> Also, I think the code as it stands is wrong (which is part of the
> reason why it's hard to understand) in the reformatter-case
> (where we (well, they) use `insert-file-contents` to REPLACE the
> buffer's content with that of another file (which, in that case,
> contains a reformatted version of the buffer's contents)), because it
> will fail to prompt the user about the change you're performing
> this way.  E.g.
> 
> - Start with:
> 
>       % src/emacs -Q BUGS &
>       % echo foo >>BUGS
> 
> - In the Emacs session type:
> 
>       a
> 
>   Notice how Emacs prompts about "changed on disk, really edit".
>   Hit `C-g` so we don't actually edit the buffer.
> 
> - In the Emacs session do:
> 
>       M-: (insert-file-contents "README" nil nil nil t) RET
> 
>   Notice how Emacs just blindly changed the contents of the buffer
>   without prompting about "changed on disk, really edit".
> 
> I think this last step is an error, we should be prompted just as we are
> with any other buffer modification that diverges from the file's contents.

AFAIU, the problem for this particular aspect is not VISIT alone, the
problem is when both VISIT is nil _and_ REPLACE is non-nil.  We also
need to make sure we don't ask the user more than once in that case,
and do not ask at all if replacing is a no-op (i.e. the new contents
matches the old contents exactly).

> >> > IMO, any of these alternatives is better than your proposal, because
> >> > it solves the problem completely, not partially, and because it
> >> > doesn't run any risks of regressions due to the VISIT case which until
> >> > now did not matter.
> >> 
> >> IOW, at least in my mind, the code as it stands is mysterious, whereas
> >> with the `!NILP (visit)` it makes perfect sense.  This is orthogonal to
> >> whether we want to inhibit ask-supersession by binding
> >> `buffer-file-name` or via some other (new) mechanism.
> >
> > But then this bug report is not the right place to discuss your
> > suggestions, right?
> 
> Except that it's where I discovered the problem and that it happens to
> make the OP's problem disappear.  IOW, we have now 3 ways to fix the
> PS's problem, all of which have other benefits.  I don't think we need
> to choose: we should apply all three (except that the third needs to
> be done in `lsp-mode` so it's not under our control).

Each problem should be fixed separately, so the fixes could then be
reverted separately if needed, let alone analyzed and understood
separately.  IOW, we need two separate patches, and we need to discuss
them separately.  The issue you raise will also need serious testing
(whereas the other one just changes how the existing behavior is
implemented), because it changes behavior, and we so need to make sure
we don't cause any regressions, what with the many possible scenarios
that insert-file-contents could be and is used.  You insist on
discussing both issues in a single bug report, but please keep in mind
that too many unrelated discussions make a bug report very long and
hard to read later on.  So I still prefer to have a separate bug
report about the missing user prompt.

Message #77 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sat, 21 Jun 2025 14:16:48 +0100

[Message part 1 (text/plain, inline)]

> An alternative is to go back to using replace-buffer-content.  If its
> default operation is (or could be) too slow, you should be able to
> avoid that by passing zero for the MAX-SECS argument.
>

This is not a great solution. The point and mark will often move far away
from where it was after replacing content, even inside a
save-mark-and-incursion. There is a [RCS patch algorithm](
https://github.com/radian-software/apheleia/blob/main/apheleia-rcs.el)
available in a third party package, but there's no built-in Emacs function
that can do better than insert-file-contents in terms of speed and behavior.

[Message part 2 (text/html, inline)]

Message #80 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Kenichi Handa <handa <at> gnu.org>, 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sat, 21 Jun 2025 14:18:35 +0100

[Message part 1 (text/plain, inline)]

On Wed, Jun 18, 2025 at 8:24 PM Stefan Monnier <monnier <at> iro.umontreal.ca>
wrote:

> > When investigating https://github.com/emacs-lsp/lsp-mode/issues/4782 and
> > https://github.com/purcell/emacs-reformatter/issues/63, I've discovered
> > the following bug in insert-file-contents.
> [...]
> > insert-file-contents should not set buffer-file-name to nil
>
> I have not tracked lsp-mode's code closely enough to fully understand
> what's going on, but IIUC from the above bug reports,
> `lsp-diagnostics--request-pull-diagnostics` is executed (indirectly)
> from `before/after-change-functions`.
>
> While I can agree that binding `buffer-file-name` to nil within
> `insert-file-contents` is a bug, I think lsp-mode would do well to
> change its code so it does not run things like
> `lsp-diagnostics--request-pull-diagnostics` directly from
> `before/after-change-functions`, because those hooks are a bit like
> POSIX signal handlers, i.e. you should do as little work in there as
> possible.  IOW, lsp-mode should try to just record the change there and
> move all the rest of the processing to some other place, such as
> `post-command-hook` or a timer.
>
> This would not only circumvent the current problem but also have further
> benefits, typically for operations which run `after-change-functions`
> many times, where lsp-mode could combine the corresponding LSP requests.
>
>
>         Stefan
>
>
Yes, I believe your solution is also how eglot works, it's probably best to
batch changes before assembling a bigger JSON document for notifying the
LSP server at regular intervals.

[Message part 2 (text/html, inline)]

Message #83 received at 78777 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: handa <at> gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 78777 <at> debbugs.gnu.org
Subject: Re: bug#78777: 30.1; insert-file-contents should not set
 buffer-file-name to nil
Date: Sat, 21 Jun 2025 14:25:08 +0100

[Message part 1 (text/plain, inline)]

On Thu, Jun 19, 2025 at 6:30 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> > Cc: eliz <at> gnu.org,  wyuenho <at> gmail.com,  78777 <at> debbugs.gnu.org
> > Date: Wed, 18 Jun 2025 15:49:07 -0400
> >
> > 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`?
>
> TBH, I don't like this, because it just makes the issue more subtle.
>
> IMO, if we want to solve this cleanly, we should introduce a variable
> which, when bound, causes ask-user-about-supersession-threat to do
> nothing and return.  Then insert-file-contents could bind that
> variable instead of buffer-file-name.  Alternatively, the test for
> this new variable could be in prepare_to_modify_buffer, here:
>
>   if (!NILP (BVAR (base_buffer, file_truename))
>       /* Make binding buffer-file-name to nil effective.  */
>       && !NILP (BVAR (base_buffer, filename))
>       && SAVE_MODIFF >= MODIFF)
>     Flock_file (BVAR (base_buffer, file_truename));
>
> As yet another alternative, we could add to the test above some other
> internal knob C variable that insert-file-contents could use, so that
> the above condition is not satisfied.
>
> IMO, any of these alternatives is better than your proposal, because
> it solves the problem completely, not partially, and because it
> doesn't run any risks of regressions due to the VISIT case which until
> now did not matter.
>


I agree either of these approaches is better than binding buffer-file-name
to nil at any point in time during the execution of insert-file-contents.

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.