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

[Mattias Engdegård]

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.

0001-Don-t-poll-auto-revert-files-that-use-notification.patch
Description: Binary data