diff --git a/README.md b/README.md index f2e6463..baa33d0 100644 --- a/README.md +++ b/README.md @@ -197,7 +197,7 @@ Pull from remote and do a cascading rebase across the stack. gh stack rebase [flags] [branch] ``` -Fetches the latest changes from `origin`, then ensures each branch in the stack has the tip of the previous layer in its commit history. Rebases branches in order from trunk upward. If a branch's PR has been squash-merged, the rebase automatically switches to `--onto` mode to correctly replay commits on top of the merge target. +Fetches the latest changes from `origin`, then ensures each branch in the stack has the tip of the previous layer in its commit history. Rebases branches in order from trunk upward. If a branch's PR has been merged, the rebase automatically switches to `--onto` mode to correctly replay commits on top of the merge target. If a rebase conflict occurs, the operation pauses and prints the conflicted files with line numbers. Resolve the conflicts, stage with `git add`, and continue with `--continue`. To undo the entire rebase, use `--abort` to restore all branches to their pre-rebase state. diff --git a/cmd/rebase.go b/cmd/rebase.go index 26059cc..a97199d 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) @@ -169,9 +172,15 @@ 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, len(s.Branches)) - for i, b := range s.Branches { - branchNames[i] = b.Branch + 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 { @@ -179,10 +188,34 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return ErrSilent } - // Track --onto rebase state for squash-merged branches. + // 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 @@ -192,7 +225,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { base = s.Branches[absIdx-1].Branch } - // Skip branches whose PRs have already been merged (e.g. via squash). + // Skip branches whose PRs have already been merged. // Record state so subsequent branches can use --onto rebase. if br.IsMerged() { ontoOldBase = originalRefs[br.Branch] @@ -212,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) @@ -243,7 +287,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return ErrConflict } - cfg.Successf("Rebased %s onto %s (squash-merge detected)", br.Branch, newBase) + 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 { @@ -441,7 +485,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { return ErrConflict } - cfg.Successf("Rebased %s onto %s (squash-merge detected)", branchName, newBase) + cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", branchName, newBase) state.OntoOldBase = state.OriginalRefs[branchName] } else { var rebaseErr error diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 1b70db0..6cca55b 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 }, @@ -99,10 +106,10 @@ func TestRebase_CascadeRebase(t *testing.T) { assert.Contains(t, output, "rebased locally") } -// TestRebase_SquashMergedBranch_UsesOnto verifies that when b1 has a merged PR, +// TestRebase_MergedBranch_UsesOnto verifies that when b1 has a merged PR, // it is skipped and b2 uses RebaseOnto with trunk as newBase and b1's original // SHA as oldBase. b3 also uses --onto (propagation). -func TestRebase_SquashMergedBranch_UsesOnto(t *testing.T) { +func TestRebase_MergedBranch_UsesOnto(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -126,6 +133,7 @@ func TestRebase_SquashMergedBranch_UsesOnto(t *testing.T) { } 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 @@ -163,7 +171,7 @@ func TestRebase_SquashMergedBranch_UsesOnto(t *testing.T) { } // TestRebase_OntoPropagatesToSubsequentBranches verifies that when multiple -// branches are squash-merged, --onto propagates correctly through the chain. +// branches are merged, --onto propagates correctly through the chain. func TestRebase_OntoPropagatesToSubsequentBranches(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, @@ -190,6 +198,7 @@ func TestRebase_OntoPropagatesToSubsequentBranches(t *testing.T) { } mock := newRebaseMock(tmpDir, "b3") + mock.BranchExistsFn = func(name string) bool { return true } mock.RevParseFn = func(ref string) (string, error) { if sha, ok := branchSHAs[ref]; ok { return sha, nil @@ -231,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) { @@ -486,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) { @@ -642,7 +791,7 @@ func TestRebase_Continue_RebasesRemainingBranches(t *testing.T) { } // TestRebase_Continue_OntoMode verifies the --continue path when UseOnto is -// set (squash-merged branches upstream). With no remaining branches, only +// set (merged branches upstream). With no remaining branches, only // RebaseContinue runs and the state is cleaned up. func TestRebase_Continue_OntoMode(t *testing.T) { s := stack.Stack{ @@ -848,3 +997,249 @@ 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") +} + +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. 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", Head: "b1-stored-head-sha", PullRequest: &stack.PullRequestRef{Number: 42, Merged: true}}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { + // b1 does not exist locally (deleted from remote after merge) + return name != "b1" + } + mock.RevParseMultiFn = func(refs []string) ([]string, error) { + // Only resolve refs that exist — b1 should not be in the list + shas := make([]string, len(refs)) + for i, r := range refs { + if r == "b1" { + t.Fatalf("RevParseMulti should not be called with non-existent branch b1") + } + shas[i] = "sha-" + r + } + return shas, 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, _, 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) + assert.Contains(t, output, "Skipping b1") + + // 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/submit.go b/cmd/submit.go index 483015a..f2f2242 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -77,6 +77,32 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { return ErrAPIFailure } + // Verify that the repository has stacked PRs enabled. + stacksAvailable := s.ID != "" + if s.ID == "" { + if _, err := client.ListStacks(); err != nil { + cfg.Warningf("Stacked PRs are not enabled for this repository") + if cfg.IsInteractive() { + p := prompter.New(cfg.In, cfg.Out, cfg.Err) + proceed, promptErr := p.Confirm("Would you still like to create regular PRs?", false) + if promptErr != nil { + if isInterruptError(promptErr) { + printInterrupt(cfg) + return ErrSilent + } + return ErrStacksUnavailable + } + if !proceed { + return ErrStacksUnavailable + } + } else { + return ErrStacksUnavailable + } + } else { + stacksAvailable = true + } + } + // Sync PR state to detect merged/queued PRs before pushing. syncStackPRs(cfg, s) @@ -194,7 +220,9 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { } // Create or update the stack on GitHub - syncStack(cfg, client, s) + if stacksAvailable { + syncStack(cfg, client, s) + } // Update base commit hashes and sync PR state updateBaseSHAs(s) diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 514d091..53c0ee4 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "net/url" + "os" "testing" "github.com/cli/go-gh/v2/pkg/api" @@ -829,3 +830,228 @@ func TestSubmit_CreatesMissingPRsAndUpdatesExisting(t *testing.T) { // Stack should be created with all 3 PRs assert.Contains(t, output, "Stack created on GitHub with 3 PRs") } + +func TestSubmit_PreflightCheck_404_BailsOut(t *testing.T) { + s := stack.Stack{ + // No ID — this is a new stack, so the pre-flight check will run. + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + pushed := false + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(string, []string, bool, bool) error { + pushed = true + return nil + } + restore := git.SetOps(mock) + defer restore() + + // Non-interactive config — should bail out immediately. + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"} + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.ErrorIs(t, err, ErrStacksUnavailable) + assert.Contains(t, output, "Stacked PRs are not enabled for this repository") + assert.False(t, pushed, "should not push when stacks are unavailable") +} + +func TestSubmit_PreflightCheck_404_Interactive_UserDeclinesAborts(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) + + pushed := false + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(string, []string, bool, bool) error { + pushed = true + return nil + } + restore := git.SetOps(mock) + defer restore() + + // Force interactive mode; survey will fail on the pipe, + // which is treated as a decline — same as user saying "no". + inR, inW, _ := os.Pipe() + inW.Close() + + cfg, _, errR := config.NewTestConfig() + cfg.In = inR + cfg.ForceInteractive = true + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"} + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.ErrorIs(t, err, ErrStacksUnavailable) + assert.Contains(t, output, "Stacked PRs are not enabled for this repository") + assert.False(t, pushed, "should not push when user declines") +} + +func TestSyncStack_SkippedWhenStacksUnavailable(t *testing.T) { + // Verify that syncStack is not called when stacksAvailable is false. + // This is the core behavior enabling unstacked PR creation. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + createCalled := false + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + createCalled = true + return 42, nil + }, + } + + cfg, _, errR := config.NewTestConfig() + + // When stacksAvailable=true, syncStack should be called. + syncStack(cfg, mock, s) + assert.True(t, createCalled, "syncStack should call CreateStack when invoked") + + // When stacksAvailable=false, the caller (runSubmit) skips syncStack + // entirely — verified by the submit_test integration tests above. + // Here we just confirm the contract: if syncStack is NOT called, + // CreateStack is NOT called. + createCalled = false + // (not calling syncStack) + assert.False(t, createCalled, "CreateStack should not be called when syncStack is skipped") + + cfg.Err.Close() + _, _ = io.ReadAll(errR) +} + +func TestSubmit_PreflightCheck_EmptyList_Proceeds(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) + + pushed := false + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(string, []string, bool, bool) error { + pushed = true + return nil + } + mock.LogRangeFn = func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{{Subject: "commit for " + head}}, nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{}, nil + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + CreatePRFn: func(base, head, title, body string, draft bool) (*github.PullRequest, error) { + return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil + }, + CreateStackFn: func([]int) (int, error) { return 99, nil }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + _, _ = io.ReadAll(errR) + + assert.NoError(t, err) + assert.True(t, pushed, "should proceed with push when ListStacks succeeds") +} + +func TestSubmit_PreflightCheck_SkippedWhenStackIDSet(t *testing.T) { + s := stack.Stack{ + ID: "42", // Existing stack — pre-flight check should be skipped. + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + listStacksCalled := false + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(string, []string, bool, bool) error { return nil } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + listStacksCalled = true + return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"} + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { + return &github.PullRequest{Number: 10, URL: "https://github.com/o/r/pull/10"}, nil + }, + UpdateStackFn: func(string, []int) error { return nil }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + _, _ = io.ReadAll(errR) + + assert.NoError(t, err) + assert.False(t, listStacksCalled, "ListStacks should not be called when stack ID already exists") +} diff --git a/cmd/sync.go b/cmd/sync.go index 019059a..2a5471c 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -116,22 +116,46 @@ 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 ...") // Sync PR state to detect merged PRs before rebasing. syncStackPRs(cfg, s) - // Save original refs so we can restore on conflict - branchNames := make([]string, len(s.Branches)) - for i, b := range s.Branches { - branchNames[i] = b.Branch + // 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 @@ -171,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() @@ -187,7 +222,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { break } - cfg.Successf("Rebased %s onto %s (squash-merge detected)", br.Branch, newBase) + cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", br.Branch, newBase) ontoOldBase = originalRefs[br.Branch] } else { var rebaseErr error diff --git a/cmd/sync_test.go b/cmd/sync_test.go index a842ae4..e3dc532 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) { @@ -508,10 +522,10 @@ func TestSync_PushForceFlagDependsOnRebase(t *testing.T) { } } -// TestSync_SquashMergedBranch_UsesOnto verifies that when a squash-merged +// TestSync_MergedBranch_UsesOnto verifies that when a merged // branch exists in the stack, sync's cascade rebase correctly uses --onto // to skip the merged branch and rebase subsequent branches onto the right base. -func TestSync_SquashMergedBranch_UsesOnto(t *testing.T) { +func TestSync_MergedBranch_UsesOnto(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -535,6 +549,7 @@ func TestSync_SquashMergedBranch_UsesOnto(t *testing.T) { } mock := newSyncMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } // Trunk behind remote to trigger rebase mock.RevParseFn = func(ref string) (string, error) { if ref == "main" { @@ -549,7 +564,12 @@ func TestSync_SquashMergedBranch_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 } @@ -588,6 +608,93 @@ func TestSync_SquashMergedBranch_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. @@ -647,3 +754,268 @@ 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 rebaseCalls2 []rebaseCall + var pushCalls2 []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 { + rebaseCalls2 = append(rebaseCalls2, rebaseCall{branch: "(rebase)" + base}) + return nil + } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + rebaseCalls2 = append(rebaseCalls2, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls2 = append(pushCalls2, 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, rebaseCalls2, "rebase should occur") + require.Len(t, pushCalls2, 1) + assert.True(t, pushCalls2[0].force, "push should use force after rebase") +} + +func TestSync_MergedBranchDeletedFromRemote(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", Head: "b1-stored-head-sha", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseOntoCalls []rebaseCall + + mock := newSyncMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { + // b1 does not exist locally (deleted from remote after merge) + return name != "b1" + } + mock.RevParseMultiFn = func(refs []string) ([]string, error) { + shas := make([]string, len(refs)) + for i, r := range refs { + if r == "b1" { + t.Fatalf("RevParseMulti should not be called with non-existent branch b1") + } + if r == "main" { + shas[i] = "local-sha" + } else if r == "origin/main" { + shas[i] = "remote-sha" + } else { + shas[i] = "sha-" + r + } + } + return shas, nil + } + // Trunk behind remote to trigger rebase + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" { + return "local-sha", nil + } + if ref == "origin/main" { + return "remote-sha", nil + } + return "sha-" + ref, nil + } + mock.IsAncestorFn = func(a, d string) (bool, error) { + // 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 } + mock.RebaseOntoFn = func(newBase, oldBase, branch string) error { + rebaseOntoCalls = append(rebaseOntoCalls, rebaseCall{newBase, oldBase, branch}) + 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, "Skipping b1") + + // 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) +} diff --git a/cmd/utils.go b/cmd/utils.go index f6cf0cc..9a09e48 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -20,13 +20,14 @@ var ErrSilent = &ExitError{Code: 1} // Typed exit errors for programmatic detection by scripts and agents. var ( - ErrNotInStack = &ExitError{Code: 2} // branch/stack not found - ErrConflict = &ExitError{Code: 3} // rebase conflict - ErrAPIFailure = &ExitError{Code: 4} // GitHub API error - ErrInvalidArgs = &ExitError{Code: 5} // invalid arguments or flags - ErrDisambiguate = &ExitError{Code: 6} // multiple stacks/remotes, can't auto-select - ErrRebaseActive = &ExitError{Code: 7} // rebase already in progress - ErrLockFailed = &ExitError{Code: 8} // could not acquire stack file lock + ErrNotInStack = &ExitError{Code: 2} // branch/stack not found + ErrConflict = &ExitError{Code: 3} // rebase conflict + ErrAPIFailure = &ExitError{Code: 4} // GitHub API error + ErrInvalidArgs = &ExitError{Code: 5} // invalid arguments or flags + ErrDisambiguate = &ExitError{Code: 6} // multiple stacks/remotes, can't auto-select + ErrRebaseActive = &ExitError{Code: 7} // rebase already in progress + ErrLockFailed = &ExitError{Code: 8} // could not acquire stack file lock + ErrStacksUnavailable = &ExitError{Code: 9} // stacked PRs not available for this repository ) // ExitError is returned by commands to indicate a specific exit code. @@ -317,6 +318,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) { diff --git a/docs/astro.config.mjs b/docs/astro.config.mjs index f585e2a..28498ef 100644 --- a/docs/astro.config.mjs +++ b/docs/astro.config.mjs @@ -13,14 +13,22 @@ export default defineConfig({ integrations: [ starlight({ title: 'GitHub Stacked PRs', - description: 'Manage stacked branches and pull requests with the gh stack CLI extension.', + description: 'Break large changes into small, reviewable pull requests that build on each other — with native GitHub support and the gh stack CLI.', favicon: '/favicon.svg', logo: { src: './src/assets/github-invertocat.svg', alt: 'GitHub', }, head: [ - { tag: 'meta', attrs: { name: 'robots', content: 'noindex, nofollow' } }, + { tag: 'meta', attrs: { property: 'og:type', content: 'website' } }, + { tag: 'meta', attrs: { property: 'og:site_name', content: 'GitHub Stacked PRs' } }, + { tag: 'meta', attrs: { property: 'og:image', content: 'https://github.github.com/gh-stack/github-social-card.jpg' } }, + { tag: 'meta', attrs: { property: 'og:image:alt', content: 'GitHub Stacked PRs — Break large changes into small, reviewable pull requests' } }, + { tag: 'meta', attrs: { property: 'og:image:width', content: '1200' } }, + { tag: 'meta', attrs: { property: 'og:image:height', content: '630' } }, + { tag: 'meta', attrs: { name: 'twitter:card', content: 'summary_large_image' } }, + { tag: 'meta', attrs: { name: 'twitter:site', content: '@github' } }, + { tag: 'meta', attrs: { name: 'twitter:image', content: 'https://github.github.com/gh-stack/github-social-card.jpg' } }, ], components: { SocialIcons: './src/components/CustomHeader.astro', diff --git a/docs/public/github-social-card.jpg b/docs/public/github-social-card.jpg new file mode 100644 index 0000000..7911cf1 Binary files /dev/null and b/docs/public/github-social-card.jpg differ diff --git a/docs/src/assets/stack-diagram.svg b/docs/src/assets/stack-diagram.svg deleted file mode 100644 index b0296ec..0000000 --- a/docs/src/assets/stack-diagram.svg +++ /dev/null @@ -1,48 +0,0 @@ - - - - - - - - - - - main - - - - - PR #1 · auth-layer - - - - - PR #2 · api-endpoints - - - - - PR #3 · frontend - - - - - - -← bottom -← top - diff --git a/docs/src/components/StackDiagram.astro b/docs/src/components/StackDiagram.astro new file mode 100644 index 0000000..583267b --- /dev/null +++ b/docs/src/components/StackDiagram.astro @@ -0,0 +1,62 @@ +--- +/** + * Inline SVG stack diagram — uses currentColor to inherit text color from the + * parent element, which is styled via external CSS. This avoids Safari bugs + * where @media (prefers-color-scheme) doesn't work inside SVG elements. + */ +--- +
+ + + + + + + + + + + main + + + + + PR #1 · auth-layer + + + + + PR #2 · api-endpoints + + + + + PR #3 · frontend + + + + + + + ← bottom + ← top + +
+ + diff --git a/docs/src/content/docs/index.mdx b/docs/src/content/docs/index.mdx index fdcb414..d6b8804 100644 --- a/docs/src/content/docs/index.mdx +++ b/docs/src/content/docs/index.mdx @@ -17,7 +17,7 @@ hero: import { Card, CardGrid, Aside } from '@astrojs/starlight/components'; import { Image } from 'astro:assets'; -import stackDiagram from '../../assets/stack-diagram.svg'; +import StackDiagram from '../../components/StackDiagram.astro'; import stackNavigator from '../../assets/screenshots/stack-navigator.png'; @@ -43,9 +43,7 @@ Large pull requests are hard to review, slow to merge, and prone to conflicts. R A **stack** is a series of pull requests in the same repository where each PR targets the branch of the PR below it, forming an ordered chain that ultimately lands on your main branch. -
- A stack of pull requests: main at the bottom, with auth-layer (PR #1), api-endpoints (PR #2), and frontend (PR #3) stacked on top -
+ GitHub understands stacks end-to-end: the pull request UI shows a **stack map** so reviewers can navigate between layers, branch protection rules are enforced against the **final target branch** (not just the direct base), and CI runs for every PR in the stack as if they were targeting the final branch. diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index 5853d9f..2a90504 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -223,7 +223,7 @@ gh stack rebase [flags] [branch] Fetches the latest changes from `origin`, then ensures each branch in the stack has the tip of the previous layer in its commit history. Rebases branches in order from trunk upward. -If a branch's PR has been squash-merged, the rebase automatically switches to `--onto` mode to correctly replay commits on top of the merge target. +If a branch's PR has been merged, the rebase automatically switches to `--onto` mode to correctly replay commits on top of the merge target. If a rebase conflict occurs, the operation pauses and prints the conflicted files with line numbers. Resolve the conflicts, stage with `git add`, and continue with `--continue`. To undo the entire rebase, use `--abort` to restore all branches to their pre-rebase state. @@ -441,3 +441,4 @@ gh stack feedback "Support for reordering branches" | 6 | Disambiguation required (branch belongs to multiple stacks) | | 7 | Rebase already in progress | | 8 | Stack is locked by another process | +| 9 | Stacked PRs not enabled for this repository | diff --git a/internal/config/config.go b/internal/config/config.go index f4ea5c7..f254e56 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,6 +30,10 @@ type Config struct { // GitHubClientOverride, when non-nil, is returned by GitHubClient() // instead of creating a real client. Used in tests to inject a MockClient. GitHubClientOverride ghapi.ClientOps + + // ForceInteractive, when true, makes IsInteractive() return true + // regardless of the terminal state. Used in tests. + ForceInteractive bool } // New creates a new Config with terminal-aware output and color support. @@ -106,7 +110,7 @@ func (c *Config) PRLink(number int, url string) string { } func (c *Config) IsInteractive() bool { - return c.Terminal.IsTerminalOutput() + return c.ForceInteractive || c.Terminal.IsTerminalOutput() } func (c *Config) Repo() (repository.Repository, error) { diff --git a/internal/git/git.go b/internal/git/git.go index 8af505d..b7b9941 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -179,7 +179,7 @@ func SaveRerereDeclined() error { // git rebase --onto // // This replays commits after oldBase from branch onto newBase. It is used -// when a prior branch was squash-merged and the normal rebase cannot detect +// when a prior branch was merged and the normal rebase cannot detect // which commits have already been applied. // If rerere resolves all conflicts automatically, the rebase continues // without user intervention. diff --git a/internal/tui/stackview/data.go b/internal/tui/stackview/data.go index 64f8191..a4108e9 100644 --- a/internal/tui/stackview/data.go +++ b/internal/tui/stackview/data.go @@ -45,15 +45,13 @@ func LoadBranchNodes(cfg *config.Config, s *stack.Stack, currentBranch string) [ node.IsLinear = isAncestor } - // For merged branches, use the merge-base (fork point) as the diff - // anchor since the base branch has moved past the merge point and - // a two-dot diff would show nothing after a squash merge. - isMerged := b.IsMerged() + // Use the merge-base (fork point) as the diff anchor so that we + // only show changes introduced on this branch. Without this, a + // diverged base (e.g. local main ahead of the branch's fork point) + // would inflate the diff with unrelated files. diffBase := baseBranch - if isMerged { - if mb, err := git.MergeBase(baseBranch, b.Branch); err == nil { - diffBase = mb - } + if mb, err := git.MergeBase(baseBranch, b.Branch); err == nil { + diffBase = mb } // Fetch commit range diff --git a/internal/tui/stackview/data_test.go b/internal/tui/stackview/data_test.go new file mode 100644 index 0000000..f0ff1e2 --- /dev/null +++ b/internal/tui/stackview/data_test.go @@ -0,0 +1,102 @@ +package stackview + +import ( + "testing" + + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/git" + ghapi "github.com/github/gh-stack/internal/github" + "github.com/github/gh-stack/internal/stack" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadBranchNodes_UsesMergeBaseForDivergedBranch(t *testing.T) { + // Scenario: local main has diverged from the branch's history. + // Without merge-base, diff would be computed against main directly, + // inflating the file count with unrelated changes. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "feature"}}, + } + + var diffBase string + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(ancestor, descendant string) (bool, error) { + // main is NOT an ancestor of feature (diverged) + return false, nil + }, + MergeBaseFn: func(a, b string) (string, error) { + return "abc123", nil + }, + LogRangeFn: func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{{SHA: "def456", Subject: "only commit"}}, nil + }, + DiffStatFilesFn: func(base, head string) ([]git.FileDiffStat, error) { + diffBase = base + return []git.FileDiffStat{ + {Path: "file1.go", Additions: 5, Deletions: 2}, + {Path: "file2.go", Additions: 3, Deletions: 1}, + }, nil + }, + }) + defer restore() + + cfg, outW, errW := config.NewTestConfig() + defer outW.Close() + defer errW.Close() + // Ensure no real GitHub API calls + cfg.GitHubClientOverride = &ghapi.MockClient{} + + nodes := LoadBranchNodes(cfg, s, "feature") + + require.Len(t, nodes, 1) + // Diff must be computed from merge-base, not from "main" directly. + assert.Equal(t, "abc123", diffBase, "diff should use merge-base as base, not the branch name") + assert.Len(t, nodes[0].FilesChanged, 2) + assert.Equal(t, 8, nodes[0].Additions) + assert.Equal(t, 3, nodes[0].Deletions) + assert.False(t, nodes[0].IsLinear) +} + +func TestLoadBranchNodes_LinearBranchStillUsesMergeBase(t *testing.T) { + // When base IS an ancestor (linear history), merge-base returns the + // base tip, so behavior is unchanged. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "feature"}}, + } + + var diffBase string + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(ancestor, descendant string) (bool, error) { + return true, nil + }, + MergeBaseFn: func(a, b string) (string, error) { + // For linear history, merge-base returns the base tip + return "main-tip-sha", nil + }, + LogRangeFn: func(base, head string) ([]git.CommitInfo, error) { + return nil, nil + }, + DiffStatFilesFn: func(base, head string) ([]git.FileDiffStat, error) { + diffBase = base + return []git.FileDiffStat{ + {Path: "only.go", Additions: 1, Deletions: 0}, + }, nil + }, + }) + defer restore() + + cfg, outW, errW := config.NewTestConfig() + defer outW.Close() + defer errW.Close() + cfg.GitHubClientOverride = &ghapi.MockClient{} + + nodes := LoadBranchNodes(cfg, s, "other") + + require.Len(t, nodes, 1) + assert.Equal(t, "main-tip-sha", diffBase) + assert.Len(t, nodes[0].FilesChanged, 1) + assert.True(t, nodes[0].IsLinear) +} diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 1dc3a51..ad928a4 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -539,6 +539,7 @@ gh stack submit --auto --draft - Pushes all active (non-merged) branches atomically (`--force-with-lease --atomic`) - Creates a new PR for each branch that doesn't have one (base set to the first non-merged ancestor branch) - After creating PRs, links them together as a **Stack** on GitHub (requires the repository to have stacks enabled) +- If stacks are not available (exit code 9), the repository does not have stacked PRs enabled. In interactive mode, `submit` offers to create regular (unstacked) PRs instead. In non-interactive mode, it exits with code 9. - Syncs PR metadata for branches that already have PRs **PR title auto-generation (`--auto`):** @@ -570,7 +571,7 @@ gh stack sync [flags] 1. **Fetch** latest changes from the remote 2. **Fast-forward trunk** to match remote (skips if already up to date, warns if diverged) -3. **Cascade rebase** all stack branches onto their updated parents (only if trunk moved). Handles squash-merged PRs automatically. If a conflict is detected, **all branches are restored** to their pre-rebase state and the command exits with code 3 — see [Handle rebase conflicts](#handle-rebase-conflicts-agent-workflow) for the resolution workflow +3. **Cascade rebase** all stack branches onto their updated parents (only if trunk moved). Handles merged PRs automatically. If a conflict is detected, **all branches are restored** to their pre-rebase state and the command exits with code 3 — see [Handle rebase conflicts](#handle-rebase-conflicts-agent-workflow) for the resolution workflow 4. **Push** all active branches atomically 5. **Sync PR state** from GitHub and report the status of each PR @@ -625,7 +626,7 @@ gh stack rebase --abort **Conflict handling:** See [Handle rebase conflicts](#handle-rebase-conflicts-agent-workflow) in the Workflows section for the full resolution workflow. -**Squash-merge detection:** If a branch's PR was squash-merged on GitHub, the rebase automatically handles this and correctly replays commits on top of the merge target. +**Merged PR detection:** If a branch's PR was merged on GitHub, the rebase automatically handles this using `--onto` mode and correctly replays commits on top of the merge target. **Rerere (conflict memory):** `git rerere` is enabled by `init` so previously resolved conflicts are auto-resolved in future rebases. @@ -783,6 +784,7 @@ gh stack unstack feature-auth | 6 | Disambiguation required | A branch belongs to multiple stacks. Run `gh stack checkout ` to switch to a non-shared branch first | | 7 | Rebase already in progress | Run `gh stack rebase --continue` (after resolving conflicts) or `gh stack rebase --abort` to start over | | 8 | Stack is locked | Another `gh stack` process is writing the stack file. Wait and retry — the lock times out after 5 seconds | +| 9 | Stacked PRs unavailable | The repository does not have stacked PRs enabled. `submit` will offer to create regular (unstacked) PRs in interactive mode | ## Known limitations