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

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

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

13 maj 2019 kl. 17.08 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> The patch LGTM. Do you want also add some tests to autorevert-tests.el?

Thank you -- I added one, but have only access to kqueue at the moment so I haven't run it on anything else.


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

From 75ba64612ed8f990a462b7ea65316e5ff4693f67 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            | 122 ++++++++++++++++++++++++++--------
 lisp/files.el                 |   6 +-
 test/lisp/autorevert-tests.el |  99 ++++++++++++++++++++++++++-
 5 files changed, 201 insertions(+), 30 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 b4aa8d98ff..62ace1ac12 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..56b8a3fed3 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)
@@ -700,9 +772,7 @@ If the buffer needs to be reverted, do it now."
   "Check if auto-revert is active (in current buffer or globally)."
   (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 8cdddf824d..751c2291c8 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.
 
@@ -435,6 +435,103 @@ 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-region string nil file nil 'no-message))
+
+(defun auto-revert-test--buffer-string (buffer)
+  (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."
+  (let* ((auto-revert-avoid-polling t)
+         (auto-revert-interval 2)       ; To speed up the test.
+         (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.
+      (global-auto-revert-mode 0)       ; Turn it off.
+      (dolist (buf (list buf-1 buf-2 buf-3))
+        (when (buffer-live-p buf)
+          (kill-buffer buf)))
+      (dolist (file (list file-1 file-2 file-2b file-3))
+        (ignore-errors (delete-file file)))
+      )))
+
+
 (defun auto-revert-test-all (&optional interactive)
   "Run all tests for \\[auto-revert]."
   (interactive "p")
-- 
2.20.1 (Apple Git-117)