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

From: Eli Zaretskii <[email-masked]>
Date: Fri, 20 Jun 2025 09:41:13 +0300

> From: Stefan Monnier <[email-masked]>
> Cc: [email-masked],  [email-masked],  [email-masked]
> 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.