GNU bug report logs - #78866
[PATCH] (Finsert_file_contents): Refine commit d07af40d8826

Previous Next

To reply to this bug, email your comments to 78866 AT debbugs.gnu.org.
There is no need to reopen the bug first.

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] (Finsert_file_contents): Refine commit d07af40d8826
Date: Sun, 22 Jun 2025 22:28:31 -0400

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

Tags: patch

As requested in bug#78777, here's a separate bug report for the
following problem:

- Start with:

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

- In the Emacs session type:

      a

  Notice how Emacs correctly 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.

The patch below does just that,


        Stefan

[0002-Finsert_file_contents-Refine-commit-d07af40d8826.patch (text/patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78866 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#78866: [PATCH] (Finsert_file_contents): Refine commit
 d07af40d8826
Date: Thu, 26 Jun 2025 12:09:49 +0300

> Cc: <monnier <at> iro.umontreal.ca>
> Date: Sun, 22 Jun 2025 22:28:31 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> - Start with:
> 
>       % src/emacs -Q BUGS &
>       % echo foo >>BUGS
> 
> - In the Emacs session type:
> 
>       a
> 
>   Notice how Emacs correctly 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.
> 
> The patch below does just that,

I agree that we should prompt, but I don't understand the logic behind
the proposed solution.  Why is it OK not to prompt when VISIT is
non-nil?  It is true that in that case we will be reading from
FILENAME, but the contents after the insertion will not necessarily be
identical to FILENAME, because not necessarily the entire contents of
the buffer is erased.  The case where the entire buffer is erased and
its contents replaced by that of FILENAME, in which case the prompt
indeed makes no sense, is a special case, not the general case.

Also note that the prompt, if we issue it here, will not be about
FILENAME, it will be about the file visited by the buffer, which at
this point is still the original buffer-file-name.  In your example
above, it's "BUGS", not "README".  Which is wrong when VISIT is
non-nil, but the correct solution is not just disable the prompt, it's
to issue the prompt about a different file, no?

So I'd prefer to fix this more thoroughly and correctly.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78866 <at> debbugs.gnu.org
Subject: Re: bug#78866: [PATCH] (Finsert_file_contents): Refine commit
 d07af40d8826
Date: Thu, 26 Jun 2025 09:38:14 -0400

> I agree that we should prompt, but I don't understand the logic behind
> the proposed solution.  Why is it OK not to prompt when VISIT is
> non-nil?  It is true that in that case we will be reading from
> FILENAME, but the contents after the insertion will not necessarily be
> identical to FILENAME, because not necessarily the entire contents of
> the buffer is erased.

If VISIT is non-nil, then REPLACE is also non-nil (or irrelevant
because the buffer is empty):

    if (!NILP (visit))
      {
        if (!NILP (beg) || !NILP (end))
          error ("Attempt to visit less than an entire file");
        if (BEG < Z && NILP (replace))
          error ("Cannot do file visiting in a non-empty buffer");
      }


- Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78866 <at> debbugs.gnu.org
Subject: Re: bug#78866: [PATCH] (Finsert_file_contents): Refine commit
 d07af40d8826
Date: Thu, 26 Jun 2025 16:44:08 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78866 <at> debbugs.gnu.org
> Date: Thu, 26 Jun 2025 09:38:14 -0400
> 
> > I agree that we should prompt, but I don't understand the logic behind
> > the proposed solution.  Why is it OK not to prompt when VISIT is
> > non-nil?  It is true that in that case we will be reading from
> > FILENAME, but the contents after the insertion will not necessarily be
> > identical to FILENAME, because not necessarily the entire contents of
> > the buffer is erased.
> 
> If VISIT is non-nil, then REPLACE is also non-nil (or irrelevant
> because the buffer is empty):
> 
>     if (!NILP (visit))
>       {
>         if (!NILP (beg) || !NILP (end))
>           error ("Attempt to visit less than an entire file");
>         if (BEG < Z && NILP (replace))
>           error ("Cannot do file visiting in a non-empty buffer");
>       }

Sure, but REPLACE replaces only the accessible portion of the buffert,
not all of it.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78866 <at> debbugs.gnu.org
Subject: Re: bug#78866: [PATCH] (Finsert_file_contents): Refine commit
 d07af40d8826
Date: Thu, 26 Jun 2025 12:39:14 -0400

>> > I agree that we should prompt, but I don't understand the logic behind
>> > the proposed solution.  Why is it OK not to prompt when VISIT is
>> > non-nil?  It is true that in that case we will be reading from
>> > FILENAME, but the contents after the insertion will not necessarily be
>> > identical to FILENAME, because not necessarily the entire contents of
>> > the buffer is erased.
>> If VISIT is non-nil, then REPLACE is also non-nil (or irrelevant
>> because the buffer is empty):
>> 
>>     if (!NILP (visit))
>>       {
>>         if (!NILP (beg) || !NILP (end))
>>           error ("Attempt to visit less than an entire file");
>>         if (BEG < Z && NILP (replace))
>>           error ("Cannot do file visiting in a non-empty buffer");
>>       }
>
> Sure, but REPLACE replaces only the accessible portion of the buffert,
> not all of it.

Then we should tighten the check even further.  Instead of just adding

    if (!NILP (visit))

we should additionally check that BEG==BEGV and Z==ZV:

    if (!NILP (visit) && BEG == BEGV && Z == ZV)

Tho, maybe the `BEG == BEGV && Z == ZV` check should be performed in the
above quoted sanity check, because I think using a non-nil VISIT within
a narrowed buffer goes against the intention of VISIT.


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78866 <at> debbugs.gnu.org
Subject: Re: bug#78866: [PATCH] (Finsert_file_contents): Refine commit
 d07af40d8826
Date: Thu, 26 Jun 2025 19:51:31 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78866 <at> debbugs.gnu.org
> Date: Thu, 26 Jun 2025 12:39:14 -0400
> 
> >> If VISIT is non-nil, then REPLACE is also non-nil (or irrelevant
> >> because the buffer is empty):
> >> 
> >>     if (!NILP (visit))
> >>       {
> >>         if (!NILP (beg) || !NILP (end))
> >>           error ("Attempt to visit less than an entire file");
> >>         if (BEG < Z && NILP (replace))
> >>           error ("Cannot do file visiting in a non-empty buffer");
> >>       }
> >
> > Sure, but REPLACE replaces only the accessible portion of the buffert,
> > not all of it.
> 
> Then we should tighten the check even further.  Instead of just adding
> 
>     if (!NILP (visit))
> 
> we should additionally check that BEG==BEGV and Z==ZV:
> 
>     if (!NILP (visit) && BEG == BEGV && Z == ZV)

To this I agree: the condition means that the previous buffer contents
is discarded, and likewise its relation to the original file, and the
buffer will from now on visit the new file, originally having the
exact contents of that new file.  In that specific case, indeed
there's no sense in any super-session warnings.

> Tho, maybe the `BEG == BEGV && Z == ZV` check should be performed in the
> above quoted sanity check, because I think using a non-nil VISIT within
> a narrowed buffer goes against the intention of VISIT.

I'd be reluctant to make such a change: who knows which package out
there uses this unintentional feature?

Message #25 received at 78866-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78866-done <at> debbugs.gnu.org
Subject: Re: bug#78866: [PATCH] (Finsert_file_contents): Refine commit
 d07af40d8826
Date: Fri, 27 Jun 2025 23:14:45 -0400

>> we should additionally check that BEG==BEGV and Z==ZV:
>> 
>>     if (!NILP (visit) && BEG == BEGV && Z == ZV)
>
> To this I agree: the condition means that the previous buffer contents
> is discarded, and likewise its relation to the original file, and the
> buffer will from now on visit the new file, originally having the
> exact contents of that new file.  In that specific case, indeed
> there's no sense in any super-session warnings.

Thanks, pushed.

>> Tho, maybe the `BEG == BEGV && Z == ZV` check should be performed in the
>> above quoted sanity check, because I think using a non-nil VISIT within
>> a narrowed buffer goes against the intention of VISIT.
> I'd be reluctant to make such a change: who knows which package out
> there uses this unintentional feature?

Fair enough, closing,


        Stefan

Previous Next

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