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

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

[Jonathan Tomer]

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.
 \f
 * 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

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

Hi Jonathan,

Thanks for the patch.

> --- 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.
>  \f
>  * Editing Changes in Emacs 27.1

We don't describe bug fixes in etc/NEWS.

> +(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."

I haven't checked the situation with Tramp. It cares also for
file-precious-flag, bug I don't remember whether it behaves as strict as
you have tested here. Do you want to write a Tramp test for this? It
would fit into tramp-tests.el.

Best regards, Michael.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On Tue, Apr 30, 2019 at 12:18 AM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Jonathan Tomer <jktomer@google.com> writes:
>
> Hi Jonathan,
>
> Thanks for the patch.
>
> > --- 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
>
> We don't describe bug fixes in etc/NEWS.
>

Thanks, I'll fix. What's the preferred mechanism for sending an updated
patch -- send an entirely new patch (relative to upstream) on a new thread,
or on this thread, or a delta to apply as an additional commit on top of my
previous patch?

> +(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."
>
> I haven't checked the situation with Tramp. It cares also for
> file-precious-flag, bug I don't remember whether it behaves as strict as
> you have tested here. Do you want to write a Tramp test for this? It
> would fit into tramp-tests.el.
>

The actual implementation of file-precious-flag's behavior is entirely
handled by basic-save-buffer-2 -- TRAMP substitutes different
implementations for write-region and rename-file but the decision of which
to use comes from outside. The only feature TRAMP adds is that, when
file-precious-flag is set, the local and remote temp files are checksummed
and the write is considered to have failed if they differ (preventing the
final rename into place). I suppose the reason this is done only when
file-precious-flag is set is that in the normal case it's too late to do
anything about an error.

In other words, I don't believe TRAMP is able to change the strictness of
file-precious-flag, unless its implementation of rename-file ends up being
nonatomic (which is likely the case for some remotes, but in that case an
atomic write is probably impossible anyway). That said, I'm happy to add a
test to tramp-tests.el as well; at the very least, with the mock tramp
method we should see that the destination file is renamed-to rather than
overwritten as well.

Best regards, Michael.
>

Thanks for the fast review!

[-- Attachment #2: Type: text/html, Size: 3746 bytes --]

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

Hi Jonathan,

> Thanks, I'll fix. What's the preferred mechanism for sending an
> updated patch -- send an entirely new patch (relative to upstream) on
> a new thread, or on this thread, or a delta to apply as an additional
> commit on top of my previous patch?

Please reply to these messages, keeping 35497@debbugs.gnu.org in To: or Cc:.
This archives your message in the bug tracker.

Usually, we appreciate a new patch relative to upstream. But in this
case, not changing etc/NEWS, I believe it isn't necessary to send a new
patch until there are other changes you want to show.

> That said, I'm happy to add a test to tramp-tests.el as well; at the
> very least, with the mock tramp method we should see that the
> destination file is renamed-to rather than overwritten as well.

Pls do.

Btw, your new test in files-tests.el uses file notifications. Possible
of course. But wouldn't it be more robust to check the inode number of
the involved files, and how it changes, or not? See file-attributes how
to retrieve the inode number of a file.

> Thanks for the fast review!

Best regards, Michael.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

On Tue, Apr 30, 2019 at 1:47 PM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Jonathan Tomer <jktomer@google.com> writes:
>
> Hi Jonathan,
>
> > Thanks, I'll fix. What's the preferred mechanism for sending an
> > updated patch -- send an entirely new patch (relative to upstream) on
> > a new thread, or on this thread, or a delta to apply as an additional
> > commit on top of my previous patch?
>
> Please reply to these messages, keeping 35497@debbugs.gnu.org in To: or
> Cc:.
> This archives your message in the bug tracker.
>
> Usually, we appreciate a new patch relative to upstream. But in this
> case, not changing etc/NEWS, I believe it isn't necessary to send a new
> patch until there are other changes you want to show.
>

OK, will send along with the patch adding the TRAMP test shortly.

> That said, I'm happy to add a test to tramp-tests.el as well; at the
> > very least, with the mock tramp method we should see that the
> > destination file is renamed-to rather than overwritten as well.
>
> Pls do.
>
> Btw, your new test in files-tests.el uses file notifications. Possible
> of course. But wouldn't it be more robust to check the inode number of
> the involved files, and how it changes, or not? See file-attributes how
> to retrieve the inode number of a file.
>

I thought about checking that the inode number changes, but that wouldn't
have
caught this particular bug (where the file is renamed into place with the
correct contents, and then rewritten in place again); indeed, that doesn't
appear to be easily caught with any examination of the final state alone,
since what we're looking for is to prove the *absence* of any write that
fails
to change the inode number. (Perhaps we could check that the modification
time
of the file, after write, is *less* than its inode change time, proving that
there has been no ordinary write since the rename -- but in my experience,
inode timestamps are not actually more reliable than inotify, and in
particular this check is easily defeated by the mode-setting that happens
after the write is complete, requiring care to make sure that save-buffer
will
not attempt to do so.)

Best,
Jonathan

[-- Attachment #2: Type: text/html, Size: 3162 bytes --]

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

Hi Jonathan,

> I thought about checking that the inode number changes, but that
> wouldn't have caught this particular bug (where the file is renamed
> into place with the correct contents, and then rewritten in place
> again); indeed, that doesn't appear to be easily caught with any
> examination of the final state alone, since what we're looking for is
> to prove the *absence* of any write that fails to change the inode
> number. (Perhaps we could check that the modification time of the
> file, after write, is *less* than its inode change time, proving that
> there has been no ordinary write since the rename -- but in my
> experience, inode timestamps are not actually more reliable than
> inotify, and in particular this check is easily defeated by the
> mode-setting that happens after the write is complete, requiring care
> to make sure that save-buffer will not attempt to do so.)

I see. But pls keep in mind, that inotify is not the only file
notification backend. Currently, we have six different beasts for this.

> Best,
> Jonathan

Best regards, Michael.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On Tue, Apr 30, 2019 at 2:21 PM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Jonathan Tomer <jktomer@google.com> writes:
>
> Hi Jonathan,
>
> > I thought about checking that the inode number changes, but that
> > wouldn't have caught this particular bug (where the file is renamed
> > into place with the correct contents, and then rewritten in place
> > again); indeed, that doesn't appear to be easily caught with any
> > examination of the final state alone, since what we're looking for is
> > to prove the *absence* of any write that fails to change the inode
> > number. (Perhaps we could check that the modification time of the
> > file, after write, is *less* than its inode change time, proving that
> > there has been no ordinary write since the rename -- but in my
> > experience, inode timestamps are not actually more reliable than
> > inotify, and in particular this check is easily defeated by the
> > mode-setting that happens after the write is complete, requiring care
> > to make sure that save-buffer will not attempt to do so.)
>
> I see. But pls keep in mind, that inotify is not the only file
> notification backend. Currently, we have six different beasts for this.
>

I was a bit worried about that; I considered disabling the test when
file-notify--library is not inotify, but instead designed the test so that
*missed* notifications would not cause it to fail, and I believe the other
implementations at least should not record a spurious "changed"
notification.
It's still not ideal but it is the best regression coverage I can think of
here.

I don't have BSD or win32 machines available to test on, so if you can point
me to a way to test this change on those targets I'm happy to run the new
tests.

Updated patch incoming.

[-- Attachment #2: Type: text/html, Size: 2464 bytes --]

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

[Jonathan Tomer]

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):
* test/lisp/net/tramp-tests.el (tramp-test46-file-precious-flag):
  Regression tests for this change.
---
 lisp/files.el                |  4 ++--
 test/lisp/files-tests.el     | 26 ++++++++++++++++++++++++++
 test/lisp/net/tramp-tests.el | 28 ++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

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..15f2a760c4 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,31 @@ 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
+    (let* (temp-file-events
+           (watch (file-notify-add-watch
+                   temp-file-name '(change)
+                   (lambda (event)
+                     (push (cadr event) temp-file-events)))))
+      (unwind-protect
+          (with-current-buffer (find-file-noselect temp-file-name)
+            (setq-local file-precious-flag t)
+            (insert "foobar")
+            (should (null (save-buffer)))
+
+            ;; file-notify callbacks are triggered by input events,
+            ;; so we need to accept input before checking results.
+            (with-timeout (3.0 (ignore))
+              (while (read-event nil nil 0.01) (ignore)))
+
+            ;; 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
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index cba697da18..1ca98520b3 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -44,6 +44,7 @@
 (require 'dired)
 (require 'ert)
 (require 'ert-x)
+(require 'filenotify)
 (require 'tramp)
 (require 'vc)
 (require 'vc-bzr)
@@ -5741,6 +5742,33 @@ tramp--test-asynchronous-requests-timeout
 	  (ignore-errors (all-completions "tramp" (symbol-value x)))
 	  (ert-fail (format "Hook `%s' still contains Tramp function" x))))))
 
+(ert-deftest tramp-test46-file-precious-flag ()
+  "Check that file-precious-flag is respected with Tramp in use."
+  (let* ((temp-file (make-temp-file "emacs"))
+         (remote-file (concat "/mock:localhost:" temp-file))
+         temp-file-events
+         (watch
+          (file-notify-add-watch
+           temp-file '(change)
+           (lambda (event)
+             (push (cadr event) 'temp-file-events)))))
+    (unwind-protect
+        (with-current-buffer (find-file-noselect remote-file)
+          (setq-local file-precious-flag t)
+          (insert "foobar")
+          (should (null (save-buffer)))
+
+          ;; file-notify callbacks are triggered by input events, so
+          ;; we need to accept input before checking results.
+          (with-timeout (3.0 (ignore))
+            (while (read-event nil nil 0.01) (ignore)))
+
+          ;; When file-precious-flag is set, the visited file should
+          ;; never be modified, only renamed-to.
+          (should (not (memq 'changed temp-file-events)))))
+      (file-notify-rm-watch watch)
+      (delete-file temp-file)))
+
 (defun tramp-test-all (&optional interactive)
   "Run all tests for \\[tramp]."
   (interactive "p")
-- 
2.21.0.593.g511ec345e18-goog

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

[Eli Zaretskii]

> From: Jonathan Tomer <jktomer@google.com>
> Date: Tue, 30 Apr 2019 14:10:32 -0700
> Cc: 35497@debbugs.gnu.org
> 
>  Btw, your new test in files-tests.el uses file notifications. Possible
>  of course. But wouldn't it be more robust to check the inode number of
>  the involved files, and how it changes, or not? See file-attributes how
>  to retrieve the inode number of a file.
> 
> I thought about checking that the inode number changes, but that wouldn't have
> caught this particular bug (where the file is renamed into place with the
> correct contents, and then rewritten in place again); indeed, that doesn't
> appear to be easily caught with any examination of the final state alone,
> since what we're looking for is to prove the *absence* of any write that fails
> to change the inode number. (Perhaps we could check that the modification time
> of the file, after write, is *less* than its inode change time, proving that
> there has been no ordinary write since the rename -- but in my experience,
> inode timestamps are not actually more reliable than inotify, and in
> particular this check is easily defeated by the mode-setting that happens
> after the write is complete, requiring care to make sure that save-buffer will
> not attempt to do so.)

I suggest to make a step back and clearly define what you are trying
to test.  Is it that we don't rewrite the file after saving it, or is
it some condition regarding the inodes of the original and the new
file?

These are two different things, and the second one is extremely
platform-dependent (because inodes exist only in certain filesystems,
and are emulated in others, and also because which notifications are
generated in such complex situations is very hard to guess in
advance).

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

On Wed, May 1, 2019, 10:48 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Jonathan Tomer <jktomer@google.com>
> > Date: Tue, 30 Apr 2019 14:10:32 -0700
> > Cc: 35497@debbugs.gnu.org
> >
> >  Btw, your new test in files-tests.el uses file notifications. Possible
> >  of course. But wouldn't it be more robust to check the inode number of
> >  the involved files, and how it changes, or not? See file-attributes how
> >  to retrieve the inode number of a file.
> >
> > I thought about checking that the inode number changes, but that
> wouldn't have
> > caught this particular bug (where the file is renamed into place with the
> > correct contents, and then rewritten in place again); indeed, that
> doesn't
> > appear to be easily caught with any examination of the final state alone,
> > since what we're looking for is to prove the *absence* of any write that
> fails
> > to change the inode number. (Perhaps we could check that the
> modification time
> > of the file, after write, is *less* than its inode change time, proving
> that
> > there has been no ordinary write since the rename -- but in my
> experience,
> > inode timestamps are not actually more reliable than inotify, and in
> > particular this check is easily defeated by the mode-setting that happens
> > after the write is complete, requiring care to make sure that
> save-buffer will
> > not attempt to do so.)
>
> I suggest to make a step back and clearly define what you are trying
> to test.  Is it that we don't rewrite the file after saving it, or is
> it some condition regarding the inodes of the original and the new
> file?
>
> These are two different things, and the second one is extremely
> platform-dependent (because inodes exist only in certain filesystems,
> and are emulated in others, and also because which notifications are
> generated in such complex situations is very hard to guess in
> advance).
>

Indeed. What I am testing, as you say, is that the file is not changed by
writing to it under its final name (ever, not just after renaming, though
the latter happened to be the bug in this case) when file-precious-flag is
non-nil.

Any discussion of inodes was only because of the perceived unreliability,
and actual relative unportability, of filenotify and the systems underlying
it.

It's true that notifications are imperfect, but IMO they are the only
possible way to test that particular invariant, and this test
implementation is designed to be as strict as the available notification
system allows without breaking under any reasonable notification API.

>

[-- Attachment #2: Type: text/html, Size: 3466 bytes --]

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

[Eli Zaretskii]

> From: Jonathan Tomer <jktomer@google.com>
> Date: Wed, 1 May 2019 12:29:42 -0700
> Cc: Michael Albinus <michael.albinus@gmx.de>, 35497@debbugs.gnu.org
> 
> It's true that notifications are imperfect, but IMO they are the only possible way to test that particular invariant,

I'm not sure.  You could instead advise or hook write-region, and see
that it is not being called to write to the file, for example.
Wouldn't that be easier and more reliable?

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Wed, May 1, 2019 at 12:55 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Jonathan Tomer <jktomer@google.com>
> > Date: Wed, 1 May 2019 12:29:42 -0700
> > Cc: Michael Albinus <michael.albinus@gmx.de>, 35497@debbugs.gnu.org
> >
> > It's true that notifications are imperfect, but IMO they are the only
> possible way to test that particular invariant,
>
> I'm not sure.  You could instead advise or hook write-region, and see
> that it is not being called to write to the file, for example.
> Wouldn't that be easier and more reliable?
>

Oh, of course -- that's quite a bit better. Thanks, will send a new patch
in a moment.

[-- Attachment #2: Type: text/html, Size: 1194 bytes --]

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

[Jonathan Tomer]

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):
* test/lisp/net/tramp-tests.el (tramp-test46-file-precious-flag):
  Regression tests for this change.
---
 lisp/files.el                |  4 ++--
 test/lisp/files-tests.el     | 13 +++++++++++++
 test/lisp/net/tramp-tests.el | 16 ++++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

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..0becde4184 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,18 @@ 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
+    (files-tests--with-advice
+        write-region :before
+        (lambda (_start _end filename &rest r)
+          (should (not (string= filename temp-file-name))))
+      (with-current-buffer (find-file-noselect temp-file-name)
+        (setq-local file-precious-flag t)
+        (insert "foobar")
+        (should (null (save-buffer)))))))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index cba697da18..03ce6a5e94 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -5741,6 +5741,22 @@ tramp--test-asynchronous-requests-timeout
 	  (ignore-errors (all-completions "tramp" (symbol-value x)))
 	  (ert-fail (format "Hook `%s' still contains Tramp function" x))))))
 
+(ert-deftest tramp-test46-file-precious-flag ()
+  "Check that file-precious-flag is respected with Tramp in use."
+  (let* ((temp-file (make-temp-file "emacs"))
+         (remote-file (concat "/mock:localhost:" temp-file))
+         (advice (lambda (_start _end filename &rest r)
+                   (should (not (string= filename temp-file)))
+                   (should (not (string= filename remote-file))))))
+      (unwind-protect
+          (with-current-buffer (find-file-noselect remote-file)
+            (advice-add 'write-region :before advice)
+            (setq-local file-precious-flag t)
+            (insert "foobar")
+            (should (null (save-buffer))))
+          (advice-remove 'write-region advice)
+          (delete-file temp-file))))
+
 (defun tramp-test-all (&optional interactive)
   "Run all tests for \\[tramp]."
   (interactive "p")
-- 
2.21.0.593.g511ec345e18-goog

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

Hi Jonathan,

> +(ert-deftest tramp-test46-file-precious-flag ()

Since this belongs rather to write-region, I would call it
`tramp-test10-write-region-file-precious-flag', and move it to the
repsective place in file.

> +  "Check that file-precious-flag is respected with Tramp in use."
> +  (let* ((temp-file (make-temp-file "emacs"))
> +         (remote-file (concat "/mock:localhost:" temp-file))

Please don't do this. The mock method does not work everywhere, for
example on an MS Windows machine.

`file-precious-flag' is handled in the tramp-sh.el handler only. So you
might start with the test `tramp--test-sh-p', and skip otherwise.

And then you could use the same mechanism like in the other
tests. Something like this:

--8<---------------cut here---------------start------------->8---
(ert-deftest tramp-test10-write-region-file-precious-flag ()
  "Check that `file-precious-flag' is respected with Tramp in use."
  (skip-unless (tramp--test-enabled))
  (skip-unless (tramp--test-sh-p))

  (let ((tmp-name (tramp--test-make-temp-name))
	(advice (lambda (_start _end filename &rest r)
		  (should-not (string= filename tmp-name)))))

    (unwind-protect
        (with-current-buffer (find-file-noselect tmp-name)
	  ;; Write initial contents.  Adapt `visited-file-modtime'
	  ;; in order to suppress confirmation.
	  (insert "foo")
	  (write-region nil nil tmp-name)
          (set-visited-file-modtime)
	  ;; Run the test.
          (advice-add 'write-region :before advice)
          (setq-local file-precious-flag t)
          (insert "bar")
          (should (null (save-buffer))))

      ;; Cleanup.
      (ignore-errors (advice-remove 'write-region advice))
      (ignore-errors (delete-file tmp-name)))))
--8<---------------cut here---------------end--------------->8---

I haven't tested further, this gives an error for me. Don't know yet,
whether it is the test definition, or (more likely) a problem in Tramp.

Best regards, Michael.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]

On Thu, May 2, 2019 at 4:50 AM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Jonathan Tomer <jktomer@google.com> writes:
>
> Hi Jonathan,
>
> > +(ert-deftest tramp-test46-file-precious-flag ()
>
> Since this belongs rather to write-region, I would call it
> `tramp-test10-write-region-file-precious-flag', and move it to the
> repsective place in file.
>
> > +  "Check that file-precious-flag is respected with Tramp in use."
> > +  (let* ((temp-file (make-temp-file "emacs"))
> > +         (remote-file (concat "/mock:localhost:" temp-file))
>
> Please don't do this. The mock method does not work everywhere, for
> example on an MS Windows machine.
>
> `file-precious-flag' is handled in the tramp-sh.el handler only. So you
> might start with the test `tramp--test-sh-p', and skip otherwise.
>
> And then you could use the same mechanism like in the other
> tests. Something like this:
>
> --8<---------------cut here---------------start------------->8---
> (ert-deftest tramp-test10-write-region-file-precious-flag ()
>   "Check that `file-precious-flag' is respected with Tramp in use."
>   (skip-unless (tramp--test-enabled))
>   (skip-unless (tramp--test-sh-p))
>
>   (let ((tmp-name (tramp--test-make-temp-name))
>         (advice (lambda (_start _end filename &rest r)
>                   (should-not (string= filename tmp-name)))))
>
>     (unwind-protect
>         (with-current-buffer (find-file-noselect tmp-name)
>           ;; Write initial contents.  Adapt `visited-file-modtime'
>           ;; in order to suppress confirmation.
>           (insert "foo")
>           (write-region nil nil tmp-name)
>           (set-visited-file-modtime)
>           ;; Run the test.
>           (advice-add 'write-region :before advice)
>           (setq-local file-precious-flag t)
>           (insert "bar")
>           (should (null (save-buffer))))
>
>       ;; Cleanup.
>       (ignore-errors (advice-remove 'write-region advice))
>       (ignore-errors (delete-file tmp-name)))))
> --8<---------------cut here---------------end--------------->8---
>
> I haven't tested further, this gives an error for me. Don't know yet,
> whether it is the test definition, or (more likely) a problem in Tramp.
>

Changing let to let* fixes the test. New patch incoming.


>
> Best regards, Michael.
>

[-- Attachment #2: Type: text/html, Size: 3457 bytes --]

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

[Jonathan Tomer]

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):
* test/lisp/net/tramp-tests.el (tramp-test10-write-region-file-precious-flag):
  Regression tests for this change.
---
 lisp/files.el                |  4 ++--
 test/lisp/files-tests.el     | 13 +++++++++++++
 test/lisp/net/tramp-tests.el | 26 ++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

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..0becde4184 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,18 @@ 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
+    (files-tests--with-advice
+        write-region :before
+        (lambda (_start _end filename &rest r)
+          (should (not (string= filename temp-file-name))))
+      (with-current-buffer (find-file-noselect temp-file-name)
+        (setq-local file-precious-flag t)
+        (insert "foobar")
+        (should (null (save-buffer)))))))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index cba697da18..dee5e5e0f9 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2270,6 +2270,32 @@ tramp--test-print-duration
 	;; Cleanup.
 	(ignore-errors (delete-file tmp-name))))))
 
+(ert-deftest tramp-test10-write-region-file-precious-flag ()
+    "Check that `file-precious-flag' is respected with Tramp in use."
+  (skip-unless (tramp--test-enabled))
+  (skip-unless (tramp--test-sh-p))
+
+  (let* ((tmp-name (tramp--test-make-temp-name))
+        (advice (lambda (_start _end filename &rest r)
+                  (should-not (string= filename tmp-name)))))
+
+    (unwind-protect
+        (with-current-buffer (find-file-noselect tmp-name)
+          ;; Write initial contents.  Adapt `visited-file-modtime'
+          ;; in order to suppress confirmation.
+          (insert "foo")
+          (write-region nil nil tmp-name)
+          (set-visited-file-modtime)
+          ;; Run the test.
+          (advice-add 'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "bar")
+          (should (null (save-buffer))))
+
+      ;; Cleanup.
+      (ignore-errors (advice-remove 'write-region advice))
+      (ignore-errors (delete-file tmp-name)))))
+
 (ert-deftest tramp-test11-copy-file ()
   "Check `copy-file'."
   (skip-unless (tramp--test-enabled))
-- 
2.21.0.593.g511ec345e18-goog

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

Hi Jonathan,

I still get an error with the Tramp test:

--8<---------------cut here---------------start------------->8---
Test tramp-test10-write-region-file-precious-flag condition:
    (file-error "Copying directly failed, see buffer `*tramp/mock detlef*' for details.")
   FAILED  2/2  tramp-test10-write-region-file-precious-flag (0.995086 sec)
--8<---------------cut here---------------end--------------->8---

You might keep it as it is, but mark it unstable:

> +(ert-deftest tramp-test10-write-region-file-precious-flag ()
> +  "Check that `file-precious-flag' is respected with Tramp in use."
> +  :tags '(:unstable)

I will check then what's up.

Looks to me like you haven't signed yet the FSF papers. This is
necessary to contribute to Emacs. Are you willing to sign them? You
could read about this at <https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html>.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 35497@debbugs.gnu.org,  eliz@gnu.org
> Date: Fri, 03 May 2019 09:52:13 +0200
> 
> Looks to me like you haven't signed yet the FSF papers. This is
> necessary to contribute to Emacs. Are you willing to sign them? You
> could read about this at <https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html>.

He's @google.com, so if this work was done on Google's time, it's
covered by Google's assignment.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

On Fri, May 3, 2019 at 12:52 AM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Jonathan Tomer <jktomer@google.com> writes:
>
> Hi Jonathan,
>
> I still get an error with the Tramp test:
>
> --8<---------------cut here---------------start------------->8---
> Test tramp-test10-write-region-file-precious-flag condition:
>     (file-error "Copying directly failed, see buffer `*tramp/mock detlef*'
> for details.")
>    FAILED  2/2  tramp-test10-write-region-file-precious-flag (0.995086 sec)
> --8<---------------cut here---------------end--------------->8---
>

This is the form of the errors I get when the test fails intentionally
(i.e. because I un-fix the bug and re-run the test). I've rewritten the
test to fail properly when it's supposed to, but also left the unstable tag
on it.


> You might keep it as it is, but mark it unstable:
>
> > +(ert-deftest tramp-test10-write-region-file-precious-flag ()
> > +  "Check that `file-precious-flag' is respected with Tramp in use."
> > +  :tags '(:unstable)
>
> I will check then what's up.
>
> Looks to me like you haven't signed yet the FSF papers. This is
> necessary to contribute to Emacs. Are you willing to sign them? You
> could read about this at <
> https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html>.
>

As eliz@ notes, I cannot sign the personal form of the copyright assignment
while I work for Google, since I have no copyright to assign, but Google's
assignment should suffice.


>
> Best regards, Michael.
>

[-- Attachment #2: Type: text/html, Size: 2453 bytes --]

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

Hi Jonathan,

> As eliz@ notes, I cannot sign the personal form of the copyright
> assignment while I work for Google, since I have no copyright to
> assign, but Google's assignment should suffice.

I didn't know this special Google rule. OK.

If the dust has settled, I could commit it then in your name.

Best regards, Michael.

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

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > As eliz@ notes, I cannot sign the personal form of the copyright
  > > assignment while I work for Google, since I have no copyright to
  > > assign, but Google's assignment should suffice.

  > I didn't know this special Google rule. OK.

Actually it applies to most employees who are programmers.

Usually we get the company to disclaim copyright
so that the human author can assign copyright,
but sometimes the company assigns the copyright itself.
Either way is ok.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

[Jonathan Tomer]

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):
* test/lisp/net/tramp-tests.el (tramp-test10-write-region-file-precious-flag):
  Regression tests for this change.
---
 lisp/files.el                |  4 ++--
 test/lisp/files-tests.el     | 15 +++++++++++++++
 test/lisp/net/tramp-tests.el | 27 +++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

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..fe2e958f1c 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,20 @@ 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."
+  (let* ((temp-file-name (make-temp-file "files-tests"))
+         (advice (lambda (_start _end filename &rest _r)
+                   (should-not (string= filename temp-file-name)))))
+    (unwind-protect
+        (with-current-buffer (find-file-noselect temp-file-name)
+          (advice-add #'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "foobar")
+          (should (null (save-buffer))))
+      (ignore-errors (advice-remove #'write-region advice))
+      (ignore-errors (delete-file temp-file-name)))))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index cba697da18..a18cb19c68 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2270,6 +2270,33 @@ tramp--test-print-duration
 	;; Cleanup.
 	(ignore-errors (delete-file tmp-name))))))
 
+(ert-deftest tramp-test10-write-region-file-precious-flag ()
+    "Check that `file-precious-flag' is respected with Tramp in use."
+    :tags '(:unstable)
+  (skip-unless (tramp--test-enabled))
+  (skip-unless (tramp--test-sh-p))
+
+  (let* ((tmp-name (tramp--test-make-temp-name))
+        (advice (lambda (_start _end filename &rest _r)
+                  (should-not (string= filename tmp-name)))))
+
+    (unwind-protect
+        (with-current-buffer (find-file-noselect tmp-name)
+          ;; Write initial contents.  Adapt `visited-file-modtime'
+          ;; in order to suppress confirmation.
+          (insert "foo")
+          (write-region nil nil tmp-name)
+          (set-visited-file-modtime)
+          ;; Run the test.
+          (advice-add 'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "bar")
+          (should (null (save-buffer))))
+
+      ;; Cleanup.
+      (ignore-errors (advice-remove 'write-region advice))
+      (ignore-errors (delete-file tmp-name)))))
+
 (ert-deftest tramp-test11-copy-file ()
   "Check `copy-file'."
   (skip-unless (tramp--test-enabled))
-- 
2.21.0.1020.gf2820cf01a-goog

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

[Jonathan Tomer]

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):
* test/lisp/net/tramp-tests.el (tramp-test10-write-region-file-precious-flag):
  Regression tests for this change.
---
 lisp/files.el                |  4 ++--
 test/lisp/files-tests.el     | 15 +++++++++++++++
 test/lisp/net/tramp-tests.el | 30 ++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)

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..fe2e958f1c 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,20 @@ 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."
+  (let* ((temp-file-name (make-temp-file "files-tests"))
+         (advice (lambda (_start _end filename &rest _r)
+                   (should-not (string= filename temp-file-name)))))
+    (unwind-protect
+        (with-current-buffer (find-file-noselect temp-file-name)
+          (advice-add #'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "foobar")
+          (should (null (save-buffer))))
+      (ignore-errors (advice-remove #'write-region advice))
+      (ignore-errors (delete-file temp-file-name)))))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index cba697da18..2e25f23b23 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -41,6 +41,7 @@
 
 ;;; Code:
 
+(require 'cl-seq)
 (require 'dired)
 (require 'ert)
 (require 'ert-x)
@@ -2270,6 +2271,35 @@ tramp--test-print-duration
 	;; Cleanup.
 	(ignore-errors (delete-file tmp-name))))))
 
+(ert-deftest tramp-test10-write-region-file-precious-flag ()
+    "Check that `file-precious-flag' is respected with Tramp in use."
+    :tags '(:unstable)
+  (skip-unless (tramp--test-enabled))
+  (skip-unless (tramp--test-sh-p))
+
+  (let* ((tmp-name (tramp--test-make-temp-name))
+         written-files
+        (advice (lambda (_start _end filename &rest _r)
+                  (push filename written-files))))
+
+    (unwind-protect
+        (with-current-buffer (find-file-noselect tmp-name)
+          ;; Write initial contents.  Adapt `visited-file-modtime'
+          ;; in order to suppress confirmation.
+          (insert "foo")
+          (write-region nil nil tmp-name)
+          (set-visited-file-modtime)
+          ;; Run the test.
+          (advice-add 'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "bar")
+          (should (null (save-buffer)))
+          (should-not (cl-member tmp-name written-files :test #'string=))
+
+      ;; Cleanup.
+      (ignore-errors (advice-remove 'write-region advice))
+      (ignore-errors (delete-file tmp-name))))))
+
 (ert-deftest tramp-test11-copy-file ()
   "Check `copy-file'."
   (skip-unless (tramp--test-enabled))
-- 
2.21.0.1020.gf2820cf01a-goog

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

[Michael Albinus]

Jonathan Tomer <jktomer@google.com> writes:

> +(ert-deftest tramp-test10-write-region-file-precious-flag ()
> +    "Check that `file-precious-flag' is respected with Tramp in use."
> +    :tags '(:unstable)
> +  (skip-unless (tramp--test-enabled))
> +  (skip-unless (tramp--test-sh-p))
> +
> +  (let* ((tmp-name (tramp--test-make-temp-name))
> +         written-files
> +        (advice (lambda (_start _end filename &rest _r)
> +                  (push filename written-files))))
> +
> +    (unwind-protect
> +        (with-current-buffer (find-file-noselect tmp-name)
> +          ;; Write initial contents.  Adapt `visited-file-modtime'
> +          ;; in order to suppress confirmation.
> +          (insert "foo")
> +          (write-region nil nil tmp-name)
> +          (set-visited-file-modtime)
> +          ;; Run the test.
> +          (advice-add 'write-region :before advice)
> +          (setq-local file-precious-flag t)
> +          (insert "bar")
> +          (should (null (save-buffer)))
> +          (should-not (cl-member tmp-name written-files :test #'string=))

I believe a closing parenthesis ")" is missing.

> +      ;; Cleanup.
> +      (ignore-errors (advice-remove 'write-region advice))
> +      (ignore-errors (delete-file tmp-name))))))

Best regards, Michael.

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

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> I believe a closing parenthesis ")" is missing.

PS: When this is fixed, the test passes for me.

Best regards, Michael.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

On Tue, May 7, 2019 at 7:11 AM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
> > I believe a closing parenthesis ")" is missing.
>
> PS: When this is fixed, the test passes for me.
>

Whoops. Sending fix now, and removing unstable tag.


>
> Best regards, Michael.
>

[-- Attachment #2: Type: text/html, Size: 867 bytes --]

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

[Jonathan Tomer]

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):
* test/lisp/net/tramp-tests.el (tramp-test10-write-region-file-precious-flag):
  Regression tests for this change.
---
 lisp/files.el                |  4 ++--
 test/lisp/files-tests.el     | 15 +++++++++++++++
 test/lisp/net/tramp-tests.el | 29 +++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 2 deletions(-)

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..fe2e958f1c 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1244,5 +1244,20 @@ 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."
+  (let* ((temp-file-name (make-temp-file "files-tests"))
+         (advice (lambda (_start _end filename &rest _r)
+                   (should-not (string= filename temp-file-name)))))
+    (unwind-protect
+        (with-current-buffer (find-file-noselect temp-file-name)
+          (advice-add #'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "foobar")
+          (should (null (save-buffer))))
+      (ignore-errors (advice-remove #'write-region advice))
+      (ignore-errors (delete-file temp-file-name)))))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index cba697da18..0c5ea03741 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -41,6 +41,7 @@
 
 ;;; Code:
 
+(require 'cl-seq)
 (require 'dired)
 (require 'ert)
 (require 'ert-x)
@@ -2270,6 +2271,34 @@ tramp--test-print-duration
 	;; Cleanup.
 	(ignore-errors (delete-file tmp-name))))))
 
+(ert-deftest tramp-test10-write-region-file-precious-flag ()
+    "Check that `file-precious-flag' is respected with Tramp in use."
+  (skip-unless (tramp--test-enabled))
+  (skip-unless (tramp--test-sh-p))
+
+  (let* ((tmp-name (tramp--test-make-temp-name))
+         written-files
+         (advice (lambda (_start _end filename &rest _r)
+                   (push filename written-files))))
+
+    (unwind-protect
+        (with-current-buffer (find-file-noselect tmp-name)
+          ;; Write initial contents.  Adapt `visited-file-modtime'
+          ;; in order to suppress confirmation.
+          (insert "foo")
+          (write-region nil nil tmp-name)
+          (set-visited-file-modtime)
+          ;; Run the test.
+          (advice-add 'write-region :before advice)
+          (setq-local file-precious-flag t)
+          (insert "bar")
+          (should (null (save-buffer)))
+          (should-not (cl-member tmp-name written-files :test #'string=)))
+
+      ;; Cleanup.
+      (ignore-errors (advice-remove 'write-region advice))
+      (ignore-errors (delete-file tmp-name)))))
+
 (ert-deftest tramp-test11-copy-file ()
   "Check `copy-file'."
   (skip-unless (tramp--test-enabled))
-- 
2.21.0.1020.gf2820cf01a-goog

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

[Michael Albinus]

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.

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

[Jonathan Tomer]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Thanks!

On Wed, May 8, 2019 at 12:48 AM Michael Albinus <michael.albinus@gmx.de>
wrote:

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

[-- Attachment #2: Type: text/html, Size: 613 bytes --]