Skip to content

Commit 50e5514

Browse files
committed
Don't treat queued PRs as merged when rebasing the stack
gh stack rebase and gh stack sync share cascadeRebase, which skipped branches via IsSkipped() (merged or queued) and then switched to a `git rebase --onto` that drops the skipped branch's commits from every downstream branch. That is right for a merged PR — its commits are already in trunk — but wrong for a queued PR: its commits only exist on its own branch, which is frozen in the merge queue, so the branches above it were rebased onto trunk and lost work they depend on. Handle the two cases separately. A merged branch still activates --onto so its commits are dropped. A queued branch is still skipped (its branch is frozen and is not rebased or pushed), but onto mode is reset so downstream branches rebase normally onto the queued branch, keeping its commits underneath. The --onto target search, the runRebase --onto seed, and the continueRebase display base now key on IsMerged() instead of IsSkipped(), so a queued predecessor no longer forces downstream branches onto trunk. gh stack sync is fixed through the same shared helper. Add rebase coverage for a queued branch mid-stack, a merged branch below a queued branch, and --upstack above a queued branch, plus a sync test that also asserts the queued branch is excluded from the push. The transient queued state is injected through the GitHub mock's merge-queue entry.
1 parent b3250b9 commit 50e5514

4 files changed

Lines changed: 282 additions & 12 deletions

File tree

cmd/rebase.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -198,14 +198,14 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
198198
return fmt.Errorf("resolving branch refs: %w", err)
199199
}
200200

