-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize the recursive watch behavior of the various back-ends #88
Conversation
Also implement RecursiveMode for watch(..).
10 ms seems a bit short especially since the mtime resolution is just 1 second.
Very very cool! Really awesome work |
This all looks good. Again, many thanks and great work on this. |
Do you have any more awesomeness you're working on before I merge all this and cut 3.0.0? |
Glad you like it. There still are some issues I would like to fix, so don't release 3.0.0 just yet. |
In that case, can you merge or rebase your other PR on top of this, so I On Mon, 11 Jul 2016, 18:46 Daniel Faust notifications@github.com wrote:
|
Actually, do you want commit rights on the repo for this? I can remove you afterwards if you don't want the long term, but that should make it easier for this code effort :) |
Well, that's up to you. I certainly wouldn't mind having commit rights. But you should make it clear what kind of changes I should push directly and which you want to review first. My next plans are to better detect renames by adding a cookie (like in inotify) to the events. And then make sure that all events reported are the same for at least Linux and Windows. Doing that for OS X is somewhat difficult. And I want to clean up the tests some more. |
My idea was that given what you've been doing so far has been excellent, I tentatively trust you to commit good things. Several things, in no particular order:
Unrelated, may I ask your preferred pronouns/gender so I use the right ones when I talk about you or your work here? The anime avatar is rather opaque to guessing :) And finally, and on topic: during week days nowadays I have access to a real OS X box, so if you need something tested do ask. Sorry, that's not really "make it clear what kind of changes I should push directly and which you want to review first." Summary: given the scope of this very PR, if I put a limit on what you can commit directly, it would defeat entirely the point of giving you commit rights. Hence, thus, and therefore, flourish here you go. |
Ok, I think I got the gist :) ... no backdoors then ... |
This patch normalizes the recursive behavior of the various back-ends when adding watches.
Currently inotify initially watches directories recursively, but doesn't watch new directories and cannot unwatch directories recursively.
Windows and FSEvent watch directories recursively and automatically watch new directories.
With this patch, all back-ends can watch directories recursively (while automatically watching new directories) or non-recursively depending on a new flag for the watch(..) function.
It also fixes a memory leak in the inotify back-end.
I implemented CREATE and DELETE for PollWatcher in order to make it compatible with the new tests.
And I increased the PollWatcher's default delay to 10 seconds. I guess that the default value of 10 ms was a mistake, since that seems a bit low. Plus the PollWatcher has only a time resolution of 1 second anyway.
I also moved some code from tests/notify.rs. to the new files tests/watcher.rs and tests/utils.rs in order to reduce its size.
Here are some issues, this patch might affect:
Fixes: #60
Fixes: #61
Implements some features from: #20
Fixes: #68