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" <bug-gnu-emacs@gnu.org>]

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