diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index dafcc8d54..09f2e0e16 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -39,8 +39,13 @@ public LibGit2Repo(ITracer tracer, string repoPath) } protected LibGit2Repo() + : this(NullTracer.Instance) { - this.Tracer = NullTracer.Instance; + } + + protected LibGit2Repo(ITracer tracer) + { + this.Tracer = tracer; } ~LibGit2Repo() @@ -306,6 +311,55 @@ public virtual string GetConfigString(string name) } } + /// + /// Reads a boolean config value from this already-open repo, falling back to + /// if the key is unset or the read fails for any reason + /// (e.g. a corrupt/unreadable config). + /// + public bool GetConfigBoolOrDefault(string key, bool defaultValue) + { + try + { + return this.GetConfigBool(key) ?? defaultValue; + } + catch (Exception e) + { + this.Tracer.RelatedWarning($"Failed to read {key} config, using default: {e.Message}"); + return defaultValue; + } + } + + /// + /// Reads a single boolean config value from the repo at , + /// opening and disposing a transient for the lookup. Prefer + /// this over for one-off config reads: + /// LibGit2RepoInvoker.InitializeSharedRepo forces the object store to load, which is + /// wasted work when all that's needed is a single config value. Falls back to + /// if the repo can't be opened or the read fails for any + /// reason. + /// + public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, string key, bool defaultValue) + { + try + { + using (LibGit2Repo repo = new LibGit2Repo(tracer, repoPath)) + { + return repo.GetConfigBoolOrDefault(key, defaultValue); + } + } + catch (InvalidDataException) + { + // The LibGit2Repo constructor already logged a RelatedWarning with the native + // failure reason before throwing; avoid logging the same failure twice. + return defaultValue; + } + catch (Exception e) + { + tracer.RelatedWarning($"Failed to read {key} config, using default: {e.Message}"); + return defaultValue; + } + } + public void ForEachMultiVarConfig(string key, MultiVarConfigCallback callback) { if (Native.Config.GetConfig(out IntPtr configHandle, this.RepoHandle) != Native.ResultCode.Success) diff --git a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj index 69988ac80..3b996578e 100644 --- a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj +++ b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj @@ -118,4 +118,3 @@ - diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 00db23872..940b2cf37 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -171,10 +171,11 @@ private static bool HasShortFlag(string arg, string flag) private static bool ConfigurationAllowsHydrationStatus() { - using (LibGit2RepoInvoker repo = new LibGit2RepoInvoker(NullTracer.Instance, normalizedCurrentDirectory)) - { - return repo.GetConfigBoolOrDefault(GVFSConstants.GitConfig.ShowHydrationStatus, GVFSConstants.GitConfig.ShowHydrationStatusDefault); - } + return LibGit2Repo.GetConfigBoolOrDefault( + NullTracer.Instance, + normalizedCurrentDirectory, + GVFSConstants.GitConfig.ShowHydrationStatus, + GVFSConstants.GitConfig.ShowHydrationStatusDefault); } /// diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 1e2c69ffb..4cde0a876 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -475,27 +475,11 @@ private GVFSContext CreateContext() private bool IsBackgroundCacheAuthEnabled() { - // Read the flag via libgit2 (in-process) rather than spawning git.exe. - // The GVFSContext (and its shared libgit2 repo) is not created until - // later in mount, so open a short-lived repo here just for the config - // read. Default to off on any failure. - try - { - using (LibGit2Repo repo = new LibGit2Repo(this.tracer, this.enlistment.WorkingDirectoryBackingRoot)) - { - return repo.GetConfigBool(GVFSConstants.GitConfig.BackgroundCacheAuth) - ?? GVFSConstants.GitConfig.BackgroundCacheAuthDefault; - } - } - catch (Exception e) - { - this.tracer.RelatedWarning( - "Failed to read {0} config, defaulting to {1}: {2}", - GVFSConstants.GitConfig.BackgroundCacheAuth, - GVFSConstants.GitConfig.BackgroundCacheAuthDefault, - e.Message); - return GVFSConstants.GitConfig.BackgroundCacheAuthDefault; - } + return LibGit2Repo.GetConfigBoolOrDefault( + this.tracer, + this.enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.BackgroundCacheAuth, + GVFSConstants.GitConfig.BackgroundCacheAuthDefault); } private void ValidateMountPoints() diff --git a/GVFS/GVFS.UnitTests/CommandLine/CloneVerbTests.cs b/GVFS/GVFS.UnitTests/CommandLine/CloneVerbTests.cs new file mode 100644 index 000000000..e8055ce5e --- /dev/null +++ b/GVFS/GVFS.UnitTests/CommandLine/CloneVerbTests.cs @@ -0,0 +1,97 @@ +using GVFS.Common; +using GVFS.CommandLine; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.IO; + +namespace GVFS.UnitTests.CommandLine +{ + [TestFixture] + public class CloneVerbTests + { + private CloneVerb cloneVerb; + private string testDir; + + [SetUp] + public void Setup() + { + this.cloneVerb = new CloneVerb(); + this.testDir = Path.Combine(Path.GetTempPath(), "CloneVerbTests_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(this.testDir); + } + + [TearDown] + public void TearDown() + { + if (Directory.Exists(this.testDir)) + { + Directory.Delete(this.testDir, recursive: true); + } + } + + [TestCase] + public void TryCreateEnlistmentFailsWithoutEnlistmentWhenTargetDirectoryIsNotEmpty() + { + File.WriteAllText(Path.Combine(this.testDir, "preexisting.txt"), "content"); + + CloneVerb.Result result = this.cloneVerb.TryCreateEnlistment( + this.testDir, + this.testDir, + out GVFSEnlistment enlistment); + + Assert.IsFalse(result.Success); + Assert.IsNull(enlistment); + StringAssert.Contains("exists and is not empty", result.ErrorMessage); + } + + [TestCase] + public void TryCreateEnlistmentDoesNotFailForEmptyTargetDirectory() + { + // testDir is created empty by Setup and never written to in this test. + CloneVerb.Result result = this.cloneVerb.TryCreateEnlistment( + this.testDir, + this.testDir, + out GVFSEnlistment enlistment); + + StringAssert.DoesNotContain("exists and is not empty", result.ErrorMessage ?? string.Empty); + } + + [TestCase] + public void TryCreateEnlistmentReportsNormalizedPathWhenItDiffersFromFullPath() + { + File.WriteAllText(Path.Combine(this.testDir, "preexisting.txt"), "content"); + string fullPath = this.testDir + Path.DirectorySeparatorChar; + + CloneVerb.Result result = this.cloneVerb.TryCreateEnlistment( + fullPath, + this.testDir, + out GVFSEnlistment enlistment); + + Assert.IsFalse(result.Success); + Assert.IsNull(enlistment); + StringAssert.Contains($"'{fullPath}'", result.ErrorMessage); + StringAssert.Contains($"['{this.testDir}']", result.ErrorMessage); + } + + // Regression test: this is the actual code path that used to throw a + // NullReferenceException when `gvfs clone` targeted a non-empty directory. + // TryCreateEnlistment (above) fails and returns a null enlistment; CloneVerb.Execute() + // used to unconditionally dereference that null enlistment to read the trustPackIndexes + // config, crashing instead of reporting the "exists and is not empty" error. Execute() + // itself cannot be unit-tested (it terminates the process via Environment.Exit()), so + // GetTrustPackIndexes was extracted as the smallest testable seam that reproduces the + // exact failure condition: a failed clone result with a null enlistment. + [TestCase] + public void GetTrustPackIndexesDoesNotThrowWhenCloneFailedAndEnlistmentIsNull() + { + MockTracer tracer = new MockTracer(); + CloneVerb.Result failedCloneResult = new CloneVerb.Result("Clone directory exists and is not empty"); + + bool trustPackIndexes = true; + Assert.DoesNotThrow(() => trustPackIndexes = this.cloneVerb.GetTrustPackIndexes(tracer, failedCloneResult, enlistment: null)); + + Assert.AreEqual(GVFSConstants.GitConfig.TrustPackIndexesDefault, trustPackIndexes); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs new file mode 100644 index 000000000..70031e2ec --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs @@ -0,0 +1,127 @@ +using GVFS.Common.Git; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.IO; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class LibGit2RepoConfigLookupTests + { + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsConfiguredValue() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, true)) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", false); + + value.ShouldEqual(true); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsDefaultWhenKeyIsUnset() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, (bool?)null)) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", true); + + value.ShouldEqual(true); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnLibGit2ExceptionAndLogsOnce() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, new LibGit2Exception("boom"))) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", false); + + value.ShouldEqual(false); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Failed to read gvfs.test config, using default: boom"); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnInvalidDataExceptionAndLogsOnce() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, new InvalidDataException("corrupt config"))) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", false); + + value.ShouldEqual(false); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Failed to read gvfs.test config, using default: corrupt config"); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnPathReturnsDefaultForMissingRepoAndLogsExactlyOnce() + { + MockTracer tracer = new MockTracer(); + + // A GUID-suffixed path under the OS temp directory is guaranteed not to exist and + // does not depend on any particular drive letter being unmapped (unlike a + // hardcoded "Z:\..." path, which could resolve on a host with that drive mapped). + string missingRepoPath = Path.Combine( + Path.GetTempPath(), + "LibGit2RepoConfigLookupTests_" + Guid.NewGuid().ToString("N")); + + bool value = LibGit2Repo.GetConfigBoolOrDefault( + tracer, + missingRepoPath, + "gvfs.test", + false); + + value.ShouldEqual(false); + + // The LibGit2Repo constructor logs a RelatedWarning with the native open-failure + // reason before throwing InvalidDataException; the static helper's catch does not + // log a second time for that case (see LibGit2Repo.GetConfigBoolOrDefault), so + // exactly one warning is expected here. + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Couldn't open repo at"); + } + + private class MockConfigRepo : LibGit2Repo + { + private readonly bool? value; + private readonly Exception exceptionToThrow; + + public MockConfigRepo(MockTracer tracer, bool? value) + : base(tracer) + { + this.value = value; + } + + public MockConfigRepo(MockTracer tracer, Exception exceptionToThrow) + : base(tracer) + { + this.exceptionToThrow = exceptionToThrow; + } + + public override bool? GetConfigBool(string name) + { + if (this.exceptionToThrow != null) + { + throw this.exceptionToThrow; + } + + return this.value; + } + } + } +} diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 977c5b082..23e6b2303 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -152,7 +152,7 @@ public override void Execute() CacheServerInfo cacheServer = null; ServerGVFSConfig serverGVFSConfig = null; - bool trustPackIndexes; + bool trustPackIndexes = GVFSConstants.GitConfig.TrustPackIndexesDefault; using (JsonTracer tracer = new JsonTracer(GVFSConstants.GVFSEtwProviderName, "GVFSClone")) { @@ -249,10 +249,7 @@ public override void Execute() tracer.RelatedError(cloneResult.ErrorMessage); } - using (var repo = new LibGit2RepoInvoker(tracer, enlistment.WorkingDirectoryBackingRoot)) - { - trustPackIndexes = repo.GetConfigBoolOrDefault(GVFSConstants.GitConfig.TrustPackIndexes, GVFSConstants.GitConfig.TrustPackIndexesDefault); - } + trustPackIndexes = this.GetTrustPackIndexes(tracer, cloneResult, enlistment); } if (cloneResult.Success) @@ -361,7 +358,30 @@ private static bool IsForceCheckoutErrorCloneFailure(string checkoutError) return true; } - private Result TryCreateEnlistment( + /// + /// Determines whether pack indexes should be trusted for the newly cloned enlistment. + /// Only reads the enlistment's git config when indicates + /// the clone succeeded; is null when the clone failed + /// (e.g. TryCreateEnlistment failed because the target directory was not empty), and must + /// not be dereferenced in that case. This gating is what fixes the NullReferenceException + /// regression where `gvfs clone` into a non-empty directory used to crash instead of + /// reporting "exists and is not empty". + /// + internal bool GetTrustPackIndexes(ITracer tracer, Result cloneResult, GVFSEnlistment enlistment) + { + if (!cloneResult.Success) + { + return GVFSConstants.GitConfig.TrustPackIndexesDefault; + } + + return LibGit2Repo.GetConfigBoolOrDefault( + tracer, + enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.TrustPackIndexes, + GVFSConstants.GitConfig.TrustPackIndexesDefault); + } + + internal Result TryCreateEnlistment( string fullEnlistmentRootPathParameter, string normalizedEnlistementRootPath, out GVFSEnlistment enlistment) @@ -813,7 +833,7 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo return new Result(true); } - private class Result + internal class Result { public Result(bool success) { diff --git a/GVFS/GVFS/CommandLine/PrefetchVerb.cs b/GVFS/GVFS/CommandLine/PrefetchVerb.cs index 945984d62..6fa0d91f4 100644 --- a/GVFS/GVFS/CommandLine/PrefetchVerb.cs +++ b/GVFS/GVFS/CommandLine/PrefetchVerb.cs @@ -700,19 +700,11 @@ private string GetCacheServerDisplay(CacheServerInfo cacheServer, string repoUrl private bool IsPrefetchOffloadEnabled(ITracer tracer, GVFSEnlistment enlistment) { - try - { - using (LibGit2Repo repo = new LibGit2Repo(tracer, enlistment.WorkingDirectoryBackingRoot)) - { - bool? enabled = repo.GetConfigBool(GVFSConstants.GitConfig.PrefetchOffload); - return enabled ?? GVFSConstants.GitConfig.PrefetchOffloadDefault; - } - } - catch (Exception ex) - { - tracer.RelatedWarning($"Failed to read '{GVFSConstants.GitConfig.PrefetchOffload}' config; defaulting to {GVFSConstants.GitConfig.PrefetchOffloadDefault}: {ex.GetType().Name}: {ex.Message}"); - return GVFSConstants.GitConfig.PrefetchOffloadDefault; - } + return LibGit2Repo.GetConfigBoolOrDefault( + tracer, + enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.PrefetchOffload, + GVFSConstants.GitConfig.PrefetchOffloadDefault); } ///