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

[Michael Albinus <michael.albinus@gmx.de>]

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.