bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

X-Debbugs-Cc: Eli Zaretskii <eliz@gnu.org>
Severity: minor

Eli, sorry to disturb you with another edge-case bug, but the commit
that has introduced it was yours, I think ...

*** Issue A

In a shell execute:

echo foo > foo
chmod 0400 foo
./src/emacs -Q foo
C-x C-q
bar
C-0 C-x C-s
=> File foo is write-protected; try to save anyway?
yes RET
=> basic-save-buffer-2: Opening output file: Permission denied,
   /home/jschmidt/work/emacs-master/foo

When saving *with* backup, that is, without prefix C-0, everything works
as expected: The file temporarily gets write permissions, gets written
to, and write permissions get removed again.

The root cause is related to Eli's introduction of calls to
`set-file-extended-attributes' in ccad023bc3c7.  Here:

------------------------- files.el -------------------------
	;; If file not writable, see if we can make it writable
	;; temporarily while we write it.
	;; But no need to do so if we have just backed it up
	;; (setmodes is set) because that says we're superseding.
	(cond ((and tempsetmodes (not setmodes))
	       ;; Change the mode back, after writing.
	       (setq setmodes
                     (list (file-modes buffer-file-name)
                           (with-demoted-errors
                               "Error getting extended attributes: %s"
			     (file-extended-attributes buffer-file-name))
			   buffer-file-name))
	       ;; If set-file-extended-attributes fails, fall back on
	       ;; set-file-modes.
	       (unless
		   (with-demoted-errors "Error setting attributes: %s"
		     (set-file-extended-attributes buffer-file-name
						   (nth 1 setmodes)))
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
		 (set-file-modes buffer-file-name
				 (logior (car setmodes) 128)))))
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]
------------------------- files.el -------------------------

The marked call [1] is a no-op, since it does not change the permissions
in any way when writing them back, while the call [2] to
`set-file-modes' actually changes the file mode.

Since an extended file attribute Elisp object cannot be modified yet (is
that so?) to mark it, for example, as writable, I propose to just drop
that call to `set-file-extended-attributes', like this:

------------------------- snip -------------------------
diff --git a/lisp/files.el b/lisp/files.el
index e1421b403bf..d4d556fa470 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5944,14 +5944,8 @@ basic-save-buffer-2
                                "Error getting extended attributes: %s"
                             (file-extended-attributes buffer-file-name))
                           buffer-file-name))
-              ;; If set-file-extended-attributes fails, fall back on
-              ;; set-file-modes.
-              (unless
-                  (with-demoted-errors "Error setting attributes: %s"
-                    (set-file-extended-attributes buffer-file-name
-                                                  (nth 1 setmodes)))
-                (set-file-modes buffer-file-name
-                                (logior (car setmodes) 128)))))
+              (set-file-modes buffer-file-name
+                              (logior (car setmodes) 128))))
        (let (success)
          (unwind-protect
              (progn
------------------------- snip -------------------------

*** Issue B

But then there is another issue (?), probably even longer unnoticed.  To
notice it, apply above patch, make, then execute the following test
case:

rm -f foo
echo foo > foo
chmod 0400 foo
./src/emacs -Q

Define the following advice on `write-region' to simulate a write error:

------------------------- snip -------------------------
(advice-add 'write-region :override
	    (lambda (&rest _) (error "No space left on device")))
------------------------- snip -------------------------

Then continue

C-x C-f foo RET
C-x C-q
bar
C-0 C-x C-s
=> File foo is write-protected; try to save anyway?
yes RET
=> No space left on device

Now check the permissions of file foo:

  [emacs-master]$ ls -al foo
  -rw------- 1 jschmidt jschmidt 7 Oct 14 20:33 foo

So the write permissions did not get removed again.

Here the root cause is that the `unwind-protect' around the
`write-region' in `basic-save-buffer-2' does not handle the no-backup
case separately.  I would extend the clean-up form of the
`unwind-protect' to distinguish these both cases and handle the
no-backup case appropriately, here by trying first to set back the
extended attributes, then the regular ones.

*** What to do and where?

I can provide patches for both issues.  Plus ERT tests.

I guess that would be in one changeset, since these are related.

Should I provide a separate patch fixing only issue A for emacs-29?  Or
is that issue not relevant for emacs-29, given that it went unnoticed
for 12 years?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> Cc: eli zaretskii <eliz@gnu.org>
> Date: Sat, 14 Oct 2023 21:09:53 +0200
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> Eli, sorry to disturb you with another edge-case bug, but the commit
> that has introduced it was yours

It wasn't.

> *** Issue A
> 
> In a shell execute:
> 
> echo foo > foo
> chmod 0400 foo
> ./src/emacs -Q foo
> C-x C-q
> bar
> C-0 C-x C-s
> => File foo is write-protected; try to save anyway?
> yes RET
> => basic-save-buffer-2: Opening output file: Permission denied,
>    /home/jschmidt/work/emacs-master/foo
> 
> When saving *with* backup, that is, without prefix C-0, everything works
> as expected: The file temporarily gets write permissions, gets written
> to, and write permissions get removed again.

Please explain what happens with "C-0 C-x C-s", and why.  I don't
think I understand that, given what you wrote.

> The root cause is related to Eli's introduction of calls to
> `set-file-extended-attributes' in ccad023bc3c7.

ccad023bc3c7 didn't introduce set-file-extended-attributes, it wrapped
it with with-demoted-errors, and made the code fall back on the
traditional set-file-modes when the former call fails.

> The marked call [1] is a no-op, since it does not change the permissions
> in any way when writing them back, while the call [2] to
> `set-file-modes' actually changes the file mode.

set-file-modes is supposed to be called only if
set-file-extended-attributes fails, unless my reading of the code is
incorrect.  So again, I don't follow.

> Since an extended file attribute Elisp object cannot be modified yet (is
> that so?) to mark it, for example, as writable,

What do you mean by that?

> I propose to just drop that call to `set-file-extended-attributes',
> like this:

Why does it make sense to ignore the extended attributes here, when we
don't ignore them elsewhere in Emacs?

> rm -f foo
> echo foo > foo
> chmod 0400 foo
> ./src/emacs -Q
> 
> Define the following advice on `write-region' to simulate a write error:
> 
> ------------------------- snip -------------------------
> (advice-add 'write-region :override
> 	    (lambda (&rest _) (error "No space left on device")))
> ------------------------- snip -------------------------
> 
> Then continue
> 
> C-x C-f foo RET
> C-x C-q
> bar
> C-0 C-x C-s
> => File foo is write-protected; try to save anyway?
> yes RET
> => No space left on device
> 
> Now check the permissions of file foo:
> 
>   [emacs-master]$ ls -al foo
>   -rw------- 1 jschmidt jschmidt 7 Oct 14 20:33 foo
> 
> So the write permissions did not get removed again.
> 
> Here the root cause is that the `unwind-protect' around the
> `write-region' in `basic-save-buffer-2' does not handle the no-backup
> case separately.  I would extend the clean-up form of the
> `unwind-protect' to distinguish these both cases and handle the
> no-backup case appropriately, here by trying first to set back the
> extended attributes, then the regular ones.

Fine with me.

> I can provide patches for both issues.  Plus ERT tests.
> 
> I guess that would be in one changeset, since these are related.

Let's revisit this after we have a better understanding of the first
issue.  I'm not there yet.

> Should I provide a separate patch fixing only issue A for emacs-29?  Or
> is that issue not relevant for emacs-29, given that it went unnoticed
> for 12 years?

We will not fix this on emacs-29, no matter what.  We have lived with
these issues for many years, we can live with them for some time
longer.  So the patches should be for the master branch.

Thanks.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Eli, sorry to disturb you with another edge-case bug, but the commit
>> that has introduced it was yours
>
> It wasn't.

Sorry, I misread the annotation.  Thanks for looking into this, anyway.

>> *** Issue A
>>
>> In a shell execute:
>>
>> echo foo > foo
>> chmod 0400 foo
>> ./src/emacs -Q foo
>> C-x C-q
>> bar
>> C-0 C-x C-s
>> => File foo is write-protected; try to save anyway?
>> yes RET
>> => basic-save-buffer-2: Opening output file: Permission denied,
>>    /home/jschmidt/work/emacs-master/foo
>>
>> When saving *with* backup, that is, without prefix C-0, everything works
>> as expected: The file temporarily gets write permissions, gets written
>> to, and write permissions get removed again.
>
> Please explain what happens with "C-0 C-x C-s", and why.  I don't
> think I understand that, given what you wrote.

"C-0 C-x C-s" prompts me whether to write the modified buffer to the
write-protected file.  When I reply "yes", I expect Emacs to do the
following:

1. Get the mode bits/extended attributes of "foo".
2. Remove the write-potection on "foo".
3. Write the buffer to "foo", not creating a backup file.
4. Restore the mode bits/extended attributes on "foo" to their previous
   state taken in step 1.

What happens instead:

Emacs errors out that it cannot write to the file, even though it has
the permissions and means to temporarily remove the write-protection on
it.

>> The marked call [1] is a no-op, since it does not change the permissions
>> in any way when writing them back, while the call [2] to
>> `set-file-modes' actually changes the file mode.
>
> set-file-modes is supposed to be called only if
> set-file-extended-attributes fails, unless my reading of the code is
> incorrect.  So again, I don't follow.

Please ignore that bit, since my wording used there is indeed not quite
clear.  What I really wanted to express is explained below, hopefully
clearer now.

>> Since an extended file attribute Elisp object cannot be modified yet (is
>> that so?) to mark it, for example, as writable,
>
> What do you mean by that?

I mean that in an ideal world, my fix for this issue would have looked
like this:

