[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#66546: 30.0.50; save-buffer to write-protected file without backup f
From: |
Jens Schmidt |
Subject: |
bug#66546: 30.0.50; save-buffer to write-protected file without backup fails |
Date: |
Sat, 21 Oct 2023 19:56:44 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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.
>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, (continued)
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/15
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/16
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/16
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/17
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/17
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/18
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/18
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/19
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/19
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/20
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails,
Jens Schmidt <=
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/21
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/21
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/22
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/22
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/25
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Jens Schmidt, 2023/10/29
- bug#66546: 30.0.50; save-buffer to write-protected file without backup fails, Eli Zaretskii, 2023/10/29