Skip to content

Commit 0fd5340

Browse files
wiseemily88wiseemily88
committed
unstack: preflight PR eligibility before delete and improve API errors.Block unstack delete only when all PRs in the stack are ineligible
1 parent 84d160b commit 0fd5340

2 files changed

Lines changed: 253 additions & 2 deletions

File tree

‎cmd/unstack.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@ package cmd
22

33
import (
44
"errors"
5+
"fmt"
56

67
"github.com/cli/go-gh/v2/pkg/api"
78
"github.com/github/gh-stack/internal/config"
9+
"github.com/github/gh-stack/internal/github"
810
"github.com/github/gh-stack/internal/modify"
911
"github.com/github/gh-stack/internal/stack"
1012
"github.com/spf13/cobra"
@@ -64,13 +66,27 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error {
6466
cfg.Errorf("failed to create GitHub client: %s", err)
6567
return ErrAPIFailure
6668
}
69+
70+
blocked, err := shouldBlockUnstackDelete(cfg, client, s)
71+
if err != nil {
72+
cfg.Errorf("failed to check pull request states before unstack: %s", err)
73+
return ErrAPIFailure
74+
}
75+
if blocked {
76+
cfg.Errorf("Unstacking not allowed. Pull requests that are queued for merge, have auto-merge enabled, or are already merged will remain in the stack.")
77+
return ErrInvalidArgs
78+
}
79+
6780
if err := client.DeleteStack(s.ID); err != nil {
6881
var httpErr *api.HTTPError
6982
if errors.As(err, &httpErr) {
7083
switch httpErr.StatusCode {
7184
case 404:
7285
// Stack already deleted on GitHub — treat as success.
7386
cfg.Warningf("Stack not found on GitHub — continuing with local unstack")
87+
case 422:
88+
cfg.Errorf("Cannot delete stack on GitHub: %s", httpErr.Message)
89+
return ErrAPIFailure
7490
default:
7591
cfg.Errorf("Failed to delete stack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message)
7692
return ErrAPIFailure
@@ -101,3 +117,55 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error {
101117

102118
return nil
103119
}
120+
121+
func shouldBlockUnstackDelete(cfg *config.Config, client github.ClientOps, s *stack.Stack) (bool, error) {
122+
if s == nil || len(s.Branches) == 0 {
123+
return false, nil
124+
}
125+
126+
eligible := 0
127+
ineligible := 0
128+
for _, b := range s.Branches {
129+
// Respect stored merged status when available in local stack metadata.
130+
if b.PullRequest != nil && b.PullRequest.Merged {
131+
ineligible++
132+
continue
133+
}
134+
135+
var (
136+
pr *github.PullRequest
137+
err error
138+
)
139+
140+
if b.PullRequest != nil && b.PullRequest.Number > 0 {
141+
pr, err = client.FindPRByNumber(b.PullRequest.Number)
142+
if err != nil {
143+
return false, fmt.Errorf("checking PR #%d for branch %s: %w", b.PullRequest.Number, b.Branch, err)
144+
}
145+
} else {
146+
pr, err = client.FindPRForBranch(b.Branch)
147+
if err != nil {
148+
return false, fmt.Errorf("checking PR for branch %s: %w", b.Branch, err)
149+
}
150+
}
151+
152+
// If the PR no longer exists (or branch has no open PR), do not block unstacking.
153+
if pr == nil {
154+
eligible++
155+
continue
156+
}
157+
158+
switch {
159+
case pr.State == "MERGED":
160+
ineligible++
161+
case pr.IsQueued():
162+
ineligible++
163+
case pr.IsAutoMergeEnabled():
164+
ineligible++
165+
default:
166+
eligible++
167+
}
168+
}
169+
170+
return ineligible > 0 && eligible == 0, nil
171+
}

‎cmd/unstack_test.go‎

Lines changed: 185 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package cmd
22

33
import (
44
"encoding/json"
5+
"errors"
56
"os"
67
"path/filepath"
78
"testing"
@@ -137,7 +138,10 @@ func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) {
137138
writeStackFile(t, gitDir, stack.Stack{
138139
ID: "99",
139140
Trunk: stack.BranchRef{Branch: "main"},
140-
Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}},
141+
Branches: []stack.BranchRef{
142+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}},
143+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
144+
},
141145
})
142146

143147
cfg, outR, errR := config.NewTestConfig()
@@ -170,7 +174,10 @@ func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) {
170174
writeStackFile(t, gitDir, stack.Stack{
171175
ID: "99",
172176
Trunk: stack.BranchRef{Branch: "main"},
173-
Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}},
177+
Branches: []stack.BranchRef{
178+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}},
179+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
180+
},
174181
})
175182