------------------------- snip -------------------------
diff --git a/lisp/files.el b/lisp/files.el
index e1421b403bf..47245c43bd5 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5948,8 +5948,9 @@ basic-save-buffer-2
               ;; set-file-modes.
               (unless
                   (with-demoted-errors "Error setting attributes: %s"
-                    (set-file-extended-attributes buffer-file-name
-                                                  (nth 1 setmodes)))
+                    (set-file-extended-attributes
+                      buffer-file-name
+                     (make-exattr-writable (nth 1 setmodes))))
                 (set-file-modes buffer-file-name
                                 (logior (car setmodes) 128)))))
        (let (success)
------------------------- snip -------------------------

where I invented a function `make-exattr-writable' that takes the
Elisp encapsulation of extended file attributes and modifies it such,
that when it is applied to a file again, the file is then writable.

Just as it happens with the traditional `set-file-modes' call through
the `(logior OLD-MODE 128)'.

>> I propose to just drop that call to `set-file-extended-attributes',
>> like this:
>
> Why does it make sense to ignore the extended attributes here, when we
> don't ignore them elsewhere in Emacs?

Because we do not have the equivalent to `(logior OLD-MODE 128)' for
extended attributes yet.  Or do we?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sat, 14 Oct 2023 22:31:36 +0200
> 
> > Please explain what happens with "C-0 C-x C-s", and why.  I don't
> > think I understand that, given what you wrote.
> 
> "C-0 C-x C-s" prompts me whether to write the modified buffer to the
> write-protected file.  When I reply "yes", I expect Emacs to do the
> following:
> 
> 1. Get the mode bits/extended attributes of "foo".
> 2. Remove the write-potection on "foo".
> 3. Write the buffer to "foo", not creating a backup file.
> 4. Restore the mode bits/extended attributes on "foo" to their previous
>    state taken in step 1.

So far, so good.

> What happens instead:
> 
> Emacs errors out that it cannot write to the file, even though it has
> the permissions and means to temporarily remove the write-protection on
> it.

Why cannot Emacs write to the file, and what do the extended
attributes have to do with that?

> >> I propose to just drop that call to `set-file-extended-attributes',
> >> like this:
> >
> > Why does it make sense to ignore the extended attributes here, when we
> > don't ignore them elsewhere in Emacs?
> 
> Because we do not have the equivalent to `(logior OLD-MODE 128)' for
> extended attributes yet.

I still don't follow you: why do we need that in this situation?  I
hope if you answer to me previous question above in enough detail, it
will also answer this question.  So far I don't understand why this
fails and what do extended attributes have to do with it.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Sat, 14 Oct 2023 22:31:36 +0200
>>
>> > Please explain what happens with "C-0 C-x C-s", and why.  I don't
>> > think I understand that, given what you wrote.
>>
>> "C-0 C-x C-s" prompts me whether to write the modified buffer to the
>> write-protected file.  When I reply "yes", I expect Emacs to do the
>> following:
>>
>> 1. Get the mode bits/extended attributes of "foo".
>> 2. Remove the write-potection on "foo".
>> 3. Write the buffer to "foo", not creating a backup file.
>> 4. Restore the mode bits/extended attributes on "foo" to their previous
>>    state taken in step 1.
>
> So far, so good.
>
>> What happens instead:
>>
>> Emacs errors out that it cannot write to the file, even though it has
>> the permissions and means to temporarily remove the write-protection on
>> it.
>
> Why cannot Emacs write to the file, and what do the extended
> attributes have to do with that?

Let's check the working Emacs 23 code first, when there were no extended
attributes:

  (setq setmodes (cons (file-modes buffer-file-name) buffer-file-name))
  (set-file-modes buffer-file-name (logior (car setmodes) 128))

In procedural pseudo-code and without the cons, this does:

  setmodes = file-modes (buffer-file-name);
  set-file-modes (buffer-file-name, setmodes | 128);

which implements step 2 above: It takes the existing mode bits, adds bit
0b10000000 (u+w), and sets the resulting mode bits on the file.  The
file gets writable.  The following call to `write-region' succeeds.


Now the Emacs 29 code.  We simplify that in the assumption, that the
extended attribute calls always succeed, and that we do not need the
regular calls:

  (setq setmodes (list (file-extended-attributes buffer-file-name)
                       buffer-file-name))
  (set-file-extended-attributes buffer-file-name
                                (nth 0 setmodes))

The pseudo-code here looks like this:

  setmodes = file-extended-attributes (buffer-file-name);
  set-file-extended-attributes (setmodes);

That is, the original extended attributes of the file get written back
to the file *unchanged*.  No write permission is added to the file.
Accordingly, the following call to `write-region' fails.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sun, 15 Oct 2023 11:34:17 +0200
> 
> Now the Emacs 29 code.  We simplify that in the assumption, that the
> extended attribute calls always succeed, and that we do not need the
> regular calls:
> 
>   (setq setmodes (list (file-extended-attributes buffer-file-name)
>                        buffer-file-name))
>   (set-file-extended-attributes buffer-file-name
>                                 (nth 0 setmodes))
> 
> The pseudo-code here looks like this:
> 
>   setmodes = file-extended-attributes (buffer-file-name);
>   set-file-extended-attributes (setmodes);

That's not what the code does, since it does call

   (set-file-modes buffer-file-name (logior (car setmodes) 128))

But I think I can guess what you wanted to say: you wanted to say that
set-file-modes is called _only_ if set-file-extended-attributes fails,
and it doesn't fail in this scenario.  Is that what you wanted to say?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> But I think I can guess what you wanted to say: you wanted to say that
> set-file-modes is called _only_ if set-file-extended-attributes fails,
> and it doesn't fail in this scenario.  Is that what you wanted to say?

Yes!  Thanks for guessing :-)

And I have been asking a related question implicitly in the previous
conversation, so here is it explicitly:

Using *only* the extended-attribute Elisp functions and objects, is
there currently a way to implement the equivalent of "chmod u+w FILE" in
Elisp?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sun, 15 Oct 2023 13:39:04 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But I think I can guess what you wanted to say: you wanted to say that
> > set-file-modes is called _only_ if set-file-extended-attributes fails,
> > and it doesn't fail in this scenario.  Is that what you wanted to say?
> 
> Yes!  Thanks for guessing :-)
> 
> And I have been asking a related question implicitly in the previous
> conversation, so here is it explicitly:

In that case, does the change below fix the original problem?

> Using *only* the extended-attribute Elisp functions and objects, is
> there currently a way to implement the equivalent of "chmod u+w FILE" in
> Elisp?

AFAIU, this question has no meaningful answer.  Extended attributes
are much more fine-grained than the "traditional" file mode bits; in
particular, they are incompatible with the "user" notion to fit the
"u" part of "u+w": the same file can be writable by a specific user or
group of users, and unwritable by others.  Even if you only limit
yourself to Posix extended attributes (and Emacs doesn't limit itself
to that), there's no good answer to your question.

diff --git a/lisp/files.el b/lisp/files.el
index e1421b4..adfe8bd 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5933,9 +5933,10 @@ basic-save-buffer-2
                            buffer-file-name)
                          t))
 	;; If file not writable, see if we can make it writable
-	;; temporarily while we write it.
-	;; But no need to do so if we have just backed it up
-	;; (setmodes is set) because that says we're superseding.
+	;; temporarily while we write it (its original modes will be
+	;; restored in 'basic-save-buffer').  But no need to do so if
+	;; we have just backed it up (setmodes is set) because that
+	;; says we're superseding.
 	(cond ((and tempsetmodes (not setmodes))
 	       ;; Change the mode back, after writing.
 	       (setq setmodes
@@ -5944,12 +5945,12 @@ basic-save-buffer-2
                                "Error getting extended attributes: %s"
 			     (file-extended-attributes buffer-file-name))
 			   buffer-file-name))
-	       ;; If set-file-extended-attributes fails, fall back on
-	       ;; set-file-modes.
-	       (unless
-		   (with-demoted-errors "Error setting attributes: %s"
-		     (set-file-extended-attributes buffer-file-name
-						   (nth 1 setmodes)))
+	       ;; If set-file-extended-attributes fails to make the
+	       ;; file writable, fall back on set-file-modes.
+	       (with-demoted-errors "Error setting attributes: %s"
+		 (set-file-extended-attributes buffer-file-name
+					       (nth 1 setmodes)))
+	       (unless (file-writable-p buffer-file-name)
 		 (set-file-modes buffer-file-name
 				 (logior (car setmodes) 128)))))
 	(let (success)

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Sun, 15 Oct 2023 13:39:04 +0200

>> Yes!  Thanks for guessing :-)
>>
>> And I have been asking a related question implicitly in the previous
>> conversation, so here is it explicitly:
>
> In that case, does the change below fix the original problem?

It does, thanks.

>> Using *only* the extended-attribute Elisp functions and objects, is
>> there currently a way to implement the equivalent of "chmod u+w FILE" in
>> Elisp?

> AFAIU, this question has no meaningful answer.  Extended attributes
> are much more fine-grained than the "traditional" file mode bits; in
> particular, they are incompatible with the "user" notion to fit the
> "u" part of "u+w": the same file can be writable by a specific user or
> group of users, and unwritable by others.  Even if you only limit
> yourself to Posix extended attributes (and Emacs doesn't limit itself
> to that), there's no good answer to your question.

Then pls let me ask a more general question.  Given a sequence

  (setq ext-attrs (file-extended-attributes file-name))
  (setq ext-attrs (funcall func ext-attrs))
  (set-file-extended-attributes file-name ext-attrs)

is there any Elisp function FUNC so that the extended attributes on
FILE-NAME as seen by the OS change by executing that sequence?

> +	       ;; If set-file-extended-attributes fails to make the
> +	       ;; file writable, fall back on set-file-modes.
> +	       (with-demoted-errors "Error setting attributes: %s"
> +		 (set-file-extended-attributes buffer-file-name
> +					       (nth 1 setmodes)))

How exactly could above call to `set-file-extended-attributes' *succeed*
to make the file writable?

> +	       (unless (file-writable-p buffer-file-name)

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sun, 15 Oct 2023 20:59:42 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > In that case, does the change below fix the original problem?
> 
> It does, thanks.

I've now installed that on the master branch.

> >> Using *only* the extended-attribute Elisp functions and objects, is
> >> there currently a way to implement the equivalent of "chmod u+w FILE" in
> >> Elisp?
> 
> > AFAIU, this question has no meaningful answer.  Extended attributes
> > are much more fine-grained than the "traditional" file mode bits; in
> > particular, they are incompatible with the "user" notion to fit the
> > "u" part of "u+w": the same file can be writable by a specific user or
> > group of users, and unwritable by others.  Even if you only limit
> > yourself to Posix extended attributes (and Emacs doesn't limit itself
> > to that), there's no good answer to your question.
> 
> Then pls let me ask a more general question.  Given a sequence
> 
>   (setq ext-attrs (file-extended-attributes file-name))
>   (setq ext-attrs (funcall func ext-attrs))
>   (set-file-extended-attributes file-name ext-attrs)
> 
> is there any Elisp function FUNC so that the extended attributes on
> FILE-NAME as seen by the OS change by executing that sequence?

The only way I know of is to edit the extended attributes, which are
returned as a string, then use those edited attributes.  But the
semantics, and therefore the editing, of those strings are
platform-dependent, and editing them requires intimate knowledge of
the semantics and which edits are valid.

> > +	       ;; If set-file-extended-attributes fails to make the
> > +	       ;; file writable, fall back on set-file-modes.
> > +	       (with-demoted-errors "Error setting attributes: %s"
> > +		 (set-file-extended-attributes buffer-file-name
> > +					       (nth 1 setmodes)))
> 
> How exactly could above call to `set-file-extended-attributes' *succeed*
> to make the file writable?

I don't know, and I don't think we should care.  Due to the
above-mentioned system-dependencies, Emacs generally treats extended
attributes as opaque objects, and only tries hard to preserve them
where expected.  So the above is our best effort to preserve the
attributes, which is why we call set-file-modes only if absolutely
necessary, since doing that in general affects the extended
attributes.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Sun, 15 Oct 2023 20:59:42 +0200
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > In that case, does the change below fix the original problem?
>>
>> It does, thanks.
>
> I've now installed that on the master branch.

Thanks.  Now that's out of the way, should I then work on what I have
called issue B in the initial message and on ERT tests for both issues?
Or do you still think there is more discussion required on these
beforehand?

>> > +	       ;; If set-file-extended-attributes fails to make the
>> > +	       ;; file writable, fall back on set-file-modes.
>> > +	       (with-demoted-errors "Error setting attributes: %s"
>> > +		 (set-file-extended-attributes buffer-file-name
>> > +					       (nth 1 setmodes)))
>>
>> How exactly could above call to `set-file-extended-attributes' *succeed*
>> to make the file writable?
>
> I don't know, and I don't think we should care.  Due to the
> above-mentioned system-dependencies, Emacs generally treats extended
> attributes as opaque objects, and only tries hard to preserve them
> where expected.  So the above is our best effort to preserve the
> attributes, which is why we call set-file-modes only if absolutely
> necessary, since doing that in general affects the extended
> attributes.

That explains (partially) what I did not understand in your solution.
But still, to me it seems that the two forms

  (setq setmodes
        (list (file-modes buffer-file-name)
              (with-demoted-errors
                  "Error getting extended attributes: %s"
                (file-extended-attributes buffer-file-name))
              buffer-file-name))
  (with-demoted-errors "Error setting attributes: %s"
    (set-file-extended-attributes buffer-file-name
                                  (nth 1 setmodes)))

, if limiting them to the extended attribute calls and if ignoring outer
context, could be simplified to

  (set-file-extended-attributes
   buffer-file-name
   (file-extended-attributes buffer-file-name))

(If not, why not?)

And wouldn't that be, in this context, just a no-op?

I fully understand that the extended attributes stored in `setmodes' are
required later to restore the attributes of the file after it has been
written to.  And in that context I understand why we call
`set-file-extended-attributes'.  But here not really, yet.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Mon, 16 Oct 2023 22:04:15 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I've now installed that on the master branch.
> 
> Thanks.  Now that's out of the way, should I then work on what I have
> called issue B in the initial message and on ERT tests for both issues?
> Or do you still think there is more discussion required on these
> beforehand?

I'd like to ask you to show the relevant code again and explain why it
doesn't do the job in that case.

> >> How exactly could above call to `set-file-extended-attributes' *succeed*
> >> to make the file writable?
> >
> > I don't know, and I don't think we should care.  Due to the
> > above-mentioned system-dependencies, Emacs generally treats extended
> > attributes as opaque objects, and only tries hard to preserve them
> > where expected.  So the above is our best effort to preserve the
> > attributes, which is why we call set-file-modes only if absolutely
> > necessary, since doing that in general affects the extended
> > attributes.
> 
> That explains (partially) what I did not understand in your solution.
> But still, to me it seems that the two forms
> 
>   (setq setmodes
>         (list (file-modes buffer-file-name)
>               (with-demoted-errors
>                   "Error getting extended attributes: %s"
>                 (file-extended-attributes buffer-file-name))
>               buffer-file-name))
>   (with-demoted-errors "Error setting attributes: %s"
>     (set-file-extended-attributes buffer-file-name
>                                   (nth 1 setmodes)))
> 
> , if limiting them to the extended attribute calls and if ignoring outer
> context, could be simplified to
> 
>   (set-file-extended-attributes
>    buffer-file-name
>    (file-extended-attributes buffer-file-name))
> 
> (If not, why not?)

This is the kind of questions whose answers are in the discussions
that led to the wrapping the calls in with-demoted-errors.  If you
examine the Git history of this code and read the relevant discussions
to which it points, you will realize that the answer is: calling
primitives that access and set extended attributes sometimes fails due
to various subtle issues with those extended attributes, and we don't
want to signal errors in those cases when these primitives are called
from important commands such as the one which saves a buffer to its
visited file, if we can still do the job of saving.

> And wouldn't that be, in this context, just a no-op?

Which part of the above would be a no-op?

> I fully understand that the extended attributes stored in `setmodes' are
> required later to restore the attributes of the file after it has been
> written to.  And in that context I understand why we call
> `set-file-extended-attributes'.  But here not really, yet.

Well, then let me turn the table and ask you: why do you think we need
to restore the extended attributes later? what is the purpose of doing
that?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Mon, 16 Oct 2023 22:04:15 +0200
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > I've now installed that on the master branch.
>>
>> Thanks.  Now that's out of the way, should I then work on what I have
>> called issue B in the initial message and on ERT tests for both issues?
>> Or do you still think there is more discussion required on these
>> beforehand?
>
> I'd like to ask you to show the relevant code again and explain why it
> doesn't do the job in that case.

Let's postpone that for the time being.  I feel that there is still a
basic misunderstanding here.


Do we agree that this bug is all about the "no-backup" case (*C-0* C-x
C-s)?

For me that means: I want to save to file "foo", and I explicitly do not
want Emacs to create or touch a backup file "foo~" for that.

As a consequence, during the whole operation, there is only _one_ file
being involved, and not some second one, both as far as Emacs and the
operating system are concerned.


If I were to write a function replacing `basic-save-buffer-2' just for
that special "no-backup" case, then this way:

  (defun basic-save-buffer-2-no-backup ()
    (interactive)
    ;; ... user confirmation elided here ...
    (setq setmodes (list (file-modes buffer-file-name)
                         (file-extended-attributes buffer-file-name)
                         buffer-file-name))
    ;; No need to call `set-file-extended-attributes' here, since
    ;; we only have one file, and we just got the extended
    ;; attributes from that file.
    (set-file-modes buffer-file-name
                    (logior (car setmodes) 128))
    (let (success)
      (unwind-protect
          (progn
            (write-region nil nil
                          buffer-file-name nil t buffer-file-truename)
            (setq success t))
        (and setmodes (not success)
             (progn
               ;; No sense in calling `rename-file' here as done
               ;; in `basic-save-buffer-2', since we only have
               ;; one file.
               (set-file-extended-attributes buffer-file-name
                                             (nth 1 setmodes))
               (setq buffer-backed-up nil)))))
    setmodes)


>> And wouldn't that be, in this context, just a no-op?
>
> Which part of the above would be a no-op?

Exactly that:

  (set-file-extended-attributes
   buffer-file-name
   (file-extended-attributes buffer-file-name))

We set the extended file attributes on the same file
(`buffer-file-name') where we just got them from (`buffer-file-name').


>> I fully understand that the extended attributes stored in `setmodes' are
>> required later to restore the attributes of the file after it has been
>> written to.  And in that context I understand why we call
>> `set-file-extended-attributes'.  But here not really, yet.
>
> Well, then let me turn the table and ask you: why do you think we need
> to restore the extended attributes later? what is the purpose of doing
> that?

To restore them after we (possibly) have made the file writable.  Which
we need to do a) when the call to `write-region' has failed (then in
function `basic-save-buffer-2' itself), but also b) when the call has
succeeded (then further up the stack in `basic-save-buffer').


Thanks for your patience!

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Tue, 17 Oct 2023 22:12:58 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Do we agree that this bug is all about the "no-backup" case (*C-0* C-x
> C-s)?

The bug is, but basic-save-buffer-2 isn't.

> For me that means: I want to save to file "foo", and I explicitly do not
> want Emacs to create or touch a backup file "foo~" for that.

While true, I'm not sure what does this have to do with the issue we
are discussing.

> As a consequence, during the whole operation, there is only _one_ file
> being involved, and not some second one, both as far as Emacs and the
> operating system are concerned.

Only if this condition in basic-save-buffer-2 is NOT true:

    (let* ((dir (file-name-directory buffer-file-name))
           (dir-writable (file-writable-p dir)))
      (if (or (and file-precious-flag dir-writable)
              (and break-hardlink-on-save
                   (file-exists-p buffer-file-name)
                   (> (file-nlinks buffer-file-name) 1)
                   (or dir-writable
                       (error (concat "Directory %s write-protected; "
                                      "cannot break hardlink when saving")
                              dir))))

Again, I'm not sure why is this relevant.

> If I were to write a function replacing `basic-save-buffer-2' just for
> that special "no-backup" case, then this way:
> 
>   (defun basic-save-buffer-2-no-backup ()
>     (interactive)
>     ;; ... user confirmation elided here ...
>     (setq setmodes (list (file-modes buffer-file-name)
>                          (file-extended-attributes buffer-file-name)
>                          buffer-file-name))
>     ;; No need to call `set-file-extended-attributes' here, since
>     ;; we only have one file, and we just got the extended
>     ;; attributes from that file.
>     (set-file-modes buffer-file-name
>                     (logior (car setmodes) 128))
>     (let (success)
>       (unwind-protect
>           (progn
>             (write-region nil nil
>                           buffer-file-name nil t buffer-file-truename)
>             (setq success t))
>         (and setmodes (not success)
>              (progn
>                ;; No sense in calling `rename-file' here as done
>                ;; in `basic-save-buffer-2', since we only have
>                ;; one file.
>                (set-file-extended-attributes buffer-file-name
>                                              (nth 1 setmodes))
>                (setq buffer-backed-up nil)))))
>     setmodes)
> 
> 
> >> And wouldn't that be, in this context, just a no-op?
> >
> > Which part of the above would be a no-op?
> 
> Exactly that:
> 
>   (set-file-extended-attributes
>    buffer-file-name
>    (file-extended-attributes buffer-file-name))
> 
> We set the extended file attributes on the same file
> (`buffer-file-name') where we just got them from (`buffer-file-name').

I think you have an inaccurate mental model of what
set-file-extended-attributes does.  In particular, you seem to think
that

   (set-file-extended-attributes
    buffer-file-name
    (file-extended-attributes buffer-file-name))

leaves the file with the same unchanged extended attributes, as if it
were a set-file-modes call.  But that is not true in general,
especially if the original owner of the file was not the same user as
the one who runs the Emacs session that makes these calls.  Depending
on the OS, the actual privileges of the user running Emacs, and the
file-access setup of the underlying system, the user might not be
allowed to set some of the attributes, or might become the owner of
the file, or the OS could add some extended attributes to the original
ones.  So the above is not necessarily a no-op, although in simple
cases it probably is.

> >> I fully understand that the extended attributes stored in `setmodes' are
> >> required later to restore the attributes of the file after it has been
> >> written to.  And in that context I understand why we call
> >> `set-file-extended-attributes'.  But here not really, yet.
> >
> > Well, then let me turn the table and ask you: why do you think we need
> > to restore the extended attributes later? what is the purpose of doing
> > that?
> 
> To restore them after we (possibly) have made the file writable.  Which
> we need to do a) when the call to `write-region' has failed (then in
> function `basic-save-buffer-2' itself), but also b) when the call has
> succeeded (then further up the stack in `basic-save-buffer').

As I tried to explain above, this is not the case.  We set the
extended attributes so that the access rights to the file would be as
close as possible to the original ones, as much as the underlying
filesystem and the user privileges allow.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Tue, 17 Oct 2023 22:12:58 +0200
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> Do we agree that this bug is all about the "no-backup" case (*C-0* C-x
>> C-s)?
>
> The bug is, but basic-save-buffer-2 isn't.
>
>> For me that means: I want to save to file "foo", and I explicitly do not
>> want Emacs to create or touch a backup file "foo~" for that.
>
> While true, I'm not sure what does this have to do with the issue we
> are discussing.
>
>> As a consequence, during the whole operation, there is only _one_ file
>> being involved, and not some second one, both as far as Emacs and the
>> operating system are concerned.
>
> Only if this condition in basic-save-buffer-2 is NOT true:
>
> [...]
>
> Again, I'm not sure why is this relevant.

All of the above is relevant, at least for me, and I'm glad that we
agree up to your corrections, which make sense.

> I think you have an inaccurate mental model of what
> set-file-extended-attributes does.  In particular, you seem to think
> that
>
>    (set-file-extended-attributes
>     buffer-file-name
>     (file-extended-attributes buffer-file-name))
>
> leaves the file with the same unchanged extended attributes, as if it
> were a set-file-modes call.  But that is not true in general,
> especially if the original owner of the file was not the same user as
> the one who runs the Emacs session that makes these calls.  Depending
> on the OS, the actual privileges of the user running Emacs, and the
> file-access setup of the underlying system, the user might not be
> allowed to set some of the attributes, or might become the owner of
> the file, or the OS could add some extended attributes to the original
> ones.  So the above is not necessarily a no-op, although in simple
> cases it probably is.

I see, thanks.  The one thing I still do not understand is, however:

Why exactly do we need that

  (set-file-extended-attributes
   buffer-file-name
   (file-extended-attributes buffer-file-name))

incantation or the equivalent one

  (setq setmodes
        (list (file-modes buffer-file-name)
              (with-demoted-errors
                  "Error getting extended attributes: %s"
                (file-extended-attributes buffer-file-name))
              buffer-file-name))
  (with-demoted-errors "Error setting attributes: %s"
    (set-file-extended-attributes buffer-file-name
                                  (nth 1 setmodes)))

in function `basic-save-buffer-2'?

That doesn't seem to be a common pattern.  There are a few hits for
`set-file-extended-attributes' in the Emacs sources, but they all
attempt to transfer (as far as the OS allows) attributes from one file
to a different one.

In addition, I tried to trace back (this time hopefully correctly) the
origin of this call to `set-file-extended-attributes' and reached this
commit:

  574c05e21947 Karel Klíc - Add SELinux support.

Karel added as comment on this function:

  (basic-save-buffer-2): Set SELinux context of the newly created file,
  and return it.

But we do not have a "newly created file" here, and, in my opinion, do
not need to set an SELinux context/extended attributes on it.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Wed, 18 Oct 2023 22:36:04 +0200
> 
> Why exactly do we need that
> 
>   (set-file-extended-attributes
>    buffer-file-name
>    (file-extended-attributes buffer-file-name))
> 
> incantation or the equivalent one
> 
>   (setq setmodes
>         (list (file-modes buffer-file-name)
>               (with-demoted-errors
>                   "Error getting extended attributes: %s"
>                 (file-extended-attributes buffer-file-name))
>               buffer-file-name))
>   (with-demoted-errors "Error setting attributes: %s"
>     (set-file-extended-attributes buffer-file-name
>                                   (nth 1 setmodes)))
> 
> in function `basic-save-buffer-2'?

Because it was added there long ago, and because we have no reason to
believe it does any harm.

> In addition, I tried to trace back (this time hopefully correctly) the
> origin of this call to `set-file-extended-attributes' and reached this
> commit:
> 
>   574c05e21947 Karel Klíc - Add SELinux support.
> 
> Karel added as comment on this function:
> 
>   (basic-save-buffer-2): Set SELinux context of the newly created file,
>   and return it.
> 
> But we do not have a "newly created file" here, and, in my opinion, do
> not need to set an SELinux context/extended attributes on it.

When working with Emacs, we usually respect old code unless we have a
clear-cut case where it is wrong.  Here, we don't.  Me, I don't even
know enough about SELinux to discuss this intelligently.  So let's
leave the code as it is, until we have a good reason to change it.
(And if and when we do decide to change it, we will need a good
understanding of what this will do to the several ACL frameworks we
currently support, before we know how to change it without introducing
subtle issues.)

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 6530 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Wed, 18 Oct 2023 22:36:04 +0200
>>
>> Why exactly do we need that
>>
>>   (set-file-extended-attributes
>>    buffer-file-name
>>    (file-extended-attributes buffer-file-name))
>>
>> incantation or the equivalent one
>>
>>   (setq setmodes
>>         (list (file-modes buffer-file-name)
>>               (with-demoted-errors
>>                   "Error getting extended attributes: %s"
>>                 (file-extended-attributes buffer-file-name))
>>               buffer-file-name))
>>   (with-demoted-errors "Error setting attributes: %s"
>>     (set-file-extended-attributes buffer-file-name
>>                                   (nth 1 setmodes)))
>>
>> in function `basic-save-buffer-2'?
>
> Because it was added there long ago, and because we have no reason to
> believe it does any harm.

I see.  The only drawback to such harmless but potentially confusing
code that I see is that it attracts future questions and protracted
discussions like this one.  Of course, one could check the commit
(hopefully the right one), find this bug, and read it until your message
above.

But probably it would be helpful to shortcut that look-up process, like
this:

@@ -5946,7 +5949,9 @@ basic-save-buffer-2
                             (file-extended-attributes buffer-file-name))
                           buffer-file-name))
               ;; If set-file-extended-attributes fails to make the
-              ;; file writable, fall back on set-file-modes.
+              ;; file writable, fall back on set-file-modes.  Calling
+              ;; set-file-extended-attributes here may or may not be
+              ;; actually necessary, for details see Bug#66546.
               (with-demoted-errors "Error setting attributes: %s"
                 (set-file-extended-attributes buffer-file-name
                                               (nth 1 setmodes)))



Anyway, there is still issue B left.  Since you have fixed issue A
already, the reproducer has slightly changed:

rm -f foo
echo foo > foo
chmod 0400 foo
./src/emacs -Q

Define the following advice on `write-region' to simulate a write error
during buffer save:

------------------------- snip -------------------------
(advice-add 'write-region :override
	    (lambda (&rest _) (error "No space left on device")))
------------------------- snip -------------------------

Then continue

C-x C-f foo RET
C-x C-q
bar
C-0 C-x C-s
=> File foo is write-protected; try to save anyway?
yes RET
=> No space left on device

So far everything is as expected: The buffer is in status "unsaved", the
file unchanged.


Now check the permissions of file foo:

  [emacs-master]$ ls -al foo
  -rw------- 1 jschmidt jschmidt 7 Oct 14 20:33 foo

So the temporary write permissions of file foo did not get removed
again.  Which they should have been, in my opinion.


Here comes my analysis, hopefully more explicit and detailed than for
the first issue.  I'm aware that this bug is *really* old, but I think
this scenario, though rarely met, should be handled properly as well.
At least I'm not attempting to remove old code ...


The issue is located again in function `basic-save-buffer-2'.  There it
is limited to the "else" branch of the following `if':

  (if (or (and file-precious-flag dir-writable)
          (and break-hardlink-on-save ...))

so I'll focus only on that "else" branch plus the code before that `if'.
Let's call the file being saved TARGET-FILE.


In the focused part of the function, variable `setmodes' gets set to

A) the result of function `(backup-buffer)':

     The value is non-nil after a backup was made by renaming.
     It has the form (MODES EXTENDED-ATTRIBUTES BACKUPNAME).

B) or an analogous triple (MODES EXTENDED-ATTRIBUTES TARGET-FILE) if
   TARGET-FILE is not writable and no backup should be created:

     (setq setmodes
           (list (file-modes buffer-file-name)
                 (with-demoted-errors
                     "Error getting extended attributes: %s"
                   (file-extended-attributes buffer-file-name))
                 buffer-file-name))

   In this case, TARGET-FILE is made writable after the triple has been
   assigned to `setmodes'.


In any case, if non-nil, the resulting value of `setmodes' is intended
to restore the state (extended attributes and mode) of TARGET-FILE as
good as the OS permits after the save operation.  This holds both for
successful and unsuccessful ("no space left on device") save operations.
However, the handling of successful and unsuccessful operations differs:

- After a successful save and if `setmodes' is non-nil, the state
  recorded in `setmodes' should be applied to TARGET-FILE, which in case
  A) has been newly created or in case B) has been overwritten with new
  contents.

