mirror of
https://github.com/github/gh-stack.git
synced 2026-09-14 20:26:28 +08:00
Fix for replaying amended parent commits during rebase (#333)
* 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. * 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 --------- Copilot-Session: 30bcbf2a-5ef1-4bdb-a0fc-618294ae8ded
This commit is contained in:
+14
-3
@@ -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})
|
||||
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
|
||||
+306
-11
@@ -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"))
|
||||
}
|
||||
|
||||
+10
-10
@@ -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],
|
||||
|
||||
+55
-10
@@ -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{
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user