Path.GetFullPath(string, string) uses case insensitive volume name comparison for drive rooted, relative paths #97759
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFixes #97615 (if the case the current behavior should be changed)
|
|
@dotnet-policy-service agree |
|
@AnakinRaW thank you for your contribution. The person who owns this code will be able to review the PR after 12th of March. I am sorry for the delay. |
...ibraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/IO/PathTests_Windows.cs
Outdated
Show resolved
Hide resolved
| @@ -89,7 +89,7 @@ public static string GetFullPath(string path, string basePath) | |||
| // Drive relative paths | |||
| Debug.Assert(length == 2 || !PathInternal.IsDirectorySeparator(path[2])); | |||
|
|
|||
| if (GetVolumeName(path.AsSpan()).EqualsOrdinal(GetVolumeName(basePath.AsSpan()))) | |||
| if (GetVolumeName(path.AsSpan()).EqualsOrdinalIgnoreCase(GetVolumeName(basePath.AsSpan()))) | |||
There was a problem hiding this comment.
There are other places in this file where EqualsOrdinal is used, can you change those as well?
runtime/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
Lines 349 to 353 in 7a60900
There was a problem hiding this comment.
sure. done that. However, I omitted EqualsOrdinal(@"\\".AsSpan() as there is nothing to compare case-insensitive. If it should be consistent though, i can chance again of course.
There was a problem hiding this comment.
Are you sure this is correct for \\SERVER\SHARE as I think this is a valid volume name on Windows.
In that case the server name should be case insensitive, but the share name might not (Think non windows OSs like Samba on linux).
Testcases on these paths are usually hard, as this call might try contacting the server...
There was a problem hiding this comment.
GetVolumeName is only used when path is drive relative like C:foo, it's used to check whether path and basePath have the same root, the method is probably more complex than it needs to but I don't think it would affect UNC paths.
There was a problem hiding this comment.
Actually that was my thought when applying the changes :)
Fixes #97615 (if the case the current behavior should be changed)