176183
cfg, outR, errR := config.NewTestConfig()
@@ -226,3 +233,179 @@ func TestUnstack_RemovesCorrectStackByPointer(t *testing.T) {
226233
require.Len(t, sf.Stacks, 1, "should remove exactly one stack")
227234
assert.Equal(t, []string{"b1", "b2"}, sf.Stacks[0].BranchNames(), "should keep the OTHER stack intact")
228235
}
236+
237+
func TestUnstack_PreflightBlocksDelete_WhenAllPRsIneligible(t *testing.T) {
238+
gitDir := t.TempDir()
239+
restore := git.SetOps(&git.MockOps{
240+
GitDirFn: func() (string, error) { return gitDir, nil },
241+
CurrentBranchFn: func() (string, error) { return "b1", nil },
242+
})
243+
defer restore()
244+
245+
writeStackFile(t, gitDir, stack.Stack{
246+
ID: "99",
247+
Trunk: stack.BranchRef{Branch: "main"},
248+
Branches: []stack.BranchRef{
249+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}},
250+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
251+
},
252+
})
253+
254+
deleteCalled := false
255+
cfg, outR, errR := config.NewTestConfig()
256+
cfg.GitHubClientOverride = &github.MockClient{
257+
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
258+
switch number {
259+
case 101:
260+
return &github.PullRequest{Number: 101, State: "MERGED"}, nil
261+
case 102:
262+
return &github.PullRequest{Number: 102, State: "OPEN", MergeQueueEntry: &github.MergeQueueEntry{ID: "MQE_1"}}, nil
263+
default:
264+
return nil, nil
265+
}
266+
},
267+
DeleteStackFn: func(stackID string) error {
268+
deleteCalled = true
269+
return nil
270+
},
271+
}
272+
273+
err := runUnstack(cfg, &unstackOptions{})
274+
output := collectOutput(cfg, outR, errR)
275+
276+
assert.ErrorIs(t, err, ErrInvalidArgs)
277+
assert.False(t, deleteCalled, "DeleteStack should not be called when all PRs are ineligible")
278+
assert.Contains(t, output, "Unstacking not allowed")
279+
assert.NotContains(t, output, "Stack removed from local tracking")
280+
281+
sf, loadErr := stack.Load(gitDir)
282+
require.NoError(t, loadErr)
283+
require.Len(t, sf.Stacks, 1)
284+
}
285+
286+
func TestUnstack_PreflightAllowsDelete_WhenMixedEligibility(t *testing.T) {
287+
gitDir := t.TempDir()
288+
restore := git.SetOps(&git.MockOps{
289+
GitDirFn: func() (string, error) { return gitDir, nil },
290+
CurrentBranchFn: func() (string, error) { return "b1", nil },
291+
})
292+
defer restore()
293+
294+
writeStackFile(t, gitDir, stack.Stack{
295+
ID: "99",
296+
Trunk: stack.BranchRef{Branch: "main"},
297+
Branches: []stack.BranchRef{
298+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}},
299+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
300+
},
301+
})
302+
303+
deleteCalled := false
304+
cfg, outR, errR := config.NewTestConfig()
305+
cfg.GitHubClientOverride = &github.MockClient{
306+
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
307+
switch number {
308+
case 101:
309+
return &github.PullRequest{Number: 101, State: "MERGED"}, nil
310+
case 102:
311+
return &github.PullRequest{Number: 102, State: "OPEN"}, nil
312+
default:
313+
return nil, nil
314+
}
315+
},
316+
DeleteStackFn: func(stackID string) error {
317+
deleteCalled = true
318+
return nil
319+
},
320+
}
321+
322+
err := runUnstack(cfg, &unstackOptions{})
323+
output := collectOutput(cfg, outR, errR)
324+
325+
require.NoError(t, err)
326+
assert.True(t, deleteCalled, "DeleteStack should be called when at least one PR is eligible")
327+
assert.Contains(t, output, "Stack deleted on GitHub")
328+
assert.Contains(t, output, "Stack removed from local tracking")
329+
330+
sf, loadErr := stack.Load(gitDir)
331+
require.NoError(t, loadErr)
332+
assert.Empty(t, sf.Stacks)
333+
}
334+
335+
func TestUnstack_PreflightLookupFailure_StopsDeletion(t *testing.T) {
336+
gitDir := t.TempDir()
337+
restore := git.SetOps(&git.MockOps{
338+
GitDirFn: func() (string, error) { return gitDir, nil },
339+
CurrentBranchFn: func() (string, error) { return "b1", nil },
340+
})
341+
defer restore()
342+
343+
writeStackFile(t, gitDir, stack.Stack{
344+
ID: "99",
345+
Trunk: stack.BranchRef{Branch: "main"},
346+
Branches: []stack.BranchRef{{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}},
347+
})
348+
349+
deleteCalled := false
350+
cfg, outR, errR := config.NewTestConfig()
351+
cfg.GitHubClientOverride = &github.MockClient{
352+
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
353+
return nil, errors.New("graphql timeout")
354+
},
355+
DeleteStackFn: func(stackID string) error {
356+
deleteCalled = true
357+
return nil
358+
},
359+
}
360+
361+
err := runUnstack(cfg, &unstackOptions{})
362+
output := collectOutput(cfg, outR, errR)
363+
364+
assert.ErrorIs(t, err, ErrAPIFailure)
365+
assert.False(t, deleteCalled, "DeleteStack should not be called if preflight fails")
366+
assert.Contains(t, output, "failed to check pull request states before unstack")
367+
assert.NotContains(t, output, "Stack removed from local tracking")
368+
369+
sf, loadErr := stack.Load(gitDir)
370+
require.NoError(t, loadErr)
371+
require.Len(t, sf.Stacks, 1)
372+
}
373+
374+
func TestUnstack_API422_ShowsInformativeErrorAndStopsLocalDeletion(t *testing.T) {
375+
gitDir := t.TempDir()
376+
restore := git.SetOps(&git.MockOps{
377+
GitDirFn: func() (string, error) { return gitDir, nil },
378+
CurrentBranchFn: func() (string, error) { return "b1", nil },
379+
})
380+
defer restore()
381+
382+
writeStackFile(t, gitDir, stack.Stack{
383+
ID: "99",
384+
Trunk: stack.BranchRef{Branch: "main"},
385+
Branches: []stack.BranchRef{
386+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}},
387+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
388+
},
389+
})
390+
391+
cfg, outR, errR := config.NewTestConfig()
392+
cfg.GitHubClientOverride = &github.MockClient{
393+
FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
394+
return &github.PullRequest{Number: number, State: "OPEN"}, nil
395+
},
396+
DeleteStackFn: func(stackID string) error {
397+
return &api.HTTPError{StatusCode: 422, Message: "some pull requests cannot be removed from stack"}
398+
},
399+
}
400+
err := runUnstack(cfg, &unstackOptions{})
401+
output := collectOutput(cfg, outR, errR)
402+
403+
assert.ErrorIs(t, err, ErrAPIFailure)
404+
assert.Contains(t, output, "Cannot delete stack on GitHub")
405+
assert.Contains(t, output, "cannot be removed")
406+
assert.NotContains(t, output, "Stack removed from local tracking")
407+
408+
sf, loadErr := stack.Load(gitDir)
409+
require.NoError(t, loadErr)
410+
require.Len(t, sf.Stacks, 1)
411+
}

0 commit comments

Comments
 (0)