Skip to content

Commit 700d9cf

Browse files
skarimCopilot
andcommitted
Stop sync/rebase reporting success on a stale trunk
`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
1 parent 53ed88c commit 700d9cf

15 files changed

Lines changed: 1970 additions & 224 deletions

File tree

cmd/rebase.go

Lines changed: 111 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ type rebaseOptions struct {
2424
noTrunk bool
2525
remote string
2626
committerDateIsAuthorDate bool
27+
autostash bool
2728
}
2829

2930
type rebaseState struct {
@@ -36,6 +37,11 @@ type rebaseState struct {
3637
OntoOldBase string `json:"ontoOldBase,omitempty"`
3738
CommitterDateIsAuthorDate bool `json:"committerDateIsAuthorDate,omitempty"`
3839
NoTrunk bool `json:"noTrunk,omitempty"`
40+
AutoStash bool `json:"autoStash,omitempty"`
41+
Stashed bool `json:"stashed,omitempty"`
42+
TrunkRef string `json:"trunkRef,omitempty"`
43+
StartIndex int `json:"startIndex,omitempty"`
44+
EndIndex int `json:"endIndex,omitempty"`
3945
}
4046

4147
const rebaseStateFile = "gh-stack-rebase-state"
@@ -51,6 +57,16 @@ func RebaseCmd(cfg *config.Config) *cobra.Command {
5157
Ensures that each branch in the stack has the tip of the previous
5258
layer in its commit history, rebasing if necessary.
5359
60+
Requires no rebase in progress and no uncommitted changes to tracked
61+
files, since git refuses to rebase otherwise. Untracked files are fine.
62+
Use --autostash to stash your changes for the duration of the cascade
63+
and restore them afterwards.
64+
65+
If the local trunk branch cannot be brought up to date — because it is
66+
checked out in another worktree, or has diverged from the remote — the
67+
stack is rebased onto the remote-tracking branch instead, so it still
68+
ends up current.
69+
5470
Use --no-trunk to skip fetching and rebasing with the trunk branch.
5571
Only the inter-branch rebases are performed (branch 2 onto branch 1,
5672
branch 3 onto branch 2, etc.).`,
@@ -66,6 +82,9 @@ branch 3 onto branch 2, etc.).`,
6682
# Rebase stack branches without pulling from or rebasing with trunk
6783
$ gh stack rebase --no-trunk
6884
85+
# Rebase with uncommitted changes in the working tree
86+
$ gh stack rebase --autostash
87+
6988
# Continue after resolving conflicts
7089
$ gh stack rebase --continue
7190
@@ -88,11 +107,12 @@ branch 3 onto branch 2, etc.).`,
88107
cmd.Flags().StringVar(&opts.remote, "remote", "", "Remote to fetch from (defaults to auto-detected remote)")
89108
cmd.Flags().BoolVar(&opts.committerDateIsAuthorDate, "committer-date-is-author-date", false, "Set the committer date to the author date during rebase")
90109
cmd.Flags().BoolVar(&opts.committerDateIsAuthorDate, "preserve-dates", false, "Alias for --committer-date-is-author-date")
110+
cmd.Flags().BoolVar(&opts.autostash, "autostash", false, "Stash uncommitted changes before rebasing and restore them afterwards")
91111

92112
return cmd
93113
}
94114

