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 rooted path without volume label regression on Windows #96

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

Metapyziks
Copy link
Contributor

On Windows, PhysicalFileSystem.ConvertPathFromInternalImpl requires a volume label after resolving an absolute path. My fix for #91 meant that rooted paths without a volume label, like /example, wouldn't be resolved to include such a label.

This PR fixes that by explicitly checking for a volume label before deciding to use Path.GetFullPath, and adds a test demonstrating the fix.

Reported here: #92 (comment)

@ptasev
Copy link

ptasev commented Jul 25, 2024

Thanks for fixing this. I was just looking at APIs available in .NET. Perhaps what we want is Path.IsPathFullyQualified(). There's a link to the source to see how it works and also example code.
https://learn.microsoft.com/en-us/dotnet/api/system.io.path.ispathfullyqualified?view=net-8.0

I also found this helpful:
https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#traditional-dos-paths

There's also an ability to skip normalization with GetFullPath. It'll have to be tested to see if it can work for your ~ paths:
https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#skip-normalization

Not sure if any of these are preferable to your implementation. Just throwing ideas out there.

@Metapyziks
Copy link
Contributor Author

Thanks for fixing this. I was just looking at APIs available in .NET. Perhaps what we want is Path.IsPathFullyQualified(). There's a link to the source to see how it works and also example code. https://learn.microsoft.com/en-us/dotnet/api/system.io.path.ispathfullyqualified?view=net-8.0

That looks cleaner, I'll give it a try tomorrow. Thanks!

I also found this helpful: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#traditional-dos-paths

There's also an ability to skip normalization with GetFullPath. It'll have to be tested to see if it can work for your ~ paths: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#skip-normalization

Not sure if any of these are preferable to your implementation. Just throwing ideas out there.

That was an option I thought about for the original fix, prefixing with \\?\, then stripping it after, but I wanted to avoid the allocation if possible. Maybe it's the way to go to avoid other edge cases though.

@Metapyziks
Copy link
Contributor Author

Metapyziks commented Jul 27, 2024

Ah it's a little less clean because Path.IsPathFullyQualified() isn't supported in .NET 4.6.2. Internally it's doing a very similar check to us anyway:

https://github.com/dotnet/runtime/blob/67d5c92a2f5ac9a94d73861035141db5466e7004/src/libraries/Common/src/System/IO/PathInternal.Windows.cs#L266-L273

What do you think @xoofx, do an #if NET7_0_OR_GREATER around Path.IsPathFullyQualified or just leave it as it is?

@xoofx
Copy link
Owner

xoofx commented Jul 27, 2024

What do you think @xoofx, do an #if NET7_0_OR_GREATER around Path.IsPathFullyQualified or just leave it as it is?

Yes, it's fine. I will drop support for NET Framework at some point. I don't use it, and I don't want to continue maintaining it. That was ok a few years ago when I started this project, but now...

@xoofx
Copy link
Owner

xoofx commented Jul 30, 2024

Is the PR good to go as it is?

Also explain why we're not just always calling Path.GetFullPath

xoofx#92
@Metapyziks
Copy link
Contributor Author

Is the PR good to go as it is?

I've cleaned up the history a bit, it's ready if you're happy.

@xoofx xoofx added the bug label Aug 1, 2024
@xoofx xoofx merged commit ba7197e into xoofx:main Aug 1, 2024
2 checks passed
@xoofx
Copy link
Owner

xoofx commented Aug 1, 2024

Thank you!

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.

3 participants