Fix for rebase reporting false success & always pull the latest trunk from remote - #327
Closed
skarim wants to merge 6 commits into
Closed
Fix for rebase reporting false success & always pull the latest trunk from remote#327skarim wants to merge 6 commits into
skarim wants to merge 6 commits into
Conversation
`gh stack sync` and `gh stack rebase` could print a full success report while leaving the stack based on the trunk it was created from. Two independent causes. 1. A rebase git refused was reported as a success. `tryAutoResolveRebase` returned nil whenever no rebase was in progress. That is only a valid success signal after an auto-`--continue`; on the first check it means `git rebase` exited non-zero without ever starting. Every rebase funnels through it, so the cascade printed `✓ Rebased X onto Y` for a rebase that never ran — a dirty tree, a branch checked out in another worktree, an unresolvable upstream, or stale rebase state. A rebase that never started is now a typed `*git.RebaseStartError`, which the cascade and `modify` treat as fatal rather than as a conflict, so no bogus recovery state is written and git's own message is surfaced. 2. The trunk ref was never verified against the remote. `fastForwardTrunk` only warned when it could not move the local trunk (checked out in another worktree, diverged, remote ref gone), and the cascade then rebased onto the stale local trunk. Every freshness check compared branches to the *local* trunk, so they all passed: `rebase` claimed "rebased locally with main" and `sync` concluded nothing was stale, force-pushed anyway, and said "Branches synced". `resolveTrunkTarget` now resolves the ref the cascade must target. When the local trunk cannot be updated it returns `<remote>/<trunk>` and says why, so the stack ends up current regardless of why the local ref is stuck. When the trunk no longer exists on the remote it fails with an actionable message instead of silently rebasing onto a stale trunk. The post-cascade check measures against that ref, and `sync` runs it before pushing so an unrebased stack is never force-pushed. Also fixed along the way: - A remote-qualified trunk (`gh stack init --base origin/main`) is normalized instead of being re-qualified into `origin/origin/main`, in both commands and before fetch refspecs are built. - `git rebase <option> --continue` is a usage error (exit 129), so `--preserve-dates` broke every `--continue`. git persists the option in the rebase state, so `--continue` alone honors it. - `FetchBranches` reports real failures instead of `sync` printing "Fetched latest changes" regardless. - Preflight checks for a rebase in progress and a dirty tree, with `--autostash` to opt out. Untracked files are not treated as dirty: git rebases fine with them present. `--autostash` stashes once around the whole cascade — git's own `--autostash` pops after every individual rebase, landing the changes on the wrong branch. - Both summaries now name the trunk ref and SHA the stack landed on. Fixes #155, #176. Addresses discussion #215. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755
Amending a commit on a lower branch, pushing, and then rebasing replayed the old version of that commit into every branch above it — a conflict at best, a duplicated commit at worst (#250, #193). The same defect made a squash-merged bottom PR conflict against the branches above its direct child (#309). Root cause: `updateBaseSHAs` unconditionally recorded the parent's *current* tip as each child's base, even when the child had never been rebased onto it. `gh stack push` calls it, so the recipe "amend, push, rebase" corrupted the metadata before the rebase ever ran: after init: b2.base = <b1 original tip> correct after push: b2.base = <b1 amended tip> b2 does not contain this `branches[].base` is the `git rebase --onto <newBase> <upstream>` boundary for the next cascade, so recording a commit the branch does not contain makes git fall back to a merge base and replay the parent's superseded commits. `updateBaseSHAs` now only advances a base when the parent's tip really is in the branch's history, so the record keeps describing where the branch actually sits. `Head` is still always updated — it is the branch's own tip. `cascadeRebase` gains `resolveOntoOldBase`, which picks the latest boundary the branch genuinely contains: the parent's current tip, else the recorded metadata base, else a merge base. It replaces the ad-hoc staleness guard that existed only on the merged-PR path, and now covers the plain path too, which had none. Also fixes a regression from the previous commit: a trunk that was never pushed (a stack based on a local integration branch) was treated like a trunk that had been deleted upstream and failed outright. The two are now distinguished by whether the trunk has an upstream configured — a tracked trunk that has disappeared is still a hard error, an untracked one is used as-is. Verified against real git: amend at the bottom, in the middle, and at two depths at once; extra commits; a dropped commit force-pushed; squash-merge with three branches above it; and each combined with a trunk that moved, diverged, or was locked by another worktree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755
The previous commit stops `gh stack push` from recording a base the branch does not contain, but every stack that has already been through that path still carries a bad value on disk. Those stacks would keep hitting the conflict on their next rebase, because neither the parent's current tip nor the recorded base is a boundary the branch actually has. `resolveOntoOldBase` now also considers `git merge-base --fork-point`, which reads the parent's reflog and so still finds where the branch diverged after the parent was amended, rebased, or force-pushed — exactly the record the stack file lost. It is only a candidate: the ancestry check still gates it, and a fresh clone or an expired reflog simply falls through to the merge bases as before. Verified on a real stack whose metadata had been corrupted by the previous build: the rebase now completes, replaying one commit instead of two, and the recorded bases are genuine ancestors again afterwards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755
… fails Running `gh stack rebase` and then `gh stack sync` left the stack unpushed. sync derived the force flag purely from whether it had rebased in that same run, so after an earlier rebase it attempted a plain push; the branches had rewritten history, the atomic push was rejected as a whole, and sync still finished with "✓ Stack synced". The flag now also accounts for branches that already diverged from their remote-tracking refs, which is the state any earlier rebase leaves behind. --force-with-lease still protects against overwriting someone else's work. A push that does not land is also no longer reported as a synced stack: the remote does not have the commits, so nothing downstream of the push reflects the local stack. sync now says so and points at `gh stack push`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755
If the stack file changes while a rebase is paused on a conflict — a branch removed, the stack unstacked — `gh stack rebase --continue` failed with a bare "no stack found for branch X". The recovery state is still on disk at that point and `--abort` still restores everything, but nothing said so. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755
The new integration tests drive the production rebase helpers, which shell out to git without the identity environment variables gitExec sets, so creating a commit during a rebase failed on a runner with no global git identity — passing locally and on macOS but failing on ubuntu-latest. Recording the identity in the clone's own config covers every git process started in it, whatever path reaches git. Verified by running the whole suite with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755
Collaborator
Author
|
Closing in favor of #330 |
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 rebaseduplicates an amended downstack commit after pushing #250 / Stacking should handle rebases performed outside of gh stack #193gh stack syncmerged approved PRs into bottom branch #155gh stack syncandgh stack rebasecould print a complete success report while leaving the stack based on the trunk it was created from. Users consistently described it as "rebase never actually pulls in main" — the branches are rebased onto each other exactly as expected, the command exits 0, but the stack is not on the latest trunk:…while
git merge-base --is-ancestor main HEADstill saidmainwas not an ancestor.Root cause
Two independent causes, both reproduced against real git in the new integration tests.
1. A rebase that git refused was reported as a success
tryAutoResolveRebase(internal/git/git.go) — the shared error path for every rebase — returnednilwhenever no rebase was in progress:That early return is only a valid success signal after an auto-
--continue. On the first check it meansgit rebaseexited non-zero without ever starting. All of these hit it, and each madecascadeRebaseprint✓ Rebased X onto Yfor a rebase that never ran:error: cannot rebase: You have unstaged changes.fatal: 'b2' is already used by worktree at …fatal: invalid upstream 'main'fatal: It seems that there is already a rebase-merge directory…2. The trunk ref was never verified against the remote
fastForwardTrunkreturnedfalsewith only a warning when it could not move the local trunk — checked out in another worktree, diverged from the remote, or with the remote branch gone. The cascade then rebased the whole stack onto the stale local trunk.Every freshness check in the codebase —
stackNeedsRebase()and the post-rebase display — compared each branch to the local trunk ref, so they all passed.rebaseprintedAll branches in stack rebased locally with main;syncconcluded nothing was stale, skipped the rebase entirely, pushed anyway, and printedBranches synced. Nothing ever compared the stack against<remote>/<trunk>.#155's log shows both halves at once — the warning that the trunk could not be moved, immediately followed by a clean run:
Behavior
A rebase git refused is now a hard failure, not a conflict. There is nothing to
--continueor--abort, so no recovery state is written and git's own message is surfaced:When the local trunk can't be updated, the stack is rebased onto the remote-tracking ref instead, so it ends up current regardless of why the local ref is stuck. The local trunk is left untouched:
The same applies to a locally diverged trunk, which previously skipped the rebase entirely. Unpushed commits on the local trunk are respected — if the local trunk is ahead of the remote it is kept, so those commits aren't dropped from the stack's base.
A trunk that was tracked on the remote and has since been deleted is an error, instead of silently rebasing onto a stale local copy (the shape reported in #225):
A trunk that was never pushed is a different situation — a stack based on a local integration branch — and is used as-is with a warning.
syncverifies the stack is really stacked before pushing. An unrebased stack is never force-pushed over the remote.Both summaries name the ref and SHA the stack landed on, so a stale run is visible at a glance —
All branches in stack rebased locally with origin/main (82a197b), andStacked on main (a1b2c3d)forsync.Preflight checks with
--autostash. Both commands now fail up front on a rebase in progress or uncommitted changes to tracked files, rather than letting every branch in the cascade fail for the same reason. Untracked files are deliberately not treated as dirty —git rebaseruns fine with them present.--autostashopts out, stashing once around the whole cascade and restoring afterwards (through--continue/--abortif a conflict interrupts it).Also fixed: replayed commits when a parent moves out of band
Verifying the above against real repositories turned up a second, unrelated defect in the same cascade — reported as #250 and #193, and responsible for the conflicts in discussion #309. Amending a commit on a lower branch, pushing, then rebasing replayed the old version of that commit into every branch above it.
The cause was not in the rebase at all — it was in the metadata.
updateBaseSHAsunconditionally recorded the parent's current tip as each child's base, even when the child had never been rebased onto it.gh stack pushcalls it, so the recipe "amend, push, rebase" corrupted the record before the rebase ever ran:branches[].baseis thegit rebase --onto <newBase> <upstream>boundary for the next cascade, so recording a commit the branch does not contain makes git fall back to a merge base and replay the parent's superseded commits.updateBaseSHAsnow only advances a base when the parent's tip really is in the branch's history.Headis still always updated — it is the branch's own tip.cascadeRebasegainsresolveOntoOldBase, which picks the latest boundary the branch genuinely contains: the parent's current tip, else the recorded metadata base, elsegit merge-base --fork-point, else a merge base. It replaces the ad-hoc staleness guard that existed only on the merged-PR path — the plain path had none.--fork-pointcandidate reads the parent's reflog, so stacks whose metadata was already corrupted by an earlier version heal on their next rebase rather than conflicting one more time.Also fixed: sync leaving the stack unpushed
Running
gh stack rebaseand thengh stack syncleft the branches unpushed. sync derived its force flag purely from whether it had rebased in that same run, so after an earlier rebase it attempted a plain push, the atomic push was rejected as a whole, and sync still finished with✓ Stack synced.The flag now also accounts for branches that already diverged from their remote-tracking refs;
--force-with-leasestill protects against overwriting someone else's work. A push that does not land is no longer reported as a synced stack — sync says so and points atgh stack push.Changes
internal/git/git.go:RebaseStartError+IsRebaseStartError— a typed error for a rebase that failed before creating any rebase state.tryAutoResolveRebasereturns it wheni == 0instead ofnil.runRebaseCommand— shared entry point forRebase/RebaseOntowith an up-frontIsRebaseInProgress()guard, so leftover state from an earlier rebase isn't mistaken for this rebase's conflicts.rebaseContinueOnceno longer passes rebase options.git rebase <option> --continueis a usage error (exit 129), which made--preserve-datesbreak every--continue; git persists the option in.git/rebase-merge, so--continuealone honors it.HasUncommittedTrackedChanges,StashPush,StashPop,UpstreamRemote,MergeBaseForkPointadded toOps(+MockOps).internal/git/gitops.go:FetchBranchesreturns real failures instead of alwaysnil, tolerating onlycouldn't find remote ref(a branch never pushed, or deleted upstream).cmd/utils.go:resolveTrunkTarget(new, replacesfastForwardTrunk) returns thetrunkTargetthe cascade must rebase onto: normalizes a remote-qualified trunk, ensures the local trunk exists, fast-forwards it, and falls back to<remote>/<trunk>when the local ref can't be moved.trunkWithoutRemotedistinguishes a trunk deleted upstream from one that was never pushed.normalizeTrunkBranch/normalizeStackTrunkstrip a leading<remote>/from the recorded trunk name, fixing theorigin/origin/mainfailure in gh stack rebase doubles remote prefix for remote-qualified trunk #176. Only applied when no local branch is literally named that way.verifyStacked(generalizesstackNeedsRebase) takes the trunk ref explicitly and returns the branches that don't have their effective parent in history. Doubles as the post-cascade post-condition.updateBaseSHAsonly advances a base that the branch actually contains;resolveOntoOldBasepicks the rebase upstream.cascadeRebasetakesTrunkRef, and treatsRebaseStartErrorasErr(fatal) rather thanConflicted.preflightRebase,stashForRebase,restoreStash— the new preflight and stash helpers.cmd/rebase.go/cmd/sync.go:syncverifies before pushing, normalizes the trunk name before building fetch refspecs, and force-pushes branches that already diverged (branchesNeedForcePush).rebaseStaterecordstrunkRef, the rebased range, and whether a stash is pending, so--continueand--abortresume against the same target and restore the stash.--autostashflag on both.continueRebasealso points at--abortwhen the stack file changed under a paused rebase, instead of failing with a bare "no stack found".internal/modify/apply.go:ApplyPlanandContinueApplytreatRebaseStartErroras fatal instead of recording conflict state.Testing
internal/git/rebase_start_test.go(new) — real-git integration tests: dirty tree, branch in another worktree, unknown upstream, and a rebase already in progress all produce aRebaseStartErrorand leave the branch untouched; a real conflict does not;--continueworks with--committer-date-is-author-date; untracked files aren't reported as dirty and don't block a rebase; stash push/pop round-trips around a cascade;FetchBrancheserror reporting.cmd/trunk_target_test.go(new) —normalizeTrunkBranch, everyresolveTrunkTargetoutcome (up to date, fast-forward,MergeFFon trunk, worktree-locked fallback, diverged fallback, local-ahead, tracked-but-deleted, never-pushed, absent entirely, remote-qualified),verifyStacked,preflightRebase,stashForRebase.cmd/onto_base_test.go(new) —resolveOntoOldBaseacross recorded / metadata / fork-point / merge-base boundaries, andupdateBaseSHAsadvancing, preserving, and initialising a base.cmd/stale_trunk_test.go(new) — end-to-end command tests:rebaseandsynctargetorigin/mainwhen the trunk is worktree-locked; an unstacked cascade reports failure instead of a success summary;syncdoesn't push an unrebased stack and does force-push branches rebased earlier; aRebaseStartErroris fatal and writes no rebase state;origin/maintrunks are normalized; a deleted trunk fails while a local-only one still rebases;--autostashstashes once, restores once, and keeps the stash on conflict.Existing
sync_test.go/rebase_test.gomocks were made directional via a sharedancestorMockhelper — several previously reported ancestry as true in both directions, or modelled a rebase that never lands.TestSync_TrunkDivergednow asserts the new behavior (rebase ontoorigin/main) rather than the old skip.go vet ./...is clean and the fullgo test -race -count=1 ./...suite passes.