-
Notifications
You must be signed in to change notification settings - Fork 222
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
Support watching the same inode through multiple paths. #573
base: main
Are you sure you want to change the base?
Conversation
As a single change event can now refer to multiple paths, use "path groups" for renames to differentiate sources and destinations. Fixes notify-rs#572.
/// in distinct groups. | ||
/// | ||
/// If there are different path groups, the `path_group_split_index` attribute indicates the | ||
/// first index with paths in the second group. | ||
pub paths: Vec<PathBuf>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a behavioral change, we probably want some kind of API change so users explicitly decide how to handle multiple paths in one group, or this newly introduced concept of 2 different groups.
I didn't want to break this into two separate vectors, as to not increase the size of the Event
struct. Maybe we should make paths
private and have a fn path_groups(&self) -> (&[PathBuf], Option<&[PathBuf]>)
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about instead of having the api on the attrs interact directly with the fields, there's a more intuitive API on Event like renamed_from_paths() -> Option<&[Path]>
and renamed_to_paths() -> Option<&[Path]>
that interprets the kind/paths/attr information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a good way to only expose these APIs on renames only. Are you proposing to check the event kind and only return Some
in case of a rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was, yes.
Ping @passcod, @dfaust, @0xpr03, would any of you be able to review? The current implementation is buggy if the watched directory contains two hard or symbolic links to the same file. I can see an argument for symbolic links to not be followed at all, but something like this change is needed for hard links I think. |
After revisiting this PR I think the ignoring symlink while walking the directories is the better solution to my problem: #635. Some of the changes here might still be useful to handle hardlinks. |
Support watching the same inode through multiple paths.
As a single change event can now refer to multiple paths, use "path groups" for renames to differentiate sources and destinations.
Fixes #572.