Make matching case insensitive regardless of OS Fixes #5749 #5888
Conversation
Forgind
changed the title
Forgind
changed the title
There was a problem hiding this comment.
What exactly regressed #5749?
Should we consider leaving relevant operating systems case sensitive under a change wave?
| /// <returns></returns> | ||
| internal static bool ComparePathsNoThrow(string first, string second, string currentDirectory) | ||
| internal static bool ComparePathsNoThrow(string first, string second, string currentDirectory, bool alwaysIgnoreCase = false) |
There was a problem hiding this comment.
Nit: Not sure why it's 'always'IgnoreCase
| internal static bool ComparePathsNoThrow(string first, string second, string currentDirectory, bool alwaysIgnoreCase = false) | |
| internal static bool ComparePathsNoThrow(string first, string second, string currentDirectory, bool ignoreCase = false) |
There was a problem hiding this comment.
I prefer alwaysIgnoreCase because putting false here doesn't mean that you're case sensitive but that it depends on the OS. OS-dependent and always case-insensitive are the two I think of as common cases, so this distinguishes between them.
| @@ -42,7 +43,7 @@ public bool IsMatch(string fileToMatch) | |||
| return _regex.IsMatch(normalizedFileToMatch); | |||
There was a problem hiding this comment.
Shouldn't we account for ignoring case here as well?
There was a problem hiding this comment.
Absolutely. The beautiful (terrible?) part is that it already ignored case:
https://github.com/dotnet/msbuild/blob/master/src/Shared/FileMatcher.cs#L51
I can add a test for that, though.
|
https://github.com/dotnet/msbuild/pull/4997/files#diff-b62db5f7e7f90841af8ef58195eb4318cabc0742747ba3f2e97f10cfa21d6658L56-R80 was the change that brought it in. Totally reasonable, but it changed behavior from "todo: choose case-sensitivity based on OS" to actually doing that. |
Forgind
force-pushed
the
match-case-test
branch
from
520ffc8
to
fc63f60
Compare
|
@marcpopMSFT we'd like your thoughts on retroactively putting this behind a changewave. |
|
Putting it in a change waves means that once 17 releases, this would no longer be controllable. Is the customer open to changing by then and is that the ultimate behavior we want to drive towards? We could put a flag in just for this customer to opt out but that'll complicate our testing. I don't think we'd want to go back to the way we were before since it's already been two releases this way and going back could break other customers (though is admittedly less likely to break customers than switching to be case-sensitive) |
|
@marcpopMSFT, we haven't asked, and I don't think it's obvious that that's what we should drive towards. It's probably the best way, but it's hard to take advantage of that in our case, so I'd consider it only slightly beneficial and slightly more of a breaking change. For clarity, for going back to break someone, they would have had to:
|
|
@Forgind , I wasn't that worried about the impact of reversing this as you pointed out, it's more likely for folks to have been impacted by the original change than the revert. The question comes back to what is the right long term behavior? Do customers want it to be case sensitive or insensitive or OS dependent? Reverting solves the current problem. |
|
Let's just go with "updates are case insensitive, period".
|
|
In that case, this and #5853 are good to go, as far as I know. |
First commit: update a test such that it passes only if matching is case insensitive.
Second commit: modify FileUtilities.ComparePathsNoThrow such that it can be made to be always case sensitive or case insensitive. Note that this was changed about seven months ago in #4997 where it had previously been a todo to make case sensitivity depend on OS but that implemented it.
Third commit: fix a test that theoretically ensured case insensitivity but in practice did not, changed to care about case in the same PR.
The second commit fixes #5749.