Fix NullReferenceException when cloning into a non-empty directory#2065
Draft
tyrielv wants to merge 3 commits into
Draft
Fix NullReferenceException when cloning into a non-empty directory#2065tyrielv wants to merge 3 commits into
tyrielv wants to merge 3 commits into
Conversation
Add a shared helper for transient config-only LibGit2Repo usage and switch short-lived call sites away from LibGit2RepoInvoker/object-store preload or duplicated try/catch blocks. Assisted-by: GPT-5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Replace the standalone LibGit2RepoExtensions static class with instance and static GetConfigBoolOrDefault methods directly on LibGit2Repo. This matches the existing repo.GetConfigBoolOrDefault(name, default) convention already documented in AGENTS.md and used by LibGit2RepoInvoker, and avoids an unnecessary extension-method indirection for a class we own in the same assembly. Also fixes a regression reintroduced by the earlier refactor: CloneVerb called LibGit2Repo.GetConfigBoolOrDefault using enlistment.WorkingDirectoryBackingRoot unconditionally, even when TryCreateEnlistment failed and enlistment was null. The call is now correctly gated on cloneResult.Success. Added a protected LibGit2Repo(ITracer tracer) constructor so test doubles can inject a mock tracer and assert on RelatedWarning calls from the new instance method, and rewrote the test file accordingly. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
CloneVerb.Execute() unconditionally created a LibGit2RepoInvoker using
enlistment.WorkingDirectoryBackingRoot to determine trustPackIndexes,
even when TryCreateEnlistment had failed (e.g. because the target
directory already exists and is not empty). In that failure case
enlistment is null, so the failed clone produced:
Cannot clone @ <target>: System.NullReferenceException: Object
reference not set to an instance of an object.
at GVFS.CommandLine.CloneVerb.Execute() + 0x9fc
instead of the expected v1.0 behavior:
Cannot clone @ <target>
Error: Clone directory '<target>' exists and is not empty
Extract the trustPackIndexes lookup into GetTrustPackIndexes(), which
only dereferences enlistment when cloneResult.Success is true. Also
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 covering:
- TryCreateEnlistment fails with a null enlistment when the target
directory is not empty (the failure precondition).
- GetTrustPackIndexes does not throw and returns the default when the
clone failed and enlistment is null (the regression itself; this
test reproduces the NullReferenceException when reverted).
Assisted-by: Claude Sonnet 5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
tyrielv
force-pushed
the
tyrielv/fix-clone-nonempty-nre
branch
from
37377d1 to
f4a4a25
Compare
auto-merge was automatically disabled
July 15, 2026 19:55
Pull request was converted to draft
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.
Summary
gvfs clone <url> <target>into a non-empty<target>directory now throws an unhandledNullReferenceExceptioninstead of reporting the expected error:v1.0 behavior (expected):
Root cause
CloneVerb.Execute()unconditionally constructs aLibGit2RepoInvokerfromenlistment.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
GetTrustPackIndexes(), which only touchesenlistmentwhencloneResult.Successistrue; otherwise it returns the default.Testing
Added
GVFS.UnitTests.CommandLine.CloneVerbTests:TryCreateEnlistmentFailsWithoutEnlistmentWhenTargetDirectoryIsNotEmpty— confirms the failure precondition (null enlistment on non-empty target dir).GetTrustPackIndexesDoesNotThrowWhenCloneFailedAndEnlistmentIsNull— reproduces the exactNullReferenceExceptionwhen the fix is reverted, and passes with it in place.Full unit test suite: 884 passed, 0 failed (11 pre-existing skips).