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

Distinguish events from files with the same name #193

Conversation

6367f766
Copy link

Describe the issue

Hi!

While using this API I found myself unable to distinguish between events that arrive from files with the same name yet in different paths. The problem can be formalised as below.


Given two file watchers with the same file name but in different paths:

file watch 1: /tmp/file_a
file watch 2: /tmp/another_dir/file_a

And randomly triggering an event on one of the files. For which file was the event triggered?


Proposed changes

This PR proposes a getter method for the WatchDescriptor, allowing one the ability to store the id for a given file watch after an .add_watch() and using this to compare the Event struct coming from an event stream.


As a side note, I am still learning Rust so if I used something where perhaps something else was better let me know and I am happy to make the changes! Also apologies for the auto-formatting on the files, I can revert them. Let me know! Thanks

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pull request, @initard! This looks reasonable.

Unfortunately the CI build fails, due to #194 (which I've just opened). This would need to be addressed (preferably in a separate pull request) before this one can be merged.

Are you in a hurry to get this merged? We could wait for a while, to get feedback on #194, or we could do the conservative MSRV upgrade right now, to get things going again.

Also apologies for the auto-formatting on the files, I can revert them. Let me know!

This is fine. In fact, it would be better to auto-format the whole code base and enforce the formatting on CI. But please put formatting changes in separate commits in the future. Having them mixed in with actual changes makes those changes needlessly hard to review.

@hannobraun hannobraun added the status: blocked The issue is currently blocked by something outside this project. label Jul 22, 2022
@6367f766 6367f766 force-pushed the improvement/inotify-two-files-with-the-same-name branch from 627e2bb to ba0f0c6 Compare July 22, 2022 09:54
@6367f766
Copy link
Author

Are you in a hurry to get this merged? We could wait for a while, to get feedback on #194, or we could do the conservative MSRV upgrade right now, to get things going again.

Thanks for reviewing so quickly. Not in a hurry, this can wait 😄

This is fine. In fact, it would be better to auto-format the whole code base and enforce the formatting on CI. But please put formatting changes in separate commits in the future. Having them mixed in with actual changes makes those changes needlessly hard to review.

I went ahead and turned off auto formatting and removed the linting stuff. It makes the PR clearer. Sorry again! I can make a separate PR for the linting stuff! Happy to help where I can.

@hannobraun
Copy link
Owner

Thanks for reviewing so quickly. Not in a hurry, this can wait 😄

Great 👍

If no feedback on the issue comes for a while, feel free to just submit a PR with an MSRV upgrade to the latest version.

I went ahead and turned off auto formatting and removed the linting stuff. It makes the PR clearer. Sorry again! I can make a separate PR for the linting stuff! Happy to help where I can.

No problem, it wasn't a big deal! If you're willing to sort out the formatting, that would definitely be welcome! It would prevent issues like this in the future.

What basically needs to happen, is to run cargo fmt (which should auto-format the whole codebase), commit that, then add a step to the CI configuration that runs cargo fmt --check (that's from memory; could be slightly different).

@hannobraun hannobraun force-pushed the improvement/inotify-two-files-with-the-same-name branch from ba0f0c6 to 8cb5ebd Compare August 12, 2022 09:34
@hannobraun
Copy link
Owner

Hey @initard! The MSRV was raised in #196. I've rebased this pull request and re-run the CI build, but it still fails, although for a different reason. Looks like some test code fails to compile. Could you have a look?

@hannobraun hannobraun removed the status: blocked The issue is currently blocked by something outside this project. label Aug 12, 2022
@6367f766 6367f766 force-pushed the improvement/inotify-two-files-with-the-same-name branch from 8cb5ebd to 3827026 Compare September 20, 2022 10:24
initard added 2 commits September 20, 2022 11:36
- add `get_watch_descriptor_id` method to `WatchDescriptor` enabling
inotify events to be distinguished (on id) by the crate user

Signed-off-by: initard <alex.solomes@softwareag.com>
- add test to check that two files with the same name
yet coming from different paths can be distinguished.

Signed-off-by: initard <alex.solomes@softwareag.com>
@6367f766 6367f766 force-pushed the improvement/inotify-two-files-with-the-same-name branch from 3827026 to 760a156 Compare September 20, 2022 10:36
@6367f766 6367f766 changed the title Distinguish between events from files with the same name Distinguish events from files with the same name Sep 20, 2022
@6367f766
Copy link
Author

6367f766 commented Sep 20, 2022

Hi @hannobraun! Apologies for the wait. I updated the tests — it was an issue with the feature flags for the "stream" feature.

Can you please try again?

As you can see from the linked issue above (thin-edge/thin-edge.io#1453), our team ran into this exact issue recently and it would be nice to have this merged.

PS. I noticed there was also an API change (.add_watcher and .event_stream were changed), so I hope this PR is still relevant.

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @initard! Everything works great now.

I noticed there was also an API change (.add_watcher and .event_stream were changed), so I hope this PR is still relevant.

I believe those changes are orthogonal, so this PR should be just as relevant as it was before.

@hannobraun hannobraun merged commit 9ec50c5 into hannobraun:master Sep 20, 2022
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.

2 participants