Workaround libgit2 safe.directory case mismatch bug#1899
Workaround libgit2 safe.directory case mismatch bug#1899tyrielv wants to merge 2 commits intomicrosoft:masterfrom
Conversation
tyrielv
force-pushed
the
tyrielv/safedirectory-caseinsensitive
branch
from
9f64938 to
ba58df0
Compare
tyrielv
force-pushed
the
tyrielv/safedirectory-caseinsensitive
branch
from
61345a8 to
a5e262e
Compare
| using System.Security.AccessControl; | ||
| using System.Security.Principal; | ||
|
|
||
| namespace GVFS.FunctionalTests.Tests.EnlistmentPerTestCase |
There was a problem hiding this comment.
That's a full-blown integration test, and it is heavy: it P/Invokes AdjustTokenPrivileges to enable SeRestorePrivilege, changes actual directory ownership to BUILTIN\Users, manipulates global git config, and cleans it all up. That requires elevation and is fragile in CI, as well as hard to reproduce locally in case of CI failures.
A lighter approach for CI would be to split into two layers:
-
Add unit tests specifically for
NormalizePath(): C:\Repos\Foo matches c:/repos/foo, trailing slashes are trimmed, mixed separators work, empty/null is safe. This is pure string manipulation, no native calls, runs anywhere. -
Unit test the control flow with a mock. LibGit2Repo already has a protected LibGit2Repo() constructor and virtual methods for mocking. The PR could make
CheckSafeDirectoryConfigForCaseSensitivityIssue(or at least the config-reading part) virtual, so a test subclass can feed fake safe.directory entries without touching global git config or real libgit2. Then you test: "if Open() fails with the ownership message and config contains a case-variant match, retry with corrected casing" vs. "if config has no match, throw as before".
Then you could drop entirely the directory-ownership manipulation. The actual libgit2 bug is well-established (issue libgit2/libgit2#7037). The question VFS for Git needs to test is not "does libgit2 have this bug?" but "does our workaround correctly detect the situation and retry with the right path?" That can be tested without triggering the real ownership mismatch.
There was a problem hiding this comment.
I've refactored to allow the needed mocks and converted the functional tests to unit tests. Thanks for the feedback!
Replace the heavyweight SafeDirectoryOwnershipTests functional test (which required elevation, P/Invoke for SeRestorePrivilege, directory ownership changes, and global git config manipulation) with two layers of lightweight unit tests: Layer 1: NormalizePathForSafeDirectoryComparison - pure string tests covering backslash/forward-slash conversion, case normalization, trailing slash trimming, and null/empty safety. Layer 2: Constructor control-flow tests using a mock subclass that overrides the native calls (InitNative, TryOpenRepo, GetLastNativeError, GetSafeDirectoryConfigEntries) to verify the safe.directory case- sensitivity workaround logic without touching libgit2 or real config. To support testability, extract virtual methods from LibGit2Repo for native interactions and make CheckSafeDirectoryConfigForCaseSensitivity- Issue protected. The NormalizePath helper is renamed to NormalizePathForSafeDirectoryComparison and scoped as internal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Libgit2 has a bug where it is case sensitive for safe.directory (especially the drive letter) when git.exe isn't. Until a fix can be made and propagated, work around it by matching the repo path requested to the configured safe directory.
See libgit2/libgit2#7037