bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches

[Andreas Politz <politza@hochschule-trier.de>]

Michael Albinus <michael.albinus@gmx.de> writes:

> Andreas Politz <politza@hochschule-trier.de> writes:

>> 1. Change inotify.c and make it return/receive a unique descriptor per client.
>
> I agree with you, that's the best choice.
>

Ok.

>> Here is a first draft of a
>> corresponding patch, let me know what you think.
>
> I've applied the patch, and filenotify-tests.el passes all tests
> (except `file-notify-test04-autorevert-remote', but that's another
> story). So I believe it is OK to apply it to master, and see how it goes
> (waiting for feedback).

Let me work on this a little more. I think, I'm not removing the
descriptors in inotify.c correctly.

>
> Some comments:
>
>> diff --git a/lisp/filenotify.el b/lisp/filenotify.el

>> -(defun file-notify--descriptor (desc file)
>> +(defun file-notify--descriptor (desc _file)

> In this case, we shall remove `file-notify--descriptor', and replace all
> calls by the `desc' argument.

Yes, and since (with the patch added) we now have a one-to-one relation
between clients and descriptors across all implementations, we could
also simplify the hash values.


>> @@ -408,9 +400,8 @@ file-notify-add-watch
>>  (defun file-notify-rm-watch (descriptor)
>>    "Remove an existing watch specified by its DESCRIPTOR.
>>  DESCRIPTOR should be an object returned by `file-notify-add-watch'."
>> -  (let* ((desc (if (consp descriptor) (car descriptor) descriptor))
>> -	 (file (if (consp descriptor) (cdr descriptor)))
>> -         (registered (gethash desc file-notify-descriptors))
>> +  (let* ((file nil)
>> +         (registered (gethash descriptor file-notify-descriptors))
>
> I'm not sure we can eliminate the `file' binding. I believe, it is
> needed for the kqueue library. Maybe you add a TODO comment for
> retesting instead.

Shouldn't be, since kqueue, w32notify and gfilenotify all return a
pointer wrapped in a Lisp-Integer, i.e. for these back-ends the file
value was already nil all the time.

> (My virtual machine running BSD is in a bad shape. I should reanimate
> it.)

We should have a server, doing this sort of thing.

>> diff --git a/src/inotify.c b/src/inotify.c
>> index 61ef615328..302f52225e 100644
>> --- a/src/inotify.c
>> +++ b/src/inotify.c
>> +#ifdef DEBUG
>
> Please use a more specific flag, like INOTIFY_DEBUG.

Will do.

>> (ert-deftest file-notify-test03c-events () [...]


> I'm a little bit undecided, whether we shall add this as extra test
> case, or whether we shall integrate it into
> `file-notify-test03-events'. The former might be better, but it would
> also mean that we shall break down `file-notify-test03-events' into
> smaller tests.

I think it would be better to split those tests into smaller units.  For
once it makes it easier to determine which should-form actually failed.
And secondly, it makes it easier to add a new test (especially for
people not to familiar with the code), without being anxious about
interfering with existing ones.

>> * inotify_add_watch internally uses a single watch per directory, which
>>   may be shared by multiple clients (using filenotify.el).  The problem
>>   here seems to be that these clients may use different FLAGS as
>>   argument to file-notify-add-watch.  Currently, the last added client's
>>   FLAGS become the effective mask for the underlying C-descriptor,
>>   meaning that clients added before may not receive change or
>>   attribute-change events if the mask was modified accordingly.
>
> I'm aware of this problem (it happens also for other libraries, I
> believe). No idea yet whether it is important to fix it. But maybe you
> add a TODO entry at the end of filenotify.el.

I think, it is important.  For example, auto-revert's file-notify
mechanism (using '(change attribute-change) as flags) would break, if
some other package decides to watch the same file, but for
attribute-changes only.

It seems to me that this only affects inotify, since all other back-ends
return a newly created descriptor, but I haven't explicitly checked
this.

>
>> * It seems to me that the right word here is "unified".

>>  Since all these libraries emit different events on notified file
>>  changes, there is the Emacs library @code{filenotify} which provides a
>> -unique interface.
>> +unified interface.

> My English is not good enough to decide what's better. But I don't
> object if you want to change.

I would translate it in this context as "einzigartig"
vs. "vereinheitlicht".  Native speakers to the rescue !

-ap