From d487f91bf94a7df4d3ee4d2897a4c003c8dbefda Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 7 Aug 2026 15:52:59 -0700 Subject: [PATCH] Fix NullReferenceException when cloning into a non-empty directory `gvfs clone ` into a non-empty directory throws an unhandled NullReferenceException instead of reporting the expected error: Cannot clone @ : System.NullReferenceException: Object reference not set to an instance of an object. at GVFS.CommandLine.CloneVerb.Execute() + 0x9fc v1.0 behavior (expected): Cannot clone @ Error: Clone directory '' exists and is not empty Root cause: CloneVerb.Execute() unconditionally read enlistment.WorkingDirectoryBackingRoot to determine trustPackIndexes, even when TryCreateEnlistment failed (e.g. because the target directory already exists and is not empty). In that failure path enlistment is null, so the dereference throws. Extract the lookup into internal bool GetTrustPackIndexes(ITracer, Result, GVFSEnlistment), which only touches enlistment when cloneResult.Success is true; otherwise it returns the default without dereferencing enlistment. Delegates to the shared LibGit2Repo.GetConfigBoolOrDefault helper. Widen TryCreateEnlistment/Result from private to internal so they are directly unit-testable (GVFS assembly already grants InternalsVisibleTo to GVFS.UnitTests). Add GVFS.UnitTests.CommandLine.CloneVerbTests: - TryCreateEnlistmentFailsWithoutEnlistmentWhenTargetDirectoryIsNotEmpty: confirms the failure precondition (null enlistment on non-empty target dir). - TryCreateEnlistmentDoesNotFailForEmptyTargetDirectory: boundary case, an empty target directory does not trigger the "exists and is not empty" error. - TryCreateEnlistmentReportsNormalizedPathWhenItDiffersFromFullPath: covers the divergent full-vs-normalized-path error message branch. - GetTrustPackIndexesDoesNotThrowWhenCloneFailedAndEnlistmentIsNull: the actual regression test, driving the exact failed-clone/null-enlistment composition that used to throw. Verified by temporarily removing the cloneResult.Success gate: the test failed with the original NullReferenceException, then passed again once the gate was restored. Reviewed with an internal 6-lens review-swarm pass; the main finding was that an earlier draft of the regression test only proved TryCreateEnlistment's own contract (already true pre-fix) without exercising the actual Execute() null-dereference, addressed by the extraction and test above. Full unit test suite: 895 passed, 0 failed, 11 skipped (pre-existing, unrelated). Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../CommandLine/CloneVerbTests.cs | 97 +++++++++++++++++++ GVFS/GVFS/CommandLine/CloneVerb.cs | 37 +++++-- 2 files changed, 124 insertions(+), 10 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/CommandLine/CloneVerbTests.cs 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/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 2e9bb5276..4fa935de1 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -302,14 +302,8 @@ public override void Execute() { tracer.RelatedError(cloneResult.ErrorMessage); } - else - { - trustPackIndexes = LibGit2Repo.GetConfigBoolOrDefault( - tracer, - enlistment.WorkingDirectoryBackingRoot, - GVFSConstants.GitConfig.TrustPackIndexes, - GVFSConstants.GitConfig.TrustPackIndexesDefault); - } + + trustPackIndexes = this.GetTrustPackIndexes(tracer, cloneResult, enlistment); } if (cloneResult.Success) @@ -418,7 +412,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) @@ -883,7 +900,7 @@ private Result TryInitRepo(ITracer tracer, GitRefs refs, Enlistment enlistmentTo return new Result(true); } - private class Result + internal class Result { public Result(bool success) {