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.