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

[Michael Albinus]

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

> Anyway, here is the more progressive version of the patch, adding both
> of the above points.  (I guess, I'm to conservative sometimes and/or
> seeing only problems everywhere.)

Thanks!

>> 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.
>
> I implemented it by always using constantly watching for all events
> (IN_ALL_EVENTS) and storing the given user-flags with the callback etc.
> When an event occurs, I check whether it matches the given mask.

Sounds good.

> For that to work, I had to restrict the flag-usage by the user to those
> not having an effect on the shared descriptor.

What does this mean in practice? Any restriction we need to document?

> I also added IN_EXCL_UNLINK as a default.  This avoids reporting events
> for already deleted filenames, which are still opened by some process,
> which seems what we want as a default.  

OK.

> I have no push privileges.

I'm willing to push the patch in your name, if you provide me a ChangeLog
style commit message. For the future, I recommend to obtain push privileges.

Some nitpicks:

> --- a/lisp/filenotify.el
> +++ b/lisp/filenotify.el
> +(defun file-notify--watch-absolute-filename (watch)

This deserves a docstring.

> +handler.  The value in the hash table is file-notify--watch
> +struct.")

Please quote `file-notify--watch'.

>  (defun file-notify--rm-descriptor (descriptor)
> +DESCRIPTOR should be an object returned by
> +`file-notify-add-watch'.  If it is registered in
> +`file-notify-descriptors', a stopped event is sent."

Don't reformat the docstring, keep the first line as complete sentence.

> -      (dolist (action actions)

> +      (while actions
> +        (let ((action (pop actions)))

Being curious: why did you change this?

> --- a/src/inotify.c
> +++ b/src/inotify.c
> @@ -264,10 +360,6 @@ close
>  The following symbols can also be added to a list of aspects:
>  
>  dont-follow
> -excl-unlink
> -mask-add
> -oneshot
> -onlydir

Maybe we shall say explicitely, that those inotify events are not supported.

> -COOKIE is an object that can be compared using `equal' to identify two 
> matching
> +COOKIE is an object that can be compared using `equal' to identify two 
> matchingt

Typo.

> -ap

Best regards, Michael.