Background
While reviewing PR #326 (AVD skin enumeration), two independent rubber-duck reviews surfaced that the skin-dedup SortedSet was using StringComparer.OrdinalIgnoreCase unconditionally, which would silently collapse legitimately distinct skin directories (e.g. Pixel vs pixel) on case-sensitive Linux/macOS filesystems. That was fixed in #326 with an inline OS.IsWindows ? OrdinalIgnoreCase : Ordinal ternary.
A broader scan of the codebase found the same pattern (and the same latent bug) in several other places that compare filesystem names or paths, with no shared helper to reach for. Worth consolidating before more drift accumulates.
Proposed helpers
In src/Xamarin.Android.Tools.AndroidSdk/OS.cs:
public static readonly StringComparer FileSystemComparer =
IsWindows ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal;
public static readonly StringComparison FileSystemComparison =
IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
Caveat to document inline: this is a heuristic by OS, not a true per-volume filesystem-case-sensitivity query. macOS APFS is case-insensitive by default, and Windows ReFS can be case-sensitive. The helper matches the codebase's implicit existing assumption (Windows = case-insensitive, everything else = case-sensitive) and is strictly more correct than the current state, but isn't perfect. A future iteration could query the actual filesystem (e.g. via FILE_CASE_SENSITIVE_INFORMATION on Windows, pathconf(_PC_CASE_SENSITIVE) on macOS) if it ever matters.
Sites to migrate
| File |
Line(s) |
Note |
src/Xamarin.Android.Tools.AndroidSdk/FileUtil.cs |
193, 195 |
IsUnderDirectory — pre-existing bug on Linux: path.StartsWith(dir + Sep, OrdinalIgnoreCase) would say /foo/Bar/x is under /foo/bar, which is wrong. |
src/Xamarin.Android.Tools.AndroidSdk/AndroidVersions.cs |
40 |
dirs.Distinct(OrdinalIgnoreCase) on framework dir paths — same pre-existing bug on Linux. |
src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs |
184 |
Skin dedup (currently uses the inline ternary added in #326 — replace with the helper). |
src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs |
66, 99, 113 |
AVD name lookups — debatable. AVD .ini files live on disk so are filesystem-cased, but avdmanager itself treats AVD names case-insensitively in commands. Probably keep as OrdinalIgnoreCase (tool semantics, not filesystem semantics) and add a comment explaining why. |
Out of scope
- The many other
StringComparison.OrdinalIgnoreCase uses in the codebase that operate on non-filesystem strings (license IDs, AVD names in process output, "Name:" prefixes in avdmanager list output, ADB serials, etc.). Those are correctly case-insensitive regardless of platform.
- Adding a per-volume case-sensitivity probe — defer until there's a concrete need.
Acceptance
- New helpers added to
OS.cs with an XML doc summary explaining the heuristic and its limits.
- All four sites above migrated (or explicitly excluded with a comment).
- No new
OrdinalIgnoreCase introductions for filesystem-name comparisons elsewhere — call out in .github/copilot-instructions.md so future PRs reach for the helper.
- Build with
--no-incremental: 0 warnings, 0 errors. PublicAPI files updated for the new public surface (the helpers are public).
- A regression test for
FileUtil.IsUnderDirectory exercising the case-sensitive Linux behavior (the existing tests likely only cover Windows).
Origin
Spotted during second-pass rubber-duck review of #326. The inline ternary added there works for the AVD skin case but doesn't help the other sites and is itself a candidate for consolidation.
Background
While reviewing PR #326 (AVD skin enumeration), two independent rubber-duck reviews surfaced that the skin-dedup
SortedSetwas usingStringComparer.OrdinalIgnoreCaseunconditionally, which would silently collapse legitimately distinct skin directories (e.g.Pixelvspixel) on case-sensitive Linux/macOS filesystems. That was fixed in #326 with an inlineOS.IsWindows ? OrdinalIgnoreCase : Ordinalternary.A broader scan of the codebase found the same pattern (and the same latent bug) in several other places that compare filesystem names or paths, with no shared helper to reach for. Worth consolidating before more drift accumulates.
Proposed helpers
In
src/Xamarin.Android.Tools.AndroidSdk/OS.cs:Caveat to document inline: this is a heuristic by OS, not a true per-volume filesystem-case-sensitivity query. macOS APFS is case-insensitive by default, and Windows ReFS can be case-sensitive. The helper matches the codebase's implicit existing assumption (Windows = case-insensitive, everything else = case-sensitive) and is strictly more correct than the current state, but isn't perfect. A future iteration could query the actual filesystem (e.g. via
FILE_CASE_SENSITIVE_INFORMATIONon Windows,pathconf(_PC_CASE_SENSITIVE)on macOS) if it ever matters.Sites to migrate
src/Xamarin.Android.Tools.AndroidSdk/FileUtil.csIsUnderDirectory— pre-existing bug on Linux:path.StartsWith(dir + Sep, OrdinalIgnoreCase)would say/foo/Bar/xis under/foo/bar, which is wrong.src/Xamarin.Android.Tools.AndroidSdk/AndroidVersions.csdirs.Distinct(OrdinalIgnoreCase)on framework dir paths — same pre-existing bug on Linux.src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cssrc/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs.inifiles live on disk so are filesystem-cased, butavdmanageritself treats AVD names case-insensitively in commands. Probably keep asOrdinalIgnoreCase(tool semantics, not filesystem semantics) and add a comment explaining why.Out of scope
StringComparison.OrdinalIgnoreCaseuses in the codebase that operate on non-filesystem strings (license IDs, AVD names in process output, "Name:" prefixes inavdmanager listoutput, ADB serials, etc.). Those are correctly case-insensitive regardless of platform.Acceptance
OS.cswith an XML doc summary explaining the heuristic and its limits.OrdinalIgnoreCaseintroductions for filesystem-name comparisons elsewhere — call out in.github/copilot-instructions.mdso future PRs reach for the helper.--no-incremental: 0 warnings, 0 errors. PublicAPI files updated for the new public surface (the helpers are public).FileUtil.IsUnderDirectoryexercising the case-sensitive Linux behavior (the existing tests likely only cover Windows).Origin
Spotted during second-pass rubber-duck review of #326. The inline ternary added there works for the AVD skin case but doesn't help the other sites and is itself a candidate for consolidation.