From cedf847c8d84c59c3258638ef120fc344ada9db2 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 15 May 2026 15:15:25 -0400 Subject: [PATCH] fix: sync performs cascade rebase even when trunk is already up-to-date Previously, `gh stack sync` gated the cascade rebase on whether trunk or stack branches were fast-forwarded during the current run. This meant that if the user had already updated trunk locally (e.g., `git pull`), sync would skip the rebase entirely even though stack branches hadn't been rebased onto the current trunk. This change: - Adds `stackNeedsRebase()` to detect stale branches regardless of whether trunk was updated in this run - Extracts shared helpers (`fastForwardTrunk`, `cascadeRebase`, `resolveOriginalRefs`) from duplicated code in sync.go and rebase.go into utils.go, reducing ~450 lines of duplication - Fixes rebase.go to skip queued branches (was only skipping merged), consistent with sync's behavior via `IsSkipped()` - Refactors rebase --continue to reuse the shared cascade helper Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/rebase.go | 352 +++++++++++----------------------------------- cmd/sync.go | 202 ++++---------------------- cmd/sync_test.go | 93 +++++++++++- cmd/utils.go | 244 ++++++++++++++++++++++++++++++++ cmd/utils_test.go | 63 +++++++++ 5 files changed, 506 insertions(+), 448 deletions(-) diff --git a/cmd/rebase.go b/cmd/rebase.go index b6bcb9e..13415e8 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -127,33 +127,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { } // Fast-forward trunk so the cascade rebase targets the latest upstream. - trunk := s.Trunk.Branch - localSHA, remoteSHA := "", "" - trunkRefs, trunkErr := git.RevParseMulti([]string{trunk, remote + "/" + trunk}) - if trunkErr == nil { - localSHA, remoteSHA = trunkRefs[0], trunkRefs[1] - } - - if trunkErr == nil && localSHA != remoteSHA { - isAncestor, err := git.IsAncestor(localSHA, remoteSHA) - if err != nil { - cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, err) - } else if !isAncestor { - cfg.Warningf("Trunk %s has diverged from %s — skipping trunk update", trunk, remote) - } else if currentBranch == trunk { - if err := git.MergeFF(remote + "/" + trunk); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - } else { - cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - } - } else { - if err := updateBranchRef(trunk, remoteSHA); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - } else { - cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - } - } - } + fastForwardTrunk(cfg, s.Trunk.Branch, remote, currentBranch) // Fast-forward stack branches that are behind their remote tracking branch. fastForwardBranches(cfg, s, remote, currentBranch) @@ -192,43 +166,16 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { // Sync PR state before rebase so we can detect merged PRs. _ = syncStackPRs(cfg, s) - branchNames := make([]string, 0, len(s.Branches)) - for _, b := range s.Branches { - // Merged branches that no longer exist locally have no ref to - // resolve. They are always skipped during rebase, but we must - // also exclude them here to avoid a rev-parse error. - if b.IsMerged() && !git.BranchExists(b.Branch) { - continue - } - branchNames = append(branchNames, b.Branch) - } - originalRefs, err := git.RevParseMap(branchNames) - if err != nil { - cfg.Errorf("failed to resolve branch SHAs: %s", err) - 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 - } - } - } + originalRefs := resolveOriginalRefs(s) - // Track --onto rebase state for merged branches. + // Get --onto state from merged/queued branches below the rebase range. + // Ensures that when --upstack excludes skipped branches, we still check + // the immediate predecessor and use --onto if needed. 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 prev.IsSkipped() { if sha, ok := originalRefs[prev.Branch]; ok { needsOnto = true ontoOldBase = sha @@ -236,127 +183,40 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { } } - for i, br := range branchesToRebase { - var base string - absIdx := startIdx + i - if absIdx == 0 { - base = s.Trunk.Branch - } else { - base = s.Branches[absIdx-1].Branch + rebaseResult := cascadeRebase(cascadeRebaseOpts{ + Cfg: cfg, + Stack: s, + Branches: branchesToRebase, + StartAbsIdx: startIdx, + OriginalRefs: originalRefs, + NeedsOnto: needsOnto, + OntoOldBase: ontoOldBase, + }) + + if rebaseResult.Conflicted { + cfg.Warningf("Rebasing %s onto %s — conflict", rebaseResult.ConflictBranch, rebaseResult.ConflictBase) + + state := &rebaseState{ + CurrentBranchIndex: rebaseResult.ConflictIdx, + ConflictBranch: rebaseResult.ConflictBranch, + RemainingBranches: rebaseResult.Remaining, + OriginalBranch: currentBranch, + OriginalRefs: originalRefs, + UseOnto: rebaseResult.NeedsOnto, + OntoOldBase: rebaseResult.OntoOldBase, } - - // Skip branches whose PRs have already been merged. - // Record state so subsequent branches can use --onto rebase. - if br.IsMerged() { - ontoOldBase = originalRefs[br.Branch] - needsOnto = true - cfg.Successf("Skipping %s (PR %s merged)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) - continue + if err := saveRebaseState(gitDir, state); err != nil { + cfg.Warningf("failed to save rebase state: %s", err) } - if needsOnto { - // Find the proper --onto target: the first non-merged ancestor, or trunk. - newBase := s.Trunk.Branch - for j := absIdx - 1; j >= 0; j-- { - b := s.Branches[j] - if !b.IsMerged() { - newBase = b.Branch - break - } - } - - // 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) - for j := i + 1; j < len(branchesToRebase); j++ { - remaining = append(remaining, branchesToRebase[j].Branch) - } - - state := &rebaseState{ - CurrentBranchIndex: absIdx, - ConflictBranch: br.Branch, - RemainingBranches: remaining, - OriginalBranch: currentBranch, - OriginalRefs: originalRefs, - UseOnto: true, - OntoOldBase: originalRefs[br.Branch], - } - if err := saveRebaseState(gitDir, state); err != nil { - cfg.Warningf("failed to save rebase state: %s", err) - } - - printConflictDetails(cfg, newBase) - cfg.Printf("") - - cfg.Printf("Resolve conflicts on %s, then run `%s`", - br.Branch, cfg.ColorCyan("gh stack rebase --continue")) - cfg.Printf("Or abort this operation with `%s`", - cfg.ColorCyan("gh stack rebase --abort")) - return ErrConflict - } - - cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", br.Branch, newBase) - // Keep --onto mode; update old base for the next branch. - ontoOldBase = originalRefs[br.Branch] - } else { - var rebaseErr error - if absIdx > 0 { - // Use --onto to replay only this branch's unique commits. - // Without --onto, git may try to replay commits shared with - // the parent, causing duplicate-patch conflicts when the - // parent's rebase rewrote those commits. - rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch) - } else { - if err := git.CheckoutBranch(br.Branch); err != nil { - return fmt.Errorf("checking out %s: %w", br.Branch, err) - } - // Use regular rebase for the first branch. - rebaseErr = git.Rebase(base) - } - - if rebaseErr != nil { - cfg.Warningf("Rebasing %s onto %s — conflict", br.Branch, base) - - remaining := make([]string, 0) - for j := i + 1; j < len(branchesToRebase); j++ { - remaining = append(remaining, branchesToRebase[j].Branch) - } - - state := &rebaseState{ - CurrentBranchIndex: absIdx, - ConflictBranch: br.Branch, - RemainingBranches: remaining, - OriginalBranch: currentBranch, - OriginalRefs: originalRefs, - } - if err := saveRebaseState(gitDir, state); err != nil { - cfg.Warningf("failed to save rebase state: %s", err) - } - - printConflictDetails(cfg, base) - cfg.Printf("") - - cfg.Printf("Resolve conflicts on %s, then run `%s`", - br.Branch, cfg.ColorCyan("gh stack rebase --continue")) - cfg.Printf("Or abort this operation with `%s`", - cfg.ColorCyan("gh stack rebase --abort")) - return ErrConflict - } + printConflictDetails(cfg, rebaseResult.ConflictBase) + cfg.Printf("") - cfg.Successf("Rebased %s onto %s", br.Branch, base) - } + cfg.Printf("Resolve conflicts on %s, then run `%s`", + rebaseResult.ConflictBranch, cfg.ColorCyan("gh stack rebase --continue")) + cfg.Printf("Or abort this operation with `%s`", + cfg.ColorCyan("gh stack rebase --abort")) + return ErrConflict } _ = git.CheckoutBranch(currentBranch) @@ -431,10 +291,10 @@ func continueRebase(cfg *config.Config, gitDir string) error { var baseBranch string if state.UseOnto { - // The --onto path targets the first non-merged ancestor, or trunk. + // The --onto path targets the first non-skipped ancestor, or trunk. baseBranch = s.Trunk.Branch for j := state.CurrentBranchIndex - 1; j >= 0; j-- { - if !s.Branches[j].IsMerged() { + if !s.Branches[j].IsSkipped() { baseBranch = s.Branches[j].Branch break } @@ -446,106 +306,52 @@ func continueRebase(cfg *config.Config, gitDir string) error { } cfg.Successf("Rebased %s onto %s", conflictBranch, baseBranch) - for _, branchName := range state.RemainingBranches { - idx := s.IndexOf(branchName) - if idx < 0 { - return fmt.Errorf("branch %q from saved rebase state is no longer in the stack — the stack may have been modified since the rebase started; consider aborting with --abort", branchName) - } - - // Skip branches whose PRs have already been merged. - br := s.Branches[idx] - if br.IsMerged() { - state.OntoOldBase = state.OriginalRefs[branchName] - state.UseOnto = true - cfg.Successf("Skipping %s (PR %s merged)", branchName, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) - continue - } - - var base string - if idx == 0 { - base = s.Trunk.Branch - } else { - base = s.Branches[idx-1].Branch - } - - if state.UseOnto { - // Find the proper --onto target: first non-merged ancestor, or trunk. - newBase := s.Trunk.Branch - for j := idx - 1; j >= 0; j-- { - b := s.Branches[j] - if !b.IsMerged() { - newBase = b.Branch - break - } + // Rebase remaining branches using the shared cascade helper. + if len(state.RemainingBranches) > 0 { + // Validate all remaining branches still exist in the stack and + // build the BranchRef slice with correct absolute indices. + remainingRefs := make([]stack.BranchRef, 0, len(state.RemainingBranches)) + startAbsIdx := -1 + for _, name := range state.RemainingBranches { + idx := s.IndexOf(name) + if idx < 0 { + return fmt.Errorf("branch %q from saved rebase state is no longer in the stack — the stack may have been modified since the rebase started; consider aborting with --abort", name) } - - if err := git.RebaseOnto(newBase, state.OntoOldBase, branchName); err != nil { - remainIdx := -1 - for ri, rb := range state.RemainingBranches { - if rb == branchName { - remainIdx = ri - break - } - } - state.RemainingBranches = state.RemainingBranches[remainIdx+1:] - state.CurrentBranchIndex = idx - state.ConflictBranch = branchName - state.OntoOldBase = state.OriginalRefs[branchName] - if err := saveRebaseState(gitDir, state); err != nil { - cfg.Warningf("failed to save rebase state: %s", err) - } - - cfg.Warningf("Rebasing %s onto %s — conflict", branchName, newBase) - printConflictDetails(cfg, newBase) - cfg.Printf("") - cfg.Printf("Resolve conflicts on %s, then run `%s`", - branchName, cfg.ColorCyan("gh stack rebase --continue")) - cfg.Printf("Or abort this operation with `%s`", - cfg.ColorCyan("gh stack rebase --abort")) - return ErrConflict - } - - cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", branchName, newBase) - state.OntoOldBase = state.OriginalRefs[branchName] - } else { - var rebaseErr error - if idx > 0 { - // Use --onto to replay only this branch's unique commits. - rebaseErr = git.RebaseOnto(base, state.OriginalRefs[base], branchName) - } else { - if err := git.CheckoutBranch(branchName); err != nil { - cfg.Errorf("checking out %s: %s", branchName, err) - return ErrSilent - } - rebaseErr = git.Rebase(base) + if startAbsIdx < 0 { + startAbsIdx = idx } + remainingRefs = append(remainingRefs, s.Branches[idx]) + } - if rebaseErr != nil { - remainIdx := -1 - for ri, rb := range state.RemainingBranches { - if rb == branchName { - remainIdx = ri - break - } - } - state.RemainingBranches = state.RemainingBranches[remainIdx+1:] - state.CurrentBranchIndex = idx - state.ConflictBranch = branchName - if err := saveRebaseState(gitDir, state); err != nil { - cfg.Warningf("failed to save rebase state: %s", err) - } - - cfg.Warningf("Rebasing %s onto %s — conflict", branchName, base) - printConflictDetails(cfg, base) - cfg.Printf("") - cfg.Printf("Resolve conflicts on %s, then run `%s`", - branchName, cfg.ColorCyan("gh stack rebase --continue")) - cfg.Printf("Or abort this operation with `%s`", - cfg.ColorCyan("gh stack rebase --abort")) - return ErrConflict + result := cascadeRebase(cascadeRebaseOpts{ + Cfg: cfg, + Stack: s, + Branches: remainingRefs, + StartAbsIdx: startAbsIdx, + OriginalRefs: state.OriginalRefs, + NeedsOnto: state.UseOnto, + OntoOldBase: state.OntoOldBase, + }) + + if result.Conflicted { + cfg.Warningf("Rebasing %s onto %s — conflict", result.ConflictBranch, result.ConflictBase) + + state.CurrentBranchIndex = result.ConflictIdx + state.ConflictBranch = result.ConflictBranch + state.RemainingBranches = result.Remaining + state.UseOnto = result.NeedsOnto + state.OntoOldBase = result.OntoOldBase + if err := saveRebaseState(gitDir, state); err != nil { + cfg.Warningf("failed to save rebase state: %s", err) } - cfg.Successf("Rebased %s onto %s", branchName, base) + printConflictDetails(cfg, result.ConflictBase) + cfg.Printf("") + cfg.Printf("Resolve conflicts on %s, then run `%s`", + result.ConflictBranch, cfg.ColorCyan("gh stack rebase --continue")) + cfg.Printf("Or abort this operation with `%s`", + cfg.ColorCyan("gh stack rebase --abort")) + return ErrConflict } } diff --git a/cmd/sync.go b/cmd/sync.go index 447f7be..32eaadc 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -92,196 +92,57 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // --- Step 2: Fast-forward trunk --- trunk := s.Trunk.Branch - trunkUpdated := false - - localSHA, remoteSHA := "", "" - trunkRefs, trunkErr := git.RevParseMulti([]string{trunk, remote + "/" + trunk}) - if trunkErr == nil { - localSHA, remoteSHA = trunkRefs[0], trunkRefs[1] - } - - if trunkErr != nil { - cfg.Warningf("Could not compare trunk %s with remote — skipping trunk update", trunk) - } else if localSHA == remoteSHA { - cfg.Successf("Trunk %s is already up to date", trunk) - } else { - isAncestor, err := git.IsAncestor(localSHA, remoteSHA) - if err != nil { - cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, err) - } else if !isAncestor { - cfg.Warningf("Trunk %s has diverged from %s — skipping trunk update", trunk, remote) - cfg.Printf(" Local and remote %s have diverged. Resolve manually.", trunk) - } else { - // Fast-forward the trunk branch - if currentBranch == trunk { - if err := git.MergeFF(remote + "/" + trunk); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - } else { - cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - trunkUpdated = true - } - } else { - if err := updateBranchRef(trunk, remoteSHA); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - } else { - cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - trunkUpdated = true - } - } - } - } + trunkUpdated := fastForwardTrunk(cfg, trunk, remote, currentBranch) // --- 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) --- + // --- Step 3: Cascade rebase --- + // Rebase if trunk or any branch moved, or if the stack is stale + // (branches not yet rebased onto their parent's current tip). + needsRebase := trunkUpdated || branchesUpdated || stackNeedsRebase(s) rebased := false - if trunkUpdated || branchesUpdated { + if needsRebase { cfg.Printf("") cfg.Printf("Rebasing stack ...") // Sync PR state to detect merged PRs before rebasing. _ = syncStackPRs(cfg, s) - // Save original refs so we can restore on conflict. - // Merged branches that no longer exist locally have no ref to - // resolve. They are always skipped during rebase but we must - // also exclude them here to avoid a rev-parse error. - branchNames := make([]string, 0, len(s.Branches)) - for _, b := range s.Branches { - if b.IsMerged() && !git.BranchExists(b.Branch) { - continue - } - branchNames = append(branchNames, b.Branch) - } - 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 - - conflicted := false - for i, br := range s.Branches { - var base string - if i == 0 { - base = trunk - } else { - base = s.Branches[i-1].Branch - } + originalRefs := resolveOriginalRefs(s) - // Skip branches whose PRs have already been merged. - if br.IsMerged() { - ontoOldBase = originalRefs[br.Branch] - needsOnto = true - cfg.Successf("Skipping %s (PR %s merged)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) - continue - } + result := cascadeRebase(cascadeRebaseOpts{ + Cfg: cfg, + Stack: s, + Branches: s.Branches, + StartAbsIdx: 0, + OriginalRefs: originalRefs, + }) - // Skip branches whose PRs are currently in a merge queue. - if br.IsQueued() { - ontoOldBase = originalRefs[br.Branch] - needsOnto = true - cfg.Successf("Skipping %s (PR %s queued)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) - continue + if result.Conflicted { + // Abort and restore everything — sync is non-interactive. + if git.IsRebaseInProgress() { + _ = git.RebaseAbort() } + restoreErrors := restoreBranches(originalRefs) + _ = git.CheckoutBranch(currentBranch) - if needsOnto { - // Find --onto target: first non-merged/queued ancestor, or trunk. - newBase := trunk - for j := i - 1; j >= 0; j-- { - b := s.Branches[j] - if !b.IsSkipped() { - newBase = b.Branch - break - } - } - - // 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() - } - restoreErrors := restoreBranches(originalRefs) - _ = git.CheckoutBranch(currentBranch) - - cfg.Errorf("Conflict detected rebasing %s onto %s", br.Branch, newBase) - reportRestoreStatus(cfg, restoreErrors) - cfg.Printf(" Run `%s` to resolve conflicts interactively.", - cfg.ColorCyan("gh stack rebase")) - conflicted = true - break - } - - cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", br.Branch, newBase) - ontoOldBase = originalRefs[br.Branch] - } else { - var rebaseErr error - if i > 0 { - // Use --onto to replay only this branch's unique commits. - rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch) - } else { - if err := git.CheckoutBranch(br.Branch); err != nil { - cfg.Errorf("Failed to checkout %s: %v", br.Branch, err) - conflicted = true - break - } - rebaseErr = git.Rebase(base) - } - - if rebaseErr != nil { - // Conflict detected — abort and restore everything - if git.IsRebaseInProgress() { - _ = git.RebaseAbort() - } - restoreErrors := restoreBranches(originalRefs) - _ = git.CheckoutBranch(currentBranch) - - cfg.Errorf("Conflict detected rebasing %s onto %s", br.Branch, base) - reportRestoreStatus(cfg, restoreErrors) - cfg.Printf(" Run `%s` to resolve conflicts interactively.", - cfg.ColorCyan("gh stack rebase")) - conflicted = true - break - } - - cfg.Successf("Rebased %s onto %s", br.Branch, base) - } - } + cfg.Errorf("Conflict detected rebasing %s onto %s", result.ConflictBranch, result.ConflictBase) + reportRestoreStatus(cfg, restoreErrors) + cfg.Printf(" Run `%s` to resolve conflicts interactively.", + cfg.ColorCyan("gh stack rebase")) - if !conflicted { - rebased = true - _ = git.CheckoutBranch(currentBranch) - } else { // Persist refreshed PR state even on conflict, then bail out // before pushing or reporting success. stack.SaveNonBlocking(gitDir, sf) return ErrConflict } + + if result.Rebased { + rebased = true + } + _ = git.CheckoutBranch(currentBranch) } // --- Step 4: Push --- @@ -449,11 +310,6 @@ func runSync(cfg *config.Config, opts *syncOptions) error { return nil } -// updateBranchRef updates a branch ref to point to a new SHA (for branches not checked out). -func updateBranchRef(branch, sha string) error { - return git.UpdateBranchRef(branch, sha) -} - // restoreBranches resets each branch to its original SHA, collecting any errors. func restoreBranches(originalRefs map[string]string) []string { var errors []string diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 6431ece..8bc0284 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -106,6 +106,83 @@ func TestSync_TrunkAlreadyUpToDate(t *testing.T) { assert.False(t, pushCalls[0].force, "push should not use force when no rebase occurred") } +// TestSync_TrunkUpToDate_StackStale verifies that when trunk is already up to +// date locally (no FF needed) but the stack branches haven't been rebased onto +// the current trunk, sync still performs the cascade rebase. This is the core +// bug fix — previously sync would skip the rebase entirely in this scenario. +func TestSync_TrunkUpToDate_StackStale(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 + + mock := newSyncMock(tmpDir, "b1") + // Trunk is already up to date — same SHA locally and remotely. + mock.RevParseFn = func(ref string) (string, error) { + 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 + } + // Stack branches are NOT rebased onto trunk — parent is not an ancestor. + mock.IsAncestorFn = func(a, d string) (bool, error) { + // main is NOT an ancestor of b1 → stack is stale + if a == "main" && d == "b1" { + return false, nil + } + return true, 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) + assert.Contains(t, output, "up to date") + + // Rebase SHOULD occur even though trunk was already up to date, + // because the stack branches are stale (not rebased onto trunk). + assert.NotEmpty(t, rebaseCalls, "rebase should occur when stack is stale even if trunk is up to date") + + // Push should use force since rebase occurred + require.Len(t, pushCalls, 1) + assert.True(t, pushCalls[0].force, "push should use force-with-lease after rebase") +} + // TestSync_TrunkFastForward_TriggersRebase verifies that when trunk is behind // origin/trunk, it fast-forwards and triggers a cascade rebase with force push. func TestSync_TrunkFastForward_TriggersRebase(t *testing.T) { @@ -277,11 +354,23 @@ func TestSync_TrunkDiverged(t *testing.T) { if ref == "origin/main" { return "remote-sha", nil } + // Stack branches: local and remote have same SHA (no branch FF needed) + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } return "sha-" + ref, nil } - // Neither is ancestor of the other → diverged + // Neither is ancestor of the other → diverged (for trunk FF check) + // But stack branches DO have local trunk as ancestor (for stackNeedsRebase) mock.IsAncestorFn = func(a, d string) (bool, error) { - return false, nil + if a == "local-sha" && d == "remote-sha" { + return false, nil + } + if a == "remote-sha" && d == "local-sha" { + return false, nil + } + // Stack branches have their parent as ancestor + return true, nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) diff --git a/cmd/utils.go b/cmd/utils.go index 7ec0d3f..05c3906 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -611,6 +611,250 @@ func fastForwardBranches(cfg *config.Config, s *stack.Stack, remote, currentBran return updated } +// resolveOriginalRefs builds a map from branch name to current SHA for all +// branches in the stack. Merged branches that no longer exist locally are +// backfilled from the stack metadata. This map is used as the "before" state +// for cascade rebases and conflict recovery. +func resolveOriginalRefs(s *stack.Stack) map[string]string { + branchNames := make([]string, 0, len(s.Branches)) + for _, b := range s.Branches { + if b.IsMerged() && !git.BranchExists(b.Branch) { + continue + } + branchNames = append(branchNames, b.Branch) + } + originalRefs, _ := git.RevParseMap(branchNames) + + // Backfill merged branches that were deleted locally. + 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 + } + } + } + return originalRefs +} + +// fastForwardTrunk fast-forwards the trunk branch to match its remote tracking +// branch. Returns true if trunk was updated. +func fastForwardTrunk(cfg *config.Config, trunk, remote, currentBranch string) bool { + localSHA, remoteSHA := "", "" + trunkRefs, trunkErr := git.RevParseMulti([]string{trunk, remote + "/" + trunk}) + if trunkErr == nil { + localSHA, remoteSHA = trunkRefs[0], trunkRefs[1] + } + + if trunkErr != nil { + cfg.Warningf("Could not compare trunk %s with remote — skipping trunk update", trunk) + return false + } + + if localSHA == remoteSHA { + cfg.Successf("Trunk %s is already up to date", trunk) + return false + } + + isAncestor, err := git.IsAncestor(localSHA, remoteSHA) + if err != nil { + cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, err) + return false + } + if !isAncestor { + cfg.Warningf("Trunk %s has diverged from %s — skipping trunk update", trunk, remote) + cfg.Printf(" Local and remote %s have diverged. Resolve manually.", trunk) + return false + } + + if currentBranch == trunk { + if err := git.MergeFF(remote + "/" + trunk); err != nil { + cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) + return false + } + } else { + if err := git.UpdateBranchRef(trunk, remoteSHA); err != nil { + cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) + return false + } + } + + cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) + return true +} + +// cascadeRebaseOpts holds parameters for a cascade rebase across a range of +// stack branches. +type cascadeRebaseOpts struct { + Cfg *config.Config + Stack *stack.Stack + Branches []stack.BranchRef // the range of branches to rebase + StartAbsIdx int // index of Branches[0] in Stack.Branches + OriginalRefs map[string]string + NeedsOnto bool + OntoOldBase string +} + +// cascadeRebaseResult describes the outcome of a cascade rebase. +type cascadeRebaseResult struct { + Rebased bool // at least one branch was successfully rebased + Conflicted bool // a conflict was detected + ConflictIdx int // absolute index in Stack.Branches of the conflicting branch + ConflictBranch string // name of the conflicting branch + ConflictBase string // base branch we were rebasing onto + Remaining []string // branch names after the conflict point + NeedsOnto bool // --onto state at the conflict point (for --continue) + OntoOldBase string // ontoOldBase at the conflict point (for --continue) +} + +// cascadeRebase performs a cascade rebase across the given branch range. It +// stops at the first conflict and returns a result describing what happened. +// The caller is responsible for conflict recovery (abort+restore or save state). +func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { + s := opts.Stack + cfg := opts.Cfg + needsOnto := opts.NeedsOnto + ontoOldBase := opts.OntoOldBase + originalRefs := opts.OriginalRefs + result := cascadeRebaseResult{} + + for i, br := range opts.Branches { + absIdx := opts.StartAbsIdx + i + + var base string + if absIdx == 0 { + base = s.Trunk.Branch + } else { + base = s.Branches[absIdx-1].Branch + } + + // Skip merged and queued branches. + if br.IsSkipped() { + ontoOldBase = originalRefs[br.Branch] + needsOnto = true + if br.IsMerged() { + cfg.Successf("Skipping %s (PR %s merged)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) + } else if br.IsQueued() { + cfg.Successf("Skipping %s (PR %s queued)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) + } + continue + } + + if needsOnto { + // Find --onto target: first non-skipped ancestor, or trunk. + newBase := s.Trunk.Branch + for j := absIdx - 1; j >= 0; j-- { + if !s.Branches[j].IsSkipped() { + newBase = s.Branches[j].Branch + break + } + } + + // If ontoOldBase is stale (not an ancestor of the branch), the + // branch was already rebased past it. 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 { + remaining := make([]string, 0, len(opts.Branches)-i-1) + for j := i + 1; j < len(opts.Branches); j++ { + remaining = append(remaining, opts.Branches[j].Branch) + } + return cascadeRebaseResult{ + Rebased: result.Rebased, + Conflicted: true, + ConflictIdx: absIdx, + ConflictBranch: br.Branch, + ConflictBase: newBase, + Remaining: remaining, + NeedsOnto: true, + OntoOldBase: originalRefs[br.Branch], + } + } + + cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", br.Branch, newBase) + result.Rebased = true + ontoOldBase = originalRefs[br.Branch] + } else { + var rebaseErr error + if absIdx > 0 { + rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch) + } else { + if err := git.CheckoutBranch(br.Branch); err != nil { + remaining := make([]string, 0, len(opts.Branches)-i-1) + for j := i + 1; j < len(opts.Branches); j++ { + remaining = append(remaining, opts.Branches[j].Branch) + } + return cascadeRebaseResult{ + Rebased: result.Rebased, + Conflicted: true, + ConflictIdx: absIdx, + ConflictBranch: br.Branch, + ConflictBase: base, + Remaining: remaining, + } + } + rebaseErr = git.Rebase(base) + } + + if rebaseErr != nil { + remaining := make([]string, 0, len(opts.Branches)-i-1) + for j := i + 1; j < len(opts.Branches); j++ { + remaining = append(remaining, opts.Branches[j].Branch) + } + return cascadeRebaseResult{ + Rebased: result.Rebased, + Conflicted: true, + ConflictIdx: absIdx, + ConflictBranch: br.Branch, + ConflictBase: base, + Remaining: remaining, + NeedsOnto: false, + OntoOldBase: originalRefs[br.Branch], + } + } + + cfg.Successf("Rebased %s onto %s", br.Branch, base) + result.Rebased = true + } + } + + return result +} + +// stackNeedsRebase returns true if any active branch in the stack is not based +// on its parent's current tip. This detects when the stack needs rebasing even +// if trunk was not updated in the current run. +func stackNeedsRebase(s *stack.Stack) bool { + trunk := s.Trunk.Branch + for i, br := range s.Branches { + if br.IsSkipped() { + continue + } + // Find the nearest non-skipped parent. + parent := trunk + for j := i - 1; j >= 0; j-- { + if !s.Branches[j].IsSkipped() { + parent = s.Branches[j].Branch + break + } + } + isAnc, err := git.IsAncestor(parent, br.Branch) + if err != nil || !isAnc { + return true + } + } + return false +} + // 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) { diff --git a/cmd/utils_test.go b/cmd/utils_test.go index 59d7a28..9068c69 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -625,3 +625,66 @@ func TestParsePRURL(t *testing.T) { }) } } + +func TestStackNeedsRebase_AllCurrent(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + mock := &git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + return true, nil + }, + } + restore := git.SetOps(mock) + defer restore() + + assert.False(t, stackNeedsRebase(s), "stack should not need rebase when all branches are current") +} + +func TestStackNeedsRebase_FirstBranchStale(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + mock := &git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + if a == "main" && d == "b1" { + return false, nil + } + return true, nil + }, + } + restore := git.SetOps(mock) + defer restore() + + assert.True(t, stackNeedsRebase(s), "stack should need rebase when first branch is stale") +} + +func TestStackNeedsRebase_SkipsMergedBranches(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Merged: true}}, + {Branch: "b2"}, + }, + } + + mock := &git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + return true, nil + }, + } + restore := git.SetOps(mock) + defer restore() + + assert.False(t, stackNeedsRebase(s), "should skip merged branches and find stack up to date") +}