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/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..1277355ab 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")) { @@ -248,10 +248,13 @@ public override void Execute() { tracer.RelatedError(cloneResult.ErrorMessage); } - - using (var repo = new LibGit2RepoInvoker(tracer, enlistment.WorkingDirectoryBackingRoot)) + else { - trustPackIndexes = repo.GetConfigBoolOrDefault(GVFSConstants.GitConfig.TrustPackIndexes, GVFSConstants.GitConfig.TrustPackIndexesDefault); + trustPackIndexes = LibGit2Repo.GetConfigBoolOrDefault( + tracer, + enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.TrustPackIndexes, + GVFSConstants.GitConfig.TrustPackIndexesDefault); } } 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); } ///