bug#35418: [PATCH] Don't poll auto-revert files that use notification

["Mattias Engdegård" <mattiase@acm.org>]

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

19 maj 2019 kl. 11.12 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Btw, you could always run the four tests on emba.gnu.org. Submit a
> change (also a git branch would do), and the bot on emba.git.org would
> run these test constellations for you. Same for filenotify-tests.

That's good to know -- I'll try that next time.

>>   "Check if auto-revert is active (in current buffer or globally)."
> 
> Remove "or globally".

Done.

>> +(defun auto-revert-test--write-file (string file)
>> +  (write-region string nil file nil 'no-message))
>> +
>> +(defun auto-revert-test--buffer-string (buffer)
>> +  (with-current-buffer buffer
>> +    (buffer-string)))
> 
> Pls add a docstring.

Added.

>> +(ert-deftest auto-revert-test05-global-notify ()
>> +  "Test global-auto-revert-mode without polling."
> 
> Quote `global-auto-revert-mode'. Check also, whether file notification
> is enabled:
> 
> (skip-unless (or file-notify--library
>                 (file-remote-p temporary-file-directory)))

Quoted, and check added.

>> +  (let* ((auto-revert-avoid-polling t)
> 
> Enable file notification explicitly. You don't know, whether the user
> has disabled it.
> 
> (let* ((auto-revert-use-notify t)
>       (auto-revert-avoid-polling t)

Done.

>> +         (auto-revert-interval 2)       ; To speed up the test.
> 
> Do we really want this? I prefer to test the unmodified package. If you
> believe this takes too much time for ordinary "make check" calls, you
> might tag the test case as :expensive-test, like
> auto-revert-test01-auto-revert-several-files and
> auto-revert-test02-auto-revert-deleted-file.

That was probably just me being impatient; I've removed that line and added :expensive-test.

>> +          (global-auto-revert-mode 1)   ; Turn it on.
> 
> Save the value of global-auto-revert-mode, and reset it in Cleanup. You
> don't know the user's settings.

Done.

>> +      (dolist (buf (list buf-1 buf-2 buf-3))
>> +        (when (buffer-live-p buf)
>> +          (kill-buffer buf)))
> 
> Why not (ignore-errors (kill-buffer buf)) ?

Using a condition felt more precise, but I have no strong opinion in this case. Changed to ignore-errors.

> Add the remote test case:
> 
> (auto-revert--deftest-remote auto-revert-test05-global-notify
>  "Test `global-auto-revert-mode' without polling for remote buffers."

Added.

Thanks for the tests and review! Revised patch attached.


[-- Attachment #2: 0001-Avoid-polling-in-global-auto-revert-mode-bug-35418.patch --]
[-- Type: application/octet-stream, Size: 18122 bytes --]

From 2877e205b2fd90fac03b56aa8022d1c7722bfd6f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 9 May 2019 09:40:46 +0200
Subject: [PATCH] Avoid polling in global-auto-revert-mode (bug#35418)

Make `auto-revert-avoid-polling' have effect in global-auto-revert-mode.
Buffers actually handled by that mode are marked with a non-nil value
of `auto-revert--global-mode'.  When global-auto-revert-mode is
entered, eligible buffers are marked in that way, and hooks are set up
to mark new buffers and take care of buffers whose file names change.
This way the existing poll-avoidance logic can be used, since the
entire set of buffers in auto-revert is known.

A new hook, `after-set-visited-file-name-hook', was added to handle
the case when the file name of a tracked buffer changes.

* lisp/autorevert.el (auto-revert-avoid-polling): Amend doc string.
(auto-revert--global-mode): New buffer-local variable.
(global-auto-revert-mode): Mark existing buffers and set up hooks when
mode is entered; do the opposite when exited.
(auto-revert--global-add-current-buffer)
(auto-revert--global-adopt-current-buffer)
(auto-revert--set-visited-file-name-advice): New functions.
(auto-revert--polled-buffers, auto-revert--need-polling-p)
(auto-revert-notify-handler)
(auto-revert-active-p): Modify logic to cover global-auto-revert-mode.
* lisp/files.el (after-set-visited-file-name-hook): New hook.
(set-visited-file-name-hook): Call new hook.
* test/lisp/autorevert-tests.el (top):  Use lexical-binding.
(auto-revert-test--write-file, auto-revert-test--buffer-string)
(auto-revert-test--wait-for, auto-revert-test--wait-for-buffer-text)
(auto-revert-test05-global-notify): New test.
* doc/lispref/hooks.texi (Standard Hooks):
Mention new hook (in a comment, since it's unclear whether it should
actually be documented here)
* etc/NEWS (Changes in Specialized Modes and Packages): Update entry.
---
 doc/lispref/hooks.texi        |   1 +
 etc/NEWS                      |   3 +-
 lisp/autorevert.el            | 124 ++++++++++++++++++++++++++--------
 lisp/files.el                 |   6 +-
 test/lisp/autorevert-tests.el | 107 ++++++++++++++++++++++++++++-
 5 files changed, 210 insertions(+), 31 deletions(-)

diff --git a/doc/lispref/hooks.texi b/doc/lispref/hooks.texi
index 71992464e0..f775aa4d4b 100644
--- a/doc/lispref/hooks.texi
+++ b/doc/lispref/hooks.texi
@@ -251,6 +251,7 @@ I thought did not need to be mentioned here:
 
 Lisp:
 after-load-functions
+after-set-visited-file-name-hook
 auto-coding-functions
 choose-completion-string-functions
 completing-read-function
diff --git a/etc/NEWS b/etc/NEWS
index d70cda179e..9ca98c370e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1443,8 +1443,7 @@ When set to a non-nil value, buffers in Auto Revert mode are no longer
 polled for changes periodically.  This reduces the power consumption
 of an idle Emacs, but may fail on some network file systems; set
 'auto-revert-notify-exclude-dir-regexp' to match files where
-notification is not supported.  The new variable currently has no
-effect in 'global-auto-revert-mode'.  The default value is nil.
+notification is not supported.  The default value is nil.
 
 *** New variable 'buffer-auto-revert-by-notification'
 A major mode can declare that notification on the buffer's default
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 197a2bf157..2de855b303 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -312,10 +312,7 @@ when those files are modified from another computer.
 
 When nil, buffers in Auto-Revert Mode will always be polled for
 changes to their files on disk every `auto-revert-interval'
-seconds, in addition to using notification for those files.
-
-In Global Auto-Revert Mode, polling is always done regardless of
-the value of this variable."
+seconds, in addition to using notification for those files."
   :group 'auto-revert
   :type 'boolean
   :set (lambda (variable value)
@@ -335,6 +332,9 @@ buffers to this list.
 The timer function `auto-revert-buffers' is responsible for purging
 the list of old buffers.")
 
+(defvar-local auto-revert--global-mode nil
+  "Non-nil if buffer is handled by Global Auto-Revert mode.")
+
 (defvar auto-revert-remaining-buffers ()
   "Buffers not checked when user input stopped execution.")
 
@@ -501,34 +501,107 @@ specifies in the mode line."
   :global t :group 'auto-revert :lighter global-auto-revert-mode-text
   (auto-revert-set-timer)
   (if global-auto-revert-mode
-      (auto-revert-buffers)
+      ;; Turn global-auto-revert-mode ON.
+      (progn
+        (dolist (buf (buffer-list))
+          (with-current-buffer buf
+            (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.
+        (when global-auto-revert-non-file-buffers
+          (add-hook 'after-change-major-mode-hook
+                    #'auto-revert--global-adopt-current-buffer))
+        (auto-revert-buffers))
+    ;; 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
-        (when (and auto-revert-notify-watch-descriptor
-                   (not (memq buf auto-revert-buffer-list)))
-	  (auto-revert-notify-rm-watch))))))
+        (when auto-revert--global-mode
+          (setq auto-revert--global-mode nil)
+          (when (and auto-revert-notify-watch-descriptor
+                     (not (or auto-revert-mode auto-revert-tail-mode)))
+	    (auto-revert-notify-rm-watch)))))))
+
+(defun auto-revert--global-add-current-buffer ()
+  "Set current buffer to be tracked by Global Auto-Revert if appropriate."
+  (when (and (not auto-revert--global-mode)
+             (or buffer-file-name
+                 (and global-auto-revert-non-file-buffers
+                      (not (string-prefix-p " " (buffer-name)))
+                      ;; Any non-file buffer must have a custom
+                      ;; `buffer-stale-function' to be tracked, since
+                      ;; we wouldn't know when to revert it otherwise.
+                      (not (eq buffer-stale-function
+                               #'buffer-stale--default-function))))
+             (not (memq 'major-mode global-auto-revert-ignore-modes))
+             (not global-auto-revert-ignore-buffer))
+    (setq auto-revert--global-mode t)))
+
+(defun auto-revert--global-adopt-current-buffer ()
+  "Consider tracking current buffer in a running Global Auto-Revert mode."
+  (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.
+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
+    (auto-revert-notify-rm-watch))
+  (auto-revert--global-adopt-current-buffer))
 
 (defun auto-revert--polled-buffers ()
   "List of buffers that need to be polled."
-  (cond (global-auto-revert-mode (buffer-list))
+  (cond (global-auto-revert-mode
+         (mapcan (lambda (buffer)
+                   (and (not (and auto-revert-avoid-polling
+                                  (buffer-local-value
+                                   'auto-revert-notify-watch-descriptor
+                                   buffer)))
+                        (or (buffer-local-value
+                             'auto-revert--global-mode buffer)
+                            (buffer-local-value 'auto-revert-mode buffer)
+                            (buffer-local-value 'auto-revert-tail-mode buffer))
+                        (list buffer)))
+                 (buffer-list)))
         (auto-revert-avoid-polling
          (mapcan (lambda (buffer)
-                     (and (not (buffer-local-value
-                                'auto-revert-notify-watch-descriptor buffer))
-                          (list buffer)))
-                   auto-revert-buffer-list))
+                   (and (not (buffer-local-value
+                              'auto-revert-notify-watch-descriptor buffer))
+                        (list buffer)))
+                 auto-revert-buffer-list))
         (t auto-revert-buffer-list)))
 
 ;; Same as above in a boolean context, but cheaper.
 (defun auto-revert--need-polling-p ()
   "Whether periodic polling is required."
-  (or global-auto-revert-mode
-      (if auto-revert-avoid-polling
-          (not (cl-every (lambda (buffer)
-                           (buffer-local-value
-                            'auto-revert-notify-watch-descriptor buffer))
-                         auto-revert-buffer-list))
-        auto-revert-buffer-list)))
+  (cond (global-auto-revert-mode
+         (or (not auto-revert-avoid-polling)
+             (cl-some
+              (lambda (buffer)
+                (and (not (buffer-local-value
+                           'auto-revert-notify-watch-descriptor buffer))
+                     (or (buffer-local-value 'auto-revert--global-mode buffer)
+                         (buffer-local-value 'auto-revert-mode buffer)
+                         (buffer-local-value 'auto-revert-tail-mode buffer))))
+              (buffer-list))))
+        (auto-revert-avoid-polling
+         (not (cl-every
+               (lambda (buffer)
+                 (buffer-local-value
+                  'auto-revert-notify-watch-descriptor buffer))
+               auto-revert-buffer-list)))
+        (t auto-revert-buffer-list)))
 
 (defun auto-revert-set-timer ()
   "Restart or cancel the timer used by Auto-Revert Mode.
@@ -652,9 +725,8 @@ system.")
                      (null buffer-file-name))
                 (auto-revert-notify-rm-watch)
                 ;; Restart the timer if it wasn't running.
-                (when (and (memq buffer auto-revert-buffer-list)
-                           (not auto-revert-timer))
-                  (auto-revert-set-timer)))))
+                (unless auto-revert-timer)
+                  (auto-revert-set-timer))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -697,12 +769,10 @@ If the buffer needs to be reverted, do it now."
         (auto-revert-handler)))))
 
 (defun auto-revert-active-p ()
-  "Check if auto-revert is active (in current buffer or globally)."
+  "Check if auto-revert is active in current buffer."
   (or auto-revert-mode
       auto-revert-tail-mode
-      (and global-auto-revert-mode
-           (not global-auto-revert-ignore-buffer)
-           (not (memq major-mode global-auto-revert-ignore-modes)))))
+      auto-revert--global-mode))
 
 (defun auto-revert-handler ()
   "Revert current buffer, if appropriate.
diff --git a/lisp/files.el b/lisp/files.el
index 1dec0ed7ca..287ad14aec 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4269,6 +4269,9 @@ However, the mode will not be changed if
   :type 'boolean
   :group 'editing-basics)
 
+(defvar after-set-visited-file-name-hook nil
+  "Normal hook run just after setting visited file name of current buffer.")
+
 (defun set-visited-file-name (filename &optional no-query along-with-file)
   "Change name of file visited in current buffer to FILENAME.
 This also renames the buffer to correspond to the new file.
@@ -4389,7 +4392,8 @@ the old visited file has been renamed to the new name FILENAME."
 	      (set-auto-mode t)
 	      (or (eq old major-mode)
 		  (hack-local-variables))))
-    (error nil))))
+      (error nil))
+    (run-hooks 'after-set-visited-file-name-hook)))
 
 (defun write-file (filename &optional confirm)
   "Write current buffer into file FILENAME.
diff --git a/test/lisp/autorevert-tests.el b/test/lisp/autorevert-tests.el
index af9edac1be..7c8a83d6ca 100644
--- a/test/lisp/autorevert-tests.el
+++ b/test/lisp/autorevert-tests.el
@@ -1,4 +1,4 @@
-;;; auto-revert-tests.el --- Tests of auto-revert
+;;; auto-revert-tests.el --- Tests of auto-revert   -*- lexical-binding: t -*-
 
 ;; Copyright (C) 2015-2019 Free Software Foundation, Inc.
 
@@ -433,6 +433,111 @@ This expects `auto-revert--messages' to be bound by
 (auto-revert--deftest-remote auto-revert-test04-auto-revert-mode-dired
   "Check remote autorevert for dired.")
 
+(defun auto-revert-test--write-file (string file)
+  "Write STRING to FILE."
+  (write-region string nil file nil 'no-message))
+
+(defun auto-revert-test--buffer-string (buffer)
+  "Contents of BUFFER as a string."
+  (with-current-buffer buffer
+    (buffer-string)))
+
+(defun auto-revert-test--wait-for (pred max-wait)
+  "Wait until PRED is true, or MAX-WAIT seconds elapsed."
+  (let ((ct (current-time)))
+    (while (and (< (float-time (time-subtract (current-time) ct)) max-wait)
+                (not (funcall pred)))
+      (read-event nil nil 0.1))))
+
+(defun auto-revert-test--wait-for-buffer-text (buffer string max-wait)
+  "Wait until BUFFER has the contents STRING, or MAX-WAIT seconds elapsed."
+  (auto-revert-test--wait-for
+   (lambda () (string-equal (auto-revert-test--buffer-string buffer) string))
+   max-wait))
+
+(ert-deftest auto-revert-test05-global-notify ()
+  "Test `global-auto-revert-mode' without polling."
+  :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)
+         (was-in-global-auto-revert-mode global-auto-revert-mode)
+         (file-1 (make-temp-file "global-auto-revert-test-1"))
+         (file-2 (make-temp-file "global-auto-revert-test-2"))
+         (file-3 (make-temp-file "global-auto-revert-test-3"))
+         (file-2b (concat file-2 "-b"))
+         buf-1 buf-2 buf-3)
+    (unwind-protect
+        (progn
+          (setq buf-1 (find-file-noselect file-1))
+          (setq buf-2 (find-file-noselect file-2))
+          (auto-revert-test--write-file "1-a" file-1)
+          (should (equal (auto-revert-test--buffer-string buf-1) ""))
+
+          (global-auto-revert-mode 1)   ; Turn it on.
+
+          (should (buffer-local-value
+                   'auto-revert-notify-watch-descriptor buf-1))
+          (should (buffer-local-value
+                   'auto-revert-notify-watch-descriptor buf-2))
+
+          ;; buf-1 should have been reverted immediately when the mode
+          ;; was enabled.
+          (should (equal (auto-revert-test--buffer-string buf-1) "1-a"))
+
+          ;; Alter a file.
+          (auto-revert-test--write-file "2-a" file-2)
+          ;; Allow for some time to handle notification events.
+          (auto-revert-test--wait-for-buffer-text buf-2 "2-a" 1)
+          (should (equal (auto-revert-test--buffer-string buf-2) "2-a"))
+
+          ;; Visit a file, and modify it on disk.
+          (setq buf-3 (find-file-noselect file-3))
+          ;; Newly opened buffers won't be use notification until the
+          ;; first poll cycle; wait for it.
+          (auto-revert-test--wait-for
+           (lambda () (buffer-local-value
+                       'auto-revert-notify-watch-descriptor buf-3))
+           (+ auto-revert-interval 1))
+          (should (buffer-local-value
+                   'auto-revert-notify-watch-descriptor buf-3))
+          (auto-revert-test--write-file "3-a" file-3)
+          (auto-revert-test--wait-for-buffer-text buf-3 "3-a" 1)
+          (should (equal (auto-revert-test--buffer-string buf-3) "3-a"))
+
+          ;; Delete a visited file, and re-create it with new contents.
+          (delete-file file-1)
+          (sleep-for 0.5)
+          (should (equal (auto-revert-test--buffer-string buf-1) "1-a"))
+          (auto-revert-test--write-file "1-b" file-1)
+          (auto-revert-test--wait-for-buffer-text buf-1 "1-b"
+           (+ auto-revert-interval 1))
+          (should (buffer-local-value
+                   'auto-revert-notify-watch-descriptor buf-1))
+
+          ;; Write a buffer to a new file, then modify the new file on disk.
+          (with-current-buffer buf-2
+            (write-file file-2b))
+          (should (equal (auto-revert-test--buffer-string buf-2) "2-a"))
+          (auto-revert-test--write-file "2-b" file-2b)
+          (auto-revert-test--wait-for-buffer-text buf-2 "2-b"
+           (+ auto-revert-interval 1))
+          (should (buffer-local-value
+                   'auto-revert-notify-watch-descriptor buf-2)))
+
+      ;; Clean up.
+      (unless was-in-global-auto-revert-mode
+        (global-auto-revert-mode 0))    ; Turn it off.
+      (dolist (buf (list buf-1 buf-2 buf-3))
+        (ignore-errors (kill-buffer buf)))
+      (dolist (file (list file-1 file-2 file-2b file-3))
+        (ignore-errors (delete-file file)))
+      )))
+
+(auto-revert--deftest-remote auto-revert-test04-auto-revert-mode-dired
+  "Test `global-auto-revert-mode' without polling for remote buffers.")
+
 (defun auto-revert-test-all (&optional interactive)
   "Run all tests for \\[auto-revert]."
   (interactive "p")
-- 
2.20.1 (Apple Git-117)