inotify: remove watch when renaming a watched path #518
Conversation
Renaming a watched path is problematic; on inotify we just get a IN_MOVED_SELF event with the old filename and that's it; no more events for you! So if you do: watch one mv one two cat asd >two You still continue to get events for the file "one", even though it's now named "two" (the file descriptor doesn't care about the rename). There is no way we can know the new event as far as I can tell, inotifywait(1) also behaves like this. So instead of continuing in a semi-broken state just remove the watcher, like we do for deletes. On kqueue and FEN the situation is similar, and we actually already removed watchers on renames. On Windows this all works nicely; the watch is preserved and the filename is updated. I decided to keep this as-is for now, even though it's inconsistent. We actually fixed the Windows behaviour for the 1.6.0 release in #370 , so people do seem to care about it and use it, and experience with the symlink change in 1.5.0 shows it's better to keep inconsistent behaviour rather than change it. Fixing this up is something for a v2. Fixes #172 Fixes #503
There was a problem hiding this comment.
One nit on a comment.
Between the tests passing and my review, I think we're good.
I do think I have a pretty good handle on the changes you made to the tests, but all the same I'd prefer if someone else with more inotify experience approved too.
That said, if folks are busy, I wouldn't wait an inordinate amount of time for a second review.
You're basically the only person regularly doing reviews at the moment, so ... 🙃 Even at $dayjob getting people to review things in a vaguely timely manner has often been a challenge. My general approach has been "just merge it if I'm fairly confident it's good". People can always comment later, or we can fix things later, or even backout something if need be. I tend to work on these kind of things in "bursts", e.g. a lot for a week and then nothing for a bit while I do other stuff. I don't really expect people to review >10 PRs in a week for a project like this. I think I will just stop asking them except for the larger changes like #521 that change the API; technical fixes like this one are easy to prove correct, but API changes and the like are a more subjective matter of taste, and it's good to have some people look at that, which is more important than stuff like this. That would reduce the requests, improving the chance people actually look at it. |
Renaming a watched path is problematic; on inotify we just get a IN_MOVED_SELF event with the old filename and that's it; no more events for you! So if you do:
You still continue to get events for the file "one", even though it's now named "two" (the file descriptor doesn't care about the rename). There is no way we can know the new event as far as I can tell, inotifywait(1) also behaves like this. So instead of continuing in a semi-broken state just remove the watcher, like we do for deletes.
On kqueue and FEN the situation is similar, and we actually already removed watchers on renames.
On Windows this all works nicely; the watch is preserved and the filename is updated. I decided to keep this as-is for now, even though it's inconsistent. We actually fixed the Windows behaviour for the 1.6.0 release in #370 , so people do seem to care about it and use it, and experience with the symlink change in 1.5.0 shows it's better to keep inconsistent behaviour rather than change it. Fixing this up is something for a v2.
Fixes #172
Fixes #503