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

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

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

Hi Andreas,

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

OK. Take your time.

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

We shall recheck, once your changed inotify implementation has hit the repo.

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

Likely yes. I have the same feeling, but haven't done due to lack of
energy and time.

For the time being, I have added a modified version of your test
removing watch descriptors out of order to
`file-notify-test02-rm-watch'. Since this fails for inotify, I've added
an :expected-result tag to this test. Can be removed, when fixed in
inotify.c.

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

So I would let it for you to implement it in inotify.c. When there is
also a respective test, we will see whether it is a problem for other
backends, too.

> -ap

Best regards, Michael.