Fix issue in editor config filepath comparison #73033
Conversation
This code previously just did a simple string.StartsWith to check if a file was in a folder. However, this would incorrectly return true for a case such as testing if c:\foo\test.txt was in folder c:\foo\test.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| @@ -325,6 +325,26 @@ static void freeKey(List<Section> sectionKey, ObjectPool<List<Section>> pool) | |||
| sectionKey.Clear(); | |||
| pool.Free(sectionKey); | |||
| } | |||
|
|
|||
| static bool isNormalizedPathInDirectory(string path, string directory) | |||
There was a problem hiding this comment.
Should we use helper PathUtilities.IsSameDirectoryOrChildOf instead?
There was a problem hiding this comment.
I actually didn't know that method existed. I can go either way, a couple thoughts:
- The change I made is simple, but the other method is probably fairly well tested
- PathUtilities.IsSameDirectoryOrChildOf appears to do quite a bit of allocation
- PathUtilities.IsSameDirectoryOrChildOf does an OrdinalIgnoreCase comparison whereas this code does an Ordinal comparison
There was a problem hiding this comment.
I would push toward use of the existing helpers, if the task we need to perform appears to be the same. I feel these kinds of helpers are not easy to get right. I'm def open to review any optimization of existing helpers. I would prefer to avoid having multiple helpers for doing almost the same thing with paths across the codebase with subtle differences which we aren't sure are correct.
There was a problem hiding this comment.
I agree with Rikki on this. FilePath stuff is tricky (i just broke it myself in the IDE). consolidating on helpers is the way to go.
There was a problem hiding this comment.
I've gone ahead and used IsSameDirectoryOrChildOf per the suggestion above and modified it to take in casing behavior. I also saw IsChildPath, which looked significantly more performant, but used IsUnixLikePlatform to determine case sensitivity.
There was a problem hiding this comment.
Note: orthogonal to your PR, case sensitivity cannot be determined by asking what platform you're on. It's dependent on the file system, which could be mounted into arbitrary hosts at a per path-segment level. For example /A/B/C/D might transition through 4 different file systems, on different hosts, with differing case-sensitivity behavior.
We likely have bugs here, which will hit users doing things like mounting win-directories into wsl, or vice versa.
|
It looks like test |
…ut other tests do it, so modified the test so it should be happier in that context
Made unix/windows specific versions of the test. PTAL at your convenience. Thanks! |
This code previously just did a simple string.StartsWith to check if a file was in a folder. However, this would incorrectly return true for a case such as testing if c:\foo\test.txt was in folder c:\foo\test.