From 1493492d93857b553da1343574a56fe2b4b841d8 Mon Sep 17 00:00:00 2001 From: Sergiy Kulanov Date: Mon, 3 Aug 2026 23:24:23 +0300 Subject: [PATCH] EPMDEDP-17253: fix: Check the fetched ref namespace and force-fetch in remote checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Business impact: this hardens onboarding of existing repositories (clone strategy), the highest-visibility flow of platform adoption. Both of its shapes route through Checkout(remote=true): the explicit "branch to copy in default branch" option and the implicit case where the requested default branch exists in the source repository but is not its HEAD. The remote checkout fetched with a refspec that maps remote branches straight into local heads, then verified existence in the remote-tracking namespace that this fetch never writes. The check only passed because the preceding clone happens to leave remote-tracking refs behind — correctness by accident. Two real consequences: - a branch pushed to the source repository between the clone and the checkout (a minutes-wide window on large repositories, during active team work) was fetched into local heads, missed by the check, and collided with Create: a branch named X already exists. Users experienced this as flaky onboarding failures that disappear on retry, since a failed reconcile wipes the workdir and the fresh clone masks the defect again. - the fetch had no force flag, so a rebased or force-pushed upstream branch failed the checkout outright, unlike the sibling CheckoutRemoteBranch which already forces the identical refspec. Fix: verify existence in local heads, the namespace the fetch actually writes, so Create fires only for a genuinely absent branch; set Force on the fetch. Covered by four regression tests, the primary one red on the previous code, and verified on the kind testbed against GitLab with a clone-strategy codebase copying a non-HEAD branch into the default branch. Signed-off-by: Sergiy Kulanov --- pkg/git/checkout_remote_test.go | 137 ++++++++++++++++++++++++++++++++ pkg/git/provider.go | 15 ++-- 2 files changed, 146 insertions(+), 6 deletions(-) create mode 100644 pkg/git/checkout_remote_test.go diff --git a/pkg/git/checkout_remote_test.go b/pkg/git/checkout_remote_test.go new file mode 100644 index 00000000..6c0e71fe --- /dev/null +++ b/pkg/git/checkout_remote_test.go @@ -0,0 +1,137 @@ +package v2 + +import ( + "context" + "os" + "path" + "testing" + "time" + + gogit "github.com/go-git/go-git/v5" + gogitconfig "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/stretchr/testify/require" +) + +func seedCommit(t *testing.T, originDir, fileContent, pushRefSpec string) plumbing.Hash { + t.Helper() + + seed := t.TempDir() + repo, err := gogit.PlainInit(seed, false) + require.NoError(t, err) + + require.NoError(t, os.WriteFile(path.Join(seed, "f.txt"), []byte(fileContent), 0o600)) + + wt, err := repo.Worktree() + require.NoError(t, err) + + _, err = wt.Add("f.txt") + require.NoError(t, err) + + hash, err := wt.Commit("init "+fileContent, &gogit.CommitOptions{ + Author: &object.Signature{Name: "t", Email: "t@t", When: time.Now()}, + }) + require.NoError(t, err) + + _, err = repo.CreateRemote(&gogitconfig.RemoteConfig{Name: "origin", URLs: []string{originDir}}) + require.NoError(t, err) + require.NoError(t, repo.Push(&gogit.PushOptions{ + RemoteName: "origin", + RefSpecs: []gogitconfig.RefSpec{gogitconfig.RefSpec(pushRefSpec)}, + Force: true, + })) + + return hash +} + +func buildOriginWithClone(t *testing.T) (string, string) { + t.Helper() + + originDir := t.TempDir() + _, err := gogit.PlainInit(originDir, true) + require.NoError(t, err) + + seedCommit(t, originDir, "base", "+refs/heads/master:refs/heads/master") + + workDir := path.Join(t.TempDir(), "clone") + gp := NewGitProvider(Config{}) + require.NoError(t, gp.Clone(context.Background(), originDir, workDir)) + + return originDir, workDir +} + +func workdirHead(t *testing.T, workDir string) *plumbing.Reference { + t.Helper() + + repo, err := gogit.PlainOpen(workDir) + require.NoError(t, err) + + head, err := repo.Head() + require.NoError(t, err) + + return head +} + +// A branch pushed to origin after the workdir was cloned is fetched into +// refs/heads by Checkout's refs/*:refs/* refspec; the existence check must look +// there, or the checkout collides on Create with the ref the fetch just wrote. +func TestCheckout_RemoteBranchCreatedAfterClone(t *testing.T) { + originDir, workDir := buildOriginWithClone(t) + + origin, err := gogit.PlainOpen(originDir) + require.NoError(t, err) + + masterRef, err := origin.Reference(plumbing.NewBranchReferenceName("master"), false) + require.NoError(t, err) + require.NoError(t, origin.Storer.SetReference( + plumbing.NewHashReference(plumbing.NewBranchReferenceName("late-branch"), masterRef.Hash()), + )) + + gp := NewGitProvider(Config{}) + err = gp.Checkout(context.Background(), workDir, "late-branch", true) + require.NoError(t, err, "checkout of a branch created after clone must succeed") + + require.Equal(t, "late-branch", workdirHead(t, workDir).Name().Short()) +} + +// TestCheckout_RemoteBranchPresentAtCloneTime covers the branchToCopy flow in +// its common shape: the target branch existed when the workdir was cloned. +func TestCheckout_RemoteBranchPresentAtCloneTime(t *testing.T) { + originDir := t.TempDir() + _, err := gogit.PlainInit(originDir, true) + require.NoError(t, err) + + seedCommit(t, originDir, "base", "+refs/heads/master:refs/heads/master") + seedCommit(t, originDir, "base", "+refs/heads/master:refs/heads/feature") + + workDir := path.Join(t.TempDir(), "clone") + gp := NewGitProvider(Config{}) + require.NoError(t, gp.Clone(context.Background(), originDir, workDir)) + + require.NoError(t, gp.Checkout(context.Background(), workDir, "feature", true)) + require.Equal(t, "feature", workdirHead(t, workDir).Name().Short()) +} + +// TestCheckout_BranchAbsentEverywhere preserves the create-fallback: a branch +// that exists neither locally nor on the remote is created from HEAD. +func TestCheckout_BranchAbsentEverywhere(t *testing.T) { + _, workDir := buildOriginWithClone(t) + + gp := NewGitProvider(Config{}) + require.NoError(t, gp.Checkout(context.Background(), workDir, "brand-new", true)) + require.Equal(t, "brand-new", workdirHead(t, workDir).Name().Short()) +} + +// TestCheckout_ForcePushedRemoteBranch: a rewritten upstream branch must not +// fail the fetch of a cached workdir; the checkout lands on the new tip. +func TestCheckout_ForcePushedRemoteBranch(t *testing.T) { + originDir, workDir := buildOriginWithClone(t) + + rewritten := seedCommit(t, originDir, "rewritten-history", "+refs/heads/master:refs/heads/master") + + gp := NewGitProvider(Config{}) + require.NoError(t, gp.Checkout(context.Background(), workDir, "master", true), + "force-pushed upstream branch must not fail the checkout fetch") + require.Equal(t, rewritten, workdirHead(t, workDir).Hash()) +} diff --git a/pkg/git/provider.go b/pkg/git/provider.go index 6f46b226..9e9b6ec6 100644 --- a/pkg/git/provider.go +++ b/pkg/git/provider.go @@ -315,7 +315,6 @@ func (p *GitProvider) Checkout(ctx context.Context, directory, branchName string createBranch := true if remote { - // Fetch from remote first auth, err := p.getAuth() if err != nil { return fmt.Errorf("failed to get authentication: %w", err) @@ -325,6 +324,10 @@ func (p *GitProvider) Checkout(ctx context.Context, directory, branchName string RefSpecs: []config.RefSpec{"refs/*:refs/*"}, Auth: auth, Progress: os.Stdout, + // The refspec has no force prefix, so without this a rebased or + // force-pushed upstream branch fails the fetch with ErrForceNeeded + // against a cached workdir. + Force: true, } err = repo.FetchContext(ctx, fetchOptions) @@ -332,12 +335,12 @@ func (p *GitProvider) Checkout(ctx context.Context, directory, branchName string return fmt.Errorf("failed to fetch: %w", err) } - // Check if branch exists remotely - remoteBranchRef := plumbing.NewRemoteReferenceName("origin", branchName) - - _, err = repo.Reference(remoteBranchRef, false) + // The refspec above maps remote branches straight into local + // refs/heads, so that is the namespace that proves existence here. + // Checking refs/remotes/origin instead would miss any branch the + // fetch just materialized and collide on Create below. + _, err = repo.Reference(plumbing.NewBranchReferenceName(branchName), false) if err == nil { - // Branch exists remotely, don't create locally createBranch = false } }