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
62 changes: 29 additions & 33 deletions cmd/checkout.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,13 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string,
return nil, "", ErrAPIFailure
}

// Step 1: List stacks and find one containing the target PR
remoteStack, err := findRemoteStackForPR(client, prNumber)
// Step 1: Find the stack containing the target PR via the list endpoint's
// server-side pull_request filter.
remoteStack, err := client.FindStackForPR(prNumber)
if err != nil {
var httpErr *api.HTTPError
if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
warnStacksUnavailableOrPAT(cfg)
warnStacksUnavailable(cfg)
return nil, "", ErrAPIFailure
}
cfg.Errorf("failed to list stacks: %v", err)
Expand All @@ -205,7 +206,7 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string,
}

// Step 2: Fetch PR details for every PR in the remote stack
prs, err := fetchStackPRDetails(client, remoteStack.PullRequests)
prs, err := fetchStackPRDetails(client, remoteStack.PRNumbers())
if err != nil {
cfg.Errorf("failed to fetch PR details: %v", err)
return nil, "", ErrAPIFailure
Expand Down Expand Up @@ -245,11 +246,14 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string,
syncRemotePRState(localStack, prs)

// Case A: branch is in a local stack — check composition
if stackCompositionMatches(localStack, remoteStack.PullRequests) {
if stackCompositionMatches(localStack, remoteStack.PRNumbers()) {
// Composition matches — checkout
if localStack.ID == "" {
localStack.ID = remoteStackID
}
// remoteStack is authoritative for both identifiers here, so
// refresh them together. Updating only one (e.g. the number while
// keeping a stale ID) breaks later ID-based discovery when the old
// remote stack was replaced by a new one holding the same PRs.
localStack.ID = remoteStackID
localStack.Number = remoteStack.Number
if err := stack.Save(gitDir, sf); err != nil {
return nil, "", handleSaveError(cfg, err)
}
Expand All @@ -274,7 +278,7 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string,
return nil, "", ErrSilent
}

s, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID)
s, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID, remoteStack.Number)
if err != nil {
return nil, "", err
}
Expand All @@ -286,23 +290,6 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string,
return s, targetBranch, nil
}

// findRemoteStackForPR queries the list stacks API and returns the stack
// containing the given PR number, or nil if no stack contains it.
func findRemoteStackForPR(client github.ClientOps, prNumber int) (*github.RemoteStack, error) {
stacks, err := client.ListStacks()
if err != nil {
return nil, err
}
for i := range stacks {
for _, n := range stacks[i].PullRequests {
if n == prNumber {
return &stacks[i], nil
}
}
}
return nil, nil
}

