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

Fix SubFileSystem watcher (sometimes) silently discarding events #92

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

Metapyziks
Copy link
Contributor

@Metapyziks Metapyziks commented Jun 21, 2024

Fixes #91 by only calling Path.GetFullPath for non-rooted paths on Windows in PhysicalFileSystem.ConvertPathFromInternalImpl.

Paths from filesystem events are already rooted, so this skips the weird behaviour for paths containing a ~.

Watcher events get silently discarded if all conditions are met:
* SubFileSystem was created with incorrect case
* Changed file path includes a ~
* Any segment of changed file path is short enough to possibly be a SFN
Fixes TestSubFileSystem.TestWatcherCaseSensitive
Metapyziks added a commit to Facepunch/zio that referenced this pull request Jun 21, 2024
@xoofx xoofx merged commit a60abe4 into xoofx:main Jun 21, 2024
2 checks passed
@xoofx
Copy link
Owner

xoofx commented Jun 21, 2024

That's great, thanks for spotting this and making a PR to fix it, appreciated!

@xoofx xoofx added the bug label Jun 21, 2024
@Metapyziks Metapyziks deleted the fix/sub-watcher-case-sensitive branch June 21, 2024 15:08
@gmkado
Copy link

gmkado commented Jul 19, 2024

@xoofx this is a breaking change, should the package get a major version bump?

var fs = new PhysicalFileSystem();
fs.ConvertPathFromInternal(@"/test")

In 0.18.0 this results in to /mnt/c/test on windows but now throws an exception Expecting a drive for the path "\test"

@xoofx
Copy link
Owner

xoofx commented Jul 19, 2024

@xoofx this is a breaking change, should the package get a major version bump?

It is using semver 2.0, so 0.x.y is considered as unstable, so the API has never been stabilized, so technically, any 0.x.y can be a major version bump 🙂

That being said, if you can find a fix to workaround this, PR welcome.

@Metapyziks, would adding a check for the specific case of ~ would be enough? (I'm not sure what ~ is causing trouble with)

@Metapyziks
Copy link
Contributor Author

Sorry for the delay, I'll have a look now.

Metapyziks added a commit to Facepunch/zio that referenced this pull request Jul 25, 2024
Metapyziks added a commit to Facepunch/zio that referenced this pull request Jul 25, 2024
Metapyziks added a commit to Facepunch/zio that referenced this pull request Jul 30, 2024
Also explain why we're not just always calling Path.GetFullPath

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

Successfully merging this pull request may close these issues.

SubFileSystem watcher (sometimes) silently discards events
3 participants