diff --git a/cmd/rebase.go b/cmd/rebase.go index 9b79efdc..cd267000 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,9 @@ type rebaseState struct { OntoOldBase string `json:"ontoOldBase,omitempty"` CommitterDateIsAuthorDate bool `json:"committerDateIsAuthorDate,omitempty"` NoTrunk bool `json:"noTrunk,omitempty"` + AutoStash bool `json:"autoStash,omitempty"` + StartIndex int `json:"startIndex,omitempty"` + EndIndex int `json:"endIndex,omitempty"` } const rebaseStateFile = "gh-stack-rebase-state" @@ -51,6 +55,10 @@ 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 a clean working tree and no rebase in progress, since git +refuses to rebase otherwise. Use --autostash to let git stash your +local changes and restore them after the rebase. + 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 +74,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,6 +99,7 @@ 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 } @@ -120,6 +132,13 @@ 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 the working + // tree is 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 + } + // Enable git rerere so conflict resolutions are remembered. if err := ensureRerere(cfg); errors.Is(err, errInterrupt) { return ErrSilent @@ -222,6 +241,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { NeedsOnto: needsOnto, OntoOldBase: ontoOldBase, CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate, + AutoStash: opts.autostash, }) if rebaseResult.Err != nil { @@ -242,6 +262,9 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { OntoOldBase: rebaseResult.OntoOldBase, CommitterDateIsAuthorDate: opts.committerDateIsAuthorDate, NoTrunk: opts.noTrunk, + AutoStash: opts.autostash, + StartIndex: startIdx, + EndIndex: endIdx, } if err := saveRebaseState(gitDir, state); err != nil { cfg.Warningf("failed to save rebase state: %s", err) @@ -259,6 +282,13 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { _ = git.CheckoutBranch(currentBranch) + // The cascade reported success — verify the stack actually ended up + // stacked before saying so. + if unstacked := verifyStacked(s, startIdx, endIdx); len(unstacked) > 0 { + reportUnstacked(cfg, s, unstacked) + return ErrSilent + } + updateBaseSHAs(s) _ = syncStackPRs(cfg, s) @@ -385,6 +415,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { NeedsOnto: state.UseOnto, OntoOldBase: state.OntoOldBase, CommitterDateIsAuthorDate: state.CommitterDateIsAuthorDate, + AutoStash: state.AutoStash, }) if result.Err != nil { @@ -417,6 +448,21 @@ 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 + } + } + if unstacked := verifyStacked(s, verifyStart, verifyEnd); len(unstacked) > 0 { + reportUnstacked(cfg, s, unstacked) + return ErrSilent + } + updateBaseSHAs(s) _ = syncStackPRs(cfg, s) diff --git a/cmd/rebase_integration_test.go b/cmd/rebase_integration_test.go new file mode 100644 index 00000000..72555e90 --- /dev/null +++ b/cmd/rebase_integration_test.go @@ -0,0 +1,249 @@ +package cmd + +import ( + "encoding/json" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/github" + "github.com/github/gh-stack/internal/stack" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// These tests exercise `gh stack rebase` against a real git repository with a +// real remote, using the production git operations rather than mocks. They +// guard the class of bug where the command reports a successful cascade while +// git actually refused to do anything. + +// gitRun runs a git command in dir and fails the test if it errors. +func gitRun(t *testing.T, dir string, args ...string) string { + t.Helper() + out, err := gitTry(t, dir, args...) + require.NoError(t, err, "git %s in %s:\n%s", strings.Join(args, " "), dir, out) + return out +} + +// gitTry runs a git command in dir and returns its combined output and error. +func gitTry(t *testing.T, dir string, args ...string) (string, error) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=Test", + "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=Test", + "GIT_COMMITTER_EMAIL=test@test.com", + "GIT_CONFIG_NOSYSTEM=1", + ) + out, err := cmd.CombinedOutput() + return strings.TrimSpace(string(out)), err +} + +// commitFile writes a file and commits it in dir. +func commitFile(t *testing.T, dir, name, content, message string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0644)) + gitRun(t, dir, "add", ".") + gitRun(t, dir, "commit", "-m", message) +} + +// setupRealStackRepo creates a clone with a bare origin containing +// main <- b1 <- b2, pushes everything, then adds a new commit to main on the +// remote so a rebase has real work to do. Returns the clone directory and the +// SHA of the commit that landed on the remote trunk. +func setupRealStackRepo(t *testing.T) (string, string) { + t.Helper() + + bareDir := filepath.Join(t.TempDir(), "bare.git") + _, err := gitTry(t, ".", "-c", "safe.bareRepository=all", "init", "--bare", "-b", "main", bareDir) + require.NoError(t, err) + + cloneDir := filepath.Join(t.TempDir(), "clone") + gitRun(t, ".", "clone", bareDir, cloneDir) + gitRun(t, cloneDir, "config", "user.name", "Test") + gitRun(t, cloneDir, "config", "user.email", "test@test.com") + + commitFile(t, cloneDir, "init.txt", "hello", "initial commit") + gitRun(t, cloneDir, "push", "origin", "main") + + gitRun(t, cloneDir, "checkout", "-b", "b1") + commitFile(t, cloneDir, "b1.txt", "b1", "b1 work") + gitRun(t, cloneDir, "push", "-u", "origin", "b1") + + gitRun(t, cloneDir, "checkout", "-b", "b2") + commitFile(t, cloneDir, "b2.txt", "b2", "b2 work") + gitRun(t, cloneDir, "push", "-u", "origin", "b2") + + // Someone else lands a commit on main. + otherDir := filepath.Join(t.TempDir(), "other") + gitRun(t, ".", "clone", bareDir, otherDir) + gitRun(t, otherDir, "config", "user.name", "Other") + gitRun(t, otherDir, "config", "user.email", "other@test.com") + commitFile(t, otherDir, "upstream.txt", "landed", "upstream work") + gitRun(t, otherDir, "push", "origin", "main") + upstreamSHA := gitRun(t, otherDir, "rev-parse", "main") + + gitRun(t, cloneDir, "checkout", "b2") + return cloneDir, upstreamSHA +} + +// writeRealStackFile writes the stack file into the repo's .git directory. +func writeRealStackFile(t *testing.T, cloneDir string, s stack.Stack) { + t.Helper() + sf := &stack.StackFile{SchemaVersion: 1, Stacks: []stack.Stack{s}} + data, err := json.MarshalIndent(sf, "", " ") + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(cloneDir, ".git", "gh-stack"), data, 0644)) +} + +// runRealRebase runs the rebase command with real git ops inside cloneDir. +func runRealRebase(t *testing.T, cloneDir string, args []string) (string, error) { + t.Helper() + old, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(cloneDir)) + defer func() { _ = os.Chdir(old) }() + + cfg, _, errR := config.NewTestConfig() + // Keep the command off the network; the stack has no PRs. + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, + } + cmd := RebaseCmd(cfg) + cmd.SetArgs(args) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmdErr := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + return string(errOut), cmdErr +} + +// isAncestor reports whether ancestor is in descendant's history. +func isAncestor(t *testing.T, dir, ancestor, descendant string) bool { + t.Helper() + _, err := gitTry(t, dir, "merge-base", "--is-ancestor", ancestor, descendant) + return err == nil +} + +func twoBranchStack() stack.Stack { + return stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + } +} + +// TestIntegration_Rebase_PullsInTrunk is the regression test for stacks that +// stayed on their original base while the command reported success. +func TestIntegration_Rebase_PullsInTrunk(t *testing.T) { + cloneDir, upstreamSHA := setupRealStackRepo(t) + writeRealStackFile(t, cloneDir, twoBranchStack()) + + output, err := runRealRebase(t, cloneDir, nil) + require.NoError(t, err, output) + + assert.True(t, isAncestor(t, cloneDir, upstreamSHA, "b1"), "b1 must contain the new trunk commit") + assert.True(t, isAncestor(t, cloneDir, "b1", "b2"), "b2 must be stacked on b1") + assert.Contains(t, output, "rebased locally with main") + + // Each branch keeps exactly its own commit — nothing duplicated. + assert.Equal(t, "b1 work", gitRun(t, cloneDir, "log", "--format=%s", "main..b1")) + assert.Equal(t, "b2 work", gitRun(t, cloneDir, "log", "--format=%s", "b1..b2")) +} + +// TestIntegration_Rebase_DirtyTreeIsRejected verifies the command refuses to +// run rather than reporting a rebase it never performed. +func TestIntegration_Rebase_DirtyTreeIsRejected(t *testing.T) { + cloneDir, upstreamSHA := setupRealStackRepo(t) + writeRealStackFile(t, cloneDir, twoBranchStack()) + + require.NoError(t, os.WriteFile(filepath.Join(cloneDir, "init.txt"), []byte("uncommitted"), 0644)) + + output, err := runRealRebase(t, cloneDir, nil) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "uncommitted changes in working tree") + assert.NotContains(t, output, "rebased locally") + assert.False(t, isAncestor(t, cloneDir, upstreamSHA, "b1"), "the stack must be untouched") +} + +// TestIntegration_Rebase_AutostashRebasesAndRestores verifies --autostash both +// completes the rebase and puts the local changes back. +func TestIntegration_Rebase_AutostashRebasesAndRestores(t *testing.T) { + cloneDir, upstreamSHA := setupRealStackRepo(t) + writeRealStackFile(t, cloneDir, twoBranchStack()) + + require.NoError(t, os.WriteFile(filepath.Join(cloneDir, "init.txt"), []byte("uncommitted"), 0644)) + + output, err := runRealRebase(t, cloneDir, []string{"--autostash"}) + require.NoError(t, err, output) + + assert.True(t, isAncestor(t, cloneDir, upstreamSHA, "b1"), "b1 must contain the new trunk commit") + assert.True(t, isAncestor(t, cloneDir, "b1", "b2"), "b2 must be stacked on b1") + + status := gitRun(t, cloneDir, "status", "--porcelain") + assert.Contains(t, status, "init.txt", "autostash must restore the local changes") +} + +// TestIntegration_Rebase_BranchCheckedOutInAnotherWorktree covers the case +// reported by users working with multiple worktrees: git refuses to rebase a +// branch that is checked out elsewhere. +func TestIntegration_Rebase_BranchCheckedOutInAnotherWorktree(t *testing.T) { + cloneDir, _ := setupRealStackRepo(t) + writeRealStackFile(t, cloneDir, twoBranchStack()) + + gitRun(t, cloneDir, "checkout", "b1") + wtDir := filepath.Join(t.TempDir(), "wt") + gitRun(t, cloneDir, "worktree", "add", wtDir, "b2") + + output, err := runRealRebase(t, cloneDir, nil) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "could not start rebase of b2 onto b1") + assert.NotContains(t, output, "rebased locally") + assert.False(t, isAncestor(t, cloneDir, "b1", "b2"), "b2 must be untouched") +} + +// TestIntegration_Rebase_ParentAmendedDoesNotDuplicateCommits verifies that a +// parent branch rewritten outside gh-stack does not get its commits replayed +// onto the child. +func TestIntegration_Rebase_ParentAmendedDoesNotDuplicateCommits(t *testing.T) { + cloneDir, _ := setupRealStackRepo(t) + + // Record the parent tip b2 was stacked on, exactly as a prior + // rebase/sync/push would have. + b1Tip := gitRun(t, cloneDir, "rev-parse", "b1") + s := twoBranchStack() + s.Branches[1].Base = b1Tip + writeRealStackFile(t, cloneDir, s) + + // The user amends b1 out of band, changing its content. + gitRun(t, cloneDir, "checkout", "b1") + require.NoError(t, os.WriteFile(filepath.Join(cloneDir, "b1.txt"), []byte("b1 amended"), 0644)) + gitRun(t, cloneDir, "add", ".") + gitRun(t, cloneDir, "commit", "--amend", "-m", "b1 work (amended)") + gitRun(t, cloneDir, "checkout", "b2") + + output, err := runRealRebase(t, cloneDir, nil) + require.NoError(t, err, output) + + assert.True(t, isAncestor(t, cloneDir, "b1", "b2"), "b2 must be stacked on the amended b1") + assert.Equal(t, "b2 work", gitRun(t, cloneDir, "log", "--format=%s", "b1..b2"), + "b2 must contain only its own commit, not a replay of b1's rewritten commit") + assert.Equal(t, "b1 amended", readFileOnBranch(t, cloneDir, "b2", "b1.txt"), + "b2 must carry the amended content, not a duplicate of the old commit") +} + +// readFileOnBranch returns the contents of a file as of the given branch. +func readFileOnBranch(t *testing.T, dir, branch, path string) string { + t.Helper() + return gitRun(t, dir, "show", branch+":"+path) +} diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 683bfe36..74d93d82 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "slices" "strings" "testing" @@ -44,9 +45,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 +1293,10 @@ func TestRebase_FastForwardsBranchFromRemote(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, 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 - } + return a == "b1-local-sha" && d == "b1-remote-sha", nil + }) mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) return nil @@ -1413,10 +1411,8 @@ 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 - } + // Neither SHA is an ancestor of the other — diverged + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, nil) mock.UpdateBranchRefFn = func(string, string) error { updateBranchRefCalls++ return nil @@ -1963,3 +1959,217 @@ func TestRebase_NoTrunk_ConflictSavesState(t *testing.T) { assert.True(t, loaded.NoTrunk, "saved rebase state should preserve NoTrunk flag") } + +// --------------------------------------------------------------------------- +// Rebase failure classification, preflight checks, and post-rebase verification +// --------------------------------------------------------------------------- + +// threeBranchStack is the stack used by the failure-handling tests. +func threeBranchStack() stack.Stack { + return stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } +} + +// runRebaseArgs runs the rebase command with the given arguments and returns +// its stderr output and error. +func runRebaseArgs(t *testing.T, gitMock *git.MockOps, args []string) (string, error) { + t.Helper() + restore := git.SetOps(gitMock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cmd := RebaseCmd(cfg) + cmd.SetArgs(args) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + return string(errOut), err +} + +// TestRebase_StartFailure_IsFatal verifies that a rebase git refused to start +// (dirty tree, branch checked out elsewhere, bad upstream) aborts the cascade +// with git's own message instead of being reported as a successful rebase. +func TestRebase_StartFailure_IsFatal(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, threeBranchStack()) + + var rebaseCalls []string + mock := newRebaseMock(tmpDir, "b1") + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(base string, _ git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, base) + return &git.RebaseStartError{Err: fmt.Errorf("cannot rebase: You have unstaged changes")} + } + mock.RebaseOntoFn = func(newBase, _, _ string, _ git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, newBase) + return nil + } + + output, err := runRebaseArgs(t, mock, nil) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "could not start rebase of b1 onto main") + assert.Contains(t, output, "cannot rebase: You have unstaged changes") + assert.NotContains(t, output, "rebased locally", "must not claim the stack was rebased") + assert.Equal(t, []string{"main"}, rebaseCalls, "cascade must stop at the first hard failure") + + _, statErr := os.Stat(filepath.Join(tmpDir, rebaseStateFile)) + assert.True(t, os.IsNotExist(statErr), "a rebase that never started leaves nothing to --continue") +} + +// TestRebase_Conflict_StillRecoverable verifies that a genuine conflict keeps +// its existing behavior: rebase state is saved for --continue. +func TestRebase_Conflict_StillRecoverable(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, threeBranchStack()) + + mock := newRebaseMock(tmpDir, "b1") + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(string, git.RebaseOpts) error { return fmt.Errorf("conflict") } + + output, err := runRebaseArgs(t, mock, nil) + + assert.ErrorIs(t, err, ErrConflict) + assert.Contains(t, output, "conflict") + + _, statErr := os.Stat(filepath.Join(tmpDir, rebaseStateFile)) + assert.NoError(t, statErr, "a conflict must leave state for --continue") +} + +// TestRebase_Preflight verifies the checks that run before any refs are +// touched, and that --autostash lifts the clean-tree requirement. +func TestRebase_Preflight(t *testing.T) { + tests := []struct { + name string + dirty bool + rebaseActive bool + args []string + wantErr error + wantOutput string + wantRebaseRun bool + }{ + { + name: "dirty working tree is rejected", + dirty: true, + wantErr: ErrSilent, + wantOutput: "uncommitted changes in working tree", + }, + { + name: "rebase already in progress is rejected", + rebaseActive: true, + wantErr: ErrRebaseActive, + wantOutput: "a rebase is currently in progress", + }, + { + name: "autostash allows a dirty working tree", + dirty: true, + args: []string{"--autostash"}, + wantRebaseRun: true, + }, + { + name: "autostash does not bypass an in-progress rebase", + rebaseActive: true, + args: []string{"--autostash"}, + wantErr: ErrRebaseActive, + wantOutput: "a rebase is currently in progress", + }, + { + name: "clean tree proceeds", + wantRebaseRun: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, threeBranchStack()) + + var autostash []bool + mock := newRebaseMock(tmpDir, "b1") + mock.HasUncommittedChangesFn = func() (bool, error) { return tt.dirty, nil } + mock.IsRebaseInProgressFn = func() bool { return tt.rebaseActive } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(_ string, opts git.RebaseOpts) error { + autostash = append(autostash, opts.AutoStash) + return nil + } + mock.RebaseOntoFn = func(_, _, _ string, opts git.RebaseOpts) error { + autostash = append(autostash, opts.AutoStash) + return nil + } + + output, err := runRebaseArgs(t, mock, tt.args) + + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) + assert.Contains(t, output, tt.wantOutput) + } else { + assert.NoError(t, err) + } + + if !tt.wantRebaseRun { + assert.Empty(t, autostash, "no branch should be rebased when the preflight fails") + return + } + + require.Len(t, autostash, 3) + wantAutostash := slices.Contains(tt.args, "--autostash") + for _, got := range autostash { + assert.Equal(t, wantAutostash, got, "--autostash must be passed through to git") + } + }) + } +} + +// TestRebase_VerifiesStackAfterCascade verifies that a cascade which reports +// success but leaves a branch off its parent is treated as a failure rather +// than printing the success summary. +func TestRebase_VerifiesStackAfterCascade(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, threeBranchStack()) + + mock := newRebaseMock(tmpDir, "b1") + 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 } + // b2 never ends up on top of b1, even though every rebase "succeeded". + mock.IsAncestorFn = func(a, d string) (bool, error) { + return !(a == "b1" && d == "b2"), nil + } + + output, err := runRebaseArgs(t, mock, nil) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "Rebase reported success but this branch is still not based on its parent: b2") + assert.NotContains(t, output, "rebased locally") +} + +// TestRebase_NoTrunk_SkipsVerificationOfBottomBranch verifies that --no-trunk +// does not fail verification for the bottom branch, which is deliberately left +// off trunk. +func TestRebase_NoTrunk_SkipsVerificationOfBottomBranch(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, threeBranchStack()) + + mock := newRebaseMock(tmpDir, "b1") + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseOntoFn = func(string, string, string, git.RebaseOpts) error { return nil } + mock.IsAncestorFn = func(a, d string) (bool, error) { + // b1 is intentionally not on trunk. + return !(a == "main" && d == "b1"), nil + } + + output, err := runRebaseArgs(t, mock, []string{"--no-trunk"}) + + assert.NoError(t, err) + assert.Contains(t, output, "rebased locally (without trunk)") +} diff --git a/cmd/sync.go b/cmd/sync.go index 34bbbc05..6acb9f76 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,13 @@ 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 a clean working tree and no rebase in progress, since git refuses +to rebase otherwise. Use --autostash to let git stash your local changes +and restore them after the rebase. + +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 +78,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 +95,13 @@ func runSync(cfg *config.Config, opts *syncOptions) error { return ErrModifyRecovery } + // git refuses to rebase when another rebase is in progress or the working + // tree is 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 + } + sf := result.StackFile s := result.Stack currentBranch := result.CurrentBranch @@ -108,8 +124,12 @@ 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). 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 @@ -141,6 +161,15 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // --- Step 2: Fast-forward trunk --- trunk := s.Trunk.Branch + + // The cascade rebases the bottom branch onto the local trunk ref, so the + // trunk has to exist locally. Without this, git rejects the rebase with + // "invalid upstream". + if err := ensureLocalTrunk(cfg, trunk, remote); err != nil { + cfg.Errorf("%s", err) + return ErrSilent + } + trunkUpdated := fastForwardTrunk(cfg, trunk, remote, currentBranch) // --- Step 2b: Fast-forward stack branches behind their remote tracking branch --- @@ -169,6 +198,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { Branches: s.Branches, StartAbsIdx: 0, OriginalRefs: originalRefs, + AutoStash: opts.autostash, }) if result.Err != nil { @@ -197,6 +227,16 @@ func runSync(cfg *config.Config, opts *syncOptions) error { return ErrConflict } + // Never push branches that are not actually stacked. A rebase that + // reported success but left the stack unchanged must not be + // force-pushed over the remote. + if unstacked := verifyStacked(s, 0, len(s.Branches)); len(unstacked) > 0 { + _ = git.CheckoutBranch(currentBranch) + reportUnstacked(cfg, s, unstacked) + stack.SaveNonBlocking(gitDir, sf) + return ErrSilent + } + if result.Rebased { rebased = true } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 4f76d8e7..79cdba35 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -3,6 +3,7 @@ package cmd import ( "fmt" "io" + "slices" "strings" "testing" @@ -140,16 +141,18 @@ func TestSync_TrunkUpToDate_StackStale(t *testing.T) { return "sha-" + ref, nil } // Stack branches are NOT rebased onto trunk — parent is not an ancestor. + // main is NOT an ancestor of b1 until the cascade runs → stack is stale. + rebased := false 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 rebased, nil } return true, nil } mock.CheckoutBranchFn = func(string) error { return nil } mock.RebaseFn = func(base string, opts git.RebaseOpts) error { rebaseCalls = append(rebaseCalls, rebaseCall{branch: "(rebase)" + base}) + rebased = true return nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { @@ -300,9 +303,9 @@ func TestSync_TrunkFastForward_WhenOnTrunk(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, func(a, d string) (bool, error) { return a == "local-sha" && d == "remote-sha", nil - } + }) mock.MergeFFFn = func(target string) error { mergeFFCalls = append(mergeFFCalls, target) return nil @@ -481,9 +484,9 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, func(a, d string) (bool, error) { return a == "local-sha" && d == "remote-sha", nil - } + }) mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(name string) error { checkouts = append(checkouts, name) @@ -624,9 +627,9 @@ func TestSync_PushForceFlagDependsOnRebase(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, func(a, d string) (bool, error) { return a == "local-sha" && d == "remote-sha", nil - } + }) mock.UpdateBranchRefFn = func(string, string) error { return nil } } else { mock.RevParseFn = func(ref string) (string, error) { @@ -937,9 +940,9 @@ func TestSync_PushFailureAfterRebase(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, func(a, d string) (bool, error) { return a == "local-sha" && d == "remote-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 +1008,9 @@ 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 = stackedAncestry(defaultStackBranches, func(a, d string) (bool, error) { + return a == "b1-local-sha" && d == "b1-remote-sha", nil + }) mock.MergeFFFn = func(target string) error { mergeFFCalls = append(mergeFFCalls, target) return nil @@ -1095,15 +1095,12 @@ func TestSync_BranchFastForward_WithTrunkUpdate(t *testing.T) { } return "sha-" + ref, nil } - mock.IsAncestorFn = func(a, d string) (bool, error) { + mock.IsAncestorFn = stackedAncestry(defaultStackBranches, 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 - } + return a == "b2-local" && d == "b2-remote", nil + }) mock.UpdateBranchRefFn = func(branch, sha string) error { updateBranchRefCalls = append(updateBranchRefCalls, struct{ branch, sha string }{branch, sha}) return nil @@ -1960,6 +1957,12 @@ func TestSync_PRsSpanMultipleStacks_BranchesSynced(t *testing.T) { // caller to configure the Config (GitHub override, interactivity, SelectFn). // It returns the captured stderr output and the command's error. func runSyncCfg(t *testing.T, gitMock *git.MockOps, configure func(*config.Config)) (string, error) { + t.Helper() + return runSyncArgs(t, gitMock, nil, configure) +} + +// runSyncArgs runs the sync command with the given CLI arguments. +func runSyncArgs(t *testing.T, gitMock *git.MockOps, args []string, configure func(*config.Config)) (string, error) { t.Helper() restore := git.SetOps(gitMock) defer restore() @@ -1969,6 +1972,7 @@ func runSyncCfg(t *testing.T, gitMock *git.MockOps, configure func(*config.Confi configure(cfg) } cmd := SyncCmd(cfg) + cmd.SetArgs(args) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) err := cmd.Execute() @@ -2172,8 +2176,8 @@ func TestSync_RemoteAhead_DuplicateBranchAborts(t *testing.T) { } // TestSync_Divergent_UseRemote_DirtyCheckErrorAborts verifies that when the -// working-tree status cannot be determined, "use remote" aborts instead of -// treating the tree as clean and running the destructive replace. +// working-tree status cannot be determined, sync aborts instead of treating +// the tree as clean and running the destructive replace. func TestSync_Divergent_UseRemote_DirtyCheckErrorAborts(t *testing.T) { tmpDir := t.TempDir() divergentStack(t, tmpDir) @@ -2190,6 +2194,34 @@ func TestSync_Divergent_UseRemote_DirtyCheckErrorAborts(t *testing.T) { cfg.SelectFn = func(_, _ string, _ []string) (int, error) { return 0, nil } }) + assert.Error(t, err) + assert.Contains(t, output, "failed to check working tree status") + assert.Empty(t, created, "must not replace the local stack when the working-tree check fails") + + sf, loadErr := stack.Load(tmpDir) + require.NoError(t, loadErr) + assert.Equal(t, []string{"b1", "b2", "b3"}, sf.Stacks[0].BranchNames(), "local stack untouched") +} + +// TestSync_Divergent_UseRemote_DirtyCheckErrorAborts_Autostash verifies the +// same protection for --autostash, which skips the up-front clean-tree +// preflight and so relies on the divergence path's own check. +func TestSync_Divergent_UseRemote_DirtyCheckErrorAborts_Autostash(t *testing.T) { + tmpDir := t.TempDir() + divergentStack(t, tmpDir) + + ghMock := divergentRemoteMock() + var created []string + mock := newSyncMockNoRebase(tmpDir, "b1") + mock.CreateBranchFn = func(name, base string) error { created = append(created, name); return nil } + mock.HasUncommittedChangesFn = func() (bool, error) { return false, fmt.Errorf("git status failed") } + + output, err := runSyncArgs(t, mock, []string{"--autostash"}, func(cfg *config.Config) { + cfg.GitHubClientOverride = ghMock + cfg.ForceInteractive = true + cfg.SelectFn = func(_, _ string, _ []string) (int, error) { return 0, nil } + }) + assert.Error(t, err) assert.Contains(t, output, "Could not determine whether the working tree is clean") assert.Empty(t, created, "must not replace the local stack when the working-tree check fails") @@ -2571,3 +2603,210 @@ func TestSync_MergedBranchPruned_NoFalseDivergence(t *testing.T) { assert.Empty(t, created) assert.NotContains(t, output, "diverged") } + +// --------------------------------------------------------------------------- +// Sync failure classification, preflight checks, and post-rebase verification +// --------------------------------------------------------------------------- + +// staleSyncMock returns a sync mock whose stack is stale (b1 is not on trunk), +// so sync always runs the cascade rebase. +func staleSyncMock(tmpDir, currentBranch string) *git.MockOps { + mock := newSyncMockNoRebase(tmpDir, currentBranch) + mock.IsAncestorFn = func(a, d string) (bool, error) { + return !(a == "main" && d == "b1"), nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + return mock +} + +// TestSync_StartFailure_IsFatalAndDoesNotPush verifies that a rebase git +// refused to start aborts sync before pushing, instead of being reported as a +// successful rebase and force-pushed over the remote. +func TestSync_StartFailure_IsFatalAndDoesNotPush(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + }) + + var pushCalls []pushCall + mock := staleSyncMock(tmpDir, "b1") + mock.RebaseFn = func(string, git.RebaseOpts) error { + return &git.RebaseStartError{Err: fmt.Errorf("cannot rebase: You have unstaged changes")} + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + output, err := runSyncCfg(t, mock, nil) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "could not start rebase of b1 onto main") + assert.Empty(t, pushCalls, "nothing may be pushed when the rebase never happened") + assert.NotContains(t, output, "Stack synced") + assert.NotContains(t, output, "Branches synced") +} + +// TestSync_VerifiesStackBeforePushing verifies that a cascade which reports +// success but leaves a branch off its parent stops sync before the push. +func TestSync_VerifiesStackBeforePushing(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + }) + + var pushCalls []pushCall + mock := staleSyncMock(tmpDir, "b1") + // Every rebase "succeeds" but never moves a ref. + 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 + } + + output, err := runSyncCfg(t, mock, nil) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "Rebase reported success but this branch is still not based on its parent: b1") + assert.Empty(t, pushCalls, "an unrebased stack must not be force-pushed") +} + +// TestSync_Preflight verifies the checks that run before sync touches any refs. +func TestSync_Preflight(t *testing.T) { + tests := []struct { + name string + dirty bool + rebaseActive bool + args []string + wantErr error + wantOutput string + wantSync bool + }{ + { + name: "dirty working tree is rejected", + dirty: true, + wantErr: ErrSilent, + wantOutput: "uncommitted changes in working tree", + }, + { + name: "rebase already in progress is rejected", + rebaseActive: true, + wantErr: ErrRebaseActive, + wantOutput: "a rebase is currently in progress", + }, + { + name: "autostash allows a dirty working tree", + dirty: true, + args: []string{"--autostash"}, + wantSync: true, + }, + { + name: "clean tree proceeds", + wantSync: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + }) + + var autostash []bool + // b1 is off trunk until the cascade runs, so sync rebases. + rebased := false + mock := newSyncMockNoRebase(tmpDir, "b1") + mock.HasUncommittedChangesFn = func() (bool, error) { return tt.dirty, nil } + mock.IsRebaseInProgressFn = func() bool { return tt.rebaseActive } + mock.IsAncestorFn = func(a, d string) (bool, error) { + if a == "main" && d == "b1" { + return rebased, nil + } + return true, nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseFn = func(_ string, opts git.RebaseOpts) error { + autostash = append(autostash, opts.AutoStash) + rebased = true + return nil + } + mock.RebaseOntoFn = func(_, _, _ string, opts git.RebaseOpts) error { + autostash = append(autostash, opts.AutoStash) + return nil + } + + output, err := runSyncArgs(t, mock, tt.args, nil) + + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) + assert.Contains(t, output, tt.wantOutput) + assert.Empty(t, autostash, "nothing should be rebased when the preflight fails") + return + } + + assert.NoError(t, err) + require.Len(t, autostash, 2) + wantAutostash := slices.Contains(tt.args, "--autostash") + for _, got := range autostash { + assert.Equal(t, wantAutostash, got, "--autostash must be passed through to git") + } + }) + } +} + +// TestSync_CreatesMissingLocalTrunk verifies that sync creates the local trunk +// branch when it is absent. Without it the cascade's `git rebase ` +// fails with "invalid upstream". +func TestSync_CreatesMissingLocalTrunk(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + }) + + var created []struct{ name, base string } + mock := newSyncMockNoRebase(tmpDir, "b1") + mock.BranchExistsFn = func(name string) bool { return name != "main" } + mock.CreateBranchFn = func(name, base string) error { + created = append(created, struct{ name, base string }{name, base}) + 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 } + + output, err := runSyncCfg(t, mock, nil) + + assert.NoError(t, err) + require.Len(t, created, 1) + assert.Equal(t, "main", created[0].name) + assert.Equal(t, "origin/main", created[0].base) + assert.Contains(t, output, "Created local trunk branch main") +} + +// TestSync_ReportsFetchFailure verifies that sync says so when the fetch fails +// instead of unconditionally reporting that it fetched. +func TestSync_ReportsFetchFailure(t *testing.T) { + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + }) + + mock := newSyncMockNoRebase(tmpDir, "b1") + mock.FetchBranchesFn = func(string, []string) error { + return fmt.Errorf("could not read from remote repository") + } + mock.CheckoutBranchFn = func(string) error { return nil } + + output, err := runSyncCfg(t, mock, nil) + + assert.NoError(t, err, "a failed fetch is a warning, not a hard stop") + assert.Contains(t, output, "Failed to fetch from origin") + assert.NotContains(t, output, "Fetched latest changes") +} diff --git a/cmd/utils.go b/cmd/utils.go index 1c5dc139..9f62fd2a 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -909,6 +909,53 @@ func ensureLocalTrunk(cfg *config.Config, trunk, remote string) error { return nil } +// requireNoRebaseInProgress fails when a rebase is already in progress. git +// refuses to start another one, so continuing would leave the caller reporting +// work it never did. +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 the working tree has uncommitted changes. +// git refuses to rebase a dirty tree, so this turns a confusing mid-cascade +// failure into an actionable message up front. 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.HasUncommittedChanges() + 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, git stashes and restores local changes itself, so the +// clean-tree requirement is lifted. +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) +} + // 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 { @@ -926,7 +973,15 @@ func fastForwardTrunk(cfg *config.Config, trunk, remote, currentBranch string) b } if trunkErr != nil { - cfg.Warningf("Could not compare trunk %s with remote — skipping trunk update", trunk) + // The most common cause is that the trunk branch no longer exists on + // the remote — for example a stack whose trunk was itself a feature + // branch that has since been merged and deleted. Without a resolvable + // remote ref the cascade silently rebases onto a stale local trunk, so + // say so explicitly. + cfg.Warningf("Could not resolve %s/%s — skipping trunk update", remote, trunk) + cfg.Printf(" The stack will be rebased onto the local %s, which may be out of date.", trunk) + cfg.Printf(" If %s was merged and deleted on %s, re-point the stack with `%s`.", + trunk, remote, cfg.ColorCyan("gh stack init")) return false } @@ -973,6 +1028,7 @@ type cascadeRebaseOpts struct { NeedsOnto bool OntoOldBase string CommitterDateIsAuthorDate bool + AutoStash bool } // cascadeRebaseResult describes the outcome of a cascade rebase. @@ -988,6 +1044,68 @@ 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 +// that the branch does not actually contain makes git replay commits that are +// already represented in newBase — the cause of duplicate commits after a +// parent branch was amended, reordered, or squash-merged. +// +// 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. +// 2. metadataBase — the parent tip the branch was last stacked on, from the +// stack file. Correct when the parent moved out of band. +// 3. 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 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. + 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,7 +1116,19 @@ 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, + AutoStash: opts.AutoStash, + } + + // remainingAfter lists the branch names after position i in the range. + remainingAfter := func(i int) []string { + 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) + } + return remaining + } for i, br := range opts.Branches { absIdx := opts.StartAbsIdx + i @@ -1042,21 +1172,14 @@ 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 - } - } + actualOldBase := resolveOntoOldBase(ontoOldBase, br.Base, newBase, br.Branch) if err := git.RebaseOnto(newBase, actualOldBase, br.Branch, rebaseOpts); err != nil { - 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) + if git.IsRebaseStartError(err) { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: rebaseStartFailure(br.Branch, newBase, err), + } } return cascadeRebaseResult{ Rebased: result.Rebased, @@ -1064,7 +1187,7 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { ConflictIdx: absIdx, ConflictBranch: br.Branch, ConflictBase: newBase, - Remaining: remaining, + Remaining: remainingAfter(i), NeedsOnto: true, OntoOldBase: originalRefs[br.Branch], } @@ -1076,7 +1199,8 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } else { var rebaseErr error if absIdx > 0 { - rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch, rebaseOpts) + 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,9 +1212,11 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } if rebaseErr != nil { - 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) + if git.IsRebaseStartError(rebaseErr) { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: rebaseStartFailure(br.Branch, base, rebaseErr), + } } return cascadeRebaseResult{ Rebased: result.Rebased, @@ -1098,7 +1224,7 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { ConflictIdx: absIdx, ConflictBranch: br.Branch, ConflictBase: base, - Remaining: remaining, + Remaining: remainingAfter(i), NeedsOnto: false, OntoOldBase: originalRefs[br.Branch], } @@ -1112,12 +1238,39 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { return result } +// rebaseStartFailure describes a rebase that never started. git's own message +// (dirty working tree, branch checked out elsewhere, unresolvable upstream, +// rebase already in progress) is the useful part, 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 { + return len(verifyStacked(s, 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 trunk) 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 happen and +// reporting success would be a lie. +func verifyStacked(s *stack.Stack, startIdx, endIdx int) []string { trunk := s.Trunk.Branch - for i, br := range s.Branches { + 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 } @@ -1131,10 +1284,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, s *stack.Stack, 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", s.Trunk.Branch) + 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..54d4157e 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -851,3 +851,183 @@ 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") } + +// defaultStackBranches lists the branch names used by most cascade tests. +var defaultStackBranches = []string{"main", "trunk", "b1", "b2", "b3", "b4"} + +// stackedAncestry wraps an IsAncestor mock so that ancestry questions about +// plain branch names answer true, while questions about the fake SHAs used to +// drive fast-forward detection fall through to fn. +// +// Mock rebases never move refs, so without this the post-rebase verification +// (verifyStacked) would always see an unstacked stack. +func stackedAncestry(branches []string, fn func(a, d string) (bool, error)) func(string, string) (bool, error) { + known := make(map[string]bool, len(branches)) + for _, b := range branches { + known[b] = true + } + return func(a, d string) (bool, error) { + if known[a] && known[d] { + return true, nil + } + if fn == nil { + return false, nil + } + return fn(a, d) + } +} + +// TestResolveOntoOldBase covers the `git rebase --onto ` +// upstream selection. Passing a commit the branch does not contain makes git +// replay commits that are already in the new base, which is how a cascade +// rebase ends up duplicating a parent's commits. +func TestResolveOntoOldBase(t *testing.T) { + // History used throughout: fork <- p1 <- p2 (parent), and the child was + // built on p1 before the parent moved on to p2. + ancestorsOfChild := map[string]bool{"fork": true, "p1": true} + + tests := []struct { + name string + recordedOldBase string + metadataBase string + newBase string + mergeBases map[string]string // "a|b" -> merge-base + want string + wantReason string + }{ + { + name: "recorded old base is used when the branch contains it", + recordedOldBase: "p1", + metadataBase: "fork", + newBase: "parent", + want: "p1", + wantReason: "the common case must not pay for extra lookups", + }, + { + name: "falls back to the recorded metadata base when the parent moved", + recordedOldBase: "p2", + metadataBase: "p1", + newBase: "parent", + mergeBases: map[string]string{"p2|child": "fork", "parent|child": "fork"}, + want: "p1", + wantReason: "p1 is the latest commit the child actually contains", + }, + { + name: "uses the latest merge-base when no metadata base is recorded", + recordedOldBase: "p2", + metadataBase: "", + newBase: "parent", + mergeBases: map[string]string{"p2|child": "p1", "parent|child": "fork"}, + want: "p1", + wantReason: "replaying from fork would duplicate the parent's commits", + }, + { + name: "prefers the new base merge-base when it is later", + recordedOldBase: "p2", + metadataBase: "fork", + newBase: "parent", + mergeBases: map[string]string{"p2|child": "fork", "parent|child": "p1"}, + want: "p1", + wantReason: "the child is already on top of part of the new base", + }, + { + name: "keeps the recorded old base when nothing better is an ancestor", + recordedOldBase: "p2", + metadataBase: "unrelated", + newBase: "parent", + mergeBases: map[string]string{"p2|child": "unrelated", "parent|child": "unrelated"}, + want: "p2", + wantReason: "never invent an upstream out of thin air", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + if d == "child" { + return ancestorsOfChild[a], nil + } + // Ordering between two ancestors of the child. + return a == "fork" && d == "p1", nil + }, + MergeBaseFn: func(a, b string) (string, error) { + mb, ok := tt.mergeBases[a+"|"+b] + if !ok { + return "", fmt.Errorf("no merge base for %s and %s", a, b) + } + return mb, nil + }, + }) + defer restore() + + got := resolveOntoOldBase(tt.recordedOldBase, tt.metadataBase, tt.newBase, "child") + assert.Equal(t, tt.want, got, tt.wantReason) + }) + } +} + +func TestVerifyStacked(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + // b1 is not on main and b3 is not on b2. + broken := map[string]bool{"main|b1": true, "b2|b3": true} + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { return !broken[a+"|"+d], nil }, + }) + defer restore() + + tests := []struct { + name string + start int + end int + want []string + wantWhy string + }{ + {name: "whole stack", start: 0, end: 3, want: []string{"b1", "b3"}}, + {name: "no-trunk range skips the bottom branch", start: 1, end: 3, want: []string{"b3"}, + wantWhy: "b1 is deliberately left off trunk by --no-trunk"}, + {name: "downstack range", start: 0, end: 2, want: []string{"b1"}}, + {name: "upstack range", start: 2, end: 3, want: []string{"b3"}}, + {name: "out of range end is clamped", start: 0, end: 99, want: []string{"b1", "b3"}}, + {name: "negative start is clamped", start: -5, end: 1, want: []string{"b1"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, verifyStacked(s, tt.start, tt.end), tt.wantWhy) + }) + } +} + +// TestVerifyStacked_SkipsMergedAndQueued verifies that merged and queued +// branches are excluded and that their children are checked against the +// nearest active ancestor instead. +func TestVerifyStacked_SkipsMergedAndQueued(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2", Queued: true}, + {Branch: "b3"}, + }, + } + var checked []string + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, d string) (bool, error) { + checked = append(checked, a+"->"+d) + return true, nil + }, + }) + defer restore() + + assert.Empty(t, verifyStacked(s, 0, 3)) + assert.Equal(t, []string{"main->b3"}, checked, + "b3's nearest active ancestor is trunk, since b1 is merged and b2 is queued") +} diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index a102281e..ad9ae7cd 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -174,6 +174,14 @@ gh stack rebase --upstack gh stack rebase --no-trunk ``` +Rebasing needs a clean working tree, because git refuses to rebase over uncommitted changes. Commit or stash your work first, or let git do 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. + 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..cf0e639c 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -306,13 +306,16 @@ 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 a clean working tree and no rebase in progress, because git refuses to rebase otherwise. Commit or stash your changes first, or pass `--autostash` to let git stash and restore them around the rebase. Performs a synchronization of the entire stack: 1. **Fetch** — fetches the latest changes from `origin`. 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. +3. **Fast-forward trunk** — creates the local trunk branch if it is missing, then fast-forwards it to match the remote (skips if diverged, or if the trunk branch no longer exists on the remote). +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. Afterwards sync verifies that every branch really does sit on top of its parent, and stops without pushing if any does not. 5. **Push** — pushes all branches (uses `--force-with-lease` if a rebase occurred). 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). @@ -337,6 +340,9 @@ 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 ``` ### `gh stack rebase` @@ -356,6 +362,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 +370,14 @@ 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 a clean working tree and no rebase in progress, because git refuses to rebase otherwise. Commit or stash your changes first, or pass `--autostash` to let git stash and restore them around the rebase. + 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 message instead of continuing. When the cascade finishes, it verifies that every rebased branch really does sit on top of its parent and reports a failure if any does not. + **Examples:** ```sh @@ -390,6 +401,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..3cbb8593 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,65 @@ 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") +// 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) +} + +// errRebaseAlreadyInProgress is returned when a rebase is started while +// another one is 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`, and 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 (see RebaseStartError). +func IsRebaseStartError(err error) bool { + var rse *RebaseStartError + return errors.As(err, &rse) +} + +// rebaseContinueOnce runs a single git rebase --continue without auto-resolve. +// +// No rebase options are passed: git rejects `git rebase