// fetchStackPRDetails fetches PR details for each number in the stack.
// Returns PRs in the same order as the input numbers.
func fetchStackPRDetails(client github.ClientOps, prNumbers []int) ([]*github.PullRequest, error) {
Expand Down Expand Up @@ -418,7 +405,7 @@ func handleCompositionConflict(
return nil, ErrSilent
}

s, importErr := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID)
s, importErr := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID, remoteStack.Number)
if importErr != nil {
return nil, importErr
}
Expand All @@ -429,19 +416,26 @@ func handleCompositionConflict(
return s, nil

case 1:
// Delete remote stack, keep local
if err := client.DeleteStack(remoteStackID); err != nil {
// Unstack the remote stack, keep local
_, dissolved, err := client.Unstack(remoteStack.Number)
if err != nil {
var httpErr *api.HTTPError
if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
cfg.Warningf("Remote stack already deleted")
cfg.Warningf("Remote stack already removed")
} else if errors.As(err, &httpErr) && httpErr.StatusCode == 422 {
cfg.Errorf("Cannot unstack remote stack: %s", httpErr.Message)
return nil, ErrAPIFailure
} else {
cfg.Errorf("failed to delete remote stack: %v", err)
cfg.Errorf("failed to unstack remote stack: %v", err)
return nil, ErrAPIFailure
}
} else if dissolved {
cfg.Successf("Remote stack removed")
} else {
cfg.Successf("Remote stack deleted")
cfg.Warningf("Some pull requests could not be unstacked and remain on GitHub")
}
localStack.ID = ""
localStack.Number = 0
Comment thread
skarim marked this conversation as resolved.
if err := stack.Save(gitDir, sf); err != nil {
return nil, handleSaveError(cfg, err)
}
Expand Down Expand Up @@ -475,6 +469,7 @@ func importRemoteStack(
trunk string,
prs []*github.PullRequest,
remoteStackID string,
remoteStackNumber int,
) (*stack.Stack, error) {
// Fetch latest refs from remote
if err := git.Fetch(remote); err != nil {
Expand Down Expand Up @@ -512,7 +507,8 @@ func importRemoteStack(

trunkSHA, _ := git.RevParse(trunk)
newStack := stack.Stack{
ID: remoteStackID,
ID: remoteStackID,
Number: remoteStackNumber,
Trunk: stack.BranchRef{
Branch: trunk,
Head: trunkSHA,
Expand Down
112 changes: 33 additions & 79 deletions cmd/checkout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ func TestCheckout_NumericTarget_StacksNotAvailable(t *testing.T) {
require.NoError(t, stack.Save(gitDir, &stack.StackFile{SchemaVersion: 1, Stacks: []stack.Stack{}}))

cfg, outR, errR := config.NewTestConfig()
setTestTokenForHost(cfg, "gho_test_oauth_token")
setTestRepo(cfg)
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"}
},
}
Expand All @@ -184,10 +184,8 @@ func TestCheckout_NumericTarget_PRNotInStack(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 1, PullRequests: []int{10, 11}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return nil, nil // PR 99 is not part of any stack
},
}

Expand Down Expand Up @@ -243,10 +241,8 @@ func TestCheckout_NumericTarget_NewStack(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 42, PullRequests: []int{10, 11, 12}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{10, 11, 12}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand Down Expand Up @@ -331,10 +327,8 @@ func TestCheckout_NumericTarget_BranchExistsNoStack(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 99, PullRequests: []int{10, 11}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand Down Expand Up @@ -445,11 +439,9 @@ func TestCheckout_NumericTarget_LocalMiss_RemoteMatch(t *testing.T) {
apiCalled := false
cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
apiCalled = true
return []github.RemoteStack{
{ID: 99, PullRequests: []int{10, 11}},
}, nil
return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand All @@ -464,7 +456,7 @@ func TestCheckout_NumericTarget_LocalMiss_RemoteMatch(t *testing.T) {
_ = collectOutput(cfg, outR, errR)

require.NoError(t, err)
assert.True(t, apiCalled, "should have called ListStacks API when local miss")
assert.True(t, apiCalled, "should have queried the remote stack API when local miss")
assert.Equal(t, "feat-2", checkedOut)
}

Expand Down Expand Up @@ -493,8 +485,8 @@ func TestCheckout_NumericTarget_FallbackToBranchName(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{}, nil // no remote stacks
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return nil, nil // no remote stack contains this PR
},
}

Expand Down Expand Up @@ -527,11 +519,9 @@ func TestCheckout_NumericTarget_CompositionMismatch_NonInteractive(t *testing.T)

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
// Remote stack has PRs 10, 11, 12 (extra PR added)
return []github.RemoteStack{
{ID: 42, PullRequests: []int{10, 11, 12}},
}, nil
return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{10, 11, 12}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand Down Expand Up @@ -590,10 +580,8 @@ func TestCheckout_NumericTarget_ClosedMergedPR(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 50, PullRequests: []int{10, 11}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return &github.RemoteStack{ID: 50, Number: 50, PullRequests: []int{10, 11}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand Down Expand Up @@ -662,10 +650,8 @@ func TestCheckout_NumericTarget_MergedBranchDeletedFromRemote(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 60, PullRequests: []int{10, 11}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return &github.RemoteStack{ID: 60, Number: 60, PullRequests: []int{10, 11}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand Down Expand Up @@ -698,10 +684,8 @@ func TestCheckout_NumericTarget_AllPRsMerged(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 70, PullRequests: []int{10, 11}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return &github.RemoteStack{ID: 70, Number: 70, PullRequests: []int{10, 11}}, nil
},
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
prs := map[int]*github.PullRequest{
Expand Down Expand Up @@ -732,7 +716,7 @@ func TestCheckout_NumericTarget_APIError(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return nil, fmt.Errorf("network error")
},
}
Expand Down Expand Up @@ -796,8 +780,8 @@ func TestCheckout_NumericTarget_EmptyStacks(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{}, nil // no stacks at all
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return nil, nil // no stacks at all
},
}

Expand Down Expand Up @@ -903,34 +887,6 @@ func TestStackCompositionMatches(t *testing.T) {
}
}

func TestFindRemoteStackForPR(t *testing.T) {
mock := &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 1, PullRequests: []int{10, 11}},
{ID: 2, PullRequests: []int{20, 21, 22}},
}, nil
},
}

// Found in first stack
rs, err := findRemoteStackForPR(mock, 11)
require.NoError(t, err)
require.NotNil(t, rs)
assert.Equal(t, 1, rs.ID)

// Found in second stack
rs, err = findRemoteStackForPR(mock, 21)
require.NoError(t, err)
require.NotNil(t, rs)
assert.Equal(t, 2, rs.ID)

// Not found
rs, err = findRemoteStackForPR(mock, 99)
require.NoError(t, err)
assert.Nil(t, rs)
}

func TestCheckout_ByPRURL_Local(t *testing.T) {
// When a PR URL resolves to a locally tracked stack, no API call needed
gitDir := t.TempDir()
Expand Down Expand Up @@ -973,14 +929,14 @@ func TestCheckout_ByPRURL_Remote(t *testing.T) {
}

restore := git.SetOps(&git.MockOps{
GitDirFn: func() (string, error) { return gitDir, nil },
CurrentBranchFn: func() (string, error) { return "main", nil },
BranchExistsFn: func(name string) bool { return name == "main" },
FetchFn: func(string) error { return nil },
CreateBranchFn: func(string, string) error { return nil },
GitDirFn: func() (string, error) { return gitDir, nil },
CurrentBranchFn: func() (string, error) { return "main", nil },
BranchExistsFn: func(name string) bool { return name == "main" },
FetchFn: func(string) error { return nil },
CreateBranchFn: func(string, string) error { return nil },
SetUpstreamTrackingFn: func(string, string) error { return nil },
RevParseFn: func(string) (string, error) { return "abc123", nil },
ResolveRemoteFn: func(string) (string, error) { return "origin", nil },
RevParseFn: func(string) (string, error) { return "abc123", nil },
ResolveRemoteFn: func(string) (string, error) { return "origin", nil },
CheckoutBranchFn: func(name string) error {
checkedOut = name
return nil
Expand All @@ -993,10 +949,8 @@ func TestCheckout_ByPRURL_Remote(t *testing.T) {

cfg, outR, errR := config.NewTestConfig()
cfg.GitHubClientOverride = &github.MockClient{
ListStacksFn: func() ([]github.RemoteStack, error) {
return []github.RemoteStack{
{ID: 1, PullRequests: []int{10, 11}},
}, nil
FindStackForPRFn: func(int) (*github.RemoteStack, error) {
return &github.RemoteStack{ID: 1, Number: 1, PullRequests: []int{10, 11}}, nil
},
FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
if pr, ok := prDB[n]; ok {
Expand Down
Loading
Loading