201-
// Get --onto state from merged/queued branches below the rebase range.
202-
// Ensures that when --upstack excludes skipped branches, we still check
203-
// the immediate predecessor and use --onto if needed.
201+
// Get --onto state from a merged branch immediately below the rebase range.
202+
// Ensures that when --upstack excludes merged branches, we still check the
203+
// immediate predecessor and use --onto if needed.
204204
needsOnto := false
205205
var ontoOldBase string
206206
if startIdx > 0 {
207207
prev := s.Branches[startIdx-1]
208-
if prev.IsSkipped() {
208+
if prev.IsMerged() {
209209
if sha, ok := originalRefs[prev.Branch]; ok {
210210
needsOnto = true
211211
ontoOldBase = sha
@@ -334,10 +334,10 @@ func continueRebase(cfg *config.Config, gitDir string) error {
334334

335335
var baseBranch string
336336
if state.UseOnto {
337-
// The --onto path targets the first non-skipped ancestor, or trunk.
337+
// The --onto path targets the first non-merged ancestor, or trunk.
338338
baseBranch = s.Trunk.Branch
339339
for j := state.CurrentBranchIndex - 1; j >= 0; j-- {
340-
if !s.Branches[j].IsSkipped() {
340+
if !s.Branches[j].IsMerged() {
341341
baseBranch = s.Branches[j].Branch
342342
break
343343
}

cmd/rebase_test.go

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/github/gh-stack/internal/config"
1313
"github.com/github/gh-stack/internal/git"
14+
"github.com/github/gh-stack/internal/github"
1415
"github.com/github/gh-stack/internal/stack"
1516
"github.com/stretchr/testify/assert"
1617
"github.com/stretchr/testify/require"
@@ -680,6 +681,188 @@ func TestRebase_SkipsMergedBranches(t *testing.T) {
680681
assert.Equal(t, "b2", rebaseCalls[0].branch)
681682
}
682683

684+
// queuedPRClient returns a MockClient whose FindPRByNumber reports the given PR
685+
// numbers as queued (in a merge queue, open, not merged) and finds no PR by
686+
// branch name. Used to drive the transient Queued state through syncStackPRs in
687+
// rebase/sync tests.
688+
func queuedPRClient(headByNumber map[int]string) *github.MockClient {
689+
return &github.MockClient{
690+
FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
691+
head, ok := headByNumber[n]
692+
if !ok {
693+
return nil, nil
694+
}
695+
return &github.PullRequest{
696+
Number: n,
697+
HeadRefName: head,
698+
State: "OPEN",
699+
Merged: false,
700+
MergeQueueEntry: &github.MergeQueueEntry{ID: fmt.Sprintf("MQ_%d", n)},
701+
}, nil
702+
},
703+
FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil },
704+
}
705+
}
706+
707+
// TestRebase_QueuedBranch_DownstreamStaysStacked verifies the #144 fix: a queued
708+
// PR is NOT treated as merged. Its branch is skipped (frozen in the merge queue),
709+
// but downstream branches stay stacked on top of it — they rebase onto the queued
710+
// branch, not --onto trunk with the queued commits dropped.
711+
func TestRebase_QueuedBranch_DownstreamStaysStacked(t *testing.T) {
712+
s := stack.Stack{
713+
Trunk: stack.BranchRef{Branch: "main"},
714+
Branches: []stack.BranchRef{
715+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
716+
{Branch: "b2"},
717+
{Branch: "b3"},
718+
},
719+
}
720+
721+
tmpDir := t.TempDir()
722+
writeStackFile(t, tmpDir, s)
723+
724+
var rebaseCalls []rebaseCall
725+
726+
mock := newRebaseMock(tmpDir, "b2")
727+
mock.BranchExistsFn = func(name string) bool { return true }
728+
mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error {
729+
rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch})
730+
return nil
731+
}
732+
733+
restore := git.SetOps(mock)
734+
defer restore()
735+
736+
cfg, _, errR := config.NewTestConfig()
737+
cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"})
738+
cmd := RebaseCmd(cfg)
739+
cmd.SetOut(io.Discard)
740+
cmd.SetErr(io.Discard)
741+
err := cmd.Execute()
742+
743+
cfg.Err.Close()
744+
errOut, _ := io.ReadAll(errR)
745+
output := string(errOut)
746+
747+
assert.NoError(t, err)
748+
assert.Contains(t, output, "Skipping b1")
749+
assert.Contains(t, output, "queued")
750+
assert.NotContains(t, output, "adjusted for merged PR",
751+
"queued branches must not trigger the merged --onto path")
752+
753+
// b2 stays stacked on the queued b1 (not rebased --onto main); b3 onto b2.
754+
require.Len(t, rebaseCalls, 2)
755+
assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseCalls[0],
756+
"b2 should rebase onto the queued branch b1, keeping its commits")
757+
assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1],
758+
"b3 should rebase onto b2")
759+
}
760+
761+
// TestRebase_MergedBelowQueued_KeepsStackedOnQueued verifies that when a merged
762+
// branch sits below a queued branch, the branch above the queued one stays
763+
// stacked on the queued branch. The queued branch is frozen and still carries the
764+
// merged branch's commits, so downstream cannot drop them via --onto.
765+
func TestRebase_MergedBelowQueued_KeepsStackedOnQueued(t *testing.T) {
766+
s := stack.Stack{
767+
Trunk: stack.BranchRef{Branch: "main"},
768+
Branches: []stack.BranchRef{
769+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}},
770+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}},
771+
{Branch: "b3"},
772+
},
773+
}
774+
775+
tmpDir := t.TempDir()
776+
writeStackFile(t, tmpDir, s)
777+
778+
var rebaseCalls []rebaseCall
779+
780+
mock := newRebaseMock(tmpDir, "b3")
781+
mock.BranchExistsFn = func(name string) bool { return true }
782+
mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error {
783+
rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch})
784+
return nil
785+
}
786+
787+
restore := git.SetOps(mock)
788+
defer restore()
789+
790+
cfg, _, errR := config.NewTestConfig()
791+
cfg.GitHubClientOverride = queuedPRClient(map[int]string{11: "b2"})
792+
cmd := RebaseCmd(cfg)
793+
cmd.SetOut(io.Discard)
794+
cmd.SetErr(io.Discard)
795+
err := cmd.Execute()
796+
797+
cfg.Err.Close()
798+
errOut, _ := io.ReadAll(errR)
799+
output := string(errOut)
800+
801+
assert.NoError(t, err)
802+
assert.Contains(t, output, "Skipping b1")
803+
assert.Contains(t, output, "PR #10 merged")
804+
assert.Contains(t, output, "Skipping b2")
805+
assert.Contains(t, output, "queued")
806+
807+
// b1 merged and b2 queued are both skipped. b3 stays stacked on the queued
808+
// b2 — it must NOT be rebased --onto main (which would drop b2's + b1's
809+
// commits while b2 is frozen).
810+
require.Len(t, rebaseCalls, 1)
811+
assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[0],
812+
"b3 should rebase onto the queued b2, not --onto main")
813+
assert.NotContains(t, output, "adjusted for merged PR")
814+
}
815+
816+
// TestRebase_UpstackAboveQueuedBranch verifies the onto-seed fix: with --upstack
817+
// starting just above a queued branch, the first in-range branch rebases normally
818+
// onto the queued predecessor rather than dropping its commits via --onto.
819+
func TestRebase_UpstackAboveQueuedBranch(t *testing.T) {
820+
s := stack.Stack{
821+
Trunk: stack.BranchRef{Branch: "main"},
822+
Branches: []stack.BranchRef{
823+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
824+
{Branch: "b2"},
825+
{Branch: "b3"},
826+
},
827+
}
828+
829+
tmpDir := t.TempDir()
830+
writeStackFile(t, tmpDir, s)
831+
832+
var rebaseCalls []rebaseCall
833+
834+
mock := newRebaseMock(tmpDir, "b2")
835+
mock.BranchExistsFn = func(name string) bool { return true }
836+
mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error {
837+
rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch})
838+
return nil
839+
}
840+
841+
restore := git.SetOps(mock)
842+
defer restore()
843+
844+
cfg, _, errR := config.NewTestConfig()
845+
cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"})
846+
cmd := RebaseCmd(cfg)
847+
cmd.SetArgs([]string{"--upstack"})
848+
cmd.SetOut(io.Discard)
849+
cmd.SetErr(io.Discard)
850+
err := cmd.Execute()
851+
852+
cfg.Err.Close()
853+
errOut, _ := io.ReadAll(errR)
854+
output := string(errOut)
855+
856+
assert.NoError(t, err)
857+
// upstack from b2 = [b2, b3]; b1 (queued) is below the range.
858+
require.Len(t, rebaseCalls, 2)
859+
assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseCalls[0],
860+
"b2 should rebase onto the queued predecessor b1, not --onto main")
861+
assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1],
862+
"b3 should rebase onto b2")
863+
assert.NotContains(t, output, "adjusted for merged PR")
864+
}
865+
683866
// TestRebase_StateRoundTrip verifies that rebase state can be saved and loaded
684867
// back with all fields preserved, including the --onto fields.
685868
func TestRebase_StateRoundTrip(t *testing.T) {

cmd/sync_test.go

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -745,6 +745,83 @@ func TestSync_MergedBranch_UsesOnto(t *testing.T) {
745745
assert.True(t, pushCalls[0].force)
746746
}
747747

748+
// TestSync_QueuedBranch_DownstreamStaysStacked verifies the #144 fix in the sync
749+
// path: a queued branch is skipped from push (frozen in the merge queue) but
750+
// downstream branches stay stacked on top of it — they are NOT rebased --onto
751+
// trunk with the queued commits dropped.
752+
func TestSync_QueuedBranch_DownstreamStaysStacked(t *testing.T) {
753+
s := stack.Stack{
754+
Trunk: stack.BranchRef{Branch: "main"},
755+
Branches: []stack.BranchRef{
756+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
757+
{Branch: "b2"},
758+
{Branch: "b3"},
759+
},
760+
}
761+
762+
tmpDir := t.TempDir()
763+
writeStackFile(t, tmpDir, s)
764+
765+
var rebaseOntoCalls []rebaseCall
766+
var pushCalls []pushCall
767+
768+
mock := newSyncMock(tmpDir, "b2")
769+
// Trunk behind remote to trigger rebase; branches match their remote.
770+
mock.RevParseFn = func(ref string) (string, error) {
771+
switch ref {
772+
case "main":
773+
return "local-sha", nil
774+
case "origin/main":
775+
return "remote-sha", nil
776+
}
777+
if strings.HasPrefix(ref, "origin/") {
778+
return "sha-" + strings.TrimPrefix(ref, "origin/"), nil
779+
}
780+
return "sha-" + ref, nil
781+
}
782+
mock.UpdateBranchRefFn = func(string, string) error { return nil }
783+
mock.CheckoutBranchFn = func(string) error { return nil }
784+
mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error {
785+
rebaseOntoCalls = append(rebaseOntoCalls, rebaseCall{newBase, oldBase, branch})
786+
return nil
787+
}
788+
mock.PushFn = func(remote string, branches []string, force, atomic bool) error {
789+
pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic})
790+
return nil
791+
}
792+
793+
restore := git.SetOps(mock)
794+
defer restore()
795+
796+
cfg, _, errR := config.NewTestConfig()
797+
cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"})
798+
cmd := SyncCmd(cfg)
799+
cmd.SetOut(io.Discard)
800+
cmd.SetErr(io.Discard)
801+
err := cmd.Execute()
802+
803+
cfg.Out.Close()
804+
cfg.Err.Close()
805+
errOut, _ := io.ReadAll(errR)
806+
output := string(errOut)
807+
808+
assert.NoError(t, err)
809+
assert.Contains(t, output, "queued")
810+
811+
// b1 is queued → skipped, but downstream stays stacked on it:
812+
// b2 onto b1 (not --onto main), b3 onto b2.
813+
require.Len(t, rebaseOntoCalls, 2)
814+
assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseOntoCalls[0],
815+
"b2 should rebase onto the queued b1, keeping its commits")
816+
assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseOntoCalls[1],
817+
"b3 should rebase onto b2")
818+
819+
// The queued branch is excluded from push; only b2 and b3 are pushed.
820+
require.Len(t, pushCalls, 1)
821+
assert.Equal(t, []string{"b2", "b3"}, pushCalls[0].branches,
822+
"queued b1 must not be pushed")
823+
}
824+
748825
// TestSync_StaleOntoOldBase_FallsBackToMergeBase verifies that when a branch
749826
// was already rebased past the merged branch's tip, sync detects the stale
750827
// ontoOldBase and falls back to merge-base for the correct divergence point.

