search

hannobraun / inotify-rs

Idiomatic inotify wrapper for the Rust programming language
ISC License
254 stars 64 forks source link

Distinguish events from files with the same name #193

Closed cmosd closed 1 year ago

cmosd commented 2 years ago

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

cmosd commented 2 years ago

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

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 commented 2 years ago

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

Great :+1:

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 commented 2 years ago

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?

cmosd commented 1 year ago

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 (https://github.com/thin-edge/thin-edge.io/issues/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.