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

[Mattias Engdegård]

16 apr. 2019 kl. 09.04 skrev Michael Albinus <address@hidden>:

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

(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'?)

I'm not necessarily enamoured with `file-notify-valid-p'. It just tells whether 
there is, right now, a descriptor that looks like the argument, in use by 
someone, somewhere. These descriptors are reused, making the predicate 
dangerous to rely on, or requires brittle code that just knows that no reuse 
has occurred.

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.

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.

(For that matter, the documentation doesn't say what mutation is permitted 
inside `maphash'. I can guess from the source.)

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.

0001-autorevert.el-auto-revert-notify-rm-watch-Simplify.patch
Description: Binary data