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

[Mattias Engdegård]

19 apr. 2019 kl. 16.56 skrev Michael Albinus <address@hidden>:
> 
>> (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.

Thanks, I'll do this separately.

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

What about using the checks in assertions to validate the assumptions, instead 
of making the code bug-tolerant? Assertions both inform the reader and check 
correctness.

After all, bug-tolerant code just leads to more bugs, to code where the 
just-in-case conditions cannot be distinguished from the essential ones, and 
where nobody understands the 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.
> 
> I'm still not convinced that we need REMOVE-DESCRIPTOR. We shall always
> remove the descriptor, and assure, that no superfluous events are raised.

Can I take it that you are happy with the patch attached to that message, which 
just does away with the maphash?

Regarding getting rid of remove-descriptor, that's fine with me -- I'm 
attaching a new patch which does the work in file-notify instead, which is how 
I interpret your wish. With that patch, e9e807e931 (addition of the 
remove-descriptor parameter) can be reverted.

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

Agreed, and filenotify seems to ensure that each watch gets a distinct 
descriptor even for the same file. I'll prepare a patch for simplifying the 
table in autorevert.el, unless you can remember the reason for introducing the 
buffer lists in ef3544f6a6. I have read bug#13540, which is mentioned in the 
commit message, and am none the wiser.

0001-Make-file-notify-rm-watch-robust-against-reentry.patch
Description: Binary data