diff --git a/cmd/checkout.go b/cmd/checkout.go index 4fe24f2c..3c394d15 100644 --- a/cmd/checkout.go +++ b/cmd/checkout.go @@ -685,13 +685,8 @@ func importRemoteStack( branchRefs := make([]stack.BranchRef, len(prs)) for i, pr := range prs { branchRefs[i] = stack.BranchRef{ - Branch: pr.HeadRefName, - PullRequest: &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - Merged: pr.Merged, - }, + Branch: pr.HeadRefName, + PullRequest: pullRequestRef(pr, nil), } } @@ -727,12 +722,7 @@ func syncRemotePRState(s *stack.Stack, prs []*github.PullRequest) { if !ok { continue } - s.Branches[i].PullRequest = &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - Merged: pr.Merged, - } + s.Branches[i].PullRequest = pullRequestRef(pr, s.Branches[i].PullRequest) s.Branches[i].Queued = pr.IsQueued() } } diff --git a/cmd/checkout_test.go b/cmd/checkout_test.go index f6623838..51ba2bf0 100644 --- a/cmd/checkout_test.go +++ b/cmd/checkout_test.go @@ -1151,7 +1151,7 @@ func TestCheckout_NumericTarget_MergedBranchDeletedFromRemote(t *testing.T) { }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ - 10: {ID: "PR_10", Number: 10, HeadRefName: "feat-1", BaseRefName: "main", Merged: true, State: "MERGED", URL: "https://github.com/o/r/pull/10"}, + 10: {ID: "PR_10", Number: 10, HeadRefName: "feat-1", HeadRefOID: "merged-head", BaseRefName: "main", Merged: true, State: "MERGED", URL: "https://github.com/o/r/pull/10"}, 11: {ID: "PR_11", Number: 11, HeadRefName: "feat-2", BaseRefName: "feat-1", State: "OPEN", URL: "https://github.com/o/r/pull/11"}, } return prs[number], nil @@ -1165,6 +1165,24 @@ func TestCheckout_NumericTarget_MergedBranchDeletedFromRemote(t *testing.T) { assert.Equal(t, "feat-2", checkedOut) assert.Contains(t, output, "Skipping merged branch feat-1") assert.Contains(t, output, "Imported stack with 2 branches") + sf, err := stack.Load(gitDir) + require.NoError(t, err) + assert.Equal(t, "merged-head", sf.Stacks[0].Branches[0].PullRequest.MergedHead) + assert.Empty(t, sf.Stacks[0].Branches[0].Head) +} + +func TestSyncRemotePRState_MergedHead(t *testing.T) { + s := &stack.Stack{Branches: []stack.BranchRef{{ + Branch: "parent", Head: "local", + PullRequest: &stack.PullRequestRef{Number: 10, Merged: true, MergedHead: "cached"}, + }}} + syncRemotePRState(s, []*github.PullRequest{{Number: 10, HeadRefName: "parent", Merged: true}}) + assert.Equal(t, "cached", s.Branches[0].PullRequest.MergedHead) + assert.Equal(t, "local", s.Branches[0].Head) + + syncRemotePRState(s, []*github.PullRequest{{Number: 11, HeadRefName: "parent", State: "OPEN"}}) + assert.Empty(t, s.Branches[0].PullRequest.MergedHead) + assert.False(t, s.Branches[0].IsMerged()) } func TestCheckout_NumericTarget_AllPRsMerged(t *testing.T) { diff --git a/cmd/rebase.go b/cmd/rebase.go index d7433483..a567c119 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -250,10 +250,8 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { if startIdx > 0 { prev := s.Branches[startIdx-1] if prev.IsMerged() { - if sha, ok := originalRefs[prev.Branch]; ok { - needsOnto = true - ontoOldBase = sha - } + needsOnto = true + ontoOldBase = originalRefs[prev.Branch] } } @@ -456,7 +454,8 @@ func continueRebase(cfg *config.Config, gitDir string) error { NeedsOnto: state.UseOnto, OntoOldBase: state.OntoOldBase, CommitterDateIsAuthorDate: state.CommitterDateIsAuthorDate, - TrunkRef: trunkBase, + TrunkRef: trunkRef, + TrunkSHA: state.TrunkSHA, }) if result.Err != nil { diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 8feda3cc..e00c4a49 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -2216,7 +2216,7 @@ func TestResolveRebaseOldBase(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") + return "", nil }, }) defer restore() @@ -2234,6 +2234,442 @@ type amendedParentRepo struct { newParent string } +func TestResolveRebaseOldBase_FallbackOrdering(t *testing.T) { + for _, tt := range []struct { + name string + ancestors map[string]bool + want string + wantErr string + }{ + {"newer fork point", map[string]bool{"recorded:child": true, "fork:child": true, "recorded:fork": true}, "fork", ""}, + {"older fork point", map[string]bool{"recorded:child": true, "fork:child": true, "fork:recorded": true}, "recorded", ""}, + {"invalid fork point", map[string]bool{"recorded:child": true}, "recorded", ""}, + {"invalid recorded base", map[string]bool{"fork:child": true}, "fork", ""}, + {"incomparable candidates", map[string]bool{"recorded:child": true, "fork:child": true}, "", "conflicting previous bases"}, + {"no candidates", nil, "", "rebase this branch manually"}, + } { + t.Run(tt.name, func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, b string) (bool, error) { return tt.ancestors[a+":"+b], nil }, + MergeBaseForkPointFn: func(string, string) (string, error) { return "fork", nil }, + }) + defer restore() + got, err := resolveRebaseOldBase("rewritten", "recorded", "parent", "child") + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestResolveRebaseOldBase_LookupErrors(t *testing.T) { + lookupErr := errors.New("Git lookup failed") + for _, failAt := range []string{"ancestor", "fork", "comparison"} { + t.Run(failAt, func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, b string) (bool, error) { + if failAt == "ancestor" || (failAt == "comparison" && b == "fork") { + return false, lookupErr + } + return a != "rewritten", nil + }, + MergeBaseForkPointFn: func(string, string) (string, error) { + if failAt == "fork" { + return "", lookupErr + } + return "fork", nil + }, + }) + defer restore() + _, err := resolveRebaseOldBase("rewritten", "recorded", "parent", "child") + require.ErrorIs(t, err, lookupErr) + }) + } + t.Run("unavailable optional head permits local fallback", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, b string) (bool, error) { + if a == "missing" { + return false, lookupErr + } + return a == "local" && b == "child", nil + }, + CommitExistsFn: func(ref string) (bool, error) { return ref != "missing", nil }, + }) + defer restore() + got, err := resolveMergedRebaseOldBase("missing", "local", "recorded", "main", "child") + require.NoError(t, err) + assert.Equal(t, "local", got) + }) +} + +func TestRebase_MergedHeadSnapshot(t *testing.T) { + dir := t.TempDir() + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "parent", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true, MergedHead: "saved-head"}}, + {Branch: "child"}, + }, + } + state := newWorktreeRebaseState(s, nil, "child", + map[string]string{"parent": "local-parent", "child": "local-child"}, trunkTarget{}, 0, 2) + syncRemotePRState(s, []*github.PullRequest{{ + Number: 10, HeadRefName: "parent", Merged: true, HeadRefOID: "later-api-head", + }}) + mock := newRebaseMock(dir, "child") + var oldBase string + mock.RebaseOntoFn = func(_, old, _ string, _ git.RebaseOpts) error { + oldBase = old + return nil + } + restore := git.SetOps(mock) + defer restore() + cfg := issue250TestConfig(t) + result := cascadeRebase(savedCascadeOpts(cfg, dir, state, s)) + require.NoError(t, result.Err) + assert.False(t, result.Conflicted) + assert.Equal(t, "saved-head", oldBase) + assert.Equal(t, "local-parent", state.OriginalRefs["parent"]) +} + +type issue512Repo struct { + dir string + writer string + gitDir string + main string + parent string + child string + mergedHead string + stack stack.Stack +} + +func setupIssue512Repo(t *testing.T) issue512Repo { + t.Helper() + root := t.TempDir() + remote := filepath.Join(root, "remote.git") + dir := filepath.Join(root, "clone") + writer := filepath.Join(root, "writer") + issue250Git(t, ".", "-c", "safe.bareRepository=all", "init", "--bare", "-b", "main", remote) + issue250Git(t, ".", "clone", remote, dir) + issue250Git(t, dir, "config", "user.name", "Test") + issue250Git(t, dir, "config", "user.email", "test@example.com") + issue250Git(t, dir, "config", "commit.gpgSign", "false") + issue250Git(t, dir, "config", "core.hooksPath", os.DevNull) + issue250Git(t, dir, "config", "rerere.enabled", "true") + issue250WriteFile(t, dir, "base.txt", "base\n") + issue250Git(t, dir, "add", ".") + issue250Git(t, dir, "commit", "-m", "base") + issue250Git(t, dir, "push", "-u", "origin", "main") + main := issue250Git(t, dir, "rev-parse", "main") + issue250Git(t, dir, "checkout", "-b", "parent") + for _, content := range []string{"one\n", "two\n"} { + issue250WriteFile(t, dir, "shared.txt", content) + issue250Git(t, dir, "add", ".") + issue250Git(t, dir, "commit", "-m", "parent "+strings.TrimSpace(content)) + } + parent := issue250Git(t, dir, "rev-parse", "parent") + issue250Git(t, dir, "push", "-u", "origin", "parent") + issue250Git(t, dir, "checkout", "-b", "child") + issue250WriteFile(t, dir, "child.txt", "child\n") + issue250Git(t, dir, "add", ".") + issue250Git(t, dir, "commit", "-m", "child only") + child := issue250Git(t, dir, "rev-parse", "child") + issue250Git(t, dir, "push", "-u", "origin", "child") + issue250Git(t, ".", "clone", remote, writer) + issue250Git(t, writer, "config", "user.name", "Test") + issue250Git(t, writer, "config", "user.email", "test@example.com") + issue250Git(t, writer, "config", "commit.gpgSign", "false") + issue250Git(t, writer, "config", "core.hooksPath", os.DevNull) + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main", Head: main}, + Branches: []stack.BranchRef{ + {Branch: "parent", Head: parent, Base: main, PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "child", Head: child, Base: parent}, + }, + } + gitDir := filepath.Join(dir, ".git") + writeStackFile(t, gitDir, s) + return issue512Repo{dir, writer, gitDir, main, parent, child, parent, s} +} + +func issue512Config(t *testing.T, mergedHead string) *config.Config { + t.Helper() + cfg := issue250TestConfig(t) + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(number int) (*github.PullRequest, error) { + if number != 10 { + return nil, nil + } + return &github.PullRequest{ + Number: 10, HeadRefName: "parent", BaseRefName: "main", + State: "MERGED", Merged: true, HeadRefOID: mergedHead, + }, nil + }, + } + return cfg +} + +func issue512Squash(t *testing.T, repo issue512Repo) string { + t.Helper() + issue250Git(t, repo.writer, "checkout", "main") + issue250Git(t, repo.writer, "merge", "--squash", "origin/parent") + issue250Git(t, repo.writer, "commit", "-m", "squash parent") + issue250Git(t, repo.writer, "push", "origin", "main") + return issue250Git(t, repo.writer, "rev-parse", "main") +} + +func issue512Advance(t *testing.T, repo issue512Repo) string { + t.Helper() + issue250WriteFile(t, repo.writer, "shared.txt", "three\n") + issue250Git(t, repo.writer, "add", ".") + issue250Git(t, repo.writer, "commit", "-m", "advance trunk") + issue250Git(t, repo.writer, "push", "origin", "main") + return issue250Git(t, repo.writer, "rev-parse", "main") +} + +func issue512RewriteParent(t *testing.T, repo *issue512Repo, rewrite, updateChild bool) { + t.Helper() + issue250Git(t, repo.writer, "checkout", "-b", "parent", "origin/parent") + issue250WriteFile(t, repo.writer, "shared.txt", "updated parent\n") + issue250Git(t, repo.writer, "add", ".") + args := []string{"commit", "-m", "updated parent"} + if rewrite { + args = append(args, "--amend") + } + issue250Git(t, repo.writer, args...) + repo.mergedHead = issue250Git(t, repo.writer, "rev-parse", "parent") + issue250Git(t, repo.writer, "push", "--force-with-lease", "origin", "parent") + if updateChild { + issue250Git(t, repo.writer, "checkout", "-b", "child", "origin/child") + issue250Git(t, repo.writer, "rebase", "--onto", "parent", repo.parent, "child") + issue250Git(t, repo.writer, "push", "--force-with-lease", "origin", "child") + } + issue512Squash(t, *repo) + issue250Git(t, repo.dir, "checkout", "main") + issue250Git(t, repo.dir, "fetch", "origin") + if updateChild { + remoteChild := issue250Git(t, repo.writer, "rev-parse", "child") + issue250Git(t, repo.dir, "update-ref", "refs/heads/child", remoteChild, repo.child) + } + issue250Git(t, repo.dir, "checkout", "child") +} + +func assertIssue512History(t *testing.T, repo issue512Repo) { + t.Helper() + assert.Equal(t, "child only", issue250Git(t, repo.dir, "log", "--format=%s", "origin/main..child")) + assert.Equal(t, "child", strings.TrimSpace(issue250Git(t, repo.dir, "show", "child:child.txt"))) + assert.Equal(t, issue250Git(t, repo.dir, "show", "origin/main:shared.txt"), issue250Git(t, repo.dir, "show", "child:shared.txt")) + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + trunk := issue250Git(t, repo.dir, "rev-parse", "origin/main") + assert.Equal(t, trunk, sf.Stacks[0].Trunk.Head) + assert.Equal(t, trunk, sf.Stacks[0].Branches[1].Base) + assert.NoFileExists(t, filepath.Join(repo.gitDir, rebaseStateFile)) +} + +func testIssue512StaleMergedParent(t *testing.T, run func(*config.Config) error) { + t.Helper() + for _, tt := range []struct { + name string + rewrite bool + updateChild bool + pruned bool + cached bool + advanced bool + }{ + {name: "rewritten parent", rewrite: true, updateChild: true}, + {name: "fast-forwarded parent", updateChild: true}, + {name: "old child history", rewrite: true}, + {name: "pruned parent", rewrite: true, updateChild: true, pruned: true}, + {name: "cached merge head", rewrite: true, updateChild: true, cached: true}, + {name: "post-merge local parent", rewrite: true, updateChild: true, advanced: true}, + } { + t.Run(tt.name, func(t *testing.T) { + repo := setupIssue512Repo(t) + issue512RewriteParent(t, &repo, tt.rewrite, tt.updateChild) + if tt.cached { + repo.stack.Branches[0].PullRequest.MergedHead = repo.mergedHead + writeStackFile(t, repo.gitDir, repo.stack) + } + if tt.pruned { + issue250Git(t, repo.dir, "branch", "-D", "parent") + } + parentBefore := repo.parent + if tt.advanced { + parentBefore = issue250Git(t, repo.dir, "rev-parse", "child") + issue250Git(t, repo.dir, "update-ref", "refs/heads/parent", parentBefore, repo.parent) + } + withIssue250Repo(t, repo.dir) + cfg := issue512Config(t, repo.mergedHead) + require.NoError(t, run(cfg)) + assertIssue512History(t, repo) + if tt.pruned { + require.Error(t, issue250GitMayFail(t, repo.dir, "show-ref", "--verify", "refs/heads/parent")) + } else { + assert.Equal(t, parentBefore, issue250Git(t, repo.dir, "rev-parse", "parent")) + } + }) + } +} + +func testIssue512StaleRecordedBase(t *testing.T, run func(*config.Config) error) { + t.Helper() + repo := setupIssue512Repo(t) + issue512Squash(t, repo) + issue250Git(t, repo.dir, "fetch", "origin") + issue250Git(t, repo.dir, "checkout", "main") + issue250Git(t, repo.dir, "merge", "--ff-only", "origin/main") + issue250Git(t, repo.dir, "checkout", "child") + issue250Git(t, repo.dir, "rebase", "--onto", "main", repo.parent, "child") + repo.stack.Branches[1].Base = repo.main + writeStackFile(t, repo.gitDir, repo.stack) + issue512Advance(t, repo) + withIssue250Repo(t, repo.dir) + require.NoError(t, run(issue512Config(t, repo.mergedHead))) + assertIssue512History(t, repo) +} + +func TestRebase_Issue512StaleMergedParent(t *testing.T) { + testIssue512StaleMergedParent(t, func(cfg *config.Config) error { + return runRebase(cfg, &rebaseOptions{remote: "origin"}) + }) +} + +func TestRebase_Issue512StaleRecordedBase(t *testing.T) { + testIssue512StaleRecordedBase(t, func(cfg *config.Config) error { + return runRebase(cfg, &rebaseOptions{remote: "origin"}) + }) +} + +func testIssue512RepeatedRegularMerge(t *testing.T, run func(*config.Config) error) { + t.Helper() + repo := setupIssue512Repo(t) + issue250WriteFile(t, repo.writer, "trunk-only.txt", "before merge\n") + issue250Git(t, repo.writer, "add", ".") + issue250Git(t, repo.writer, "commit", "-m", "trunk before merge") + issue250Git(t, repo.writer, "merge", "--no-ff", "origin/parent", "-m", "merge parent") + issue250Git(t, repo.writer, "push", "origin", "main") + withIssue250Repo(t, repo.dir) + cfg := issue512Config(t, repo.mergedHead) + require.NoError(t, run(cfg)) + assertIssue512History(t, repo) + + issue250Git(t, repo.dir, "branch", "-D", "parent") + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + sf.Stacks[0].Branches[0].Head = "" + require.NoError(t, stack.Save(repo.gitDir, sf)) + issue250WriteFile(t, repo.writer, "trunk-only.txt", "after merge\n") + issue250Git(t, repo.writer, "add", ".") + issue250Git(t, repo.writer, "commit", "-m", "trunk after merge") + issue250Git(t, repo.writer, "push", "origin", "main") + + require.NoError(t, run(cfg)) + assertIssue512History(t, repo) + assert.Equal(t, "after merge", issue250Git(t, repo.dir, "show", "child:trunk-only.txt")) +} + +func TestRebase_Issue512RepeatedRegularMerge(t *testing.T) { + testIssue512RepeatedRegularMerge(t, func(cfg *config.Config) error { + return runRebase(cfg, &rebaseOptions{remote: "origin"}) + }) +} + +func TestRebase_MergedHeadPreservesUnmergedRecordedBase(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, b string) (bool, error) { + return !(a == "unmerged-local" && b == "main"), nil + }, + }) + defer restore() + base, err := resolveMergedRebaseOldBase("merged", "unmerged-local", "unmerged-local", "main", "child") + require.NoError(t, err) + assert.Equal(t, "merged", base, "a recorded base not in the destination must not discard unmerged work") +} + +func TestRebase_Issue512SelectedRangeWithPrunedParent(t *testing.T) { + for _, mode := range []string{"upstack", "downstack", "no-trunk"} { + t.Run(mode, func(t *testing.T) { + repo := setupIssue512Repo(t) + issue512RewriteParent(t, &repo, true, true) + issue250Git(t, repo.dir, "checkout", "main") + issue250Git(t, repo.dir, "merge", "--ff-only", "origin/main") + issue250Git(t, repo.dir, "checkout", "child") + issue250Git(t, repo.dir, "branch", "-D", "parent") + repo.stack.Trunk.Head = issue250Git(t, repo.dir, "rev-parse", "main") + repo.stack.Branches[0].Head = "" + repo.stack.Branches[0].PullRequest.MergedHead = repo.mergedHead + writeStackFile(t, repo.gitDir, repo.stack) + withIssue250Repo(t, repo.dir) + require.NoError(t, runRebase(issue512Config(t, repo.mergedHead), &rebaseOptions{ + remote: "origin", upstack: mode == "upstack", downstack: mode == "downstack", noTrunk: mode == "no-trunk", + })) + assertIssue512History(t, repo) + require.Error(t, issue250GitMayFail(t, repo.dir, "show-ref", "--verify", "refs/heads/parent")) + }) + } +} + +func setupIssue512Conflict(t *testing.T) (issue512Repo, string, string) { + t.Helper() + repo := setupIssue512Repo(t) + issue512RewriteParent(t, &repo, true, true) + issue250WriteFile(t, repo.writer, "child.txt", "trunk conflict\n") + issue250Git(t, repo.writer, "add", "child.txt") + issue250Git(t, repo.writer, "commit", "-m", "trunk conflict") + issue250Git(t, repo.writer, "push", "origin", "main") + issue250Git(t, repo.dir, "checkout", "main") + parentDir := filepath.Join(t.TempDir(), "parent-owner") + childDir := filepath.Join(t.TempDir(), "child-owner") + issue250Git(t, repo.dir, "worktree", "add", parentDir, "parent") + issue250Git(t, repo.dir, "worktree", "add", childDir, "child") + return repo, parentDir, childDir +} + +func TestRebase_Issue512WorktreeRecovery(t *testing.T) { + for _, action := range []string{"continue", "abort"} { + t.Run(action, func(t *testing.T) { + repo, parentDir, childDir := setupIssue512Conflict(t) + childBefore := issue250Git(t, repo.dir, "rev-parse", "child") + withIssue250Repo(t, repo.dir) + cfg := issue512Config(t, repo.mergedHead) + require.ErrorIs(t, runRebase(cfg, &rebaseOptions{remote: "origin"}), ErrConflict) + + state, err := loadRebaseState(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, repo.mergedHead, state.RebaseOldBase) + assert.Equal(t, repo.mergedHead, state.OriginalStack.Branches[0].PullRequest.MergedHead) + assert.Equal(t, repo.parent, state.OriginalRefs["parent"]) + assert.Equal(t, childBefore, state.OriginalRefs["child"]) + require.NotEqual(t, repo.mergedHead, state.OriginalRefs["parent"]) + assert.True(t, worktree.SamePath(childDir, state.Worktrees.Location("child").Path)) + + if action == "continue" { + issue250WriteFile(t, childDir, "child.txt", "resolved child\n") + issue250Git(t, childDir, "add", "child.txt") + } + withIssue250Repo(t, parentDir) + cfg = issue512Config(t, childBefore) + require.NoError(t, runRebase(cfg, &rebaseOptions{cont: action == "continue", abort: action == "abort"})) + + assert.Equal(t, repo.parent, issue250Git(t, repo.dir, "rev-parse", "parent")) + assert.Equal(t, "main", issue250Git(t, repo.dir, "branch", "--show-current")) + assert.Equal(t, "parent", issue250Git(t, parentDir, "branch", "--show-current")) + assert.Equal(t, "child", issue250Git(t, childDir, "branch", "--show-current")) + assert.NoFileExists(t, filepath.Join(repo.gitDir, rebaseStateFile)) + if action == "abort" { + assert.Equal(t, childBefore, issue250Git(t, repo.dir, "rev-parse", "child")) + } else { + assert.Equal(t, "child only", issue250Git(t, repo.dir, "log", "--format=%s", "origin/main..child")) + assert.Equal(t, "resolved child", issue250Git(t, repo.dir, "show", "child:child.txt")) + } + }) + } +} + func issue250Git(t *testing.T, dir string, args ...string) string { t.Helper() cmd := exec.Command("git", args...) @@ -3068,6 +3504,44 @@ func TestRebase_LegacyContinuePersistsCatalogUnderOperationLock(t *testing.T) { assert.ErrorIs(t, err, os.ErrNotExist) } +func TestRebase_LegacyContinueKeepsNamedTrunkForForkPoint(t *testing.T) { + dir := t.TempDir() + writeStackFile(t, dir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "parent", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "child", Base: "recorded"}, + }, + }) + require.NoError(t, saveRebaseState(dir, &rebaseState{ + OriginalBranch: "child", ConflictBranch: "parent", RemainingBranches: []string{"child"}, + OriginalRefs: map[string]string{"parent": "stale", "child": "before"}, + UseOnto: true, OntoOldBase: "stale", TrunkRef: "main", TrunkSHA: "pinned", + })) + mock := newRebaseMock(dir, "child") + mock.IsAncestorFn = func(a, b string) (bool, error) { return a != "stale", nil } + mock.MergeBaseForkPointFn = func(ref, branch string) (string, error) { + assert.Equal(t, "main", ref) + if ref != "main" { + return "", fmt.Errorf("No such ref: %s", ref) + } + return "", nil + } + rebased := false + mock.RebaseOntoFn = func(target, oldBase, branch string, _ git.RebaseOpts) error { + rebased = true + assert.Equal(t, "pinned", target) + assert.Equal(t, "recorded", oldBase) + assert.Equal(t, "child", branch) + return nil + } + restore := git.SetOps(mock) + defer restore() + require.NoError(t, runRebase(issue250TestConfig(t), &rebaseOptions{cont: true})) + assert.True(t, rebased) + assert.NoFileExists(t, filepath.Join(dir, rebaseStateFile)) +} + func TestRebase_LegacyPhaseGuards(t *testing.T) { for _, tc := range []struct { phase string diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 1060fdd0..f8d7dbf2 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -2669,6 +2669,72 @@ func TestSync_WorktreesPublishesFromLinkedWorktree(t *testing.T) { require.NoError(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", "parent", "child")) } +func TestSync_Issue512StaleMergedParent(t *testing.T) { + testIssue512StaleMergedParent(t, func(cfg *config.Config) error { + return runSync(cfg, &syncOptions{remote: "origin"}) + }) +} + +func TestSync_Issue512StaleRecordedBase(t *testing.T) { + testIssue512StaleRecordedBase(t, func(cfg *config.Config) error { + return runSync(cfg, &syncOptions{remote: "origin"}) + }) +} + +func TestSync_Issue512RepeatedRegularMerge(t *testing.T) { + testIssue512RepeatedRegularMerge(t, func(cfg *config.Config) error { + return runSync(cfg, &syncOptions{remote: "origin"}) + }) +} + +func TestSync_Issue512WorktreePruneRepeatedRebase(t *testing.T) { + repo := setupIssue512Repo(t) + squash := issue512Squash(t, repo) + issue250Git(t, repo.dir, "checkout", "main") + issue250WriteFile(t, repo.dir, "base.txt", "dirty trunk worktree\n") + childDir := filepath.Join(t.TempDir(), "child-worktree") + issue250Git(t, repo.dir, "worktree", "add", childDir, "child") + withIssue250Repo(t, childDir) + cfg := issue512Config(t, repo.mergedHead) + + require.NoError(t, runSync(cfg, &syncOptions{remote: "origin", prune: true})) + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, squash, sf.Stacks[0].Trunk.Head) + assert.Equal(t, squash, sf.Stacks[0].Branches[1].Base) + assert.Equal(t, repo.main, issue250Git(t, repo.dir, "rev-parse", "main")) + require.Error(t, issue250GitMayFail(t, repo.dir, "show-ref", "--verify", "refs/heads/parent")) + + newTrunk := issue512Advance(t, repo) + require.NoError(t, runRebase(cfg, &rebaseOptions{remote: "origin"})) + sf, err = stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, newTrunk, sf.Stacks[0].Trunk.Head) + assert.Equal(t, newTrunk, sf.Stacks[0].Branches[1].Base) + assert.Equal(t, repo.main, issue250Git(t, repo.dir, "rev-parse", "main")) + assertIssue512History(t, repo) + assert.Equal(t, "main", issue250Git(t, repo.dir, "branch", "--show-current")) + assert.Equal(t, "child", issue250Git(t, childDir, "branch", "--show-current")) + content, err := os.ReadFile(filepath.Join(repo.dir, "base.txt")) + require.NoError(t, err) + assert.Equal(t, "dirty trunk worktree\n", string(content)) +} + +func TestSync_Issue512ConflictRestoresLocalSnapshots(t *testing.T) { + repo, parentDir, childDir := setupIssue512Conflict(t) + childBefore := issue250Git(t, repo.dir, "rev-parse", "child") + remoteBefore := issue250Git(t, repo.dir, "rev-parse", "origin/child") + withIssue250Repo(t, childDir) + cfg := issue512Config(t, repo.mergedHead) + require.ErrorIs(t, runSync(cfg, &syncOptions{remote: "origin"}), ErrConflict) + assert.Equal(t, repo.parent, issue250Git(t, repo.dir, "rev-parse", "parent")) + assert.Equal(t, childBefore, issue250Git(t, repo.dir, "rev-parse", "child")) + assert.Equal(t, remoteBefore, issue250Git(t, repo.dir, "rev-parse", "origin/child")) + assert.Equal(t, "parent", issue250Git(t, parentDir, "branch", "--show-current")) + assert.Equal(t, "child", issue250Git(t, childDir, "branch", "--show-current")) + assert.NoFileExists(t, filepath.Join(repo.gitDir, rebaseStateFile)) +} + func TestSync_SharedTrunkSelectionRestoresOrigin(t *testing.T) { repo := setupSharedTrunkRebaseRepo(t, false) observerHead := issue250Git(t, repo.childDir, "rev-parse", "HEAD") diff --git a/cmd/utils.go b/cmd/utils.go index c1a8f1a8..cec37ecb 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -497,7 +497,7 @@ func resolveStack(sf *stack.StackFile, branch string, cfg *config.Config) (*stac // 1. No tracked PR — look for an OPEN PR by head branch name. // 2. Tracked PR (not merged) — refresh status by number; if closed, // clear the association and fall through to path 1. -// 3. Tracked PR (merged) — skip; the merged state is final. +// 3. Tracked PR (merged) — only look up a missing merged head; the state is final. // // The transient Queued flag is also populated from the API response. // @@ -536,7 +536,7 @@ func syncStackPRs(cfg *config.Config, s *stack.Stack) map[string]*github.PRDetai for i := range s.Branches { b := s.Branches[i] - if b.IsMerged() { + if b.IsMerged() && (b.PullRequest.MergedHead != "" || b.PullRequest.Number == 0) { results[i] = branchResult{index: i, skip: true} // Provide PRDetails for merged branches from existing tracked PR if b.PullRequest != nil && b.PullRequest.Number != 0 { @@ -564,7 +564,7 @@ func syncStackPRs(cfg *config.Config, s *stack.Stack) map[string]*github.PRDetai if branch.PullRequest != nil && branch.PullRequest.Number != 0 { // Tracked PR — refresh its state. pr, err := client.FindPRByNumber(branch.PullRequest.Number) - if err != nil { + if err != nil || (branch.IsMerged() && (pr == nil || !pr.Merged)) { // API error — keep existing tracked PR res.skip = true res.details = prDetailsFromTracked(branch.PullRequest) @@ -573,12 +573,7 @@ func syncStackPRs(cfg *config.Config, s *stack.Stack) map[string]*github.PRDetai } if pr != nil && pr.State != "CLOSED" { // PR is open or merged — keep it - res.pullRequest = &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - Merged: pr.Merged, - } + res.pullRequest = pullRequestRef(pr, branch.PullRequest) res.queued = pr.IsQueued() res.details = prDetailsFromPR(pr) results[idx] = res @@ -597,11 +592,7 @@ func syncStackPRs(cfg *config.Config, s *stack.Stack) map[string]*github.PRDetai results[idx] = res return } - res.pullRequest = &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - } + res.pullRequest = pullRequestRef(pr, nil) res.queued = pr.IsQueued() // FindPRForBranch only returns OPEN PRs res.details = &github.PRDetails{ @@ -647,6 +638,32 @@ func syncStackPRs(cfg *config.Config, s *stack.Stack) map[string]*github.PRDetai // maxAPIConcurrency limits the number of concurrent API calls to avoid hitting secondary rate limits. const maxAPIConcurrency = 6 +func pullRequestRef(pr *github.PullRequest, previous *stack.PullRequestRef) *stack.PullRequestRef { + ref := &stack.PullRequestRef{ + Number: pr.Number, + ID: pr.ID, + URL: pr.URL, + Merged: pr.Merged, + } + if previous != nil && previous.Number == pr.Number && + (previous.ID == "" || pr.ID == "" || previous.ID == pr.ID) { + if ref.ID == "" { + ref.ID = previous.ID + } + if ref.URL == "" { + ref.URL = previous.URL + } + if previous.Merged { + ref.Merged = true + ref.MergedHead = previous.MergedHead + } + } + if pr.Merged && pr.HeadRefOID != "" { + ref.MergedHead = pr.HeadRefOID + } + return ref +} + // prDetailsFromPR builds PRDetails from a PullRequest returned by FindPRByNumber. func prDetailsFromPR(pr *github.PullRequest) *github.PRDetails { if pr == nil { @@ -738,6 +755,19 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string return nil, false } + remotePRCount := len(remotePRNumbers) + queried := make(map[int]bool, remotePRCount) + for _, number := range remotePRNumbers { + queried[number] = true + } + for _, b := range s.Branches { + if b.IsMerged() && b.PullRequest.MergedHead == "" && b.PullRequest.Number != 0 && + !queried[b.PullRequest.Number] { + remotePRNumbers = append(remotePRNumbers, b.PullRequest.Number) + queried[b.PullRequest.Number] = true + } + } + // Fetch each remote PR concurrently. Results are written to an ordered // slice (one slot per PR number) so that when we build the branch map // below, later entries win on duplicate HeadRefNames — matching the @@ -768,9 +798,13 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string // Build map sequentially to preserve order semantics. prByBranch := make(map[string]*github.PullRequest, len(remotePRNumbers)) - for _, pr := range prResults { + prByNumber := make(map[int]*github.PullRequest, len(remotePRNumbers)) + for i, pr := range prResults { if pr != nil { - prByBranch[pr.HeadRefName] = pr + if i < remotePRCount { + prByBranch[pr.HeadRefName] = pr + } + prByNumber[pr.Number] = pr } } @@ -779,15 +813,16 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string for i := range s.Branches { b := &s.Branches[i] pr, ok := prByBranch[b.Branch] + if !ok && b.IsMerged() { + pr, ok = prByNumber[b.PullRequest.Number] + } if !ok { continue } - b.PullRequest = &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - Merged: pr.Merged, + if b.IsMerged() && b.PullRequest.Number == pr.Number && !pr.Merged { + continue } + b.PullRequest = pullRequestRef(pr, b.PullRequest) b.Queued = pr.IsQueued() details[b.Branch] = prDetailsFromPR(pr) } @@ -1235,21 +1270,45 @@ func (o cascadeRebaseOpts) trunkRef() string { // 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 valid, err := rebaseBoundaryContains(currentParentTip, branch); err != nil { + return "", err + } else if valid { + return currentParentTip, nil } - if isValid(currentParentTip) { - return currentParentTip, nil + recordedValid, err := rebaseBoundaryContains(recordedBase, branch) + if err != nil { + return "", err + } + forkPoint, err := git.MergeBaseForkPoint(newBase, branch) + if err != nil { + return "", fmt.Errorf("finding the previous base of %s from %s: %w", branch, newBase, err) + } + forkValid, err := rebaseBoundaryContains(forkPoint, branch) + if err != nil { + return "", err + } + if recordedValid && forkValid && recordedBase != forkPoint { + // Both can be ancestors of the child; prefer the later cutoff. + older, err := git.IsAncestor(recordedBase, forkPoint) + if err != nil { + return "", fmt.Errorf("comparing previous bases of %s: %w", branch, err) + } + if older { + return forkPoint, nil + } + older, err = git.IsAncestor(forkPoint, recordedBase) + if err != nil { + return "", fmt.Errorf("comparing previous bases of %s: %w", branch, err) + } + if !older { + return "", fmt.Errorf("conflicting previous bases for %s after %s changed; rebase this branch manually", branch, newBase) + } } - if recordedBase != currentParentTip && isValid(recordedBase) { + if recordedValid { return recordedBase, nil } - if forkPoint, err := git.MergeBaseForkPoint(newBase, branch); err == nil && isValid(forkPoint) { + if forkValid { return forkPoint, nil } @@ -1259,6 +1318,58 @@ func resolveRebaseOldBase(currentParentTip, recordedBase, newBase, branch string ) } +func rebaseBoundaryContains(candidate, branch string) (bool, error) { + if candidate == "" { + return false, nil + } + valid, err := git.IsAncestor(candidate, branch) + if err == nil { + return valid, nil + } + exists, lookupErr := git.CommitExists(candidate) + if lookupErr == nil && !exists { + return false, nil + } + return false, fmt.Errorf("checking previous base %s of %s: %w", candidate, branch, errors.Join(err, lookupErr)) +} + +func (o cascadeRebaseOpts) mergedHead(branch string) string { + s := o.Stack + if o.State != nil && o.State.OriginalStack != nil { + // PR refreshes during continuation must not change saved cutoffs. + s = o.State.OriginalStack + } + if i := s.IndexOf(branch); i >= 0 && s.Branches[i].IsMerged() { + return s.Branches[i].PullRequest.MergedHead + } + return "" +} + +func resolveMergedRebaseOldBase(mergedHead, localTip, recordedBase, newBase, branch string) (string, error) { + if valid, err := rebaseBoundaryContains(mergedHead, branch); err != nil { + return "", err + } else if valid { + if recordedBase != "" && recordedBase != mergedHead { + // A regular merge keeps the PR head reachable through a newer base. + // Only skip that extra history if the destination contains it too. + for _, pair := range [][2]string{ + {recordedBase, branch}, {mergedHead, recordedBase}, {recordedBase, newBase}, + } { + contains, err := rebaseBoundaryContains(pair[0], pair[1]) + if err != nil { + return "", err + } + if !contains { + return mergedHead, nil + } + } + return recordedBase, nil + } + return mergedHead, nil + } + return resolveRebaseOldBase(localTip, recordedBase, newBase, branch) +} + // cascadeRebaseResult describes the outcome of a cascade rebase. type cascadeRebaseResult struct { Rebased bool // at least one branch was successfully rebased @@ -1396,7 +1507,14 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } } - actualOldBase, err := resolveRebaseOldBase(ontoOldBase, br.Base, newBase, br.Branch) + var actualOldBase string + var err error + if absIdx > 0 && s.Branches[absIdx-1].IsMerged() { + parent := s.Branches[absIdx-1].Branch + actualOldBase, err = resolveMergedRebaseOldBase(opts.mergedHead(parent), ontoOldBase, br.Base, newBase, br.Branch) + } else { + actualOldBase, err = resolveRebaseOldBase(ontoOldBase, br.Base, newBase, br.Branch) + } if err != nil { return cascadeRebaseResult{ Rebased: result.Rebased, @@ -1956,13 +2074,8 @@ func pullRemoteAdditions(cfg *config.Config, sf *stack.StackFile, s *stack.Stack continue } s.Branches = append(s.Branches, stack.BranchRef{ - Branch: pr.HeadRefName, - PullRequest: &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - Merged: pr.Merged, - }, + Branch: pr.HeadRefName, + PullRequest: pullRequestRef(pr, nil), }) added++ } diff --git a/cmd/utils_test.go b/cmd/utils_test.go index 304e27f7..c50dc470 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -1282,6 +1282,7 @@ func TestSyncStackPRs_TrackedPR_DetectsMerge(t *testing.T) { Branches: []stack.BranchRef{ { Branch: "feature", + Head: "local-head", PullRequest: &stack.PullRequestRef{ Number: 42, ID: "PR_42", @@ -1295,11 +1296,12 @@ func TestSyncStackPRs_TrackedPR_DetectsMerge(t *testing.T) { cfg.GitHubClientOverride = &github.MockClient{ FindPRByNumberFn: func(number int) (*github.PullRequest, error) { return &github.PullRequest{ - Number: 42, - ID: "PR_42", - URL: "https://github.com/o/r/pull/42", - State: "MERGED", - Merged: true, + Number: 42, + ID: "PR_42", + URL: "https://github.com/o/r/pull/42", + State: "MERGED", + Merged: true, + HeadRefOID: "merged-head", }, nil }, } @@ -1310,20 +1312,23 @@ func TestSyncStackPRs_TrackedPR_DetectsMerge(t *testing.T) { require.NotNil(t, s.Branches[0].PullRequest) assert.Equal(t, 42, s.Branches[0].PullRequest.Number) assert.True(t, s.Branches[0].PullRequest.Merged) + assert.Equal(t, "merged-head", s.Branches[0].PullRequest.MergedHead) + assert.Equal(t, "local-head", s.Branches[0].Head) } func TestSyncStackPRs_MergedBranch_StaysMerged(t *testing.T) { - // A merged branch should stay merged — no API calls, no changes. + // A merged branch with a known head needs no further lookup. s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ { Branch: "merged-branch", PullRequest: &stack.PullRequestRef{ - Number: 20, - ID: "PR_20", - URL: "https://github.com/o/r/pull/20", - Merged: true, + Number: 20, + ID: "PR_20", + URL: "https://github.com/o/r/pull/20", + Merged: true, + MergedHead: "merged-head", }, }, }, @@ -1351,6 +1356,151 @@ func TestSyncStackPRs_MergedBranch_StaysMerged(t *testing.T) { assert.False(t, apiCalled, "no API calls should be made for merged branches") } +func TestPullRequestRef_MergedHead(t *testing.T) { + for _, tt := range []struct { + name string + previous *stack.PullRequestRef + pr github.PullRequest + wantHead string + wantMerged bool + }{ + { + name: "captures confirmed merge head", + pr: github.PullRequest{Number: 10, Merged: true, HeadRefOID: "remote"}, + wantHead: "remote", wantMerged: true, + }, + { + name: "preserves head on sparse response", + previous: &stack.PullRequestRef{Number: 10, ID: "PR_10", Merged: true, MergedHead: "cached"}, + pr: github.PullRequest{Number: 10, Merged: true}, + wantHead: "cached", wantMerged: true, + }, + { + name: "merged state is final", + previous: &stack.PullRequestRef{Number: 10, Merged: true, MergedHead: "cached"}, + pr: github.PullRequest{Number: 10, HeadRefOID: "open-head"}, + wantHead: "cached", wantMerged: true, + }, + { + name: "does not copy cache to replacement PR", + previous: &stack.PullRequestRef{Number: 10, Merged: true, MergedHead: "cached"}, + pr: github.PullRequest{Number: 11}, + }, + { + name: "does not copy cache across conflicting identities", + previous: &stack.PullRequestRef{Number: 10, ID: "old", Merged: true, MergedHead: "cached"}, + pr: github.PullRequest{Number: 10, ID: "new"}, + }, + { + name: "open heads are not merge heads", + pr: github.PullRequest{Number: 10, HeadRefOID: "open-head"}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + var before stack.PullRequestRef + if tt.previous != nil { + before = *tt.previous + } + ref := pullRequestRef(&tt.pr, tt.previous) + assert.Equal(t, tt.wantHead, ref.MergedHead) + assert.Equal(t, tt.wantMerged, ref.Merged) + if tt.previous != nil { + assert.Equal(t, before, *tt.previous, "operation snapshots must not be mutated") + } + }) + } +} + +func TestSyncStackPRs_MergedHeadBackfill(t *testing.T) { + for _, tt := range []struct { + name string + pr *github.PullRequest + err error + want string + }{ + {"found", &github.PullRequest{Number: 10, Merged: true, HeadRefOID: "remote"}, nil, "remote"}, + {"empty", &github.PullRequest{Number: 10, Merged: true}, nil, ""}, + {"missing", nil, nil, ""}, + {"unavailable", nil, fmt.Errorf("API unavailable"), ""}, + {"non-merged response", &github.PullRequest{Number: 10, State: "OPEN", HeadRefOID: "open"}, nil, ""}, + } { + t.Run(tt.name, func(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{ + Branch: "parent", Head: "local", + PullRequest: &stack.PullRequestRef{Number: 10, ID: "PR_10", Merged: true}, + }}, + } + calls := 0 + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(number int) (*github.PullRequest, error) { + assert.Equal(t, 10, number) + calls++ + return tt.pr, tt.err + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { + t.Error("must not rediscover a replacement for a merged PR") + return nil, nil + }, + } + syncStackPRs(cfg, s) + if tt.want != "" { + syncStackPRs(cfg, s) + } + collectOutput(cfg, outR, errR) + assert.Equal(t, 1, calls) + assert.True(t, s.Branches[0].IsMerged()) + assert.Equal(t, tt.want, s.Branches[0].PullRequest.MergedHead) + assert.Equal(t, "PR_10", s.Branches[0].PullRequest.ID) + assert.Equal(t, "local", s.Branches[0].Head) + }) + } +} + +func TestSyncStackPRs_RemoteStack_MergedHeadBackfill(t *testing.T) { + for _, included := range []bool{false, true} { + t.Run(fmt.Sprintf("merged PR listed=%t", included), func(t *testing.T) { + s := &stack.Stack{ + ID: "100", Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "parent", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "child", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + calls := 0 + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + numbers := []int{11} + if included { + numbers = []int{10, 11} + } + return []github.RemoteStack{{ID: 100, PullRequests: numbers}}, nil + }, + FindPRByNumberFn: func(number int) (*github.PullRequest, error) { + if number == 10 { + calls++ + return &github.PullRequest{Number: 10, HeadRefName: "parent", Merged: true, HeadRefOID: "remote"}, nil + } + return &github.PullRequest{Number: 11, HeadRefName: "child", State: "OPEN"}, nil + }, + } + syncStackPRs(cfg, s) + syncStackPRs(cfg, s) + collectOutput(cfg, outR, errR) + wantCalls := 1 + if included { + wantCalls = 2 + } + assert.Equal(t, wantCalls, calls) + assert.Equal(t, "remote", s.Branches[0].PullRequest.MergedHead) + assert.True(t, s.Branches[0].IsMerged()) + }) + } +} + func TestSyncStackPRs_ClosedPR_ReplacedByOpenPR(t *testing.T) { // A tracked PR that was closed (not merged) should be replaced // by a new OPEN PR if one exists. diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index 01fb1f93..f806f8da 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -437,6 +437,17 @@ Fetches the latest changes from `origin`, then ensures each branch in the stack If a branch's PR has been merged, the rebase automatically switches to `--onto` mode to correctly replay commits on top of the merge target. +Merged PR heads are cached separately from local branch tips, so a stale or deleted +local merged branch does not determine which commits to replay. Both `rebase` +and `sync` validate the cutoff against the child's history, including recorded +bases and reflog fork points when the parent was rewritten or the child was +already restacked. Older stacks acquire missing PR-head metadata when GitHub +can provide it. + +If no safe cutoff can be established, the command stops with guidance to rebase +the affected branch manually rather than guessing. This does not automatically +reconcile diverged active branches, and `--no-trunk` still skips fetching. + If a rebase conflict occurs, the operation pauses and prints the conflicted files with line numbers. Resolve the conflicts, stage with `git add`, and continue with `--continue`. To undo the entire rebase, use `--abort` to restore all branches to their pre-rebase state. **Examples:** diff --git a/internal/config/testing.go b/internal/config/testing.go index 91835b57..ad3bca26 100644 --- a/internal/config/testing.go +++ b/internal/config/testing.go @@ -4,10 +4,11 @@ import ( "os" "github.com/cli/go-gh/v2/pkg/term" + "github.com/github/gh-stack/internal/github" ) // NewTestConfig creates a Config suitable for testing with captured output buffers. -// Color functions are no-ops, and the config is non-interactive. +// Color functions are no-ops, GitHub calls are stubbed, and the config is non-interactive. func NewTestConfig() (*Config, *os.File, *os.File) { outR, outW, _ := os.Pipe() errR, errW, _ := os.Pipe() @@ -15,18 +16,19 @@ func NewTestConfig() (*Config, *os.File, *os.File) { noop := func(s string) string { return s } cfg := &Config{ - Terminal: term.FromEnv(), - Out: outW, - Err: errW, - In: os.Stdin, - ColorSuccess: noop, - ColorError: noop, - ColorWarning: noop, - ColorBold: noop, - ColorBlue: noop, - ColorMagenta: noop, - ColorCyan: noop, - ColorGray: noop, + Terminal: term.FromEnv(), + Out: outW, + Err: errW, + In: os.Stdin, + ColorSuccess: noop, + ColorError: noop, + ColorWarning: noop, + ColorBold: noop, + ColorBlue: noop, + ColorMagenta: noop, + ColorCyan: noop, + ColorGray: noop, + GitHubClientOverride: &github.MockClient{}, } return cfg, outR, errR diff --git a/internal/git/git.go b/internal/git/git.go index 2413a2fa..37bd7c4e 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -454,10 +454,16 @@ func MergeBase(a, b string) (string, error) { // MergeBaseForkPoint returns where branch forked from ref, using ref's reflog // to see through rewrites such as amend, rebase, or force-push. +// An unavailable fork point is returned as an empty string without an error. func MergeBaseForkPoint(ref, branch string) (string, error) { return ops.MergeBaseForkPoint(ref, branch) } +// CommitExists reports whether ref resolves to a locally available commit. +func CommitExists(ref string) (bool, error) { + return ops.CommitExists(ref) +} + // 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 df95848b..30afe3d5 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -64,6 +64,7 @@ type Ops interface { ConflictedFiles() ([]string, error) FindConflictMarkers(filePath string) (*ConflictMarkerInfo, error) IsAncestor(ancestor, descendant string) (bool, error) + CommitExists(ref string) (bool, error) RevParse(ref string) (string, error) RevParseMulti(refs []string) ([]string, error) MergeBase(a, b string) (string, error) @@ -518,6 +519,18 @@ func (d *defaultOps) IsAncestor(ancestor, descendant string) (bool, error) { return false, err } +func (d *defaultOps) CommitExists(ref string) (bool, error) { + _, err := d.run("rev-parse", "--verify", "--quiet", "--end-of-options", ref+"^{commit}") + if err == nil { + return true, nil + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + return false, err +} + func (d *defaultOps) RevParse(ref string) (string, error) { return d.run("rev-parse", ref) } @@ -543,7 +556,12 @@ func (d *defaultOps) MergeBase(a, b string) (string, error) { } func (d *defaultOps) MergeBaseForkPoint(ref, branch string) (string, error) { - return d.run("merge-base", "--fork-point", ref, branch) + point, err := d.run("merge-base", "--fork-point", ref, branch) + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return "", nil + } + return point, err } func (d *defaultOps) Log(ref string, maxCount int) ([]CommitInfo, error) { diff --git a/internal/git/gitops_test.go b/internal/git/gitops_test.go index 512b8d64..9b9e105f 100644 --- a/internal/git/gitops_test.go +++ b/internal/git/gitops_test.go @@ -91,6 +91,50 @@ func setupLocalRepo(t *testing.T) string { return dir } +func TestIntegration_RebaseBoundaryLookups(t *testing.T) { + dir := setupLocalRepo(t) + restore := withGitDir(t, dir) + defer restore() + d := &defaultOps{} + + for _, tt := range []struct { + ref string + exists bool + }{ + {"main", true}, + {gitExec(t, dir, "rev-parse", "HEAD"), true}, + {"missing", false}, + {strings.Repeat("f", 40), false}, + {"HEAD:init.txt", false}, + } { + t.Run(tt.ref, func(t *testing.T) { + exists, err := d.CommitExists(tt.ref) + require.NoError(t, err) + assert.Equal(t, tt.exists, exists) + }) + } + + point, err := d.MergeBaseForkPoint("main", "main") + require.NoError(t, err) + assert.Equal(t, gitExec(t, dir, "rev-parse", "main"), point) + _, err = d.MergeBaseForkPoint("missing", "main") + require.Error(t, err) + + gitExec(t, dir, "checkout", "--orphan", "unrelated") + gitExec(t, dir, "commit", "--allow-empty", "-m", "unrelated root") + point, err = d.MergeBaseForkPoint("main", "unrelated") + require.NoError(t, err) + assert.Empty(t, point) + + scoped := requireWorktree(t, d, dir) + restore() + renamed := dir + "-moved" + require.NoError(t, os.Rename(dir, renamed)) + t.Cleanup(func() { require.NoError(t, os.Rename(renamed, dir)) }) + _, err = scoped.CommitExists("main") + require.Error(t, err, "a lost worktree is not a missing commit") +} + // withGitDir temporarily sets the git client to operate in dir by changing // the working directory. Returns a restore function. func withGitDir(t *testing.T, dir string) func() { diff --git a/internal/git/mock_ops.go b/internal/git/mock_ops.go index f0f9cbac..a3f894ce 100644 --- a/internal/git/mock_ops.go +++ b/internal/git/mock_ops.go @@ -41,6 +41,7 @@ type MockOps struct { ConflictedFilesFn func() ([]string, error) FindConflictMarkersFn func(string) (*ConflictMarkerInfo, error) IsAncestorFn func(string, string) (bool, error) + CommitExistsFn func(string) (bool, error) RevParseFn func(string) (string, error) RevParseMultiFn func([]string) ([]string, error) MergeBaseFn func(string, string) (string, error) @@ -292,6 +293,13 @@ func (m *MockOps) IsAncestor(ancestor, descendant string) (bool, error) { return false, nil } +func (m *MockOps) CommitExists(ref string) (bool, error) { + if m.CommitExistsFn != nil { + return m.CommitExistsFn(ref) + } + return true, nil +} + func (m *MockOps) RevParse(ref string) (string, error) { if m.RevParseFn != nil { return m.RevParseFn(ref) diff --git a/internal/github/github.go b/internal/github/github.go index 9da4954a..2b4a7c5a 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -33,6 +33,7 @@ type PullRequest struct { Title string `graphql:"title"` Body string `graphql:"body"` HeadRefName string `graphql:"headRefName"` + HeadRefOID string `graphql:"headRefOid"` BaseRefName string `graphql:"baseRefName"` IsDraft bool `graphql:"isDraft"` Merged bool `graphql:"merged"` @@ -354,6 +355,7 @@ func (c *Client) FindPRByNumber(number int) (*PullRequest, error) { Title string `graphql:"title"` Body string `graphql:"body"` HeadRefName string `graphql:"headRefName"` + HeadRefOID string `graphql:"headRefOid"` BaseRefName string `graphql:"baseRefName"` IsDraft bool `graphql:"isDraft"` Merged bool `graphql:"merged"` @@ -385,6 +387,7 @@ func (c *Client) FindPRByNumber(number int) (*PullRequest, error) { Title: n.Title, Body: n.Body, HeadRefName: n.HeadRefName, + HeadRefOID: n.HeadRefOID, BaseRefName: n.BaseRefName, IsDraft: n.IsDraft, Merged: n.Merged, diff --git a/internal/github/github_test.go b/internal/github/github_test.go index baeff335..8f460cca 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -88,6 +88,55 @@ func TestToGraphQLInt(t *testing.T) { }) } +func TestFindPRByNumber_MergedHead(t *testing.T) { + for _, tt := range []struct { + name string + head string + omit bool + }{ + {name: "merged PR head", head: "0123456789abcdef0123456789abcdef01234567"}, + {name: "empty head"}, + {name: "missing head", omit: true}, + } { + t.Run(tt.name, func(t *testing.T) { + pr := map[string]any{ + "number": 42, "state": "MERGED", "merged": true, "headRefName": "feature", + } + if !tt.omit { + pr["headRefOid"] = tt.head + } + body, err := json.Marshal(map[string]any{ + "data": map[string]any{"repository": map[string]any{"pullRequest": pr}}, + }) + require.NoError(t, err) + transport := roundTripFunc(func(r *http.Request) (*http.Response, error) { + var request struct { + Query string `json:"query"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&request)) + assert.Contains(t, request.Query, "headRefOid") + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewReader(body)), + Header: http.Header{"Content-Type": []string{"application/json"}}, + Request: r, + }, nil + }) + gql, err := api.NewGraphQLClient(api.ClientOptions{ + Host: "github.com", AuthToken: "x", Transport: transport, + }) + require.NoError(t, err) + + client := &Client{gql: gql, owner: "o", repo: "r"} + got, err := client.FindPRByNumber(42) + require.NoError(t, err) + require.NotNil(t, got) + assert.True(t, got.Merged) + assert.Equal(t, tt.head, got.HeadRefOID) + }) + } +} + // TestRemoteStack_UnmarshalJSON verifies the custom decoder that flattens the // Stacks REST API's pull_requests object array into ordered PullRequests // numbers while preserving the full entries in PRDetails. This is the only path diff --git a/internal/stack/schema.json b/internal/stack/schema.json index 49547bc1..d5cd1eb8 100644 --- a/internal/stack/schema.json +++ b/internal/stack/schema.json @@ -89,6 +89,10 @@ "merged": { "type": "boolean", "description": "Whether the pull request has been merged." + }, + "mergedHead": { + "type": "string", + "description": "The head commit associated with the merged pull request, distinct from the resulting merge commit and the current local branch tip." } } } diff --git a/internal/stack/stack.go b/internal/stack/stack.go index 0e9db7e6..2881ceef 100644 --- a/internal/stack/stack.go +++ b/internal/stack/stack.go @@ -18,10 +18,11 @@ const ( // PullRequestRef holds relatively immutable metadata about an associated PR. type PullRequestRef struct { - Number int `json:"number"` - ID string `json:"id,omitempty"` - URL string `json:"url,omitempty"` - Merged bool `json:"merged,omitempty"` + Number int `json:"number"` + ID string `json:"id,omitempty"` + URL string `json:"url,omitempty"` + Merged bool `json:"merged,omitempty"` + MergedHead string `json:"mergedHead,omitempty"` } // BranchRef represents a branch and its associated commit hash. diff --git a/internal/stack/stack_test.go b/internal/stack/stack_test.go index 22d5a581..71d8105d 100644 --- a/internal/stack/stack_test.go +++ b/internal/stack/stack_test.go @@ -242,7 +242,7 @@ func TestLoad_Save_RoundTrip(t *testing.T) { Trunk: BranchRef{Branch: "main", Head: "abc123"}, Branches: []BranchRef{ {Branch: "b1", Head: "def456", Base: "abc123"}, - {Branch: "b2", PullRequest: &PullRequestRef{Number: 42, ID: "PR_id", URL: "https://example.com", Merged: true}}, + {Branch: "b2", PullRequest: &PullRequestRef{Number: 42, ID: "PR_id", URL: "https://example.com", Merged: true, MergedHead: "merged-pr-head"}}, }, }, }, @@ -269,6 +269,17 @@ func TestLoad_Save_RoundTrip(t *testing.T) { require.NotNil(t, s.Branches[1].PullRequest) assert.Equal(t, 42, s.Branches[1].PullRequest.Number) assert.True(t, s.Branches[1].PullRequest.Merged) + assert.Equal(t, "merged-pr-head", s.Branches[1].PullRequest.MergedHead) + }) + + t.Run("legacy merged PR metadata remains valid", func(t *testing.T) { + var ref PullRequestRef + require.NoError(t, json.Unmarshal([]byte(`{"number":42,"merged":true}`), &ref)) + assert.True(t, ref.Merged) + assert.Empty(t, ref.MergedHead) + data, err := json.Marshal(ref) + require.NoError(t, err) + assert.JSONEq(t, `{"number":42,"merged":true}`, string(data)) }) t.Run("missing file returns empty stack file", func(t *testing.T) {