bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Mattias Engdegård]

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

Auto-revert mode stops working effectively when the user changes the buffer file name, by running `write-file' for example, because the notifier still tracks the old file.

The attached patch attempts to rectify this.

[-- Attachment #2: 0001-Keep-auto-revert-mode-working-when-changing-buffer-f.patch --]
[-- Type: application/octet-stream, Size: 5146 bytes --]

From fa1e9cb90db4a296c1e7c879c21b68285b7026a3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 10 Jun 2019 17:52:50 +0200
Subject: [PATCH] Keep auto-revert-mode working when changing buffer file name

* lisp/autorevert.el (after-set-visited-file-name-hook):
Add unconditionally.
(global-auto-revert-mode): Don't use
`after-set-visited-file-name-hook' here.
(auto-revert--set-visited-file-name): Rename from
`auto-revert--global-set-visited-file-name' and generalise.
* test/lisp/autorevert-tests.el (auto-revert-test06-write-file): New.
---
 lisp/autorevert.el            | 19 ++++++++++---------
 test/lisp/autorevert-tests.el | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 2de855b303..3944dcdc00 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -347,6 +347,8 @@ auto-revert-find-file-function
 
 (add-hook 'find-file-hook
 	  #'auto-revert-find-file-function)
+(add-hook 'after-set-visited-file-name-hook
+          #'auto-revert--set-visited-file-name)
 
 (defvar auto-revert--buffers-by-watch-descriptor
   (make-hash-table :test 'equal)
@@ -508,8 +510,6 @@ global-auto-revert-mode
             (auto-revert--global-add-current-buffer)))
         ;; Make sure future buffers are added as well.
         (add-hook 'find-file-hook #'auto-revert--global-adopt-current-buffer)
-        (add-hook 'after-set-visited-file-name-hook
-                  #'auto-revert--global-set-visited-file-name)
         ;; To track non-file buffers, we need to listen in to buffer
         ;; creation in general.  Listening to major-mode changes is
         ;; suitable, since we then know whether it's a mode that is tracked.
@@ -520,8 +520,6 @@ global-auto-revert-mode
     ;; Turn global-auto-revert-mode OFF.
     (remove-hook 'after-change-major-mode-hook
                  #'auto-revert--global-adopt-current-buffer)
-    (remove-hook 'after-set-visited-file-name-hook
-                 #'auto-revert--global-set-visited-file-name)
     (remove-hook 'find-file-hook #'auto-revert--global-adopt-current-buffer)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
@@ -551,14 +549,17 @@ auto-revert--global-adopt-current-buffer
   (auto-revert--global-add-current-buffer)
   (auto-revert-set-timer))
 
-(defun auto-revert--global-set-visited-file-name ()
-  "Update Global Auto-Revert management of the current buffer.
+(defun auto-revert--set-visited-file-name ()
+  "Update Auto-Revert management of the current buffer.
 Called after `set-visited-file-name'."
-  ;; Remove any existing notifier first so that we don't track the
-  ;; wrong file in case the file name was changed.
   (when auto-revert-notify-watch-descriptor
+    ;; Remove any existing notifier so that we don't track the wrong
+    ;; file in case the file name was changed.
     (auto-revert-notify-rm-watch))
