Subject: bug#35497: closed (Re: [PATCH v7] Don't rewrite buffer contents after saving by rename)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 08 May 2019 07:49:02 +0000

Your bug report

#35497: [PATCH] Don't rewrite buffer contents after saving by rename

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
35497: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=35497
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: [PATCH v7] Don't rewrite buffer contents after saving by rename
From: Michael Albinus <[email-masked]>
To: Jonathan Tomer <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Wed, 08 May 2019 09:48:07 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi Jonathan,

thanks for this final version, I've pushed it to master. I've added a
skip for Emacs < 27, since it fails there. Obviously.

Marking the bug as closed.

Best regards, Michael.

Subject: [PATCH] Don't rewrite buffer contents after saving by rename
From: Jonathan Tomer <[email-masked]>
To: [email-masked]
Cc: Jonathan Tomer <[email-masked]>
Date: Mon, 29 Apr 2019 16:20:09 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

When `file-precious-flag' is non-nil, files are saved by renaming a
temporary file to the new name; this is an atomic operation on POSIX
so other programs will not see the file in an intermediate state.
Unfortunately, due to a paren-matching error introduced in change
574c05e219476912db3105fa164accd9ba12b35f, we would then write the
contents again in the usual way after this rename.  In addition to
being wasteful, this is a serious bug: the whole point of
`file-precious-flag' is to prevent race conditions with other programs
that might otherwise see an empty file, but with this bug the race is
actually much *more* likely to be visible: the rename will alert any
inotify watchers of a change, and then the subsequent write is very
likely to truncate the file just as those programs start to read it!
* lisp/files.el (basic-save-buffer-2): Don't rewrite file contents
  after saving-by-renaming.
* test/lisp/files-tests.el (files-tests-dont-rewrite-precious-files):
  Regression test for this change.
---
 etc/NEWS                 |  7 +++++++
 lisp/files.el            |  4 ++--
 test/lisp/files-tests.el | 27 +++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 9b32d720b6..5bfadcbbd6 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -340,6 +340,13 @@ longer.
 ** Multicolor fonts such as "Noto Color Emoji" can be displayed on
 Emacs configured with Cairo drawing and linked with cairo >= 1.16.0.
 
+---
+** The file-precious-flag is now respected correctly.
+A bug previously caused files to be saved twice when
+`file-precious-flag' or `break-hardlinks-on-save' were specified: once
+by renaming a temporary file to the destination name, and then again
+by truncating and rewriting the file, which is exactly what
+`file-precious-flag' is supposed to avoid.
 
 * Editing Changes in Emacs 27.1
 
diff --git a/lisp/files.el b/lisp/files.el
index c05d70a00e..72518e8127 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5256,7 +5256,7 @@ basic-save-buffer-2
 		     (set-file-extended-attributes buffer-file-name
 						   (nth 1 setmodes)))
 		 (set-file-modes buffer-file-name
-				 (logior (car setmodes) 128))))))
+				 (logior (car setmodes) 128)))))
 	(let (success)
 	  (unwind-protect
 	      (progn
@@ -5272,7 +5272,7 @@ basic-save-buffer-2
 	    (and setmodes (not success)
 		 (progn
 		   (rename-file (nth 2 setmodes) buffer-file-name t)
-		   (setq buffer-backed-up nil))))))
+		   (setq buffer-backed-up nil)))))))
     setmodes))
 
 (declare-function diff-no-select "diff"
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index ae8ea41a79..07012fea6e 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,32 @@ files-tests-file-attributes-equal
                     (executable-find (file-name-nondirectory tmpfile))))))
       (delete-file tmpfile))))
 
+(ert-deftest files-tests-dont-rewrite-precious-files ()
+  "Test that `file-precious-flag' forces files to be saved by
+renaming only, rather than modified in-place."
+  (files-tests--with-temp-file temp-file-name
+    (with-current-buffer (find-file-noselect temp-file-name)
+      (let* (temp-file-events watch)
+        (unwind-protect
+            (progn
+              (setq watch
+                    (file-notify-add-watch
+                     temp-file-name '(change)
+                     (lambda (event)
+                       (add-to-list 'temp-file-events (cadr event) 'append))))
+              (setq-local file-precious-flag t)
+              (insert "foobar")
+              (should (null (save-buffer)))
+
+              ;; file-notify callbacks are input events, so we need to
+              ;; accept input before checking results.
+              (sit-for 0.1)
+
+              ;; When file-precious-flag is set, the visited file
+              ;; should never be modified, only renamed-to (which may
+              ;; appear as "renamed" and/or "created" to file-notify).
+              (should (not (memq 'changed temp-file-events))))
+        (file-notify-rm-watch watch))))))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here
-- 
2.21.0.593.g511ec345e18-goog