From 6dca4c9cf56dc2315bc9bdb57e59371fbbc287d1 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Tue, 28 Jul 2026 02:11:35 -0400 Subject: [PATCH 1/2] Avoid replaying amended parent commits Preserve a branch's last valid base when its parent is rewritten, and only use verified ancestor commits as rebase boundaries. Recover previously corrupted metadata from the parent reflog when possible, otherwise stop safely instead of replaying superseded parent commits. --- cmd/rebase_test.go | 260 +++++++++++++++++++++++++++++++++++++-- cmd/sync_test.go | 20 +-- cmd/utils.go | 65 ++++++++-- cmd/utils_test.go | 34 +++++ internal/git/git.go | 6 + internal/git/gitops.go | 5 + internal/git/mock_ops.go | 8 ++ 7 files changed, 367 insertions(+), 31 deletions(-) diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index c8adc0a..c234e41 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,239 @@ 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")) +} 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) From c7144b4e193acd96b54d87a764bce50d9960b897 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Tue, 28 Jul 2026 18:04:38 -0400 Subject: [PATCH 2/2] Record adopted branch merge bases Store the actual common ancestor when adding an existing branch so cascade rebases replay only that branch's unique commits while retaining the amended-parent safety guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 30bcbf2a-5ef1-4bdb-a0fc-618294ae8ded --- cmd/add.go | 17 +++++++++++--- cmd/add_test.go | 41 +++++++++++++++++++++++++++++++++ cmd/rebase_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) 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 c234e41..1be2f9f 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -2201,3 +2201,60 @@ func TestIntegration_AmendedParentWithoutForkPointFailsSafely(t *testing.T) { 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")) +}