Fix NullReferenceException when cloning into a non-empty directory - #2065
Open
tyrielv wants to merge 2 commits into
Open
Fix NullReferenceException when cloning into a non-empty directory#2065tyrielv wants to merge 2 commits into
tyrielv wants to merge 2 commits into
Conversation
tyrielv
marked this pull request as ready for review
July 15, 2026 18:37
tyrielv
enabled auto-merge
July 15, 2026 18:37
tyrielv
force-pushed
the
tyrielv/fix-clone-nonempty-nre
branch
from
July 15, 2026 19:11
37377d1 to
f4a4a25
Compare
tyrielv
marked this pull request as draft
July 15, 2026 19:55
auto-merge was automatically disabled
July 15, 2026 19:55
Pull request was converted to draft
tyrielv
force-pushed
the
tyrielv/fix-clone-nonempty-nre
branch
from
August 7, 2026 22:19
f4a4a25 to
72ce378
Compare
tyrielv
marked this pull request as ready for review
August 7, 2026 22:47
Add LibGit2Repo.GetConfigBoolOrDefault(...) (instance + static overloads) for one-off boolean config reads, replacing scattered short-lived LibGit2Repo/LibGit2RepoInvoker usage at 4 call sites: - GVFS/CommandLine/CloneVerb.cs (gvfs.trust-pack-indexes) - GVFS.Hooks/Program.cs (gvfs.show-hydration-status) - GVFS.Mount/InProcessMount.cs (gvfs.background-cache-auth) - GVFS/CommandLine/PrefetchVerb.cs (gvfs.prefetch-offload) LibGit2RepoInvoker.InitializeSharedRepo() intentionally forces an object-store probe so long-lived/shared callers can amortize object-store load costs. That is wasted work for one-off config reads that immediately dispose the repo. The helper methods live directly on LibGit2Repo rather than a separate extension class, matching the repo.GetConfigBoolOrDefault(name, default) convention already documented in AGENTS.md, and avoiding unnecessary indirection for a class the team owns in the same assembly. Both methods fall back to defaultValue and log a RelatedWarning on any failure, matching the "default on any failure" contract each call site previously implemented independently. Added a protected LibGit2Repo(ITracer tracer) constructor to support test doubles that inject a mock tracer without opening a real repo. Surveyed master for other short-lived config-only LibGit2Repo/ LibGit2RepoInvoker usage; PrefetchStep.cs, GitStatusCache.cs, and GitRepo.cs were left alone because they use shared/long-lived repo access, not the transient anti-pattern this change addresses. Reviewed with an internal 6-lens review-swarm pass (correctness, security, design, tests, async-parallelism, risk-rollout); addressed all actionable findings: - Widened the shared helper's exception handling to a plain catch (Exception), restoring the "default on any failure" guarantee InProcessMount/PrefetchVerb relied on before this refactor. - Fixed a double-RelatedWarning log on the repo-open-failure path. - Replaced a hardcoded, non-portable "Z:\..." path in a unit test with a GUID-suffixed temp path. - Added test coverage for the unset-key (null-coalescing) branch and the InvalidDataException catch arm. - Simplified the parameterless constructor to delegate to the tracer-accepting one. Full unit test suite: 891 passed, 0 failed, 11 skipped (pre-existing, unrelated). Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
`gvfs clone <url> <target>` into a non-empty <target> directory throws an
unhandled NullReferenceException instead of reporting the expected error:
Cannot clone @ <target>: System.NullReferenceException: Object
reference not set to an instance of an object.
at GVFS.CommandLine.CloneVerb.Execute() + 0x9fc
v1.0 behavior (expected):
Cannot clone @ <target>
Error: Clone directory '<target>' 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 <tyrielv@gmail.com>
tyrielv
force-pushed
the
tyrielv/fix-clone-nonempty-nre
branch
from
August 7, 2026 22:53
72ce378 to
332b4fc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR is stacked on #2066 ("Share transient libgit2 config lookup helper"). #2066 must merge first; this branch will then be a clean single-commit diff against
master.This PR is a single commit —
332b4fc"Fix NullReferenceException when cloning into a non-empty directory" is this PR's entire contribution. Everything else visible in the diff (theLibGit2Repo.GetConfigBoolOrDefaulthelper this commit depends on) belongs to #2066 and is under review there.Both are stabilization/bug-fix work for the current shippable line (targets
master, notvnext), so I'd rather not block either on waiting for the other — happy to have them reviewed in parallel.Summary
gvfs clone <url> <target>into a non-empty<target>directory throws an unhandledNullReferenceExceptioninstead of reporting the expected error:v1.0 behavior (expected):
Root cause
CloneVerb.Execute()unconditionally readenlistment.WorkingDirectoryBackingRootto determinetrustPackIndexes, even whenTryCreateEnlistmentfailed (e.g. because the target directory already exists and is not empty). In that failure pathenlistmentisnull, so the dereference throws.Fix
Extracted the lookup into
internal bool GetTrustPackIndexes(ITracer, Result, GVFSEnlistment), which only touchesenlistmentwhencloneResult.Successistrue; otherwise it returns the default without dereferencingenlistment. Delegates to the sharedLibGit2Repo.GetConfigBoolOrDefaulthelper added in #2066.Testing
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: drives the exact failed-clone/null-enlistment composition that used to throw. Verified by temporarily removing thecloneResult.Successgate — the test failed with the originalNullReferenceException, then passed again once the gate was restored.Review
Reviewed with an internal 6-lens review-swarm pass. The main finding: an earlier draft of the regression test only proved
TryCreateEnlistment's own contract (already true pre-fix) without exercising the actualExecute()null-dereference — addressed by the extraction and new test above.Validation
Full unit test suite: 895 passed, 0 failed (11 pre-existing skips, unrelated to this change).