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:

> Hey !

Hi Andreas,

> Below is a second version of the previous patch.  It is somewhat
> conservative, since neither did I attempt to
>
> + further simplify filenotify.el nor
>
> + handle differing masks in inotify.c .

Thanks for this. Next time you provide a patch, could you pls merge
emacs recent changes from master first? Your patch was rejected partly,
and I had to apply rejected hunks manually. Comments:

> diff --git a/lisp/filenotify.el b/lisp/filenotify.el
> index 7eb6229976..5dea67b580 100644
> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> @@ -25,6 +25,20 @@
>  ;; file notification packages `inotify', `kqueue', `gfilenotify' and
>  ;; `w32notify'.
>
> +;; TODO:

Pls move TODOs at the end of the file.

> +;; * "inotify_add_watch adds a new watch, or modifies an existing watch"
> +;;   We need to make sure that different watches for the same directory
> +;;   don't set the mask in a conflicting way regarding changed/attribute-changed
> +;; * Also check which other inotify flags are problematic
> +;;   for concurrent use of the underlying descriptor

Well, I had always the hope to modify inotify watches in this case. If
there is a watch with flags f1, and a new watch for the same file is
requested with flags f2, and f2 contains a flag which is not part of f1,
then either the existing watch shall be adapted, or the existing watch
shall be removed, and a new shall be installed. Don't know what's
possible in inotify.

> @@ -48,16 +62,14 @@ file-notify-descriptors
>
>    (DIR (FILE . CALLBACK) (FILE . CALLBACK) ...)
>
> -Several values for a given DIR happen only for `inotify', when
> -different files from the same directory are watched.")
> +Several values for a given DIR should currently not occur.")

Remove "currently". Every docstring speaks about current state.

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

>  (defun file-notify-valid-p (descriptor)
>    "Check a 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)

In both functions I believe we don't need to bind `file'. The code could
be simplified, because (or (not file) ...) always succeeds.

Your changes look good; "make -C test filenotify-tests
SELECTOR='$(SELECTOR_DEFAULT)'" passes all tests. Even if there is room
for improvement I believe you could push your patch to master now, in
order to get feedback from other developers.

> I also thought about the test-cases and more generally about how to
> develop a specification for this library, i.e. how do we want this to
> behave.  Do we have the desire that it works uniformly across all
> participating back-ends ? And is that even possible ?

As Eli said, that's the intention. But we cannot reach this goal
completely due to the different behaviour of the libraries.

> I think it is to easy to adapt the tests for each back-end, until they
> succeed and thereby potentially masking actual bugs.

That's what file-notify-test.el intends to do. Well, the code has
evolved over the time, and it is somehow hard to read.  Improvements are
welcome!

> One way to go about this would be to write a series of definitive
> unit-tests which specify the intended behavior. Then, allow them to fail
> for a specific back-end, until someone has fixed potential bugs for it
> and confirmed that the test succeeds.  This would allow for an
> incremental improvement on fairly solid grounds.  I'm assuming that
> people of the future are interested in improving their used back-end
> (e.g. make kqueue watch directories properly, if that is possible).

Could you show an example how this shall look like?

> Anyway, I was bored today, so I took a look at what events these
> libraries actually produce, the result of which you may also find below.

Thanks; I'll review it next days.

> Finally, I'm tempted to suggest to get rid of the flags argument of
> file-notify-add-watch.  As it is, things are already complicated enough
> and we don't seem to have many people working on this.  I think we could
> make it backward-compatible to a certain degree.  Note also, that many
> file operations trigger both kinds of events anyway.

I agree with you. I haven't seen any different use of the flags yet (but
I maybe wrong).

Best regards, Michael.