Stop reporting rebases that git refused to run as successes

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)

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)
+}