diff --git a/cmd/rebase.go b/cmd/rebase.go index d0c2814..9b79efd 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -198,14 +198,14 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return fmt.Errorf("resolving branch refs: %w", err) } - // 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. + // Get --onto state from a merged branch immediately below the rebase range. + // Ensures that when --upstack excludes merged branches, we still check the + // immediate predecessor and use --onto if needed. needsOnto := false var ontoOldBase string if startIdx > 0 { prev := s.Branches[startIdx-1] - if prev.IsSkipped() { + if prev.IsMerged() { if sha, ok := originalRefs[prev.Branch]; ok { needsOnto = true ontoOldBase = sha @@ -315,6 +315,14 @@ func continueRebase(cfg *config.Config, gitDir string) error { return fmt.Errorf("no stack found for branch %s", state.OriginalBranch) } + // Refresh PR state before selecting the base and cascading the remaining + // branches. The queued flag is transient (not persisted), so it was lost + // when the stack was reloaded from disk above. Without this, a queued + // branch in the remaining cascade would be treated as active and its + // frozen merge-queue branch would be rebased. Mirrors the syncStackPRs + // call in runRebase before its cascade. + _ = syncStackPRs(cfg, s) + // The branch that had the conflict is stored in state; fall back to // looking it up by index for backwards compatibility with older state files. conflictBranch := state.ConflictBranch @@ -334,10 +342,10 @@ func continueRebase(cfg *config.Config, gitDir string) error { var baseBranch string if state.UseOnto { - // The --onto path targets the first non-skipped ancestor, or trunk. + // The --onto path targets the first non-merged ancestor, or trunk. baseBranch = s.Trunk.Branch for j := state.CurrentBranchIndex - 1; j >= 0; j-- { - if !s.Branches[j].IsSkipped() { + if !s.Branches[j].IsMerged() { baseBranch = s.Branches[j].Branch break } diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 611f2b4..683bfe3 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-stack/internal/config" "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/github" "github.com/github/gh-stack/internal/stack" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -680,6 +681,188 @@ func TestRebase_SkipsMergedBranches(t *testing.T) { assert.Equal(t, "b2", rebaseCalls[0].branch) } +// queuedPRClient returns a MockClient whose FindPRByNumber reports the given PR +// numbers as queued (in a merge queue, open, not merged) and finds no PR by +// branch name. Used to drive the transient Queued state through syncStackPRs in +// rebase/sync tests. +func queuedPRClient(headByNumber map[int]string) *github.MockClient { + return &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + head, ok := headByNumber[n] + if !ok { + return nil, nil + } + return &github.PullRequest{ + Number: n, + HeadRefName: head, + State: "OPEN", + Merged: false, + MergeQueueEntry: &github.MergeQueueEntry{ID: fmt.Sprintf("MQ_%d", n)}, + }, nil + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + } +} + +// TestRebase_QueuedBranch_DownstreamStaysStacked verifies the #144 fix: a queued +// PR is NOT treated as merged. Its branch is skipped (frozen in the merge queue), +// but downstream branches stay stacked on top of it — they rebase onto the queued +// branch, not --onto trunk with the queued commits dropped. +func TestRebase_QueuedBranch_DownstreamStaysStacked(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"}) + 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") + assert.Contains(t, output, "queued") + assert.NotContains(t, output, "adjusted for merged PR", + "queued branches must not trigger the merged --onto path") + + // b2 stays stacked on the queued b1 (not rebased --onto main); b3 onto b2. + require.Len(t, rebaseCalls, 2) + assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseCalls[0], + "b2 should rebase onto the queued branch b1, keeping its commits") + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1], + "b3 should rebase onto b2") +} + +// TestRebase_MergedBelowQueued_KeepsStackedOnQueued verifies that when a merged +// branch sits below a queued branch, the branch above the queued one stays +// stacked on the queued branch. The queued branch is frozen and still carries the +// merged branch's commits, so downstream cannot drop them via --onto. +func TestRebase_MergedBelowQueued_KeepsStackedOnQueued(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", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b3") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{11: "b2"}) + 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") + assert.Contains(t, output, "PR #10 merged") + assert.Contains(t, output, "Skipping b2") + assert.Contains(t, output, "queued") + + // b1 merged and b2 queued are both skipped. b3 stays stacked on the queued + // b2 — it must NOT be rebased --onto main (which would drop b2's + b1's + // commits while b2 is frozen). + require.Len(t, rebaseCalls, 1) + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[0], + "b3 should rebase onto the queued b2, not --onto main") + assert.NotContains(t, output, "adjusted for merged PR") +} + +// TestRebase_UpstackAboveQueuedBranch verifies the onto-seed fix: with --upstack +// starting just above a queued branch, the first in-range branch rebases normally +// onto the queued predecessor rather than dropping its commits via --onto. +func TestRebase_UpstackAboveQueuedBranch(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"}) + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--upstack"}) + 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) + // upstack from b2 = [b2, b3]; b1 (queued) is below the range. + require.Len(t, rebaseCalls, 2) + assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseCalls[0], + "b2 should rebase onto the queued predecessor b1, not --onto main") + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1], + "b3 should rebase onto b2") + assert.NotContains(t, output, "adjusted for merged PR") +} + // TestRebase_StateRoundTrip verifies that rebase state can be saved and loaded // back with all fields preserved, including the --onto fields. func TestRebase_StateRoundTrip(t *testing.T) { @@ -791,6 +974,80 @@ func TestRebase_Continue_RebasesRemainingBranches(t *testing.T) { assert.Contains(t, checkouts, "b1", "should checkout original branch") } +// TestRebase_Continue_QueuedBranchBelowConflict verifies that a queued branch is +// still skipped when the cascade resumes via --continue after a conflict below +// it. The Queued flag is transient and lost when continueRebase reloads the +// stack from disk, so it must be refreshed before the remaining cascade — else +// the frozen merge-queue branch would be rebased. +func TestRebase_Continue_QueuedBranchBelowConflict(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 20}}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + // State: b1 (below the queued b2) conflicted; b2 and b3 remain. + state := &rebaseState{ + CurrentBranchIndex: 0, + ConflictBranch: "b1", + RemainingBranches: []string{"b2", "b3"}, + OriginalBranch: "b3", + OriginalRefs: map[string]string{ + "main": "main-orig-sha", + "b1": "sha-b1", + "b2": "sha-b2", + "b3": "sha-b3", + }, + } + stateData, _ := json.MarshalIndent(state, "", " ") + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "gh-stack-rebase-state"), stateData, 0644)) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b1") + mock.BranchExistsFn = func(name string) bool { return true } + mock.IsRebaseInProgressFn = func() bool { return true } + mock.RebaseContinueFn = func(opts git.RebaseOpts) error { return nil } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{20: "b2"}) + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--continue"}) + 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 b2") + assert.Contains(t, output, "queued") + + // Only b3 is rebased, onto the queued b2. The queued b2 itself must not be + // rebased (its branch is frozen in the merge queue). + require.Len(t, rebaseCalls, 1) + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[0]) + for _, c := range rebaseCalls { + assert.NotEqual(t, "b2", c.branch, "the frozen queued branch must not be rebased") + } +} + // TestRebase_Continue_OntoMode verifies the --continue path when UseOnto is // set (merged branches upstream). With no remaining branches, only // RebaseContinue runs and the state is cleaned up. diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 746739e..6eaebcb 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -745,6 +745,83 @@ func TestSync_MergedBranch_UsesOnto(t *testing.T) { assert.True(t, pushCalls[0].force) } +// TestSync_QueuedBranch_DownstreamStaysStacked verifies the #144 fix in the sync +// path: a queued branch is skipped from push (frozen in the merge queue) but +// downstream branches stay stacked on top of it — they are NOT rebased --onto +// trunk with the queued commits dropped. +func TestSync_QueuedBranch_DownstreamStaysStacked(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseOntoCalls []rebaseCall + var pushCalls []pushCall + + mock := newSyncMock(tmpDir, "b2") + // Trunk behind remote to trigger rebase; branches match their remote. + mock.RevParseFn = func(ref string) (string, error) { + switch ref { + case "main": + return "local-sha", nil + case "origin/main": + return "remote-sha", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + } + mock.UpdateBranchRefFn = func(string, string) error { return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseOntoCalls = append(rebaseOntoCalls, 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() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"}) + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Out.Close() + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.Contains(t, output, "queued") + + // b1 is queued → skipped, but downstream stays stacked on it: + // b2 onto b1 (not --onto main), b3 onto b2. + require.Len(t, rebaseOntoCalls, 2) + assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseOntoCalls[0], + "b2 should rebase onto the queued b1, keeping its commits") + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseOntoCalls[1], + "b3 should rebase onto b2") + + // The queued branch is excluded from push; only b2 and b3 are pushed. + require.Len(t, pushCalls, 1) + assert.Equal(t, []string{"b2", "b3"}, pushCalls[0].branches, + "queued b1 must not be pushed") +} + // 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. diff --git a/cmd/utils.go b/cmd/utils.go index 3a822b0..4266461 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -850,23 +850,33 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { base = s.Branches[absIdx-1].Branch } - // Skip merged and queued branches. + // Skip merged and queued branches — but treat them differently for + // downstream rebasing. if br.IsSkipped() { - ontoOldBase = originalRefs[br.Branch] - needsOnto = true if br.IsMerged() { + // A merged PR's commits are already in trunk, so downstream + // branches must drop them by rebasing --onto the first + // non-merged ancestor. + ontoOldBase = originalRefs[br.Branch] + needsOnto = true cfg.Successf("Skipping %s (PR %s merged)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) - } else if br.IsQueued() { + } else { + // A queued PR is frozen in the merge queue and its commits are + // NOT yet in trunk. Downstream branches must stay stacked on top + // of it, so do not switch to --onto (which would drop its + // commits). Reset onto state in case a merged branch set it. + needsOnto = false 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. + // Find --onto target: first non-merged ancestor, or trunk. Queued + // ancestors keep their commits, so they are valid --onto targets. newBase := s.Trunk.Branch for j := absIdx - 1; j >= 0; j-- { - if !s.Branches[j].IsSkipped() { + if !s.Branches[j].IsMerged() { newBase = s.Branches[j].Branch break }