Eli Zaretskii writes: >> From: Jens Schmidt >> 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.