Fix rebase treating queued PRs as merged#173
Open
skarim wants to merge 2 commits into
Open
Conversation
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes queued PR handling during cascade rebases so downstream branches retain queued commits.
Changes:
- Distinguishes merged and queued branches in rebase logic.
- Corrects
--upstackand continuation base selection. - Adds rebase and sync regression tests.
Show a summary per file
| File | Description |
|---|---|
cmd/utils.go |
Preserves queued branches as downstream rebase bases. |
cmd/rebase.go |
Updates --onto selection logic. |
cmd/rebase_test.go |
Adds queued-branch rebase coverage. |
cmd/sync_test.go |
Adds queued-branch sync coverage. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
continueRebase reloads the stack from disk, where the Queued flag is transient (json:"-") and therefore lost, and it only called syncStackPRs after the cascade. So if the initial rebase conflicted on a branch below a queued branch, `gh stack rebase --continue` resumed with that branch seen as active: it rebased the frozen merge-queue branch and rebuilt the downstream branches on a local history that differs from the queued branch. Call syncStackPRs right after resolving the stack — before selecting the base and cascading the remaining branches — mirroring the refresh runRebase already does before its cascade. The queued flag is repopulated, so queued branches stay skipped and downstream branches stay stacked on them. Add TestRebase_Continue_QueuedBranchBelowConflict, which conflicts below a queued branch and asserts the frozen branch is not rebased and the branch above stays stacked on it. Verified to fail without the refresh.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gh stack rebase(andgh stack sync, which shares the same cascade-rebase code) treated a queued PR as if it had already merged. When a branch's PR was in the merge queue, the rebase skipped that branch and then rebased the branches above it--ontotrunk, dropping the queued branch's commits from every downstream branch:A queued PR is not merged into trunk yet — its commits only exist on its own branch, which is frozen while it sits in the merge queue. Rebasing the branches above it onto trunk therefore discarded work that those downstream branches depend on.
Root cause
cascadeRebase(cmd/utils.go) branched onBranchRef.IsSkipped(), which isIsMerged() || IsQueued(). Both merged and queued branches setneedsOnto = true, activating thegit rebase --ontopath that drops the skipped branch's commits from downstream branches. That is correct for a merged PR — its commits really are in trunk — but wrong for a queued PR. The--ontoseed inrunRebase(cmd/rebase.go) shared the same conflation for--upstack.Behavior
Merged and queued branches are now handled differently during a cascade rebase:
--ontothe first non-merged ancestor to drop them.gh stack syncgets the same fix, since it calls the sharedcascadeRebasehelper.Changes
cmd/utils.go:cascadeRebasesplits the skip handling — a merged branch setsneedsOnto/ontoOldBaseas before, while a queued branch resetsneedsOntoso downstream branches rebase onto it instead of trunk. The--ontotarget search now skips only merged ancestors (queued ancestors keep their commits, so they are valid targets).cmd/rebase.go:runRebaseonly seeds--ontomode when the branch immediately below the rangeIsMerged()(wasIsSkipped()), so a queued predecessor no longer forces the first in-range branch onto trunk.continueRebaseresolves its display base againstIsMerged()for consistency.Testing
cmd/rebase_test.go— aqueuedPRClienthelper (drives the transient queued state throughsyncStackPRsvia a merge-queue entry) plus:TestRebase_QueuedBranch_DownstreamStaysStacked: downstream branches rebase onto the queued branch, not--ontotrunk, with no "adjusted for merged PR".TestRebase_MergedBelowQueued_KeepsStackedOnQueued: a branch above a queued branch stays stacked on it even when a merged branch sits below.TestRebase_UpstackAboveQueuedBranch:--upstackabove a queued branch rebases onto the queued predecessor.cmd/sync_test.go:TestSync_QueuedBranch_DownstreamStaysStacked: the same behavior throughsync, and the queued branch is excluded from the push.go vet ./...is clean and the fullgo test -race -count=1 ./...suite passes.Stack created with GitHub Stacks CLI • Give Feedback 💬