GNU bug report logs -
#35497
[PATCH] Don't rewrite buffer contents after saving by rename
Previous Next
Reported by: Jonathan Tomer <jktomer <at> google.com>
Date: Mon, 29 Apr 2019 23:32:01 UTC
Severity: normal
Tags: patch
Done: Michael Albinus <michael.albinus <at> gmx.de>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35497 in the body.
You can then email your comments to 35497 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Mon, 29 Apr 2019 23:32:03 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Jonathan Tomer <jktomer <at> google.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Mon, 29 Apr 2019 23:32:03 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 30 Apr 2019 07:19:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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.
> +(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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 30 Apr 2019 19:29:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, Apr 30, 2019 at 12:18 AM Michael Albinus <michael.albinus <at> gmx.de>
wrote:
> Jonathan Tomer <jktomer <at> 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!
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 30 Apr 2019 20:49:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 30 Apr 2019 21:11:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, Apr 30, 2019 at 1:47 PM Michael Albinus <michael.albinus <at> gmx.de>
wrote:
> Jonathan Tomer <jktomer <at> 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 <at> 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
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 30 Apr 2019 21:22:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 30 Apr 2019 22:43:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, Apr 30, 2019 at 2:21 PM Michael Albinus <michael.albinus <at> gmx.de>
wrote:
> Jonathan Tomer <jktomer <at> 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.
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 01 May 2019 00:28:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 35497 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 01 May 2019 17:49:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 35497 <at> debbugs.gnu.org (full text, mbox):
> From: Jonathan Tomer <jktomer <at> google.com>
> Date: Tue, 30 Apr 2019 14:10:32 -0700
> Cc: 35497 <at> 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).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 01 May 2019 19:31:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Wed, May 1, 2019, 10:48 Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Jonathan Tomer <jktomer <at> google.com>
> > Date: Tue, 30 Apr 2019 14:10:32 -0700
> > Cc: 35497 <at> 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.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 01 May 2019 19:56:01 GMT)
Full text and
rfc822 format available.
Message #35 received at 35497 <at> debbugs.gnu.org (full text, mbox):
> From: Jonathan Tomer <jktomer <at> google.com>
> Date: Wed, 1 May 2019 12:29:42 -0700
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, 35497 <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 01 May 2019 19:57:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Wed, May 1, 2019 at 12:55 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Jonathan Tomer <jktomer <at> google.com>
> > Date: Wed, 1 May 2019 12:29:42 -0700
> > Cc: Michael Albinus <michael.albinus <at> gmx.de>, 35497 <at> 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.
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 01 May 2019 23:04:01 GMT)
Full text and
rfc822 format available.
Message #41 received at 35497 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Thu, 02 May 2019 11:52:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Thu, 02 May 2019 22:05:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Thu, May 2, 2019 at 4:50 AM Michael Albinus <michael.albinus <at> gmx.de>
wrote:
> Jonathan Tomer <jktomer <at> 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.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Thu, 02 May 2019 22:08:02 GMT)
Full text and
rfc822 format available.
Message #50 received at 35497 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Fri, 03 May 2019 07:53:01 GMT)
Full text and
rfc822 format available.
Message #53 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Fri, 03 May 2019 12:30:03 GMT)
Full text and
rfc822 format available.
Message #56 received at 35497 <at> debbugs.gnu.org (full text, mbox):
> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: 35497 <at> debbugs.gnu.org, eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Mon, 06 May 2019 20:46:01 GMT)
Full text and
rfc822 format available.
Message #59 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Fri, May 3, 2019 at 12:52 AM Michael Albinus <michael.albinus <at> gmx.de>
wrote:
> Jonathan Tomer <jktomer <at> 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.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Mon, 06 May 2019 20:48:02 GMT)
Full text and
rfc822 format available.
Message #62 received at 35497 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Mon, 06 May 2019 20:49:01 GMT)
Full text and
rfc822 format available.
Message #65 received at 35497 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 07 May 2019 14:04:01 GMT)
Full text and
rfc822 format available.
Message #68 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 07 May 2019 14:07:01 GMT)
Full text and
rfc822 format available.
Message #71 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Jonathan Tomer <jktomer <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 07 May 2019 14:12:02 GMT)
Full text and
rfc822 format available.
Message #74 received at 35497 <at> debbugs.gnu.org (full text, mbox):
Michael Albinus <michael.albinus <at> gmx.de> writes:
> I believe a closing parenthesis ")" is missing.
PS: When this is fixed, the test passes for me.
Best regards, Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 07 May 2019 17:26:01 GMT)
Full text and
rfc822 format available.
Message #77 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, May 7, 2019 at 7:11 AM Michael Albinus <michael.albinus <at> gmx.de>
wrote:
> Michael Albinus <michael.albinus <at> 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.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 07 May 2019 17:37:02 GMT)
Full text and
rfc822 format available.
Message #80 received at 35497 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Tue, 07 May 2019 23:47:01 GMT)
Full text and
rfc822 format available.
Message #83 received at 35497 <at> debbugs.gnu.org (full text, mbox):
[[[ 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)
Reply sent
to
Michael Albinus <michael.albinus <at> gmx.de>:
You have taken responsibility.
(Wed, 08 May 2019 07:49:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Jonathan Tomer <jktomer <at> google.com>:
bug acknowledged by developer.
(Wed, 08 May 2019 07:49:02 GMT)
Full text and
rfc822 format available.
Message #88 received at 35497-done <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35497; Package
emacs.
(Wed, 08 May 2019 17:04:01 GMT)
Full text and
rfc822 format available.
Message #91 received at 35497-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Thanks!
On Wed, May 8, 2019 at 12:48 AM Michael Albinus <michael.albinus <at> 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.
>
[Message part 2 (text/html, inline)]
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 06 Jun 2019 11:24:08 GMT)
Full text and
rfc822 format available.
This bug report was last modified 4 years and 320 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.