diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 8c2c50f..0107211 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -459,6 +459,9 @@ func TestSubmit_ForksWhenRemoteStackFullyMerged(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{{ID: 42, PullRequests: []int{1, 2}}}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{1, 2}}, nil + }, FindPRByNumberFn: func(n int) (*github.PullRequest, error) { switch n { case 1: @@ -1853,10 +1856,9 @@ func TestSubmit_PreflightCheck_SkippedWhenStackIDSet(t *testing.T) { _, _ = io.ReadAll(errR) assert.NoError(t, err) - // ListStacks is called by syncStackPRs (remote sync), but NOT by the - // preflight check. Two syncStackPRs calls happen in submit (before and - // after PR creation), so expect exactly 2 ListStacks calls. - assert.Equal(t, 2, listStacksCallCount, "ListStacks should only be called by syncStackPRs, not by the preflight check") + // With the stack number known, neither the preflight check nor the PR + // refresh needs to scan every stack in the repository. + assert.Equal(t, 0, listStacksCallCount, "ListStacks should not be called for a tracked stack with a number") } // --- Modify + Submit integration tests --- @@ -2012,6 +2014,9 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { if number == 7 && !oldDeleted { return &github.RemoteStack{ID: 42, Number: 7, PullRequests: oldNumbers}, nil } + if number == 7 && tc.mergedBase { + return &github.RemoteStack{ID: 42, Number: 7, PullRequests: []int{9}}, nil + } if number == 8 && replacement != nil { return replacement, nil } @@ -2661,6 +2666,12 @@ func TestSubmit_WithPendingModify_AtomicPush(t *testing.T) { } return []github.RemoteStack{{ID: 42, Number: 7, PullRequests: []int{10, 11, 12}}}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + if unstacked { + return nil, &api.HTTPError{StatusCode: 404} + } + return &github.RemoteStack{ID: 42, Number: 7, PullRequests: []int{10, 11, 12}}, nil + }, } cmd := SubmitCmd(cfg) diff --git a/cmd/sync.go b/cmd/sync.go index b5ece46..fb53c95 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -162,7 +162,12 @@ func runSync(cfg *config.Config, opts *syncOptions) error { if cb, cbErr := git.CurrentBranch(); cbErr == nil { currentBranch = cb } - _ = syncStackPRs(cfg, s) + // Reuse reconcile's fetch when it left the stack unchanged. + if reconcileRes.remotePRs != nil { + _ = applyRemoteStackPRs(s, reconcileRes.remotePRs) + } else { + _ = syncStackPRs(cfg, s) + } newBase, err := retargetedBase(cfg, s, remote) if err != nil { cfg.Errorf("failed to read the stack's base on GitHub: %v", err) @@ -335,10 +340,9 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } } - // --- Step 5: Sync PR state --- + // --- Step 5: Report PR state --- cfg.Printf("") - cfg.Printf("Syncing PRs ...") - _ = syncStackPRs(cfg, s) + cfg.Printf("Reporting PR status ...") // Report PR status for each branch for _, b := range s.Branches { @@ -369,12 +373,12 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } // --- Step 5b: Reconcile the remote stack object --- - // syncStackPRs above only refreshes local PR associations; it does not touch - // the stack object on GitHub. When the branches have open PRs, link them into - // a stack so the remote reflects the local stack. This never opens PRs — that - // is still `gh stack submit`'s job. stackSynced records whether the remote - // stack object actually reflects the local stack, which determines the final - // summary message below. + // The earlier PR refresh only updates local PR associations; it does not + // touch the stack object on GitHub. When the branches have open PRs, link + // them into a stack so the remote reflects the local stack. This never opens + // PRs — that is still `gh stack submit`'s job. stackSynced records whether + // the remote stack object actually reflects the local stack, which determines + // the final summary message below. stackSynced := false if client, err := cfg.GitHubClient(); err == nil { stackSynced = syncStack(cfg, client, s) diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 063b7e9..e92a6a6 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "github.com/cli/go-gh/v2/pkg/api" @@ -2214,6 +2215,9 @@ func TestSync_RemoteAhead_DuplicateBranchAborts(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}, nil + }, FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2", 103: "b3"}), } @@ -2294,6 +2298,47 @@ func TestSync_RemoteInSync_NoPull(t *testing.T) { assert.Contains(t, output, "Stack synced") } +func TestSync_RemoteInSync_ReusesRemotePRFetch(t *testing.T) { + s := stack.Stack{ + ID: "9", + Number: 9, + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, + }, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + listStacksCalls := 0 + var mu sync.Mutex + prLookups := map[int]int{} + findPR := prByNumberFinder(map[int]string{101: "b1", 102: "b2"}) + ghMock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + listStacksCalls++ + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102}}}, nil + }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102}}, nil + }, + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + mu.Lock() + prLookups[n]++ + mu.Unlock() + return findPR(n) + }, + } + + output, err := runSyncCfg(t, newSyncMockNoRebase(tmpDir, "b1"), func(cfg *config.Config) { cfg.GitHubClientOverride = ghMock }) + require.NoError(t, err) + assert.Contains(t, output, "Stack synced") + + assert.Zero(t, listStacksCalls, "a numbered stack should not scan every stack in the repository") + assert.Equal(t, map[int]int{101: 1, 102: 1}, prLookups, "each PR should be fetched once per sync") +} + // divergentStack returns a stack file (ID 9) and GitHub mock configured so that // the local stack [b1,b2,b3] diverges from the remote stack [b1,b2,b4]. func divergentStack(t *testing.T, tmpDir string) { diff --git a/cmd/utils.go b/cmd/utils.go index db43aa9..3d842cb 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -762,32 +762,54 @@ func enrichPRContent(client github.ClientOps, details map[string]*github.PRDetai // closed. Returns the PRDetails map and true if sync succeeded, or nil and // false if we should fall back to local discovery. func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string]*github.PRDetails, bool) { - stacks, err := client.ListStacks() - if err != nil { + fetched, ok := fetchRemoteStackPRs(client, s) + if !ok { return nil, false } + return applyRemoteStackPRs(s, fetched), true +} - // Find our stack in the remote list. - var remotePRNumbers []int - for _, rs := range stacks { - if strconv.Itoa(rs.ID) == s.ID { - remotePRNumbers = rs.PRNumbers() - // Backfill the human-facing stack number for stack files created - // before it was tracked, so callers (view, submit TUI) can display - // it. Persisted by whichever command later saves the stack file. - if s.Number == 0 { - s.Number = rs.Number - } - break +// remoteStackPRs is one fetch of a tracked stack's pull requests. +// - numbers lists the stack's PRs from bottom to top +// - queried appends merged PRs that are no longer in the stack but still need their head SHA +// - prs is parallel to queried and holds nil where a lookup failed +type remoteStackPRs struct { + numbers []int + queried []int + prs []*github.PullRequest +} + +// stackPRs returns the stack's PRs in order, or false if any lookup failed. +func (r *remoteStackPRs) stackPRs() ([]*github.PullRequest, bool) { + prs := r.prs[:len(r.numbers)] + for _, pr := range prs { + if pr == nil { + return nil, false } } - if remotePRNumbers == nil { + return prs, true +} + +// fetchRemoteStackPRs reads the tracked stack by number, then fetches its PRs concurrently. +// It returns false when the stack can't be read. +func fetchRemoteStackPRs(client github.ClientOps, s *stack.Stack) (*remoteStackPRs, bool) { + if s.ID == "" { return nil, false } - remotePRCount := len(remotePRNumbers) - queried := make(map[int]bool, remotePRCount) - for _, number := range remotePRNumbers { + number, err := ensureStackNumber(client, s) + if err != nil || number == 0 { + return nil, false + } + rs, err := client.GetStack(number) + if err != nil || rs == nil || strconv.Itoa(rs.ID) != s.ID || rs.PRNumbers() == nil { + return nil, false + } + + numbers := rs.PRNumbers() + remotePRNumbers := slices.Clone(numbers) + queried := make(map[int]bool, len(numbers)) + for _, number := range numbers { queried[number] = true } for _, b := range s.Branches { @@ -798,10 +820,7 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string } } - // 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 - // sequential behavior of the old code. + // Fetch each remote PR concurrently. prResults := make([]*github.PullRequest, len(remotePRNumbers)) var wg sync.WaitGroup @@ -826,6 +845,16 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string } wg.Wait() + return &remoteStackPRs{numbers: numbers, queried: remotePRNumbers, prs: prResults}, true +} + +// applyRemoteStackPRs updates the local branches from a remote stack fetch and +// returns PRDetails keyed by branch name. +func applyRemoteStackPRs(s *stack.Stack, fetched *remoteStackPRs) map[string]*github.PRDetails { + remotePRCount := len(fetched.numbers) + prResults := fetched.prs + remotePRNumbers := fetched.queried + // Build map sequentially to preserve order semantics. prByBranch := make(map[string]*github.PullRequest, len(remotePRNumbers)) prByNumber := make(map[int]*github.PullRequest, len(remotePRNumbers)) @@ -857,7 +886,7 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string details[b.Branch] = prDetailsFromPR(pr) } - return details, true + return details } // updateBaseSHAs refreshes the Base and Head SHAs for all active branches @@ -2053,6 +2082,10 @@ type remoteReconcileResult struct { // cancels or deletes the remote stack, and when a divergence is detected in a // non-interactive terminal. The resolving path prints its own outcome message. stop bool + + // remotePRs, when non-nil, is the PR fetch reconcile made for a stack it + // left unchanged, so runSync can refresh PR state without fetching again. + remotePRs *remoteStackPRs } // remoteStackClass classifies the relationship between the local stack's active @@ -2086,29 +2119,12 @@ func reconcileRemoteStack(cfg *config.Config, sf *stack.StackFile, s *stack.Stac return res, nil } - stacks, err := client.ListStacks() - if err != nil { - // Covers 404 (stacked PRs unavailable) and transient API errors. - return res, nil - } - - var remotePRNumbers []int - found := false - for _, rs := range stacks { - if strconv.Itoa(rs.ID) == s.ID { - remotePRNumbers = rs.PRNumbers() - found = true - break - } - } - if !found { - // The remote stack was deleted; the existing updateStack 404 → recreate - // path in syncStack handles this. + fetched, ok := fetchRemoteStackPRs(client, s) + if !ok { return res, nil } - - prs, err := fetchStackPRDetails(client, remotePRNumbers) - if err != nil { + prs, ok := fetched.stackPRs() + if !ok { return res, nil } @@ -2121,6 +2137,7 @@ func reconcileRemoteStack(cfg *config.Config, sf *stack.StackFile, s *stack.Stac // fast-forward/rebase/push steps skip merged and merge-queued branches // rather than rewriting them before the later PR-sync step runs. syncRemotePRState(s, prs) + res.remotePRs = fetched return res, nil case remoteStackCleanAhead: return pullRemoteAdditions(cfg, sf, s, gitDir, remote, prs) diff --git a/cmd/utils_test.go b/cmd/utils_test.go index f421188..555de90 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -1465,7 +1465,7 @@ 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"}, + ID: "100", Number: 7, Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "parent", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, {Branch: "child", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -1474,12 +1474,12 @@ func TestSyncStackPRs_RemoteStack_MergedHeadBackfill(t *testing.T) { calls := 0 cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + GetStackFn: func(int) (*github.RemoteStack, error) { numbers := []int{11} if included { numbers = []int{10, 11} } - return []github.RemoteStack{{ID: 100, PullRequests: numbers}}, nil + return &github.RemoteStack{ID: 100, Number: 7, PullRequests: numbers}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { if number == 10 { @@ -1625,20 +1625,25 @@ func TestSyncStackPRs_RemoteStack_UsesStackAPI(t *testing.T) { // When the stack has a remote ID, sync should use the stack API // as source of truth, matching PRs to branches by head ref name. s := &stack.Stack{ - ID: "100", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "100", + Number: 7, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1"}, {Branch: "b2"}, }, } + var gotNumber int cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 100, PullRequests: []int{10, 11}}, - }, nil + t.Error("ListStacks should not be called when the stack number is known") + return nil, nil + }, + GetStackFn: func(number int) (*github.RemoteStack, error) { + gotNumber = number + return &github.RemoteStack{ID: 100, Number: 7, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { switch number { @@ -1654,6 +1659,8 @@ func TestSyncStackPRs_RemoteStack_UsesStackAPI(t *testing.T) { _ = syncStackPRs(cfg, s) collectOutput(cfg, outR, errR) + assert.Equal(t, 7, gotNumber) + // b1 should be tracked with open PR require.NotNil(t, s.Branches[0].PullRequest) assert.Equal(t, 10, s.Branches[0].PullRequest.Number) @@ -1685,6 +1692,9 @@ func TestSyncStackPRs_BackfillsStackNumber(t *testing.T) { {ID: 100, Number: 5, PullRequests: []int{10, 11}}, }, nil }, + GetStackFn: func(number int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 100, Number: number, PullRequests: []int{10, 11}}, nil + }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { switch number { case 10: @@ -1706,8 +1716,9 @@ func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) { // When using the stack API, a closed (not merged) PR should remain // associated — the stack API is the source of truth, not PR state. s := &stack.Stack{ - ID: "200", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "200", + Number: 2, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "feature", PullRequest: &stack.PullRequestRef{Number: 5}}, }, @@ -1715,10 +1726,8 @@ func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 200, PullRequests: []int{5}}, - }, nil + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 200, Number: 2, PullRequests: []int{5}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { return &github.PullRequest{Number: 5, ID: "PR_5", URL: "https://github.com/o/r/pull/5", HeadRefName: "feature", State: "CLOSED"}, nil @@ -1737,8 +1746,9 @@ func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) { func TestSyncStackPRs_RemoteStack_FallsBackOnAPIError(t *testing.T) { // If the stack API fails, fall back to local discovery. s := &stack.Stack{ - ID: "300", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "300", + Number: 3, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "feature"}, }, @@ -1746,7 +1756,7 @@ func TestSyncStackPRs_RemoteStack_FallsBackOnAPIError(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + GetStackFn: func(int) (*github.RemoteStack, error) { return nil, fmt.Errorf("API error") }, FindPRForBranchFn: func(branch string) (*github.PullRequest, error) {