- After an unsuccessful save and if `setmodes' is non-nil, in case A)
  the backup file of TARGET-FILE should be renamed to TARGET-FILE.  In
  case B), however, I think that the original permissions of TARGET-FILE
  should be restored.  As far as the OS permits.


The save operation is implemented by the call to function `write-region'
in the following `unwind-protect':

  (unwind-protect
      (progn
        ;; Pass in nil&nil rather than point-min&max to indicate
        ;; we're saving the buffer rather than just a region.
        ;; write-region-annotate-functions may make use of it.
        (write-region nil nil
                      buffer-file-name nil t buffer-file-truename)
        (when save-silently (message nil))
        (setq success t))
    ;; If we get an error writing the new file, and we made
    ;; the backup by renaming, undo the backing-up.
    (and setmodes (not success)
         (progn
           (rename-file (nth 2 setmodes) buffer-file-name t)
           (setq buffer-backed-up nil))))

The problem here is that the UNWINDFORMS do not distinguish whether the
value of `setmodes' is non-nil because a backup file has been created
(case A above) or because TARGET-FILE has been made temporarily writable
(case B above).  As a result, in case B) function `rename-file' is
called with both arguments FILE and NEWNAME equalling TARGET-FILE.
While this is not the bug per se, it does at least not restore the
permissions on TARGET-FILE.


My patch fixes this issue by distinguishing cases A) and B) in the
UNWDINFORMS.  Please review.  I also attached a slightly unrelated,
minor doc fix I came across when working on this bug.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-argument-name-for-function-copy-file.patch --]
[-- Type: text/x-diff, Size: 1262 bytes --]

From 4e959868b6fecd9399454a81877f151dfca218ac Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Wed, 18 Oct 2023 22:43:37 +0200
Subject: [PATCH 1/2] ; Fix argument name for function copy-file

* doc/lispref/files.texi (Changing Files): Change name of last
argument of function `copy-file' from `preserve-extended-attributes'
to `preserve-permissions', as used in the function's description and
doc string.  (Bug#66546)
---
 doc/lispref/files.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index afedf776c86..dc66ea8bc9c 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1803,7 +1803,7 @@ Changing Files
 @var{oldname} is a directory and a non-directory otherwise.
 @end deffn
 
-@deffn Command copy-file oldname newname &optional ok-if-already-exists time preserve-uid-gid preserve-extended-attributes
+@deffn Command copy-file oldname newname &optional ok-if-already-exists time preserve-uid-gid preserve-permissions
 This command copies the file @var{oldname} to @var{newname}.  An
 error is signaled if @var{oldname} is not a regular file.  If @var{newname}
 names a directory, it copies @var{oldname} into that directory,
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Better-handle-errors-when-writing-r-o-files-without-.patch --]
[-- Type: text/x-diff, Size: 2703 bytes --]

From 223284c964164cd5e07dbbfb763925922fbcb598 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 19 Oct 2023 23:00:32 +0200
Subject: [PATCH 2/2] Better handle errors when writing r-o files without
 backup

* lisp/files.el (basic-save-buffer-2): Restore file permissions when
writing read-only files without backup fails.  (Bug#66546)
---
 lisp/files.el | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index adfe8bd44b9..9fc2f5f376a 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5934,10 +5934,13 @@ basic-save-buffer-2
                          t))
 	;; If file not writable, see if we can make it writable
 	;; temporarily while we write it (its original modes will be
-	;; restored in 'basic-save-buffer').  But no need to do so if
-	;; we have just backed it up (setmodes is set) because that
-	;; says we're superseding.
+	;; restored in 'basic-save-buffer' or, in case of an error, in
+	;; the `unwind-protect' below).  But no need to do so if we
+	;; have just backed it up (setmodes is set) because that says
+	;; we're superseding.
 	(cond ((and tempsetmodes (not setmodes))
+               ;; Remember we made the file writable.
+               (setq tempsetmodes 'u+w)
 	       ;; Change the mode back, after writing.
 	       (setq setmodes
                      (list (file-modes buffer-file-name)
@@ -5963,12 +5966,23 @@ basic-save-buffer-2
                               buffer-file-name nil t buffer-file-truename)
                 (when save-silently (message nil))
 		(setq success t))
-	    ;; If we get an error writing the new file, and we made
-	    ;; the backup by renaming, undo the backing-up.
-	    (and setmodes (not success)
-		 (progn
-		   (rename-file (nth 2 setmodes) buffer-file-name t)
-		   (setq buffer-backed-up nil)))))))
+            (cond
+             ;; If we get an error writing the file which we
+             ;; previously made writable, attempt to undo the
+             ;; write-access.
+             ((and (eq tempsetmodes 'u+w) (not success))
+              (condition-case ()
+		  (unless
+		      (with-demoted-errors "Error setting file modes: %S"
+			(set-file-modes buffer-file-name (car setmodes)))
+		    (set-file-extended-attributes buffer-file-name
+						  (nth 1 setmodes)))
+		(error nil)))
+	     ;; If we get an error writing the new file, and we made
+	     ;; the backup by renaming, undo the backing-up.
+	     ((and setmodes (not success))
+	      (rename-file (nth 2 setmodes) buffer-file-name t)
+	      (setq buffer-backed-up nil)))))))
     setmodes))
 
 (declare-function diff-no-select "diff"
-- 
2.30.2

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Thu, 19 Oct 2023 23:12:59 +0200
> 
> But probably it would be helpful to shortcut that look-up process, like
> this:
> 
> @@ -5946,7 +5949,9 @@ basic-save-buffer-2
>                              (file-extended-attributes buffer-file-name))
>                            buffer-file-name))
>                ;; If set-file-extended-attributes fails to make the
> -              ;; file writable, fall back on set-file-modes.
> +              ;; file writable, fall back on set-file-modes.  Calling
> +              ;; set-file-extended-attributes here may or may not be
> +              ;; actually necessary, for details see Bug#66546.
>                (with-demoted-errors "Error setting attributes: %s"
>                  (set-file-extended-attributes buffer-file-name
>                                                (nth 1 setmodes)))