95-
func runRebase(cfg *config.Config, opts *rebaseOptions) error {
115+
func runRebase(cfg *config.Config, opts *rebaseOptions) (rerr error) {
96116
gitDir, err := git.GitDir()
97117
if err != nil {
98118
cfg.Errorf("not a git repository")
@@ -120,11 +140,38 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
120140
s := result.Stack
121141
currentBranch := result.CurrentBranch
122142

143+
// git refuses to rebase when another rebase is in progress or tracked files
144+
// are dirty. Fail up front with an actionable message instead of letting
145+
// every branch in the cascade fail for the same reason.
146+
if err := preflightRebase(cfg, "rebase", opts.autostash); err != nil {
147+
return err
148+
}
149+
150+
// Stash once around the whole cascade rather than per rebase, so local
151+
// changes survive intact and are restored on the branch they came from.
152+
stashed := false
153+
if opts.autostash {
154+
var stashErr error
155+
stashed, stashErr = stashForRebase(cfg)
156+
if stashErr != nil {
157+
cfg.Errorf("%s", stashErr)
158+
return ErrSilent
159+
}
160+
}
161+
// A conflict hands control back to the user, so --continue or --abort
162+
// restores the stash instead. Every other exit path restores it here.
163+
defer func() {
164+
if stashed && !errors.Is(rerr, ErrConflict) {
165+
restoreStash(cfg)
166+
}
167+
}()
168+
123169
// Enable git rerere so conflict resolutions are remembered.
124170
if err := ensureRerere(cfg); errors.Is(err, errInterrupt) {
125171
return ErrSilent
126172
}
127173

174+
var trunk trunkTarget
128175
if !opts.noTrunk {
129176
// Resolve remote for fetch and trunk comparison
130177
remote, err := pickRemote(cfg, currentBranch, opts.remote)
@@ -141,15 +188,15 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
141188
cfg.Successf("Fetched %s", remote)
142189
}
143190

144-
// Ensure trunk exists locally before fast-forward or cascade rebase.
145-
if err := ensureLocalTrunk(cfg, s.Trunk.Branch, remote); err != nil {
146-
cfg.Errorf("%s", err)
147-
return ErrSilent
191+
// Resolve the ref the cascade rebases the bottom branch onto. This
192+
// creates the local trunk if missing, fast-forwards it when possible,
193+
// and falls back to the remote-tracking ref when the local ref cannot
194+
// be moved.
195+
trunk, err = resolveTrunkTarget(cfg, s, remote, currentBranch)
196+
if err != nil {
197+
return err
148198
}
149199

150-
// Fast-forward trunk so the cascade rebase targets the latest upstream.
151-
fastForwardTrunk(cfg, s.Trunk.Branch, remote, currentBranch)
152-
153200
// Fast-forward stack branches that are behind their remote tracking branch.
154201
fastForwardBranches(cfg, s, remote, currentBranch)
155202
}
@@ -222,6 +269,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
222269
NeedsOnto: needsOnto,
223270
OntoOldBase: ontoOldBase,
224271
CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate,
272+
TrunkRef: trunk.Ref,
225273
})
226274

227275
if rebaseResult.Err != nil {
@@ -242,6 +290,10 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
242290
OntoOldBase: rebaseResult.OntoOldBase,
243291
CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate,
244292
NoTrunk: opts.noTrunk,
293+
Stashed: stashed,
294+
TrunkRef: trunk.Ref,
295+
StartIndex: startIdx,
296+
EndIndex: endIdx,
245297
}
246298
if err := saveRebaseState(gitDir, state); err != nil {
247299
cfg.Warningf("failed to save rebase state: %s", err)
@@ -259,6 +311,13 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
259311

260312
_ = git.CheckoutBranch(currentBranch)
261313

314+
// The cascade reported success — verify the stack actually ended up
315+
// stacked on the trunk target before saying so.
316+
if unstacked := verifyStacked(s, trunk.Ref, startIdx, endIdx); len(unstacked) > 0 {
317+
reportUnstacked(cfg, trunkRefOrStack(trunk.Ref, s), unstacked)
318+
return ErrSilent
319+
}
320+
262321
updateBaseSHAs(s)
263322

264323
_ = syncStackPRs(cfg, s)
@@ -284,21 +343,39 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
284343
if opts.noTrunk {
285344
cfg.Printf("%s rebased locally (without trunk)", rangeDesc)
286345
} else {
287-
cfg.Printf("%s rebased locally with %s", rangeDesc, s.Trunk.Branch)
346+
cfg.Printf("%s rebased locally with %s", rangeDesc, trunk.Describe())
288347
}
289348
cfg.Printf("To push up your changes, run `%s`",
290349
cfg.ColorCyan("gh stack push"))
291350

292351
return nil
293352
}
294353

295-
func continueRebase(cfg *config.Config, gitDir string) error {
354+
// trunkRefOrStack returns ref when set, falling back to the stack's local trunk
355+
// branch (used with --no-trunk, and for rebase state files written before the
356+
// trunk ref was recorded).
357+
func trunkRefOrStack(ref string, s *stack.Stack) string {
358+
if ref != "" {
359+
return ref
360+
}
361+
return s.Trunk.Branch
362+
}
363+
364+
func continueRebase(cfg *config.Config, gitDir string) (rerr error) {
296365
state, err := loadRebaseState(gitDir)
297366
if err != nil {
298367
cfg.Errorf("no rebase in progress")
299368
return ErrSilent
300369
}
301370

371+
// The interrupted rebase stashed the user's local changes; restore them
372+
// once this run finishes without hitting another conflict.
373+
defer func() {
374+
if state.Stashed && !errors.Is(rerr, ErrConflict) {
375+
restoreStash(cfg)
376+
}
377+
}()
378+
302379
sf, err := stack.Load(gitDir)
303380
if err != nil {
304381
cfg.Errorf("failed to load stack state: %s", err)
@@ -343,7 +420,7 @@ func continueRebase(cfg *config.Config, gitDir string) error {
343420
var baseBranch string
344421
if state.UseOnto {
345422
// The --onto path targets the first non-merged ancestor, or trunk.
346-
baseBranch = s.Trunk.Branch
423+
baseBranch = trunkRefOrStack(state.TrunkRef, s)
347424
for j := state.CurrentBranchIndex - 1; j >= 0; j-- {
348425
if !s.Branches[j].IsMerged() {
349426
baseBranch = s.Branches[j].Branch
@@ -353,7 +430,7 @@ func continueRebase(cfg *config.Config, gitDir string) error {
353430
} else if state.CurrentBranchIndex > 0 {
354431
baseBranch = s.Branches[state.CurrentBranchIndex-1].Branch
355432
} else {
356-
baseBranch = s.Trunk.Branch
433+
baseBranch = trunkRefOrStack(state.TrunkRef, s)
357434
}
358435
cfg.Successf("Rebased %s onto %s", conflictBranch, baseBranch)
359436

@@ -385,6 +462,7 @@ func continueRebase(cfg *config.Config, gitDir string) error {
385462
NeedsOnto: state.UseOnto,
386463
OntoOldBase: state.OntoOldBase,
387464
CommitterDateIsAuthorDate: state.CommitterDateIsAuthorDate,
465+
TrunkRef: state.TrunkRef,
388466
})
389467

390468
if result.Err != nil {
@@ -417,6 +495,22 @@ func continueRebase(cfg *config.Config, gitDir string) error {
417495
clearRebaseState(gitDir)
418496
_ = git.CheckoutBranch(state.OriginalBranch)
419497

498+
// Verify the rebased range actually ended up stacked. Older state files
499+
// have no recorded range, in which case fall back to the whole stack —
500+
// minus the first branch when trunk was deliberately skipped.
501+
verifyStart, verifyEnd := state.StartIndex, state.EndIndex
502+
if verifyEnd <= verifyStart {
503+
verifyStart, verifyEnd = 0, len(s.Branches)
504+
if state.NoTrunk && verifyStart < 1 {
505+
verifyStart = 1
506+
}
507+
}
508+
trunkRef := trunkRefOrStack(state.TrunkRef, s)
509+
if unstacked := verifyStacked(s, trunkRef, verifyStart, verifyEnd); len(unstacked) > 0 {
510+
reportUnstacked(cfg, trunkRef, unstacked)
511+
return ErrSilent
512+
}
513+
420514
updateBaseSHAs(s)
421515

422516
_ = syncStackPRs(cfg, s)
@@ -426,7 +520,7 @@ func continueRebase(cfg *config.Config, gitDir string) error {
426520
if state.NoTrunk {
427521
cfg.Printf("All branches in stack rebased locally (without trunk)")
428522
} else {
429-
cfg.Printf("All branches in stack rebased locally with %s", s.Trunk.Branch)
523+
cfg.Printf("All branches in stack rebased locally with %s", trunkRef)
430524
}
431525
cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`",
432526
cfg.ColorCyan("gh stack submit"))
@@ -459,6 +553,10 @@ func abortRebase(cfg *config.Config, gitDir string) error {
459553
_ = git.CheckoutBranch(state.OriginalBranch)
460554
clearRebaseState(gitDir)
461555

556+
if state.Stashed {
557+
restoreStash(cfg)
558+
}
559+
462560
if len(restoreErrors) > 0 {
463561
cfg.Warningf("Rebase aborted but some branches could not be fully restored:")
464562
for _, e := range restoreErrors {

cmd/rebase_test.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,13 +1292,7 @@ func TestRebase_FastForwardsBranchFromRemote(t *testing.T) {
12921292
}
12931293
return "sha-" + ref, nil
12941294
}
1295-
mock.IsAncestorFn = func(a, d string) (bool, error) {
1296-
// b1-local is ancestor of b1-remote → can fast-forward
1297-
if a == "b1-local-sha" && d == "b1-remote-sha" {
1298-
return true, nil
1299-
}
1300-
return false, nil
1301-
}
1295+
mock.IsAncestorFn, _ = ancestorMock([][2]string{{"b1-remote-sha", "b1-local-sha"}}, nil)
13021296
mock.UpdateBranchRefFn = func(branch, sha string) error {
13031297
updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha})
13041298
return nil
@@ -1414,9 +1408,10 @@ func TestRebase_BranchDiverged_NoFF(t *testing.T) {
14141408
return "sha-" + ref, nil
14151409
}
14161410
// Neither is ancestor of the other — diverged
1417-
mock.IsAncestorFn = func(a, d string) (bool, error) {
1418-
return false, nil
1419-
}
1411+
mock.IsAncestorFn, _ = ancestorMock([][2]string{
1412+
{"b1-local-sha", "b1-remote-sha"},
1413+
{"b1-remote-sha", "b1-local-sha"},
1414+
}, nil)
14201415
mock.UpdateBranchRefFn = func(string, string) error {
14211416
updateBranchRefCalls++
14221417
return nil

0 commit comments

Comments
 (0)