diff --git a/README.md b/README.md index 8cd3ee7..5efeed3 100644 --- a/README.md +++ b/README.md @@ -514,7 +514,7 @@ With no argument, the current active local stack is used. Pass a stack number to In an interactive terminal, a short wizard walks you through choosing which PRs to merge, picking the merge method, and confirming. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. -Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Bypassing merge requirements is not supported** for stacked PR merges. +Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. A closed pull request blocks itself and everything above it; reopen it, or drop or fold it with `gh stack modify` and then run `gh stack submit` to recreate the stack on GitHub. **Bypassing merge requirements is not supported** for stacked PR merges. If the base branch uses a merge queue, the stack is added to the queue instead of merging directly. The queue chooses the merge method, so the wizard skips the method step and any `--merge-method` (or `--squash`/`--rebase`/`--merge`) flag is ignored with a warning. The selected pull requests are added to the queue together but merge as the queue processes them — they may land in separate groups rather than all at once. diff --git a/cmd/merge.go b/cmd/merge.go index e7957d1..8201a46 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -188,6 +188,7 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { top := candidates[len(candidates)-1].Number cfg.Errorf("cannot merge the whole stack: pull request #%d is %s", blocker.Number, blockerState(blocker)) cfg.Printf("Merge up to #%d with `%s`", top, cfg.ColorAccent(fmt.Sprintf("gh stack merge %d", top))) + hintClosedBlocker(cfg, blocker) return ErrInvalidArgs } targetPR = candidates[len(candidates)-1].Number @@ -543,6 +544,7 @@ func explainNothingToMerge(cfg *config.Config, rs *github.RemoteStack, blocker * } if blocker != nil { cfg.Errorf("nothing to merge: pull request #%d is %s", blocker.Number, blockerState(blocker)) + hintClosedBlocker(cfg, blocker) return ErrNotInStack } cfg.Errorf("this stack has no open pull requests to merge") @@ -563,9 +565,11 @@ func explainNonMergeableTarget(cfg *config.Config, rs *github.RemoteStack, prNum return ErrInvalidArgs case pr.State == "closed": cfg.Errorf("pull request #%d is closed", prNumber) + hintClosedBlocker(cfg, pr) return ErrInvalidArgs case blocker != nil: cfg.Errorf("pull request #%d cannot be merged yet: #%d below it is %s", prNumber, blocker.Number, blockerState(blocker)) + hintClosedBlocker(cfg, blocker) return ErrInvalidArgs default: cfg.Errorf("pull request #%d cannot be merged", prNumber) @@ -682,6 +686,14 @@ func blockerState(pr *github.RemoteStackPR) string { return "not mergeable" } +// hintClosedBlocker points to the recovery path when the blocker is closed. +func hintClosedBlocker(cfg *config.Config, blocker *github.RemoteStackPR) { + // Drafts do not need a hint because marking them ready unblocks the merge. + if blocker != nil && blocker.State == "closed" && !blocker.IsMerged() { + reportClosedPRHint(cfg, blocker.Number) + } +} + func shortMergeSHA(sha string) string { if len(sha) > 7 { return sha[:7] diff --git a/cmd/merge_test.go b/cmd/merge_test.go index 61a2166..b57c9de 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -439,9 +439,74 @@ func TestRunMerge_WholeStackBlockedByDraft(t *testing.T) { assert.Contains(t, output, "cannot merge the whole stack") assert.Contains(t, output, "#2 is a draft") assert.Contains(t, output, "gh stack merge 1") + assert.NotContains(t, output, "gh stack modify", "drafts are unblocked by marking them ready") assert.False(t, submitCalled, "must not silently merge only the portion below the blocker") } +// runMergeWithClosedPR runs merge against stack #5 and checks that it fails +// with the given message and the closed-PR recovery hint for hintPR. +func runMergeWithClosedPR(t *testing.T, args []string, prs []github.RemoteStackPR, wantErr error, wantMsg, hintPR string) { + t.Helper() + defer mockRemoteOnlyGit()() + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + if n != 5 { + return nil, notFoundErr() + } + return remoteStack(5, "main", prs...), nil + }, + FindStackForPRFn: func(int) (*github.RemoteStack, error) { return remoteStack(5, "main", prs...), nil }, + RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { + return &github.RepoMergeConfig{MergeAllowed: true, DefaultMethod: "merge"}, nil + }, + MergeStackAsyncFn: func(int, string, string) (*github.AsyncMergeResult, error) { + t.Fatal("must not merge past a closed pull request") + return nil, nil + }, + } + + err := runMerge(cfg, fastOptions(), args) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, wantErr) + assert.Contains(t, output, wantMsg) + assert.Contains(t, output, "Reopen "+hintPR+", or run `gh stack modify` to drop it") + assert.Contains(t, output, "gh stack submit") +} + +func TestRunMerge_ClosedBottomBlocksWholeStack(t *testing.T) { + runMergeWithClosedPR(t, []string{"5"}, + []github.RemoteStackPR{closedStackPR(1, "b1"), openStackPR(2, "b2")}, + ErrNotInStack, "nothing to merge: pull request #1 is closed", "#1") +} + +func TestRunMerge_ClosedPRBelowTarget(t *testing.T) { + runMergeWithClosedPR(t, []string{"2"}, + []github.RemoteStackPR{closedStackPR(1, "b1"), openStackPR(2, "b2")}, + ErrInvalidArgs, "#1 below it is closed", "#1") +} + +func TestRunMerge_ClosedTarget(t *testing.T) { + runMergeWithClosedPR(t, []string{"1"}, + []github.RemoteStackPR{closedStackPR(1, "b1"), openStackPR(2, "b2")}, + ErrInvalidArgs, "pull request #1 is closed", "#1") +} + +// TestRunMerge_ClosedTargetAboveDraft verifies that the hint names the closed +// target rather than being suppressed by a draft below it. +func TestRunMerge_ClosedTargetAboveDraft(t *testing.T) { + runMergeWithClosedPR(t, []string{"2"}, + []github.RemoteStackPR{draftStackPR(1, "b1"), closedStackPR(2, "b2")}, + ErrInvalidArgs, "pull request #2 is closed", "#2") +} + +func TestRunMerge_ClosedMiddleWithYes(t *testing.T) { + runMergeWithClosedPR(t, []string{"5"}, + []github.RemoteStackPR{openStackPR(1, "b1"), closedStackPR(2, "b2"), openStackPR(3, "b3")}, + ErrInvalidArgs, "cannot merge the whole stack: pull request #2 is closed", "#2") +} + func TestRunMerge_NothingToMerge_AllMerged(t *testing.T) { setupLocalStack(t, 100, "b1", "b1", "b2") cfg, outR, errR := config.NewTestConfig() diff --git a/cmd/sync.go b/cmd/sync.go index b5ece46..6f5b516 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -338,23 +338,59 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // --- Step 5: Sync PR state --- cfg.Printf("") cfg.Printf("Syncing PRs ...") - _ = syncStackPRs(cfg, s) + prs := syncStackPRs(cfg, s) // Report PR status for each branch + closedPR := 0 + closedHasAbove := false + // mergeableBelow holds the contiguous bottom run of open, ready PRs, which + // can still be merged when a closed PR sits above them. + var mergeableBelow []int + prefixOpen := true for _, b := range s.Branches { if b.IsMerged() { continue } + if closedPR != 0 { + closedHasAbove = true + } if b.IsQueued() { cfg.Successf("PR %s (%s) — Queued", cfg.PRLink(b.PullRequest.Number, b.PullRequest.URL), b.Branch) continue } if b.PullRequest != nil { + // The stack on GitHub keeps closed PRs, which blocks merging it. + d := prs[b.Branch] + if d != nil && d.State == "CLOSED" { + cfg.Warningf("PR %s (%s) — Closed", cfg.PRLink(b.PullRequest.Number, b.PullRequest.URL), b.Branch) + if closedPR == 0 { + closedPR = b.PullRequest.Number + } + prefixOpen = false + continue + } + if prefixOpen && d != nil && !d.IsDraft { + mergeableBelow = append(mergeableBelow, b.PullRequest.Number) + } else { + prefixOpen = false + } cfg.Successf("PR %s (%s) — Open", cfg.PRLink(b.PullRequest.Number, b.PullRequest.URL), b.Branch) } else { + prefixOpen = false cfg.Warningf("%s has no PR", b.Branch) } } + if closedPR != 0 { + switch { + case len(mergeableBelow) == 0: + cfg.Warningf("#%d is closed and blocks merging the stack", closedPR) + case closedHasAbove: + cfg.Warningf("#%d is closed and blocks merging it and the PRs above it; %s can still be merged", closedPR, prNumberList(mergeableBelow)) + default: + cfg.Warningf("#%d is closed and blocks merging it; %s can still be merged", closedPR, prNumberList(mergeableBelow)) + } + reportClosedPRHint(cfg, closedPR) + } merged := s.MergedBranches() if len(merged) > 0 { names := make([]string, len(merged)) diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 063b7e9..d0a8096 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1921,6 +1921,69 @@ func TestSync_UpdatesPartialRemoteStack(t *testing.T) { assert.Equal(t, "9", sf.Stacks[0].ID) } +// runSyncWithClosedPR syncs a three-layer remote stack (#101-#103) in which +// the given PR is closed, and checks that it is reported as closed with the +// recovery hint. +func runSyncWithClosedPR(t *testing.T, closed int) string { + t.Helper() + t.Setenv("GH_STACK_HYPERLINKS", "0") + 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}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 103}}, + }, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + remote := &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}} + ghMock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{*remote}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { return remote, nil }, + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + state := "OPEN" + if n == closed { + state = "CLOSED" + } + return &github.PullRequest{Number: n, HeadRefName: fmt.Sprintf("b%d", n-100), State: state, URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n)}, nil + }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b3"), ghMock) + + closedBranch := fmt.Sprintf("b%d", closed-100) + assert.Contains(t, output, "("+closedBranch+") — Closed") + assert.NotContains(t, output, "("+closedBranch+") — Open") + assert.Contains(t, output, fmt.Sprintf("Reopen #%d, or run `gh stack modify` to drop it", closed)) + assert.Contains(t, output, "gh stack submit") + return output +} + +// TestSync_ClosedBottomPR_BlocksStack verifies that a closed bottom PR is +// reported as blocking the whole stack. +func TestSync_ClosedBottomPR_BlocksStack(t *testing.T) { + output := runSyncWithClosedPR(t, 101) + assert.Contains(t, output, "#101 is closed and blocks merging the stack") +} + +// TestSync_ClosedMiddlePR_ListsMergeableBelow verifies that a closed middle PR +// blocks itself and the PRs above it while the PRs below can still merge. +func TestSync_ClosedMiddlePR_ListsMergeableBelow(t *testing.T) { + output := runSyncWithClosedPR(t, 102) + assert.Contains(t, output, "#102 is closed and blocks merging it and the PRs above it; #101 can still be merged") +} + +// TestSync_ClosedTopPR_ListsMergeableBelow verifies that a closed top PR only +// blocks itself. +func TestSync_ClosedTopPR_ListsMergeableBelow(t *testing.T) { + output := runSyncWithClosedPR(t, 103) + assert.Contains(t, output, "#103 is closed and blocks merging it; #101, #102 can still be merged") +} + // TestSync_FewerThanTwoPRs_BranchesSynced verifies that with only one open PR // (no stack is possible), sync skips all stack API calls and reports // "Branches synced" rather than "Stack synced". diff --git a/cmd/utils.go b/cmd/utils.go index db43aa9..cde9789 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -665,6 +665,15 @@ func syncStackPRs(cfg *config.Config, s *stack.Stack) map[string]*github.PRDetai return details } +// reportClosedPRHint explains how to recover a stack that a closed pull request +// blocks. Reopening keeps the stack intact. Otherwise, because a stacked PR's +// base cannot be changed in place, the layer has to leave the stack and the +// stack has to be recreated on GitHub. +func reportClosedPRHint(cfg *config.Config, number int) { + cfg.Printf(" Reopen #%d, or run `%s` to drop it from the stack (or fold it to keep its changes), then run `%s` to recreate the stack on GitHub", + number, cfg.ColorAccent("gh stack modify"), cfg.ColorAccent("gh stack submit")) +} + // maxAPIConcurrency limits the number of concurrent API calls to avoid hitting secondary rate limits. const maxAPIConcurrency = 6 diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index acc8ec1..b6e4b68 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -576,7 +576,7 @@ With no argument, the current active local stack is used. Pass a stack number to In an interactive terminal, a short wizard walks you through choosing which PRs to merge, picking the merge method, and confirming. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. -Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Bypassing merge requirements is not supported** for stacked PR merges. +Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. A closed pull request blocks itself and everything above it; reopen it, or drop or fold it with `gh stack modify` and then run `gh stack submit` to recreate the stack on GitHub. **Bypassing merge requirements is not supported** for stacked PR merges. If the base branch uses a merge queue, the stack is added to the queue instead of merging directly. The queue chooses the merge method, so the wizard skips the method step and any `--merge-method` (or `--squash`/`--rebase`/`--merge`) flag is ignored with a warning. The selected pull requests are added to the queue together but merge as the queue processes them — they may land in separate groups rather than all at once.