Re: master e9e807e: Don't remove notify descriptor that is already gone

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

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

Hi Mattias,

>> I haven't tested thoroughly yet, but wouldn't it suffice if in
>> auto-revert-notify-rm-watch there is just the test
>> 
>> (when (file-notify-valid-p auto-revert-notify-watch-descriptor)
>> 
>> instead of
>> 
>> (when auto-revert-notify-watch-descriptor
>
> Thanks for reading my change. It is a fair question!
>
> First of all, the descriptor wouldn't then be removed from
> `auto-revert-notify-watch-descriptor-hash-list' since that part is
> also guarded by the condition, but that's just a matter of rearranging
> code.

Yes.

> (Not only is `auto-revert-notify-watch-descriptor-hash-list' a
> mouthful, it is a bit misleading. It maps descriptors to lists of
> buffers. How about `auto-revert--buffers-by-watch-descriptor'?)

No objection.

> Slightly more robust would be to stop reusing descriptors: either made
> mutable, so that they can be invalidated, or made unique by using a
> counter. However, the basic design is still somewhat dubious: it tells
> us whether the descriptor is valid, but that just raises the question:
> why do we even have to ask? Correct code should understand its own
> invariants.

In theory you are right. But I fear there could be situations where such
assumptions do ne keep. A double-check is OK.

> Now that you `mentioned auto-revert-notify-rm-watch', does it strike
> you as odd the way it does
>
>     (maphash
>      (lambda (key value)
>        (when (equal key some-key)
>          do-something))
>      some-hashtable)
>
> instead of using the hash table directly? Suggested patch to fix this
> attached.

I'm still not convinced that we need REMOVE-DESCRIPTOR. We shall always
remove the descriptor, and assure, that no superfluous events are raised.

> By the way, why don't we give each buffer in auto-revert-mode a unique
> descriptor, so that the table just maps descriptors to buffers,
> instead of to lists of buffers? It would simplify the code in many
> places, and it cannot be that common to have multiple buffers for the
> same file that it warrants the descriptor-sharing optimisation.

Well, the descriptor is the one we get from filenotify. I don't believe
we shall do double housekeeping. Sounds to me error-prone.

Best regards, Michael.