-  (auto-revert--global-adopt-current-buffer))
+    (cond (global-auto-revert-mode
+           (auto-revert--global-adopt-current-buffer))
+          ((or auto-revert-mode auto-revert-tail-mode)
+           (auto-revert-set-timer))))
 
 (defun auto-revert--polled-buffers ()
   "List of buffers that need to be polled."
diff --git a/test/lisp/autorevert-tests.el b/test/lisp/autorevert-tests.el
index 3c9e04bf19..618923e1bf 100644
--- a/test/lisp/autorevert-tests.el
+++ b/test/lisp/autorevert-tests.el
@@ -542,6 +542,40 @@ auto-revert-test--wait-for-buffer-text
 (auto-revert--deftest-remote auto-revert-test05-global-notify
   "Test `global-auto-revert-mode' without polling for remote buffers.")
 
+(ert-deftest auto-revert-test06-write-file ()
+  "Verify that notification follows `write-file' correctly."
+  :tags '(:expensive-test)
+  (skip-unless (or file-notify--library
+                   (file-remote-p temporary-file-directory)))
+  (let* ((auto-revert-use-notify t)
+         (auto-revert-avoid-polling t)
+         (file-1 (make-temp-file "auto-revert-test"))
+         (file-2 (concat file-1 "-2"))
+         (buf nil))
+    (unwind-protect
+        (progn
+          (setq buf (find-file-noselect file-1))
+          (with-current-buffer buf
+            (insert "A")
+            (save-buffer)
+
+            (auto-revert-mode 1)
+            (should auto-revert-notify-watch-descriptor)
+
+            (insert "B")
+            (write-file file-2)
+
+            (auto-revert-test--write-file "C" file-2)
+            (auto-revert-test--wait-for-buffer-text
+             buf "C" (+ auto-revert-interval 1))
+            (should (equal (buffer-string) "C"))))
+      (ignore-errors (kill-buffer buf))
+      (ignore-errors (delete-file file-1))
+      (ignore-errors (delete-file file-2)))))
+
+(auto-revert--deftest-remote auto-revert-test06-write-file
+  "Test `write-file' in `auto-revert-mode' for remote buffers.")
+
 (defun auto-revert-test-all (&optional interactive)
   "Run all tests for \\[auto-revert]."
   (interactive "p")
-- 
2.20.1 (Apple Git-117)

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

> Auto-revert mode stops working effectively when the user changes the
> buffer file name, by running `write-file' for example, because the
> notifier still tracks the old file.
>
> The attached patch attempts to rectify this.

LGTM. Two minor nits:

> +(defun auto-revert--set-visited-file-name ()

Since we add it to a hook from another package, it shouldn't be marked
internal. Call it `auto-revert-set-visited-file-name'.

> +            (should (equal (buffer-string) "C"))))
> +      (ignore-errors (kill-buffer buf))
> +      (ignore-errors (delete-file file-1))
> +      (ignore-errors (delete-file file-2)))))

In the other tests, we separate the unwindforms from the bodyform (an
empty line, plus a comment). Maybe you could do it here as well.

For me, the last two tests fail in the remote case:

2 unexpected results:
   FAILED  auto-revert-test05-global-notify-remote
   FAILED  auto-revert-test06-write-file-remote

Do you want to check, or shall I do it?

Best regards, Michael.

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Mattias Engdegård]

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