I don't object, though a more detailed explanation (instead of sending
people to read the bug discussion) would be better.

> +             ;; If we get an error writing the file which we
> +             ;; previously made writable, attempt to undo the
> +             ;; write-access.
> +             ((and (eq tempsetmodes 'u+w) (not success))

Isn't it easier, safer, and more portable to compare buffer-file-name
with (nth 2 setmodes) instead?

> +              (condition-case ()
> +		  (unless
> +		      (with-demoted-errors "Error setting file modes: %S"
> +			(set-file-modes buffer-file-name (car setmodes)))
> +		    (set-file-extended-attributes buffer-file-name
> +						  (nth 1 setmodes)))
> +		(error nil)))

Why do we need condition-case here if we use with-demoted-errors?

> I also attached a slightly unrelated, minor doc fix I came across
> when working on this bug.

Thanks, installed on the emacs-29 branch (with minor changes in the
commit log message).

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1853 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Thu, 19 Oct 2023 23:12:59 +0200
>>
>> But probably it would be helpful to shortcut that look-up process, like
>> this:
>
> I don't object, though a more detailed explanation (instead of sending
> people to read the bug discussion) would be better.

I added the reasons why you wanted to keep the call to
`set-file-extended-attributes' in the attached patch.

>> +             ;; If we get an error writing the file which we
>> +             ;; previously made writable, attempt to undo the
>> +             ;; write-access.
>> +             ((and (eq tempsetmodes 'u+w) (not success))
>
> Isn't it easier, safer, and more portable to compare buffer-file-name
> with (nth 2 setmodes) instead?

I wanted to make 100% sure that we execute that first cond-branch if and
only if we previously changed the file mode.  IOW, I feel I cannot
exclude that by some strange configuration

  (equal buffer-file-name (nth 2 setmodes))

could also be true in other cases.

I added more documentation and renamed the u+w symbol to something
looking less Unix-specific.

>> +              (condition-case ()
>> +		  (unless
>> +		      (with-demoted-errors "Error setting file modes: %S"
>> +			(set-file-modes buffer-file-name (car setmodes)))
>> +		    (set-file-extended-attributes buffer-file-name
>> +						  (nth 1 setmodes)))
>> +		(error nil)))
>
> Why do we need condition-case here if we use with-demoted-errors?

We don't need it, I just copied that snippet from function
`basic-save-buffer' without thinking about it.  I changed that.

As mentioned earlier, I can also add unit tests for both issues.  If I
remember correctly, you would prefer these in the same changeset.

Please review the next version of the patch.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Better-handle-errors-when-writing-r-o-files-without-.patch --]
[-- Type: text/x-diff, Size: 4077 bytes --]