cmd/utils.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -850,23 +850,33 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult {
850850
base = s.Branches[absIdx-1].Branch
851851
}
852852

853-
// Skip merged and queued branches.
853+
// Skip merged and queued branches — but treat them differently for
854+
// downstream rebasing.
854855
if br.IsSkipped() {
855-
ontoOldBase = originalRefs[br.Branch]
856-
needsOnto = true
857856
if br.IsMerged() {
857+
// A merged PR's commits are already in trunk, so downstream
858+
// branches must drop them by rebasing --onto the first
859+
// non-merged ancestor.
860+
ontoOldBase = originalRefs[br.Branch]
861+
needsOnto = true
858862
cfg.Successf("Skipping %s (PR %s merged)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL))
859-
} else if br.IsQueued() {
863+
} else {
864+
// A queued PR is frozen in the merge queue and its commits are
865+
// NOT yet in trunk. Downstream branches must stay stacked on top
866+
// of it, so do not switch to --onto (which would drop its
867+
// commits). Reset onto state in case a merged branch set it.
868+
needsOnto = false
860869
cfg.Successf("Skipping %s (PR %s queued)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL))
861870
}
862871
continue
863872
}
864873

865874
if needsOnto {
866-
// Find --onto target: first non-skipped ancestor, or trunk.
875+
// Find --onto target: first non-merged ancestor, or trunk. Queued
876+
// ancestors keep their commits, so they are valid --onto targets.
867877
newBase := s.Trunk.Branch
868878
for j := absIdx - 1; j >= 0; j-- {
869-
if !s.Branches[j].IsSkipped() {
879+
if !s.Branches[j].IsMerged() {
870880
newBase = s.Branches[j].Branch
871881
break
872882
}

0 commit comments

Comments
 (0)