11 juni 2019 kl. 15.15 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> +(defun auto-revert--set-visited-file-name ()
> 
> Since we add it to a hook from another package, it shouldn't be marked
> internal. Call it `auto-revert-set-visited-file-name'.

Done. I actually wanted to add a buffer-local hook, but it turns out that `after-set-visited-file-name-hook', as written, wouldn't work for local hooks -- the very action of `set-visited-file-name' wipes out local hooks, no doubt for good reasons, before they can trigger.

> In the other tests, we separate the unwindforms from the bodyform (an
> empty line, plus a comment). Maybe you could do it here as well.

Done.
I also removed the binding of `auto-revert-avoid-polling' in the new test, since it isn't strictly necessary to exhibit the bug.

>   FAILED  auto-revert-test05-global-notify-remote
>   FAILED  auto-revert-test06-write-file-remote
> 
> Do you want to check, or shall I do it?

Oh, would you do that? I feel bad about causing test failures like this!
The first error may be from a recently committed (f2e4c34de6) fix of an earlier copy-paste mistake; sorry about that. I'm unsure how to deal with these errors myself; I have no inotifywait equivalent on this machine (macOS).

Thanks a lot for your help!


[-- Attachment #2: 0001-Keep-auto-revert-mode-working-when-changing-buffer-f.patch --]
[-- Type: application/octet-stream, Size: 5067 bytes --]

From 4baa9d2f7e704cc722e962a4ab5245157f0ff547 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 10 Jun 2019 17:52:50 +0200
Subject: [PATCH] Keep auto-revert-mode working when changing buffer file name

* lisp/autorevert.el (after-set-visited-file-name-hook):
Add unconditionally.
(global-auto-revert-mode): Don't use
`after-set-visited-file-name-hook' here.
(auto-revert-set-visited-file-name): Rename from
`auto-revert--global-set-visited-file-name' and generalise.
* test/lisp/autorevert-tests.el (auto-revert-test06-write-file): New.
---
 lisp/autorevert.el            | 19 ++++++++++---------
 test/lisp/autorevert-tests.el | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 2de855b303..5c79a7e795 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -347,6 +347,8 @@ auto-revert-find-file-function
 
 (add-hook 'find-file-hook
 	  #'auto-revert-find-file-function)
+(add-hook 'after-set-visited-file-name-hook
+          #'auto-revert-set-visited-file-name)
 
 (defvar auto-revert--buffers-by-watch-descriptor
   (make-hash-table :test 'equal)
@@ -508,8 +510,6 @@ global-auto-revert-mode
             (auto-revert--global-add-current-buffer)))
         ;; Make sure future buffers are added as well.
         (add-hook 'find-file-hook #'auto-revert--global-adopt-current-buffer)
-        (add-hook 'after-set-visited-file-name-hook
-                  #'auto-revert--global-set-visited-file-name)
         ;; To track non-file buffers, we need to listen in to buffer
         ;; creation in general.  Listening to major-mode changes is
         ;; suitable, since we then know whether it's a mode that is tracked.
@@ -520,8 +520,6 @@ global-auto-revert-mode
     ;; Turn global-auto-revert-mode OFF.
     (remove-hook 'after-change-major-mode-hook
                  #'auto-revert--global-adopt-current-buffer)
-    (remove-hook 'after-set-visited-file-name-hook
-                 #'auto-revert--global-set-visited-file-name)
     (remove-hook 'find-file-hook #'auto-revert--global-adopt-current-buffer)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
@@ -551,14 +549,17 @@ auto-revert--global-adopt-current-buffer
   (auto-revert--global-add-current-buffer)
   (auto-revert-set-timer))
 
-(defun auto-revert--global-set-visited-file-name ()
-  "Update Global Auto-Revert management of the current buffer.
+(defun auto-revert-set-visited-file-name ()
+  "Update Auto-Revert management of the current buffer.
 Called after `set-visited-file-name'."
-  ;; Remove any existing notifier first so that we don't track the
-  ;; wrong file in case the file name was changed.
   (when auto-revert-notify-watch-descriptor
+    ;; Remove any existing notifier so that we don't track the wrong
+    ;; file in case the file name was changed.
     (auto-revert-notify-rm-watch))
-  (auto-revert--global-adopt-current-buffer))
+    (cond (global-auto-revert-mode
+           (auto-revert--global-adopt-current-buffer))
+          ((or auto-revert-mode auto-revert-tail-mode)
+           (auto-revert-set-timer))))
 
 (defun auto-revert--polled-buffers ()
   "List of buffers that need to be polled."
diff --git a/test/lisp/autorevert-tests.el b/test/lisp/autorevert-tests.el
index 3c9e04bf19..f21fb864f2 100644
--- a/test/lisp/autorevert-tests.el
+++ b/test/lisp/autorevert-tests.el
@@ -542,6 +542,40 @@ auto-revert-test--wait-for-buffer-text
 (auto-revert--deftest-remote auto-revert-test05-global-notify
   "Test `global-auto-revert-mode' without polling for remote buffers.")
 
+(ert-deftest auto-revert-test06-write-file ()
+  "Verify that notification follows `write-file' correctly."
+  :tags '(:expensive-test)
+  (skip-unless (or file-notify--library
+                   (file-remote-p temporary-file-directory)))
+  (let* ((auto-revert-use-notify t)
+         (file-1 (make-temp-file "auto-revert-test"))
+         (file-2 (concat file-1 "-2"))
+         (buf nil))
+    (unwind-protect
+        (progn
+          (setq buf (find-file-noselect file-1))
+          (with-current-buffer buf
+            (insert "A")
+            (save-buffer)
+
+            (auto-revert-mode 1)
+
+            (insert "B")
+            (write-file file-2)
+
+            (auto-revert-test--write-file "C" file-2)
+            (auto-revert-test--wait-for-buffer-text
+             buf "C" (+ auto-revert-interval 1))
+            (should (equal (buffer-string) "C"))))
+
+      ;; Clean up.
+      (ignore-errors (kill-buffer buf))
+      (ignore-errors (delete-file file-1))
+      (ignore-errors (delete-file file-2)))))
+
+(auto-revert--deftest-remote auto-revert-test06-write-file
+  "Test `write-file' in `auto-revert-mode' for remote buffers.")
+
 (defun auto-revert-test-all (&optional interactive)
   "Run all tests for \\[auto-revert]."
   (interactive "p")
-- 
2.20.1 (Apple Git-117)

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

>> Since we add it to a hook from another package, it shouldn't be marked
>> internal. Call it `auto-revert-set-visited-file-name'.
>
> Done. I actually wanted to add a buffer-local hook, but it turns out
> that `after-set-visited-file-name-hook', as written, wouldn't work for
> local hooks -- the very action of `set-visited-file-name' wipes out
> local hooks, no doubt for good reasons, before they can trigger.
>
>> In the other tests, we separate the unwindforms from the bodyform (an
>> empty line, plus a comment). Maybe you could do it here as well.
>
> Done.
> I also removed the binding of `auto-revert-avoid-polling' in the new
> test, since it isn't strictly necessary to exhibit the bug.

Thanks, looks OK. You could push to master, from my pov.

>>   FAILED  auto-revert-test05-global-notify-remote
>>   FAILED  auto-revert-test06-write-file-remote
>> 
>> Do you want to check, or shall I do it?
>
> Oh, would you do that? I feel bad about causing test failures like this!
> The first error may be from a recently committed (f2e4c34de6) fix of
> an earlier copy-paste mistake; sorry about that. I'm unsure how to
> deal with these errors myself; I have no inotifywait equivalent on
> this machine (macOS).

Sure, will do. I'll start with auto-revert-test05-global-notify-remote.
Once you have pushed, I'll check also auto-revert-test06-write-file-remote.

> Thanks a lot for your help!

Best regards, Michael.

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Mattias Engdegård]

11 juni 2019 kl. 20.02 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Thanks, looks OK. You could push to master, from my pov.

Pushed, thank you. Let's keep the bug open until we know more about the failing tests.

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

> Pushed, thank you. Let's keep the bug open until we know more about
> the failing tests.

I've spent two days making remote autorevert tests succeeding. Although
I could fix some bugs (see the patch for tramp-sh.el yesterday), they
still not run sufficiently. Sometimes they pass, sometimes they fail -
obviously, race conditions. Seems to be related to deletion of files
(and the flow of deleted and stopped events), but I still have no clear
picture.

For the time being I've marked remote autorevert tests as unstable, so
they won't fail in Emacs CI tests. I hope to fix them in the future.

I guess you could close this bug report.

Best regards, Michael.

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Mattias Engdegård]

15 juni 2019 kl. 11.16 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> I've spent two days making remote autorevert tests succeeding. Although
> I could fix some bugs (see the patch for tramp-sh.el yesterday), they
> still not run sufficiently. Sometimes they pass, sometimes they fail -
> obviously, race conditions. Seems to be related to deletion of files
> (and the flow of deleted and stopped events), but I still have no clear
> picture.

I'm so sorry for having put you through all this, and I wish there were anything I could do to make the tests more robust. Does autorevert 'feel' reliable and responsive for remote files when you use it manually, or is there anything that could cast the implementation in doubt?

> For the time being I've marked remote autorevert tests as unstable, so
> they won't fail in Emacs CI tests. I hope to fix them in the future.

Thank you once again; I suppose that will have to do for now.

bug#36159: [PATCH] auto-revert mode doesn't work when changing buffer file name

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

> I'm so sorry for having put you through all this, and I wish there
> were anything I could do to make the tests more robust. Does
> autorevert 'feel' reliable and responsive for remote files when you
> use it manually, or is there anything that could cast the
> implementation in doubt?

Don't worry, that's what tests are good for :-)

The basic autorevert part seems to be OK even for remote files. The
trouble comes when file deletion happens;
auto-revert-test02-auto-revert-deleted-file-remote,
auto-revert-test05-global-notify-remote and
auto-revert-test06-write-file-remote become flaky. That is, sometimes
they pass, and sometimes not. I suppose problems in tramp-sh.el, but I
couldn't nail it down yet.

Likely, the combination of file deletion and autorevert hasn't been
tested thoroughly yet for remote files. OTOH, filenotify tests pass all,
also for remote files.

>> For the time being I've marked remote autorevert tests as unstable, so
>> they won't fail in Emacs CI tests. I hope to fix them in the future.
>
> Thank you once again; I suppose that will have to do for now.

Yes. I believe I give myself a timeout for this, some days. After hours
of unsuccessful debugging, I need a restart in mind for that problem.

Best regards, Michael.