xoofx / zio

A cross-platform abstract/virtual filesystem framework with many built-ins filesystems for .NET
BSD 2-Clause "Simplified" License
817 stars 61 forks source link

Fix rooted path without volume label regression on Windows #96

Closed Metapyziks closed 1 month ago

Metapyziks commented 1 month ago

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: https://github.com/xoofx/zio/pull/92#issuecomment-2239530728

ptasev commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Is the PR good to go as it is?

Metapyziks commented 1 month ago

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 commented 1 month ago

Thank you!