Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions cmd/submit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 ---
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down
24 changes: 14 additions & 10 deletions cmd/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
45 changes: 45 additions & 0 deletions cmd/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"path/filepath"
"strings"
"sync"
"testing"

"github.com/cli/go-gh/v2/pkg/api"
Expand Down Expand Up @@ -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"}),
}

Expand Down Expand Up @@ -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) {
Expand Down
105 changes: 61 additions & 44 deletions cmd/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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))
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand All @@ -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)
Expand Down
Loading
Loading