Fix NullReferenceException when cloning into a non-empty directory

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)
             }
         }
 
+        /// <summary>
+        /// Reads a boolean config value from this already-open repo, falling back to
+        /// <paramref name="defaultValue"/> if the key is unset or the read fails for any reason
+        /// (e.g. a corrupt/unreadable config).
+        /// </summary>
+        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;
+            }
+        }
+
+        /// <summary>
+        /// Reads a single boolean config value from the repo at <paramref name="repoPath"/>,
+        /// opening and disposing a transient <see cref="LibGit2Repo"/> for the lookup. Prefer
+        /// this over <see cref="LibGit2RepoInvoker"/> 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
+        /// <paramref name="defaultValue"/> if the repo can't be opened or the read fails for any
+        /// reason.
+        /// </summary>
+        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)

GVFS/GVFS.Hooks/GVFS.Hooks.csproj

@@ -118,4 +118,3 @@
 
 
 </Project>
-

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);
         }
 
         /// <summary>

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()

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);
+        }
+    }
+}

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;
+            }
+        }
+    }
+}