From 32c7d0858df1e5576a25ef64bb5ab971d1018c85 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 19 Oct 2023 23:00:32 +0200
Subject: [PATCH] Better handle errors when writing r-o files without backup

* lisp/files.el (basic-save-buffer-2): Restore file permissions when
writing read-only files without backup fails.  (Bug#66546)
---
 lisp/files.el | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index adfe8bd44b9..22c4f845a9b 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5856,6 +5856,10 @@ basic-save-buffer-1
 ;; This returns a value (MODES EXTENDED-ATTRIBUTES BACKUPNAME), like
 ;; backup-buffer.
 (defun basic-save-buffer-2 ()
+  ;; Variable `tempsetmodes' tracks temporary changes to the file
+  ;; modes: We set it to t if we (probably) need to make the file
+  ;; writable, and later to `reset-on-error' if we actually made the
+  ;; file writable.
   (let (tempsetmodes setmodes)
     (if (not (file-writable-p buffer-file-name))
 	(let ((dir (file-name-directory buffer-file-name)))
@@ -5934,10 +5938,14 @@ basic-save-buffer-2
                          t))
 	;; If file not writable, see if we can make it writable
 	;; temporarily while we write it (its original modes will be
-	;; restored in 'basic-save-buffer').  But no need to do so if
-	;; we have just backed it up (setmodes is set) because that
-	;; says we're superseding.
+	;; restored in 'basic-save-buffer' or, in case of an error, in
+	;; the `unwind-protect' below).  But no need to do so if we
+	;; have just backed it up (setmodes is set) because that says
+	;; we're superseding.
 	(cond ((and tempsetmodes (not setmodes))
+               ;; Remember we made the file writable so that we can
+               ;; try to undo that in case of errors.
+               (setq tempsetmodes 'reset-on-error)
 	       ;; Change the mode back, after writing.
 	       (setq setmodes
                      (list (file-modes buffer-file-name)
@@ -5946,7 +5954,12 @@ basic-save-buffer-2
 			     (file-extended-attributes buffer-file-name))
 			   buffer-file-name))
 	       ;; If set-file-extended-attributes fails to make the
-	       ;; file writable, fall back on set-file-modes.
+	       ;; file writable, fall back on set-file-modes.  Calling
+	       ;; set-file-extended-attributes here may or may not be
+	       ;; actually necessary.  However, since its exact
+	       ;; behavior is highly port-specific, since calling it
+	       ;; does not do any harm, and since the call has a long
+	       ;; history, we decided to leave it in (bug#66546).
 	       (with-demoted-errors "Error setting attributes: %s"
 		 (set-file-extended-attributes buffer-file-name
 					       (nth 1 setmodes)))
@@ -5963,12 +5976,21 @@ basic-save-buffer-2
                               buffer-file-name nil t buffer-file-truename)
                 (when save-silently (message nil))
 		(setq success t))
-	    ;; If we get an error writing the new file, and we made
-	    ;; the backup by renaming, undo the backing-up.
-	    (and setmodes (not success)
-		 (progn
-		   (rename-file (nth 2 setmodes) buffer-file-name t)
-		   (setq buffer-backed-up nil)))))))
+            (cond
+             ;; If we get an error writing the file which we
+             ;; previously made writable, attempt to undo the
+             ;; write-access.
+             ((and (eq tempsetmodes 'reset-on-error) (not success))
+	      (with-demoted-errors "Error setting file modes: %S"
+		(set-file-modes buffer-file-name (car setmodes)))
+	      (with-demoted-errors "Error setting attributes: %s"
+		(set-file-extended-attributes buffer-file-name
+					      (nth 1 setmodes))))
+	     ;; If we get an error writing the new file, and we made
+	     ;; the backup by renaming, undo the backing-up.
+	     ((and setmodes (not success))
+	      (rename-file (nth 2 setmodes) buffer-file-name t)
+	      (setq buffer-backed-up nil)))))))
     setmodes))
 
 (declare-function diff-no-select "diff"
-- 
2.30.2

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sat, 21 Oct 2023 19:56:44 +0200
> 
> >> +             ;; If we get an error writing the file which we
> >> +             ;; previously made writable, attempt to undo the
> >> +             ;; write-access.
> >> +             ((and (eq tempsetmodes 'u+w) (not success))
> >
> > Isn't it easier, safer, and more portable to compare buffer-file-name
> > with (nth 2 setmodes) instead?
> 
> I wanted to make 100% sure that we execute that first cond-branch if and
> only if we previously changed the file mode.  IOW, I feel I cannot
> exclude that by some strange configuration
> 
>   (equal buffer-file-name (nth 2 setmodes))
> 
> could also be true in other cases.

It doesn't matter which cases could cause this.  What matters is that
only when these two are identical that we need to make sure the file
ends up read-only as it was before the call to set-file-modes.

So please do make that change.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Sat, 21 Oct 2023 19:56:44 +0200

>> I wanted to make 100% sure that we execute that first cond-branch if and
>> only if we previously changed the file mode.  IOW, I feel I cannot
>> exclude that by some strange configuration
>> 
>>   (equal buffer-file-name (nth 2 setmodes))
>> 
>> could also be true in other cases.
>
> It doesn't matter which cases could cause this.  What matters is that
> only when these two are identical that we need to make sure the file
> ends up read-only as it was before the call to set-file-modes.

Makes sense, will do.  In that case, would a plain `equal' be enough to
compare file paths?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sat, 21 Oct 2023 22:17:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>   (equal buffer-file-name (nth 2 setmodes))
> >> 
> >> could also be true in other cases.
> >
> > It doesn't matter which cases could cause this.  What matters is that
> > only when these two are identical that we need to make sure the file
> > ends up read-only as it was before the call to set-file-modes.
> 
> Makes sense, will do.  In that case, would a plain `equal' be enough to
> compare file paths?

Yes, I think so.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Sat, 21 Oct 2023 22:17:18 +0200

>> Makes sense, will do.  In that case, would a plain `equal' be enough to
>> compare file paths?
>
> Yes, I think so.

Please review the next version, then.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Better-handle-errors-when-writing-r-o-files-without-.patch --]
[-- Type: text/x-diff, Size: 3398 bytes --]

From 249a53e340c5c6cd4c9b8c405e5a9c6074876585 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 19 Oct 2023 23:00:32 +0200
Subject: [PATCH] Better handle errors when writing r-o files without backup

* lisp/files.el (basic-save-buffer-2): Restore file permissions when
writing read-only files without backup fails.  (Bug#66546)
---
 lisp/files.el | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index adfe8bd44b9..1be5b374ae8 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5934,9 +5934,10 @@ basic-save-buffer-2
                          t))
 	;; If file not writable, see if we can make it writable
 	;; temporarily while we write it (its original modes will be
-	;; restored in 'basic-save-buffer').  But no need to do so if
-	;; we have just backed it up (setmodes is set) because that
-	;; says we're superseding.
+	;; restored in 'basic-save-buffer' or, in case of an error, in
+	;; the `unwind-protect' below).  But no need to do so if we
+	;; have just backed it up (setmodes is set) because that says
+	;; we're superseding.
 	(cond ((and tempsetmodes (not setmodes))
 	       ;; Change the mode back, after writing.
 	       (setq setmodes
@@ -5946,7 +5947,12 @@ basic-save-buffer-2
 			     (file-extended-attributes buffer-file-name))
 			   buffer-file-name))
 	       ;; If set-file-extended-attributes fails to make the
-	       ;; file writable, fall back on set-file-modes.
+	       ;; file writable, fall back on set-file-modes.  Calling
+	       ;; set-file-extended-attributes here may or may not be
+	       ;; actually necessary.  However, since its exact
+	       ;; behavior is highly port-specific, since calling it
+	       ;; does not do any harm, and since the call has a long
+	       ;; history, we decided to leave it in (bug#66546).
 	       (with-demoted-errors "Error setting attributes: %s"
 		 (set-file-extended-attributes buffer-file-name
 					       (nth 1 setmodes)))
@@ -5963,12 +5969,22 @@ basic-save-buffer-2
                               buffer-file-name nil t buffer-file-truename)
                 (when save-silently (message nil))
 		(setq success t))
-	    ;; If we get an error writing the new file, and we made
-	    ;; the backup by renaming, undo the backing-up.
-	    (and setmodes (not success)
-		 (progn
-		   (rename-file (nth 2 setmodes) buffer-file-name t)
-		   (setq buffer-backed-up nil)))))))
+            (cond
+             ;; If we get an error writing the file, and there is no
+             ;; backup file, then we (most likely) made that file
+             ;; writable above.  Attempt to undo the write-access.
+             ((and setmodes (not success)
+                   (equal (nth 2 setmodes) buffer-file-name))
+	      (with-demoted-errors "Error setting file modes: %S"
+		(set-file-modes buffer-file-name (car setmodes)))
+	      (with-demoted-errors "Error setting attributes: %s"
+		(set-file-extended-attributes buffer-file-name
+					      (nth 1 setmodes))))
+	     ;; If we get an error writing the new file, and we made
+	     ;; the backup by renaming, undo the backing-up.
+	     ((and setmodes (not success))
+	      (rename-file (nth 2 setmodes) buffer-file-name t)
+	      (setq buffer-backed-up nil)))))))
     setmodes))
 
 (declare-function diff-no-select "diff"
-- 
2.30.2

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sun, 22 Oct 2023 11:45:52 +0200
> 
> Please review the next version, then.

Thanks, installed on master, and closing the bug.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-10-25  14:58, Eli Zaretskii wrote:
>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>> Date: Sun, 22 Oct 2023 11:45:52 +0200
>>
>> Please review the next version, then.
> 
> Thanks, installed on master, and closing the bug.

Thanks for installing.

Would you accept a unit test for these issues?

If yes, should I reopen this bug or a new one to track that?

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> Date: Sun, 29 Oct 2023 15:23:26 +0100
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> 
> On 2023-10-25  14:58, Eli Zaretskii wrote:
> >> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> >> Cc: 66546@debbugs.gnu.org
> >> Date: Sun, 22 Oct 2023 11:45:52 +0200
> >>
> >> Please review the next version, then.
> > 
> > Thanks, installed on master, and closing the bug.
> 
> Thanks for installing.
> 
> Would you accept a unit test for these issues?

Sure, addition to the test suite are always welcome.

> If yes, should I reopen this bug or a new one to track that?

It doesn't matter.  Up to you.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

reopen 66546
thanks

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sun, 29 Oct 2023 15:23:26 +0100
>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66546@debbugs.gnu.org
>>
>> Thanks for installing.
>> 
>> Would you accept a unit test for these issues?
>
> Sure, addition to the test suite are always welcome.

Please review attached test.  I hope I haven't been overperforming.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-tests-for-writing-to-write-protected-files-with-.patch --]
[-- Type: text/x-diff, Size: 7057 bytes --]

From d8ed8ed8ff4e81b7bbee484a3dade45bc15967db Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Wed, 1 Nov 2023 19:56:06 +0100
Subject: [PATCH] ; Add tests for writing to write-protected files with
 save-buffer

* test/lisp/files-tests.el (files-tests--with-yes-or-no-p): Add new
macro.
(files-tests-save-buffer-read-only-file): Add new test to test writing
to write-protected files with `save-buffer'.
---
 test/lisp/files-tests.el | 144 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 63ce4dab7eb..50f41cedc9b 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1717,6 +1717,150 @@ files-tests--save-some-buffers
             (set-buffer-modified-p nil)
             (kill-buffer buf)))))))
 
