diff --git a/cmd/onto_base_test.go b/cmd/onto_base_test.go new file mode 100644 index 00000000..0a433f93 --- /dev/null +++ b/cmd/onto_base_test.go @@ -0,0 +1,243 @@ +package cmd + +import ( + "errors" + "testing" + + "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/stack" + "github.com/stretchr/testify/assert" +) + +// ancestryMock builds an IsAncestor func from an explicit linear history per +// branch: history[branch] lists the SHAs that branch contains, oldest first. +func ancestryMock(history map[string][]string) func(string, string) (bool, error) { + index := func(list []string, sha string) int { + for i, s := range list { + if s == sha { + return i + } + } + return -1 + } + return func(ancestor, descendant string) (bool, error) { + // descendant may be a branch name or a SHA within some branch. + for branch, list := range history { + if branch != descendant && index(list, descendant) < 0 { + continue + } + end := len(list) + if i := index(list, descendant); i >= 0 { + end = i + 1 + } + if index(list[:end], ancestor) >= 0 { + return true, nil + } + } + return false, nil + } +} + +func TestResolveOntoOldBase(t *testing.T) { + // b2 contains: trunk -> a-old -> b2-own + // The parent (b1) has since been amended to a-new, which b2 does NOT have. + history := map[string][]string{ + "b2": {"trunk", "a-old", "b2-own"}, + "b1": {"trunk", "a-new"}, + } + + t.Run("recorded upstream is used when the branch contains it", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{IsAncestorFn: ancestryMock(history)}) + defer restore() + + got := resolveOntoOldBase("a-old", "a-old", "b1", "b2") + assert.Equal(t, "a-old", got) + }) + + // The #250 case: the parent was amended, so its current tip is not in the + // branch's history. Falling back to a merge base would replay the parent's + // superseded commit; the recorded metadata base is the correct boundary. + t.Run("falls back to the metadata base when the parent was amended", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: ancestryMock(history), + MergeBaseFn: func(a, b string) (string, error) { return "trunk", nil }, + }) + defer restore() + + got := resolveOntoOldBase("a-new", "a-old", "b1", "b2") + assert.Equal(t, "a-old", got, + "should replay only b2's own commits, not the parent's superseded one") + }) + + t.Run("prefers the latest usable boundary", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: ancestryMock(history), + MergeBaseFn: func(a, b string) (string, error) { return "trunk", nil }, + }) + defer restore() + + // Both "trunk" and "a-old" are ancestors of b2; a-old replays fewer commits. + got := resolveOntoOldBase("a-new", "a-old", "b1", "b2") + assert.Equal(t, "a-old", got) + }) + + t.Run("falls back to a merge base when no metadata base is recorded", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: ancestryMock(history), + MergeBaseFn: func(a, b string) (string, error) { return "trunk", nil }, + }) + defer restore() + + got := resolveOntoOldBase("a-new", "", "b1", "b2") + assert.Equal(t, "trunk", got) + }) + + // Stacks written by an older version recorded the parent's amended tip as + // the child's base, so the metadata is no more usable than the recorded + // tip. git's fork-point reads the parent's reflog and still finds the real + // boundary, which lets those stacks heal on their next rebase. + t.Run("uses the fork point when the metadata base is also stale", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: ancestryMock(history), + MergeBaseFn: func(a, b string) (string, error) { return "trunk", nil }, + MergeBaseForkPointFn: func(ref, branch string) (string, error) { return "a-old", nil }, + }) + defer restore() + + // Both the recorded tip and the metadata base are the amended commit. + got := resolveOntoOldBase("a-new", "a-new", "b1", "b2") + assert.Equal(t, "a-old", got, + "the fork point is the only remaining record of where b2 diverged") + }) + + t.Run("ignores an unavailable fork point", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: ancestryMock(history), + MergeBaseFn: func(a, b string) (string, error) { return "trunk", nil }, + MergeBaseForkPointFn: func(ref, branch string) (string, error) { + return "", errors.New("no fork point found") + }, + }) + defer restore() + + got := resolveOntoOldBase("a-new", "", "b1", "b2") + assert.Equal(t, "trunk", got) + }) + + t.Run("returns the recorded value when nothing is usable", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(string, string) (bool, error) { return false, nil }, + MergeBaseFn: func(string, string) (string, error) { return "", errors.New("no merge base") }, + }) + defer restore() + + got := resolveOntoOldBase("a-new", "unrelated", "b1", "b2") + assert.Equal(t, "a-new", got) + }) + + t.Run("ignores an unusable metadata base", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: ancestryMock(history), + MergeBaseFn: func(a, b string) (string, error) { return "trunk", nil }, + }) + defer restore() + + // A metadata base from an unrelated history must not be trusted. + got := resolveOntoOldBase("a-new", "bogus", "b1", "b2") + assert.Equal(t, "trunk", got) + }) +} + +func TestUpdateBaseSHAs(t *testing.T) { + newStack := func(b2Base string) *stack.Stack { + return &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2", Base: b2Base}, + }, + } + } + + t.Run("advances the base when the branch really is stacked on the parent", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + IsAncestorFn: func(string, string) (bool, error) { return true, nil }, + }) + defer restore() + + s := newStack("stale-base") + updateBaseSHAs(s) + assert.Equal(t, "sha-b1", s.Branches[1].Base) + assert.Equal(t, "sha-b2", s.Branches[1].Head) + }) + + // The root cause of #250: `gh stack push` recorded the parent's amended tip + // as the child's base even though the child had not been rebased onto it, + // which made the next cascade replay the parent's superseded commits. + t.Run("keeps the recorded base when the parent moved out of band", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + IsAncestorFn: func(ancestor, descendant string) (bool, error) { + // b2 does not contain b1's amended tip. + if ancestor == "sha-b1" && descendant == "b2" { + return false, nil + } + return true, nil + }, + }) + defer restore() + + s := newStack("b1-original-tip") + updateBaseSHAs(s) + assert.Equal(t, "b1-original-tip", s.Branches[1].Base, + "the base must keep describing where b2 actually sits") + assert.Equal(t, "sha-b2", s.Branches[1].Head, "head is always the branch's own tip") + }) + + t.Run("records a base when none was known yet", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + IsAncestorFn: func(string, string) (bool, error) { + return false, nil + }, + }) + defer restore() + + s := newStack("") + updateBaseSHAs(s) + assert.Equal(t, "sha-b1", s.Branches[1].Base, + "with nothing recorded there is no truth to preserve") + }) + + t.Run("keeps the recorded base when ancestry cannot be determined", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + IsAncestorFn: func(string, string) (bool, error) { return false, errors.New("boom") }, + }) + defer restore() + + s := newStack("known-base") + updateBaseSHAs(s) + assert.Equal(t, "known-base", s.Branches[1].Base) + }) + + t.Run("skips merged branches", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil }, + IsAncestorFn: func(string, string) (bool, error) { return true, nil }, + }) + defer restore() + + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", Base: "frozen", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2"}, + }, + } + updateBaseSHAs(s) + assert.Equal(t, "frozen", s.Branches[0].Base, "a merged branch's base is left alone") + assert.Equal(t, "sha-main", s.Branches[1].Base, "b2 is measured against trunk") + }) +} diff --git a/cmd/rebase.go b/cmd/rebase.go index 9b79efdc..306d4f00 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -24,6 +24,7 @@ type rebaseOptions struct { noTrunk bool remote string committerDateIsAuthorDate bool + autostash bool } type rebaseState struct { @@ -36,6 +37,11 @@ type rebaseState struct { OntoOldBase string `json:"ontoOldBase,omitempty"` CommitterDateIsAuthorDate bool `json:"committerDateIsAuthorDate,omitempty"` NoTrunk bool `json:"noTrunk,omitempty"` + AutoStash bool `json:"autoStash,omitempty"` + Stashed bool `json:"stashed,omitempty"` + TrunkRef string `json:"trunkRef,omitempty"` + StartIndex int `json:"startIndex,omitempty"` + EndIndex int `json:"endIndex,omitempty"` } const rebaseStateFile = "gh-stack-rebase-state" @@ -51,6 +57,16 @@ func RebaseCmd(cfg *config.Config) *cobra.Command { Ensures that each branch in the stack has the tip of the previous layer in its commit history, rebasing if necessary. +Requires no rebase in progress and no uncommitted changes to tracked +files, since git refuses to rebase otherwise. Untracked files are fine. +Use --autostash to stash your changes for the duration of the cascade +and restore them afterwards. + +If the local trunk branch cannot be brought up to date — because it is +checked out in another worktree, or has diverged from the remote — the +stack is rebased onto the remote-tracking branch instead, so it still +ends up current. + Use --no-trunk to skip fetching and rebasing with the trunk branch. Only the inter-branch rebases are performed (branch 2 onto branch 1, branch 3 onto branch 2, etc.).`, @@ -66,6 +82,9 @@ branch 3 onto branch 2, etc.).`, # Rebase stack branches without pulling from or rebasing with trunk $ gh stack rebase --no-trunk + # Rebase with uncommitted changes in the working tree + $ gh stack rebase --autostash + # Continue after resolving conflicts $ gh stack rebase --continue @@ -88,11 +107,12 @@ branch 3 onto branch 2, etc.).`, cmd.Flags().StringVar(&opts.remote, "remote", "", "Remote to fetch from (defaults to auto-detected remote)") cmd.Flags().BoolVar(&opts.committerDateIsAuthorDate, "committer-date-is-author-date", false, "Set the committer date to the author date during rebase") cmd.Flags().BoolVar(&opts.committerDateIsAuthorDate, "preserve-dates", false, "Alias for --committer-date-is-author-date") + cmd.Flags().BoolVar(&opts.autostash, "autostash", false, "Stash uncommitted changes before rebasing and restore them afterwards") return cmd } -func runRebase(cfg *config.Config, opts *rebaseOptions) error { +func runRebase(cfg *config.Config, opts *rebaseOptions) (rerr error) { gitDir, err := git.GitDir() if err != nil { cfg.Errorf("not a git repository") @@ -120,11 +140,38 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { s := result.Stack currentBranch := result.CurrentBranch + // git refuses to rebase when another rebase is in progress or tracked files + // are dirty. Fail up front with an actionable message instead of letting + // every branch in the cascade fail for the same reason. + if err := preflightRebase(cfg, "rebase", opts.autostash); err != nil { + return err + } + + // Stash once around the whole cascade rather than per rebase, so local + // changes survive intact and are restored on the branch they came from. + stashed := false + if opts.autostash { + var stashErr error + stashed, stashErr = stashForRebase(cfg) + if stashErr != nil { + cfg.Errorf("%s", stashErr) + return ErrSilent + } + } + // A conflict hands control back to the user, so --continue or --abort + // restores the stash instead. Every other exit path restores it here. + defer func() { + if stashed && !errors.Is(rerr, ErrConflict) { + restoreStash(cfg) + } + }() + // Enable git rerere so conflict resolutions are remembered. if err := ensureRerere(cfg); errors.Is(err, errInterrupt) { return ErrSilent } + var trunk trunkTarget if !opts.noTrunk { // Resolve remote for fetch and trunk comparison remote, err := pickRemote(cfg, currentBranch, opts.remote) @@ -141,15 +188,15 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { cfg.Successf("Fetched %s", remote) } - // Ensure trunk exists locally before fast-forward or cascade rebase. - if err := ensureLocalTrunk(cfg, s.Trunk.Branch, remote); err != nil { - cfg.Errorf("%s", err) - return ErrSilent + // Resolve the ref the cascade rebases the bottom branch onto. This + // creates the local trunk if missing, fast-forwards it when possible, + // and falls back to the remote-tracking ref when the local ref cannot + // be moved. + trunk, err = resolveTrunkTarget(cfg, s, remote, currentBranch) + if err != nil { + return err } - // Fast-forward trunk so the cascade rebase targets the latest upstream. - fastForwardTrunk(cfg, s.Trunk.Branch, remote, currentBranch) - // Fast-forward stack branches that are behind their remote tracking branch. fastForwardBranches(cfg, s, remote, currentBranch) } @@ -222,6 +269,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { NeedsOnto: needsOnto, OntoOldBase: ontoOldBase, CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate, + TrunkRef: trunk.Ref, }) if rebaseResult.Err != nil { @@ -242,6 +290,10 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { OntoOldBase: rebaseResult.OntoOldBase, CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate, NoTrunk: opts.noTrunk, + Stashed: stashed, + TrunkRef: trunk.Ref, + StartIndex: startIdx, + EndIndex: endIdx, } if err := saveRebaseState(gitDir, state); err != nil { cfg.Warningf("failed to save rebase state: %s", err) @@ -259,6 +311,13 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { _ = git.CheckoutBranch(currentBranch) + // The cascade reported success — verify the stack actually ended up + // stacked on the trunk target before saying so. + if unstacked := verifyStacked(s, trunk.Ref, startIdx, endIdx); len(unstacked) > 0 { + reportUnstacked(cfg, trunkRefOrStack(trunk.Ref, s), unstacked) + return ErrSilent + } + updateBaseSHAs(s) _ = syncStackPRs(cfg, s) @@ -284,7 +343,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { if opts.noTrunk { cfg.Printf("%s rebased locally (without trunk)", rangeDesc) } else { - cfg.Printf("%s rebased locally with %s", rangeDesc, s.Trunk.Branch) + cfg.Printf("%s rebased locally with %s", rangeDesc, trunk.Describe()) } cfg.Printf("To push up your changes, run `%s`", cfg.ColorCyan("gh stack push")) @@ -292,13 +351,31 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return nil } -func continueRebase(cfg *config.Config, gitDir string) error { +// trunkRefOrStack returns ref when set, falling back to the stack's local trunk +// branch (used with --no-trunk, and for rebase state files written before the +// trunk ref was recorded). +func trunkRefOrStack(ref string, s *stack.Stack) string { + if ref != "" { + return ref + } + return s.Trunk.Branch +} + +func continueRebase(cfg *config.Config, gitDir string) (rerr error) { state, err := loadRebaseState(gitDir) if err != nil { cfg.Errorf("no rebase in progress") return ErrSilent } + // The interrupted rebase stashed the user's local changes; restore them + // once this run finishes without hitting another conflict. + defer func() { + if state.Stashed && !errors.Is(rerr, ErrConflict) { + restoreStash(cfg) + } + }() + sf, err := stack.Load(gitDir) if err != nil { cfg.Errorf("failed to load stack state: %s", err) @@ -312,7 +389,12 @@ func continueRebase(cfg *config.Config, gitDir string) error { return err } if s == nil { - return fmt.Errorf("no stack found for branch %s", state.OriginalBranch) + // The stack changed under the paused rebase. Recovery state is still on + // disk, so say how to get out of it rather than leaving a dead end. + cfg.Errorf("no stack found for branch %s — the stack may have been modified since the rebase started", state.OriginalBranch) + cfg.Printf(" Run `%s` to restore all branches to their pre-rebase state.", + cfg.ColorCyan("gh stack rebase --abort")) + return ErrSilent } // Refresh PR state before selecting the base and cascading the remaining @@ -343,7 +425,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { var baseBranch string if state.UseOnto { // The --onto path targets the first non-merged ancestor, or trunk. - baseBranch = s.Trunk.Branch + baseBranch = trunkRefOrStack(state.TrunkRef, s) for j := state.CurrentBranchIndex - 1; j >= 0; j-- { if !s.Branches[j].IsMerged() { baseBranch = s.Branches[j].Branch @@ -353,7 +435,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { } else if state.CurrentBranchIndex > 0 { baseBranch = s.Branches[state.CurrentBranchIndex-1].Branch } else { - baseBranch = s.Trunk.Branch + baseBranch = trunkRefOrStack(state.TrunkRef, s) } cfg.Successf("Rebased %s onto %s", conflictBranch, baseBranch) @@ -385,6 +467,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { NeedsOnto: state.UseOnto, OntoOldBase: state.OntoOldBase, CommitterDateIsAuthorDate: state.CommitterDateIsAuthorDate, + TrunkRef: state.TrunkRef, }) if result.Err != nil { @@ -417,6 +500,22 @@ func continueRebase(cfg *config.Config, gitDir string) error { clearRebaseState(gitDir) _ = git.CheckoutBranch(state.OriginalBranch) + // Verify the rebased range actually ended up stacked. Older state files + // have no recorded range, in which case fall back to the whole stack — + // minus the first branch when trunk was deliberately skipped. + verifyStart, verifyEnd := state.StartIndex, state.EndIndex + if verifyEnd <= verifyStart { + verifyStart, verifyEnd = 0, len(s.Branches) + if state.NoTrunk && verifyStart < 1 { + verifyStart = 1 + } + } + trunkRef := trunkRefOrStack(state.TrunkRef, s) + if unstacked := verifyStacked(s, trunkRef, verifyStart, verifyEnd); len(unstacked) > 0 { + reportUnstacked(cfg, trunkRef, unstacked) + return ErrSilent + } + updateBaseSHAs(s) _ = syncStackPRs(cfg, s) @@ -426,7 +525,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { if state.NoTrunk { cfg.Printf("All branches in stack rebased locally (without trunk)") } else { - cfg.Printf("All branches in stack rebased locally with %s", s.Trunk.Branch) + cfg.Printf("All branches in stack rebased locally with %s", trunkRef) } cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`", cfg.ColorCyan("gh stack submit")) @@ -459,6 +558,10 @@ func abortRebase(cfg *config.Config, gitDir string) error { _ = git.CheckoutBranch(state.OriginalBranch) clearRebaseState(gitDir) + if state.Stashed { + restoreStash(cfg) + } + if len(restoreErrors) > 0 { cfg.Warningf("Rebase aborted but some branches could not be fully restored:") for _, e := range restoreErrors { diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 683bfe36..e1a1cafe 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -44,9 +44,9 @@ func newRebaseMock(tmpDir string, currentBranch string) *git.MockOps { } return "sha-" + ref, nil }, - IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, - FetchFn: func(string) error { return nil }, - EnableRerereFn: func() error { return nil }, + IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, + FetchFn: func(string) error { return nil }, + EnableRerereFn: func() error { return nil }, IsRebaseInProgressFn: func() bool { return false }, } } @@ -1292,13 +1292,7 @@ func TestRebase_FastForwardsBranchFromRemote(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - // b1-local is ancestor of b1-remote → can fast-forward - if a == "b1-local-sha" && d == "b1-remote-sha" { - return true, nil - } - return false, nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"b1-remote-sha", "b1-local-sha"}}, nil) mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) return nil @@ -1414,9 +1408,10 @@ func TestRebase_BranchDiverged_NoFF(t *testing.T) { return "sha-" + ref, nil } // Neither is ancestor of the other — diverged - mock.IsAncestorFn = func(a, d string) (bool, error) { - return false, nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{ + {"b1-local-sha", "b1-remote-sha"}, + {"b1-remote-sha", "b1-local-sha"}, + }, nil) mock.UpdateBranchRefFn = func(string, string) error { updateBranchRefCalls++ return nil diff --git a/cmd/stale_trunk_test.go b/cmd/stale_trunk_test.go new file mode 100644 index 00000000..806d672d --- /dev/null +++ b/cmd/stale_trunk_test.go @@ -0,0 +1,646 @@ +package cmd + +import ( + "errors" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/stack" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// twoBranchStack is the stack shared by the tests in this file. +func twoBranchStack() stack.Stack { + return stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } +} + +// staleTrunkRevParse resolves the trunk behind its remote and every other +// branch identically on both sides, so only the trunk needs updating. +func staleTrunkRevParse(ref string) (string, error) { + switch ref { + case "main": + return "trunk-local", nil + case "origin/main": + return "trunk-remote", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil +} + +// TestRebase_TrunkLockedByWorktree_RebasesOntoRemoteRef covers issue #155 and +// the Slack report: the local trunk cannot be moved because another worktree +// has it checked out. The cascade must target origin/main instead of silently +// rebasing the stack onto the stale local trunk and reporting success. +func TestRebase_TrunkLockedByWorktree_RebasesOntoRemoteRef(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var rebaseBases []string + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.RevParseFn = staleTrunkRevParse + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"trunk-remote", "trunk-local"}}, nil) + mock.UpdateBranchRefFn = func(string, string) error { + return errors.New("cannot force update the branch 'main' used by worktree at '/elsewhere'") + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, base) + return nil + } + mock.RebaseOntoFn = func(newBase, _, _ string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, newBase) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.NoError(t, err) + require.Equal(t, []string{"origin/main", "b1"}, rebaseBases, + "the bottom branch must be rebased onto the remote-tracking ref") + assert.Contains(t, output, "worktree") + assert.Contains(t, output, "rebased locally with origin/main", + "the summary must name the ref the stack actually landed on") +} + +// TestRebase_UnstackedAfterCascade_ReportsFailure verifies the post-condition: +// if the branches are still not sitting on their parents once the cascade +// claims success, the command fails instead of printing a success summary. +func TestRebase_UnstackedAfterCascade_ReportsFailure(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + // Every rebase "succeeds" but nothing ever ends up stacked. + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"main", "b1"}}, nil) + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { return nil } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "still not based on") + assert.Contains(t, output, "b1") + assert.NotContains(t, output, "rebased locally with") +} + +// A rebase git refused to start is not a conflict: it must fail outright +// without writing recovery state that --continue would act on. +func TestRebase_RebaseStartError_IsFatalNotConflict(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { return nil } + mock.RebaseOntoFn = func(_, _, branch string, _ git.RebaseOpts) error { + return &git.RebaseStartError{ + Err: errors.New("fatal: 'b2' is already used by worktree at '/elsewhere'"), + } + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "could not start rebase of b2 onto b1") + assert.Contains(t, output, "already used by worktree") + assert.NotContains(t, output, "conflict") + + _, statErr := os.Stat(filepath.Join(tmpDir, rebaseStateFile)) + assert.True(t, os.IsNotExist(statErr), + "no rebase state should be written for a rebase that never started") +} + +// TestSync_UnstackedAfterCascade_DoesNotPush is the important half of the fix +// for sync: a stack that is not actually rebased must never be force-pushed +// over the remote. +func TestSync_UnstackedAfterCascade_DoesNotPush(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var pushCalls []pushCall + + mock := newSyncMock(tmpDir, "b1") + mock.RevParseFn = staleTrunkRevParse + // Trunk fast-forwards, but the stack never ends up on it. + mock.IsAncestorFn, _ = ancestorMock([][2]string{ + {"trunk-remote", "trunk-local"}, + {"main", "b1"}, + }, nil) + mock.UpdateBranchRefFn = func(string, string) error { return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { return nil } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { return nil } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.ErrorIs(t, err, ErrSilent) + assert.Empty(t, pushCalls, "an unrebased stack must not be force-pushed") + assert.Contains(t, output, "still not based on") + assert.NotContains(t, output, "Branches synced") +} + +// TestSync_TrunkLockedByWorktree_RebasesOntoRemoteRef is the sync counterpart of +// issue #155. Previously sync concluded nothing was stale (the branches did sit +// on the stale local trunk), skipped the rebase, and force-pushed anyway. +func TestSync_TrunkLockedByWorktree_RebasesOntoRemoteRef(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var rebaseBases []string + var pushCalls []pushCall + + mock := newSyncMock(tmpDir, "b1") + mock.RevParseFn = staleTrunkRevParse + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"trunk-remote", "trunk-local"}}, nil) + mock.UpdateBranchRefFn = func(string, string) error { + return errors.New("cannot force update the branch 'main' used by worktree at '/elsewhere'") + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, base) + return nil + } + mock.RebaseOntoFn = func(newBase, _, _ string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, newBase) + return nil + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.NoError(t, err) + require.Equal(t, []string{"origin/main", "b1"}, rebaseBases, + "sync must rebase onto the remote-tracking ref when the local trunk is locked") + require.Len(t, pushCalls, 1) + assert.True(t, pushCalls[0].force) + assert.Contains(t, output, "Stacked on origin/main") +} + +// Issue #176: a remote-qualified trunk must not be re-qualified into +// "origin/origin/main", in either command. +func TestRebase_RemoteQualifiedTrunk_IsNormalized(t *testing.T) { + s := twoBranchStack() + s.Trunk.Branch = "origin/main" + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var createdBranches []string + var rebaseBases []string + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return name != "origin/main" } + mock.CreateBranchFn = func(name, base string) error { + createdBranches = append(createdBranches, name+" from "+base) + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, base) + return nil + } + mock.RebaseOntoFn = func(newBase, _, _ string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, newBase) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + + assert.NoError(t, err) + assert.Empty(t, createdBranches, "the existing local main should be reused") + require.Equal(t, []string{"main", "b1"}, rebaseBases) + assert.NotContains(t, string(out), "origin/origin/main") +} + +// Issue #225 shape: the trunk branch was tracked on the remote and has since +// been merged and deleted, so there is nothing to bring the stack up to date +// with. That must be reported rather than silently rebasing onto a stale local +// trunk. +func TestRebase_TrunkMissingOnRemote_Fails(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var rebaseCalls int + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.UpstreamRemoteFn = func(string) (string, error) { return "origin", nil } + mock.RevParseFn = func(ref string) (string, error) { + if ref == "origin/main" { + return "", errors.New("unknown revision") + } + return "sha-" + ref, nil + } + mock.RebaseFn = func(string, git.RebaseOpts) error { rebaseCalls++; return nil } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { rebaseCalls++; return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.ErrorIs(t, err, ErrSilent) + assert.Zero(t, rebaseCalls, "nothing should be rebased onto a stale trunk") + assert.Contains(t, output, "no longer exists on origin") + assert.Contains(t, output, "--no-trunk") +} + +// A trunk that was never pushed is a local integration branch, not an orphaned +// stack: the cascade must still run against it. +func TestRebase_LocalOnlyTrunk_StillRebases(t *testing.T) { + s := twoBranchStack() + s.Trunk.Branch = "integration" + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseBases []string + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.UpstreamRemoteFn = func(string) (string, error) { return "", nil } + mock.RevParseFn = func(ref string) (string, error) { + if strings.HasPrefix(ref, "origin/") { + return "", errors.New("unknown revision") + } + return "sha-" + ref, nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, base) + return nil + } + mock.RebaseOntoFn = func(newBase, _, _ string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, newBase) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + + assert.NoError(t, err) + assert.Equal(t, []string{"integration", "b1"}, rebaseBases) + assert.Contains(t, string(out), "only exists locally") +} + +// --no-trunk still works without a reachable remote trunk. +func TestRebase_NoTrunk_SkipsTrunkResolution(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var rebaseBases []string + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.RevParseFn = func(ref string) (string, error) { + if ref == "origin/main" { + return "", errors.New("unknown revision") + } + return "sha-" + ref, nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseOntoFn = func(newBase, _, _ string, _ git.RebaseOpts) error { + rebaseBases = append(rebaseBases, newBase) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--no-trunk"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + + assert.NoError(t, err) + assert.Equal(t, []string{"b1"}, rebaseBases, "only the inter-branch rebase runs") + assert.Contains(t, string(out), "without trunk") +} + +// --autostash stashes once around the whole cascade and restores afterwards. +// git's own --autostash cannot be used: it pops after every individual rebase, +// landing the changes on whichever branch that rebase left checked out. +func TestRebase_Autostash_StashesOnceAndRestores(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var stashPushes []string + stashPops := 0 + var rebaseOpts []git.RebaseOpts + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.HasUncommittedTrackedChangesFn = func() (bool, error) { return true, nil } + mock.StashPushFn = func(msg string) error { + stashPushes = append(stashPushes, msg) + return nil + } + mock.StashPopFn = func() error { stashPops++; return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(_ string, o git.RebaseOpts) error { + rebaseOpts = append(rebaseOpts, o) + return nil + } + mock.RebaseOntoFn = func(_, _, _ string, o git.RebaseOpts) error { + rebaseOpts = append(rebaseOpts, o) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--autostash"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + + assert.NoError(t, err) + require.Len(t, stashPushes, 1, "the stack should be stashed exactly once") + assert.Equal(t, stashMessage, stashPushes[0]) + assert.Equal(t, 1, stashPops, "the stash should be restored exactly once") + assert.Len(t, rebaseOpts, 2) + assert.Contains(t, string(out), "Restored stashed changes") +} + +// On conflict the stash stays put: --continue and --abort restore it once the +// user is done, rather than popping it into a half-rebased working tree. +func TestRebase_Autostash_KeepsStashOnConflict(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + stashPops := 0 + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(string) bool { return true } + mock.HasUncommittedTrackedChangesFn = func() (bool, error) { return true, nil } + mock.StashPushFn = func(string) error { return nil } + mock.StashPopFn = func() error { stashPops++; return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { return errors.New("conflict") } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--autostash"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + assert.ErrorIs(t, err, ErrConflict) + assert.Zero(t, stashPops, "the stash must survive a conflict") + + state, loadErr := loadRebaseState(tmpDir) + require.NoError(t, loadErr) + assert.True(t, state.Stashed, "recovery state must record the pending stash") + assert.Equal(t, "main", state.TrunkRef, "recovery must resume against the same trunk ref") +} + +// A `gh stack rebase` before `gh stack sync` leaves the branches diverged from +// their remote refs. sync used to derive the force flag purely from whether it +// had rebased in that same run, so the atomic push failed and the stack stayed +// unpushed while sync still reported success. +func TestSync_ForcePushesBranchesRebasedEarlier(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + var pushCalls []pushCall + + mock := newSyncMock(tmpDir, "b1") + mock.RevParseFn = func(ref string) (string, error) { + if ref == "main" || ref == "origin/main" { + return "trunk-sha", nil + } + // The local branches carry rewritten history. + if strings.HasPrefix(ref, "origin/") { + return "old-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "new-" + ref, nil + } + // The stack is already correctly stacked, so no rebase happens this run, + // but the remote tips are not contained in the local branches. + mock.IsAncestorFn = func(ancestor, descendant string) (bool, error) { + if strings.HasPrefix(ancestor, "old-") { + return false, nil + } + return true, nil + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + + assert.NoError(t, err) + require.Len(t, pushCalls, 1) + assert.True(t, pushCalls[0].force, + "branches rebased before this run still need --force-with-lease") + assert.Contains(t, string(out), "Pushed") +} + +// When the push does not land, the stack on the remote does not reflect the +// local one, so sync must not sign off as if it did. +func TestSync_PushFailure_DoesNotReportSuccess(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, twoBranchStack()) + + mock := newSyncMock(tmpDir, "b1") + mock.PushFn = func(string, []string, bool, bool) error { + return errors.New("remote rejected") + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.NoError(t, err, "a failed push is a warning, not a fatal error") + assert.Contains(t, output, "were not pushed") + assert.NotContains(t, output, "Stack synced") + assert.NotContains(t, output, "Branches synced") +} + +func TestBranchesNeedForcePush(t *testing.T) { + tests := []struct { + name string + ancestor func(string, string) (bool, error) + revParse func(string) (string, error) + want bool + }{ + { + name: "remote tip contained locally needs no force", + ancestor: func(string, string) (bool, error) { return true, nil }, + want: false, + }, + { + name: "rewritten history needs force", + ancestor: func(string, string) (bool, error) { return false, nil }, + want: true, + }, + { + name: "a branch with no remote ref is ignored", + revParse: func(string) (string, error) { return "", errors.New("unknown revision") }, + ancestor: func(string, string) (bool, error) { return false, nil }, + want: false, + }, + { + name: "unknown ancestry does not force", + ancestor: func(string, string) (bool, error) { return false, errors.New("boom") }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + revParse := tt.revParse + if revParse == nil { + revParse = func(ref string) (string, error) { return "sha-" + ref, nil } + } + restore := git.SetOps(&git.MockOps{ + RevParseFn: revParse, + IsAncestorFn: tt.ancestor, + }) + defer restore() + + assert.Equal(t, tt.want, branchesNeedForcePush("origin", []string{"b1", "b2"})) + }) + } +} diff --git a/cmd/sync.go b/cmd/sync.go index 34bbbc05..34f78db6 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -14,8 +14,9 @@ import ( ) type syncOptions struct { - remote string - prune bool + remote string + prune bool + autostash bool } func SyncCmd(cfg *config.Config) *cobra.Command { @@ -39,6 +40,17 @@ This command performs a safe synchronization: 7. Links the stack's open PRs into a stack on GitHub (creating or updating the remote stack object) when two or more PRs exist +Requires no rebase in progress and no uncommitted changes to tracked files, +since git refuses to rebase otherwise. Untracked files are fine. Use +--autostash to stash your changes for the duration of the sync and restore +them afterwards. + +If the local trunk branch cannot be brought up to date — because it is +checked out in another worktree, or has diverged from the remote — the +stack is rebased onto the remote-tracking branch instead. Branches are only +pushed once sync has verified that each one really does sit on top of its +parent. + If PRs have been added to the stack on GitHub, their branches are pulled down and appended to your local stack so it mirrors the remote. A clean "remote is ahead" update happens automatically without prompting. If the @@ -70,6 +82,7 @@ the first active branch in the stack, or the trunk if all are merged.`, cmd.Flags().StringVar(&opts.remote, "remote", "", "Remote to fetch from and push to (defaults to auto-detected remote)") cmd.Flags().BoolVar(&opts.prune, "prune", false, "Delete local branches for merged PRs") + cmd.Flags().BoolVar(&opts.autostash, "autostash", false, "Stash uncommitted changes before rebasing and restore them afterwards") return cmd } @@ -86,6 +99,27 @@ func runSync(cfg *config.Config, opts *syncOptions) error { return ErrModifyRecovery } + // git refuses to rebase when another rebase is in progress or tracked files + // are dirty, and sync has no interactive conflict recovery, so check before + // touching any refs. + if err := preflightRebase(cfg, "sync", opts.autostash); err != nil { + return err + } + + // Stash once around the whole run. sync has no interactive conflict + // recovery — it restores the branches itself — so the stash is always + // popped before returning. + if opts.autostash { + stashed, stashErr := stashForRebase(cfg) + if stashErr != nil { + cfg.Errorf("%s", stashErr) + return ErrSilent + } + if stashed { + defer restoreStash(cfg) + } + } + sf := result.StackFile s := result.Stack currentBranch := result.CurrentBranch @@ -106,10 +140,17 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } // Fetch trunk + active branches so tracking refs are current for - // fast-forward detection (Step 2) and --force-with-lease (Step 4). + // fast-forward detection (Step 2) and --force-with-lease (Step 4). The + // trunk name is normalized first so a remote-qualified trunk does not + // produce a "origin/origin/main" refspec. + normalizeStackTrunk(cfg, s, remote) fetchTargets := append([]string{s.Trunk.Branch}, activeBranchNames(s)...) - _ = git.FetchBranches(remote, fetchTargets) - cfg.Successf("Fetched latest changes from %s", remote) + if err := git.FetchBranches(remote, fetchTargets); err != nil { + cfg.Warningf("Failed to fetch from %s: %v", remote, err) + cfg.Printf(" Continuing with the refs already available locally, which may be out of date.") + } else { + cfg.Successf("Fetched latest changes from %s", remote) + } // --- Step 1b: Reconcile remote-ahead stack changes --- // Pull in branches for PRs that were added to the stack on GitHub, or @@ -139,9 +180,16 @@ func runSync(cfg *config.Config, opts *syncOptions) error { currentBranch = cb } - // --- Step 2: Fast-forward trunk --- - trunk := s.Trunk.Branch - trunkUpdated := fastForwardTrunk(cfg, trunk, remote, currentBranch) + // --- Step 2: Resolve and fast-forward trunk --- + // Resolves the ref the cascade rebases the bottom branch onto: the local + // trunk when it can be brought up to date, otherwise the remote-tracking + // ref, so a trunk that cannot be moved never leaves the stack silently + // based on a stale commit. + trunk, err := resolveTrunkTarget(cfg, s, remote, currentBranch) + if err != nil { + return err + } + trunkUpdated := trunk.Moved // --- Step 2b: Fast-forward stack branches behind their remote tracking branch --- updatedBranches := fastForwardBranches(cfg, s, remote, currentBranch) @@ -150,7 +198,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // --- Step 3: Cascade rebase --- // Rebase if trunk or any branch moved, or if the stack is stale // (branches not yet rebased onto their parent's current tip). - needsRebase := trunkUpdated || branchesUpdated || stackNeedsRebase(s) + needsRebase := trunkUpdated || branchesUpdated || stackNeedsRebase(s, trunk.Ref) rebased := false if needsRebase { cfg.Printf("") @@ -169,6 +217,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { Branches: s.Branches, StartAbsIdx: 0, OriginalRefs: originalRefs, + TrunkRef: trunk.Ref, }) if result.Err != nil { @@ -204,9 +253,20 @@ func runSync(cfg *config.Config, opts *syncOptions) error { _ = git.CheckoutBranch(currentBranch) } + // Never push branches that are not actually stacked on the trunk target. A + // rebase that reported success but left the stack where it was must not be + // force-pushed over the remote. + if unstacked := verifyStacked(s, trunk.Ref, 0, len(s.Branches)); len(unstacked) > 0 { + _ = git.CheckoutBranch(currentBranch) + reportUnstacked(cfg, trunk.Ref, unstacked) + stack.SaveNonBlocking(gitDir, sf) + return ErrSilent + } + // --- Step 4: Push --- cfg.Printf("") branches := activeBranchNames(s) + pushed := true if mergedCount := len(s.MergedBranches()); mergedCount > 0 { cfg.Printf("Skipping %d merged %s", mergedCount, plural(mergedCount, "branch", "branches")) @@ -218,11 +278,14 @@ func runSync(cfg *config.Config, opts *syncOptions) error { if len(branches) == 0 { cfg.Printf("No active branches to push (all merged)") } else { - // After rebase, force-with-lease is required (history rewritten). - // Without rebase, try a normal push first. - force := rebased + // Rewritten history needs --force-with-lease. That is not only true of + // a rebase this run performed: a `gh stack rebase` beforehand leaves + // exactly the same divergence, and a plain push then fails for the + // whole atomic set. + force := rebased || branchesNeedForcePush(remote, branches) cfg.Printf("Pushing %d %s to %s...", len(branches), plural(len(branches), "branch", "branches"), remote) if err := git.Push(remote, branches, force, true); err != nil { + pushed = false if !force { cfg.Warningf("Push failed — branches may need force push after rebase") cfg.Printf(" Run `%s` to push with --force-with-lease.", @@ -329,7 +392,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } } if needsSwitch { - switchTarget := trunk + switchTarget := trunk.Branch for _, b := range s.Branches { if !b.IsSkipped() { switchTarget = b.Branch @@ -377,17 +440,45 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } cfg.Printf("") - if stackSynced { + switch { + case !pushed: + // The branches were rebased and the PR state refreshed, but the remote + // does not have the new commits, so nothing downstream of the push + // actually reflects the local stack yet. + cfg.Warningf("Synced locally, but the branches were not pushed") + cfg.Printf(" Run `%s` to push them.", cfg.ColorCyan("gh stack push")) + case stackSynced: cfg.Successf("Stack synced") - } else { + default: // The branches were fetched, rebased, and pushed, but no stack object on // GitHub was created or updated (no PRs, fewer than two PRs, stacked PRs // unavailable, or a divergence). Report only what actually happened. cfg.Successf("Branches synced") } + cfg.Printf(" Stacked on %s", trunk.Describe()) return nil } +// branchesNeedForcePush reports whether pushing any of the branches would +// rewrite history on the remote, which requires --force-with-lease. +// +// A branch qualifies when its remote-tracking ref is not contained in the local +// branch — the state a rebase leaves behind. Branches with no remote ref yet, +// or whose ancestry cannot be determined, do not qualify: a plain push is the +// safer default and its failure is reported. +func branchesNeedForcePush(remote string, branches []string) bool { + for _, b := range branches { + remoteSHA, err := git.RevParse(remote + "/" + b) + if err != nil { + continue + } + if isAnc, err := git.IsAncestor(remoteSHA, b); err == nil && !isAnc { + return true + } + } + return false +} + // restoreBranches resets each branch to its original SHA, collecting any errors. func restoreBranches(originalRefs map[string]string) []string { var errors []string diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 4f76d8e7..bf50c882 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -139,21 +139,19 @@ func TestSync_TrunkUpToDate_StackStale(t *testing.T) { } return "sha-" + ref, nil } - // Stack branches are NOT rebased onto trunk — parent is not an ancestor. - mock.IsAncestorFn = func(a, d string) (bool, error) { - // main is NOT an ancestor of b1 → stack is stale - if a == "main" && d == "b1" { - return false, nil - } - return true, nil - } + // Stack branches are NOT rebased onto trunk — parent is not an ancestor + // until the cascade runs. + isAncestor, markRebased := ancestorMock(nil, [][2]string{{"main", "b1"}}) + mock.IsAncestorFn = isAncestor mock.CheckoutBranchFn = func(string) error { return nil } mock.RebaseFn = func(base string, opts git.RebaseOpts) error { rebaseCalls = append(rebaseCalls, rebaseCall{branch: "(rebase)" + base}) + markRebased() return nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + markRebased() return nil } mock.PushFn = func(remote string, branches []string, force, atomic bool) error { @@ -219,13 +217,7 @@ func TestSync_TrunkFastForward_TriggersRebase(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - // local is ancestor of remote → can fast-forward - if a == "local-sha" && d == "remote-sha" { - return true, nil - } - return true, nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"remote-sha", "local-sha"}}, nil) mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) return nil @@ -298,11 +290,13 @@ func TestSync_TrunkFastForward_WhenOnTrunk(t *testing.T) { if ref == "origin/main" { return "remote-sha", nil } + // Stack branches match their remote — no branch fast-forward. + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"remote-sha", "local-sha"}}, nil) mock.MergeFFFn = func(target string) error { mergeFFCalls = append(mergeFFCalls, target) return nil @@ -333,8 +327,9 @@ func TestSync_TrunkFastForward_WhenOnTrunk(t *testing.T) { assert.Empty(t, updateBranchRefCalls, "should NOT use UpdateBranchRef when on trunk") } -// TestSync_TrunkDiverged verifies that when trunk has diverged from origin, -// no rebase occurs and a warning is shown. +// TestSync_TrunkDiverged verifies that when the local trunk has diverged from +// its remote, sync warns and rebases the stack onto the remote-tracking ref +// rather than silently leaving it on the stale local trunk. func TestSync_TrunkDiverged(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, @@ -346,7 +341,7 @@ func TestSync_TrunkDiverged(t *testing.T) { tmpDir := t.TempDir() writeStackFile(t, tmpDir, s) - var rebaseCalls []rebaseCall + var rebaseBases []string var pushCalls []pushCall mock := newSyncMock(tmpDir, "b1") @@ -363,20 +358,14 @@ func TestSync_TrunkDiverged(t *testing.T) { } return "sha-" + ref, nil } - // Neither is ancestor of the other → diverged (for trunk FF check) - // But stack branches DO have local trunk as ancestor (for stackNeedsRebase) - mock.IsAncestorFn = func(a, d string) (bool, error) { - if a == "local-sha" && d == "remote-sha" { - return false, nil - } - if a == "remote-sha" && d == "local-sha" { - return false, nil - } - // Stack branches have their parent as ancestor - return true, nil - } - mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { - rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + // Neither trunk SHA is an ancestor of the other → diverged. + mock.IsAncestorFn, _ = ancestorMock([][2]string{ + {"local-sha", "remote-sha"}, + {"remote-sha", "local-sha"}, + }, nil) + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string, opts git.RebaseOpts) error { + rebaseBases = append(rebaseBases, base) return nil } mock.PushFn = func(remote string, branches []string, force, atomic bool) error { @@ -399,11 +388,15 @@ func TestSync_TrunkDiverged(t *testing.T) { assert.NoError(t, err) assert.Contains(t, output, "diverged") - assert.Empty(t, rebaseCalls, "no rebase should occur when trunk diverged") + assert.Contains(t, output, "origin/main") + + // The stack is rebased onto the remote-tracking ref, not the stale local + // trunk, so it still ends up current. + require.Equal(t, []string{"origin/main"}, rebaseBases, + "stack should be rebased onto origin/main when local trunk diverged") - // Push should happen without force (no rebase occurred) require.Len(t, pushCalls, 1) - assert.False(t, pushCalls[0].force, "push should not use force when no rebase") + assert.True(t, pushCalls[0].force, "push should use force after the rebase") } // TestSync_NoLocalTrunk_SkipsSilently verifies that when the trunk branch @@ -624,9 +617,7 @@ func TestSync_PushForceFlagDependsOnRebase(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"remote-sha", "local-sha"}}, nil) mock.UpdateBranchRefFn = func(string, string) error { return nil } } else { mock.RevParseFn = func(ref string) (string, error) { @@ -779,6 +770,7 @@ func TestSync_QueuedBranch_DownstreamStaysStacked(t *testing.T) { } return "sha-" + ref, nil } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"remote-sha", "local-sha"}}, nil) mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(string) error { return nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { @@ -937,9 +929,7 @@ func TestSync_PushFailureAfterRebase(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"remote-sha", "local-sha"}}, nil) mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(string) error { return nil } mock.RebaseFn = func(string, git.RebaseOpts) error { return nil } @@ -1005,12 +995,7 @@ func TestSync_BranchFastForward_TriggersRebase(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - if a == "b1-local-sha" && d == "b1-remote-sha" { - return true, nil - } - return false, nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"b1-remote-sha", "b1-local-sha"}}, nil) mock.MergeFFFn = func(target string) error { mergeFFCalls = append(mergeFFCalls, target) return nil @@ -1095,15 +1080,10 @@ func TestSync_BranchFastForward_WithTrunkUpdate(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { - if a == "trunk-local" && d == "trunk-remote" { - return true, nil - } - if a == "b2-local" && d == "b2-remote" { - return true, nil - } - return false, nil - } + mock.IsAncestorFn, _ = ancestorMock([][2]string{ + {"trunk-remote", "trunk-local"}, + {"b2-remote", "b2-local"}, + }, nil) mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) return nil diff --git a/cmd/trunk_target_test.go b/cmd/trunk_target_test.go new file mode 100644 index 00000000..d58e1571 --- /dev/null +++ b/cmd/trunk_target_test.go @@ -0,0 +1,495 @@ +package cmd + +import ( + "errors" + "io" + "strings" + "testing" + + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/stack" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNormalizeTrunkBranch(t *testing.T) { + tests := []struct { + name string + trunk string + remote string + localExists map[string]bool + want string + }{ + { + name: "plain branch is unchanged", + trunk: "main", + remote: "origin", + want: "main", + }, + { + name: "remote-qualified trunk is stripped", + trunk: "origin/main", + remote: "origin", + want: "main", + }, + { + name: "non-default remote is stripped", + trunk: "upstream/trunk", + remote: "upstream", + want: "trunk", + }, + { + name: "prefix of a different remote is kept", + trunk: "upstream/main", + remote: "origin", + want: "upstream/main", + }, + { + name: "a real local branch named like the remote ref is kept", + trunk: "origin/main", + remote: "origin", + localExists: map[string]bool{"origin/main": true}, + want: "origin/main", + }, + { + name: "bare remote name is kept", + trunk: "origin/", + remote: "origin", + want: "origin/", + }, + { + name: "no remote resolved", + trunk: "origin/main", + remote: "", + want: "origin/main", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + BranchExistsFn: func(name string) bool { return tt.localExists[name] }, + }) + defer restore() + + assert.Equal(t, tt.want, normalizeTrunkBranch(tt.trunk, tt.remote)) + }) + } +} + +// trunkTargetMock builds a git mock for resolveTrunkTarget with the given +// local/remote trunk SHAs. +func trunkTargetMock(localSHA, remoteSHA string) *git.MockOps { + return &git.MockOps{ + BranchExistsFn: func(string) bool { return true }, + RevParseFn: func(ref string) (string, error) { + switch ref { + case "main": + return localSHA, nil + case "origin/main": + if remoteSHA == "" { + return "", errors.New("unknown revision") + } + return remoteSHA, nil + } + return "sha-" + ref, nil + }, + } +} + +func TestResolveTrunkTarget(t *testing.T) { + t.Run("already up to date", func(t *testing.T) { + mock := trunkTargetMock("same", "same") + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, "main", target.Ref) + assert.False(t, target.Moved) + assert.False(t, target.Detached) + }) + + t.Run("fast-forwards the local trunk", func(t *testing.T) { + mock := trunkTargetMock("old", "new") + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"new", "old"}}, nil) + var updated []string + mock.UpdateBranchRefFn = func(branch, sha string) error { + updated = append(updated, branch+"="+sha) + return nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, []string{"main=new"}, updated) + assert.Equal(t, "main", target.Ref) + assert.Equal(t, "new", target.SHA) + assert.True(t, target.Moved) + assert.False(t, target.Detached) + }) + + t.Run("uses MergeFF when trunk is checked out", func(t *testing.T) { + mock := trunkTargetMock("old", "new") + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"new", "old"}}, nil) + var mergeFF []string + mock.MergeFFFn = func(target string) error { + mergeFF = append(mergeFF, target) + return nil + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "main") + require.NoError(t, err) + assert.Equal(t, []string{"origin/main"}, mergeFF) + assert.Equal(t, "main", target.Ref) + }) + + // The regression behind issue #155 and the Slack report: the local trunk + // ref cannot be moved because another worktree has it checked out. The + // stack must still be rebased onto the latest trunk. + t.Run("falls back to the remote ref when the local trunk is locked", func(t *testing.T) { + mock := trunkTargetMock("old", "new") + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"new", "old"}}, nil) + mock.UpdateBranchRefFn = func(string, string) error { + return errors.New("cannot force update the branch 'main' used by worktree at '/elsewhere'") + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, "origin/main", target.Ref, "cascade must target the remote ref") + assert.Equal(t, "new", target.SHA) + assert.True(t, target.Moved) + assert.True(t, target.Detached) + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + assert.Contains(t, string(out), "worktree") + assert.Contains(t, string(out), "origin/main") + }) + + t.Run("falls back to the remote ref when the local trunk diverged", func(t *testing.T) { + mock := trunkTargetMock("local", "remote") + mock.IsAncestorFn, _ = ancestorMock([][2]string{ + {"local", "remote"}, + {"remote", "local"}, + }, nil) + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, "origin/main", target.Ref) + assert.True(t, target.Detached) + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + assert.Contains(t, string(out), "diverged") + }) + + // Unpushed commits on the local trunk must not be dropped by rebasing the + // stack onto the older remote ref. + t.Run("keeps the local trunk when it is ahead of the remote", func(t *testing.T) { + mock := trunkTargetMock("ahead", "behind") + mock.IsAncestorFn, _ = ancestorMock([][2]string{{"ahead", "behind"}}, nil) + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, "main", target.Ref) + assert.False(t, target.Detached) + }) + + // Issue #225 shape: the trunk branch was tracked on the remote and has since + // been merged and deleted, orphaning the stack. + t.Run("fails when a tracked trunk no longer exists on the remote", func(t *testing.T) { + mock := trunkTargetMock("local", "") + mock.UpstreamRemoteFn = func(string) (string, error) { return "origin", nil } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + _, err := resolveTrunkTarget(cfg, s, "origin", "b1") + assert.ErrorIs(t, err, ErrSilent) + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + assert.Contains(t, string(out), "no longer exists on origin") + assert.Contains(t, string(out), "--no-trunk") + }) + + // A stack based on a local integration branch that was never pushed: the + // local branch is the source of truth, so the cascade must still run. + t.Run("uses a local-only trunk that was never pushed", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + BranchExistsFn: func(string) bool { return true }, + UpstreamRemoteFn: func(string) (string, error) { return "", nil }, + RevParseFn: func(ref string) (string, error) { + if strings.HasPrefix(ref, "origin/") { + return "", errors.New("unknown revision") + } + return "sha-" + ref, nil + }, + }) + defer restore() + + cfg, _, errR := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "integration"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, "integration", target.Ref) + assert.False(t, target.Detached) + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + assert.Contains(t, string(out), "only exists locally") + }) + + t.Run("fails when the trunk exists neither locally nor remotely", func(t *testing.T) { + mock := trunkTargetMock("local", "") + mock.BranchExistsFn = func(string) bool { return false } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "main"}} + + _, err := resolveTrunkTarget(cfg, s, "origin", "b1") + assert.ErrorIs(t, err, ErrSilent) + + cfg.Err.Close() + out, _ := io.ReadAll(errR) + assert.Contains(t, string(out), "neither locally nor on origin") + }) + + // Issue #176: `gh stack init --base origin/main` records a remote-qualified + // trunk, which used to be re-qualified into "origin/origin/main". + t.Run("normalizes a remote-qualified trunk", func(t *testing.T) { + mock := trunkTargetMock("same", "same") + mock.BranchExistsFn = func(name string) bool { return name == "main" } + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "origin/main"}} + + target, err := resolveTrunkTarget(cfg, s, "origin", "b1") + require.NoError(t, err) + assert.Equal(t, "main", target.Ref) + assert.Equal(t, "main", s.Trunk.Branch, "the stack's trunk name should be repaired") + }) +} + +func TestVerifyStacked(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + t.Run("reports branches missing their parent", func(t *testing.T) { + isAncestor, _ := ancestorMock([][2]string{{"b1", "b2"}}, nil) + restore := git.SetOps(&git.MockOps{IsAncestorFn: isAncestor}) + defer restore() + + assert.Equal(t, []string{"b2"}, verifyStacked(s, "main", 0, len(s.Branches))) + }) + + t.Run("measures the bottom branch against the given trunk ref", func(t *testing.T) { + // Stacked on the local trunk but not on the remote-tracking ref: the + // exact state a stale-trunk rebase used to leave behind. + isAncestor, _ := ancestorMock([][2]string{{"origin/main", "b1"}}, nil) + restore := git.SetOps(&git.MockOps{IsAncestorFn: isAncestor}) + defer restore() + + assert.Empty(t, verifyStacked(s, "main", 0, len(s.Branches))) + assert.Equal(t, []string{"b1"}, verifyStacked(s, "origin/main", 0, len(s.Branches))) + }) + + t.Run("honours the range", func(t *testing.T) { + isAncestor, _ := ancestorMock([][2]string{{"main", "b1"}}, nil) + restore := git.SetOps(&git.MockOps{IsAncestorFn: isAncestor}) + defer restore() + + assert.Equal(t, []string{"b1"}, verifyStacked(s, "main", 0, 3)) + assert.Empty(t, verifyStacked(s, "main", 1, 3), "b1 is outside the range") + }) + + t.Run("skips merged branches and uses the nearest active parent", func(t *testing.T) { + merged := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2"}, + }, + } + isAncestor, _ := ancestorMock([][2]string{{"b1", "b2"}}, nil) + restore := git.SetOps(&git.MockOps{IsAncestorFn: isAncestor}) + defer restore() + + assert.Empty(t, verifyStacked(merged, "main", 0, 2), + "b2 should be measured against main, not the merged b1") + }) + + t.Run("empty trunk ref falls back to the stack trunk", func(t *testing.T) { + isAncestor, _ := ancestorMock([][2]string{{"main", "b1"}}, nil) + restore := git.SetOps(&git.MockOps{IsAncestorFn: isAncestor}) + defer restore() + + assert.Equal(t, []string{"b1"}, verifyStacked(s, "", 0, len(s.Branches))) + }) +} + +func TestPreflightRebase(t *testing.T) { + tests := []struct { + name string + rebaseInProgres bool + trackedDirty bool + statusErr error + autostash bool + wantErr error + }{ + { + name: "clean tree passes", + }, + { + name: "rebase in progress fails", + rebaseInProgres: true, + wantErr: ErrRebaseActive, + }, + { + name: "dirty tracked files fail", + trackedDirty: true, + wantErr: ErrSilent, + }, + { + name: "autostash lifts the clean-tree requirement", + trackedDirty: true, + autostash: true, + }, + { + name: "autostash does not lift the rebase-in-progress check", + rebaseInProgres: true, + autostash: true, + wantErr: ErrRebaseActive, + }, + { + name: "an unreadable status is not treated as clean", + statusErr: errors.New("boom"), + wantErr: ErrSilent, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsRebaseInProgressFn: func() bool { return tt.rebaseInProgres }, + HasUncommittedTrackedChangesFn: func() (bool, error) { + return tt.trackedDirty, tt.statusErr + }, + }) + defer restore() + + cfg, _, _ := config.NewTestConfig() + err := preflightRebase(cfg, "rebase", tt.autostash) + if tt.wantErr == nil { + assert.NoError(t, err) + return + } + assert.ErrorIs(t, err, tt.wantErr) + }) + } +} + +// The preflight must only consider tracked files: git rebase runs happily with +// untracked files present, so blocking on them would refuse rebases git accepts. +func TestPreflightRebase_IgnoresUntrackedFiles(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsRebaseInProgressFn: func() bool { return false }, + // A repo with only untracked files: "dirty" overall, clean for rebase. + HasUncommittedChangesFn: func() (bool, error) { return true, nil }, + HasUncommittedTrackedChangesFn: func() (bool, error) { return false, nil }, + }) + defer restore() + + cfg, _, _ := config.NewTestConfig() + assert.NoError(t, preflightRebase(cfg, "rebase", false)) +} + +func TestStashForRebase(t *testing.T) { + t.Run("stashes tracked changes", func(t *testing.T) { + var pushed []string + restore := git.SetOps(&git.MockOps{ + HasUncommittedTrackedChangesFn: func() (bool, error) { return true, nil }, + StashPushFn: func(msg string) error { + pushed = append(pushed, msg) + return nil + }, + }) + defer restore() + + cfg, _, _ := config.NewTestConfig() + stashed, err := stashForRebase(cfg) + require.NoError(t, err) + assert.True(t, stashed) + assert.Equal(t, []string{stashMessage}, pushed) + }) + + t.Run("does nothing on a clean tree", func(t *testing.T) { + called := false + restore := git.SetOps(&git.MockOps{ + HasUncommittedTrackedChangesFn: func() (bool, error) { return false, nil }, + StashPushFn: func(string) error { called = true; return nil }, + }) + defer restore() + + cfg, _, _ := config.NewTestConfig() + stashed, err := stashForRebase(cfg) + require.NoError(t, err) + assert.False(t, stashed) + assert.False(t, called, "no stash should be created") + }) + + t.Run("surfaces a failed stash", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + HasUncommittedTrackedChangesFn: func() (bool, error) { return true, nil }, + StashPushFn: func(string) error { return errors.New("disk full") }, + }) + defer restore() + + cfg, _, _ := config.NewTestConfig() + _, err := stashForRebase(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "could not stash local changes") + }) +} diff --git a/cmd/utils.go b/cmd/utils.go index 1c5dc139..b24bcaf6 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -757,6 +757,15 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string // updateBaseSHAs refreshes the Base and Head SHAs for all active branches // in a stack. Call this after any operation that may have moved branch refs // (rebase, push, etc.). +// +// Base records the parent tip a branch is actually stacked on, and is used as +// the `git rebase --onto ` boundary on the next cascade. It +// is therefore only advanced when the parent's current tip really is in the +// branch's history: a parent that moved out of band (an amended, reordered, or +// force-pushed commit) leaves the branch behind, and recording the parent's new +// tip anyway would claim a boundary the branch does not contain — making the +// next rebase replay the parent's superseded commits into it. Head is always +// accurate, since it is the branch's own tip. func updateBaseSHAs(s *stack.Stack) { // Collect all refs we need to resolve, then batch into one git call. var refs []string @@ -791,7 +800,7 @@ func updateBaseSHAs(s *stack.Stack) { return } for _, p := range pairs { - if base, ok := shaMap[p.parent]; ok { + if base, ok := shaMap[p.parent]; ok && isStackedOn(base, p.branch, s.Branches[p.index].Base) { s.Branches[p.index].Base = base } if head, ok := shaMap[p.branch]; ok { @@ -800,6 +809,26 @@ func updateBaseSHAs(s *stack.Stack) { } } +// isStackedOn reports whether parentSHA can be recorded as branch's base. +// +// The parent tip must be in the branch's history for the record to be true. A +// previously recorded base is only ever replaced by another genuine ancestor, +// so a parent that has moved out of band cannot overwrite the boundary the +// branch is really built on. With no usable previous value there is nothing to +// preserve, so the current tip is recorded regardless. +func isStackedOn(parentSHA, branch, currentBase string) bool { + if currentBase == "" || currentBase == parentSHA { + return true + } + isAnc, err := git.IsAncestor(parentSHA, branch) + if err != nil { + // Unable to tell — keep the recorded base rather than assert a + // boundary that may not hold. + return false + } + return isAnc +} + // activeBranchNames returns the branch names for all non-merged branches in a stack. func activeBranchNames(s *stack.Stack) []string { active := s.ActiveBranches() @@ -887,11 +916,28 @@ func resolveOriginalRefs(s *stack.Stack) (map[string]string, error) { return originalRefs, nil } +// normalizeTrunkBranch strips a leading "/" from a trunk branch name. +// `gh stack init --base origin/main` records the remote-qualified name, which +// is then re-qualified into "origin/origin/main" wherever a remote ref is built +// from it. The prefix is only stripped when no local branch is literally named +// that way, so a genuine branch called "origin/main" still works. +func normalizeTrunkBranch(trunk, remote string) string { + if remote == "" { + return trunk + } + stripped, ok := strings.CutPrefix(trunk, remote+"/") + if !ok || stripped == "" || git.BranchExists(trunk) { + return trunk + } + return stripped +} + // ensureLocalTrunk ensures the trunk branch exists locally. If it does not, // it fetches the branch from the remote and creates a local tracking branch. // This handles the case where a user started their stack after renaming their // initial branch (e.g. `git branch -m newbranch`), leaving no local trunk. func ensureLocalTrunk(cfg *config.Config, trunk, remote string) error { + trunk = normalizeTrunkBranch(trunk, remote) if git.BranchExists(trunk) { return nil } @@ -909,57 +955,232 @@ func ensureLocalTrunk(cfg *config.Config, trunk, remote string) error { return nil } -// fastForwardTrunk fast-forwards the trunk branch to match its remote tracking -// branch. Returns true if trunk was updated. -func fastForwardTrunk(cfg *config.Config, trunk, remote, currentBranch string) bool { - // If the local trunk branch doesn't exist, there's nothing to - // fast-forward. Callers should use ensureLocalTrunk beforehand if - // they need trunk to be resolvable as a local ref. - if !git.BranchExists(trunk) { - return false +// normalizeStackTrunk rewrites a remote-qualified trunk name (e.g. "origin/main" +// recorded by `gh stack init --base origin/main`) to the plain local branch +// name, so refspecs and remote refs built from it are not double-prefixed. +// It is idempotent and reports the rewrite once. +func normalizeStackTrunk(cfg *config.Config, s *stack.Stack, remote string) { + trunk := normalizeTrunkBranch(s.Trunk.Branch, remote) + if trunk == s.Trunk.Branch { + return } + cfg.Warningf("Stack trunk %q is remote-qualified — using local branch %q on %s", + s.Trunk.Branch, trunk, remote) + s.Trunk.Branch = trunk +} + +// trunkTarget describes the ref a cascade rebase must use as the base of the +// bottom branch of a stack. +type trunkTarget struct { + // Branch is the normalized local trunk branch name. + Branch string + // Ref is the ref to rebase onto: Branch when the local trunk is current, + // or "/" when the local ref could not be updated. + Ref string + // SHA is the commit Ref resolves to. + SHA string + // Moved reports whether the effective trunk advanced during this run. + Moved bool + // Detached reports whether Ref is the remote-tracking ref because the local + // trunk branch could not be brought up to date. + Detached bool +} - localSHA, remoteSHA := "", "" - trunkRefs, trunkErr := git.RevParseMulti([]string{trunk, remote + "/" + trunk}) - if trunkErr == nil { - localSHA, remoteSHA = trunkRefs[0], trunkRefs[1] +// Describe renders the trunk target for the closing summary, naming the SHA so +// a run against a stale trunk is visible at a glance. +func (t trunkTarget) Describe() string { + return fmt.Sprintf("%s (%s)", t.Ref, short(t.SHA)) +} + +// resolveTrunkTarget determines the ref a cascade rebase must use as the base +// of the bottom branch of the stack, bringing the local trunk up to date where +// possible. +// +// The local trunk ref cannot always be moved: it may be checked out in another +// worktree, or have diverged from the remote. Rebasing onto it anyway bases the +// whole stack on a stale trunk while every branch still looks correctly +// stacked, which is why sync and rebase could report a complete success without +// pulling anything in. In that situation the remote-tracking ref is used as the +// rebase target instead, so the stack ends up current regardless of why the +// local ref is stuck. +// +// Diagnostics are printed here, so a returned error is always ErrSilent. +func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranch string) (trunkTarget, error) { + normalizeStackTrunk(cfg, s, remote) + trunk := s.Trunk.Branch + + remoteRef := remote + "/" + trunk + remoteSHA, remoteErr := git.RevParse(remoteRef) + if remoteErr != nil { + return trunkWithoutRemote(cfg, trunk, remote) } - if trunkErr != nil { - cfg.Warningf("Could not compare trunk %s with remote — skipping trunk update", trunk) - return false + if err := ensureLocalTrunk(cfg, trunk, remote); err != nil { + cfg.Errorf("%s", err) + return trunkTarget{}, ErrSilent + } + + localSHA, err := git.RevParse(trunk) + if err != nil { + cfg.Errorf("could not resolve trunk branch %s: %s", trunk, err) + return trunkTarget{}, ErrSilent } if localSHA == remoteSHA { cfg.Successf("Trunk %s is already up to date", trunk) - return false + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil } - isAncestor, err := git.IsAncestor(localSHA, remoteSHA) - if err != nil { - cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, err) - return false + // Local trunk already contains the remote tip — unpushed local commits on + // trunk. Rebasing onto the remote ref would drop them, so keep the local. + if isAnc, ancErr := git.IsAncestor(remoteSHA, localSHA); ancErr == nil && isAnc { + cfg.Successf("Trunk %s is ahead of %s — using the local branch", trunk, remoteRef) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil } - if !isAncestor { - cfg.Warningf("Trunk %s has diverged from %s — skipping trunk update", trunk, remote) - cfg.Printf(" Local and remote %s have diverged. Resolve manually.", trunk) - return false + + canFastForward, ffErr := git.IsAncestor(localSHA, remoteSHA) + if ffErr != nil { + cfg.Warningf("Could not determine fast-forward status for %s: %v", trunk, ffErr) + canFastForward = false } - if currentBranch == trunk { - if err := git.MergeFF(remote + "/" + trunk); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - return false + if canFastForward { + var updateErr error + if currentBranch == trunk { + updateErr = git.MergeFF(remoteRef) + } else { + updateErr = git.UpdateBranchRef(trunk, remoteSHA) } - } else { - if err := git.UpdateBranchRef(trunk, remoteSHA); err != nil { - cfg.Warningf("Failed to fast-forward %s: %v", trunk, err) - return false + if updateErr == nil { + cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: remoteSHA, Moved: true}, nil } + cfg.Warningf("Could not update local %s: %v", trunk, updateErr) + } else { + cfg.Warningf("Local %s has diverged from %s", trunk, remoteRef) } - cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - return true + // The local ref is stuck. Rebase onto the remote-tracking ref so the stack + // still ends up on the latest trunk. + cfg.Printf(" Rebasing the stack onto %s (%s) instead — your local %s is left untouched.", + remoteRef, short(remoteSHA), trunk) + return trunkTarget{Branch: trunk, Ref: remoteRef, SHA: remoteSHA, Moved: true, Detached: true}, nil +} + +// trunkWithoutRemote handles a trunk with no resolvable remote-tracking ref. +// +// Two very different situations produce that, and they need opposite handling: +// +// - The trunk was tracked on the remote and has since been deleted — the +// usual cause is a stack whose trunk was itself a feature branch that got +// merged. The stack is now orphaned, and rebasing it onto the stale local +// copy would quietly keep it off the real trunk forever, so this fails. +// - The trunk was never on the remote at all, e.g. a stack based on a local +// integration branch. There the local branch *is* the source of truth, so +// the cascade proceeds against it. +func trunkWithoutRemote(cfg *config.Config, trunk, remote string) (trunkTarget, error) { + if !git.BranchExists(trunk) { + cfg.Errorf("trunk branch %s exists neither locally nor on %s", trunk, remote) + cfg.Printf(" Re-point the stack at an existing trunk branch.") + return trunkTarget{}, ErrSilent + } + + tracked, err := git.UpstreamRemote(trunk) + if err == nil && tracked != "" { + cfg.Errorf("%s no longer exists on %s — cannot bring the stack up to date with its trunk", trunk, remote) + cfg.Printf(" If %s was merged and deleted, re-point the stack at its new trunk.", trunk) + cfg.Printf(" To rebase the stack on itself without touching the trunk, run `%s`.", + cfg.ColorCyan("gh stack rebase --no-trunk")) + return trunkTarget{}, ErrSilent + } + + localSHA, err := git.RevParse(trunk) + if err != nil { + cfg.Errorf("could not resolve trunk branch %s: %s", trunk, err) + return trunkTarget{}, ErrSilent + } + cfg.Warningf("Trunk %s only exists locally — %s has no such branch", trunk, remote) + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil +} +func requireNoRebaseInProgress(cfg *config.Config) error { + if !git.IsRebaseInProgress() { + return nil + } + cfg.Errorf("a rebase is currently in progress") + cfg.Printf("Complete the rebase with `%s` or abort with `%s`", + cfg.ColorCyan("gh stack rebase --continue"), + cfg.ColorCyan("gh stack rebase --abort")) + return ErrRebaseActive +} + +// requireCleanWorkingTree fails when tracked files have uncommitted changes. +// git refuses to rebase over them, so this turns a confusing mid-cascade +// failure into an actionable message up front. Untracked files are not checked: +// git rebases happily with them present. An inability to inspect the tree is +// treated as a reason to stop — a failed status must not read as "clean". +func requireCleanWorkingTree(cfg *config.Config, command string) error { + dirty, err := git.HasUncommittedTrackedChanges() + if err != nil { + cfg.Errorf("failed to check working tree status: %s", err) + return ErrSilent + } + if !dirty { + return nil + } + cfg.Errorf("uncommitted changes in working tree") + cfg.Printf("Commit or stash your changes before running %s, or re-run with `%s`", + command, cfg.ColorCyan("--autostash")) + return ErrSilent +} + +// preflightRebase runs the checks that must pass before any cascade rebase. +// With autostash the clean-tree requirement is lifted, since stashForRebase +// clears the working tree instead. +func preflightRebase(cfg *config.Config, command string, autostash bool) error { + if err := requireNoRebaseInProgress(cfg); err != nil { + return err + } + if autostash { + return nil + } + return requireCleanWorkingTree(cfg, command) +} + +// stashMessage labels the stash entry gh-stack creates for --autostash. +const stashMessage = "gh-stack: autostash" + +// stashForRebase stashes tracked working-tree changes for the duration of a +// cascade rebase, reporting whether anything was stashed. +// +// git's own `rebase --autostash` cannot be used here: it pops the stash at the +// end of every individual rebase, which lands the changes on whichever branch +// that rebase left checked out and conflicts against files that do not exist +// there. Stashing once around the whole cascade keeps the changes intact. +func stashForRebase(cfg *config.Config) (bool, error) { + dirty, err := git.HasUncommittedTrackedChanges() + if err != nil { + return false, fmt.Errorf("failed to check working tree status: %w", err) + } + if !dirty { + return false, nil + } + if err := git.StashPush(stashMessage); err != nil { + return false, fmt.Errorf("could not stash local changes: %w", err) + } + cfg.Successf("Stashed local changes") + return true, nil +} + +// restoreStash pops the stash created by stashForRebase, warning rather than +// failing if it cannot be applied — the entry is still in the stash list. +func restoreStash(cfg *config.Config) { + if err := git.StashPop(); err != nil { + cfg.Warningf("Could not restore your stashed changes: %v", err) + cfg.Printf(" They are still saved — run `%s` to restore them.", + cfg.ColorCyan("git stash pop")) + return + } + cfg.Successf("Restored stashed changes") } // cascadeRebaseOpts holds parameters for a cascade rebase across a range of @@ -973,6 +1194,18 @@ type cascadeRebaseOpts struct { NeedsOnto bool OntoOldBase string CommitterDateIsAuthorDate bool + // TrunkRef is the ref the bottom branch is rebased onto. It defaults to + // Stack.Trunk.Branch, but resolveTrunkTarget substitutes the + // remote-tracking ref when the local trunk could not be brought up to date. + TrunkRef string +} + +// trunkRef returns the ref to rebase the bottom branch onto. +func (o cascadeRebaseOpts) trunkRef() string { + if o.TrunkRef != "" { + return o.TrunkRef + } + return o.Stack.Trunk.Branch } // cascadeRebaseResult describes the outcome of a cascade rebase. @@ -988,6 +1221,77 @@ type cascadeRebaseResult struct { OntoOldBase string // ontoOldBase at the conflict point (for --continue) } +// resolveOntoOldBase picks the commit to pass as the argument of +// +// git rebase --onto +// +// The upstream marks the boundary of the commits to replay: everything after it, +// up to the branch tip, is re-applied on top of newBase. Passing a commit the +// branch does not actually contain makes git fall back to a merge base and +// replay commits that are already represented in newBase — the cause of the old +// version of an amended parent commit reappearing in the branches above it. +// +// Candidates are considered in order of preference and the *latest* one that is +// genuinely an ancestor of the branch wins, since that replays the fewest +// commits and therefore cannot duplicate anything: +// +// 1. recordedOldBase — the parent's tip at the start of this run. Correct +// whenever the parent has not moved out of band. +// 2. metadataBase — the parent tip this branch was last stacked on, from the +// stack file. Correct when the parent was amended, reordered, or +// squash-merged since. +// 3. merge-base --fork-point, which reads the parent's reflog and so still +// finds the right boundary when the stack file itself is stale — for +// example a stack whose metadata was written by an older version. +// 4. merge-base(recordedOldBase, branch) / merge-base(newBase, branch). +// +// Returns recordedOldBase unchanged when nothing better can be determined. +func resolveOntoOldBase(recordedOldBase, metadataBase, newBase, branch string) string { + isAncestorOfBranch := func(sha string) bool { + if sha == "" { + return false + } + ok, err := git.IsAncestor(sha, branch) + return err == nil && ok + } + + if isAncestorOfBranch(recordedOldBase) { + return recordedOldBase + } + + candidates := []string{metadataBase} + if fp, err := git.MergeBaseForkPoint(newBase, branch); err == nil && fp != "" { + candidates = append(candidates, fp) + } + if mb, err := git.MergeBase(recordedOldBase, branch); err == nil { + candidates = append(candidates, mb) + } + if mb, err := git.MergeBase(newBase, branch); err == nil { + candidates = append(candidates, mb) + } + + best := "" + for _, c := range candidates { + if !isAncestorOfBranch(c) { + continue + } + if best == "" { + best = c + continue + } + // Both are ancestors of branch, so they are ordered along its history. + // Keep the later one — it replays strictly fewer commits. + if isAnc, err := git.IsAncestor(best, c); err == nil && isAnc { + best = c + } + } + + if best == "" { + return recordedOldBase + } + return best +} + // cascadeRebase performs a cascade rebase across the given branch range. It // stops at the first conflict and returns a result describing what happened. // The caller is responsible for conflict recovery (abort+restore or save state). @@ -998,14 +1302,17 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { ontoOldBase := opts.OntoOldBase originalRefs := opts.OriginalRefs result := cascadeRebaseResult{} - rebaseOpts := git.RebaseOpts{CommitterDateIsAuthorDate: opts.CommitterDateIsAuthorDate} + rebaseOpts := git.RebaseOpts{ + CommitterDateIsAuthorDate: opts.CommitterDateIsAuthorDate, + } + trunkRef := opts.trunkRef() for i, br := range opts.Branches { absIdx := opts.StartAbsIdx + i var base string if absIdx == 0 { - base = s.Trunk.Branch + base = trunkRef } else { base = s.Branches[absIdx-1].Branch } @@ -1034,7 +1341,7 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { if needsOnto { // Find --onto target: first non-merged ancestor, or trunk. Queued // ancestors keep their commits, so they are valid --onto targets. - newBase := s.Trunk.Branch + newBase := trunkRef for j := absIdx - 1; j >= 0; j-- { if !s.Branches[j].IsMerged() { newBase = s.Branches[j].Branch @@ -1042,18 +1349,19 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } } - // If ontoOldBase is stale (not an ancestor of the branch), the - // branch was already rebased past it. 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 - } - } + // The recorded upstream can be stale — the parent may have been + // amended, reordered, or squash-merged since this branch was + // stacked on it. Resolve a boundary the branch actually contains + // so already-applied commits are not replayed. + actualOldBase := resolveOntoOldBase(ontoOldBase, br.Base, newBase, br.Branch) if err := git.RebaseOnto(newBase, actualOldBase, br.Branch, rebaseOpts); err != nil { + if git.IsRebaseStartError(err) { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: rebaseStartFailure(br.Branch, newBase, err), + } + } remaining := make([]string, 0, len(opts.Branches)-i-1) for j := i + 1; j < len(opts.Branches); j++ { remaining = append(remaining, opts.Branches[j].Branch) @@ -1076,7 +1384,12 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } else { var rebaseErr error if absIdx > 0 { - rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch, rebaseOpts) + // Same staleness problem as the --onto path above: the parent's + // current tip is the wrong boundary once the parent has been + // amended or reordered, because this branch still contains the + // parent's previous commits. + oldBase := resolveOntoOldBase(originalRefs[base], br.Base, base, br.Branch) + rebaseErr = git.RebaseOnto(base, oldBase, br.Branch, rebaseOpts) } else { if err := git.CheckoutBranch(br.Branch); err != nil { return cascadeRebaseResult{ @@ -1088,6 +1401,12 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } if rebaseErr != nil { + if git.IsRebaseStartError(rebaseErr) { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: rebaseStartFailure(br.Branch, base, rebaseErr), + } + } remaining := make([]string, 0, len(opts.Branches)-i-1) for j := i + 1; j < len(opts.Branches); j++ { remaining = append(remaining, opts.Branches[j].Branch) @@ -1112,17 +1431,53 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { return result } +// rebaseStartFailure wraps a rebase that git refused to start. There is nothing +// to resolve or continue, so it must not be reported as a conflict. git's own +// message names the cause (dirty working tree, branch checked out in another +// worktree, unresolvable upstream, rebase already in progress), so it is +// preserved verbatim. +func rebaseStartFailure(branch, base string, err error) error { + return fmt.Errorf("could not start rebase of %s onto %s: %w", branch, base, err) +} + // stackNeedsRebase returns true if any active branch in the stack is not based // on its parent's current tip. This detects when the stack needs rebasing even // if trunk was not updated in the current run. -func stackNeedsRebase(s *stack.Stack) bool { - trunk := s.Trunk.Branch - for i, br := range s.Branches { +// +// trunkRef is the ref the bottom branch must sit on. Passing an empty string +// falls back to the local trunk branch, which is only correct when the local +// trunk is known to be current — otherwise a stack sitting on a stale local +// trunk looks perfectly up to date. +func stackNeedsRebase(s *stack.Stack, trunkRef string) bool { + return len(verifyStacked(s, trunkRef, 0, len(s.Branches))) > 0 +} + +// verifyStacked returns the names of branches in s.Branches[startIdx:endIdx] +// that do not have their effective parent (the nearest non-skipped ancestor, +// or trunkRef) in their commit history. +// +// It doubles as the post-condition for a cascade rebase: if any branch in the +// rebased range still fails this check, the rebase did not actually take +// effect and reporting success would be a lie. +func verifyStacked(s *stack.Stack, trunkRef string, startIdx, endIdx int) []string { + if trunkRef == "" { + trunkRef = s.Trunk.Branch + } + if startIdx < 0 { + startIdx = 0 + } + if endIdx > len(s.Branches) { + endIdx = len(s.Branches) + } + + var unstacked []string + for i := startIdx; i < endIdx; i++ { + br := s.Branches[i] if br.IsSkipped() { continue } // Find the nearest non-skipped parent. - parent := trunk + parent := trunkRef for j := i - 1; j >= 0; j-- { if !s.Branches[j].IsSkipped() { parent = s.Branches[j].Branch @@ -1131,10 +1486,24 @@ func stackNeedsRebase(s *stack.Stack) bool { } isAnc, err := git.IsAncestor(parent, br.Branch) if err != nil || !isAnc { - return true + unstacked = append(unstacked, br.Branch) } } - return false + return unstacked +} + +// reportUnstacked prints a diagnostic for branches that are still not stacked +// on their parent after a cascade rebase reported success. +func reportUnstacked(cfg *config.Config, trunkRef string, unstacked []string) { + cfg.Errorf("Rebase reported success but %s still not based on %s: %s", + plural(len(unstacked), "this branch is", "these branches are"), + plural(len(unstacked), "its parent", "their parents"), + strings.Join(unstacked, ", ")) + cfg.Printf(" Trunk: %s", trunkRef) + cfg.Printf(" This usually means git refused the rebase. Check for uncommitted") + cfg.Printf(" changes, a branch checked out in another worktree, or a rebase") + cfg.Printf(" already in progress, then run `%s` again.", + cfg.ColorCyan("gh stack rebase")) } // resolvePR resolves a user-provided target to a stack and branch using diff --git a/cmd/utils_test.go b/cmd/utils_test.go index feb89fc9..07a3789d 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -682,7 +682,7 @@ func TestStackNeedsRebase_AllCurrent(t *testing.T) { restore := git.SetOps(mock) defer restore() - assert.False(t, stackNeedsRebase(s), "stack should not need rebase when all branches are current") + assert.False(t, stackNeedsRebase(s, ""), "stack should not need rebase when all branches are current") } func TestStackNeedsRebase_FirstBranchStale(t *testing.T) { @@ -705,7 +705,7 @@ func TestStackNeedsRebase_FirstBranchStale(t *testing.T) { restore := git.SetOps(mock) defer restore() - assert.True(t, stackNeedsRebase(s), "stack should need rebase when first branch is stale") + assert.True(t, stackNeedsRebase(s, ""), "stack should need rebase when first branch is stale") } func TestStackNeedsRebase_SkipsMergedBranches(t *testing.T) { @@ -725,7 +725,7 @@ func TestStackNeedsRebase_SkipsMergedBranches(t *testing.T) { restore := git.SetOps(mock) defer restore() - assert.False(t, stackNeedsRebase(s), "should skip merged branches and find stack up to date") + assert.False(t, stackNeedsRebase(s, ""), "should skip merged branches and find stack up to date") } // setTestRepo sets RepoOverride so tests don't depend on real git context. @@ -851,3 +851,40 @@ func TestEnrichPRContent(t *testing.T) { assert.Equal(t, "Fetched body", details["merged"].Body) assert.Equal(t, "Has it", details["open"].Title, "PRs that already have a title are untouched") } + +// ancestorMock builds a directional IsAncestor func for git.MockOps. +// +// Every pair reports true except those listed, so branch/parent relationships +// hold by default and only the modelled exception reports false: +// +// - permanent: pairs that are never in an ancestor relationship, such as a +// local trunk that has diverged from its remote. +// - untilRebase: pairs that report false until markRebased is called, which +// models a stack that is stale until the cascade fixes it. sync and rebase +// verify after the cascade that every branch really does sit on top of its +// parent, so a mock whose rebase never "lands" is correctly reported as a +// rebase that did not happen. +// +// markRebased must be called from the mock's Rebase/RebaseOnto funcs. +func ancestorMock(permanent, untilRebase [][2]string) (fn func(string, string) (bool, error), markRebased func()) { + listed := func(pairs [][2]string, a, d string) bool { + for _, p := range pairs { + if p[0] == a && p[1] == d { + return true + } + } + return false + } + rebased := false + fn = func(a, d string) (bool, error) { + if listed(permanent, a, d) { + return false, nil + } + if !rebased && listed(untilRebase, a, d) { + return false, nil + } + return true, nil + } + markRebased = func() { rebased = true } + return fn, markRebased +} diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index a102281e..60fa36d1 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -174,6 +174,20 @@ gh stack rebase --upstack gh stack rebase --no-trunk ``` +Rebasing needs a working tree with no uncommitted changes to tracked files, because git refuses to rebase over them. Untracked files are fine. Commit or stash your work first, or let `gh stack` stash it for you: + +```sh +gh stack rebase --autostash +``` + +The same applies to `gh stack sync`, which runs the cascading rebase as one of its steps. + +If your local trunk branch can't be updated — because it's checked out in another worktree, or has diverged from the remote — the stack is rebased onto `origin/` instead, and the command tells you so. Your local trunk is left alone. The closing summary always names the ref and commit the stack actually landed on: + +``` +All branches in stack rebased locally with origin/main (84bacb3) +``` + After rebasing, push the updated branches: ```sh diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index 8143627a..c779b627 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -306,18 +306,23 @@ gh stack sync [flags] |------|-------------| | `--remote ` | Remote to fetch from and push to (defaults to auto-detected remote) | | `--prune` | Delete local branches for merged PRs | +| `--autostash` | Stash uncommitted changes before rebasing and restore them afterwards | + +Sync requires no rebase in progress and no uncommitted changes to tracked files, because git refuses to rebase otherwise. Untracked files are fine. Commit or stash your changes first, or pass `--autostash` to have the changes stashed for the duration of the sync and restored afterwards. Performs a synchronization of the entire stack: -1. **Fetch** — fetches the latest changes from `origin`. +1. **Fetch** — fetches the latest changes from `origin`. A failed fetch is reported rather than silently ignored. 2. **Reconcile the remote stack** — mirrors the GitHub stack locally. When PRs have been added to the stack on GitHub (the remote is ahead of your local stack), their branches are pulled down and appended to your local stack automatically. When the local and remote stacks have genuinely diverged (for example, you added a branch locally while different PRs were added to the stack on GitHub), you are prompted to resolve (see **Diverged stacks** below). In a non-interactive terminal a divergence aborts the sync (nothing is pushed or updated). -3. **Fast-forward trunk** — fast-forwards the trunk branch to match the remote (skips if diverged). -4. **Cascade rebase** — rebases all stack branches onto their updated parents (only if trunk moved). If a conflict is detected, all branches are restored to their original state, and you are advised to run `gh stack rebase` to resolve conflicts interactively. -5. **Push** — pushes all branches (uses `--force-with-lease` if a rebase occurred). +3. **Resolve the trunk** — creates the local trunk branch if it is missing, then fast-forwards it to match the remote. If the local trunk cannot be updated, the stack is rebased onto the remote-tracking branch instead (see **When the trunk cannot be updated** below). +4. **Cascade rebase** — rebases all stack branches onto their updated parents. If a conflict is detected, all branches are restored to their original state, and you are advised to run `gh stack rebase` to resolve conflicts interactively. +5. **Push** — pushes all branches (uses `--force-with-lease` if a rebase occurred). Sync first verifies that every branch really does sit on top of its parent, and stops without pushing if any does not. 6. **Sync PRs** — syncs PR state from GitHub and reports the status of each PR. 7. **Sync the stack** — links the stack's open PRs into a stack on GitHub, creating the remote stack object if it doesn't exist yet or updating it if it's partially formed. This only happens when two or more PRs exist; sync never opens PRs (use `gh stack submit` for that). 8. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to prune automatically. +The closing summary names the trunk ref and commit the stack was rebased onto, so you can confirm at a glance that the stack really is current. + A clean remote-ahead update (PRs added on top of your local stack) is pulled down automatically without prompting, so `sync` is safe to run in automation. Sync only prompts when the stacks have truly diverged. **Diverged stacks** @@ -337,8 +342,22 @@ gh stack sync # Sync and automatically prune merged branches gh stack sync --prune + +# Sync with uncommitted changes in the working tree +gh stack sync --autostash ``` +**When the trunk cannot be updated** + +Both `sync` and `rebase` bring the local trunk branch up to date before rebasing the stack onto it. Sometimes that is not possible: + +- the trunk is checked out in another worktree, so its ref cannot be moved +- the local trunk has diverged from the remote + +In those cases the stack is rebased onto `/` (for example `origin/main`) instead, and the command says so. Your local trunk branch is left untouched — only the stack is brought up to date. The closing summary names the ref that was actually used. + +If the trunk branch no longer exists on the remote at all — typically because it was a feature branch that has since been merged and deleted — there is nothing to bring the stack up to date with, and the command stops with an error instead of silently rebasing onto a stale local trunk. Re-point the stack at its new trunk, or use `gh stack rebase --no-trunk` to rebase the stack on itself. + ### `gh stack rebase` Pull from remote and do a cascading rebase across the stack. @@ -356,6 +375,7 @@ gh stack rebase [flags] [branch] | `--abort` | Abort the rebase and restore all branches to their pre-rebase state | | `--remote ` | Remote to fetch from (defaults to auto-detected remote) | | `--committer-date-is-author-date` | Set the committer date to the author date during rebase. Alias: `--preserve-dates` | +| `--autostash` | Stash uncommitted changes before rebasing and restore them afterwards | | Argument | Description | |----------|-------------| @@ -363,10 +383,16 @@ gh stack rebase [flags] [branch] Fetches the latest changes from `origin`, then ensures each branch in the stack has the tip of the previous layer in its commit history. Rebases branches in order from trunk upward. +Rebase requires no rebase in progress and no uncommitted changes to tracked files, because git refuses to rebase otherwise. Untracked files are fine. Commit or stash your changes first, or pass `--autostash` to have them stashed for the duration of the cascade and restored afterwards. If a conflict interrupts the rebase, the stash is restored by `--continue` or `--abort`. + +If the local trunk branch cannot be brought up to date, the stack is rebased onto the remote-tracking branch instead — see **When the trunk cannot be updated** under `gh stack sync` above. + If a branch's PR has been merged, the rebase automatically switches to `--onto` mode to correctly replay commits on top of the merge target. If a rebase conflict occurs, the operation pauses and prints the conflicted files with line numbers. Resolve the conflicts, stage with `git add`, and continue with `--continue`. To undo the entire rebase, use `--abort` to restore all branches to their pre-rebase state. +If git refuses to *start* a rebase — for example because a branch is checked out in another worktree — the command stops and reports git's own message instead of continuing. That is not a conflict: there is nothing to `--continue` or `--abort`. When the cascade finishes, rebase verifies that every branch it touched really does sit on top of its parent, and reports a failure if any does not. + **Examples:** ```sh @@ -390,6 +416,9 @@ gh stack rebase --abort # Rebase and preserve committer date as author date gh stack rebase --committer-date-is-author-date + +# Rebase with uncommitted changes in the working tree +gh stack rebase --autostash ``` ### `gh stack push` diff --git a/internal/git/git.go b/internal/git/git.go index 678d13f8..d6db721a 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "fmt" "os" "os/exec" @@ -64,14 +65,63 @@ func runInteractive(args ...string) error { return cmd.Run() } -// rebaseContinueOnce runs a single git rebase --continue without auto-resolve. -func rebaseContinueOnce(opts RebaseOpts) error { - args := []string{"rebase"} - if opts.CommitterDateIsAuthorDate { - args = append(args, "--committer-date-is-author-date") +// errRebaseAlreadyInProgress is returned when a rebase is started while an +// earlier one is still unresolved. +var errRebaseAlreadyInProgress = errors.New("a rebase is already in progress — resolve or abort it first") + +// RebaseStartError indicates that a git rebase command failed before it created +// any rebase state, meaning nothing was rebased at all. Common causes are a +// dirty working tree, a branch that is checked out in another worktree, an +// upstream ref that does not resolve, or a rebase that is already in progress. +// +// It is distinct from a rebase that started and stopped on a conflict: there is +// nothing to --continue or --abort, so callers must treat it as a hard failure +// rather than a recoverable conflict. +type RebaseStartError struct { + Err error +} + +func (e *RebaseStartError) Error() string { + // cli/cli's GitError prefixes its stderr with "failed to run git: ", which + // reads as noise once the message is wrapped in gh-stack's own context. + return strings.TrimSpace(strings.TrimPrefix(e.Err.Error(), "failed to run git: ")) +} + +func (e *RebaseStartError) Unwrap() error { + return e.Err +} + +// IsRebaseStartError reports whether err indicates a rebase that never started. +func IsRebaseStartError(err error) bool { + var rse *RebaseStartError + return errors.As(err, &rse) +} + +// runRebaseCommand starts a rebase and classifies the outcome. +// +// A rebase that is already in progress is reported as a *RebaseStartError up +// front: git would refuse the new rebase, and the leftover state belongs to the +// earlier one, so its conflicted files must not be mistaken for this rebase's +// conflicts. +func runRebaseCommand(args []string, opts RebaseOpts) error { + if IsRebaseInProgress() { + return &RebaseStartError{Err: errRebaseAlreadyInProgress} } - args = append(args, "--continue") - cmd := exec.Command("git", args...) + err := runSilent(args...) + if err == nil { + return nil + } + return tryAutoResolveRebase(err, opts) +} + +// rebaseContinueOnce runs a single git rebase --continue without auto-resolve. +// +// No rebase options are passed: git rejects "git rebase