fix --onto rebase for merged branches#43
Open
skarim wants to merge 3 commits intoskarim/fix-sync-deletedfrom
Open
fix --onto rebase for merged branches#43skarim wants to merge 3 commits intoskarim/fix-sync-deletedfrom
skarim wants to merge 3 commits intoskarim/fix-sync-deletedfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves how gh stack sync and gh stack rebase compute git rebase --onto arguments when merged branches are present—especially when merged branches have been deleted locally—and adds coverage for “stale onto old base” scenarios to avoid replaying already-applied commits.
Changes:
- Backfill
originalRefsfor merged branches deleted locally using the stack file’s storedBranchRef.Headso--ontorebases have a validoldBase. - Detect stale
ontoOldBase(no longer an ancestor) and fall back tomerge-base(newBase, branch)for the divergence point. - Add/adjust tests covering stale
ontoOldBase,--upstackwith merged predecessor branches, and merged-branch-deleted scenarios.
Show a summary per file
| File | Description |
|---|---|
cmd/sync.go |
Backfills originalRefs for deleted merged branches and adds stale ontoOldBase → merge-base fallback during cascade rebase. |
cmd/rebase.go |
Same backfill + stale-base fallback, plus pre-seeding --onto state for --upstack when a merged branch is immediately below the range. |
cmd/sync_test.go |
Adds a stale ontoOldBase test and strengthens merged-branch-deleted expectations using stored Head. |
cmd/rebase_test.go |
Adds stale ontoOldBase test, --upstack merged-predecessor coverage, and updates merged-branch-deleted test to validate stored Head usage. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
cmd/sync.go:207
- The stale
ontoOldBasefallback currently runs only whenIsAncestorreturns(false, nil), and it silently ignores errors from bothIsAncestorandMergeBase. IfontoOldBaseis empty (e.g., merged branch deleted locally and no storedHead) or either git call errors,actualOldBasecan remain empty/stale andRebaseOntowill fail with a misleading “Conflict detected” path. Consider explicitly handlingontoOldBase == ""(computemerge-base(newBase, branch)or return a targeted error), and surface errors fromIsAncestor/MergeBaseas warnings or failures rather than silently proceeding.
// 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
}
}
cmd/rebase.go:257
- Similar to sync: the stale
ontoOldBasedetection ignores errors and doesn’t guard against an emptyontoOldBase. IfontoOldBaseis empty/stale andIsAncestorerrors (orMergeBasefails),actualOldBasecan remain invalid andRebaseOntowill fail, surfacing as a generic conflict flow. Prefer treating emptyontoOldBaseas a first-class case (computemerge-base(newBase, branch)or error), and propagate/emit warnings whenIsAncestororMergeBasefail so users aren’t left with unexplained behavior.
// 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
}
}
- Files reviewed: 4/4 changed files
- Comments generated: 2
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.
Fix
--ontorebase for merged branches (fixes #31)When a branch in the stack is merged on remote, subsequent
rebaseandsyncoperations need to usegit rebase --ontoto transplant the next branch onto trunk. Fixes for this in a couple of areas:originalRefshad no entry andontoOldBasewas empty — causinggit rebase --onto main "" b2to fail. Fixed by backfillingoriginalRefsfrom the persistedBranchRef.HeadSHA in the stack file.--upstackflag: When using--upstack, the rebase loop starts at the current branch, so it never iterates over merged branches below it and never setsneedsOnto. Fixed by checking the immediate predecessor first.IsAncestorguard that falls back tomerge-basewhen the old base is stale.