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

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

[Mattias Engdegård]

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

It is a waste of power, on battery-powered devices in particular, to poll files in auto-revert mode periodically when change notification is used. The change is straightforward (attached patch); the main concern is whether the notification system is reliable enough.

In general, it probably is. There is a comment in w32notify.c about SMB-mounted file systems from Samba servers; while Samba does support notification nowadays, there are probably older systems still be deficient in that regard. However, isn't this what `auto-revert-notify-exclude-dir-regexp' is for? I'm not familiar with the way Emacs is used on Windows, but would adding something like

 (rx bos
     (or "\\\\" "//")
     (one-or-more (not (any "/:\\")))
     (any "/\\"))

to `auto-revert-notify-exclude-dir-regexp' be a good start?

Another note about what this patch does not do: global-auto-revert-mode will still use polling. This could be added later on, if there is a good place to hook into for buffer creation.


[-- Attachment #2: 0001-Don-t-poll-auto-revert-files-that-use-notification.patch --]
[-- Type: application/octet-stream, Size: 9323 bytes --]

From 5074c65347736a716f335842206a1e2a2ad36a87 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 24 Apr 2019 18:39:05 +0200
Subject: [PATCH] Don't poll auto-revert files that use notification

It is a waste to periodically poll files that use change notification
in auto-revert mode; stop doing that.  If no files need polling,
turn off the periodic execution entirely to further avoid wasting power.
Use a timer to inhibit immediate reversion for some time after a
notification.

This change does not apply to files in global-auto-revert-mode, where
polling is still necessary.

* lisp/autorevert.el (auto-revert--polled-buffers): New.
(auto-revert-remove-current-buffer, auto-revert-mode)
(global-auto-revert-mode, auto-revert-set-timer)
(auto-revert-notify-add-watch, auto-revert-buffers):
Maintain and use auto-revert--polled-buffers.
(auto-revert-buffers-counter): Remove.
(auto-revert-buffers-counter-lockedout): Remove.
(auto-revert--lockout-interval): New.
(auto-revert--lockout-timer): New.
(auto-revert-notify-handler): Maintain and use auto-revert--polled-buffers.
Honour new lockout timer.  Start lockout timer if necessary.
(auto-revert--end-lockout): New.
---
 lisp/autorevert.el | 84 +++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 1d20896c83..f7b33360ef 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -319,6 +319,11 @@ the list of old buffers.")
 (defvar auto-revert-tail-pos 0
   "Position of last known end of file.")
 
+(defvar auto-revert--polled-buffers ()
+  "List of buffers in Auto-Revert Mode that must be polled.
+It contains the buffers in `auto-revert-buffer-list' whose
+`auto-revert-notify-watch-descriptor' is nil.")
+
 (defun auto-revert-find-file-function ()
   (setq-local auto-revert-tail-pos
               (file-attribute-size (file-attributes buffer-file-name))))
@@ -346,8 +351,12 @@ This has been reported by a file notification event.")
 (defun auto-revert-remove-current-buffer (&optional buffer)
   "Remove BUFFER from `auto-revert-buffer-list'.
 BUFFER defaults to `current-buffer'."
+  (unless buffer
+    (setq buffer (current-buffer)))
   (setq auto-revert-buffer-list
-        (delq (or buffer (current-buffer)) auto-revert-buffer-list)))
+        (delq buffer auto-revert-buffer-list))
+  (setq auto-revert--polled-buffers
+        (delq buffer auto-revert--polled-buffers)))
 
 ;;;###autoload
 (define-minor-mode auto-revert-mode
@@ -367,6 +376,7 @@ without being changed in the part that is already in the buffer."
   (if auto-revert-mode
       (when (not (memq (current-buffer) auto-revert-buffer-list))
         (push (current-buffer) auto-revert-buffer-list)
+        (push (current-buffer) auto-revert--polled-buffers)
         (add-hook
          'kill-buffer-hook
          #'auto-revert-remove-current-buffer
@@ -479,7 +489,8 @@ specifies in the mode line."
       (auto-revert-buffers)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
-	(when auto-revert-notify-watch-descriptor
+        (when (and auto-revert-notify-watch-descriptor
+                   (not (memq buf auto-revert-buffer-list)))
 	  (auto-revert-notify-rm-watch))))))
 
 (defun auto-revert-set-timer ()
@@ -492,7 +503,7 @@ will use an up-to-date value of `auto-revert-interval'"
   (if (timerp auto-revert-timer)
       (cancel-timer auto-revert-timer))
   (setq auto-revert-timer
-	(if (or global-auto-revert-mode auto-revert-buffer-list)
+	(if (or global-auto-revert-mode auto-revert--polled-buffers)
 	    (run-with-timer auto-revert-interval
 			    auto-revert-interval
 			    'auto-revert-buffers))))
@@ -551,6 +562,8 @@ will use an up-to-date value of `auto-revert-interval'"
 	       (gethash auto-revert-notify-watch-descriptor
 		        auto-revert--buffers-by-watch-descriptor))
          auto-revert--buffers-by-watch-descriptor)
+        (setq auto-revert--polled-buffers
+              (delq (current-buffer) auto-revert--polled-buffers))
         (add-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch nil t)))))
 
 ;; If we have file notifications, we want to update the auto-revert buffers
@@ -558,24 +571,20 @@ will use an up-to-date value of `auto-revert-interval'"
 ;; often, we want to skip some revert operations so that we don't spend all our
 ;; time reverting the buffer.
 ;;
-;; We do this by reverting immediately in response to the first in a flurry of
-;; notifications. We suppress subsequent notifications until the next time
-;; `auto-revert-buffers' is called (this happens on a timer with a period set by
-;; `auto-revert-interval').
-(defvar auto-revert-buffers-counter 1
-  "Incremented each time `auto-revert-buffers' is called")
-(defvar-local auto-revert-buffers-counter-lockedout 0
-  "Buffer-local value to indicate whether we should immediately
-update the buffer on a notification event or not. If
-
-  (= auto-revert-buffers-counter-lockedout
-     auto-revert-buffers-counter)
-
-then the updates are locked out, and we wait until the next call
-of `auto-revert-buffers' to revert the buffer. If no lockout is
-present, then we revert immediately and set the lockout, so that
-no more reverts are possible until the next call of
-`auto-revert-buffers'")
+;; We do this by reverting immediately in response to the first in a
+;; flurry of notifications. Any notifications during the following
+;; `auto-revert-lockout-interval' seconds are noted but not acted upon
+;; until the end of that interval.
+
+(defconst auto-revert--lockout-interval 2.5
+  "Duration, in seconds, of the Auto-Revert Mode notification lockout.
+This is the quiescence after each notification of a file being
+changed during which no automatic reverting takes place, to
+prevent many updates in rapid succession from overwhelming the
+system.")
+
+(defvar-local auto-revert--lockout-timer nil
+  "Timer awaiting the end of the notification lockout interval, or nil.")
 
 (defun auto-revert-notify-handler (event)
   "Handle an EVENT returned from file notification."
@@ -604,7 +613,13 @@ no more reverts are possible until the next call of
                            (file-name-nondirectory buffer-file-name)))
                      ;; A buffer w/o a file, like dired.
                      (null buffer-file-name))
-                (auto-revert-notify-rm-watch))))
+                (auto-revert-notify-rm-watch)
+                (when (memq buffer auto-revert-buffer-list)
+                  (unless (memq buffer auto-revert--polled-buffers)
+                    (push buffer auto-revert--polled-buffers))
+                  ;; Restart the timer if it wasn't running.
+                  (unless auto-revert-timer
+                    (auto-revert-set-timer))))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -630,11 +645,21 @@ no more reverts are possible until the next call of
                 (setq auto-revert-notify-modified-p t)
 
                 ;; Revert the buffer now if we're not locked out.
-                (when (/= auto-revert-buffers-counter-lockedout
-                          auto-revert-buffers-counter)
+                (unless auto-revert--lockout-timer
                   (auto-revert-handler)
-                  (setq auto-revert-buffers-counter-lockedout
-                        auto-revert-buffers-counter))))))))))
+                  (setq auto-revert--lockout-timer
+                        (run-with-timer
+                         auto-revert--lockout-interval nil
+                         #'auto-revert--end-lockout buffer)))))))))))
+
+(defun auto-revert--end-lockout (buffer)
+  "End the lockout period after a notification.
+If the buffer needs to be reverted, do it now."
+  (when (buffer-live-p buffer)
+    (with-current-buffer buffer
+      (setq auto-revert--lockout-timer nil)
+      (when auto-revert-notify-modified-p
+        (auto-revert-handler)))))
 
 (defun auto-revert-active-p ()
   "Check if auto-revert is active (in current buffer or globally)."
@@ -755,13 +780,10 @@ This function is also responsible for removing buffers no longer in
 Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
 
-  (setq auto-revert-buffers-counter
-        (1+ auto-revert-buffers-counter))
-
   (save-match-data
     (let ((bufs (if global-auto-revert-mode
 		    (buffer-list)
-		  auto-revert-buffer-list))
+		  auto-revert--polled-buffers))
 	  remaining new)
       ;; Buffers with remote contents shall be reverted only if the
       ;; connection is established already.
@@ -811,7 +833,7 @@ the timer when no buffers need to be checked."
       (setq auto-revert-remaining-buffers bufs)
       ;; Check if we should cancel the timer.
       (when (and (not global-auto-revert-mode)
-		 (null auto-revert-buffer-list))
+		 (null auto-revert--polled-buffers))
         (if (timerp auto-revert-timer)
             (cancel-timer auto-revert-timer))
 	(setq auto-revert-timer nil)))))
-- 
2.20.1 (Apple Git-117)

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 24 Apr 2019 20:14:46 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>
> 
> It is a waste of power, on battery-powered devices in particular, to poll files in auto-revert mode periodically when change notification is used. The change is straightforward (attached patch); the main concern is whether the notification system is reliable enough.

The polling was added for a reason, and the reason was not reliability
of the notifications.  The reason is hinted upon in this comment:

  ;; If we have file notifications, we want to update the auto-revert buffers
  ;; immediately when a notification occurs. Since file updates can happen very
  ;; often, we want to skip some revert operations so that we don't spend all our
  ;; time reverting the buffer.
  ;;
  ;; We do this by reverting immediately in response to the first in a flurry of
  ;; notifications. We suppress subsequent notifications until the next time
  ;; `auto-revert-buffers' is called (this happens on a timer with a period set by
  ;; `auto-revert-interval').

If you look at bug reports and discussions around the time this
comment was written, you will find the descriptions of the use cases
that caused this design.  AFAIR, the main problem was with inotify,
not with w32notify.

> In general, it probably is. There is a comment in w32notify.c about SMB-mounted file systems from Samba servers; while Samba does support notification nowadays, there are probably older systems still be deficient in that regard. However, isn't this what `auto-revert-notify-exclude-dir-regexp' is for? I'm not familiar with the way Emacs is used on Windows, but would adding something like
> 
>  (rx bos
>      (or "\\\\" "//")
>      (one-or-more (not (any "/:\\")))
>      (any "/\\"))
> 
> to `auto-revert-notify-exclude-dir-regexp' be a good start?

If you imply that Samba drives can be identified by the syntax of the
file name alone, then I don't think this is a valid assumption.  A
certain drive letter can be mapped to a Samba volume, and we can never
know that by looking just at the file name.

More generally, auto-revert-notify-exclude-dir-regexp is for any
situation where a filesystem doesn't cause notifications.  You will
find caveats about such issues in the documentation of every
notification system we support.

Thanks.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

> If you look at bug reports and discussions around the time this
> comment was written, you will find the descriptions of the use cases
> that caused this design.  AFAIR, the main problem was with inotify,
> not with w32notify.

Inotify didn't work on mounted directories. I don't know whether this
has improved.

Gfile is agnostic to file systems being mounted or not. If inotify (used
internally) doesn't work, it uses polling. However, gfile has shown
instability; that's why it isn't the first choice anymore.

I don't remember the behavior of kqueue.

>> However, isn't this what `auto-revert-notify-exclude-dir-regexp' is
>> for? I'm not familiar with the way Emacs is used on Windows, but
>> would adding something like
>>
>>  (rx bos
>>      (or "\\\\" "//")
>>      (one-or-more (not (any "/:\\")))
>>      (any "/\\"))
>>
>> to `auto-revert-notify-exclude-dir-regexp' be a good start?
>
> If you imply that Samba drives can be identified by the syntax of the
> file name alone, then I don't think this is a valid assumption.  A
> certain drive letter can be mapped to a Samba volume, and we can never
> know that by looking just at the file name.
>
> More generally, auto-revert-notify-exclude-dir-regexp is for any
> situation where a filesystem doesn't cause notifications.  You will
> find caveats about such issues in the documentation of every
> notification system we support.

The default value of `auto-revert-notify-exclude-dir-regexp' tries to
identify common mount points, like /mnt or /media and alike. But like Eli
said already for Samba mounts, we cannot detect all of them by their
name.

What might be an alternative is to let the user decide. If we provide a
user option `auto-revert-dont-poll', a user could set it to t, and would
live with the consequences. If she tries to enable autorevert for a
mounted directory, which is not covered by file notifications, she might
be surprised.

> Thanks.

Best regards, Michael.

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

[Mattias Engdegård]

24 apr. 2019 kl. 21.36 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Inotify didn't work on mounted directories. I don't know whether this
> has improved.

By 'work', do you mean receiving notification about changes made by the same machine or another machine? As far as I know, the NFS protocol has no means of propagating notifications, in contrast to SMB.

> What might be an alternative is to let the user decide. If we provide a
> user option `auto-revert-dont-poll', a user could set it to t, and would
> live with the consequences. If she tries to enable autorevert for a
> mounted directory, which is not covered by file notifications, she might
> be surprised.

That is a possibility, although I'm generally not too fond of user-adjustable behaviour of this sort. If I understand you right, you propose that the default value should be 'always poll'?
I would prefer the default to be 'avoid polling' -- I doubt that malfunctions will be common or serious if they occur -- but I suppose it is better than the status quo.

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

[Michael Albinus]

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

>> Inotify didn't work on mounted directories. I don't know whether this
>> has improved.
>
> By 'work', do you mean receiving notification about changes made by
> the same machine or another machine? As far as I know, the NFS
> protocol has no means of propagating notifications, in contrast to
> SMB.

It fires notifications for changes made by the same machine:

--8<---------------cut here---------------start------------->8---
;; "/net/ford/albinus" is a mounted directory.
(write-region "foo" nil "/net/ford/albinus/tmp/foo")

(inotify-add-watch "/net/ford/albinus/tmp/foo"
		   t (lambda (event) (message "inotify %S" event)))
=> (3 . 0)

(write-region "foo" nil "/net/ford/albinus/tmp/foo")
=> inotify ((3 . 0) (modify) "/net/ford/albinus/tmp/foo" 0)
   inotify ((3 . 0) (open) "/net/ford/albinus/tmp/foo" 0)
   inotify ((3 . 0) (modify) "/net/ford/albinus/tmp/foo" 0)
   inotify ((3 . 0) (close-write) "/net/ford/albinus/tmp/foo" 0)
--8<---------------cut here---------------end--------------->8---

If I make a modification on the remote machine, nothing happens:

--8<---------------cut here---------------start------------->8---
;; Remote "/ssh:ford:/share/albinus" is the same as local "/net/ford/albinus".
(write-region "foo" nil "/ssh:ford:/share/albinus/tmp/foo")
--8<---------------cut here---------------end--------------->8---

>> What might be an alternative is to let the user decide. If we provide a
>> user option `auto-revert-dont-poll', a user could set it to t, and would
>> live with the consequences. If she tries to enable autorevert for a
>> mounted directory, which is not covered by file notifications, she might
>> be surprised.
>
> That is a possibility, although I'm generally not too fond of
> user-adjustable behaviour of this sort. If I understand you right, you
> propose that the default value should be 'always poll'?

The policy in Emacs is to set the default value to be compatible with
previous behavior. If time passes, and we see no drawback, the default
value could be changed. Usually, the next major E,acs version .

> I would prefer the default to be 'avoid polling' -- I doubt that
> malfunctions will be common or serious if they occur -- but I suppose
> it is better than the status quo.

Yes.

Best regards, Michael.

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

[Mattias Engdegård]

27 apr. 2019 kl. 11.40 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> By 'work', do you mean receiving notification about changes made by
>> the same machine or another machine? As far as I know, the NFS
>> protocol has no means of propagating notifications, in contrast to
>> SMB.
> 
> It fires notifications for changes made by the same machine:
[...]
> If I make a modification on the remote machine, nothing happens:

Thank you for confirming that. The current state of remote notifications on Linux seems to be:

- could possibly be added to cifs in the future
- no explicit support in NFS but could possibly use NFS v4.1 directory delegations
- either requires VFS-level support
- no work in progress as far as I can tell

> The policy in Emacs is to set the default value to be compatible with
> previous behavior. If time passes, and we see no drawback, the default
> value could be changed. Usually, the next major E,acs version .

A most sensible policy, although reality isn't always black or white -- sometimes, a useful change outweighs a minor incompatibility.

Thanks again, I'll prepare a new patch with a defcustom.

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

[Mattias Engdegård]

24 apr. 2019 kl. 20.58 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> The polling was added for a reason, and the reason was not reliability
> of the notifications.  The reason is hinted upon in this comment:
> 
>  ;; If we have file notifications, we want to update the auto-revert buffers
>  ;; immediately when a notification occurs. Since file updates can happen very
>  ;; often, we want to skip some revert operations so that we don't spend all our
>  ;; time reverting the buffer.
>  ;;
>  ;; We do this by reverting immediately in response to the first in a flurry of
>  ;; notifications. We suppress subsequent notifications until the next time
>  ;; `auto-revert-buffers' is called (this happens on a timer with a period set by
>  ;; `auto-revert-interval').

Thank you, interesting! In any case, that should not be a problem: the patch takes care of it in a more principled way, by the means of a timer. Currently, auto-revert is inhibited until next periodic poll, which can be anything between 0 and 5 seconds away. The patch sets this to a fixed value (2.5 s).

> If you look at bug reports and discussions around the time this
> comment was written, you will find the descriptions of the use cases
> that caused this design.  AFAIR, the main problem was with inotify,
> not with w32notify.

The inotify problems at the time seem to have stemmed from not using unique notification descriptors. This was fixed some time ago (158bb8555d etc, bug#26126).

> If you imply that Samba drives can be identified by the syntax of the
> file name alone, then I don't think this is a valid assumption.  A
> certain drive letter can be mapped to a Samba volume, and we can never
> know that by looking just at the file name.

Certainly, but the intent was to add something like the attempts to identify network file systems on Unix machines:

   "^" (regexp-opt '("/afs/" "/media/" "/mnt" "/net/" "/tmp_mnt/"))

If that regexp is acceptable as rough heuristics on Unix, surely something like the regexp proposed, matching \\SOMETHING\, shouldn't be out of the question on Windows? Full precision cannot be attained, as you point out, but perhaps we can make life easier for the user.

> More generally, auto-revert-notify-exclude-dir-regexp is for any
> situation where a filesystem doesn't cause notifications.  You will
> find caveats about such issues in the documentation of every
> notification system we support.

Yes, that is my understanding as well. Are you arguing that the default value of auto-revert-notify-exclude-dir-regexp should not be extended in the proposed way, or that the variable is fundamentally incompatible with the patch?

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 25 Apr 2019 11:56:59 +0200
> Cc: 35418@debbugs.gnu.org, michael.albinus@gmx.de
> 
> >  ;; If we have file notifications, we want to update the auto-revert buffers
> >  ;; immediately when a notification occurs. Since file updates can happen very
> >  ;; often, we want to skip some revert operations so that we don't spend all our
> >  ;; time reverting the buffer.
> >  ;;
> >  ;; We do this by reverting immediately in response to the first in a flurry of
> >  ;; notifications. We suppress subsequent notifications until the next time
> >  ;; `auto-revert-buffers' is called (this happens on a timer with a period set by
> >  ;; `auto-revert-interval').
> 
> Thank you, interesting! In any case, that should not be a problem: the patch takes care of it in a more principled way, by the means of a timer. Currently, auto-revert is inhibited until next periodic poll, which can be anything between 0 and 5 seconds away. The patch sets this to a fixed value (2.5 s).
> 
> > If you look at bug reports and discussions around the time this
> > comment was written, you will find the descriptions of the use cases
> > that caused this design.  AFAIR, the main problem was with inotify,
> > not with w32notify.
> 
> The inotify problems at the time seem to have stemmed from not using unique notification descriptors. This was fixed some time ago (158bb8555d etc, bug#26126).

I'll let Michael decide on this.

>    "^" (regexp-opt '("/afs/" "/media/" "/mnt" "/net/" "/tmp_mnt/"))
> 
> If that regexp is acceptable as rough heuristics on Unix, surely something like the regexp proposed, matching \\SOMETHING\, shouldn't be out of the question on Windows? Full precision cannot be attained, as you point out, but perhaps we can make life easier for the user.

on Windows, SOMETHING is just the name of the machine which exports
the drive, it can be anything.

> Are you arguing that the default value of auto-revert-notify-exclude-dir-regexp should not be extended in the proposed way, or that the variable is fundamentally incompatible with the patch?

I'm questioning the usefulness of extending the default value, yes.
But I don't have strong views on that.

Thanks.

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

[Mattias Engdegård]

25 apr. 2019 kl. 12.04 skrev Eli Zaretskii <eliz@gnu.org>:
> 
>>   "^" (regexp-opt '("/afs/" "/media/" "/mnt" "/net/" "/tmp_mnt/"))
>> 
>> If that regexp is acceptable as rough heuristics on Unix, surely something like the regexp proposed, matching \\SOMETHING\, shouldn't be out of the question on Windows? Full precision cannot be attained, as you point out, but perhaps we can make life easier for the user.
> 
> on Windows, SOMETHING is just the name of the machine which exports
> the drive, it can be anything.

Right, it's just a path to a file or directory on a mounted remote file system.

>> Are you arguing that the default value of auto-revert-notify-exclude-dir-regexp should not be extended in the proposed way, or that the variable is fundamentally incompatible with the patch?
> 
> I'm questioning the usefulness of extending the default value, yes.
> But I don't have strong views on that.

Nor do I. It would avoid breaking auto-revert on some non-auto-revert-capable remote file systems on Windows, at the cost of increasing the auto-revert delay on some auto-revert-capable remote file systems.
The patch does not hinge on a change to the default value of that variable.

I forgot to mention that this patch should and will be accompanied by appropriate NEWS and documentation updates, to be written.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

>> > If you look at bug reports and discussions around the time this
>> > comment was written, you will find the descriptions of the use cases
>> > that caused this design.  AFAIR, the main problem was with inotify,
>> > not with w32notify.
>>
>> The inotify problems at the time seem to have stemmed from not using
>> unique notification descriptors. This was fixed some time ago
>> (158bb8555d etc, bug#26126).
>
> I'll let Michael decide on this.

Well, in inotify you still get undesired notifications. Like this:

--8<---------------cut here---------------start------------->8---
(write-region "foo" nil "/tmp/foo")
(add-name-to-file "/tmp/foo" "/tmp/bar" 'ok)

(inotify-add-watch "/tmp/foo" t (lambda (event) (message "inotify %S" event)))
=> (1 . 0)
(inotify-add-watch "/tmp/bar" t (lambda (event) (message "inotify %S" event)))
=> (1 . 1)


(write-region "foo" nil "/tmp/foo")
=> inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (open) "/tmp/foo" 0)
   inotify ((1 . 1) (open) "/tmp/bar" 0)
   inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (close-write) "/tmp/foo" 0)
   inotify ((1 . 1) (close-write) "/tmp/bar" 0)
--8<---------------cut here---------------end--------------->8---

However, in filenotify this is fixed:

--8<---------------cut here---------------start------------->8---
(file-notify-add-watch "/tmp/foo" '(change attribute-change)
                       (lambda (event) (message "file-notify %S" event)))
=> (2 . 0)
(file-notify-add-watch "/tmp/bar" '(change attribute-change)
                       (lambda (event) (message "file-notify %S" event)))
=> (2 . 1)

(write-region "foo" nil "/tmp/foo")
=> file-notify ((2 . 0) changed "/tmp/foo")
   inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (open) "/tmp/foo" 0)
   inotify ((1 . 1) (open) "/tmp/bar" 0)
   file-notify ((2 . 0) changed "/tmp/foo")
   inotify ((1 . 0) (modify) "/tmp/foo" 0)
   inotify ((1 . 1) (modify) "/tmp/bar" 0)
   inotify ((1 . 0) (close-write) "/tmp/foo" 0)
   inotify ((1 . 1) (close-write) "/tmp/bar" 0)
--8<---------------cut here---------------end--------------->8---

Unrelated events for "/tmp/bar" are filtered out in
`file-notify-callback'. So yes, the inotify problems seem to be fixed.

>> Are you arguing that the default value of
>> auto-revert-notify-exclude-dir-regexp should not be extended in the
>> proposed way, or that the variable is fundamentally incompatible
>> with the patch?
>
> I'm questioning the usefulness of extending the default value, yes.
> But I don't have strong views on that.

We might extend this variable. *If* this regexp matches a file name, we
know that we need polling. But it is clear, that we cannot catch all
cases by just parsing file names.

(Btw, we should use the value of `mounted-file-systems', introduced in
Emacs 26.1, when initializing `auto-revert-notify-exclude-dir-regexp'.)

One alternative approach could be to analyze the file system device
number, as returned by `file-attributes'. By this, we could detect
mounted file systems.

But I don't believe that this information is always trustworty, given it
isn't used anywhere. And at least for remote files it doesn't tell you
anything. Furthermore, mounted file systems are not the only reason that
file notification doesn't work, and we need to poll.

> Thanks.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Mattias Engdegård <mattiase@acm.org>,
>   35418@debbugs.gnu.org
> Date: Sat, 27 Apr 2019 11:27:30 +0200
> 
> One alternative approach could be to analyze the file system device
> number, as returned by `file-attributes'. By this, we could detect
> mounted file systems.

Only on Posix systems, right?

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> One alternative approach could be to analyze the file system device
>> number, as returned by `file-attributes'. By this, we could detect
>> mounted file systems.
>
> Only on Posix systems, right?

Don't know. I haven't used the device number ever myself. And in Tramp,
I set it to a virtual value which is distinct from values used for local
file systems, or from values from other remote connections, w/o any
further checking.

Best regards, Michael.

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

[Mattias Engdegård]

27 apr. 2019 kl. 11.27 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Well, in inotify you still get undesired notifications. Like this:
> 
> --8<---------------cut here---------------start------------->8---
> (write-region "foo" nil "/tmp/foo")
> (add-name-to-file "/tmp/foo" "/tmp/bar" 'ok)
> 
> (inotify-add-watch "/tmp/foo" t (lambda (event) (message "inotify %S" event)))
> => (1 . 0)
> (inotify-add-watch "/tmp/bar" t (lambda (event) (message "inotify %S" event)))
> => (1 . 1)
> (write-region "foo" nil "/tmp/foo")
> => inotify ((1 . 0) (modify) "/tmp/foo" 0)
>   inotify ((1 . 1) (modify) "/tmp/bar" 0)

Thanks for the example!

I wouldn't call this undesired. Create a hard link to a file, ask for notification on both links, and then modify the file. Then both notifiers trigger, because someone has modified the file they were watching. The kqueue back-end behaves similarly.

> However, in filenotify this is fixed:
> 
> --8<---------------cut here---------------start------------->8---
> (file-notify-add-watch "/tmp/foo" '(change attribute-change)
>                       (lambda (event) (message "file-notify %S" event)))
> => (2 . 0)
> (file-notify-add-watch "/tmp/bar" '(change attribute-change)
>                       (lambda (event) (message "file-notify %S" event)))
> => (2 . 1)
> (write-region "foo" nil "/tmp/foo")
> => file-notify ((2 . 0) changed "/tmp/foo")
>   inotify ((1 . 0) (modify) "/tmp/foo" 0)
>   inotify ((1 . 1) (modify) "/tmp/bar" 0)

Actually, it is (arguably) a bug. With two buffers referring to distinct hard links for the same file, surely we want a change in that file to trigger notification for both! (It's quite an exotic case, not the least because Emacs normally recognises hard links as if they were the same file name.)

However, with the kqueue back-end, file-notify watches do trigger for both, as expected.

The reason is that file-notify does not call inotify-add-watch on individual files, as in your example above, but on their containing directory ("/tmp" in your example). When monitoring a directory with two hard links to the same file, and the file is changed, inotify (sensibly) only reports a change to one of the links (the one employed for the change). Thus, the logic is in the Linux kernel, not in filenotify.

For kqueue it is different: here, changes to files are not reported when a watch is monitoring their directory, so filenotify.el sets kqueue watches on each file instead. The same could be done with inotify (and w32notify, if I read the code right), but watching directories has certain advantages.

> Unrelated events for "/tmp/bar" are filtered out in
> `file-notify-callback'. So yes, the inotify problems seem to be fixed.

Are you really sure that the inotify problems were really about changes to files with multiple hard links? It sounds very unlikely, and as showed above, the behaviour differs between back-ends.

If I were to guess, the problem was rather that the inotify back-end used to return the kernel-provided descriptor number, which is the same for the same directory: when /dir/a and /dir/b (distinct files, not hard links) both were watched, inotify would monitor /dir twice and give the same descriptor for both, with the ensuing chaos. This was subsequently fixed by making inotify return unique descriptors.

> We might extend this variable. *If* this regexp matches a file name, we
> know that we need polling. But it is clear, that we cannot catch all
> cases by just parsing file names.
> 
> (Btw, we should use the value of `mounted-file-systems', introduced in
> Emacs 26.1, when initializing `auto-revert-notify-exclude-dir-regexp'.)

That variable contains "^//[^/]+/" on Windows, so we need to make up our minds about it.

> One alternative approach could be to analyze the file system device
> number, as returned by `file-attributes'. By this, we could detect
> mounted file systems.

Sort of; the interpretation is tricky, and as Eli commented, quite platform-specific.

> But I don't believe that this information is always trustworty, given it
> isn't used anywhere. And at least for remote files it doesn't tell you
> anything. Furthermore, mounted file systems are not the only reason that
> file notification doesn't work, and we need to poll.

What other reasons are you thinking about?

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 27 Apr 2019 18:19:36 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 35418@debbugs.gnu.org
> 
> The reason is that file-notify does not call inotify-add-watch on individual files, as in your example above, but on their containing directory ("/tmp" in your example). When monitoring a directory with two hard links to the same file, and the file is changed, inotify (sensibly) only reports a change to one of the links (the one employed for the change). Thus, the logic is in the Linux kernel, not in filenotify.
> 
> For kqueue it is different: here, changes to files are not reported when a watch is monitoring their directory, so filenotify.el sets kqueue watches on each file instead. The same could be done with inotify (and w32notify, if I read the code right), but watching directories has certain advantages.

w32notify cannot watch a single file, because the Windows notification
machinery is directory-oriented, and reports all changes in the
directory.

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

[Mattias Engdegård]

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

27 apr. 2019 kl. 18.52 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> w32notify cannot watch a single file, because the Windows notification
> machinery is directory-oriented, and reports all changes in the
> directory.

Right; thanks for the correction.

Here is an updated patch. There is a new variable, `auto-revert-always-poll', which is t by default.
There is also a note in etc/NEWS. Does it merit a mention in the manual as well?


[-- Attachment #2: 0001-Don-t-poll-auto-revert-files-that-use-notification.patch --]
[-- Type: application/octet-stream, Size: 12155 bytes --]

From 91e073992dfdf8ce557e5679a11fb5f1e733f87e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 24 Apr 2019 18:39:05 +0200
Subject: [PATCH] Don't poll auto-revert files that use notification

It is a waste to periodically poll files that use change notification
in auto-revert mode; stop doing that.  If no files need polling,
turn off the periodic execution entirely to further avoid wasting power.
Use a timer to inhibit immediate reversion for some time after a
notification.

This change does not apply to files in global-auto-revert-mode, where
polling is still necessary.  It is disabled by default, and enabled by
setting `auto-revert-always-poll' to nil.

* lisp/autorevert.el (auto-revert--polled-buffers): New.
(auto-revert-remove-current-buffer, auto-revert-mode)
(global-auto-revert-mode, auto-revert-set-timer)
(auto-revert-notify-add-watch, auto-revert-buffers):
Maintain and use auto-revert--polled-buffers.
(auto-revert-buffers-counter): Remove.
(auto-revert-buffers-counter-lockedout): Remove.
(auto-revert--lockout-interval): New.
(auto-revert--lockout-timer): New.
(auto-revert-notify-handler): Maintain and use auto-revert--polled-buffers.
Honour new lockout timer.  Start lockout timer if necessary.
(auto-revert--end-lockout): New.
(auto-revert-always-poll): New.
(auto-revert--need-polling): New.
* etc/NEWS (Changes in Specialized Modes and Packages): Mention the change.
---
 etc/NEWS           |  10 ++++
 lisp/autorevert.el | 125 +++++++++++++++++++++++++++++++--------------
 2 files changed, 98 insertions(+), 37 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index cf6f4fea3e..84a3184e8c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1389,6 +1389,16 @@ Packages deriving from 'js-mode' with 'define-derived-mode' should
 call this function to add enabled syntax extensions to their mode
 name, too.
 
+** Autorevert
+
+*** New variable 'auto-revert-always-poll' for saving power.
+Set this variable to nil to prevent buffers in auto-revert mode from
+being polled for changes periodically.  This reduces the power
+consumption of an idle Emacs, but may fail on some network file
+systems.  Make sure that 'auto-revert-notify-exclude-dir-regexp'
+matches files where notification is not supported.
+The default value is t.
+
 \f
 * New Modes and Packages in Emacs 27.1
 
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 1d20896c83..ae9ca63577 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -302,6 +302,29 @@ You should set this variable through Custom."
   :type 'regexp
   :version "24.4")
 
+(defcustom auto-revert-always-poll t
+  "Non-nil to poll files even if notification is available.
+
+Set this variable to nil to save power by avoiding polling when
+possible.  Files on file-systems that do not support file
+notifications must match `auto-revert-notify-exclude-dir-regexp'
+for Auto-Revert to work properly in this case.  This typically
+includes network file systems on Unix-like machines, for files
+that are modified from another computer.
+
+When non-nil, buffers in Auto-Revert Mode will always be polled
+for changes to their files on disk every `auto-revert-interval'
+seconds.
+
+In Global Auto-Revert Mode, polling is always done regardless of
+the value of this variable."
+  :group 'auto-revert
+  :type 'boolean
+  :set (lambda (variable value)
+         (set-default variable value)
+         (auto-revert-set-timer))
+  :version "27.1")
+
 ;; Internal variables:
 
 (defvar auto-revert-buffer-list ()
@@ -319,6 +342,11 @@ the list of old buffers.")
 (defvar auto-revert-tail-pos 0
   "Position of last known end of file.")
 
+(defvar auto-revert--polled-buffers ()
+  "List of buffers in Auto-Revert Mode that must be polled.
+It contains the buffers in `auto-revert-buffer-list' whose
+`auto-revert-notify-watch-descriptor' is nil.")
+
 (defun auto-revert-find-file-function ()
   (setq-local auto-revert-tail-pos
               (file-attribute-size (file-attributes buffer-file-name))))
@@ -346,8 +374,12 @@ This has been reported by a file notification event.")
 (defun auto-revert-remove-current-buffer (&optional buffer)
   "Remove BUFFER from `auto-revert-buffer-list'.
 BUFFER defaults to `current-buffer'."
+  (unless buffer
+    (setq buffer (current-buffer)))
   (setq auto-revert-buffer-list
-        (delq (or buffer (current-buffer)) auto-revert-buffer-list)))
+        (delq buffer auto-revert-buffer-list))
+  (setq auto-revert--polled-buffers
+        (delq buffer auto-revert--polled-buffers)))
 
 ;;;###autoload
 (define-minor-mode auto-revert-mode
@@ -367,6 +399,7 @@ without being changed in the part that is already in the buffer."
   (if auto-revert-mode
       (when (not (memq (current-buffer) auto-revert-buffer-list))
         (push (current-buffer) auto-revert-buffer-list)
+        (push (current-buffer) auto-revert--polled-buffers)
         (add-hook
          'kill-buffer-hook
          #'auto-revert-remove-current-buffer
@@ -479,9 +512,17 @@ specifies in the mode line."
       (auto-revert-buffers)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
-	(when auto-revert-notify-watch-descriptor
+        (when (and auto-revert-notify-watch-descriptor
+                   (not (memq buf auto-revert-buffer-list)))
 	  (auto-revert-notify-rm-watch))))))
 
+(defun auto-revert--need-polling ()
+  "Whether periodic polling is required."
+  (or global-auto-revert-mode
+      (if auto-revert-always-poll
+          auto-revert-buffer-list
+        auto-revert--polled-buffers)))
+
 (defun auto-revert-set-timer ()
   "Restart or cancel the timer used by Auto-Revert Mode.
 If such a timer is active, cancel it.  Start a new timer if
@@ -492,10 +533,10 @@ will use an up-to-date value of `auto-revert-interval'"
   (if (timerp auto-revert-timer)
       (cancel-timer auto-revert-timer))
   (setq auto-revert-timer
-	(if (or global-auto-revert-mode auto-revert-buffer-list)
-	    (run-with-timer auto-revert-interval
-			    auto-revert-interval
-			    'auto-revert-buffers))))
+	(and (auto-revert--need-polling)
+	     (run-with-timer auto-revert-interval
+			     auto-revert-interval
+			     'auto-revert-buffers))))
 
 (defun auto-revert-notify-rm-watch ()
   "Disable file notification for current buffer's associated file."
@@ -551,6 +592,8 @@ will use an up-to-date value of `auto-revert-interval'"
 	       (gethash auto-revert-notify-watch-descriptor
 		        auto-revert--buffers-by-watch-descriptor))
          auto-revert--buffers-by-watch-descriptor)
+        (setq auto-revert--polled-buffers
+              (delq (current-buffer) auto-revert--polled-buffers))
         (add-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch nil t)))))
 
 ;; If we have file notifications, we want to update the auto-revert buffers
@@ -558,24 +601,20 @@ will use an up-to-date value of `auto-revert-interval'"
 ;; often, we want to skip some revert operations so that we don't spend all our
 ;; time reverting the buffer.
 ;;
-;; We do this by reverting immediately in response to the first in a flurry of
-;; notifications. We suppress subsequent notifications until the next time
-;; `auto-revert-buffers' is called (this happens on a timer with a period set by
-;; `auto-revert-interval').
-(defvar auto-revert-buffers-counter 1
-  "Incremented each time `auto-revert-buffers' is called")
-(defvar-local auto-revert-buffers-counter-lockedout 0
-  "Buffer-local value to indicate whether we should immediately
-update the buffer on a notification event or not. If
-
-  (= auto-revert-buffers-counter-lockedout
-     auto-revert-buffers-counter)
-
-then the updates are locked out, and we wait until the next call
-of `auto-revert-buffers' to revert the buffer. If no lockout is
-present, then we revert immediately and set the lockout, so that
-no more reverts are possible until the next call of
-`auto-revert-buffers'")
+;; We do this by reverting immediately in response to the first in a
+;; flurry of notifications. Any notifications during the following
+;; `auto-revert-lockout-interval' seconds are noted but not acted upon
+;; until the end of that interval.
+
+(defconst auto-revert--lockout-interval 2.5
+  "Duration, in seconds, of the Auto-Revert Mode notification lockout.
+This is the quiescence after each notification of a file being
+changed during which no automatic reverting takes place, to
+prevent many updates in rapid succession from overwhelming the
+system.")
+
+(defvar-local auto-revert--lockout-timer nil
+  "Timer awaiting the end of the notification lockout interval, or nil.")
 
 (defun auto-revert-notify-handler (event)
   "Handle an EVENT returned from file notification."
@@ -604,7 +643,13 @@ no more reverts are possible until the next call of
                            (file-name-nondirectory buffer-file-name)))
                      ;; A buffer w/o a file, like dired.
                      (null buffer-file-name))
-                (auto-revert-notify-rm-watch))))
+                (auto-revert-notify-rm-watch)
+                (when (memq buffer auto-revert-buffer-list)
+                  (unless (memq buffer auto-revert--polled-buffers)
+                    (push buffer auto-revert--polled-buffers))
+                  ;; Restart the timer if it wasn't running.
+                  (unless auto-revert-timer
+                    (auto-revert-set-timer))))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -630,11 +675,21 @@ no more reverts are possible until the next call of
                 (setq auto-revert-notify-modified-p t)
 
                 ;; Revert the buffer now if we're not locked out.
-                (when (/= auto-revert-buffers-counter-lockedout
-                          auto-revert-buffers-counter)
+                (unless auto-revert--lockout-timer
                   (auto-revert-handler)
-                  (setq auto-revert-buffers-counter-lockedout
-                        auto-revert-buffers-counter))))))))))
+                  (setq auto-revert--lockout-timer
+                        (run-with-timer
+                         auto-revert--lockout-interval nil
+                         #'auto-revert--end-lockout buffer)))))))))))
+
+(defun auto-revert--end-lockout (buffer)
+  "End the lockout period after a notification.
+If the buffer needs to be reverted, do it now."
+  (when (buffer-live-p buffer)
+    (with-current-buffer buffer
+      (setq auto-revert--lockout-timer nil)
+      (when auto-revert-notify-modified-p
+        (auto-revert-handler)))))
 
 (defun auto-revert-active-p ()
   "Check if auto-revert is active (in current buffer or globally)."
@@ -755,13 +810,10 @@ This function is also responsible for removing buffers no longer in
 Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
 
-  (setq auto-revert-buffers-counter
-        (1+ auto-revert-buffers-counter))
-
   (save-match-data
-    (let ((bufs (if global-auto-revert-mode
-		    (buffer-list)
-		  auto-revert-buffer-list))
+    (let ((bufs (cond (global-auto-revert-mode (buffer-list))
+                      (auto-revert-always-poll auto-revert-buffer-list)
+                      (t auto-revert--polled-buffers)))
 	  remaining new)
       ;; Buffers with remote contents shall be reverted only if the
       ;; connection is established already.
@@ -810,8 +862,7 @@ the timer when no buffers need to be checked."
 	(setq bufs (cdr bufs)))
       (setq auto-revert-remaining-buffers bufs)
       ;; Check if we should cancel the timer.
-      (when (and (not global-auto-revert-mode)
-		 (null auto-revert-buffer-list))
+      (unless (auto-revert--need-polling)
         (if (timerp auto-revert-timer)
             (cancel-timer auto-revert-timer))
 	(setq auto-revert-timer nil)))))
-- 
2.20.1 (Apple Git-117)

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

[Michael Albinus]

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

Hi Mattias,

> Here is an updated patch. There is a new variable,
> `auto-revert-always-poll', which is t by default.
> There is also a note in etc/NEWS. Does it merit a mention in the manual as well?

Yes, please.

> +(defcustom auto-revert-always-poll t
> +  "Non-nil to poll files even if notification is available.
> +
> +Set this variable to nil to save power by avoiding polling when
> +possible.  Files on file-systems that do not support file
> +notifications must match `auto-revert-notify-exclude-dir-regexp'
> +for Auto-Revert to work properly in this case.  This typically
> +includes network file systems on Unix-like machines, for files
> +that are modified from another computer.
> +
> +When non-nil, buffers in Auto-Revert Mode will always be polled
> +for changes to their files on disk every `auto-revert-interval'
> +seconds.
> +
> +In Global Auto-Revert Mode, polling is always done regardless of
> +the value of this variable."

I believe it shall be said, that this user option does not compete with
`auto-revert-use-notify'. Rather, polling is used additionally to file
notification. When `auto-revert-use-notify' is nil, the value of
`auto-revert-always-poll' doesn't matter; there will always be polling.

Saying this, the user option might need another name. What about
`auto-revert-also-poll'?

> +(defvar auto-revert--polled-buffers ()
> +  "List of buffers in Auto-Revert Mode that must be polled.
> +It contains the buffers in `auto-revert-buffer-list' whose
> +`auto-revert-notify-watch-descriptor' is nil.")

Is this variable needed? It is used only once in
`auto-revert--need-polling', and it could be computed easily by
(untested)

(delq nil
  (mapcar (lambda (buf)
            (and (or auto-revert-always-poll
                     (not auto-revert-notify-watch-descriptor))
                 buf))
          auto-revert-buffer-list))

`auto-revert--need-polling' shall always return the buffer list, also for
`global-auto-revert-mode'.

Otherwise, the patch might work. Let's try it.

Best regards, Michael.

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

[Mattias Engdegård]

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

29 apr. 2019 kl. 09.53 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> Here is an updated patch. There is a new variable,
>> `auto-revert-always-poll', which is t by default.
>> There is also a note in etc/NEWS. Does it merit a mention in the manual as well?
> 
> Yes, please.

There is now a paragraph added to the manual.

By the way, the organisation of this part of the manual could be improved -- don't you agree?

There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.

Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.

This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.

> I believe it shall be said, that this user option does not compete with
> `auto-revert-use-notify'. Rather, polling is used additionally to file
> notification. When `auto-revert-use-notify' is nil, the value of
> `auto-revert-always-poll' doesn't matter; there will always be polling.

Good point; the doc string has been clarified.

> Saying this, the user option might need another name. What about
> `auto-revert-also-poll'?

Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.

>> +(defvar auto-revert--polled-buffers ()
>> +  "List of buffers in Auto-Revert Mode that must be polled.
>> +It contains the buffers in `auto-revert-buffer-list' whose
>> +`auto-revert-notify-watch-descriptor' is nil.")
> 
> Is this variable needed? It is used only once in
> `auto-revert--need-polling', and it could be computed easily by

It is also used in `auto-revert-buffers', but you are quite right that it could be a function. I decided to maintain it as a derived state because it felt silly to replace O(1) code with O(N), and the invariant is clear enough (stated in its doc string). (Some of the places where the variable is updated are O(N) but less frequently executed.)

I can replace it with a function if you want, but the code didn't seem to gain much from doing so.

> `auto-revert--need-polling' shall always return the buffer list, also for
> `global-auto-revert-mode'.

Sorry, it was meant as a predicate and is only used as such.
Clarified by renaming it to `auto-revert--need-polling-p'.

Thank you very much for your review! Updated patch attached.


[-- Attachment #2: 0001-Don-t-poll-auto-revert-files-that-use-notification.patch --]
[-- Type: application/octet-stream, Size: 13578 bytes --]

From 17a48cc4a106112830b1399fff2966bd16b8c23c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 24 Apr 2019 18:39:05 +0200
Subject: [PATCH] Don't poll auto-revert files that use notification

It is a waste to periodically poll files that use change notification
in auto-revert mode; stop doing that.  If no files need polling,
turn off the periodic execution entirely to further avoid wasting power.
Use a timer to inhibit immediate reversion for some time after a
notification.

This change does not apply to files in global-auto-revert-mode, where
polling is still necessary.  It is disabled by default, and enabled by
setting `auto-revert-always-poll' to nil.

* lisp/autorevert.el
(auto-revert--polled-buffers, auto-revert--lockout-interval,
auto-revert--lockout-timer, auto-revert--end-lockout, auto-revert-always-poll,
auto-revert--need-polling-p): New.
(auto-revert-remove-current-buffer, auto-revert-mode,
global-auto-revert-mode, auto-revert-set-timer,
auto-revert-notify-add-watch, auto-revert-buffers,
auto-revert-notify-handler): Maintain and use auto-revert--polled-buffers.
Honour new lockout timer.  Start lockout timer if necessary.
Maintain and use auto-revert--polled-buffers.
(auto-revert-buffers-counter, auto-revert-buffers-counter-lockedout): Remove.

* etc/NEWS (Changes in Specialized Modes and Packages): Mention the change.

* doc/emacs/files.texi (Reverting): Add paragraph describing
auto-revert-always-poll.
---
 doc/emacs/files.texi |  13 +++++
 etc/NEWS             |  10 ++++
 lisp/autorevert.el   | 125 ++++++++++++++++++++++++++++++-------------
 3 files changed, 111 insertions(+), 37 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index a57428230c..2dd48503b4 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -988,6 +988,19 @@ the polling interval through the variable @code{auto-revert-interval}.
 supported, @code{auto-revert-use-notify} will be @code{nil} by
 default.
 
+@vindex auto-revert-always-poll
+@vindex auto-revert-notify-exclude-dir-regexp
+  By default, Auto-Revert mode will poll files for changes
+periodically even when file notifications are used.  Such polling is
+usually unnecessary, and turning it off may save power by relying on
+notifications only.  To do so, set the variable
+@code{auto-revert-always-poll} to @code{nil}.  However, notification
+is ineffective on certain file systems; mainly network file system on
+Unix-like machines, where files can be altered from other machines.
+To force polling when @code{auto-revert-always-poll} is @code{nil},
+set @code{auto-revert-notify-exclude-dir-regexp} to match files that
+should be excluded from using notification.
+
   One use of Auto-Revert mode is to ``tail'' a file such as a system
 log, so that changes made to that file by other programs are
 continuously displayed.  To do this, just move the point to the end of
diff --git a/etc/NEWS b/etc/NEWS
index 9b32d720b6..cf997aa0c8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1389,6 +1389,16 @@ Packages deriving from 'js-mode' with 'define-derived-mode' should
 call this function to add enabled syntax extensions to their mode
 name, too.
 
+** Autorevert
+
+*** New variable 'auto-revert-always-poll' for saving power.
+Set this variable to nil to prevent buffers in auto-revert mode from
+being polled for changes periodically.  This reduces the power
+consumption of an idle Emacs, but may fail on some network file
+systems.  Make sure that 'auto-revert-notify-exclude-dir-regexp'
+matches files where notification is not supported.
+The default value is t.
+
 \f
 * New Modes and Packages in Emacs 27.1
 
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 1d20896c83..b16b1b5833 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -302,6 +302,29 @@ You should set this variable through Custom."
   :type 'regexp
   :version "24.4")
 
+(defcustom auto-revert-always-poll t
+  "Non-nil to poll files in addition to the use of notification.
+
+Set this variable to nil to save power by avoiding polling when
+possible.  Files on file-systems that do not support change
+notifications must match `auto-revert-notify-exclude-dir-regexp'
+for Auto-Revert to work properly in this case.  This typically
+includes files on network file systems on Unix-like machines,
+when those files are modified from another computer.
+
+When non-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."
+  :group 'auto-revert
+  :type 'boolean
+  :set (lambda (variable value)
+         (set-default variable value)
+         (auto-revert-set-timer))
+  :version "27.1")
+
 ;; Internal variables:
 
 (defvar auto-revert-buffer-list ()
@@ -319,6 +342,11 @@ the list of old buffers.")
 (defvar auto-revert-tail-pos 0
   "Position of last known end of file.")
 
+(defvar auto-revert--polled-buffers ()
+  "List of buffers in Auto-Revert Mode that must be polled.
+It contains the buffers in `auto-revert-buffer-list' whose
+`auto-revert-notify-watch-descriptor' is nil.")
+
 (defun auto-revert-find-file-function ()
   (setq-local auto-revert-tail-pos
               (file-attribute-size (file-attributes buffer-file-name))))
@@ -346,8 +374,12 @@ This has been reported by a file notification event.")
 (defun auto-revert-remove-current-buffer (&optional buffer)
   "Remove BUFFER from `auto-revert-buffer-list'.
 BUFFER defaults to `current-buffer'."
+  (unless buffer
+    (setq buffer (current-buffer)))
   (setq auto-revert-buffer-list
-        (delq (or buffer (current-buffer)) auto-revert-buffer-list)))
+        (delq buffer auto-revert-buffer-list))
+  (setq auto-revert--polled-buffers
+        (delq buffer auto-revert--polled-buffers)))
 
 ;;;###autoload
 (define-minor-mode auto-revert-mode
@@ -367,6 +399,7 @@ without being changed in the part that is already in the buffer."
   (if auto-revert-mode
       (when (not (memq (current-buffer) auto-revert-buffer-list))
         (push (current-buffer) auto-revert-buffer-list)
+        (push (current-buffer) auto-revert--polled-buffers)
         (add-hook
          'kill-buffer-hook
          #'auto-revert-remove-current-buffer
@@ -479,9 +512,17 @@ specifies in the mode line."
       (auto-revert-buffers)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
-	(when auto-revert-notify-watch-descriptor
+        (when (and auto-revert-notify-watch-descriptor
+                   (not (memq buf auto-revert-buffer-list)))
 	  (auto-revert-notify-rm-watch))))))
 
+(defun auto-revert--need-polling-p ()
+  "Whether periodic polling is required."
+  (or global-auto-revert-mode
+      (if auto-revert-always-poll
+          auto-revert-buffer-list
+        auto-revert--polled-buffers)))
+
 (defun auto-revert-set-timer ()
   "Restart or cancel the timer used by Auto-Revert Mode.
 If such a timer is active, cancel it.  Start a new timer if
@@ -492,10 +533,10 @@ will use an up-to-date value of `auto-revert-interval'"
   (if (timerp auto-revert-timer)
       (cancel-timer auto-revert-timer))
   (setq auto-revert-timer
-	(if (or global-auto-revert-mode auto-revert-buffer-list)
-	    (run-with-timer auto-revert-interval
-			    auto-revert-interval
-			    'auto-revert-buffers))))
+	(and (auto-revert--need-polling-p)
+	     (run-with-timer auto-revert-interval
+			     auto-revert-interval
+			     'auto-revert-buffers))))
 
 (defun auto-revert-notify-rm-watch ()
   "Disable file notification for current buffer's associated file."
@@ -551,6 +592,8 @@ will use an up-to-date value of `auto-revert-interval'"
 	       (gethash auto-revert-notify-watch-descriptor
 		        auto-revert--buffers-by-watch-descriptor))
          auto-revert--buffers-by-watch-descriptor)
+        (setq auto-revert--polled-buffers
+              (delq (current-buffer) auto-revert--polled-buffers))
         (add-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch nil t)))))
 
 ;; If we have file notifications, we want to update the auto-revert buffers
@@ -558,24 +601,20 @@ will use an up-to-date value of `auto-revert-interval'"
 ;; often, we want to skip some revert operations so that we don't spend all our
 ;; time reverting the buffer.
 ;;
-;; We do this by reverting immediately in response to the first in a flurry of
-;; notifications. We suppress subsequent notifications until the next time
-;; `auto-revert-buffers' is called (this happens on a timer with a period set by
-;; `auto-revert-interval').
-(defvar auto-revert-buffers-counter 1
-  "Incremented each time `auto-revert-buffers' is called")
-(defvar-local auto-revert-buffers-counter-lockedout 0
-  "Buffer-local value to indicate whether we should immediately
-update the buffer on a notification event or not. If
-
-  (= auto-revert-buffers-counter-lockedout
-     auto-revert-buffers-counter)
-
-then the updates are locked out, and we wait until the next call
-of `auto-revert-buffers' to revert the buffer. If no lockout is
-present, then we revert immediately and set the lockout, so that
-no more reverts are possible until the next call of
-`auto-revert-buffers'")
+;; We do this by reverting immediately in response to the first in a
+;; flurry of notifications. Any notifications during the following
+;; `auto-revert-lockout-interval' seconds are noted but not acted upon
+;; until the end of that interval.
+
+(defconst auto-revert--lockout-interval 2.5
+  "Duration, in seconds, of the Auto-Revert Mode notification lockout.
+This is the quiescence after each notification of a file being
+changed during which no automatic reverting takes place, to
+prevent many updates in rapid succession from overwhelming the
+system.")
+
+(defvar-local auto-revert--lockout-timer nil
+  "Timer awaiting the end of the notification lockout interval, or nil.")
 
 (defun auto-revert-notify-handler (event)
   "Handle an EVENT returned from file notification."
@@ -604,7 +643,13 @@ no more reverts are possible until the next call of
                            (file-name-nondirectory buffer-file-name)))
                      ;; A buffer w/o a file, like dired.
                      (null buffer-file-name))
-                (auto-revert-notify-rm-watch))))
+                (auto-revert-notify-rm-watch)
+                (when (memq buffer auto-revert-buffer-list)
+                  (unless (memq buffer auto-revert--polled-buffers)
+                    (push buffer auto-revert--polled-buffers))
+                  ;; Restart the timer if it wasn't running.
+                  (unless auto-revert-timer
+                    (auto-revert-set-timer))))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -630,11 +675,21 @@ no more reverts are possible until the next call of
                 (setq auto-revert-notify-modified-p t)
 
                 ;; Revert the buffer now if we're not locked out.
-                (when (/= auto-revert-buffers-counter-lockedout
-                          auto-revert-buffers-counter)
+                (unless auto-revert--lockout-timer
                   (auto-revert-handler)
-                  (setq auto-revert-buffers-counter-lockedout
-                        auto-revert-buffers-counter))))))))))
+                  (setq auto-revert--lockout-timer
+                        (run-with-timer
+                         auto-revert--lockout-interval nil
+                         #'auto-revert--end-lockout buffer)))))))))))
+
+(defun auto-revert--end-lockout (buffer)
+  "End the lockout period after a notification.
+If the buffer needs to be reverted, do it now."
+  (when (buffer-live-p buffer)
+    (with-current-buffer buffer
+      (setq auto-revert--lockout-timer nil)
+      (when auto-revert-notify-modified-p
+        (auto-revert-handler)))))
 
 (defun auto-revert-active-p ()
   "Check if auto-revert is active (in current buffer or globally)."
@@ -755,13 +810,10 @@ This function is also responsible for removing buffers no longer in
 Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
 
-  (setq auto-revert-buffers-counter
-        (1+ auto-revert-buffers-counter))
-
   (save-match-data
-    (let ((bufs (if global-auto-revert-mode
-		    (buffer-list)
-		  auto-revert-buffer-list))
+    (let ((bufs (cond (global-auto-revert-mode (buffer-list))
+                      (auto-revert-always-poll auto-revert-buffer-list)
+                      (t auto-revert--polled-buffers)))
 	  remaining new)
       ;; Buffers with remote contents shall be reverted only if the
       ;; connection is established already.
@@ -810,8 +862,7 @@ the timer when no buffers need to be checked."
 	(setq bufs (cdr bufs)))
       (setq auto-revert-remaining-buffers bufs)
       ;; Check if we should cancel the timer.
-      (when (and (not global-auto-revert-mode)
-		 (null auto-revert-buffer-list))
+      (unless (auto-revert--need-polling-p)
         (if (timerp auto-revert-timer)
             (cancel-timer auto-revert-timer))
 	(setq auto-revert-timer nil)))))
-- 
2.20.1 (Apple Git-117)

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

[Michael Albinus]

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

> 29 apr. 2019 kl. 09.53 skrev Michael Albinus <michael.albinus@gmx.de>:
>> 
>>> Here is an updated patch. There is a new variable,
>>> `auto-revert-always-poll', which is t by default.
>>> There is also a note in etc/NEWS. Does it merit a mention in the
>>> manual as well?
>> 
>> Yes, please.
>
> There is now a paragraph added to the manual.
>
> By the way, the organisation of this part of the manual could be
> improved -- don't you agree?

I hardly disagree, this is always true :-)

> There is a section called Reverting, which starts about
> `revert-buffer' but then goes on to talk about the auto-revert,
> global-auto-revert and auto-revert-tail modes and details about the
> mechanisms behind them: polling, intervals, notification.
>
> Then there is a (sibling) section called Autorevert, which despite its
> name only talks about auto-reverting non-file buffers.
>
> This can be reorganised in various ways. We could move all autorevert
> text to a sibling node to Reverting, or to one or more child nodes. In
> any case, such text shuffling should not be part of this patch.

I would let it for you.

>> Saying this, the user option might need another name. What about
>> `auto-revert-also-poll'?
>
> Naming is always hard. I started with `auto-revert-avoid-polling' but
> wanted to avoid a negative name.
> I tried `auto-revert-also-poll' but it somehow didn't feel right; not
> all buffers use notification.
> It is nothing I feel strongly about, so if you do prefer that name
> I'll change, but I've kept the original name in the patch for now.

I have also hard times when choosing a proper name. Do what you believe
is best suited, unless Eli comes with something better. (It is my
experience over years, that he beats me always with better names.)

>> Is this variable needed? It is used only once in
>> `auto-revert--need-polling', and it could be computed easily by
>
> It is also used in `auto-revert-buffers', but you are quite right that
> it could be a function.

Yes, but the function as proposed would fit as well.

> I decided to maintain it as a derived state
> because it felt silly to replace O(1) code with O(N), and the
> invariant is clear enough (stated in its doc string). (Some of the
> places where the variable is updated are O(N) but less frequently
> executed.)

Yes, but is N large enough to experience the difference?

> I can replace it with a function if you want, but the code didn't seem
> to gain much from doing so.

There are several places you need to modify the variable. This gave me
the impression that one function would fit better, because if you need
to touch (set) avariable at several places, there are good chances to
miss it somewhere. I'm not saying you do in this case, it is just my
style to keep things together (in one function, for example).

>> `auto-revert--need-polling' shall always return the buffer list, also for
>> `global-auto-revert-mode'.
>
> Sorry, it was meant as a predicate and is only used as such.
> Clarified by renaming it to `auto-revert--need-polling-p'.

My proposal was to use it NOT as a predicate, but as a function
returning the buffer list.

> Thank you very much for your review! Updated patch attached.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  35418@debbugs.gnu.org
> Date: Mon, 29 Apr 2019 14:18:20 +0200
> 
> > Naming is always hard. I started with `auto-revert-avoid-polling' but
> > wanted to avoid a negative name.
> > I tried `auto-revert-also-poll' but it somehow didn't feel right; not
> > all buffers use notification.
> > It is nothing I feel strongly about, so if you do prefer that name
> > I'll change, but I've kept the original name in the patch for now.
> 
> I have also hard times when choosing a proper name. Do what you believe
> is best suited, unless Eli comes with something better. (It is my
> experience over years, that he beats me always with better names.)

Really?  I actually consider myself being bad with naming.  Let's see
if you like my suggestion this time.

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

[Mattias Engdegård]

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

29 apr. 2019 kl. 14.18 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> I decided to maintain it as a derived state
>> because it felt silly to replace O(1) code with O(N), and the
>> invariant is clear enough (stated in its doc string). (Some of the
>> places where the variable is updated are O(N) but less frequently
>> executed.)
> 
> Yes, but is N large enough to experience the difference?

These things are tricky to measure, but obviously inefficient code just doesn't feel right to write. For example, generating a list just to see if it is non-empty, when that could be determined in a more straightforward way.

> My proposal was to use it NOT as a predicate, but as a function
> returning the buffer list.

Very well; here is an incremental patch (to make the differences clear). It's a compromise: the derived state is gone, but there are two functions: one for the list of buffers that need to be polled, and one for whether that list would be non-empty.

By the way, the patch now uses functions from cl-lib, not just macros. Is there any reason not to?


[-- Attachment #2: 0001-Eliminate-the-auto-revert-polled-buffers-variable.patch --]
[-- Type: application/octet-stream, Size: 5518 bytes --]

From e3fe91d6b760b8325c352e6220997f569f7f2d44 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 29 Apr 2019 20:21:32 +0200
Subject: [PATCH] Eliminate the auto-revert--polled-buffers variable

Instead of maintaining the `auto-revert--polled-buffers' variable,
calculate it when needed.  When only its emptiness is of interest,
do so in a more efficient way.
---
 lisp/autorevert.el | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index b16b1b5833..6387ef1e92 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -107,7 +107,7 @@
 
 ;; Dependencies:
 
-(eval-when-compile (require 'cl-lib))
+(require 'cl-lib)
 (require 'timer)
 (require 'filenotify)
 
@@ -342,11 +342,6 @@ the list of old buffers.")
 (defvar auto-revert-tail-pos 0
   "Position of last known end of file.")
 
-(defvar auto-revert--polled-buffers ()
-  "List of buffers in Auto-Revert Mode that must be polled.
-It contains the buffers in `auto-revert-buffer-list' whose
-`auto-revert-notify-watch-descriptor' is nil.")
-
 (defun auto-revert-find-file-function ()
   (setq-local auto-revert-tail-pos
               (file-attribute-size (file-attributes buffer-file-name))))
@@ -374,12 +369,8 @@ This has been reported by a file notification event.")
 (defun auto-revert-remove-current-buffer (&optional buffer)
   "Remove BUFFER from `auto-revert-buffer-list'.
 BUFFER defaults to `current-buffer'."
-  (unless buffer
-    (setq buffer (current-buffer)))
   (setq auto-revert-buffer-list
-        (delq buffer auto-revert-buffer-list))
-  (setq auto-revert--polled-buffers
-        (delq buffer auto-revert--polled-buffers)))
+        (delq (or buffer (current-buffer)) auto-revert-buffer-list)))
 
 ;;;###autoload
 (define-minor-mode auto-revert-mode
@@ -399,7 +390,6 @@ without being changed in the part that is already in the buffer."
   (if auto-revert-mode
       (when (not (memq (current-buffer) auto-revert-buffer-list))
         (push (current-buffer) auto-revert-buffer-list)
-        (push (current-buffer) auto-revert--polled-buffers)
         (add-hook
          'kill-buffer-hook
          #'auto-revert-remove-current-buffer
@@ -516,12 +506,26 @@ specifies in the mode line."
                    (not (memq buf auto-revert-buffer-list)))
 	  (auto-revert-notify-rm-watch))))))
 
+(defun auto-revert--polled-buffers ()
+  "List of buffers that need to be polled."
+  (cond (global-auto-revert-mode (buffer-list))
+        (auto-revert-always-poll auto-revert-buffer-list)
+        (t (mapcan (lambda (buffer)
+                     (and (not (buffer-local-value
+                                'auto-revert-notify-watch-descriptor buffer))
+                          (list buffer)))
+                   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-always-poll
           auto-revert-buffer-list
-        auto-revert--polled-buffers)))
+        (not (cl-every (lambda (buffer)
+                         (buffer-local-value
+                          'auto-revert-notify-watch-descriptor buffer))
+                       auto-revert-buffer-list)))))
 
 (defun auto-revert-set-timer ()
   "Restart or cancel the timer used by Auto-Revert Mode.
@@ -592,8 +596,6 @@ will use an up-to-date value of `auto-revert-interval'"
 	       (gethash auto-revert-notify-watch-descriptor
 		        auto-revert--buffers-by-watch-descriptor))
          auto-revert--buffers-by-watch-descriptor)
-        (setq auto-revert--polled-buffers
-              (delq (current-buffer) auto-revert--polled-buffers))
         (add-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch nil t)))))
 
 ;; If we have file notifications, we want to update the auto-revert buffers
@@ -644,12 +646,10 @@ system.")
                      ;; A buffer w/o a file, like dired.
                      (null buffer-file-name))
                 (auto-revert-notify-rm-watch)
-                (when (memq buffer auto-revert-buffer-list)
-                  (unless (memq buffer auto-revert--polled-buffers)
-                    (push buffer auto-revert--polled-buffers))
-                  ;; Restart the timer if it wasn't running.
-                  (unless auto-revert-timer
-                    (auto-revert-set-timer))))))
+                ;; Restart the timer if it wasn't running.
+                (when (and (memq buffer auto-revert-buffer-list)
+                           (not auto-revert-timer))
+                  (auto-revert-set-timer)))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -811,9 +811,7 @@ Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
 
   (save-match-data
-    (let ((bufs (cond (global-auto-revert-mode (buffer-list))
-                      (auto-revert-always-poll auto-revert-buffer-list)
-                      (t auto-revert--polled-buffers)))
+    (let ((bufs (auto-revert--polled-buffers))
 	  remaining new)
       ;; Buffers with remote contents shall be reverted only if the
       ;; connection is established already.
-- 
2.20.1 (Apple Git-117)

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

[Michael Albinus]

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

Hi Mattias,

> Very well; here is an incremental patch (to make the differences
> clear). It's a compromise: the derived state is gone, but there are
> two functions: one for the list of buffers that need to be polled, and
> one for whether that list would be non-empty.

Thanks, this goes to the right direction.

> By the way, the patch now uses functions from cl-lib, not just
> macros. Is there any reason not to?

No problem.

From my POV you could push it (with the final decision for the name from
Eli). If there are problems, people will react soon - that's my
experience with autorevert changes.

But we have autorevert-tests.el, so at least the important cases are
covered. I'm wondering if there are some tests which need to be added.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  35418@debbugs.gnu.org
> Date: Mon, 29 Apr 2019 22:17:23 +0200
> 
> >From my POV you could push it (with the final decision for the name from
> Eli).

I'm okay with the name.

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

[Mattias Engdegård]

30 apr. 2019 kl. 05.57 skrev Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  35418@debbugs.gnu.org
>> Date: Mon, 29 Apr 2019 22:17:23 +0200
>> 
>>> From my POV you could push it (with the final decision for the name from
>> Eli).
> 
> I'm okay with the name.

Thank you, pushed (c61bbb4c8e). What remains to be done:

- reorganise the manual as discussed before
- see what it would take to make the change work in global-auto-revert-mode

and of course fix any lingering concerns that may turn up.

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

[Michael Albinus]

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

Hi Mattias,

> Thank you, pushed (c61bbb4c8e). What remains to be done:

Thanks!

> and of course fix any lingering concerns that may turn up.

Let's start with this one: <https://emba.gnu.org/emacs/emacs/-/jobs/1587/raw>

Best regards, Michael.

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

[Mattias Engdegård]

30 apr. 2019 kl. 14.59 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Let's start with this one: <https://emba.gnu.org/emacs/emacs/-/jobs/1587/raw>

Oh dear. Now fixed in the correct way, I hope. Thank you!

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

[Michael Albinus]

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

> 30 apr. 2019 kl. 14.59 skrev Michael Albinus <michael.albinus@gmx.de>:
>> 
>> Let's start with this one: <https://emba.gnu.org/emacs/emacs/-/jobs/1587/raw>
>
> Oh dear. Now fixed in the correct way, I hope. Thank you!

Yes, looks better. You can always run a dedicated test like this:

--8<---------------cut here---------------start------------->8---
$ make -C test autorevert-tests
--8<---------------cut here---------------end--------------->8---

See also test/README.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 29 Apr 2019 13:06:50 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 35418@debbugs.gnu.org
> 
> By the way, the organisation of this part of the manual could be improved -- don't you agree?

Could be.

> There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.
> 
> Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.

You say "section" but the names you cite are node names, not section
names.  The latter are slightly more descriptive.

> This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.

I think we should have sibling sections "Reverting" and "Autorevert",
with the latter describing both types of auto-reverting.  And
"Reverting" should have a cross-reference to "Autorevert" for
automatic reverting of file-visiting buffers.

Would you like to submit a patch to that effect?

> > Saying this, the user option might need another name. What about
> > `auto-revert-also-poll'?
> 
> Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
> I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
> It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.

I actually think auto-revert-dont-poll is better, even though it's
negative.

Thanks.

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

[Mattias Engdegård]

29 apr. 2019 kl. 18.23 skrev Eli Zaretskii <eliz@gnu.org>:
> 
>> There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.
>> 
>> Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.
> 
> You say "section" but the names you cite are node names, not section
> names.  The latter are slightly more descriptive.

Correct, thank you. (The node names attract the eyes since they are highlighted as links.)

>> This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.
> 
> I think we should have sibling sections "Reverting" and "Autorevert",
> with the latter describing both types of auto-reverting.  And
> "Reverting" should have a cross-reference to "Autorevert" for
> automatic reverting of file-visiting buffers.
> 
> Would you like to submit a patch to that effect?

I'll see what I can do, once we are done with this particular patch.

>>> Saying this, the user option might need another name. What about
>>> `auto-revert-also-poll'?
>> 
>> Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
>> I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
>> It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.
> 
> I actually think auto-revert-dont-poll is better, even though it's
> negative.

Then I'd prefer auto-revert-avoid-polling; 'don't poll' sounds definitive but we may still have to poll from time to time.

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

[Michael Albinus]

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

Hi Mattias,
 
>> I actually think auto-revert-dont-poll is better, even though it's
>> negative.
>
> Then I'd prefer auto-revert-avoid-polling; 'don't poll' sounds
> definitive but we may still have to poll from time to time.

Sounds good to my ears.

Best regards, Michael.

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

[Mattias Engdegård]

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

mån 2019-04-29 klockan 19:23 +0300 skrev Eli Zaretskii:
> > 
> I think we should have sibling sections "Reverting" and "Autorevert",
> with the latter describing both types of auto-reverting.  And
> "Reverting" should have a cross-reference to "AutorevertAuto-" for
> automatic reverting of file-visiting buffers.

Here is a patch that does roughly that. I'm not entirely happy with the
old 'Auto-reverting non-buffer files' section, whose node name was just
'Autorevert'. I would have preferred it as a subsection to the new
auto-revert section, along with its existing two subsections, but since
its place isn't the same in the on-line and printed manuals, that
seemed technically tricky without duplicating a lot of text.



[-- Attachment #2: 0001-Reorganise-auto-revert-nodes-in-the-manual.patch --]
[-- Type: text/x-patch, Size: 7636 bytes --]

From d6ed8ed396a8a698ad63a8a4f3460ecdf982c507 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 30 Apr 2019 22:35:56 +0200
Subject: [PATCH] Reorganise (auto-)revert nodes in the manual

The amount of information on auto-revert has grown to deserve a
section of its own (bug#35418).

* doc/emacs/files.texi:
* doc/emacs/arevert-xtra.texi:
* doc/emacs/buffers.texi:
* doc/emacs/emacs.texi:
Rename node 'Autorevert' to 'Non-file buffers'.
Add node 'Auto-revert' and move general information on that topic there.
Shuffle text in that node into a rough least-to-most specific order.
---
 doc/emacs/arevert-xtra.texi |  2 +-
 doc/emacs/buffers.texi      |  4 +--
 doc/emacs/emacs.texi        |  3 +-
 doc/emacs/files.texi        | 61 ++++++++++++++++++++++---------------
 4 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index cd7c1ff895..8575e1efdd 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -4,7 +4,7 @@
 @c
 @c This file is included either in emacs-xtra.texi (when producing the
 @c printed version) or in the main Emacs manual (for the on-line version).
-@node Autorevert
+@node Non-file buffers
 @section Auto Reverting Non-File Buffers
 
 Global Auto Revert Mode normally only reverts file buffers.  There are
diff --git a/doc/emacs/buffers.texi b/doc/emacs/buffers.texi
index 27fcb7369a..66b21354c8 100644
--- a/doc/emacs/buffers.texi
+++ b/doc/emacs/buffers.texi
@@ -568,10 +568,10 @@ Several Buffers
 Auto Revert mode applies to the @file{*Buffer List*} buffer only if
 @code{global-auto-revert-non-file-buffers} is non-@code{nil}.
 @iftex
-@inforef{Autorevert,, emacs-xtra}, for details.
+@inforef{Non-file buffers,, emacs-xtra}, for details.
 @end iftex
 @ifnottex
-@xref{Autorevert, global-auto-revert-non-file-buffers}, for details.
+@xref{Non-file buffers, global-auto-revert-non-file-buffers}, for details.
 @end ifnottex
 
 @node Indirect Buffers
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index 58ec373029..75b12940ea 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -443,8 +443,9 @@ Top
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
+* Auto-revert::         Keeping buffers automatically up-to-date.
 @ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
+* Non-file buffers::    Auto Reverting non-file buffers.
 @end ifnottex
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 990b8f1679..93b2e79e70 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -25,8 +25,9 @@ Files
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
+* Auto-revert::         Keeping buffers automatically up-to-date.
 @ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
+* Non-file buffers::    Auto Reverting non-file buffers.
 @end ifnottex
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
@@ -955,6 +956,11 @@ Reverting
 is not modified.  (If you have edited the text, it would be wrong to
 discard your changes.)
 
+  You can also tell Emacs to revert buffers automatically when their
+visited files change on disk; @pxref{Auto-revert}.
+
+@node Auto-revert
+@section Auto-revert: Keeping buffers automatically up-to-date
 @cindex Global Auto Revert mode
 @cindex mode, Global Auto Revert
 @cindex Auto Revert mode
@@ -962,21 +968,38 @@ Reverting
 @findex global-auto-revert-mode
 @findex auto-revert-mode
 @findex auto-revert-tail-mode
-@vindex auto-revert-interval
-@vindex auto-revert-remote-files
+
+  A buffer can get out of sync with respect to its visited file on
+disk if that file is changed by another program.  To keep it up to
+date, you can enable Auto-revert mode by typing @kbd{M-x auto-revert-mode}.
+This automatically reverts the buffer when its visited file changes on
+disk.  To do the same for all file buffers, type
+@kbd{M-x global-auto-revert-mode} to enable Global Auto-Revert mode.
+
+  Auto-revert will not revert a buffer if it has unsaved changes, or if
+its file on disk is deleted or renamed.
+
+  One use of Auto-Revert mode is to ``tail'' a file such as a system
+log, so that changes made to that file by other programs are
+continuously displayed.  To do this, just move the point to the end of
+the buffer, and it will stay there as the file contents change.
+However, if you are sure that the file will only change by growing at
+the end, use Auto-Revert Tail mode instead
+(@code{auto-revert-tail-mode}).  It is more efficient for this.
+Auto-Revert Tail mode also works for remote files.
+
 @vindex auto-revert-verbose
-  You can also tell Emacs to revert buffers periodically.  To do this
-for a specific buffer, enable the minor mode Auto-Revert mode by
-typing @kbd{M-x auto-revert-mode}.  This automatically reverts the
-current buffer when its visited file changes on disk.  To do the same
-for all file buffers, type @kbd{M-x global-auto-revert-mode} to enable
-Global Auto-Revert mode.  These minor modes do not check or revert
-remote files, because that is usually too slow.  This behavior can be
-changed by setting the variable @code{auto-revert-remote-files} to
-non-@code{nil}.
+  When a buffer is auto-reverted, a message is generated.  This can be
+suppressed by setting @code{auto-revert-verbose} to @code{nil}.
+
+@vindex auto-revert-remote-files
+  These minor modes do not check or revert remote files, because that is
+usually too slow.  This behavior can be changed by setting the
+variable @code{auto-revert-remote-files} to non-@code{nil}.
 
 @cindex file notifications
 @vindex auto-revert-use-notify
+@vindex auto-revert-interval
   By default, Auto-Revert mode works using @dfn{file notifications},
 whereby changes in the filesystem are reported to Emacs by the OS.
 You can disable use of file notifications by customizing the variable
@@ -1002,24 +1025,14 @@ Reverting
 @code{auto-revert-notify-exclude-dir-regexp} to match files that
 should be excluded from using notification.
 
-  One use of Auto-Revert mode is to ``tail'' a file such as a system
-log, so that changes made to that file by other programs are
-continuously displayed.  To do this, just move the point to the end of
-the buffer, and it will stay there as the file contents change.
-However, if you are sure that the file will only change by growing at
-the end, use Auto-Revert Tail mode instead
-(@code{auto-revert-tail-mode}).  It is more efficient for this.
-Auto-Revert Tail mode works also for remote files.
-
-  When a buffer is auto-reverted, a message is generated.  This can be
-suppressed by setting @code{auto-revert-verbose} to @code{nil}.
-
   In Dired buffers (@pxref{Dired}), Auto-Revert mode refreshes the
 buffer when a file is created or deleted in the buffer's directory.
 
   @xref{VC Undo}, for commands to revert to earlier versions of files
 under version control.  @xref{VC Mode Line}, for Auto Revert
 peculiarities when visiting files under version control.
+@xref{Non-file buffers} for auto-reverting buffers that do not visit
+files.
 
 @ifnottex
 @include arevert-xtra.texi
-- 
2.20.1

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Cc: michael.albinus@gmx.de, 35418@debbugs.gnu.org
> Date: Tue, 30 Apr 2019 23:09:08 +0200
> 
> Here is a patch that does roughly that. I'm not entirely happy with the
> old 'Auto-reverting non-buffer files' section, whose node name was just
> 'Autorevert'. I would have preferred it as a subsection to the new
> auto-revert section, along with its existing two subsections, but since
> its place isn't the same in the on-line and printed manuals, that
> seemed technically tricky without duplicating a lot of text.

Didn't yet review the patch, but I don't understand the difficulty
with moving 'Auto-reverting non-buffer files' into Auto-revert.  Can
you explain what gets in the way?

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

[Mattias Engdegård]

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

ons 2019-05-01 klockan 20:45 +0300 skrev Eli Zaretskii:
> 
> Didn't yet review the patch, but I don't understand the difficulty
> with moving 'Auto-reverting non-buffer files' into Auto-revert.  Can
> you explain what gets in the way?

After applying the patch, the on-line manual would have the nodes

* Reverting (about reverting)
* Auto-revert (about auto-revert)
* Non-file buffers (about auto-reverting non-file buffers)
** Auto-reverting the buffer menu
** Auto-reverting Dired

but I'd rather have

* Reverting (about reverting)
* Auto-revert (about auto-revert, including non-buffer files)
** Auto-reverting the buffer menu
** Auto-reverting Dired

except that in the printed manual, the non-buffer part is a section of
its own. The attached patch hacks around it by removing @node and
@section from arevert-xtra.texi; perhaps it can be stomached.


[-- Attachment #2: 0001-Reorganise-auto-revert-nodes-in-the-manual.patch --]
[-- Type: text/x-patch, Size: 7972 bytes --]

From 21c4451f463700e2a6f7a256b214eed89a9e3d3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 30 Apr 2019 22:35:56 +0200
Subject: [PATCH] Reorganise (auto-)revert nodes in the manual

The amount of information on auto-revert has grown to deserve a
section of its own (bug#35418).

* doc/emacs/files.texi:
* doc/emacs/arevert-xtra.texi:
* doc/emacs/buffers.texi:
* doc/emacs/emacs.texi:
* doc/emacs/emacs-xtra.texi:
Add node 'Auto-revert' and move general information on that topic there.
Sort paragraphs in that node in a rough least-to-most specific order.
Include the old 'Autorevert' node into that node when building the
on-line manual.
---
 doc/emacs/arevert-xtra.texi |  3 +-
 doc/emacs/buffers.texi      |  4 +--
 doc/emacs/emacs-xtra.texi   |  2 ++
 doc/emacs/emacs.texi        |  4 +--
 doc/emacs/files.texi        | 60 +++++++++++++++++++++----------------
 5 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index cd7c1ff895..8cc5b053b5 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -4,8 +4,7 @@
 @c
 @c This file is included either in emacs-xtra.texi (when producing the
 @c printed version) or in the main Emacs manual (for the on-line version).
-@node Autorevert
-@section Auto Reverting Non-File Buffers
+@c The including file must provide its own @node and @section lines.
 
 Global Auto Revert Mode normally only reverts file buffers.  There are
 two ways to auto-revert certain non-file buffers: by enabling Auto
diff --git a/doc/emacs/buffers.texi b/doc/emacs/buffers.texi
index 27fcb7369a..14a0a01ca8 100644
--- a/doc/emacs/buffers.texi
+++ b/doc/emacs/buffers.texi
@@ -568,10 +568,10 @@ Several Buffers
 Auto Revert mode applies to the @file{*Buffer List*} buffer only if
 @code{global-auto-revert-non-file-buffers} is non-@code{nil}.
 @iftex
-@inforef{Autorevert,, emacs-xtra}, for details.
+@inforef{Auto Reverting the Buffer Menu,, emacs-xtra}, for details.
 @end iftex
 @ifnottex
-@xref{Autorevert, global-auto-revert-non-file-buffers}, for details.
+@xref{Auto Reverting the Buffer Menu, global-auto-revert-non-file-buffers}, for details.
 @end ifnottex
 
 @node Indirect Buffers
diff --git a/doc/emacs/emacs-xtra.texi b/doc/emacs/emacs-xtra.texi
index dcd8fae1b6..b63f222621 100644
--- a/doc/emacs/emacs-xtra.texi
+++ b/doc/emacs/emacs-xtra.texi
@@ -111,6 +111,8 @@ Introduction
 @raisesections
 @include picture-xtra.texi
 
+@node Non-file buffers
+@section Auto Reverting Non-File Buffers
 @include arevert-xtra.texi
 
 @include dired-xtra.texi
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index 58ec373029..0fc34dd63d 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -443,9 +443,7 @@ Top
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto-revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 990b8f1679..104cb24e82 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -25,9 +25,7 @@ Files
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto-revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
@@ -955,6 +953,11 @@ Reverting
 is not modified.  (If you have edited the text, it would be wrong to
 discard your changes.)
 
+  You can also tell Emacs to revert buffers automatically when their
+visited files change on disk; @pxref{Auto-revert}.
+
+@node Auto-revert
+@section Auto-revert: Keeping buffers automatically up-to-date
 @cindex Global Auto Revert mode
 @cindex mode, Global Auto Revert
 @cindex Auto Revert mode
@@ -962,21 +965,38 @@ Reverting
 @findex global-auto-revert-mode
 @findex auto-revert-mode
 @findex auto-revert-tail-mode
-@vindex auto-revert-interval
-@vindex auto-revert-remote-files
+
+  A buffer can get out of sync with respect to its visited file on
+disk if that file is changed by another program.  To keep it up to
+date, you can enable Auto-revert mode by typing @kbd{M-x auto-revert-mode}.
+This automatically reverts the buffer when its visited file changes on
+disk.  To do the same for all file buffers, type
+@kbd{M-x global-auto-revert-mode} to enable Global Auto-Revert mode.
+
+  Auto-revert will not revert a buffer if it has unsaved changes, or if
+its file on disk is deleted or renamed.
+
+  One use of Auto-Revert mode is to ``tail'' a file such as a system
+log, so that changes made to that file by other programs are
+continuously displayed.  To do this, just move the point to the end of
+the buffer, and it will stay there as the file contents change.
+However, if you are sure that the file will only change by growing at
+the end, use Auto-Revert Tail mode instead
+(@code{auto-revert-tail-mode}).  It is more efficient for this.
+Auto-Revert Tail mode also works for remote files.
+
 @vindex auto-revert-verbose
-  You can also tell Emacs to revert buffers periodically.  To do this
-for a specific buffer, enable the minor mode Auto-Revert mode by
-typing @kbd{M-x auto-revert-mode}.  This automatically reverts the
-current buffer when its visited file changes on disk.  To do the same
-for all file buffers, type @kbd{M-x global-auto-revert-mode} to enable
-Global Auto-Revert mode.  These minor modes do not check or revert
-remote files, because that is usually too slow.  This behavior can be
-changed by setting the variable @code{auto-revert-remote-files} to
-non-@code{nil}.
+  When a buffer is auto-reverted, a message is generated.  This can be
+suppressed by setting @code{auto-revert-verbose} to @code{nil}.
+
+@vindex auto-revert-remote-files
+  These minor modes do not check or revert remote files, because that is
+usually too slow.  This behavior can be changed by setting the
+variable @code{auto-revert-remote-files} to non-@code{nil}.
 
 @cindex file notifications
 @vindex auto-revert-use-notify
+@vindex auto-revert-interval
   By default, Auto-Revert mode works using @dfn{file notifications},
 whereby changes in the filesystem are reported to Emacs by the OS.
 You can disable use of file notifications by customizing the variable
@@ -1002,18 +1022,6 @@ Reverting
 @code{auto-revert-notify-exclude-dir-regexp} to match files that
 should be excluded from using notification.
 
-  One use of Auto-Revert mode is to ``tail'' a file such as a system
-log, so that changes made to that file by other programs are
-continuously displayed.  To do this, just move the point to the end of
-the buffer, and it will stay there as the file contents change.
-However, if you are sure that the file will only change by growing at
-the end, use Auto-Revert Tail mode instead
-(@code{auto-revert-tail-mode}).  It is more efficient for this.
-Auto-Revert Tail mode works also for remote files.
-
-  When a buffer is auto-reverted, a message is generated.  This can be
-suppressed by setting @code{auto-revert-verbose} to @code{nil}.
-
   In Dired buffers (@pxref{Dired}), Auto-Revert mode refreshes the
 buffer when a file is created or deleted in the buffer's directory.
 
-- 
2.20.1

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

[Michael Albinus]

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

Hi Mattias,

In general it looks OK. Just some few comments:

> --- a/doc/emacs/emacs.texi
> +++ b/doc/emacs/emacs.texi
> @@ -443,9 +443,7 @@ Top
>  * Visiting::            Visiting a file prepares Emacs to edit the file.
>  * Saving::              Saving makes your changes permanent.
>  * Reverting::           Reverting cancels all the changes not saved.
> -@ifnottex
> -* Autorevert::          Auto Reverting non-file buffers.
> -@end ifnottex
> +* Auto-revert::         Keeping buffers automatically up-to-date.
>  * Auto Save::           Auto Save periodically protects against loss of data.

Please call the node "Auto Revert", like the following "Auto Save".

> +date, you can enable Auto-revert mode by typing @kbd{M-x auto-revert-mode}.

This shall be "Auto Revert mode" (or "Auto Revert Mode", don't know).

> +the end, use Auto-Revert Tail mode instead

dito, "Auto Revert Tail mode".

I know that it was called already like this. But it looks more
consistent to me, when changing it.

Best regards, Michael.

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

[Mattias Engdegård]

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

tor 2019-05-02 klockan 14:18 +0200 skrev Michael Albinus:
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> Please call the node "Auto Revert", like the following "Auto Save".

Done.

> > +date, you can enable Auto-revert mode by typing @kbd{M-x auto-
> > revert-mode}.
> 
> This shall be "Auto Revert mode" (or "Auto Revert Mode", don't know).

Done (I went with the former).

> > +the end, use Auto-Revert Tail mode instead
> 
> dito, "Auto Revert Tail mode".

Done.

> I know that it was called already like this. But it looks more
> consistent to me, when changing it.

Of course. New patch attached.


[-- Attachment #2: 0001-Reorganise-auto-revert-nodes-in-the-manual.patch --]
[-- Type: text/x-patch, Size: 9337 bytes --]

From ad27e857bb7a623e137407c45b65744b3fc35ca5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 30 Apr 2019 22:35:56 +0200
Subject: [PATCH] Reorganise (auto-)revert nodes in the manual

The amount of information on auto-revert has grown to deserve a
section of its own (bug#35418).

* doc/emacs/files.texi:
* doc/emacs/arevert-xtra.texi:
* doc/emacs/buffers.texi:
* doc/emacs/emacs.texi:
* doc/emacs/emacs-xtra.texi:
Add node 'Auto-revert' and move general information on that topic there.
Sort paragraphs in that node in a rough least-to-most specific order.
Include the old 'Autorevert' node into that node when building the
on-line manual.
---
 doc/emacs/arevert-xtra.texi |  3 +-
 doc/emacs/buffers.texi      |  4 +--
 doc/emacs/emacs-xtra.texi   |  4 ++-
 doc/emacs/emacs.texi        |  4 +--
 doc/emacs/files.texi        | 67 +++++++++++++++++++++----------------
 5 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index cd7c1ff895..8cc5b053b5 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -4,8 +4,7 @@
 @c
 @c This file is included either in emacs-xtra.texi (when producing the
 @c printed version) or in the main Emacs manual (for the on-line version).
-@node Autorevert
-@section Auto Reverting Non-File Buffers
+@c The including file must provide its own @node and @section lines.
 
 Global Auto Revert Mode normally only reverts file buffers.  There are
 two ways to auto-revert certain non-file buffers: by enabling Auto
diff --git a/doc/emacs/buffers.texi b/doc/emacs/buffers.texi
index 27fcb7369a..14a0a01ca8 100644
--- a/doc/emacs/buffers.texi
+++ b/doc/emacs/buffers.texi
@@ -568,10 +568,10 @@ Several Buffers
 Auto Revert mode applies to the @file{*Buffer List*} buffer only if
 @code{global-auto-revert-non-file-buffers} is non-@code{nil}.
 @iftex
-@inforef{Autorevert,, emacs-xtra}, for details.
+@inforef{Auto Reverting the Buffer Menu,, emacs-xtra}, for details.
 @end iftex
 @ifnottex
-@xref{Autorevert, global-auto-revert-non-file-buffers}, for details.
+@xref{Auto Reverting the Buffer Menu, global-auto-revert-non-file-buffers}, for details.
 @end ifnottex
 
 @node Indirect Buffers
diff --git a/doc/emacs/emacs-xtra.texi b/doc/emacs/emacs-xtra.texi
index dcd8fae1b6..84bbdb84d0 100644
--- a/doc/emacs/emacs-xtra.texi
+++ b/doc/emacs/emacs-xtra.texi
@@ -59,7 +59,7 @@ Top
 * Picture Mode::        Editing pictures made up of characters using
                          the quarter-plane screen model.
 
-* Autorevert::          Auto Reverting non-file buffers.
+* Non-File Buffers::    Auto Reverting non-file buffers.
 * Subdir Switches::     Subdirectory switches in Dired.
 * Advanced Calendar/Diary Usage:: Advanced Calendar/Diary customization.
 * Emerge::              A convenient way of merging two versions of a program.
@@ -111,6 +111,8 @@ Introduction
 @raisesections
 @include picture-xtra.texi
 
+@node Non-File Buffers
+@section Auto Reverting Non-File Buffers
 @include arevert-xtra.texi
 
 @include dired-xtra.texi
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index 58ec373029..a34cef221e 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -443,9 +443,7 @@ Top
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto Revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 990b8f1679..76be4c112f 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -25,9 +25,7 @@ Files
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto Revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
@@ -955,6 +953,11 @@ Reverting
 is not modified.  (If you have edited the text, it would be wrong to
 discard your changes.)
 
+  You can also tell Emacs to revert buffers automatically when their
+visited files change on disk; @pxref{Auto Revert}.
+
+@node Auto Revert
+@section Auto Revert: Keeping buffers automatically up-to-date
 @cindex Global Auto Revert mode
 @cindex mode, Global Auto Revert
 @cindex Auto Revert mode
@@ -962,22 +965,39 @@ Reverting
 @findex global-auto-revert-mode
 @findex auto-revert-mode
 @findex auto-revert-tail-mode
-@vindex auto-revert-interval
-@vindex auto-revert-remote-files
+
+  A buffer can get out of sync with respect to its visited file on
+disk if that file is changed by another program.  To keep it up to
+date, you can enable Auto Revert mode by typing @kbd{M-x auto-revert-mode}.
+This automatically reverts the buffer when its visited file changes on
+disk.  To do the same for all file buffers, type
+@kbd{M-x global-auto-revert-mode} to enable Global Auto Revert mode.
+
+  Auto Revert will not revert a buffer if it has unsaved changes, or if
+its file on disk is deleted or renamed.
+
+  One use of Auto Revert mode is to ``tail'' a file such as a system
+log, so that changes made to that file by other programs are
+continuously displayed.  To do this, just move the point to the end of
+the buffer, and it will stay there as the file contents change.
+However, if you are sure that the file will only change by growing at
+the end, use Auto Revert Tail mode instead
+(@code{auto-revert-tail-mode}).  It is more efficient for this.
+Auto Revert Tail mode also works for remote files.
+
 @vindex auto-revert-verbose
-  You can also tell Emacs to revert buffers periodically.  To do this
-for a specific buffer, enable the minor mode Auto-Revert mode by
-typing @kbd{M-x auto-revert-mode}.  This automatically reverts the
-current buffer when its visited file changes on disk.  To do the same
-for all file buffers, type @kbd{M-x global-auto-revert-mode} to enable
-Global Auto-Revert mode.  These minor modes do not check or revert
-remote files, because that is usually too slow.  This behavior can be
-changed by setting the variable @code{auto-revert-remote-files} to
-non-@code{nil}.
+  When a buffer is auto-reverted, a message is generated.  This can be
+suppressed by setting @code{auto-revert-verbose} to @code{nil}.
+
+@vindex auto-revert-remote-files
+  These minor modes do not check or revert remote files, because that is
+usually too slow.  This behavior can be changed by setting the
+variable @code{auto-revert-remote-files} to non-@code{nil}.
 
 @cindex file notifications
 @vindex auto-revert-use-notify
-  By default, Auto-Revert mode works using @dfn{file notifications},
+@vindex auto-revert-interval
+  By default, Auto Revert mode works using @dfn{file notifications},
 whereby changes in the filesystem are reported to Emacs by the OS.
 You can disable use of file notifications by customizing the variable
 @code{auto-revert-use-notify} to a @code{nil} value, then Emacs will
@@ -990,7 +1010,7 @@ Reverting
 
 @vindex auto-revert-avoid-polling
 @vindex auto-revert-notify-exclude-dir-regexp
-  By default, Auto-Revert mode will poll files for changes
+  By default, Auto Revert mode will poll files for changes
 periodically even when file notifications are used.  Such polling is
 usually unnecessary, and turning it off may save power by relying on
 notifications only.  To do so, set the variable
@@ -1002,19 +1022,7 @@ Reverting
 @code{auto-revert-notify-exclude-dir-regexp} to match files that
 should be excluded from using notification.
 
-  One use of Auto-Revert mode is to ``tail'' a file such as a system
-log, so that changes made to that file by other programs are
-continuously displayed.  To do this, just move the point to the end of
-the buffer, and it will stay there as the file contents change.
-However, if you are sure that the file will only change by growing at
-the end, use Auto-Revert Tail mode instead
-(@code{auto-revert-tail-mode}).  It is more efficient for this.
-Auto-Revert Tail mode works also for remote files.
-
-  When a buffer is auto-reverted, a message is generated.  This can be
-suppressed by setting @code{auto-revert-verbose} to @code{nil}.
-
-  In Dired buffers (@pxref{Dired}), Auto-Revert mode refreshes the
+  In Dired buffers (@pxref{Dired}), Auto Revert mode refreshes the
 buffer when a file is created or deleted in the buffer's directory.
 
   @xref{VC Undo}, for commands to revert to earlier versions of files
@@ -1022,6 +1030,7 @@ Reverting
 peculiarities when visiting files under version control.
 
 @ifnottex
+@c The node text continues here.
 @include arevert-xtra.texi
 @end ifnottex
 
-- 
2.20.1

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

[Michael Albinus]

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

Hi Mattias,

> Of course. New patch attached.

Thanks, LGTM.

Best regards, Michael.

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

[Mattias Engdegård]

2 maj 2019 kl. 15.02 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Thanks, LGTM.

Thank you. I'll give Eli a chance to comment before committing.
(I'm in no hurry; take your time.)

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 35418@debbugs.gnu.org
> Date: Thu, 02 May 2019 14:53:20 +0200
> 
> * doc/emacs/files.texi:
> * doc/emacs/arevert-xtra.texi:
> * doc/emacs/buffers.texi:
> * doc/emacs/emacs.texi:
> * doc/emacs/emacs-xtra.texi:
> Add node 'Auto-revert' and move general information on that topic there.
> Sort paragraphs in that node in a rough least-to-most specific order.
> Include the old 'Autorevert' node into that node when building the
> on-line manual.

It is better to cite the node names in the log, unless doing that is
completely impractical.

> diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
> index cd7c1ff895..8cc5b053b5 100644
> --- a/doc/emacs/arevert-xtra.texi
> +++ b/doc/emacs/arevert-xtra.texi
> @@ -4,8 +4,7 @@
>  @c
>  @c This file is included either in emacs-xtra.texi (when producing the
>  @c printed version) or in the main Emacs manual (for the on-line version).
> -@node Autorevert
> -@section Auto Reverting Non-File Buffers
> +@c The including file must provide its own @node and @section lines.

So maybe we shouldn't remove the @node here?  How about making it a
subsection of "Auto Revert" instead?

> +@vindex auto-revert-remote-files
> +  These minor modes do not check or revert remote files, because that is

Which "these minor modes"?  Such wording is only appropriate when it
closely follows a list of modes, which is not the case here.  I think
it's better to enumerate the modes explicitly here.

> +usually too slow.  This behavior can be changed by setting the
> +variable @code{auto-revert-remote-files} to non-@code{nil}.
>  
>  @cindex file notifications
>  @vindex auto-revert-use-notify
> -  By default, Auto-Revert mode works using @dfn{file notifications},
> +@vindex auto-revert-interval
> +  By default, Auto Revert mode works using @dfn{file notifications},
>  whereby changes in the filesystem are reported to Emacs by the OS.
>  You can disable use of file notifications by customizing the variable
>  @code{auto-revert-use-notify} to a @code{nil} value, then Emacs will
> @@ -990,7 +1010,7 @@ Reverting
>  
>  @vindex auto-revert-avoid-polling
>  @vindex auto-revert-notify-exclude-dir-regexp
> -  By default, Auto-Revert mode will poll files for changes
> +  By default, Auto Revert mode will poll files for changes
>  periodically even when file notifications are used.  Such polling is
>  usually unnecessary, and turning it off may save power by relying on
   ^^^^^^^^^^^^^^^^^^^
I would say "unnecessary in many cases".  "usually" begs the question
why by default we do poll.  Bonus points for adding some hint about
what rare situations do need such polling, as I think this description
sounds like a small riddle without that, and doesn't allow people to
make an educated decision regarding whether they do or don't want the
polling turned off.

Thanks.

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

[Mattias Engdegård]

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

fre 2019-05-03 klockan 16:44 +0300 skrev Eli Zaretskii:
> > 
> It is better to cite the node names in the log, unless doing that is
> completely impractical.

Done. I found it a bit impractical in this case, but did so anyway.

> > -@node Autorevert
> > -@section Auto Reverting Non-File Buffers
> > +@c The including file must provide its own @node and @section
> > lines.
> 
> So maybe we shouldn't remove the @node here?  How about making it a
> subsection of "Auto Revert" instead?

The text in arevert-xtra has two subsections already. Then we would
have three subsections, where the first acts as a sort of prelude to
the two others. That might work for the on-line manual, but how would
it fit into emacs-xtra? What would the section be then?

> > +@vindex auto-revert-remote-files
> > +  These minor modes do not check or revert remote files, because
> > that is
> 
> Which "these minor modes"?  Such wording is only appropriate when it
> closely follows a list of modes, which is not the case here.  I think
> it's better to enumerate the modes explicitly here.

Replaced with 'The Auto Revert modes'; this should be readily
understood.

> >  periodically even when file notifications are used.  Such polling
> > is
> >  usually unnecessary, and turning it off may save power by relying
> > on
>    ^^^^^^^^^^^^^^^^^^^
> I would say "unnecessary in many cases".  "usually" begs the question
> why by default we do poll.  Bonus points for adding some hint about
> what rare situations do need such polling, as I think this
> description
> sounds like a small riddle without that, and doesn't allow people to
> make an educated decision regarding whether they do or don't want the
> polling turned off.

Done, but the hint you are asking for does come right after:

  [...] However,
  notification is ineffective on certain file systems; mainly network
  file system on Unix-like machines, where files can be altered from
  other machines.

which is the most important case that I'm aware of. (According to
Michael, Tramp notifications can be unreliable sometimes, but they have
to be enabled actively.)

Thanks for the review! Revised patch attached.


[-- Attachment #2: 0001-Reorganise-auto-revert-nodes-in-the-manual.patch --]
[-- Type: text/x-patch, Size: 9942 bytes --]

From 3b9fc1d288cb7168db3f1352f6e261266dbe4349 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 30 Apr 2019 22:35:56 +0200
Subject: [PATCH] Reorganise (auto-)revert nodes in the manual

The amount of information on auto-revert has grown to deserve a
section of its own (bug#35418).

* doc/emacs/files.texi
(Files): Adjust menu.
(Reverting, Auto Revert):
Add node `Auto Revert' and move general information on that topic there
from `Reverting'.
Sort paragraphs in `Auto Revert' in a rough least-to-most specific order.
Include the old `Autorevert' text and subsections into that node when
building the on-line manual.
(Autorevert): Remove.
* doc/emacs/arevert-xtra.texi (Autorevert): Remove node and section lines.
* doc/emacs/buffers.texi (Several Buffers): Adjust references.
* doc/emacs/emacs.texi (Top): Adjust menu.
* doc/emacs/emacs-xtra.texi:
(Top): Adjust menu.
(Non-File Buffers): Add node and section lines.
---
 doc/emacs/arevert-xtra.texi |  3 +-
 doc/emacs/buffers.texi      |  4 +--
 doc/emacs/emacs-xtra.texi   |  4 ++-
 doc/emacs/emacs.texi        |  4 +--
 doc/emacs/files.texi        | 71 +++++++++++++++++++++----------------
 5 files changed, 47 insertions(+), 39 deletions(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index cd7c1ff895..8cc5b053b5 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -4,8 +4,7 @@
 @c
 @c This file is included either in emacs-xtra.texi (when producing the
 @c printed version) or in the main Emacs manual (for the on-line version).
-@node Autorevert
-@section Auto Reverting Non-File Buffers
+@c The including file must provide its own @node and @section lines.
 
 Global Auto Revert Mode normally only reverts file buffers.  There are
 two ways to auto-revert certain non-file buffers: by enabling Auto
diff --git a/doc/emacs/buffers.texi b/doc/emacs/buffers.texi
index 27fcb7369a..14a0a01ca8 100644
--- a/doc/emacs/buffers.texi
+++ b/doc/emacs/buffers.texi
@@ -568,10 +568,10 @@ Several Buffers
 Auto Revert mode applies to the @file{*Buffer List*} buffer only if
 @code{global-auto-revert-non-file-buffers} is non-@code{nil}.
 @iftex
-@inforef{Autorevert,, emacs-xtra}, for details.
+@inforef{Auto Reverting the Buffer Menu,, emacs-xtra}, for details.
 @end iftex
 @ifnottex
-@xref{Autorevert, global-auto-revert-non-file-buffers}, for details.
+@xref{Auto Reverting the Buffer Menu, global-auto-revert-non-file-buffers}, for details.
 @end ifnottex
 
 @node Indirect Buffers
diff --git a/doc/emacs/emacs-xtra.texi b/doc/emacs/emacs-xtra.texi
index dcd8fae1b6..84bbdb84d0 100644
--- a/doc/emacs/emacs-xtra.texi
+++ b/doc/emacs/emacs-xtra.texi
@@ -59,7 +59,7 @@ Top
 * Picture Mode::        Editing pictures made up of characters using
                          the quarter-plane screen model.
 
-* Autorevert::          Auto Reverting non-file buffers.
+* Non-File Buffers::    Auto Reverting non-file buffers.
 * Subdir Switches::     Subdirectory switches in Dired.
 * Advanced Calendar/Diary Usage:: Advanced Calendar/Diary customization.
 * Emerge::              A convenient way of merging two versions of a program.
@@ -111,6 +111,8 @@ Introduction
 @raisesections
 @include picture-xtra.texi
 
+@node Non-File Buffers
+@section Auto Reverting Non-File Buffers
 @include arevert-xtra.texi
 
 @include dired-xtra.texi
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index 58ec373029..a34cef221e 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -443,9 +443,7 @@ Top
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto Revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 990b8f1679..7f5e16a845 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -25,9 +25,7 @@ Files
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto Revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
@@ -955,6 +953,11 @@ Reverting
 is not modified.  (If you have edited the text, it would be wrong to
 discard your changes.)
 
+  You can also tell Emacs to revert buffers automatically when their
+visited files change on disk; @pxref{Auto Revert}.
+
+@node Auto Revert
+@section Auto Revert: Keeping buffers automatically up-to-date
 @cindex Global Auto Revert mode
 @cindex mode, Global Auto Revert
 @cindex Auto Revert mode
@@ -962,22 +965,39 @@ Reverting
 @findex global-auto-revert-mode
 @findex auto-revert-mode
 @findex auto-revert-tail-mode
-@vindex auto-revert-interval
-@vindex auto-revert-remote-files
+
+  A buffer can get out of sync with respect to its visited file on
+disk if that file is changed by another program.  To keep it up to
+date, you can enable Auto Revert mode by typing @kbd{M-x auto-revert-mode}.
+This automatically reverts the buffer when its visited file changes on
+disk.  To do the same for all file buffers, type
+@kbd{M-x global-auto-revert-mode} to enable Global Auto Revert mode.
+
+  Auto Revert will not revert a buffer if it has unsaved changes, or if
+its file on disk is deleted or renamed.
+
+  One use of Auto Revert mode is to ``tail'' a file such as a system
+log, so that changes made to that file by other programs are
+continuously displayed.  To do this, just move the point to the end of
+the buffer, and it will stay there as the file contents change.
+However, if you are sure that the file will only change by growing at
+the end, use Auto Revert Tail mode instead
+(@code{auto-revert-tail-mode}).  It is more efficient for this.
+Auto Revert Tail mode also works for remote files.
+
 @vindex auto-revert-verbose
-  You can also tell Emacs to revert buffers periodically.  To do this
-for a specific buffer, enable the minor mode Auto-Revert mode by
-typing @kbd{M-x auto-revert-mode}.  This automatically reverts the
-current buffer when its visited file changes on disk.  To do the same
-for all file buffers, type @kbd{M-x global-auto-revert-mode} to enable
-Global Auto-Revert mode.  These minor modes do not check or revert
-remote files, because that is usually too slow.  This behavior can be
-changed by setting the variable @code{auto-revert-remote-files} to
-non-@code{nil}.
+  When a buffer is auto-reverted, a message is generated.  This can be
+suppressed by setting @code{auto-revert-verbose} to @code{nil}.
+
+@vindex auto-revert-remote-files
+  The Auto Revert modes do not check or revert remote files, because
+that is usually too slow.  This behavior can be changed by setting the
+variable @code{auto-revert-remote-files} to non-@code{nil}.
 
 @cindex file notifications
 @vindex auto-revert-use-notify
-  By default, Auto-Revert mode works using @dfn{file notifications},
+@vindex auto-revert-interval
+  By default, Auto Revert mode works using @dfn{file notifications},
 whereby changes in the filesystem are reported to Emacs by the OS.
 You can disable use of file notifications by customizing the variable
 @code{auto-revert-use-notify} to a @code{nil} value, then Emacs will
@@ -990,10 +1010,10 @@ Reverting
 
 @vindex auto-revert-avoid-polling
 @vindex auto-revert-notify-exclude-dir-regexp
-  By default, Auto-Revert mode will poll files for changes
+  By default, Auto Revert mode will poll files for changes
 periodically even when file notifications are used.  Such polling is
-usually unnecessary, and turning it off may save power by relying on
-notifications only.  To do so, set the variable
+unnecessary in many cases, and turning it off may save power by
+relying on notifications only.  To do so, set the variable
 @code{auto-revert-avoid-polling} to non-@code{nil}.  However,
 notification is ineffective on certain file systems; mainly network
 file system on Unix-like machines, where files can be altered from
@@ -1002,19 +1022,7 @@ Reverting
 @code{auto-revert-notify-exclude-dir-regexp} to match files that
 should be excluded from using notification.
 
-  One use of Auto-Revert mode is to ``tail'' a file such as a system
-log, so that changes made to that file by other programs are
-continuously displayed.  To do this, just move the point to the end of
-the buffer, and it will stay there as the file contents change.
-However, if you are sure that the file will only change by growing at
-the end, use Auto-Revert Tail mode instead
-(@code{auto-revert-tail-mode}).  It is more efficient for this.
-Auto-Revert Tail mode works also for remote files.
-
-  When a buffer is auto-reverted, a message is generated.  This can be
-suppressed by setting @code{auto-revert-verbose} to @code{nil}.
-
-  In Dired buffers (@pxref{Dired}), Auto-Revert mode refreshes the
+  In Dired buffers (@pxref{Dired}), Auto Revert mode refreshes the
 buffer when a file is created or deleted in the buffer's directory.
 
   @xref{VC Undo}, for commands to revert to earlier versions of files
@@ -1022,6 +1030,7 @@ Reverting
 peculiarities when visiting files under version control.
 
 @ifnottex
+@c The node text continues here.
 @include arevert-xtra.texi
 @end ifnottex
 
-- 
2.20.1

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Cc: michael.albinus@gmx.de, 35418@debbugs.gnu.org
> Date: Fri, 03 May 2019 16:47:51 +0200
> 
> > It is better to cite the node names in the log, unless doing that is
> > completely impractical.
> 
> Done. I found it a bit impractical in this case, but did so anyway.

Thanks, but why did you start the description of the change on a new
line?  Here:

> * doc/emacs/files.texi
> (Files): Adjust menu.
> (Reverting, Auto Revert):
> Add node `Auto Revert' and move general information on that topic there
> from `Reverting'.
> Sort paragraphs in `Auto Revert' in a rough least-to-most specific order.
> Include the old `Autorevert' text and subsections into that node when
> building the on-line manual.
> (Autorevert): Remove.

This should be formatted like this (indented 2 spaces for clarity):

  * doc/emacs/files.texi (Files): Adjust menu.
  (Reverting, Auto Revert): Add node `Auto Revert' and move general
  information on that topic there from `Reverting'.  Sort paragraphs
  in `Auto Revert' in a rough least-to-most specific order.  Include
  the old `Autorevert' text and subsections into that node when
  building the on-line manual.
  (Autorevert): Remove.

IOW, only when you describe changes to another node you should start
on a new line.  The above is the formatting produced by "C-x 4 a" and
its ilk.

> > > -@node Autorevert
> > > -@section Auto Reverting Non-File Buffers
> > > +@c The including file must provide its own @node and @section
> > > lines.
> > 
> > So maybe we shouldn't remove the @node here?  How about making it a
> > subsection of "Auto Revert" instead?
> 
> The text in arevert-xtra has two subsections already. Then we would
> have three subsections, where the first acts as a sort of prelude to
> the two others. That might work for the on-line manual, but how would
> it fit into emacs-xtra? What would the section be then?

No, I meant make "Autorevert" (under its new name) a subsection of
"Auto Revert", and the 2 subsections of "Autorevert" sub-subsections.
Or did I misunderstand the problem you were describing?

> > >  periodically even when file notifications are used.  Such polling
> > > is
> > >  usually unnecessary, and turning it off may save power by relying
> > > on
> >    ^^^^^^^^^^^^^^^^^^^
> > I would say "unnecessary in many cases".  "usually" begs the question
> > why by default we do poll.  Bonus points for adding some hint about
> > what rare situations do need such polling, as I think this
> > description
> > sounds like a small riddle without that, and doesn't allow people to
> > make an educated decision regarding whether they do or don't want the
> > polling turned off.
> 
> Done, but the hint you are asking for does come right after:
> 
>   [...] However,
>   notification is ineffective on certain file systems; mainly network
>   file system on Unix-like machines, where files can be altered from
>   other machines.

Yes, but that hint doesn't mention the polling.  I think it's
important to tell that polling is needed in these rare cases.

Thanks.

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

[Mattias Engdegård]

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

lör 2019-05-04 klockan 12:04 +0300 skrev Eli Zaretskii:
> 
> IOW, only when you describe changes to another node you should start
> on a new line.  The above is the formatting produced by "C-x 4 a" and
> its ilk.

Reformatted.

> No, I meant make "Autorevert" (under its new name) a subsection of
> "Auto Revert", and the 2 subsections of "Autorevert" sub-subsections.
> Or did I misunderstand the problem you were describing?

Done. It seems that a subsection can be directly under a top node
without a section as intermediate step; I didn't know that.

> Yes, but that hint doesn't mention the polling.  I think it's
> important to tell that polling is needed in these rare cases.

Text added.

Thanks again; revised patch attached.

> 

[-- Attachment #2: 0001-Reorganise-auto-revert-nodes-in-the-manual.patch --]
[-- Type: text/x-patch, Size: 10348 bytes --]

From ea7c087df14f302543ce9e2711cc8cbde09dbc30 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 30 Apr 2019 22:35:56 +0200
Subject: [PATCH] Reorganise (auto-)revert nodes in the manual

Put all information about auto-revert into a section of its own, and
organise the text in a more logical way.  Previously it was mainly
described in the section about reverting (bug#35418).

* doc/emacs/files.texi (Files): Adjust menu.
(Reverting, Auto Revert, Autorevert): Add node `Auto Revert' and move
text on that topic from `Reverting', rearranged.  Turn the old
`Autorevert' node into a subsection under `Auto Revert'.
* doc/emacs/arevert-xtra.texi (Autorevert): Rename and turn into
subsubsection.
* doc/emacs/buffers.texi (Several Buffers): Adjust references.
* doc/emacs/emacs.texi (Top): Adjust menu.
* doc/emacs/emacs-xtra.texi (Top): Adjust menu.
(Non-File Buffers): Add node and section lines.
---
 doc/emacs/arevert-xtra.texi |  9 +++--
 doc/emacs/buffers.texi      |  4 +-
 doc/emacs/emacs-xtra.texi   |  2 +-
 doc/emacs/emacs.texi        |  4 +-
 doc/emacs/files.texi        | 75 +++++++++++++++++++++----------------
 5 files changed, 51 insertions(+), 43 deletions(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index cd7c1ff895..9e01a10ace 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -4,8 +4,9 @@
 @c
 @c This file is included either in emacs-xtra.texi (when producing the
 @c printed version) or in the main Emacs manual (for the on-line version).
-@node Autorevert
-@section Auto Reverting Non-File Buffers
+
+@node Non-File Buffers
+@subsection Auto Reverting Non-File Buffers
 
 Global Auto Revert Mode normally only reverts file buffers.  There are
 two ways to auto-revert certain non-file buffers: by enabling Auto
@@ -43,7 +44,7 @@ Autorevert
 @end menu
 
 @node Auto Reverting the Buffer Menu
-@subsection Auto Reverting the Buffer Menu
+@subsubsection Auto Reverting the Buffer Menu
 
 If auto-reverting of non-file buffers is enabled, the Buffer Menu
 @iftex
@@ -65,7 +66,7 @@ Auto Reverting the Buffer Menu
 automatically erasing the marks.
 
 @node Auto Reverting Dired
-@subsection Auto Reverting Dired buffers
+@subsubsection Auto Reverting Dired buffers
 
 Dired buffers only auto-revert when the file list of the buffer's main
 directory changes (e.g., when a new file is added or deleted).  They
diff --git a/doc/emacs/buffers.texi b/doc/emacs/buffers.texi
index 27fcb7369a..14a0a01ca8 100644
--- a/doc/emacs/buffers.texi
+++ b/doc/emacs/buffers.texi
@@ -568,10 +568,10 @@ Several Buffers
 Auto Revert mode applies to the @file{*Buffer List*} buffer only if
 @code{global-auto-revert-non-file-buffers} is non-@code{nil}.
 @iftex
-@inforef{Autorevert,, emacs-xtra}, for details.
+@inforef{Auto Reverting the Buffer Menu,, emacs-xtra}, for details.
 @end iftex
 @ifnottex
-@xref{Autorevert, global-auto-revert-non-file-buffers}, for details.
+@xref{Auto Reverting the Buffer Menu, global-auto-revert-non-file-buffers}, for details.
 @end ifnottex
 
 @node Indirect Buffers
diff --git a/doc/emacs/emacs-xtra.texi b/doc/emacs/emacs-xtra.texi
index dcd8fae1b6..e9231b4e3a 100644
--- a/doc/emacs/emacs-xtra.texi
+++ b/doc/emacs/emacs-xtra.texi
@@ -59,7 +59,7 @@ Top
 * Picture Mode::        Editing pictures made up of characters using
                          the quarter-plane screen model.
 
-* Autorevert::          Auto Reverting non-file buffers.
+* Non-File Buffers::    Auto Reverting non-file buffers.
 * Subdir Switches::     Subdirectory switches in Dired.
 * Advanced Calendar/Diary Usage:: Advanced Calendar/Diary customization.
 * Emerge::              A convenient way of merging two versions of a program.
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index 58ec373029..a34cef221e 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -443,9 +443,7 @@ Top
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto Revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 990b8f1679..c51d076fa2 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -25,9 +25,7 @@ Files
 * Visiting::            Visiting a file prepares Emacs to edit the file.
 * Saving::              Saving makes your changes permanent.
 * Reverting::           Reverting cancels all the changes not saved.
-@ifnottex
-* Autorevert::          Auto Reverting non-file buffers.
-@end ifnottex
+* Auto Revert::         Keeping buffers automatically up-to-date.
 * Auto Save::           Auto Save periodically protects against loss of data.
 * File Aliases::        Handling multiple names for one file.
 * Directories::         Creating, deleting, and listing file directories.
@@ -955,6 +953,11 @@ Reverting
 is not modified.  (If you have edited the text, it would be wrong to
 discard your changes.)
 
+  You can also tell Emacs to revert buffers automatically when their
+visited files change on disk; @pxref{Auto Revert}.
+
+@node Auto Revert
+@section Auto Revert: Keeping buffers automatically up-to-date
 @cindex Global Auto Revert mode
 @cindex mode, Global Auto Revert
 @cindex Auto Revert mode
@@ -962,22 +965,39 @@ Reverting
 @findex global-auto-revert-mode
 @findex auto-revert-mode
 @findex auto-revert-tail-mode
-@vindex auto-revert-interval
-@vindex auto-revert-remote-files
+
+  A buffer can get out of sync with respect to its visited file on
+disk if that file is changed by another program.  To keep it up to
+date, you can enable Auto Revert mode by typing @kbd{M-x auto-revert-mode}.
+This automatically reverts the buffer when its visited file changes on
+disk.  To do the same for all file buffers, type
+@kbd{M-x global-auto-revert-mode} to enable Global Auto Revert mode.
+
+  Auto Revert will not revert a buffer if it has unsaved changes, or if
+its file on disk is deleted or renamed.
+
+  One use of Auto Revert mode is to ``tail'' a file such as a system
+log, so that changes made to that file by other programs are
+continuously displayed.  To do this, just move the point to the end of
+the buffer, and it will stay there as the file contents change.
+However, if you are sure that the file will only change by growing at
+the end, use Auto Revert Tail mode instead
+(@code{auto-revert-tail-mode}).  It is more efficient for this.
+Auto Revert Tail mode also works for remote files.
+
 @vindex auto-revert-verbose
-  You can also tell Emacs to revert buffers periodically.  To do this
-for a specific buffer, enable the minor mode Auto-Revert mode by
-typing @kbd{M-x auto-revert-mode}.  This automatically reverts the
-current buffer when its visited file changes on disk.  To do the same
-for all file buffers, type @kbd{M-x global-auto-revert-mode} to enable
-Global Auto-Revert mode.  These minor modes do not check or revert
-remote files, because that is usually too slow.  This behavior can be
-changed by setting the variable @code{auto-revert-remote-files} to
-non-@code{nil}.
+  When a buffer is auto-reverted, a message is generated.  This can be
+suppressed by setting @code{auto-revert-verbose} to @code{nil}.
+
+@vindex auto-revert-remote-files
+  The Auto Revert modes do not check or revert remote files, because
+that is usually too slow.  This behavior can be changed by setting the
+variable @code{auto-revert-remote-files} to non-@code{nil}.
 
 @cindex file notifications
 @vindex auto-revert-use-notify
-  By default, Auto-Revert mode works using @dfn{file notifications},
+@vindex auto-revert-interval
+  By default, Auto Revert mode works using @dfn{file notifications},
 whereby changes in the filesystem are reported to Emacs by the OS.
 You can disable use of file notifications by customizing the variable
 @code{auto-revert-use-notify} to a @code{nil} value, then Emacs will
@@ -990,31 +1010,20 @@ Reverting
 
 @vindex auto-revert-avoid-polling
 @vindex auto-revert-notify-exclude-dir-regexp
-  By default, Auto-Revert mode will poll files for changes
-periodically even when file notifications are used.  Such polling is
-usually unnecessary, and turning it off may save power by relying on
-notifications only.  To do so, set the variable
+  By default, Auto Revert mode will poll files for changes
+periodically even when file notifications are used.  Polling is
+unnecessary in many cases, and turning it off may save power by
+relying on notifications only.  To do so, set the variable
 @code{auto-revert-avoid-polling} to non-@code{nil}.  However,
 notification is ineffective on certain file systems; mainly network
 file system on Unix-like machines, where files can be altered from
-other machines.  To force polling when
+other machines.  For such file systems, polling may be necessary.
+To force polling when
 @code{auto-revert-avoid-polling} is non-@code{nil}, set
 @code{auto-revert-notify-exclude-dir-regexp} to match files that
 should be excluded from using notification.
 
-  One use of Auto-Revert mode is to ``tail'' a file such as a system
-log, so that changes made to that file by other programs are
-continuously displayed.  To do this, just move the point to the end of
-the buffer, and it will stay there as the file contents change.
-However, if you are sure that the file will only change by growing at
-the end, use Auto-Revert Tail mode instead
-(@code{auto-revert-tail-mode}).  It is more efficient for this.
-Auto-Revert Tail mode works also for remote files.
-
-  When a buffer is auto-reverted, a message is generated.  This can be
-suppressed by setting @code{auto-revert-verbose} to @code{nil}.
-
-  In Dired buffers (@pxref{Dired}), Auto-Revert mode refreshes the
+  In Dired buffers (@pxref{Dired}), Auto Revert mode refreshes the
 buffer when a file is created or deleted in the buffer's directory.
 
   @xref{VC Undo}, for commands to revert to earlier versions of files
-- 
2.20.1

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Cc: michael.albinus@gmx.de, 35418@debbugs.gnu.org
> Date: Sat, 04 May 2019 13:21:19 +0200
> 
> Thanks again; revised patch attached.

LGTM, thanks.

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

[Michael Albinus]

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

Hi Mattias,

> +  By default, Auto Revert mode will poll files for changes
> +periodically even when file notifications are used.  Polling is
> +unnecessary in many cases, and turning it off may save power by
> +relying on notifications only.  To do so, set the variable
>  @code{auto-revert-avoid-polling} to non-@code{nil}.  However,
>  notification is ineffective on certain file systems; mainly network
>  file system on Unix-like machines, where files can be altered from
> +other machines.  For such file systems, polling may be necessary.

Maybe we could also add the other major reason why polling is necessary:
file notification is not supported for all types of remote file systems
(Tramp).

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  35418@debbugs.gnu.org
> Date: Sat, 04 May 2019 18:53:07 +0200
> 
> Maybe we could also add the other major reason why polling is necessary:
> file notification is not supported for all types of remote file systems
> (Tramp).

Yes, I think all the reasons should be mentioned, so that users who
disable polling know what they are/will be losing, and could make
educated decisions.

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

[Mattias Engdegård]

4 maj 2019 kl. 18.53 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Maybe we could also add the other major reason why polling is necessary:
> file notification is not supported for all types of remote file systems
> (Tramp).

Is the problem that notification over Tramp may fail to activate or suddenly fail functioning, in both cases without any indication of error to file-notify?

If so, we should take care of this in code instead. Assuming the above, I suggest that file-notify consider all notification from file name handlers to be unreliable, and provide an interface to auto-revert, which will then keep polling those files.

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

[Michael Albinus]

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

Hi Mattias,

>> Maybe we could also add the other major reason why polling is necessary:
>> file notification is not supported for all types of remote file systems
>> (Tramp).
>
> Is the problem that notification over Tramp may fail to activate or
> suddenly fail functioning, in both cases without any indication of
> error to file-notify?

Yes we have both cases. But file-notify is told about.

The first case is that file notification cannot be activated. Several
Tramp methods do not support this, and they return nil for the
respective file-notify-add-watch call, as specified. auto-revert
understands this, and knows that the respective file must be handled by
polling.

The second case is, that file notification for a remote file ceases to
work. Since Tramp implements file notification as asynchronous
processes, this could happen if the respective process is killed on the
remote side, or if the connection between the local Emacs and the remote
host is broken temporarily. The respective process shall own a sentinel,
which sends a "stopped" event in this case. I've just checked the code;
this is not implemented. Will do.

> If so, we should take care of this in code instead. Assuming the
> above, I suggest that file-notify consider all notification from file
> name handlers to be unreliable, and provide an interface to
> auto-revert, which will then keep polling those files.

We have this interface already. For file-notify-add-watch it works
already as expected. For broken notifications, the interface is the
"stopped" event, which must (will be) implemented in Tramp.

Btw, I believe all this file notification vs polling behavior is not
covered yet in autorevert-tests.el. Would you like to add respective
test cases for local and remote files? Some days ago I've added tests
for remote files; you could use this mechanism as well.

Best regards, Michael.

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

[Michael Albinus]

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

Hi Mattias,

> The second case is, that file notification for a remote file ceases to
> work. Since Tramp implements file notification as asynchronous
> processes, this could happen if the respective process is killed on the
> remote side, or if the connection between the local Emacs and the remote
> host is broken temporarily. The respective process shall own a sentinel,
> which sends a "stopped" event in this case. I've just checked the code;
> this is not implemented. Will do.

Implemented, pushed to master.

Best regards, Michael.

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

[Mattias Engdegård]

4 maj 2019 kl. 22.31 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> The second case is, that file notification for a remote file ceases to
>> work. Since Tramp implements file notification as asynchronous
>> processes, this could happen if the respective process is killed on the
>> remote side, or if the connection between the local Emacs and the remote
>> host is broken temporarily. The respective process shall own a sentinel,
>> which sends a "stopped" event in this case. I've just checked the code;
>> this is not implemented. Will do.
> 
> Implemented, pushed to master.

Nice, thank you! Does this mean that we can trust Tramp sufficiently in this respect? That is, notifiers will either fail to be created, fail with a 'stopped' event, or work.

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

[Michael Albinus]

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

Hi Mattias,

> Nice, thank you! Does this mean that we can trust Tramp sufficiently
> in this respect? That is, notifiers will either fail to be created,
> fail with a 'stopped' event, or work.

Yes, I think so. Until the next bug report :-)

Best regards, Michael.

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

[Mattias Engdegård]

5 maj 2019 kl. 10.22 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> Nice, thank you! Does this mean that we can trust Tramp sufficiently
>> in this respect? That is, notifiers will either fail to be created,
>> fail with a 'stopped' event, or work.
> 
> Yes, I think so. Until the next bug report :-)

Good. I've pushed the manual update.

What remains is avoiding polling in global-auto-revert-mode. I'll send a patch soon.

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

[Mattias Engdegård]

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

Here is a detail that needs to be taken care of: we should not use notification on non-file buffers, since that is generally useless (consider the Buffer List buffer) and, with `auto-revert-avoid-polling', prevents these buffers from being polled and thus updated at all.

Patch attached. There is a hack for Dired buffers, since watching their directories happens to be just what we want. It would be nice to generalise this condition somehow, but meanwhile this will have to do, unless you can come up with something better.

A patch for global-auto-revert-mode will come shortly.


[-- Attachment #2: 0001-Don-t-use-file-notification-on-non-file-buffers.patch --]
[-- Type: application/octet-stream, Size: 1868 bytes --]

From 23f8e60c110a372530a839948d7b17598517d928 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 8 May 2019 00:02:59 +0200
Subject: [PATCH] Don't use file notification on non-file buffers

Most non-file buffers aren't served by file notification in
auto-revert mode; typically, they need to be polled, like the Buffer List.
With `auto-revert-avoid-polling', setting a useless notification means
that such buffers may never be updated at all (bug#35418).

An exception is made for Dired, as that mode works well with
notification on the directory.

* lisp/autorevert.el (auto-revert-buffers):
Modify condition for using notification.
---
 lisp/autorevert.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 7cd5e7ee8b..5a62b5f6b9 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -858,8 +858,14 @@ the timer when no buffers need to be checked."
                   (auto-revert-remove-current-buffer))
               (when (auto-revert-active-p)
                 ;; Enable file notification.
+                ;; Don't bother creating a notifier for non-file buffers
+                ;; if there is a custom `revert-buffer-function'.
+                ;; An exception is made for Dired, since that mode works
+                ;; well with notifiers.
                 (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor))
+                           (not auto-revert-notify-watch-descriptor)
+                           (or buffer-file-name
+                               (eq major-mode 'dired-mode)))
                   (auto-revert-notify-add-watch))
                 (auto-revert-handler)))))
 	(setq bufs (cdr bufs)))
-- 
2.20.1 (Apple Git-117)

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 8 May 2019 10:34:13 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 35418@debbugs.gnu.org
> 
> +                ;; Don't bother creating a notifier for non-file buffers
> +                ;; if there is a custom `revert-buffer-function'.
> +                ;; An exception is made for Dired, since that mode works
> +                ;; well with notifiers.
>                  (when (and auto-revert-use-notify
> -                           (not auto-revert-notify-watch-descriptor))
> +                           (not auto-revert-notify-watch-descriptor)
> +                           (or buffer-file-name
> +                               (eq major-mode 'dired-mode)))

Is Dired the only exception from the rule?  Or is there a more general
indication that a non-file buffer may want to be automatically
reverted?  E.g., what about Info buffers? revert-buffer does work
there.

Thanks.

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

[Mattias Engdegård]

8 maj 2019 kl. 10.47 skrev Eli Zaretskii <eliz@gnu.org>:

> Is Dired the only exception from the rule?  Or is there a more general
> indication that a non-file buffer may want to be automatically
> reverted?  E.g., what about Info buffers? revert-buffer does work
> there.

It's not about whether revert-buffer works, but whether notification on default-directory is a reliable replacement for polling.

Info actually isn't a good example, since it doesn't even work with polling: it has no special `buffer-stale-function', and therefore isn't able to tell when the buffer is out of date. Furthermore, notification on a directory may not indicate changes to any of the files in it and thus wouldn't be reliable anyway.

With the patch, auto-revert on non-file buffers will work where at all possible; it just won't use notification for buffers other than Dired.

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 8 May 2019 12:18:30 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>, 35418@debbugs.gnu.org
> 
> With the patch, auto-revert on non-file buffers will work where at all possible; it just won't use notification for buffers other than Dired.

I asked whether Dired is the only exception, or is there some more
general sign that a buffer could use notifications.  I'm still not
sure what the answer is to that question.  I don't have any doubt that
Dired will work with notifications, but is the only way to have
exceptions is by naming their major modes explicitly?

IOW, I wonder whether your proposed patch could be made more general
in some way.

Thanks.

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

[Mattias Engdegård]

8 maj 2019 kl. 12.58 skrev Eli Zaretskii <eliz@gnu.org>:

> IOW, I wonder whether your proposed patch could be made more general
> in some way.

So do I, but since I could not come up with one, this ad-hoc solution appeared as a placeholder.
The code can be improved later on, but the patch is a strict improvement on the code in master.

An example of a generalisation: We could add a buffer-specific variable that tells autorevert that yes, this buffer can rely on directory notifications despite not having a buffer-file-name. All modes to which this applies would need to set that variable.

I don't think there is a passive condition, and furthermore, because of the nature of directory notifications, any mode that qualifies is likely to be some kind of variation on Dired: a buffer whose contents is determined by the set of files in its default-directory, but not their data.

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 8 May 2019 13:48:51 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>, 35418@debbugs.gnu.org
> 
> the patch is a strict improvement on the code in master.

I have no doubt it is.

> An example of a generalisation: We could add a buffer-specific variable that tells autorevert that yes, this buffer can rely on directory notifications despite not having a buffer-file-name. All modes to which this applies would need to set that variable.
> 
> I don't think there is a passive condition, and furthermore, because of the nature of directory notifications, any mode that qualifies is likely to be some kind of variation on Dired: a buffer whose contents is determined by the set of files in its default-directory, but not their data.

OK, so how about adding such a variable as part of this improvement?

Or maybe it's better to have a special property on the mode's symbol?

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

[Mattias Engdegård]

8 maj 2019 kl. 14.35 skrev Eli Zaretskii <eliz@gnu.org>:
> 
>> An example of a generalisation: We could add a buffer-specific variable that tells autorevert that yes, this buffer can rely on directory notifications despite not having a buffer-file-name. All modes to which this applies would need to set that variable.
>> 
>> I don't think there is a passive condition, and furthermore, because of the nature of directory notifications, any mode that qualifies is likely to be some kind of variation on Dired: a buffer whose contents is determined by the set of files in its default-directory, but not their data.
> 
> OK, so how about adding such a variable as part of this improvement?

If it's all the same to you, I'd like to keep it separate. This patch fixes a bug: some buffers, like Buffer Menu, are not updated automatically at all when `auto-revert-avoid-polling' is set.
What you are talking about is making some dired-like buffers update 2.5 s faster on average, if they set the right variable.

> Or maybe it's better to have a special property on the mode's symbol?

Maybe a variable goes better with `revert-buffer-function' and `buffer-stale-function' already in use by autorevert for related purposes, although I'm eager to hear what Michael has to say (about this and everything else).

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

[Michael Albinus]

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

> I'm eager to hear what Michael has to say (about
> this and everything else).

I'm very busy @work just now; will react when possible. Latest on
weekend, hopefully earlier. Sorry.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 8 May 2019 14:58:47 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>, 35418@debbugs.gnu.org
> 
> > OK, so how about adding such a variable as part of this improvement?
> 
> If it's all the same to you, I'd like to keep it separate.

I don't understand why: the change as it is looks a bit unclean to me,
whereas the effort to make it cleaner and more future-proof is not a
substantial one.

But if you insist, I won't argue.

> What you are talking about is making some dired-like buffers update 2.5 s faster on average, if they set the right variable.
> 
> > Or maybe it's better to have a special property on the mode's symbol?
> 
> Maybe a variable goes better with `revert-buffer-function' and `buffer-stale-function' already in use by autorevert for related purposes

Maybe.  Those variables are used to name a function, whereas here we
are talking about a variable that will serve just as a flag.  So I
think this is different, though I don't have strong feelings.

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

[Mattias Engdegård]

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

8 maj 2019 kl. 15.28 skrev Eli Zaretskii <eliz@gnu.org>:
>> 
>>> OK, so how about adding such a variable as part of this improvement?
>> 
>> If it's all the same to you, I'd like to keep it separate.
> 
> I don't understand why: the change as it is looks a bit unclean to me,
> whereas the effort to make it cleaner and more future-proof is not a
> substantial one.

How about dropping the Dired special case for the time being? Then the immediate bug is fixed, and all we need to worry about is the 2.5 second delay to Dired auto-revert updates, which we can fix separately in a manner of our choosing.

Simplified patch attached.

[-- Attachment #2: 0001-Don-t-use-file-notification-on-non-file-buffers.patch --]
[-- Type: application/octet-stream, Size: 1383 bytes --]

From 0ac97e913ffb3096934c64715f12921af5fce64a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 8 May 2019 00:02:59 +0200
Subject: [PATCH] Don't use file notification on non-file buffers

Most non-file buffers aren't served by file notification in
auto-revert mode; typically, they need to be polled, like the Buffer List.
With `auto-revert-avoid-polling', setting a useless notification means
that such buffers may never be updated at all (bug#35418).

* lisp/autorevert.el (auto-revert-buffers):
Modify condition for using notification.
---
 lisp/autorevert.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 7cd5e7ee8b..fbaffbf0d6 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -859,7 +859,8 @@ the timer when no buffers need to be checked."
               (when (auto-revert-active-p)
                 ;; Enable file notification.
                 (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor))
+                           (not auto-revert-notify-watch-descriptor)
+                           buffer-file-name)
                   (auto-revert-notify-add-watch))
                 (auto-revert-handler)))))
 	(setq bufs (cdr bufs)))
-- 
2.20.1 (Apple Git-117)

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 8 May 2019 16:13:37 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>, 35418@debbugs.gnu.org
> 
> >> If it's all the same to you, I'd like to keep it separate.
> > 
> > I don't understand why: the change as it is looks a bit unclean to me,
> > whereas the effort to make it cleaner and more future-proof is not a
> > substantial one.
> 
> How about dropping the Dired special case for the time being?

That's too drastic, IMO.  Feel free to push the original patch.

Thanks.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> How about dropping the Dired special case for the time being?
>
> That's too drastic, IMO.  Feel free to push the original patch.

I agree with Eli, we would loose too much. A directory might not change
for hours, polling for this every 5 seconds is expensive. Think about a
remote directory, for example.

> Thanks.

Best regards, Michael.

PS: I hope to find tomorrow the time to review the patch.

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

[Michael Albinus]

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

Hi,

> An example of a generalisation: We could add a buffer-specific
> variable that tells autorevert that yes, this buffer can rely on
> directory notifications despite not having a buffer-file-name. All
> modes to which this applies would need to set that variable.

I sympathize with this proposal. There shall be an indication that a
buffer could be auto-reverted by file notifications. This indication is
either a non-nil buffer-file-name, or a non-nil buffer-local variable
(let's call it buffer-auto-revert-by-file-notification-aware; I'm open
to any better name).

Non-file buffers which could be auto-reverted are those which provide a
buffer-stale-function. A short scan in vanilla Emacs shows
Buffer-menu-mode and dired-mode, which set buffer-stale-function.

Buffer-menu-mode does not use files, so it doesn't profit from file
notifications. One could write another kind of notification which fires
when buffers are created or deleted, but that's another story. And I
doubt it will be more useful than the current auto-reverting for buffer
lists.

So indeed, dired is left for vanilla Emacs. It shall set
buffer-auto-revert-by-file-notification-aware when a buffer is setup to
dired-mode. Other packages in the wild could do similar settings, think
about vc-dir or magit, which use their own machinery. Potentially, any
mode which uses (an own implementation of) revert-buffer, would be a
candidate for this kind of auto-revert.

Thinking about, I'm even not confident that a static value of this
indication is sufficient. In dired, it might be set to t when the dired
buffer is setup. But what if the dired buffer contains subdirectories?
Is it still possible to indicate this by file notifications over
default-directory? Don't know, maybe not, and the variable has to be set
to nil ...

Long story short: we shall start with dired, which sets a buffer-local
variable as indication, and we shall edocument this in the Elisp
manual. Let's see where we go.

Best regards, Michael.

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

[Mattias Engdegård]

9 maj 2019 kl. 13.50 skrev Michael Albinus <michael.albinus@gmx.de>:

> Thinking about, I'm even not confident that a static value of this
> indication is sufficient. In dired, it might be set to t when the dired
> buffer is setup. But what if the dired buffer contains subdirectories?
> Is it still possible to indicate this by file notifications over
> default-directory? Don't know, maybe not, and the variable has to be set
> to nil ...

There is a third possibility: a buffer requires polling, but could make use of notification. The distinction is only important when `auto-revert-avoid-polling' is set. For example, we could have, say, `buffer-auto-revert-by-notification' take the values nil, notify-only and notify-and-poll. This permits immediate updates for some changes while still not keeping stale information indefinitely.

On the other hand, maybe that sort of mode-specific complexity is better left to the modes themselves?

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

[Michael Albinus]

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

Hi Mattias,

>> Thinking about, I'm even not confident that a static value of this
>> indication is sufficient. In dired, it might be set to t when the dired
>> buffer is setup. But what if the dired buffer contains subdirectories?
>> Is it still possible to indicate this by file notifications over
>> default-directory? Don't know, maybe not, and the variable has to be set
>> to nil ...
>
> There is a third possibility: a buffer requires polling, but could
> make use of notification. The distinction is only important when
> `auto-revert-avoid-polling' is set. For example, we could have, say,
> `buffer-auto-revert-by-notification' take the values nil, notify-only
> and notify-and-poll. This permits immediate updates for some changes
> while still not keeping stale information indefinitely.

I haven't seen this requirement yet for any mode. Let's postpone this,
until there is a real request for this kind of distinction.

Furthermore, we shouldn't expect a deep knowledge from major mode
writers about the internals of auto-revert. People might have a hard
time to understand the difference between notify-only and
notify-and-poll.

Best regards, Michael.

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

[Mattias Engdegård]

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

sön 2019-05-12 klockan 10:48 +0200 skrev Michael Albinus:
> 
> I haven't seen this requirement yet for any mode. Let's postpone
> this,
> until there is a real request for this kind of distinction.

Very well. Here is an updated patch, with a new buffer-local variable
controlling whether non-file buffers can rely on notification in
autorevert.



[-- Attachment #2: 0001-Don-t-use-file-notification-on-non-file-buffers.patch --]
[-- Type: text/x-patch, Size: 5364 bytes --]

From 3240d4950eeccf6e32b9d71d0fa8bbb9dfcbbb89 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 8 May 2019 00:02:59 +0200
Subject: [PATCH] Don't use file notification on non-file buffers

Most non-file buffers aren't served by file notification in
auto-revert mode; typically, they need to be polled, like the Buffer List.
With `auto-revert-avoid-polling', setting a useless notification means
that such buffers may never be updated at all (bug#35418).

Non-file buffers can explicitly declare that notification on their
default-directory is sufficient to know when updates are required
by setting the new variable `buffer-auto-revert-by-notification' to
a non-nil value.

* lisp/autorevert.el (auto-revert-buffers):
Modify condition for using notification.
* lisp/files.el (buffer-auto-revert-by-notification): New variable.
* lisp/dired.el (dired-mode): Set buffer-auto-revert-by-notification.
* doc/emacs/arevert-xtra.texi (Non-File Buffers): Document new variable.
* etc/NEWS (Changes in Specialized Modes and Packages): Describe new variable.
---
 doc/emacs/arevert-xtra.texi |  8 ++++++++
 etc/NEWS                    |  8 ++++++++
 lisp/autorevert.el          |  6 +++++-
 lisp/dired.el               |  1 +
 lisp/files.el               | 10 ++++++++++
 5 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index 9e01a10ace..4a2c8c8942 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -35,6 +35,14 @@ Non-File Buffers
 messages while reverting, even when @code{auto-revert-verbose} is
 non-@code{nil}.
 
+@vindex buffer-auto-revert-by-notification
+Some non-file buffers can be updated reliably by file notification on
+their default directory.  This can be indicated by setting
+@code{buffer-auto-revert-by-notification} to a non-@code{nil} value in
+that buffer, allowing Auto Revert to avoid periodic polling.  Such
+notification does not include changes to files in that directory, only
+to the directory itself.
+
 The details depend on the particular types of buffers and are
 explained in the corresponding sections.
 
diff --git a/etc/NEWS b/etc/NEWS
index d10a553244..669adfea2a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1422,6 +1422,14 @@ of an idle Emacs, but may fail on some network file systems; set
 notification is not supported.  The new variable currently has no
 effect in 'global-auto-revert-mode'.  The default value is nil.
 
+*** New variable 'buffer-auto-revert-by-notification'
+Non-file buffers can explicitly declare that notification on their
+default-directory is sufficient to know when updates are required by
+setting the new variable `buffer-auto-revert-by-notification' to a
+non-nil value in that buffer.  Auto Revert mode can use this
+information to avoid polling the buffer periodically when
+'auto-revert-avoid-polling' is non-nil.
+
 \f
 * New Modes and Packages in Emacs 27.1
 
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 7cd5e7ee8b..197a2bf157 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -858,8 +858,12 @@ auto-revert-buffers
                   (auto-revert-remove-current-buffer))
               (when (auto-revert-active-p)
                 ;; Enable file notification.
+                ;; Don't bother creating a notifier for non-file buffers
+                ;; unless it explicitly indicates that this works.
                 (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor))
+                           (not auto-revert-notify-watch-descriptor)
+                           (or buffer-file-name
+                               buffer-auto-revert-by-notification))
                   (auto-revert-notify-add-watch))
                 (auto-revert-handler)))))
 	(setq bufs (cdr bufs)))
diff --git a/lisp/dired.el b/lisp/dired.el
index 385126514b..ea1943de1d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2148,6 +2148,7 @@ dired-mode
     (setq buffer-invisibility-spec (list t)))
   (setq-local revert-buffer-function #'dired-revert)
   (setq-local buffer-stale-function #'dired-buffer-stale-p)
+  (setq-local buffer-auto-revert-by-notification t)
   (setq-local page-delimiter "\n\n")
   (setq-local dired-directory (or dirname default-directory))
   ;; list-buffers uses this to display the dir being edited in this buffer.
diff --git a/lisp/files.el b/lisp/files.el
index 8477c227bc..becb5aab6f 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5843,6 +5843,16 @@ buffer-stale-function
 For more information on how this variable is used by Auto Revert mode,
 see Info node `(emacs)Supporting additional buffers'.")
 
+(defvar-local buffer-auto-revert-by-notification nil
+  "Whether a buffer can rely on notification in Auto-Revert mode.
+If non-nil, monitoring changes to the directory of the current
+buffer is sufficient for knowing when that buffer needs to be
+updated in Auto Revert Mode.  Such notification does not include
+changes to files in that directory, only to the directory itself.
+
+This variable only applies to buffers where `buffer-file-name' is
+nil; other buffers are tracked by their files.")
+
 (defvar before-revert-hook nil
   "Normal hook for `revert-buffer' to run before reverting.
 The function `revert-buffer--default' runs this.
-- 
2.20.1

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

[Michael Albinus]

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

Hi Mattias,

> --- a/doc/emacs/arevert-xtra.texi
> +++ b/doc/emacs/arevert-xtra.texi
>  
> +@vindex buffer-auto-revert-by-notification
> +Some non-file buffers can be updated reliably by file notification on
> +their default directory.  This can be indicated by setting
> +@code{buffer-auto-revert-by-notification} to a non-@code{nil} value in
> +that buffer, allowing Auto Revert to avoid periodic polling.  Such
> +notification does not include changes to files in that directory, only
> +to the directory itself.

Do we want to say that this is related to a major mode in general? That
means, that the mode function shall set this variable. And do we want to
mention dired-mode as example?
  
> --- a/etc/NEWS
> +++ b/etc/NEWS
>  
> +*** New variable 'buffer-auto-revert-by-notification'
> +Non-file buffers can explicitly declare that notification on their
> +default-directory is sufficient to know when updates are required by
> +setting the new variable `buffer-auto-revert-by-notification' to a
> +non-nil value in that buffer.  Auto Revert mode can use this
> +information to avoid polling the buffer periodically when
> +'auto-revert-avoid-polling' is non-nil.

Same remark. Mention major mode dependency.

Otherwise, the patch LGTM.

Best regards, Michael.

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

[Mattias Engdegård]

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

mån 2019-05-13 klockan 15:35 +0200 skrev Michael Albinus:
> 
> Do we want to say that this is related to a major mode in general?
> That
> means, that the mode function shall set this variable. And do we want
> to
> mention dired-mode as example?

Good point, paragraph changed.

> Same remark. Mention major mode dependency.

Done.

Updated patch attached.


[-- Attachment #2: 0001-Don-t-use-file-notification-on-non-file-buffers.patch --]
[-- Type: text/x-patch, Size: 5380 bytes --]

From efda86ef03befb73356c7b23ee21e7d9efd8b181 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 8 May 2019 00:02:59 +0200
Subject: [PATCH] Don't use file notification on non-file buffers

Most non-file buffers aren't served by file notification in
auto-revert mode; typically, they need to be polled, like the Buffer List.
With `auto-revert-avoid-polling', setting a useless notification means
that such buffers may never be updated at all (bug#35418).

Non-file buffers can explicitly declare that notification on their
default-directory is sufficient to know when updates are required
by setting the new variable `buffer-auto-revert-by-notification' to
a non-nil value.

* lisp/autorevert.el (auto-revert-buffers):
Modify condition for using notification.
* lisp/files.el (buffer-auto-revert-by-notification): New variable.
* lisp/dired.el (dired-mode): Set buffer-auto-revert-by-notification.
* doc/emacs/arevert-xtra.texi (Non-File Buffers): Document new variable.
* etc/NEWS (Changes in Specialized Modes and Packages): Describe new variable.
---
 doc/emacs/arevert-xtra.texi |  8 ++++++++
 etc/NEWS                    |  7 +++++++
 lisp/autorevert.el          |  6 +++++-
 lisp/dired.el               |  1 +
 lisp/files.el               | 10 ++++++++++
 5 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/arevert-xtra.texi b/doc/emacs/arevert-xtra.texi
index 9e01a10ace..37e2f9e581 100644
--- a/doc/emacs/arevert-xtra.texi
+++ b/doc/emacs/arevert-xtra.texi
@@ -35,6 +35,14 @@ Non-File Buffers
 messages while reverting, even when @code{auto-revert-verbose} is
 non-@code{nil}.
 
+@vindex buffer-auto-revert-by-notification
+Some non-file buffers can be updated reliably by file notification on
+their default directory; Dired buffers is an example.  The major mode
+can indicate this by setting @code{buffer-auto-revert-by-notification}
+to a non-@code{nil} value in that buffer, allowing Auto Revert to
+avoid periodic polling.  Such notification does not include changes to
+files in that directory, only to the directory itself.
+
 The details depend on the particular types of buffers and are
 explained in the corresponding sections.
 
diff --git a/etc/NEWS b/etc/NEWS
index 43ad8be1cc..57ef76614f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1428,6 +1428,13 @@ of an idle Emacs, but may fail on some network file systems; set
 notification is not supported.  The new variable currently has no
 effect in 'global-auto-revert-mode'.  The default value is nil.
 
+*** New variable 'buffer-auto-revert-by-notification'
+A major mode can declare that notification on the buffer's default
+directory is sufficient to know when updates are required, by setting
+the new variable `buffer-auto-revert-by-notification' to a non-nil
+value.  Auto Revert mode can use this information to avoid polling the
+buffer periodically when 'auto-revert-avoid-polling' is non-nil.
+
 \f
 * New Modes and Packages in Emacs 27.1
 
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 7cd5e7ee8b..197a2bf157 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -858,8 +858,12 @@ auto-revert-buffers
                   (auto-revert-remove-current-buffer))
               (when (auto-revert-active-p)
                 ;; Enable file notification.
+                ;; Don't bother creating a notifier for non-file buffers
+                ;; unless it explicitly indicates that this works.
                 (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor))
+                           (not auto-revert-notify-watch-descriptor)
+                           (or buffer-file-name
+                               buffer-auto-revert-by-notification))
                   (auto-revert-notify-add-watch))
                 (auto-revert-handler)))))
 	(setq bufs (cdr bufs)))
diff --git a/lisp/dired.el b/lisp/dired.el
index 385126514b..ea1943de1d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2148,6 +2148,7 @@ dired-mode
     (setq buffer-invisibility-spec (list t)))
   (setq-local revert-buffer-function #'dired-revert)
   (setq-local buffer-stale-function #'dired-buffer-stale-p)
+  (setq-local buffer-auto-revert-by-notification t)
   (setq-local page-delimiter "\n\n")
   (setq-local dired-directory (or dirname default-directory))
   ;; list-buffers uses this to display the dir being edited in this buffer.
diff --git a/lisp/files.el b/lisp/files.el
index 8477c227bc..becb5aab6f 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5843,6 +5843,16 @@ buffer-stale-function
 For more information on how this variable is used by Auto Revert mode,
 see Info node `(emacs)Supporting additional buffers'.")
 
+(defvar-local buffer-auto-revert-by-notification nil
+  "Whether a buffer can rely on notification in Auto-Revert mode.
+If non-nil, monitoring changes to the directory of the current
+buffer is sufficient for knowing when that buffer needs to be
+updated in Auto Revert Mode.  Such notification does not include
+changes to files in that directory, only to the directory itself.
+
+This variable only applies to buffers where `buffer-file-name' is
+nil; other buffers are tracked by their files.")
+
 (defvar before-revert-hook nil
   "Normal hook for `revert-buffer' to run before reverting.
 The function `revert-buffer--default' runs this.
-- 
2.20.1

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

[Michael Albinus]

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

Hi Mattias,

> Updated patch attached.

Almost OK, but ...

> --- a/etc/NEWS
> +++ b/etc/NEWS

> +the new variable `buffer-auto-revert-by-notification' to a non-nil

... please quote like 'this'.

From my POV, this can be pushed.

Best regards, Michael.

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

[Mattias Engdegård]

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

Actually, the comment added in the last patch was inaccurate and referred to a previous attempt.
Revised patch attached (the actual condition is unchanged).

[-- Attachment #2: 0001-Don-t-use-file-notification-on-non-file-buffers.patch --]
[-- Type: application/octet-stream, Size: 1763 bytes --]

From 52935159130964f7a4979840985c923a8d84a9ac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 8 May 2019 00:02:59 +0200
Subject: [PATCH] Don't use file notification on non-file buffers

Most non-file buffers aren't served by file notification in
auto-revert mode; typically, they need to be polled, like the Buffer List.
With `auto-revert-avoid-polling', setting a useless notification means
that such buffers may never be updated at all (bug#35418).

An exception is made for Dired, as that mode works well with
notification on the directory.

* lisp/autorevert.el (auto-revert-buffers):
Modify condition for using notification.
---
 lisp/autorevert.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 7cd5e7ee8b..37cf997c4e 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -858,8 +858,12 @@ the timer when no buffers need to be checked."
                   (auto-revert-remove-current-buffer))
               (when (auto-revert-active-p)
                 ;; Enable file notification.
+                ;; Don't bother creating a notifier for non-file buffers
+                ;; except for Dired, since that mode works well with notifiers.
                 (when (and auto-revert-use-notify
-                           (not auto-revert-notify-watch-descriptor))
+                           (not auto-revert-notify-watch-descriptor)
+                           (or buffer-file-name
+                               (eq major-mode 'dired-mode)))
                   (auto-revert-notify-add-watch))
                 (auto-revert-handler)))))
 	(setq bufs (cdr bufs)))
-- 
2.20.1 (Apple Git-117)


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

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

[Mattias Engdegård]

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

5 maj 2019 kl. 11.58 skrev Mattias Engdegård <mattiase@acm.org>:
> 
> What remains is avoiding polling in global-auto-revert-mode. I'll send a patch soon.

Here is that patch.

I understand that some people are queasy about using advice in code like this, and am open to suggestions about alternatives.
What the code needs is a reasonable (not necessarily bullet-proof) way to detect new file buffers and changes to buffer-file-name of those buffers. Monitoring `find-file-noselect' and `set-visited-file-name' turned out to be good enough.
For the former, it might be possible to get away with `after-change-major-mode-hook' instead (already used for non-file buffers).


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

From 34302c20db88917a9a5bea90a70d315b44f1647d 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

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 `global-auto-revert--tracked-buffer'.  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.
(Bug#35418).

* lisp/autorevert.el (auto-revert-avoid-polling): Amend doc string.
(global-auto-revert--tracked-buffer): 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-buffer)
(auto-revert--find-file-noselect-advice)
(auto-revert--set-visited-file-name-advice)
(auto-revert--after-change-major-mode): 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.
* etc/NEWS (Changes in Specialized Modes and Packages): Update entry.
---
 etc/NEWS           |   3 +-
 lisp/autorevert.el | 133 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 108 insertions(+), 28 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 9e3559d27e..56c7163f7f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1396,8 +1396,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.
 
 \f
 * New Modes and Packages in Emacs 27.1
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index fbaffbf0d6..402301c448 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 global-auto-revert--tracked-buffer 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,118 @@ 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
+        (mapc #'auto-revert--global-add-buffer (buffer-list))
+        ;; Make sure future buffers are added as well.
+        (advice-add 'find-file-noselect :filter-return
+                    #'auto-revert--find-file-noselect-advice)
+        (advice-add 'set-visited-file-name :after
+                    #'auto-revert--set-visited-file-name-advice)
+        ;; 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--after-change-major-mode))
+        (auto-revert-buffers))
+    ;; Turn global-auto-revert-mode OFF.
+    (remove-hook 'after-change-major-mode-hook
+                 #'auto-revert--after-change-major-mode)
+    (advice-remove 'set-visited-file-name
+                   #'auto-revert--set-visited-file-name-advice)
+    (advice-remove 'find-file-noselect
+                   #'auto-revert--find-file-noselect-advice)
     (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 global-auto-revert--tracked-buffer
+          (setq global-auto-revert--tracked-buffer 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-buffer (buffer)
+  "Set BUFFER to be tracked by Global Auto-Revert if appropriate."
+  (with-current-buffer buffer
+    (when (and (not global-auto-revert--tracked-buffer)
+               (or buffer-file-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.
+                   (and global-auto-revert-non-file-buffers
+                        (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 global-auto-revert--tracked-buffer t))))
+
+(defun auto-revert--find-file-noselect-advice (buffer)
+  "Adopt BUFFER for Global Auto-Revert if appropriate.
+Called with the return value of `find-file-noselect'."
+  (auto-revert--global-add-buffer buffer)
+  (auto-revert-set-timer)
+  buffer)
+
+(defun auto-revert--set-visited-file-name-advice (&rest _)
+  "Adopt the current buffer for Global Auto-Revert if appropriate.
+Called after `set-visited-file-name'."
+  ;; In case the file name was changed, remove any existing notifier
+  ;; first so that we don't track the wrong file.
+  (when auto-revert-notify-watch-descriptor
+    (auto-revert-notify-rm-watch))
+  (auto-revert--global-add-buffer (current-buffer))
+  (auto-revert-set-timer))
+
+(defun auto-revert--after-change-major-mode ()
+  "Adopt the current buffer for Global Auto-Revert if appropriate.
+Called after the current buffer got a new major mode."
+  (auto-revert--global-add-buffer (current-buffer))
+  (auto-revert-set-timer))
 
 (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
+                             'global-auto-revert--tracked-buffer 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
+                          'global-auto-revert--tracked-buffer 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 +736,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 +783,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)))))
+      global-auto-revert--tracked-buffer))
 
 (defun auto-revert-handler ()
   "Revert current buffer, if appropriate.
-- 
2.20.1 (Apple Git-117)

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 9 May 2019 12:00:17 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 35418@debbugs.gnu.org
> 
> What the code needs is a reasonable (not necessarily bullet-proof) way to detect new file buffers and changes to buffer-file-name of those buffers.

Did you consider utilizing buffer-list-update-hook?

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

[Mattias Engdegård]

9 maj 2019 kl. 12.48 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Did you consider utilizing buffer-list-update-hook?

It triggers far too often, including changes in the list order, creation and removal of temporary buffers, and so on. Furthermore it does not say what changed; a diff would have to be computed each time. Finally, neither the major mode nor buffer-file-name has been set at that point.

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

[Michael Albinus]

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

Hi Mattias,

> I understand that some people are queasy about using advice in code
> like this, and am open to suggestions about alternatives.

It's not just being qeasy. It is a design rule, that advising functions
do not belong to core Emacs.

> What the code needs is a reasonable (not necessarily bullet-proof) way
> to detect new file buffers and changes to buffer-file-name of those
> buffers. Monitoring `find-file-noselect' and `set-visited-file-name'
> turned out to be good enough.
> For the former, it might be possible to get away with
> `after-change-major-mode-hook' instead (already used for non-file
> buffers).

There is `find-file-hook'. If we need to hook into
`set-visited-file-name', we shall create a new hook
`after-set-visited-file-name', and run it there.

> +(defvar-local global-auto-revert--tracked-buffer nil
> +  "Non-nil if buffer is handled by Global Auto-Revert mode.")
> +

Somehow, I'm not so comfortable with that name. Could we take
`auto-revert-global-mode'? It is similar to `auto-revert-mode' and
`auto-revert-tail-mode', with the disadvantage that there does not exist
such a mode.

Alternatively, we could create a local variable `global-autorevert-mode'
in buffers which are tracked, and check always for that local value
where it matters.

> +(defun auto-revert--find-file-noselect-advice (buffer)
> +  "Adopt BUFFER for Global Auto-Revert if appropriate.
> +Called with the return value of `find-file-noselect'."
> +  (auto-revert--global-add-buffer buffer)
> +  (auto-revert-set-timer)
> +  buffer)
> +
> +(defun auto-revert--after-change-major-mode ()
> +  "Adopt the current buffer for Global Auto-Revert if appropriate.
> +Called after the current buffer got a new major mode."
> +  (auto-revert--global-add-buffer (current-buffer))
> +  (auto-revert-set-timer))

These are almost identical. Make argument buffer optional, and it is
just one function.

Best regards, Michael.

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

[Mattias Engdegård]

10 maj 2019 kl. 11.49 skrev Michael Albinus <michael.albinus@gmx.de>:

> There is `find-file-hook'. If we need to hook into
> `set-visited-file-name', we shall create a new hook
> `after-set-visited-file-name', and run it there.

Thank you, it looks like `find-file-hook' will do. It's `set-visited-file-name' that lacks a hook. We could add one, `after-visited-file-name-change-hook' say, and run it at the end of that function. It would come in handy for fixing the write-file bug, too.

That would suffice for this particular need, but we may contemplate some variations for general utility, such as passing the old value of buffer-file-name to the hook. It also wouldn't catch direct modifications of buffer-file-name, but that mostly happens in  special buffers that we don't want to autorevert anyway (?).

Perhaps we should exclude all buffers whose name start with a space from any kind of auto-revert, just in case.

>> +(defvar-local global-auto-revert--tracked-buffer nil
>> +  "Non-nil if buffer is handled by Global Auto-Revert mode.")
>> +
> 
> Somehow, I'm not so comfortable with that name. Could we take
> `auto-revert-global-mode'? It is similar to `auto-revert-mode' and
> `auto-revert-tail-mode', with the disadvantage that there does not exist
> such a mode.

Agreed, and I never liked that variable name much myself. What about `auto-revert--global-mode'? (More names in autorevert.el should have double dashes, but I suppose it was written before that convention came along.)

> Alternatively, we could create a local variable `global-autorevert-mode'
> in buffers which are tracked, and check always for that local value
> where it matters.

Possibly, but that sounds slightly more error-prone.

>> +(defun auto-revert--find-file-noselect-advice (buffer)
[..]
>> +(defun auto-revert--after-change-major-mode ()
> 
> These are almost identical. Make argument buffer optional, and it is
> just one function.

Good point, but the advice functions will probably be replaced anyway per your request; let's see what it looks like when that is done.

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

[Michael Albinus]

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

> That would suffice for this particular need, but we may contemplate
> some variations for general utility, such as passing the old value of
> buffer-file-name to the hook. It also wouldn't catch direct
> modifications of buffer-file-name, but that mostly happens in special
> buffers that we don't want to autorevert anyway (?).

So we must document in the Elisp manual, that buffers, which want to
participate in global-auto-revert-mode after a renaming, shall change
the name via set-visited-file-name.

> Perhaps we should exclude all buffers whose name start with a space
> from any kind of auto-revert, just in case.

Agreed. Those buffers are special (internal) anyway, it's already tricky
to show them. Nobody needs auto-revert for invisible buffers :-)

> Agreed, and I never liked that variable name much myself. What about
> `auto-revert--global-mode'? (More names in autorevert.el should have
> double dashes, but I suppose it was written before that convention
> came along.)

D'accord.

>>> +(defun auto-revert--find-file-noselect-advice (buffer)
> [..]
>>> +(defun auto-revert--after-change-major-mode ()
>> 
>> These are almost identical. Make argument buffer optional, and it is
>> just one function.
>
> Good point, but the advice functions will probably be replaced anyway
> per your request; let's see what it looks like when that is done.

But they will convert to hook functions then ...

Best regards, Michael.

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

[Mattias Engdegård]

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

fre 2019-05-10 klockan 14:43 +0200 skrev Michael Albinus:
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> So we must document in the Elisp manual, that buffers, which want to
> participate in global-auto-revert-mode after a renaming, shall change
> the name via set-visited-file-name.

Are you sure about that? It sounds quite technical. But if you think it
is necessary, I'll add it.

> 
> > Perhaps we should exclude all buffers whose name start with a space
> > from any kind of auto-revert, just in case.
> 
> Agreed. Those buffers are special (internal) anyway, it's already
> tricky
> to show them. Nobody needs auto-revert for invisible buffers :-)

The revised patch now excludes such non-file buffers. I wonder if
buffers with file names should be excluded as well. They trivially
occur when visiting a file whose name starts with a space.

The new patch also has the tracking variable renamed to
`auto-revert--global-mode' and added a new hook,
`after-set-visited-file-name'; the advice calls are gone.


[-- Attachment #2: 0001-Avoid-polling-in-global-auto-revert-mode.patch --]
[-- Type: text/x-patch, Size: 11730 bytes --]

From 7dabf61a47a20f45ecb7e24e98fcf735f6ab1bd2 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

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.

(Bug#35418).

* 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.
* 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 +-
 4 files changed, 103 insertions(+), 29 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 @@ Standard Hooks
 
 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 43ad8be1cc..641fc8e116 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1425,8 +1425,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.
 
 \f
 * New Modes and Packages in Emacs 27.1
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 7cd5e7ee8b..1bf29f04f4 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -312,10 +312,7 @@ auto-revert-avoid-polling
 
 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 @@ auto-revert-buffer-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 @@ global-auto-revert-mode
   :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 @@ auto-revert-notify-handler
                      (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 @@ auto-revert-active-p
   "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 8477c227bc..453a7f4584 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4222,6 +4222,9 @@ change-major-mode-with-file-name
   :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.
@@ -4342,7 +4345,8 @@ set-visited-file-name
 	      (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.
-- 
2.20.1

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

[Michael Albinus]

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

Hi Mattias,

>> So we must document in the Elisp manual, that buffers, which want to
>> participate in global-auto-revert-mode after a renaming, shall change
>> the name via set-visited-file-name.
>
> Are you sure about that? It sounds quite technical. But if you think it
> is necessary, I'll add it.

I'm not sure, of course. But how else can such a buffer participate, if
we don't poll anymore (and refresh the list of buffers to be watched)?
To be tested, I would say.

>> Agreed. Those buffers are special (internal) anyway, it's already
>> tricky to show them. Nobody needs auto-revert for invisible buffers
>> :-)
>
> The revised patch now excludes such non-file buffers. I wonder if
> buffers with file names should be excluded as well. They trivially
> occur when visiting a file whose name starts with a space.

`list-buffers' shows buffers with a leading space, if they are visiting a
file. If they don't visit a file, they are not listed.

I would say we shall apply the same rule.

The patch LGTM. Do you want also add some tests to autorevert-tests.el?

Best regards, Michael.

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

[Mattias Engdegård]

[-- 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)

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

[Michael Albinus]

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

Hi Mattias,

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

Thanks. I've applied it also for inotify, gfile, inotifywait and
gio-monitor. All tests passed, at least the local
ones. auto-revert-test05-global-notify-remote (see below) failed; this
one I could debug myself once the patch has landed in master. I suspect
timing issues.

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.

Comments:

> --- a/lisp/autorevert.el
> +++ b/lisp/autorevert.el

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

Remove "or globally".

> --- a/test/lisp/autorevert-tests.el
> +++ b/test/lisp/autorevert-tests.el

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

> +(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)))

> +  (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)

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

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

> +      (dolist (buf (list buf-1 buf-2 buf-3))
> +        (when (buffer-live-p buf)
> +          (kill-buffer buf)))

Why not (ignore-errors (kill-buffer buf)) ?

Add the remote test case:

(auto-revert--deftest-remote auto-revert-test05-global-notify
  "Test `global-auto-revert-mode' without polling for remote buffers."

Best regards, Michael.

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

[Mattias Engdegård]

[-- 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)

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

[Michael Albinus]

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

Hi Mattias,

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

You can see it just now :-)

From my POV, the patch is fine, and could be committed. Whether it is
successful will be told us by emba.gnu.org (and hydra.nixos.org).

This is the last change wrt bug#35418, isn't it? You could close the bug
then, if both CI systems do not report an error for autorevert-tests or
filenotify-tests.

Best regards, Michael.

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

[Mattias Engdegård]

20 maj 2019 kl. 09.30 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> From my POV, the patch is fine, and could be committed. Whether it is
> successful will be told us by emba.gnu.org (and hydra.nixos.org).

Thanks for your diligence and patience; closing.

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

[Michael Albinus]

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

> Actually, it is (arguably) a bug. With two buffers referring to
> distinct hard links for the same file, surely we want a change in that
> file to trigger notification for both! (It's quite an exotic case, not
> the least because Emacs normally recognises hard links as if they were
> the same file name.)

By design, in filenotify.el, we want see only events which are related
to the file *name*. If you want to be notified for both buffers, you
need to watch both file (names).

(Well, re-reading the docstring and the manual for `file-notify-add-watch',
this isn't said explicitly. Likely, we shall precise this.)

> However, with the kqueue back-end, file-notify watches do trigger for
> both, as expected.

Hmm, this is inconsistent. Worth a buig report?

> The reason is that file-notify does not call inotify-add-watch on
> individual files, as in your example above, but on their containing
> directory ("/tmp" in your example). When monitoring a directory with
> two hard links to the same file, and the file is changed, inotify
> (sensibly) only reports a change to one of the links (the one employed
> for the change). Thus, the logic is in the Linux kernel, not in
> filenotify.
>
> For kqueue it is different: here, changes to files are not reported
> when a watch is monitoring their directory, so filenotify.el sets
> kqueue watches on each file instead. The same could be done with
> inotify (and w32notify, if I read the code right), but watching
> directories has certain advantages.

It was a design decision, that filenotify.el implements directory
watching. Since kqueue does not support this, it must be emulated, somehow.

>> One alternative approach could be to analyze the file system device
>> number, as returned by `file-attributes'. By this, we could detect
>> mounted file systems.
>
> Sort of; the interpretation is tricky, and as Eli commented, quite
> platform-specific.

I'm also not in favor of this approach, I just wanted to mention it.

>> But I don't believe that this information is always trustworty, given it
>> isn't used anywhere. And at least for remote files it doesn't tell you
>> anything. Furthermore, mounted file systems are not the only reason that
>> file notification doesn't work, and we need to poll.
>
> What other reasons are you thinking about?

The reasons you have already quoted somewhere else: sometimes, file
notification is not applicable; there are not enough descriptors left; a
file might have been deleted; a file notification process has been
killed silently; you name it ...

Best regards, Michael.

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

[Mattias Engdegård]

29 apr. 2019 kl. 09.19 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> By design, in filenotify.el, we want see only events which are related
> to the file *name*. If you want to be notified for both buffers, you
> need to watch both file (names).

Well yes, but you want a change to the file to be reported for both buffers, even if they watch different names, right?

Otherwise, it wouldn't make sense at all. If someone is watching a file, surely it is because changes to the contents of that file are of interest? Why would the name employed to carry out the changes matter?

I'm quite sure there is a simple misunderstanding here; probably my fault. And again, I don't think it matters much in practice since users are unlikely to have buffers for different hard links to the same file. Let's not waste too much time on this.

>> However, with the kqueue back-end, file-notify watches do trigger for
>> both, as expected.
> 
> Hmm, this is inconsistent. Worth a buig report?

Not really, because (a) multiple hard links are rare, (b) even more rare in Emacs, and (c) inotify isn't used that way by auto-revert (the directory is watched, not the files).

> It was a design decision, that filenotify.el implements directory
> watching. Since kqueue does not support this, it must be emulated, somehow.

Well, auto-revert only uses filenotify.el for watching changes to files (that is, the data corresponding to the names). How filenotify does that isn't very important. I suppose watching directories when possible has the advantages:

+ fewer (kernel-level) descriptors used if there are multiple files of interest in the same directory
+ notification about re-created previously removed files

with at least one disadvantage:

- changes to files not of interest have to be considered and rejected, spending more CPU and power. This can be non-trivial; consider looking at a single non-changing file in a very busy directory with files being added and removed all the time.

For kqueue and w32notify (and FSEvent) there isn't much choice.

>> What other reasons are you thinking about?
> 
> The reasons you have already quoted somewhere else: sometimes, file
> notification is not applicable; there are not enough descriptors left; a
> file might have been deleted; a file notification process has been
> killed silently; you name it ...

Thank you. Most of those cases should not cause any trouble -- except unreliable file notification processes, but since `auto-revert-remote-files' defaults to nil, it didn't look like a serious problem.

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

[Michael Albinus]

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

>> By design, in filenotify.el, we want see only events which are related
>> to the file *name*. If you want to be notified for both buffers, you
>> need to watch both file (names).
>
> Well yes, but you want a change to the file to be reported for both
> buffers, even if they watch different names, right?

You don't know first hand, which buffers contain the same file hard
linked together. This can be determined only via the inode and device
numbers; something we don't apply yet.

How do you know otherwise, that "/tmp/foo" and "/tmp/bar" are the same,
visited in different buffers?

> Otherwise, it wouldn't make sense at all. If someone is watching a
> file, surely it is because changes to the contents of that file are of
> interest? Why would the name employed to carry out the changes matter?

That's a desirable feature, I agree. But we haven't implemented it
yet. Likely, we shall say so in the doc.

> I'm quite sure there is a simple misunderstanding here; probably my
> fault. And again, I don't think it matters much in practice since
> users are unlikely to have buffers for different hard links to the
> same file. Let's not waste too much time on this.

Agreed. I won't change something in this respect, until there is a bug
report / feature request. And as said, maybe you could add a sentence
about in the manual.

> Thank you. Most of those cases should not cause any trouble -- except
> unreliable file notification processes, but since
> `auto-revert-remote-files' defaults to nil, it didn't look like a
> serious problem.

As Tramp maintainer, I always set `auto-revert-remote-files' to t :-)
So I care.

Best regards, Michael.

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

[Mattias Engdegård]

29 apr. 2019 kl. 14.26 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> You don't know first hand, which buffers contain the same file hard
> linked together. This can be determined only via the inode and device
> numbers; something we don't apply yet.
> 
> How do you know otherwise, that "/tmp/foo" and "/tmp/bar" are the same,
> visited in different buffers?

Thanks, I think I understand what you are concerned about now.
It seems to work just as we expect it to (at least in kqueue and inotify):

A file has three hard links, /dir1/a, /dir1/b and /dir2/c. Then:

- A watch set on /dir1/a will report changes made to the file via any of the three links (kqueue, inotify).
- A watch set on /dir1 will report changes made to the file via /dir1/a and /dir1/b, but not /dir2/c (inotify).

Since filenotify would use /dir1 to watch /dir1/a with inotify, but /dir1/a with kqueue, the behaviour differs.
We could work around the problem by setting watches on files directly with inotify, but it's not worth the trouble or the other drawbacks (as mentioned earlier) for such an uncommon case.

> As Tramp maintainer, I always set `auto-revert-remote-files' to t :-)
> So I care.

Right, so I suppose a user like you would either:

(a) not set `auto-revert-always-poll' to nil
(b) trust remote file notification to work well enough, and if it fails, it's not a disaster (no data lost)
(c) add a pattern to `auto-revert-notify-exclude-dir-regexp' to disable particularly unreliable notifications

which sounds acceptable.

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

[Michael Albinus]

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

> We could work around the problem by setting watches on files directly
> with inotify, but it's not worth the trouble or the other drawbacks
> (as mentioned earlier) for such an uncommon case.

If you go back in history (for years), you will find in the archives
that we have tried different policies. None of them is perfect, so
unless we see serious problems, I propose to keep the things as they are.

>> As Tramp maintainer, I always set `auto-revert-remote-files' to t :-)
>> So I care.
>
> Right, so I suppose a user like you would either:
>
> (a) not set `auto-revert-always-poll' to nil
> (b) trust remote file notification to work well enough, and if it
> fails, it's not a disaster (no data lost)
> (c) add a pattern to `auto-revert-notify-exclude-dir-regexp' to
> disable particularly unreliable notifications
>
> which sounds acceptable.

Don't know what other people do, but I'll take (a). Since I have written
remote file notifications myself, likely I'm the one who has the least
trust for (b) :-) (c) could help, yes.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 29 Apr 2019 20:58:15 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 35418@debbugs.gnu.org
> 
> A file has three hard links, /dir1/a, /dir1/b and /dir2/c. Then:
> 
> - A watch set on /dir1/a will report changes made to the file via any of the three links (kqueue, inotify).
> - A watch set on /dir1 will report changes made to the file via /dir1/a and /dir1/b, but not /dir2/c (inotify).

Just FTR, w32notify reports changes made through any of the 3 links
when it watches dir1.  This is consistent with MS documentation, which
says that changing the file's data are reflected to all the hard links
immediately.

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

[Michael Albinus]

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

> It is a waste of power, on battery-powered devices in particular, to
> poll files in auto-revert mode periodically when change notification
> is used. The change is straightforward (attached patch); the main
> concern is whether the notification system is reliable enough.

An alternative approach is discussed at <https://debbugs.gnu.org/35418>:
set auto-revert-interval to a very large value (like most-positive-fixnum).

Sounds reasonable to me, if we adapt the documentation.

Best regards, Michael.

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

[Mattias Engdegård]

24 apr. 2019 kl. 21.59 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> An alternative approach is discussed at <https://debbugs.gnu.org/35418>:
> set auto-revert-interval to a very large value (like most-positive-fixnum).

Sorry, did you mean a different bug number?

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

[Michael Albinus]

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

>> An alternative approach is discussed at <https://debbugs.gnu.org/35418>:
>> set auto-revert-interval to a very large value (like most-positive-fixnum).
>
> Sorry, did you mean a different bug number?

Oops, I meant <https://emacs.stackexchange.com/a/50134/2427>. Cut'n'waste error, while
following Emacs Berlin meetup in parallel.

Best regards, Michael.

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

[Mattias Engdegård]

25 apr. 2019 kl. 13.04 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
> Oops, I meant <https://emacs.stackexchange.com/a/50134/2427>. Cut'n'waste error, while
> following Emacs Berlin meetup in parallel.

A most understandable error!

Regarding the discussion you refer to, drastically raising auto-revert-interval cannot be a practical solution in the general case, nor was it likely proposed as such. Even with the patch, polling will be used in many cases, including: filenotify failure (from running out of file descriptors, for example); deleted file; and anything matching auto-revert-notify-exclude-dir-regexp.

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

[Zhang Haijun]

> It was a design decision, that filenotify.el implements directory watching. Since kqueue does not support this, it must be emulated, somehow. 
> 

It seems that it is not true for kqueue on macOS 10.13.6.

Several weeks ago, I met a problem with emacs auto-revert. Some files in a directory can’t be auto reverted.  This directory was a soft link to another directory. I did some debug and  found that no event would be received if you use file watching for the files in a soft link directory. And use directory watching for these files worked well. So I modified filenotify.el like this:

diff --git a/lisp/filenotify.el b/lisp/filenotify.el
index 101ddb6be0..a4a0359328 100644
--- a/lisp/filenotify.el
+++ b/lisp/filenotify.el
@@ -363,7 +363,7 @@ file-notify-add-watch
       (setq desc (funcall
                   ;; kqueue does not report file changes in directory
                   ;; monitor.  So we must watch the file itself.
-                  func (if (eq file-notify--library 'kqueue) file dir)
+                  func (if (eq file-notify--library 'kqueue11) file dir)
                   l-flags 'file-notify-callback)))

     ;; Modify `file-notify-descriptors’.

It works well since then.

I don’t known from when the behavior of kqueue changed. There maybe need a user option to control whether to use file watching or directory watching for kqueue.

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

[Michael Albinus]

Zhang Haijun <ccsmile2008@outlook.com> writes:

Hi,

> diff --git a/lisp/filenotify.el b/lisp/filenotify.el
> index 101ddb6be0..a4a0359328 100644
> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> @@ -363,7 +363,7 @@ file-notify-add-watch
>        (setq desc (funcall
>                    ;; kqueue does not report file changes in directory
>                    ;; monitor.  So we must watch the file itself.
> -                  func (if (eq file-notify--library 'kqueue) file dir)
> +                  func (if (eq file-notify--library 'kqueue11) file dir)
>                    l-flags 'file-notify-callback)))
>
>      ;; Modify `file-notify-descriptors’.

I don't understand the patch. Symbol kqueue11 does not exist, so do you mean

+                  func dir

And have you applied the tests in filenotify-tests.el? Do all of them pass?

Best regards, Michael.

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

[Zhang Haijun]

> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
> 
> +                  func dir
Yes.

> 
> And have you applied the tests in filenotify-tests.el? Do all of them pass?
> 
> Best regards, Michael.

$ make -C test autorevert-tests
 ELC      lisp/autorevert-tests.elc
 GEN      lisp/autorevert-tests.log
Running 5 tests (2019-05-01 10:06:38+0800)
Reverting buffer `auto-revert-testqe4qed'.
  passed  1/5  auto-revert-test00-auto-revert-mode
(Shell command succeeded with no output)
Test auto-revert-test01-auto-revert-several-files backtrace:
 signal(ert-test-failed (((should (string-match "another text" (buffe
 ert-fail(((should (string-match "another text" (buffer-string))) :fo
 (if (unwind-protect (setq value-40 (apply fn-38 args-39)) (setq form
 (let (form-description-42) (if (unwind-protect (setq value-40 (apply
 (let ((value-40 (quote ert-form-evaluation-aborted-41))) (let (form-
 (let* ((fn-38 (function string-match)) (args-39 (condition-case err
 (save-current-buffer (set-buffer buf) (auto-revert--wait-for-revert
 (while --dolist-tail-- (setq buf (car --dolist-tail--)) (save-curren
 (let ((--dolist-tail-- (list buf1 buf2)) buf) (while --dolist-tail--
 (progn (write-region "any text" nil tmpfile1 nil (quote no-message))
 (unwind-protect (progn (write-region "any text" nil tmpfile1 nil (qu
 (let* ((auto-revert--messages "") (g30 (function (lambda (msg) (setq
 (unwind-protect (let* ((auto-revert--messages "") (g30 (function (la
 (let* ((cp (executable-find "cp")) (tmpdir1 (make-temp-file "auto-re
 (lambda nil (let* ((fn-23 (function executable-find)) (args-24 (cond
 ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
 ert-run-test(#s(ert-test :name auto-revert-test01-auto-revert-severa
 ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
 ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
 ert-run-tests-batch((not (tag :unstable)))
 ert-run-tests-batch-and-exit((not (tag :unstable)))
 eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
 command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/autorevert-tests.el"
 command-line()
 normal-top-level()
Test auto-revert-test01-auto-revert-several-files condition:
   (ert-test-failed
    ((should
      (string-match "another text"
                    (buffer-string)))
     :form
     (string-match "another text" "any text")
     :value nil))
  FAILED  2/5  auto-revert-test01-auto-revert-several-files
Reverting buffer `auto-revert-test49vRly'.
Reverting buffer `auto-revert-test49vRly'.
Reverting buffer `auto-revert-test49vRly'.
  passed  3/5  auto-revert-test02-auto-revert-deleted-file
Reverting buffer `auto-revert-testvVIyEY'.
  passed  4/5  auto-revert-test03-auto-revert-tail-mode
ls does not support --dired; see `dired-use-ls-dired' for more details.
Reverting buffer `T'.
Reverting buffer `T'.
  passed  5/5  auto-revert-test04-auto-revert-mode-dired

Ran 5 tests, 4 results as expected, 1 unexpected (2019-05-01 10:07:23+0800)

1 unexpected results:
  FAILED  auto-revert-test01-auto-revert-several-files

make[1]: *** [lisp/autorevert-tests.log] Error 1
make: *** [lisp/autorevert-tests] Error 2

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

[Zhang Haijun]

I haven’t run the test before. It seems that directory watching doesn’t work for file content only change. But it works for many common use cases while file watching doesn’t work.

1. Use vim to modify the file and save. It seems that vim will write to a temp file, then remove the original file and then rename the temp file to original file name.
2. Use git to switch branch.
3. For files in a soft link directory.

So directory watching is much more usefull for me than file watching in everyday use. 



> 在 2019年5月1日，上午10:15，张海君 <ccsmile2008@outlook.com> 写道：
> 
> 
>> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
>> 
>> +                  func dir
> Yes.
> 
>> 
>> And have you applied the tests in filenotify-tests.el? Do all of them pass?
>> 
>> Best regards, Michael.
> 
> $ make -C test autorevert-tests
> ELC      lisp/autorevert-tests.elc
> GEN      lisp/autorevert-tests.log
> Running 5 tests (2019-05-01 10:06:38+0800)
> Reverting buffer `auto-revert-testqe4qed'.
>  passed  1/5  auto-revert-test00-auto-revert-mode
> (Shell command succeeded with no output)
> Test auto-revert-test01-auto-revert-several-files backtrace:
> signal(ert-test-failed (((should (string-match "another text" (buffe
> ert-fail(((should (string-match "another text" (buffer-string))) :fo
> (if (unwind-protect (setq value-40 (apply fn-38 args-39)) (setq form
> (let (form-description-42) (if (unwind-protect (setq value-40 (apply
> (let ((value-40 (quote ert-form-evaluation-aborted-41))) (let (form-
> (let* ((fn-38 (function string-match)) (args-39 (condition-case err
> (save-current-buffer (set-buffer buf) (auto-revert--wait-for-revert
> (while --dolist-tail-- (setq buf (car --dolist-tail--)) (save-curren
> (let ((--dolist-tail-- (list buf1 buf2)) buf) (while --dolist-tail--
> (progn (write-region "any text" nil tmpfile1 nil (quote no-message))
> (unwind-protect (progn (write-region "any text" nil tmpfile1 nil (qu
> (let* ((auto-revert--messages "") (g30 (function (lambda (msg) (setq
> (unwind-protect (let* ((auto-revert--messages "") (g30 (function (la
> (let* ((cp (executable-find "cp")) (tmpdir1 (make-temp-file "auto-re
> (lambda nil (let* ((fn-23 (function executable-find)) (args-24 (cond
> ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
> ert-run-test(#s(ert-test :name auto-revert-test01-auto-revert-severa
> ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
> ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
> ert-run-tests-batch((not (tag :unstable)))
> ert-run-tests-batch-and-exit((not (tag :unstable)))
> eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
> command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/autorevert-tests.el"
> command-line()
> normal-top-level()
> Test auto-revert-test01-auto-revert-several-files condition:
>   (ert-test-failed
>    ((should
>      (string-match "another text"
>                    (buffer-string)))
>     :form
>     (string-match "another text" "any text")
>     :value nil))
>  FAILED  2/5  auto-revert-test01-auto-revert-several-files
> Reverting buffer `auto-revert-test49vRly'.
> Reverting buffer `auto-revert-test49vRly'.
> Reverting buffer `auto-revert-test49vRly'.
>  passed  3/5  auto-revert-test02-auto-revert-deleted-file
> Reverting buffer `auto-revert-testvVIyEY'.
>  passed  4/5  auto-revert-test03-auto-revert-tail-mode
> ls does not support --dired; see `dired-use-ls-dired' for more details.
> Reverting buffer `T'.
> Reverting buffer `T'.
>  passed  5/5  auto-revert-test04-auto-revert-mode-dired
> 
> Ran 5 tests, 4 results as expected, 1 unexpected (2019-05-01 10:07:23+0800)
> 
> 1 unexpected results:
>  FAILED  auto-revert-test01-auto-revert-several-files
> 
> make[1]: *** [lisp/autorevert-tests.log] Error 1
> make: *** [lisp/autorevert-tests] Error 2
> 

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

[Zhang Haijun]

If both file watching and directory watching are used for one file, it will works for all use cases.

> 在 2019年5月1日，上午10:59，张海君 <ccsmile2008@outlook.com> 写道：
> 
> I haven’t run the test before. It seems that directory watching doesn’t work for file content only change. But it works for many common use cases while file watching doesn’t work.
> 
> 1. Use vim to modify the file and save. It seems that vim will write to a temp file, then remove the original file and then rename the temp file to original file name.
> 2. Use git to switch branch.
> 3. For files in a soft link directory.
> 
> So directory watching is much more usefull for me than file watching in everyday use. 
> 
> 
> 
>> 在 2019年5月1日，上午10:15，张海君 <ccsmile2008@outlook.com> 写道：
>> 
>> 
>>> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
>>> 
>>> +                  func dir
>> Yes.
>> 
>>> 
>>> And have you applied the tests in filenotify-tests.el? Do all of them pass?
>>> 
>>> Best regards, Michael.
>> 
>> $ make -C test autorevert-tests
>> ELC      lisp/autorevert-tests.elc
>> GEN      lisp/autorevert-tests.log
>> Running 5 tests (2019-05-01 10:06:38+0800)
>> Reverting buffer `auto-revert-testqe4qed'.
>> passed  1/5  auto-revert-test00-auto-revert-mode
>> (Shell command succeeded with no output)
>> Test auto-revert-test01-auto-revert-several-files backtrace:
>> signal(ert-test-failed (((should (string-match "another text" (buffe
>> ert-fail(((should (string-match "another text" (buffer-string))) :fo
>> (if (unwind-protect (setq value-40 (apply fn-38 args-39)) (setq form
>> (let (form-description-42) (if (unwind-protect (setq value-40 (apply
>> (let ((value-40 (quote ert-form-evaluation-aborted-41))) (let (form-
>> (let* ((fn-38 (function string-match)) (args-39 (condition-case err
>> (save-current-buffer (set-buffer buf) (auto-revert--wait-for-revert
>> (while --dolist-tail-- (setq buf (car --dolist-tail--)) (save-curren
>> (let ((--dolist-tail-- (list buf1 buf2)) buf) (while --dolist-tail--
>> (progn (write-region "any text" nil tmpfile1 nil (quote no-message))
>> (unwind-protect (progn (write-region "any text" nil tmpfile1 nil (qu
>> (let* ((auto-revert--messages "") (g30 (function (lambda (msg) (setq
>> (unwind-protect (let* ((auto-revert--messages "") (g30 (function (la
>> (let* ((cp (executable-find "cp")) (tmpdir1 (make-temp-file "auto-re
>> (lambda nil (let* ((fn-23 (function executable-find)) (args-24 (cond
>> ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>> ert-run-test(#s(ert-test :name auto-revert-test01-auto-revert-severa
>> ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
>> ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
>> ert-run-tests-batch((not (tag :unstable)))
>> ert-run-tests-batch-and-exit((not (tag :unstable)))
>> eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
>> command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/autorevert-tests.el"
>> command-line()
>> normal-top-level()
>> Test auto-revert-test01-auto-revert-several-files condition:
>>  (ert-test-failed
>>   ((should
>>     (string-match "another text"
>>                   (buffer-string)))
>>    :form
>>    (string-match "another text" "any text")
>>    :value nil))
>> FAILED  2/5  auto-revert-test01-auto-revert-several-files
>> Reverting buffer `auto-revert-test49vRly'.
>> Reverting buffer `auto-revert-test49vRly'.
>> Reverting buffer `auto-revert-test49vRly'.
>> passed  3/5  auto-revert-test02-auto-revert-deleted-file
>> Reverting buffer `auto-revert-testvVIyEY'.
>> passed  4/5  auto-revert-test03-auto-revert-tail-mode
>> ls does not support --dired; see `dired-use-ls-dired' for more details.
>> Reverting buffer `T'.
>> Reverting buffer `T'.
>> passed  5/5  auto-revert-test04-auto-revert-mode-dired
>> 
>> Ran 5 tests, 4 results as expected, 1 unexpected (2019-05-01 10:07:23+0800)
>> 
>> 1 unexpected results:
>> FAILED  auto-revert-test01-auto-revert-several-files
>> 
>> make[1]: *** [lisp/autorevert-tests.log] Error 1
>> make: *** [lisp/autorevert-tests] Error 2
>> 
> 

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

[Michael Albinus]

Zhang Haijun <ccsmile2008@outlook.com> writes:

> If both file watching and directory watching are used for one file, it
> will works for all use cases.

This doesn't fit into filenotify.el. Which watch descriptor do you
expect from `file-notify-add-watch'?

Best regards, Michael.

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

[Zhang Haijun]

It seems that file watching of kqueue is useless for me. The directory watching satisfies almost all of my needs. So I will continue to use my modified version.

> 在 2019年5月2日，下午8:30，Michael Albinus <michael.albinus@gmx.de> 写道：
> 
> Zhang Haijun <ccsmile2008@outlook.com> writes:
> 
>> If both file watching and directory watching are used for one file, it
>> will works for all use cases.
> 
> This doesn't fit into filenotify.el. Which watch descriptor do you
> expect from `file-notify-add-watch'?
> 
> Best regards, Michael.

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

[Michael Albinus]

Zhang Haijun <ccsmile2008@outlook.com> writes:

> I haven’t run the test before.

autorevert-tests run successfuly on my FreeBSD 10.2 system, with
unmodified Emacs sources from master.

> It seems that directory watching
> doesn’t work for file content only change. But it works for many
> common use cases while file watching doesn’t work.

We apply directory watching wherever possible. kqueue has its
restrictions here.


Best regards, Michael.

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

[Michael Albinus]

Zhang Haijun <ccsmile2008@outlook.com> writes:

>> I don't understand the patch. Symbol kqueue11 does not exist, so do you mean
>>
>> +                  func dir
> Yes.
>
>>
>> And have you applied the tests in filenotify-tests.el? Do all of them pass?
>>
>> Best regards, Michael.
>
> $ make -C test autorevert-tests
>  ELC      lisp/autorevert-tests.elc
>  GEN      lisp/autorevert-tests.log
> Running 5 tests (2019-05-01 10:06:38+0800)
> Reverting buffer `auto-revert-testqe4qed'.
>   passed  1/5  auto-revert-test00-auto-revert-mode
> (Shell command succeeded with no output)
> Test auto-revert-test01-auto-revert-several-files backtrace:
>  signal(ert-test-failed (((should (string-match "another text" (buffe

So this shows you cannot change filenotify.el as proposed.

Best regards, Michael.