Skip to content
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

Merged
merged 7 commits into from
Jul 11, 2016
Merged

Normalize the recursive watch behavior of the various back-ends #88

merged 7 commits into from
Jul 11, 2016

Conversation

dfaust
Copy link
Collaborator

@dfaust dfaust commented Jul 9, 2016

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

@dfaust dfaust changed the title Watch recursive Normalize the recursive watch behavior of the various back-ends Jul 9, 2016
@passcod
Copy link
Member

passcod commented Jul 11, 2016

Very very cool! Really awesome work

@passcod
Copy link
Member

passcod commented Jul 11, 2016

This all looks good. Again, many thanks and great work on this.

@passcod
Copy link
Member

passcod commented Jul 11, 2016

Do you have any more awesomeness you're working on before I merge all this and cut 3.0.0?

@dfaust
Copy link
Collaborator Author

dfaust commented Jul 11, 2016

Glad you like it. There still are some issues I would like to fix, so don't release 3.0.0 just yet.
I would suggest you merge this PR now, but what really matters to me is that master doesn't change, because I would like to avoid merge conflicts. You can expect new PRs soon.

@passcod
Copy link
Member

passcod commented Jul 11, 2016

In that case, can you merge or rebase your other PR on top of this, so I
can merge both now?

On Mon, 11 Jul 2016, 18:46 Daniel Faust notifications@github.com wrote:

Glad you like it. There still are some issues I would like to fix, so
don't release 3.0.0 just yet.
I would suggest you merge this PR now, but what really matters to me is
that master doesn't change, because I would like to avoid merge conflicts.
You can expect new PRs soon.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#88 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAJgi8kltLqGppvZmpc7ypfx1v7Cgn9Rks5qUea1gaJpZM4JIppF
.

@passcod passcod merged commit b072f7f into notify-rs:master Jul 11, 2016
@passcod
Copy link
Member

passcod commented Jul 11, 2016

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

@dfaust
Copy link
Collaborator Author

dfaust commented Jul 11, 2016

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.

@dfaust dfaust deleted the watch_recursive branch July 11, 2016 10:26
@passcod
Copy link
Member

passcod commented Jul 11, 2016

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:

  • This is git. I have a copy of the entire history locally. Giving you commit rights is no big risk to me, as I can remove it at any time and revert anything back to how it was. So I'm not worrying there. (The worst thing you could possibly do is deface the repo in such a way that it brings negative publicity, I guess? But even then, again, easy to undo, and publicity is publicity! ;P)
  • I can comment on commits, so I can discuss changes in post rather than in pull requests, if needed… and in the meantime you can go forward. I'm not above history rewrites if big chunks need to be reverted, although I don't expect that to happen.
  • Obviously you can't change the project metadata (name, license, etc), and you can't replace it all by a library that sings the 99 bottle song in albanian morse code. But apart from that… more usefully, Additional maintainers + Vision #24 has a big picture view of what my goals have been with Notify from the start. The discussion attached to it is a bit dated, but the bullet points are all still current.
  • I think at this point changing the entire public API around would be detrimental, unless there's a very good reason. So, if you think that's something that should happen, that's a thing to discuss beforehand.
  • You have, with your PRs, essentially refactored most of the library. In fact, the first thing you can do with commit rights is add yourself to the author list in the Cargo.toml. I mean, I'm still project owner, and there's other people who have done a lot of work, and I'll still be maintaining and managing this library in a year, two years… even after you don't actually come around anymore, if that happens. But this is a young project and you've done a lot, albeit in a short time; you are therefore actually a part of this. For how long, who knows; and it matters not.
  • Ah, something I hadn't thought of: please don't insert backdoors in the code, mmkay? Not that you would respect that if you were going to, but yeah, please don't. Easter eggs are fine; if that can be managed sensibly in such a library, that would be an achievement in itself.
  • If you want commit rights to be revoked at any point: well, you can do it yourself anyway, but please do tell when you do it, just so I know.
  • Obviously if you're not sure about something you want to change or want a second pair of eyes, ask.

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.

@dfaust
Copy link
Collaborator Author

dfaust commented Jul 11, 2016

Ok, I think I got the gist :) ... no backdoors then ...
Thanks for the invite and fyi I'm male.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants