diff --git a/cmd/add.go b/cmd/add.go index 7e50a52..ca42fdd 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -169,6 +169,14 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { // If the branch already exists in git but is not part of any stack, // adopt it instead of erroring. This mirrors the init command's behavior. adopted := git.BranchExists(branchName) + var adoptedBase string + if adopted { + adoptedBase, err = git.MergeBase(currentBranch, branchName) + if err != nil { + cfg.Errorf("failed to determine the common base of %s and %s: %s", currentBranch, branchName, err) + return ErrSilent + } + } // Stage changes before creating the branch so we can fail early if // there's nothing to commit (avoids leaving an empty orphan branch). @@ -191,9 +199,12 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { return ErrSilent } - base, err := git.RevParse(currentBranch) - if err != nil { - cfg.Warningf("could not resolve base SHA for %s: %s", currentBranch, err) + base := adoptedBase + if !adopted { + base, err = git.RevParse(currentBranch) + if err != nil { + cfg.Warningf("could not resolve base SHA for %s: %s", currentBranch, err) + } } s.Branches = append(s.Branches, stack.BranchRef{Branch: branchName, Base: base}) diff --git a/cmd/add_test.go b/cmd/add_test.go index 9bb82d2..6a7eb25 100644 --- a/cmd/add_test.go +++ b/cmd/add_test.go @@ -446,6 +446,11 @@ func TestAdd_AdoptsExistingBranch(t *testing.T) { GitDirFn: func() (string, error) { return gitDir, nil }, CurrentBranchFn: func() (string, error) { return "b1", nil }, BranchExistsFn: func(name string) bool { return name == "existing-branch" }, + MergeBaseFn: func(parent, branch string) (string, error) { + assert.Equal(t, "b1", parent) + assert.Equal(t, "existing-branch", branch) + return "common-base", nil + }, CreateBranchFn: func(name, base string) error { createBranchCalled = true return nil @@ -472,6 +477,8 @@ func TestAdd_AdoptsExistingBranch(t *testing.T) { require.NoError(t, err) names := sf.Stacks[0].BranchNames() assert.Equal(t, "existing-branch", names[len(names)-1], "adopted branch appended to stack") + assert.Equal(t, "common-base", sf.Stacks[0].Branches[len(sf.Stacks[0].Branches)-1].Base, + "adopted branch should record the actual common ancestor") } func TestAdd_RejectsExistingBranchInStack(t *testing.T) { @@ -520,6 +527,7 @@ func TestAdd_AdoptsExistingBranchWithCommit(t *testing.T) { GitDirFn: func() (string, error) { return gitDir, nil }, CurrentBranchFn: func() (string, error) { return "b1", nil }, BranchExistsFn: func(name string) bool { return name == "existing-branch" }, + MergeBaseFn: func(string, string) (string, error) { return "common-base", nil }, RevParseMultiFn: func(refs []string) ([]string, error) { return []string{"aaa", "bbb"}, nil // different SHAs = branch has commits }, @@ -548,3 +556,36 @@ func TestAdd_AdoptsExistingBranchWithCommit(t *testing.T) { assert.True(t, commitCalled, "Commit should be called on the adopted branch") assert.Contains(t, output, "Adopted") } + +func TestAdd_AdoptExistingBranchWithoutCommonBaseFails(t *testing.T) { + gitDir := t.TempDir() + saveStack(t, gitDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}}, + }) + + checkedOut := false + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return gitDir, nil }, + CurrentBranchFn: func() (string, error) { return "b1", nil }, + BranchExistsFn: func(name string) bool { return name == "unrelated" }, + MergeBaseFn: func(string, string) (string, error) { return "", assert.AnError }, + CheckoutBranchFn: func(string) error { + checkedOut = true + return nil + }, + }) + defer restore() + + cfg, outR, errR := config.NewTestConfig() + err := runAdd(cfg, &addOptions{}, []string{"unrelated"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrSilent) + assert.False(t, checkedOut) + assert.Contains(t, output, "failed to determine the common base") + + sf, loadErr := stack.Load(gitDir) + require.NoError(t, loadErr) + assert.Equal(t, []string{"b1"}, sf.Stacks[0].BranchNames()) +} diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index c8adc0a..1be2f9f 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -2,9 +2,11 @@ package cmd import ( "encoding/json" + "errors" "fmt" "io" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -242,11 +244,11 @@ func TestRebase_OntoPropagatesToSubsequentBranches(t *testing.T) { "b4 should rebase --onto b3 with b3's original SHA as oldBase") } -// TestRebase_StaleOntoOldBase_FallsBackToMergeBase verifies that when a branch +// TestRebase_StaleOntoOldBase_UsesForkPoint verifies that when a branch // was already rebased past the merged branch's tip (e.g. by a previous run), -// the stale ontoOldBase is detected via IsAncestor and replaced with -// merge-base(newBase, branch) to avoid replaying already-applied commits. -func TestRebase_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { +// the stale ontoOldBase is replaced with a reflog fork-point that the branch +// actually contains. +func TestRebase_StaleOntoOldBase_UsesForkPoint(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -286,11 +288,11 @@ func TestRebase_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { } return true, nil } - mock.MergeBaseFn = func(a, b string) (string, error) { + mock.MergeBaseForkPointFn = func(a, b string) (string, error) { if a == "main" && b == "b2" { - return "main-b2-mergebase", nil + return "main-b2-forkpoint", nil } - return "default-mergebase", nil + return "default-forkpoint", nil } mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) @@ -312,9 +314,9 @@ func TestRebase_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { assert.NoError(t, err) require.Len(t, rebaseCalls, 2) - // b2: stale ontoOldBase detected → falls back to merge-base(main, b2) - assert.Equal(t, rebaseCall{"main", "main-b2-mergebase", "b2"}, rebaseCalls[0], - "b2 should use merge-base as oldBase when ontoOldBase is stale") + // b2: stale ontoOldBase detected → uses fork-point(main, b2) + assert.Equal(t, rebaseCall{"main", "main-b2-forkpoint", "b2"}, rebaseCalls[0], + "b2 should use the reflog fork-point when ontoOldBase is stale") // b3: b2's SHA is a valid ancestor → uses it directly assert.Equal(t, rebaseCall{"b2", "b2-on-main-sha", "b3"}, rebaseCalls[1], @@ -643,7 +645,7 @@ func TestRebase_SkipsMergedBranches(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 42, Merged: true}}, + {Branch: "b1", Head: "sha-b1", PullRequest: &stack.PullRequestRef{Number: 42, Merged: true}}, {Branch: "b2"}, }, } @@ -1963,3 +1965,296 @@ func TestRebase_NoTrunk_ConflictSavesState(t *testing.T) { assert.True(t, loaded.NoTrunk, "saved rebase state should preserve NoTrunk flag") } + +func TestResolveRebaseOldBase(t *testing.T) { + t.Run("uses current parent tip when the branch contains it", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(ancestor, branch string) (bool, error) { + return ancestor == "current-parent" && branch == "child", nil + }, + }) + defer restore() + + oldBase, err := resolveRebaseOldBase("current-parent", "recorded-base", "parent", "child") + require.NoError(t, err) + assert.Equal(t, "current-parent", oldBase) + }) + + t.Run("uses recorded base after the parent was rewritten", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(ancestor, branch string) (bool, error) { + return ancestor == "recorded-base" && branch == "child", nil + }, + }) + defer restore() + + oldBase, err := resolveRebaseOldBase("amended-parent", "recorded-base", "parent", "child") + require.NoError(t, err) + assert.Equal(t, "recorded-base", oldBase) + }) + + t.Run("uses fork point when metadata was already corrupted", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(ancestor, branch string) (bool, error) { + return ancestor == "old-parent" && branch == "child", nil + }, + MergeBaseForkPointFn: func(ref, branch string) (string, error) { + return "old-parent", nil + }, + }) + defer restore() + + oldBase, err := resolveRebaseOldBase("amended-parent", "amended-parent", "parent", "child") + require.NoError(t, err) + assert.Equal(t, "old-parent", oldBase) + }) + + t.Run("fails when no safe boundary can be recovered", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(string, string) (bool, error) { return false, nil }, + MergeBaseForkPointFn: func(string, string) (string, error) { + return "", errors.New("no fork point") + }, + }) + defer restore() + + _, err := resolveRebaseOldBase("amended-parent", "amended-parent", "parent", "child") + require.Error(t, err) + assert.Contains(t, err.Error(), "rebase this branch manually") + }) +} + +type amendedParentRepo struct { + dir string + gitDir string + oldParent string + newParent string +} + +func issue250Git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=Test", + "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=Test", + "GIT_COMMITTER_EMAIL=test@example.com", + ) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %s:\n%s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) +} + +func issue250GitMayFail(t *testing.T, dir string, args ...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@example.com", + "GIT_COMMITTER_NAME=Test", + "GIT_COMMITTER_EMAIL=test@example.com", + ) + return cmd.Run() +} + +func issue250WriteFile(t *testing.T, dir, name, content string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0644)) +} + +func setupAmendedParentRepo(t *testing.T, corruptBase bool) amendedParentRepo { + t.Helper() + remoteDir := filepath.Join(t.TempDir(), "remote.git") + cloneDir := filepath.Join(t.TempDir(), "clone") + + issue250Git(t, ".", "-c", "safe.bareRepository=all", "init", "--bare", "-b", "main", remoteDir) + issue250Git(t, ".", "clone", remoteDir, cloneDir) + issue250Git(t, cloneDir, "config", "user.name", "Test") + issue250Git(t, cloneDir, "config", "user.email", "test@example.com") + + issue250WriteFile(t, cloneDir, "base.txt", "base\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "base") + issue250Git(t, cloneDir, "push", "-u", "origin", "main") + mainSHA := issue250Git(t, cloneDir, "rev-parse", "main") + + issue250Git(t, cloneDir, "checkout", "-b", "parent") + issue250WriteFile(t, cloneDir, "old-parent.txt", "old parent\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "parent old") + oldParent := issue250Git(t, cloneDir, "rev-parse", "parent") + issue250Git(t, cloneDir, "push", "-u", "origin", "parent") + + issue250Git(t, cloneDir, "checkout", "-b", "child") + issue250WriteFile(t, cloneDir, "child.txt", "child\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "child commit") + childSHA := issue250Git(t, cloneDir, "rev-parse", "child") + issue250Git(t, cloneDir, "push", "-u", "origin", "child") + + gitDir := filepath.Join(cloneDir, ".git") + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main", Head: mainSHA}, + Branches: []stack.BranchRef{ + {Branch: "parent", Head: oldParent, Base: mainSHA}, + {Branch: "child", Head: childSHA, Base: oldParent}, + }, + } + writeStackFile(t, gitDir, s) + + issue250Git(t, cloneDir, "checkout", "parent") + issue250Git(t, cloneDir, "rm", "old-parent.txt") + issue250WriteFile(t, cloneDir, "new-parent.txt", "new parent\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "--amend", "-m", "parent amended") + newParent := issue250Git(t, cloneDir, "rev-parse", "parent") + + if corruptBase { + issue250Git(t, cloneDir, "push", "--force", "origin", "parent") + s.Branches[0].Head = newParent + s.Branches[1].Base = newParent + writeStackFile(t, gitDir, s) + } + issue250Git(t, cloneDir, "checkout", "child") + + return amendedParentRepo{ + dir: cloneDir, + gitDir: gitDir, + oldParent: oldParent, + newParent: newParent, + } +} + +func issue250TestConfig(t *testing.T) *config.Config { + t.Helper() + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{} + t.Cleanup(func() { + _ = cfg.Out.Close() + _ = cfg.Err.Close() + _ = outR.Close() + _ = errR.Close() + }) + return cfg +} + +func withIssue250Repo(t *testing.T, dir string) { + t.Helper() + originalDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(dir)) + t.Cleanup(func() { _ = os.Chdir(originalDir) }) +} + +func assertIssue250History(t *testing.T, repo amendedParentRepo) { + t.Helper() + subjects := strings.Split(issue250Git(t, repo.dir, "log", "--format=%s", "main..child"), "\n") + assert.Equal(t, []string{"child commit", "parent amended"}, subjects) + assert.Error(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", repo.oldParent, "child")) + require.NoError(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", repo.newParent, "child")) + _, oldErr := os.Stat(filepath.Join(repo.dir, "old-parent.txt")) + assert.True(t, os.IsNotExist(oldErr)) + _, newErr := os.Stat(filepath.Join(repo.dir, "new-parent.txt")) + assert.NoError(t, newErr) +} + +func TestIntegration_AmendedParentPushThenRebase(t *testing.T) { + repo := setupAmendedParentRepo(t, false) + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + + require.NoError(t, runPush(cfg, &pushOptions{remote: "origin"})) + + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + require.Len(t, sf.Stacks, 1) + assert.Equal(t, repo.oldParent, sf.Stacks[0].Branches[1].Base, + "push must not replace the child's valid base with an amended parent tip") + + require.NoError(t, runRebase(cfg, &rebaseOptions{remote: "origin"})) + assertIssue250History(t, repo) +} + +func TestIntegration_AmendedParentRecoversCorruptedBase(t *testing.T) { + repo := setupAmendedParentRepo(t, true) + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + + require.NoError(t, runRebase(cfg, &rebaseOptions{remote: "origin"})) + assertIssue250History(t, repo) +} + +func TestIntegration_AmendedParentWithoutForkPointFailsSafely(t *testing.T) { + repo := setupAmendedParentRepo(t, true) + issue250Git(t, repo.dir, "reflog", "expire", "--expire=now", "--all") + require.Error(t, issue250GitMayFail(t, repo.dir, "merge-base", "--fork-point", "parent", "child")) + + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + parentBefore := issue250Git(t, repo.dir, "rev-parse", "parent") + childBefore := issue250Git(t, repo.dir, "rev-parse", "child") + + err := runRebase(cfg, &rebaseOptions{remote: "origin"}) + require.Error(t, err) + assert.Equal(t, parentBefore, issue250Git(t, repo.dir, "rev-parse", "parent")) + assert.Equal(t, childBefore, issue250Git(t, repo.dir, "rev-parse", "child")) +} + +func TestIntegration_AdoptedBranchRebasesFromCommonAncestor(t *testing.T) { + remoteDir := filepath.Join(t.TempDir(), "remote.git") + cloneDir := filepath.Join(t.TempDir(), "clone") + + issue250Git(t, ".", "-c", "safe.bareRepository=all", "init", "--bare", "-b", "main", remoteDir) + issue250Git(t, ".", "clone", remoteDir, cloneDir) + issue250Git(t, cloneDir, "config", "user.name", "Test") + issue250Git(t, cloneDir, "config", "user.email", "test@example.com") + + issue250WriteFile(t, cloneDir, "base.txt", "base\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "base") + issue250Git(t, cloneDir, "push", "-u", "origin", "main") + mainSHA := issue250Git(t, cloneDir, "rev-parse", "main") + + issue250Git(t, cloneDir, "checkout", "-b", "parent") + issue250WriteFile(t, cloneDir, "parent.txt", "parent\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "parent commit") + parentSHA := issue250Git(t, cloneDir, "rev-parse", "parent") + + issue250Git(t, cloneDir, "checkout", "-b", "imported", "main") + issue250WriteFile(t, cloneDir, "imported-one.txt", "one\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "imported one") + issue250WriteFile(t, cloneDir, "imported-two.txt", "two\n") + issue250Git(t, cloneDir, "add", ".") + issue250Git(t, cloneDir, "commit", "-m", "imported two") + + gitDir := filepath.Join(cloneDir, ".git") + writeStackFile(t, gitDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main", Head: mainSHA}, + Branches: []stack.BranchRef{ + {Branch: "parent", Head: parentSHA, Base: mainSHA}, + }, + }) + + issue250Git(t, cloneDir, "checkout", "parent") + withIssue250Repo(t, cloneDir) + cfg := issue250TestConfig(t) + + require.NoError(t, runAdd(cfg, &addOptions{}, []string{"imported"})) + + sf, err := stack.Load(gitDir) + require.NoError(t, err) + require.Len(t, sf.Stacks, 1) + require.Len(t, sf.Stacks[0].Branches, 2) + assert.Equal(t, mainSHA, sf.Stacks[0].Branches[1].Base, + "adopting a separate main-based branch should record main as its old boundary") + + require.NoError(t, runRebase(cfg, &rebaseOptions{remote: "origin"})) + + subjects := strings.Split(issue250Git(t, cloneDir, "log", "--format=%s", "main..imported"), "\n") + assert.Equal(t, []string{"imported two", "imported one", "parent commit"}, subjects) + require.NoError(t, issue250GitMayFail(t, cloneDir, "merge-base", "--is-ancestor", "parent", "imported")) +} diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 442aef5..ee36498 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -495,7 +495,7 @@ func TestSync_RebaseConflict_RestoresAll(t *testing.T) { return "sha-" + ref, nil } mock.IsAncestorFn = func(a, d string) (bool, error) { - return a == "local-sha" && d == "remote-sha", nil + return true, nil } mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(name string) error { @@ -840,10 +840,10 @@ func TestSync_QueuedBranch_DownstreamStaysStacked(t *testing.T) { "queued b1 must not be pushed") } -// TestSync_StaleOntoOldBase_FallsBackToMergeBase verifies that when a branch +// TestSync_StaleOntoOldBase_UsesForkPoint verifies that when a branch // was already rebased past the merged branch's tip, sync detects the stale -// ontoOldBase and falls back to merge-base for the correct divergence point. -func TestSync_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { +// ontoOldBase and uses a reflog fork-point for the correct divergence point. +func TestSync_StaleOntoOldBase_UsesForkPoint(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -889,11 +889,11 @@ func TestSync_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { } return true, nil } - mock.MergeBaseFn = func(a, b string) (string, error) { + mock.MergeBaseForkPointFn = func(a, b string) (string, error) { if a == "main" && b == "b2" { - return "main-b2-mergebase", nil + return "main-b2-forkpoint", nil } - return "default-mergebase", nil + return "default-forkpoint", nil } mock.UpdateBranchRefFn = func(string, string) error { return nil } mock.CheckoutBranchFn = func(string) error { return nil } @@ -918,9 +918,9 @@ func TestSync_StaleOntoOldBase_FallsBackToMergeBase(t *testing.T) { assert.NoError(t, err) require.Len(t, rebaseOntoCalls, 2) - // b2: stale ontoOldBase → falls back to merge-base(main, b2) - assert.Equal(t, rebaseCall{"main", "main-b2-mergebase", "b2"}, rebaseOntoCalls[0], - "b2 should use merge-base as oldBase when ontoOldBase is stale") + // b2: stale ontoOldBase → uses fork-point(main, b2) + assert.Equal(t, rebaseCall{"main", "main-b2-forkpoint", "b2"}, rebaseOntoCalls[0], + "b2 should use the reflog fork-point when ontoOldBase is stale") // b3: b2's SHA is a valid ancestor → uses it directly assert.Equal(t, rebaseCall{"b2", "b2-on-main-sha", "b3"}, rebaseOntoCalls[1], diff --git a/cmd/utils.go b/cmd/utils.go index 514bf08..6da90fa 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -791,7 +791,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 && canUpdateBase(base, p.branch, s.Branches[p.index].Base) { s.Branches[p.index].Base = base } if head, ok := shaMap[p.branch]; ok { @@ -800,6 +800,18 @@ func updateBaseSHAs(s *stack.Stack) { } } +// canUpdateBase reports whether parentSHA can replace a branch's recorded base. +// Once a base is known, only a parent tip the branch actually contains may +// replace it; otherwise an amended parent would corrupt the next rebase +// boundary. Empty bases retain the historical best-effort behavior. +func canUpdateBase(parentSHA, branch, currentBase string) bool { + if currentBase == "" || currentBase == parentSHA { + return true + } + isAncestor, err := git.IsAncestor(parentSHA, branch) + return err == nil && isAncestor +} + // activeBranchNames returns the branch names for all non-merged branches in a stack. func activeBranchNames(s *stack.Stack) []string { active := s.ActiveBranches() @@ -1051,6 +1063,35 @@ func (o cascadeRebaseOpts) trunkRef() string { return o.Stack.Trunk.Branch } +// resolveRebaseOldBase returns a boundary the branch actually contains. +// An ordinary merge-base is intentionally not a fallback: after a parent is +// amended it falls below the old parent commit and would replay that commit +// into the child, which is the corruption this helper prevents. +func resolveRebaseOldBase(currentParentTip, recordedBase, newBase, branch string) (string, error) { + isValid := func(candidate string) bool { + if candidate == "" { + return false + } + ok, err := git.IsAncestor(candidate, branch) + return err == nil && ok + } + + if isValid(currentParentTip) { + return currentParentTip, nil + } + if recordedBase != currentParentTip && isValid(recordedBase) { + return recordedBase, nil + } + if forkPoint, err := git.MergeBaseForkPoint(newBase, branch); err == nil && isValid(forkPoint) { + return forkPoint, nil + } + + return "", fmt.Errorf( + "could not determine the previous base of %s after %s changed; rebase this branch manually", + branch, newBase, + ) +} + // cascadeRebaseResult describes the outcome of a cascade rebase. type cascadeRebaseResult struct { Rebased bool // at least one branch was successfully rebased @@ -1119,14 +1160,11 @@ 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, err := resolveRebaseOldBase(ontoOldBase, br.Base, newBase, br.Branch) + if err != nil { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: err, } } @@ -1159,7 +1197,14 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } else { var rebaseErr error if absIdx > 0 { - rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch, rebaseOpts) + oldBase, err := resolveRebaseOldBase(originalRefs[base], br.Base, base, br.Branch) + if err != nil { + return cascadeRebaseResult{ + Rebased: result.Rebased, + Err: err, + } + } + rebaseErr = git.RebaseOnto(base, oldBase, br.Branch, rebaseOpts) } else { if err := git.CheckoutBranch(br.Branch); err != nil { return cascadeRebaseResult{ diff --git a/cmd/utils_test.go b/cmd/utils_test.go index d95e359..464e619 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -851,3 +851,37 @@ 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") } + +func TestUpdateBaseSHAsPreservesLastValidBase(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "parent", Base: "main-tip"}, + {Branch: "child", Base: "old-parent"}, + }, + } + + restore := git.SetOps(&git.MockOps{ + RevParseFn: func(ref string) (string, error) { + switch ref { + case "main": + return "main-tip", nil + case "parent": + return "amended-parent", nil + case "child": + return "child-tip", nil + default: + return "", errors.New("unknown ref") + } + }, + IsAncestorFn: func(ancestor, branch string) (bool, error) { + return !(ancestor == "amended-parent" && branch == "child"), nil + }, + }) + defer restore() + + updateBaseSHAs(s) + + assert.Equal(t, "old-parent", s.Branches[1].Base) + assert.Equal(t, "child-tip", s.Branches[1].Head) +} diff --git a/internal/git/git.go b/internal/git/git.go index b8c43e4..180083f 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -338,6 +338,12 @@ func MergeBase(a, b string) (string, error) { return ops.MergeBase(a, b) } +// MergeBaseForkPoint returns where branch forked from ref, using ref's reflog +// to see through rewrites such as amend, rebase, or force-push. +func MergeBaseForkPoint(ref, branch string) (string, error) { + return ops.MergeBaseForkPoint(ref, branch) +} + // Log returns recent commits for the given branch. func Log(ref string, maxCount int) ([]CommitInfo, error) { return ops.Log(ref, maxCount) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index 6a49580..2464f4e 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -56,6 +56,7 @@ type Ops interface { RevParse(ref string) (string, error) RevParseMulti(refs []string) ([]string, error) MergeBase(a, b string) (string, error) + MergeBaseForkPoint(ref, branch string) (string, error) Log(ref string, maxCount int) ([]CommitInfo, error) LogRange(base, head string) ([]CommitInfo, error) DiffStatRange(base, head string) (additions, deletions int, err error) @@ -438,6 +439,10 @@ func (d *defaultOps) MergeBase(a, b string) (string, error) { return run("merge-base", a, b) } +func (d *defaultOps) MergeBaseForkPoint(ref, branch string) (string, error) { + return run("merge-base", "--fork-point", ref, branch) +} + func (d *defaultOps) Log(ref string, maxCount int) ([]CommitInfo, error) { format := "%H\t%s\t%at" output, err := run("log", ref, "--format="+format, "-n", strconv.Itoa(maxCount)) diff --git a/internal/git/mock_ops.go b/internal/git/mock_ops.go index 5bc5276..25396f5 100644 --- a/internal/git/mock_ops.go +++ b/internal/git/mock_ops.go @@ -36,6 +36,7 @@ type MockOps struct { RevParseFn func(string) (string, error) RevParseMultiFn func([]string) ([]string, error) MergeBaseFn func(string, string) (string, error) + MergeBaseForkPointFn func(string, string) (string, error) LogFn func(string, int) ([]CommitInfo, error) LogRangeFn func(string, string) ([]CommitInfo, error) DiffStatRangeFn func(string, string) (int, int, error) @@ -285,6 +286,13 @@ func (m *MockOps) MergeBase(a, b string) (string, error) { return "", nil } +func (m *MockOps) MergeBaseForkPoint(ref, branch string) (string, error) { + if m.MergeBaseForkPointFn != nil { + return m.MergeBaseForkPointFn(ref, branch) + } + return "", nil +} + func (m *MockOps) Log(ref string, maxCount int) ([]CommitInfo, error) { if m.LogFn != nil { return m.LogFn(ref, maxCount)