[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches
From: |
Michael Albinus |
Subject: |
bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches |
Date: |
Sat, 25 Mar 2017 15:04:03 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) |
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.
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, (continued)
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/22
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/24
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Eli Zaretskii, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Eli Zaretskii, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches,
Michael Albinus <=
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/25
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/18
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Michael Albinus, 2017/03/18
- bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Andreas Politz, 2017/03/18
bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches, Paul Eggert, 2017/03/30