From 44aac9328aa3a67d3b3bd639230353f81450fa46 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 12 Jun 2026 08:29:43 -0400 Subject: [PATCH] Fix force-with-lease push for branches without tracking refs gh stack sync could rebase a stack successfully then fail the final force push with "stale info" when a branch lacked a local tracking ref (refs/remotes//). This happened because: 1. FetchBranches pre-filtered branches by existing tracking ref, so a branch with no tracking ref was never fetched and never gained one. 2. Push used a bare --force-with-lease flag, which has no lease basis for a branch without a tracking ref, causing git to reject the push. FetchBranches now uses explicit refspecs for every branch: +refs/heads/:refs/remotes// This creates or updates tracking refs regardless of prior state. The fast-path (single fetch) and per-branch fallback (for branches absent on the remote) are preserved. Push now builds explicit per-branch lease arguments when force=true: --force-with-lease=refs/heads/: for branches with a tracking ref, or: --force-with-lease=refs/heads/: (empty expected value = "must not exist") for branches absent on the remote. Explicit destination refspecs (:refs/heads/) remove dependence on push.default and upstream configuration. The non-force push path is unchanged. Added 6 integration tests using real bare git remotes: - Branch with current tracking ref: push succeeds - Tracking ref deleted locally (regression test for #118): push succeeds - Remote advanced by another client: push rejected (safety preserved) - New branch absent on remote: created via empty-expect lease - New branch race condition: rejected (safety preserved) - Mixed stack (tracked + untracked branches): all succeed after fetch Fixes #118 --- internal/git/gitops.go | 57 ++++-- internal/git/gitops_test.go | 355 ++++++++++++++++++++++++++++++++++++ 2 files changed, 394 insertions(+), 18 deletions(-) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index 462a23f..214766f 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -119,28 +119,27 @@ func (d *defaultOps) Fetch(remote string) error { } func (d *defaultOps) FetchBranches(remote string, branches []string) error { - // Only fetch branches that already have a remote tracking ref. - var tracked []string - for _, b := range branches { - ref := fmt.Sprintf("refs/remotes/%s/%s", remote, b) - if err := runSilent("rev-parse", "--verify", "--quiet", ref); err == nil { - tracked = append(tracked, b) - } - } - if len(tracked) == 0 { + if len(branches) == 0 { return nil } - // Fast path: fetch all tracked branches in a single call. + // Build explicit refspecs that create/update tracking refs for every + // branch, regardless of whether a tracking ref already exists. + // The + prefix allows non-fast-forward tracking-ref updates. + refspecs := make([]string, len(branches)) + for i, b := range branches { + refspecs[i] = fmt.Sprintf("+refs/heads/%s:refs/remotes/%s/%s", b, remote, b) + } + // Fast path: fetch all branches in a single call. args := []string{"fetch", remote} - args = append(args, tracked...) + args = append(args, refspecs...) if err := runSilent(args...); err == nil { return nil } - // Fallback: a ref may have been deleted on the remote while the - // local tracking ref still exists. Fetch branches individually so - // one missing ref doesn't block the others. - for _, b := range tracked { - _ = runSilent("fetch", remote, b) + // Fallback: one branch may be absent on the remote or deleted since + // the last fetch. Fetch individually so one missing branch doesn't + // block the rest. Per-branch failure is expected and tolerated. + for _, rs := range refspecs { + _ = runSilent("fetch", remote, rs) } return nil } @@ -165,12 +164,34 @@ func (d *defaultOps) CreateBranch(name, base string) error { func (d *defaultOps) Push(remote string, branches []string, force, atomic bool) error { args := []string{"push", remote} if force { - args = append(args, "--force-with-lease") + // Build explicit per-branch leases and refspecs. This removes + // dependence on push.default / upstream configuration and + // ensures correct lease values for branches whose tracking ref + // was missing before the preceding FetchBranches call. + for _, b := range branches { + trackingRef := fmt.Sprintf("refs/remotes/%s/%s", remote, b) + sha, err := run("rev-parse", "--verify", "--quiet", trackingRef) + if err == nil && sha != "" { + // Tracking ref exists: lease against the known SHA. + args = append(args, fmt.Sprintf("--force-with-lease=refs/heads/%s:%s", b, sha)) + } else { + // No tracking ref: branch is absent on remote (never + // pushed). Empty expected value means "must not exist". + args = append(args, fmt.Sprintf("--force-with-lease=refs/heads/%s:", b)) + } + } } if atomic { args = append(args, "--atomic") } - args = append(args, branches...) + if force { + // Explicit refspecs: :refs/heads/. + for _, b := range branches { + args = append(args, fmt.Sprintf("%s:refs/heads/%s", b, b)) + } + } else { + args = append(args, branches...) + } return runSilent(args...) } diff --git a/internal/git/gitops_test.go b/internal/git/gitops_test.go index 9d0e7d4..d2c171b 100644 --- a/internal/git/gitops_test.go +++ b/internal/git/gitops_test.go @@ -1,11 +1,366 @@ package git import ( + "os" + "os/exec" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// --------------------------------------------------------------------------- +// Helpers for integration tests that use real git repos +// --------------------------------------------------------------------------- + +// gitExec runs a git command in the given directory and returns trimmed stdout. +func gitExec(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=Test", + "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=Test", + "GIT_COMMITTER_EMAIL=test@test.com", + ) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %s in %s:\n%s", strings.Join(args, " "), dir, string(out)) + return strings.TrimSpace(string(out)) +} + +// gitExecMayFail runs a git command allowing failure. Returns stdout and error. +func gitExecMayFail(t *testing.T, dir string, args ...string) (string, error) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=Test", + "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=Test", + "GIT_COMMITTER_EMAIL=test@test.com", + ) + out, err := cmd.CombinedOutput() + return strings.TrimSpace(string(out)), err +} + +// setupBareAndClone creates a bare repo, clones it, makes an initial commit on +// main, and pushes. Returns (bareDir, cloneDir). +func setupBareAndClone(t *testing.T) (string, string) { + t.Helper() + bareDir := filepath.Join(t.TempDir(), "bare.git") + + // Use -c safe.bareRepository=all so git init --bare works in temp dirs. + // Use -b main to ensure a consistent default branch name across environments. + cmd := exec.Command("git", "-c", "safe.bareRepository=all", "init", "--bare", "-b", "main", bareDir) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git init --bare: %s", string(out)) + + cloneDir := filepath.Join(t.TempDir(), "clone") + gitExec(t, ".", "clone", bareDir, cloneDir) + + // Initial commit so main exists. + writeFile(t, cloneDir, "init.txt", "hello") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "initial commit") + gitExec(t, cloneDir, "push", "origin", "main") + + return bareDir, cloneDir +} + +// writeFile creates or overwrites a file in dir. +func writeFile(t *testing.T, dir, name, content string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0644)) +} + +// withGitDir temporarily sets the git client to operate in dir by changing +// the working directory. Returns a restore function. +func withGitDir(t *testing.T, dir string) func() { + t.Helper() + old, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(dir)) + return func() { _ = os.Chdir(old) } +} + +// remoteBranchSHA returns the SHA of a branch on the bare remote. +func remoteBranchSHA(t *testing.T, bareDir, branch string) string { + t.Helper() + cmd := exec.Command("git", "-C", bareDir, "-c", "safe.bareRepository=all", + "rev-parse", "refs/heads/"+branch) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "rev-parse in bare repo %s:\n%s", bareDir, string(out)) + return strings.TrimSpace(string(out)) +} + +// --------------------------------------------------------------------------- +// Integration tests for FetchBranches + Push (force-with-lease) +// --------------------------------------------------------------------------- + +// Test 1: Branch exists remotely with a current tracking ref. +// Push should succeed and update the remote. +func TestIntegration_Push_ExistingBranchCurrentTrackingRef(t *testing.T) { + bareDir, cloneDir := setupBareAndClone(t) + restore := withGitDir(t, cloneDir) + defer restore() + + d := &defaultOps{} + + // Create a branch, push it, then make a new commit. + gitExec(t, cloneDir, "checkout", "-b", "b1") + writeFile(t, cloneDir, "b1.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 initial") + gitExec(t, cloneDir, "push", "origin", "b1") + + // Make a local commit (simulating rebase). + writeFile(t, cloneDir, "b1.txt", "v2") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 updated") + + localSHA := gitExec(t, cloneDir, "rev-parse", "b1") + + // FetchBranches should update tracking ref. + err := d.FetchBranches("origin", []string{"b1"}) + require.NoError(t, err) + + // Push with force-with-lease should succeed. + err = d.Push("origin", []string{"b1"}, true, false) + require.NoError(t, err) + + // Verify remote was updated. + remoteSHA := remoteBranchSHA(t, bareDir, "b1") + assert.Equal(t, localSHA, remoteSHA) +} + +// Test 2: Branch exists remotely but tracking ref was deleted locally. +// This is the regression test for https://github.com/github/gh-stack/issues/118. +func TestIntegration_Push_TrackingRefDeletedLocally(t *testing.T) { + bareDir, cloneDir := setupBareAndClone(t) + restore := withGitDir(t, cloneDir) + defer restore() + + d := &defaultOps{} + + // Create a branch and push it. + gitExec(t, cloneDir, "checkout", "-b", "b1") + writeFile(t, cloneDir, "b1.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 initial") + gitExec(t, cloneDir, "push", "origin", "b1") + + // Delete the local tracking ref to simulate the bug condition. + gitExec(t, cloneDir, "branch", "-dr", "origin/b1") + + // Verify tracking ref is gone. + _, err := gitExecMayFail(t, cloneDir, "rev-parse", "--verify", "--quiet", "refs/remotes/origin/b1") + require.Error(t, err, "tracking ref should be deleted") + + // Make a local commit (simulating rebase). + writeFile(t, cloneDir, "b1.txt", "v2") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 rebased") + + localSHA := gitExec(t, cloneDir, "rev-parse", "b1") + + // FetchBranches should recreate the tracking ref. + err = d.FetchBranches("origin", []string{"b1"}) + require.NoError(t, err) + + // Verify tracking ref was recreated. + _, err = gitExecMayFail(t, cloneDir, "rev-parse", "--verify", "--quiet", "refs/remotes/origin/b1") + require.NoError(t, err, "tracking ref should be recreated by FetchBranches") + + // Push with force-with-lease should succeed. + err = d.Push("origin", []string{"b1"}, true, false) + require.NoError(t, err) + + // Verify remote was updated. + remoteSHA := remoteBranchSHA(t, bareDir, "b1") + assert.Equal(t, localSHA, remoteSHA) +} + +// Test 3: Branch advanced on remote by another client. +// Push should be rejected (lease protects the other commit). +func TestIntegration_Push_RemoteAdvancedByOther(t *testing.T) { + bareDir, cloneDir := setupBareAndClone(t) + restore := withGitDir(t, cloneDir) + defer restore() + + d := &defaultOps{} + + // Create a branch and push it. + gitExec(t, cloneDir, "checkout", "-b", "b1") + writeFile(t, cloneDir, "b1.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 initial") + gitExec(t, cloneDir, "push", "origin", "b1") + + // Simulate another client advancing the branch on the remote. + otherClone := filepath.Join(t.TempDir(), "other") + gitExec(t, ".", "clone", bareDir, otherClone) + gitExec(t, otherClone, "checkout", "b1") + writeFile(t, otherClone, "b1.txt", "v-other") + gitExec(t, otherClone, "add", ".") + gitExec(t, otherClone, "commit", "-m", "other update") + gitExec(t, otherClone, "push", "origin", "b1") + + // Record the other client's SHA on the remote. + otherSHA := remoteBranchSHA(t, bareDir, "b1") + + // Local client makes a different commit (simulating rebase). + writeFile(t, cloneDir, "b1.txt", "v-local") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 local update") + + // FetchBranches — this updates tracking ref to the other client's SHA. + err := d.FetchBranches("origin", []string{"b1"}) + require.NoError(t, err) + + // But wait: after fetch, our tracking ref now matches the remote. + // The push should succeed because the lease matches. To truly test + // the "someone pushed after our fetch" scenario, we need to advance + // the remote AFTER the fetch. + // + // Advance remote again after our fetch. + writeFile(t, otherClone, "b1.txt", "v-other-2") + gitExec(t, otherClone, "add", ".") + gitExec(t, otherClone, "commit", "-m", "other update 2") + gitExec(t, otherClone, "push", "origin", "b1", "--force") + + // Now remote is ahead of our tracking ref — push should fail. + err = d.Push("origin", []string{"b1"}, true, false) + require.Error(t, err, "push should be rejected when remote was advanced after fetch") + + // Confirm no overwrite: remote still has the other client's latest SHA. + finalRemoteSHA := remoteBranchSHA(t, bareDir, "b1") + assert.NotEqual(t, otherSHA, finalRemoteSHA, "remote should have advanced past original other SHA") +} + +// Test 4: Brand-new branch, absent on remote. +// Push should create the branch via empty-expect lease. +func TestIntegration_Push_NewBranchAbsentOnRemote(t *testing.T) { + bareDir, cloneDir := setupBareAndClone(t) + restore := withGitDir(t, cloneDir) + defer restore() + + d := &defaultOps{} + + // Create a new branch locally, do NOT push it. + gitExec(t, cloneDir, "checkout", "-b", "b1") + writeFile(t, cloneDir, "b1.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 new") + + localSHA := gitExec(t, cloneDir, "rev-parse", "b1") + + // FetchBranches — branch doesn't exist on remote, should tolerate the error. + err := d.FetchBranches("origin", []string{"b1"}) + require.NoError(t, err) + + // No tracking ref should exist (branch is absent remotely). + _, err = gitExecMayFail(t, cloneDir, "rev-parse", "--verify", "--quiet", "refs/remotes/origin/b1") + require.Error(t, err, "tracking ref should not exist for branch absent on remote") + + // Push should create the branch on remote. + err = d.Push("origin", []string{"b1"}, true, false) + require.NoError(t, err) + + // Verify remote has the branch. + remoteSHA := remoteBranchSHA(t, bareDir, "b1") + assert.Equal(t, localSHA, remoteSHA) +} + +// Test 5: Brand-new branch that another client created first (race). +// Push should be rejected because empty-expect means "must not exist." +func TestIntegration_Push_NewBranchRaceCondition(t *testing.T) { + bareDir, cloneDir := setupBareAndClone(t) + restore := withGitDir(t, cloneDir) + defer restore() + + d := &defaultOps{} + + // Create branch locally but don't push. + gitExec(t, cloneDir, "checkout", "-b", "b1") + writeFile(t, cloneDir, "b1.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 new") + + // FetchBranches — branch doesn't exist on remote yet. + err := d.FetchBranches("origin", []string{"b1"}) + require.NoError(t, err) + + // Another client creates the same branch on the remote. + otherClone := filepath.Join(t.TempDir(), "other") + gitExec(t, ".", "clone", bareDir, otherClone) + gitExec(t, otherClone, "checkout", "-b", "b1") + writeFile(t, otherClone, "b1.txt", "v-other") + gitExec(t, otherClone, "add", ".") + gitExec(t, otherClone, "commit", "-m", "other b1") + gitExec(t, otherClone, "push", "origin", "b1") + + // Now our push should fail because the branch exists on remote + // but we have an empty-expect lease. + err = d.Push("origin", []string{"b1"}, true, false) + require.Error(t, err, "push should be rejected when branch was created by another client") +} + +// Test 6: Mixed stack — one branch with current tracking ref + one with +// deleted tracking ref. Both should push successfully after FetchBranches fix. +func TestIntegration_Push_MixedStack(t *testing.T) { + bareDir, cloneDir := setupBareAndClone(t) + restore := withGitDir(t, cloneDir) + defer restore() + + d := &defaultOps{} + + // Create and push b1. + gitExec(t, cloneDir, "checkout", "-b", "b1") + writeFile(t, cloneDir, "b1.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 initial") + gitExec(t, cloneDir, "push", "origin", "b1") + + // Create and push b2. + gitExec(t, cloneDir, "checkout", "-b", "b2") + writeFile(t, cloneDir, "b2.txt", "v1") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b2 initial") + gitExec(t, cloneDir, "push", "origin", "b2") + + // Delete tracking ref for b2 only (simulating the bug for one branch). + gitExec(t, cloneDir, "branch", "-dr", "origin/b2") + + // Simulate rebase: update both branches. + gitExec(t, cloneDir, "checkout", "b1") + writeFile(t, cloneDir, "b1.txt", "v2") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b1 rebased") + + gitExec(t, cloneDir, "checkout", "b2") + writeFile(t, cloneDir, "b2.txt", "v2") + gitExec(t, cloneDir, "add", ".") + gitExec(t, cloneDir, "commit", "-m", "b2 rebased") + + localB1 := gitExec(t, cloneDir, "rev-parse", "b1") + localB2 := gitExec(t, cloneDir, "rev-parse", "b2") + + // FetchBranches should handle both: b1 has tracking ref, b2 does not. + err := d.FetchBranches("origin", []string{"b1", "b2"}) + require.NoError(t, err) + + // Push both branches with force-with-lease. + err = d.Push("origin", []string{"b1", "b2"}, true, false) + require.NoError(t, err) + + // Verify both were updated on remote. + assert.Equal(t, localB1, remoteBranchSHA(t, bareDir, "b1")) + assert.Equal(t, localB2, remoteBranchSHA(t, bareDir, "b2")) +} + func TestSplitCommitMessage(t *testing.T) { tests := []struct { name string