+(defmacro files-tests--with-yes-or-no-p (reply &rest body)
+  "Execute BODY, providing replies to `yes-or-no-p' queries.
+REPLY should be a cons (PROMPT . VALUE), and during execution of
+BODY this macro provides VALUE as return value to all
+`yes-or-no-p' calls prompting for PROMPT and nil to all other
+`yes-or-no-p' calls.  After execution of BODY, this macro ensures
+that exactly one `yes-or-no-p' call prompting for PROMPT has been
+executed during execution of BODY."
+  (declare (indent 1) (debug (sexp body)))
+  `(cl-letf*
+       ((reply ,reply)
+        (prompts nil)
+        ((symbol-function 'yes-or-no-p)
+         (lambda (prompt)
+           (let ((reply (cdr (assoc prompt (list reply)))))
+             (push (cons prompt reply) prompts)
+             reply))))
+     ,@body
+     (should (equal prompts (list reply)))))
+
+(ert-deftest files-tests-save-buffer-read-only-file ()
+  "Test writing to write-protected files with `save-buffer'.
+Ensure that the issues from bug#66546 are fixed."
+  (ert-with-temp-directory dir
+    (cl-letf* (;; Define convenience functions.
+               ((symbol-function 'file-contents)
+                (lambda (file)
+                  (if (file-exists-p file)
+                      (condition-case err
+                          (with-temp-buffer
+                            (insert-file-contents file)
+                            (buffer-string))
+                        ((error err)))
+                    'missing)))
+               (signal-write-failed
+                (lambda (&rest _)
+                  (signal 'file-error "Write failed")))
+
+               ;; Sanitize environment.
+               (auto-save-default nil)
+               (backup-enable-predicate nil)
+               (before-save-hook nil)
+               (write-contents-functions nil)
+               (local-write-file-hooks nil)
+               (write-file-functions)
+               (after-save-hook nil)
+
+               ;; Set the name of the game.
+               (base "read-only-test")
+               (file (expand-file-name base dir))
+               (backup (make-backup-file-name file))
+
+               (override-read-only-prompt
+                (format "File %s is write-protected; try to save anyway? "
+                        base)))
+
+      ;; Ensure that set-file-modes renders our test file read-only,
+      ;; otherwise skip this test.  Use `file-writable-p' to test for
+      ;; read-only-ness, because that's what function `save-buffer'
+      ;; uses as well.
+      (with-temp-file file (insert "foo\n"))
+      (skip-unless (file-writable-p file))
+      (set-file-modes file (logand (file-modes file)
+                                   (lognot #o0222)))
+      (skip-unless (not (file-writable-p file)))
+
+      (with-current-buffer (find-file-noselect file)
+        ;; Prepare for tests backing up the file.
+        (setq buffer-read-only nil)
+        (goto-char (point-min))
+        (insert "bar\n")
+
+        ;; Save to read-only file with backup, declining prompt.
+        (files-tests--with-yes-or-no-p
+            (cons override-read-only-prompt nil)
+          (should-error
+           (save-buffer)
+           ;; "Attempt to save to a file that you aren't allowed to write"
+           :type 'error))
+        (should-not buffer-backed-up)
+        (should     (buffer-modified-p))
+        (should-not (file-writable-p file))
+        (should     (equal (file-contents file) "foo\n"))
+        (should     (equal (file-contents backup) 'missing))
+
+        ;; Save to read-only file with backup, accepting prompt,
+        ;; experiencing a write error.
+        (files-tests--with-yes-or-no-p
+            (cons override-read-only-prompt t)
+          (should-error
+           (cl-letf (((symbol-function 'write-region) signal-write-failed))
+             (save-buffer))
+           ;; "Write failed"
+           :type 'file-error))
+        (should-not buffer-backed-up)
+        (should     (buffer-modified-p))
+        (should-not (file-writable-p file))
+        (should     (equal (file-contents file) "foo\n"))
+        (should     (equal (file-contents backup) 'missing))
+
+        ;; Save to read-only file with backup, accepting prompt.
+        (files-tests--with-yes-or-no-p
+            (cons override-read-only-prompt t)
+          (save-buffer))
+        (should     buffer-backed-up)
+        (should-not (buffer-modified-p))
+        (should-not (file-writable-p file))
+        (should-not (file-writable-p backup))
+        (should     (equal (file-contents file) "bar\nfoo\n"))
+        (should     (equal (file-contents backup) "foo\n"))
+
+        ;; Prepare for tests not backing up the file.
+        (setq buffer-backed-up nil)
+        (delete-file backup)
+        (goto-char (point-min))
+        (insert "baz\n")
+
+        ;; Save to read-only file without backup, accepting prompt,
+        ;; experiencing a write error.  This tests that issue B of
+        ;; bug#66546 is fixed.
+        (files-tests--with-yes-or-no-p
+            (cons override-read-only-prompt t)
+          (should-error
+           (cl-letf (((symbol-function 'write-region) signal-write-failed))
+             (save-buffer 0))
+           ;; "Write failed"
+           :type 'file-error))
+        (should-not buffer-backed-up)
+        (should     (buffer-modified-p))
+        (should-not (file-writable-p file))
+        (should     (equal (file-contents file) "bar\nfoo\n"))
+        (should     (equal (file-contents backup) 'missing))
+
+        ;; Save to read-only file without backup, accepting prompt.
+        ;; This tests that issue A of bug#66546 is fixed.
+        (files-tests--with-yes-or-no-p
+            (cons override-read-only-prompt t)
+          (save-buffer 0))
+        (should-not buffer-backed-up)
+        (should-not (buffer-modified-p))
+        (should-not (file-writable-p file))
+        (should     (equal (file-contents file) "baz\nbar\nfoo\n"))
+        (should     (equal (file-contents backup) 'missing))))))
+
 (ert-deftest files-tests-save-some-buffers ()
   "Test `save-some-buffers'.
 Test the 3 cases for the second argument PRED, i.e., nil, t, or
-- 
2.30.2

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Wed, 01 Nov 2023 20:06:36 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Sure, addition to the test suite are always welcome.
> 
> Please review attached test.  I hope I haven't been overperforming.

Thanks, a few comments below:

> +    (cl-letf* (;; Define convenience functions.
> +               ((symbol-function 'file-contents)
> +                (lambda (file)
> +                  (if (file-exists-p file)
> +                      (condition-case err
> +                          (with-temp-buffer
> +                            (insert-file-contents file)
> +                            (buffer-string))
> +                        ((error err)))
> +                    'missing)))
> [...]
> +        (should     (equal (file-contents file) "foo\n"))

If you want to make sure the file's contents is _exactly_ some text,
you need to write the buffer text to the file with no encoding
conversions, and you need then to visit the file with
insert-file-contents-literally, to avoid decoding conversions.
Otherwise you might get false positives and false negatives due to
encoding/decoding of text and of EOLs.

Also, compiling the new test I get byte-compiler warnings:

  In toplevel form:
  lisp/files-tests.el:1748:39: Warning: Unused lexical variable `err'
  lisp/files-tests.el:1763:17: Warning: `local-write-file-hooks' is an obsolete variable (as of 22.1); use `write-file-functions' instead.

  In end of data:
  lisp/files-tests.el:1802:29: Warning: the function `file-contents' is not known to be defined.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, a few comments below:

Thanks for reviewing.

> If you want to make sure the file's contents is _exactly_ some text,
> you need to write the buffer text to the file with no encoding
> conversions, and you need then to visit the file with
> insert-file-contents-literally, to avoid decoding conversions.
> Otherwise you might get false positives and false negatives due to
> encoding/decoding of text and of EOLs.

Done: insert-file-contents-literally for reading, binding

  (coding-system-for-write 'no-conversion)

for writing.

> Also, compiling the new test I get byte-compiler warnings:

I obviously focused on the test results only ... all warnings fixed.

Please see the attached updated patch.

Thanks


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-tests-for-saving-to-write-protected-files.patch --]
[-- Type: text/x-diff, Size: 7403 bytes --]

From 9759cfbe48084708ab2475e92d54dc3b294e0ea2 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Wed, 1 Nov 2023 19:56:06 +0100
Subject: [PATCH] Add tests for saving to write-protected files

* test/lisp/files-tests.el (files-tests--with-yes-or-no-p): Add macro.
(files-tests-save-buffer-read-only-file): Add test for writing to
write-protected files with `save-buffer'.  (Bug#66546)
---
 test/lisp/files-tests.el | 147 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 63ce4dab7eb..d3a1a7cb667 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1717,6 +1717,153 @@ files-tests--save-some-buffers
             (set-buffer-modified-p nil)
             (kill-buffer buf)))))))
 
+(defmacro files-tests--with-yes-or-no-p (reply &rest body)
+  "Execute BODY, providing replies to `yes-or-no-p' queries.
+REPLY should be a cons (PROMPT . VALUE), and during execution of
+BODY this macro provides VALUE as return value to all
+`yes-or-no-p' calls prompting for PROMPT and nil to all other
+`yes-or-no-p' calls.  After execution of BODY, this macro ensures
+that exactly one `yes-or-no-p' call prompting for PROMPT has been
+executed during execution of BODY."
+  (declare (indent 1) (debug (sexp body)))
+  `(cl-letf*
+       ((reply ,reply)
+        (prompts nil)
+        ((symbol-function 'yes-or-no-p)
+         (lambda (prompt)
+           (let ((reply (cdr (assoc prompt (list reply)))))
+             (push (cons prompt reply) prompts)
+             reply))))
+     ,@body
+     (should (equal prompts (list reply)))))
+
+(ert-deftest files-tests-save-buffer-read-only-file ()
+  "Test writing to write-protected files with `save-buffer'.
+Ensure that the issues from bug#66546 are fixed."
+  (ert-with-temp-directory dir
+    (cl-flet (;; Define convenience functions.
+              (file-contents (file)
+                (if (file-exists-p file)
+                    (condition-case err
+                        (with-temp-buffer
+                          (insert-file-contents-literally file)
+                          (buffer-string))
+                      (error err))
+                  'missing))
+              (signal-write-failed (&rest _)
+                (signal 'file-error "Write failed")))
+
+      (let* (;; Sanitize environment.
+             (coding-system-for-write 'no-conversion)
+             (auto-save-default nil)
+             (backup-enable-predicate nil)
+             (before-save-hook nil)
+             (write-contents-functions nil)
+             (write-file-functions nil)
+             (after-save-hook nil)
+
+             ;; Set the name of the game.
+             (base "read-only-test")
+             (file (expand-file-name base dir))
+             (backup (make-backup-file-name file))
+
+             (override-read-only-prompt
+              (format "File %s is write-protected; try to save anyway? "
+                      base)))
+
+        ;; Ensure that set-file-modes renders our test file read-only,
+        ;; otherwise skip this test.  Use `file-writable-p' to test
+        ;; for read-only-ness, because that's what function
+        ;; `save-buffer' uses as well.
+        (with-temp-file file (insert "foo\n"))
+        (skip-unless (file-writable-p file))
+        (set-file-modes file (logand (file-modes file)
+                                     (lognot #o0222)))
+        (skip-unless (not (file-writable-p file)))
+
+        (with-current-buffer (find-file-noselect file)
+          ;; Prepare for tests backing up the file.
+          (setq buffer-read-only nil)
+          (goto-char (point-min))
+          (insert "bar\n")
+
+          ;; Save to read-only file with backup, declining prompt.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt nil)
+            (should-error
+             (save-buffer)
+             ;; "Attempt to save to a file that you aren't allowed to write"
+             :type 'error))
+          (should-not buffer-backed-up)
+          (should     (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "foo\n"))
+          (should     (equal (file-contents backup) 'missing))
+
+          ;; Save to read-only file with backup, accepting prompt,
+          ;; experiencing a write error.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (should-error
+             (cl-letf (((symbol-function 'write-region)
+                        #'signal-write-failed))
+               (save-buffer))
+             ;; "Write failed"
+             :type 'file-error))
+          (should-not buffer-backed-up)
+          (should     (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "foo\n"))
+          (should     (equal (file-contents backup) 'missing))
+
+          ;; Save to read-only file with backup, accepting prompt.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (save-buffer))
+          (should     buffer-backed-up)
+          (should-not (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should-not (file-writable-p backup))
+          (should     (equal (file-contents file) "bar\nfoo\n"))
+          (should     (equal (file-contents backup) "foo\n"))
+
+          ;; Prepare for tests not backing up the file.
+          (setq buffer-backed-up nil)
+          (delete-file backup)
+          (goto-char (point-min))
+          (insert "baz\n")
+
+          ;; Save to read-only file without backup, accepting prompt,
+          ;; experiencing a write error.  This tests that issue B of
+          ;; bug#66546 is fixed.  The results of the "with backup" and
+          ;; "without backup" subtests are identical when a write
+          ;; error occurs, but the code paths to reach these results
+          ;; are not.  In other words, this subtest is not redundant.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (should-error
+             (cl-letf (((symbol-function 'write-region)
+                        #'signal-write-failed))
+               (save-buffer 0))
+             ;; "Write failed"
+             :type 'file-error))
+          (should-not buffer-backed-up)
+          (should     (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "bar\nfoo\n"))
+          (should     (equal (file-contents backup) 'missing))
+
+          ;; Save to read-only file without backup, accepting prompt.
+          ;; This tests that issue A of bug#66546 is fixed.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (save-buffer 0))
+          (should-not buffer-backed-up)
+          (should-not (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "baz\nbar\nfoo\n"))
+          (should     (equal (file-contents backup) 'missing)))))))
+
 (ert-deftest files-tests-save-some-buffers ()
   "Test `save-some-buffers'.
 Test the 3 cases for the second argument PRED, i.e., nil, t, or
-- 
2.30.2

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Fri, 03 Nov 2023 22:02:03 +0100
> 
> > If you want to make sure the file's contents is _exactly_ some text,
> > you need to write the buffer text to the file with no encoding
> > conversions, and you need then to visit the file with
> > insert-file-contents-literally, to avoid decoding conversions.
> > Otherwise you might get false positives and false negatives due to
> > encoding/decoding of text and of EOLs.
> 
> Done: insert-file-contents-literally for reading, binding
> 
>   (coding-system-for-write 'no-conversion)
> 
> for writing.

I think one nit is still missing:

> +      (let* (;; Sanitize environment.
> +             (coding-system-for-write 'no-conversion)

I think you also need to bind coding-system-for-read to no-conversion,
since you later do this:

> +        (with-current-buffer (find-file-noselect file)
                                 ^^^^^^^^^^^^^^^^^^^^^^^
Otherwise, find-file-noselect will use insert-file-contents, which
will attempt to decode the text.

Thanks.

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 124 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> I think one nit is still missing:

Right you are.  Fixed in next version.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-tests-for-saving-to-write-protected-files.patch --]
[-- Type: text/x-diff, Size: 7457 bytes --]

From b9ed2833818f68d8e0b6fa71ded4599dce9d3247 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Wed, 1 Nov 2023 19:56:06 +0100
Subject: [PATCH] Add tests for saving to write-protected files

* test/lisp/files-tests.el (files-tests--with-yes-or-no-p): Add macro.
(files-tests-save-buffer-read-only-file): Add test for writing to
write-protected files with `save-buffer'.  (Bug#66546)
---
 test/lisp/files-tests.el | 148 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 148 insertions(+)

diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 63ce4dab7eb..77a4c22ed6a 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1717,6 +1717,154 @@ files-tests--save-some-buffers
             (set-buffer-modified-p nil)
             (kill-buffer buf)))))))
 
+(defmacro files-tests--with-yes-or-no-p (reply &rest body)
+  "Execute BODY, providing replies to `yes-or-no-p' queries.
+REPLY should be a cons (PROMPT . VALUE), and during execution of
+BODY this macro provides VALUE as return value to all
+`yes-or-no-p' calls prompting for PROMPT and nil to all other
+`yes-or-no-p' calls.  After execution of BODY, this macro ensures
+that exactly one `yes-or-no-p' call prompting for PROMPT has been
+executed during execution of BODY."
+  (declare (indent 1) (debug (sexp body)))
+  `(cl-letf*
+       ((reply ,reply)
+        (prompts nil)
+        ((symbol-function 'yes-or-no-p)
+         (lambda (prompt)
+           (let ((reply (cdr (assoc prompt (list reply)))))
+             (push (cons prompt reply) prompts)
+             reply))))
+     ,@body
+     (should (equal prompts (list reply)))))
+
+(ert-deftest files-tests-save-buffer-read-only-file ()
+  "Test writing to write-protected files with `save-buffer'.
+Ensure that the issues from bug#66546 are fixed."
+  (ert-with-temp-directory dir
+    (cl-flet (;; Define convenience functions.
+              (file-contents (file)
+                (if (file-exists-p file)
+                    (condition-case err
+                        (with-temp-buffer
+                          (insert-file-contents-literally file)
+                          (buffer-string))
+                      (error err))
+                  'missing))
+              (signal-write-failed (&rest _)
+                (signal 'file-error "Write failed")))
+
+      (let* (;; Sanitize environment.
+             (coding-system-for-read 'no-conversion)
+             (coding-system-for-write 'no-conversion)
+             (auto-save-default nil)
+             (backup-enable-predicate nil)
+             (before-save-hook nil)
+             (write-contents-functions nil)
+             (write-file-functions nil)
+             (after-save-hook nil)
+
+             ;; Set the name of the game.
+             (base "read-only-test")
+             (file (expand-file-name base dir))
+             (backup (make-backup-file-name file))
+
+             (override-read-only-prompt
+              (format "File %s is write-protected; try to save anyway? "
+                      base)))
+
+        ;; Ensure that set-file-modes renders our test file read-only,
+        ;; otherwise skip this test.  Use `file-writable-p' to test
+        ;; for read-only-ness, because that's what function
+        ;; `save-buffer' uses as well.
+        (with-temp-file file (insert "foo\n"))
+        (skip-unless (file-writable-p file))
+        (set-file-modes file (logand (file-modes file)
+                                     (lognot #o0222)))
+        (skip-unless (not (file-writable-p file)))
+
+        (with-current-buffer (find-file-noselect file)
+          ;; Prepare for tests backing up the file.
+          (setq buffer-read-only nil)
+          (goto-char (point-min))
+          (insert "bar\n")
+
+          ;; Save to read-only file with backup, declining prompt.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt nil)
+            (should-error
+             (save-buffer)
+             ;; "Attempt to save to a file that you aren't allowed to write"
+             :type 'error))
+          (should-not buffer-backed-up)
+          (should     (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "foo\n"))
+          (should     (equal (file-contents backup) 'missing))
+
+          ;; Save to read-only file with backup, accepting prompt,
+          ;; experiencing a write error.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (should-error
+             (cl-letf (((symbol-function 'write-region)
+                        #'signal-write-failed))
+               (save-buffer))
+             ;; "Write failed"
+             :type 'file-error))
+          (should-not buffer-backed-up)
+          (should     (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "foo\n"))
+          (should     (equal (file-contents backup) 'missing))
+
+          ;; Save to read-only file with backup, accepting prompt.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (save-buffer))
+          (should     buffer-backed-up)
+          (should-not (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should-not (file-writable-p backup))
+          (should     (equal (file-contents file) "bar\nfoo\n"))
+          (should     (equal (file-contents backup) "foo\n"))
+
+          ;; Prepare for tests not backing up the file.
+          (setq buffer-backed-up nil)
+          (delete-file backup)
+          (goto-char (point-min))
+          (insert "baz\n")
+
+          ;; Save to read-only file without backup, accepting prompt,
+          ;; experiencing a write error.  This tests that issue B of
+          ;; bug#66546 is fixed.  The results of the "with backup" and
+          ;; "without backup" subtests are identical when a write
+          ;; error occurs, but the code paths to reach these results
+          ;; are not.  In other words, this subtest is not redundant.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (should-error
+             (cl-letf (((symbol-function 'write-region)
+                        #'signal-write-failed))
+               (save-buffer 0))
+             ;; "Write failed"
+             :type 'file-error))
+          (should-not buffer-backed-up)
+          (should     (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "bar\nfoo\n"))
+          (should     (equal (file-contents backup) 'missing))
+
+          ;; Save to read-only file without backup, accepting prompt.
+          ;; This tests that issue A of bug#66546 is fixed.
+          (files-tests--with-yes-or-no-p
+              (cons override-read-only-prompt t)
+            (save-buffer 0))
+          (should-not buffer-backed-up)
+          (should-not (buffer-modified-p))
+          (should-not (file-writable-p file))
+          (should     (equal (file-contents file) "baz\nbar\nfoo\n"))
+          (should     (equal (file-contents backup) 'missing)))))))
+
 (ert-deftest files-tests-save-some-buffers ()
   "Test `save-some-buffers'.
 Test the 3 cases for the second argument PRED, i.e., nil, t, or
-- 
2.30.2

bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66546@debbugs.gnu.org
> Date: Sat, 04 Nov 2023 13:30:37 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think one nit is still missing:
> 
> Right you are.  Fixed in next version.

Thanks, installed on the master branch.

Can we close the bug now?