diff --git a/cmd/rebase.go b/cmd/rebase.go index caa3c32..a97199d 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -188,10 +188,34 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return ErrSilent } + // Backfill originalRefs for merged branches that were deleted locally. + // The rebase loop uses originalRefs[br.Branch] as ontoOldBase; without + // a valid entry the subsequent --onto rebase would receive an empty ref. + for _, b := range s.Branches { + if b.IsMerged() && !git.BranchExists(b.Branch) { + if b.Head != "" { + originalRefs[b.Branch] = b.Head + } + } + } + // Track --onto rebase state for merged branches. needsOnto := false var ontoOldBase string + // Get --onto state from merged branches below the rebase range. + // Ensures that when --upstack excludes merged branches, we still check + // the immediate predecessor for a merged PR and use --onto if needed. + if startIdx > 0 { + prev := s.Branches[startIdx-1] + if prev.IsMerged() { + if sha, ok := originalRefs[prev.Branch]; ok { + needsOnto = true + ontoOldBase = sha + } + } + } + for i, br := range branchesToRebase { var base string absIdx := startIdx + i @@ -221,7 +245,18 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { } } - if err := git.RebaseOnto(newBase, ontoOldBase, br.Branch); err != nil { + // If ontoOldBase is stale (not an ancestor of the branch), the + // branch was already rebased past it (e.g. by a previous run). + // Fall back to merge-base(newBase, branch) which gives the correct + // divergence point and avoids replaying already-applied commits. + actualOldBase := ontoOldBase + if isAnc, err := git.IsAncestor(ontoOldBase, br.Branch); err == nil && !isAnc { + if mb, err := git.MergeBase(newBase, br.Branch); err == nil { + actualOldBase = mb + } + } + + if err := git.RebaseOnto(newBase, actualOldBase, br.Branch); err != nil { cfg.Warningf("Rebasing %s onto %s — conflict", br.Branch, newBase) remaining := make([]string, 0) diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index cd77a90..6cca55b 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -240,6 +240,85 @@ func TestRebase_OntoPropagatesToSubsequentBranches(t *testing.T) { "b4 should rebase --onto b3 with b3's original SHA as oldBase") } +// TestRebase_StaleOntoOldBase_FallsBackToMergeBase verifies that when a branch +// was already rebased past the merged branch's tip (e.g. by a previous run), +// the stale ontoOldBase is detected via IsAncestor and replaced with +// merge-base(newBase, branch) to avoid replaying already-applied commits. +func TestRebase_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + // b1's local ref is the stale pre-squash tip from before a previous rebase. + // b2 was already rebased onto main by a previous run, so b1's old tip + // is NOT an ancestor of b2. + branchSHAs := map[string]string{ + "main": "main-sha", + "b1": "b1-stale-presquash-sha", + "b2": "b2-on-main-sha", + "b3": "b3-on-b2-sha", + } + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RevParseFn = func(ref string) (string, error) { + if sha, ok := branchSHAs[ref]; ok { + return sha, nil + } + return "default-sha", nil + } + mock.IsAncestorFn = func(ancestor, descendant string) (bool, error) { + // b1's stale SHA is NOT an ancestor of b2 (b2 was already rebased onto main) + if ancestor == "b1-stale-presquash-sha" { + return false, nil + } + return true, nil + } + mock.MergeBaseFn = func(a, b string) (string, error) { + if a == "main" && b == "b2" { + return "main-b2-mergebase", nil + } + return "default-mergebase", nil + } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + 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) + require.Len(t, rebaseCalls, 2) + + // b2: stale ontoOldBase detected → falls back to merge-base(main, b2) + assert.Equal(t, rebaseCall{"main", "main-b2-mergebase", "b2"}, rebaseCalls[0], + "b2 should use merge-base as oldBase when ontoOldBase is stale") + + // b3: b2's SHA is a valid ancestor → uses it directly + assert.Equal(t, rebaseCall{"b2", "b2-on-main-sha", "b3"}, rebaseCalls[1], + "b3 should use b2's original SHA as oldBase (not stale)") +} + // TestRebase_ConflictSavesState verifies that when a rebase conflict occurs, // the state is saved with the conflict branch and remaining branches. func TestRebase_ConflictSavesState(t *testing.T) { @@ -495,6 +574,67 @@ func TestRebase_UpstackOnly(t *testing.T) { assert.Equal(t, "b2", allRebaseCalls[1].newBase, "b3 should be rebased onto b2") } +// TestRebase_UpstackWithMergedBranchBelow verifies that --upstack pre-seeds +// --onto state when a merged branch exists immediately below the rebase range. +func TestRebase_UpstackWithMergedBranchBelow(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var allRebaseCalls []rebaseCall + var currentCheckedOut string + + mock := newRebaseMock(tmpDir, "b2") + mock.CheckoutBranchFn = func(name string) error { + currentCheckedOut = name + return nil + } + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseFn = func(base string) error { + allRebaseCalls = append(allRebaseCalls, rebaseCall{newBase: base, oldBase: "", branch: currentCheckedOut}) + 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, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--upstack"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Out.Close() + cfg.Err.Close() + + assert.NoError(t, err) + // b2 is at index 1, upstack = [b2, b3]. b1 is merged below. + // b2 should use --onto because b1 was merged. + require.Len(t, allRebaseCalls, 2, "upstack should rebase b2 and b3") + + // b2: --onto rebase with b1's old SHA as old base + assert.Equal(t, "main", allRebaseCalls[0].newBase, "b2 should be rebased onto main (first non-merged ancestor)") + assert.Equal(t, "sha-b1", allRebaseCalls[0].oldBase, "b2 should use b1's original SHA as old base") + assert.Equal(t, "b2", allRebaseCalls[0].branch, "b2 should be the branch being rebased") + + // b3: --onto continues to propagate + assert.Equal(t, "b2", allRebaseCalls[1].newBase, "b3 should be rebased onto b2") + assert.NotEmpty(t, allRebaseCalls[1].oldBase, "b3 should also use --onto") +} + // TestRebase_SkipsMergedBranches verifies that merged branches are skipped // with an appropriate message. func TestRebase_SkipsMergedBranches(t *testing.T) { @@ -1044,11 +1184,12 @@ func TestRebase_BranchDiverged_NoFF(t *testing.T) { func TestRebase_SkipsMergedBranchesNotExistingLocally(t *testing.T) { // Simulates a stack where b1 is merged and its branch was auto-deleted - // from the remote, so it doesn't exist locally. + // from the remote, so it doesn't exist locally. The stored Head SHA is + // used as ontoOldBase for the next branch's --onto rebase. s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 42, Merged: true}}, + {Branch: "b1", Head: "b1-stored-head-sha", PullRequest: &stack.PullRequestRef{Number: 42, Merged: true}}, {Branch: "b2"}, }, } @@ -1095,7 +1236,10 @@ func TestRebase_SkipsMergedBranchesNotExistingLocally(t *testing.T) { assert.NoError(t, err) assert.Contains(t, output, "Skipping b1") - // Only b2 should be rebased + // Only b2 should be rebased, and the rebase should use b1's stored + // Head SHA as oldBase so `git rebase --onto` receives valid arguments. require.Len(t, rebaseCalls, 1) assert.Equal(t, "b2", rebaseCalls[0].branch) + assert.Equal(t, "main", rebaseCalls[0].newBase) + assert.Equal(t, "b1-stored-head-sha", rebaseCalls[0].oldBase) } diff --git a/cmd/sync.go b/cmd/sync.go index a405817..2a5471c 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -142,6 +142,20 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } originalRefs, _ := git.RevParseMap(branchNames) + // Backfill originalRefs for merged branches that were deleted locally. + // The rebase loop uses originalRefs[br.Branch] as ontoOldBase; without + // a valid entry the subsequent --onto rebase would receive an empty ref. + for _, b := range s.Branches { + if b.IsMerged() && !git.BranchExists(b.Branch) { + if b.Head != "" { + if originalRefs == nil { + originalRefs = make(map[string]string) + } + originalRefs[b.Branch] = b.Head + } + } + } + needsOnto := false var ontoOldBase string @@ -181,7 +195,18 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } } - if err := git.RebaseOnto(newBase, ontoOldBase, br.Branch); err != nil { + // If ontoOldBase is stale (not an ancestor of the branch), the + // branch was already rebased past it (e.g. by a previous run). + // Fall back to merge-base(newBase, branch) to avoid replaying + // already-applied commits. + actualOldBase := ontoOldBase + if isAnc, err := git.IsAncestor(ontoOldBase, br.Branch); err == nil && !isAnc { + if mb, err := git.MergeBase(newBase, br.Branch); err == nil { + actualOldBase = mb + } + } + + if err := git.RebaseOnto(newBase, actualOldBase, br.Branch); err != nil { // Conflict detected — abort and restore everything if git.IsRebaseInProgress() { _ = git.RebaseAbort() diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 78a58b0..e3dc532 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -564,7 +564,12 @@ func TestSync_MergedBranch_UsesOnto(t *testing.T) { return "default-sha", nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil + // Trunk: local is behind remote → triggers fast-forward + if a == "local-sha" && d == "remote-sha" { + return true, nil + } + // For --onto stale-check: old bases are valid ancestors (first-run) + return true, nil } mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(string) error { return nil } @@ -603,6 +608,93 @@ func TestSync_MergedBranch_UsesOnto(t *testing.T) { assert.True(t, pushCalls[0].force) } +// TestSync_StaleOntoOldBase_FallsBackToMergeBase verifies that when a branch +// was already rebased past the merged branch's tip, sync detects the stale +// ontoOldBase and falls back to merge-base for the correct divergence point. +func TestSync_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseOntoCalls []rebaseCall + + branchSHAs := map[string]string{ + "b1": "b1-stale-presquash-sha", + "b2": "b2-on-main-sha", + "b3": "b3-on-b2-sha", + } + + mock := newSyncMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" { + return "local-sha", nil + } + if ref == "origin/main" { + return "remote-sha", nil + } + if sha, ok := branchSHAs[ref]; ok { + return sha, nil + } + return "default-sha", nil + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + // Trunk: local is behind remote + if a == "local-sha" && d == "remote-sha" { + return true, nil + } + // b1's stale SHA is NOT an ancestor of b2 (already rebased) + if a == "b1-stale-presquash-sha" { + return false, nil + } + return true, nil + } + mock.MergeBaseFn = func(a, b string) (string, error) { + if a == "main" && b == "b2" { + return "main-b2-mergebase", nil + } + return "default-mergebase", nil + } + mock.UpdateBranchRefFn = func(string, string) error { return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + rebaseOntoCalls = append(rebaseOntoCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.PushFn = func(string, []string, bool, bool) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Out.Close() + cfg.Err.Close() + + assert.NoError(t, err) + require.Len(t, rebaseOntoCalls, 2) + + // b2: stale ontoOldBase → falls back to merge-base(main, b2) + assert.Equal(t, rebaseCall{"main", "main-b2-mergebase", "b2"}, rebaseOntoCalls[0], + "b2 should use merge-base as oldBase when ontoOldBase is stale") + + // b3: b2's SHA is a valid ancestor → uses it directly + assert.Equal(t, rebaseCall{"b2", "b2-on-main-sha", "b3"}, rebaseOntoCalls[1], + "b3 should use b2's original SHA as oldBase") +} + // TestSync_PushFailureAfterRebase verifies that when push fails after a // successful rebase, the command does not return a fatal error — only a // warning is printed about the push failure. @@ -848,7 +940,7 @@ func TestSync_MergedBranchDeletedFromRemote(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b1", Head: "b1-stored-head-sha", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, {Branch: "b2"}, }, } @@ -890,7 +982,12 @@ func TestSync_MergedBranchDeletedFromRemote(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil + // Trunk FF check + if a == "local-sha" && d == "remote-sha" { + return true, nil + } + // For --onto stale-check: old bases are valid ancestors (first-run) + return true, nil } mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(string) error { return nil } @@ -915,7 +1012,10 @@ func TestSync_MergedBranchDeletedFromRemote(t *testing.T) { assert.NoError(t, err) assert.Contains(t, output, "Skipping b1") - // Only b2 should be rebased + // Only b2 should be rebased, and the rebase should use b1's stored + // Head SHA as oldBase so `git rebase --onto` receives valid arguments. require.Len(t, rebaseOntoCalls, 1) assert.Equal(t, "b2", rebaseOntoCalls[0].branch) + assert.Equal(t, "main", rebaseOntoCalls[0].newBase) + assert.Equal(t, "b1-stored-head-sha", rebaseOntoCalls[0].oldBase) }