diff --git a/cmd/rebase.go b/cmd/rebase.go index 26059cc..0e4ee48 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -135,6 +135,9 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { } } + // Fast-forward stack branches that are behind their remote tracking branch. + fastForwardBranches(cfg, s, remote, currentBranch) + cfg.Printf("Stack detected: %s", s.DisplayChain()) currentIdx := s.IndexOf(currentBranch) diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 1b70db0..f72eef7 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "strings" "testing" "github.com/github/gh-stack/internal/config" @@ -34,7 +35,13 @@ func newRebaseMock(tmpDir string, currentBranch string) *git.MockOps { return &git.MockOps{ GitDirFn: func() (string, error) { return tmpDir, nil }, CurrentBranchFn: func() (string, error) { return currentBranch, nil }, - RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + RevParseFn: func(ref string) (string, error) { + // Default: origin/ returns same SHA as (no FF needed) + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + }, IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, FetchFn: func(string) error { return nil }, EnableRerereFn: func() error { return nil }, @@ -848,3 +855,188 @@ func TestRebase_Abort_WithActiveRebase(t *testing.T) { // Should return to original branch assert.Contains(t, checkouts, "b1", "should checkout original branch at end") } + +// TestRebase_FastForwardsBranchFromRemote verifies that when origin/b1 is ahead +// of local b1 (someone pushed a new commit), the branch is fast-forwarded before +// the cascade rebase so downstream branches include the new commits. +func TestRebase_FastForwardsBranchFromRemote(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var allRebaseCalls []rebaseCall + var updateBranchRefCalls []struct{ branch, sha string } + + mock := newRebaseMock(tmpDir, "b2") + // b1 is behind origin/b1 (remote has new commit) + mock.RevParseFn = func(ref string) (string, error) { + if ref == "b1" { + return "b1-local-sha", nil + } + if ref == "origin/b1" { + return "b1-remote-sha", nil + } + // trunk and origin/trunk same — trunk already up to date + if ref == "main" || ref == "origin/main" { + return "main-sha", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + // b1-local is ancestor of b1-remote → can fast-forward + if a == "b1-local-sha" && d == "b1-remote-sha" { + return true, nil + } + return false, nil + } + mock.UpdateBranchRefFn = func(branch, sha string) error { + updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string) error { + allRebaseCalls = append(allRebaseCalls, rebaseCall{newBase: base}) + return nil + } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + allRebaseCalls = append(allRebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + + // b1 should be fast-forwarded to remote SHA + require.Len(t, updateBranchRefCalls, 1, "should fast-forward b1 via UpdateBranchRef") + assert.Equal(t, "b1", updateBranchRefCalls[0].branch) + assert.Equal(t, "b1-remote-sha", updateBranchRefCalls[0].sha) + + assert.Contains(t, output, "Fast-forwarded b1") + + // Cascade rebase should still occur + assert.NotEmpty(t, allRebaseCalls, "cascade rebase should still happen") +} + +// TestRebase_BranchAlreadyUpToDate_NoFF verifies that when a branch's local +// and remote SHAs match, no fast-forward occurs. +func TestRebase_BranchAlreadyUpToDate_NoFF(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var updateBranchRefCalls int + var mergeFFCalls int + + mock := newRebaseMock(tmpDir, "b1") + // Same SHA for b1 and origin/b1 — already up to date (default mock handles this) + mock.UpdateBranchRefFn = func(string, string) error { + updateBranchRefCalls++ + return nil + } + mock.MergeFFFn = func(string) error { + mergeFFCalls++ + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Out.Close() + cfg.Err.Close() + + assert.NoError(t, err) + assert.Equal(t, 0, updateBranchRefCalls, "no UpdateBranchRef for branches already up to date") + assert.Equal(t, 0, mergeFFCalls, "no MergeFF for branches already up to date") +} + +// TestRebase_BranchDiverged_NoFF verifies that when local and remote branches +// have diverged (e.g., after a previous local rebase), no fast-forward occurs. +func TestRebase_BranchDiverged_NoFF(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var updateBranchRefCalls int + + mock := newRebaseMock(tmpDir, "b1") + // Different SHAs for b1 and origin/b1 + mock.RevParseFn = func(ref string) (string, error) { + if ref == "b1" { + return "b1-local-sha", nil + } + if ref == "origin/b1" { + return "b1-remote-sha", nil + } + if ref == "main" || ref == "origin/main" { + return "main-sha", nil + } + return "sha-" + ref, nil + } + // Neither is ancestor of the other — diverged + mock.IsAncestorFn = func(a, d string) (bool, error) { + return false, nil + } + mock.UpdateBranchRefFn = func(string, string) error { + updateBranchRefCalls++ + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Out.Close() + cfg.Err.Close() + + assert.NoError(t, err) + assert.Equal(t, 0, updateBranchRefCalls, "no FF when branches have diverged") +} diff --git a/cmd/sync.go b/cmd/sync.go index 019059a..c7035a6 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -116,9 +116,13 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } } - // --- Step 3: Cascade rebase (only if trunk moved) --- + // --- Step 2b: Fast-forward stack branches behind their remote tracking branch --- + updatedBranches := fastForwardBranches(cfg, s, remote, currentBranch) + branchesUpdated := len(updatedBranches) > 0 + + // --- Step 3: Cascade rebase (if trunk or any branch moved) --- rebased := false - if trunkUpdated { + if trunkUpdated || branchesUpdated { cfg.Printf("") cfg.Printf("Rebasing stack ...") diff --git a/cmd/sync_test.go b/cmd/sync_test.go index a842ae4..6704040 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -3,6 +3,7 @@ package cmd import ( "fmt" "io" + "strings" "testing" "github.com/github/gh-stack/internal/config" @@ -27,7 +28,13 @@ func newSyncMock(tmpDir string, currentBranch string) *git.MockOps { return &git.MockOps{ GitDirFn: func() (string, error) { return tmpDir, nil }, CurrentBranchFn: func() (string, error) { return currentBranch, nil }, - RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + RevParseFn: func(ref string) (string, error) { + // Default: origin/ returns same SHA as (no FF needed) + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + }, IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, FetchFn: func(string) error { return nil }, EnableRerereFn: func() error { return nil }, @@ -59,6 +66,9 @@ func TestSync_TrunkAlreadyUpToDate(t *testing.T) { if ref == "main" || ref == "origin/main" { return "aaa111aaa111", nil } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } return "sha-" + ref, nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { @@ -123,6 +133,10 @@ func TestSync_TrunkFastForward_TriggersRebase(t *testing.T) { if ref == "origin/main" { return "remote-sha", nil } + // Default: origin/ same as — no branch FF + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { @@ -647,3 +661,185 @@ func TestSync_PushFailureAfterRebase(t *testing.T) { assert.True(t, pushCalls[0].force, "push after rebase should use force") assert.Contains(t, output, "Push failed") } + +// TestSync_BranchFastForward_TriggersRebase verifies that when trunk hasn't +// moved but a stack branch has new remote commits, the branch is fast-forwarded, +// downstream branches are cascade-rebased, and force push is used. +func TestSync_BranchFastForward_TriggersRebase(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + var pushCalls []pushCall + var mergeFFCalls []string + + mock := newSyncMock(tmpDir, "b1") + // Trunk is up to date (same SHA), but b1 is behind origin/b1 + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" || ref == "origin/main" { + return "trunk-sha", nil + } + if ref == "b1" { + return "b1-local-sha", nil + } + if ref == "origin/b1" { + return "b1-remote-sha", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + if a == "b1-local-sha" && d == "b1-remote-sha" { + return true, nil + } + return false, nil + } + mock.MergeFFFn = func(target string) error { + mergeFFCalls = append(mergeFFCalls, target) + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string) error { + rebaseCalls = append(rebaseCalls, rebaseCall{branch: "(rebase)" + base}) + return nil + } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + + // b1 should be fast-forwarded via MergeFF (since we're on b1) + require.Len(t, mergeFFCalls, 1, "should fast-forward b1 via MergeFF") + assert.Equal(t, "origin/b1", mergeFFCalls[0]) + assert.Contains(t, output, "Fast-forwarded b1") + + // Cascade rebase should be triggered (even though trunk didn't move) + assert.NotEmpty(t, rebaseCalls, "rebase should occur when branch was fast-forwarded") + + // Push should use force-with-lease after rebase + require.Len(t, pushCalls, 1) + assert.True(t, pushCalls[0].force, "push should use force when rebase occurred after branch FF") +} + +// TestSync_BranchFastForward_WithTrunkUpdate verifies that when both trunk +// and a stack branch have remote updates, both are handled correctly. +func TestSync_BranchFastForward_WithTrunkUpdate(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var updateBranchRefCalls []struct{ branch, sha string } + var rebaseCalls []rebaseCall + var pushCalls []pushCall + + mock := newSyncMock(tmpDir, "b1") + // Trunk and b2 both behind remote + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" { + return "trunk-local", nil + } + if ref == "origin/main" { + return "trunk-remote", nil + } + if ref == "b2" { + return "b2-local", nil + } + if ref == "origin/b2" { + return "b2-remote", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + if a == "trunk-local" && d == "trunk-remote" { + return true, nil + } + if a == "b2-local" && d == "b2-remote" { + return true, nil + } + return false, nil + } + mock.UpdateBranchRefFn = func(branch, sha string) error { + updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string) error { + rebaseCalls = append(rebaseCalls, rebaseCall{branch: "(rebase)" + base}) + return nil + } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + + // Both trunk and b2 should be updated + branchUpdates := make(map[string]string) + for _, c := range updateBranchRefCalls { + branchUpdates[c.branch] = c.sha + } + assert.Equal(t, "trunk-remote", branchUpdates["main"], "trunk should be fast-forwarded") + assert.Equal(t, "b2-remote", branchUpdates["b2"], "b2 should be fast-forwarded") + + assert.Contains(t, output, "fast-forwarded") + assert.NotEmpty(t, rebaseCalls, "rebase should occur") + require.Len(t, pushCalls, 1) + assert.True(t, pushCalls[0].force, "push should use force after rebase") +} diff --git a/cmd/utils.go b/cmd/utils.go index f6cf0cc..f167201 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -317,6 +317,55 @@ func activeBranchNames(s *stack.Stack) []string { return names } +// fastForwardBranches fast-forwards each active stack branch to its remote +// tracking branch when the local branch is strictly behind. Returns the names +// of branches that were updated. Branches that are up-to-date, diverged, or +// have no remote tracking branch are silently skipped. +func fastForwardBranches(cfg *config.Config, s *stack.Stack, remote, currentBranch string) []string { + var updated []string + for _, br := range s.Branches { + if br.IsSkipped() { + continue + } + + remoteRef := remote + "/" + br.Branch + refs, err := git.RevParseMulti([]string{br.Branch, remoteRef}) + if err != nil { + // Remote tracking branch doesn't exist — skip. + continue + } + localSHA, remoteSHA := refs[0], refs[1] + + if localSHA == remoteSHA { + continue + } + + isAncestor, err := git.IsAncestor(localSHA, remoteSHA) + if err != nil || !isAncestor { + // Diverged or error — skip. This commonly happens after a + // local rebase and is handled by the push step. + continue + } + + // Local is behind remote — fast-forward. + if currentBranch == br.Branch { + if err := git.MergeFF(remoteRef); err != nil { + cfg.Warningf("Failed to fast-forward %s from remote: %v", br.Branch, err) + continue + } + } else { + if err := git.UpdateBranchRef(br.Branch, remoteSHA); err != nil { + cfg.Warningf("Failed to fast-forward %s from remote: %v", br.Branch, err) + continue + } + } + + cfg.Successf("Fast-forwarded %s to %s", br.Branch, short(remoteSHA)) + updated = append(updated, br.Branch) + } + return updated +} + // resolvePR resolves a user-provided target to a stack and branch using // waterfall logic: PR URL → PR number → branch name. func resolvePR(cfg *config.Config, sf *stack.StackFile, target string) (*stack.Stack, *stack.BranchRef, error) {