From 8dd0bdc729767167d4816307473390f631474613 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 11:29:16 -0400 Subject: [PATCH 1/2] Fork unmerged branches into a new stack when the base stack is fully merged Once every PR that is officially part of a stack on GitHub has been merged -- especially after the merged branches are deleted upstream -- you can no longer add to that stack. A new PR on top would target the trunk directly instead of chaining onto the merged PRs, so the remote stack's "each PR's base ref is the previous PR's head ref" invariant no longer holds. On the next `gh stack submit`, the stack update was rejected and surfaced as a confusing, dead-end warning: Failed to update stack on GitHub: Pull requests must form a stack, where each PR's base ref is the previous PR's head ref `submit` had no handling for this: `syncStack` always sent the full PR list (including the merged-and-deleted ones), so the API rejected the broken chain even though the new PRs had already been created with correct bases. Fork the survivors into a fresh stack instead of failing. After syncing PR state and before pushing, `runSubmit` now calls `maybeForkFromMergedBase`: - It triggers only when every PR officially part of the tracked remote stack (`s.ID`) has merged. Membership is read from the stacks API, so open PRs that are not part of the remote stack do not count, and -- this is the key guard -- a normal partial, bottom-up merge (where the remote stack still lists an open PR) is left completely untouched. A cheap pre-check (the local stack must have at least one merged branch) avoids an extra ListStacks call on the common path. - The local branches are partitioned: those still in the merged remote stack stay behind; everything else (new branches, plus open PRs that were never part of that remote stack) is lifted into a brand-new stack rooted at the original trunk, with an empty remote ID. The bottom survivor is re-based onto the trunk. - `runSubmit` continues with the new stack, so the push loop, PR creation, and `syncStack` all operate on it; the empty ID routes `syncStack` through the adopt/create path and a fresh stack is created on GitHub. - The original, fully merged stack is left untouched on GitHub. Locally it is kept as a record only if at least one of its branches still exists in the working copy; otherwise it is dropped. No data is lost -- those PRs are already merged on GitHub. To restructure the stack file safely, add `StackFile.IndexOfStack`, which locates a stack by pointer identity so the fork can capture what it needs before `AddStack`/`RemoveStack` reallocate the underlying slice. Also soften the partial-merge case that does not fork: when an `UpdateStack` call fails with the "must form a stack" 422 and the stack still contains merged branches, report it as an informational note (the unmerged PRs were pushed and re-based onto the trunk) rather than a scary failure warning. Scope is limited to `submit`. `add` and `checkout` keep their existing "refuse and suggest `gh stack init`" behavior on fully merged stacks. Tests: - cmd/submit_test.go: TestSubmit_ForksWhenRemoteStackFullyMerged covers both disposition variants (the old stack is removed when its merged branches are gone locally, kept when they still exist) and asserts that only the new branches are pushed, the fork message is printed, a fresh stack is created, and the local stack file is split into two stacks. TestSubmit_NoForkWhenRemoteStackHasOpenPR verifies the everyday bottom-up merge is not forked and that the broken-chain 422 is reported calmly. TestUpdateStack_BrokenChainAfterMerge checks the calm-vs-warn branch. - internal/stack/stack_test.go: TestIndexOfStack covers identity lookup and the not-found case. Docs: README, the CLI reference, the stacked-PRs guide, the FAQ, and the agent SKILL.md note that submitting onto a fully merged stack starts a new stack rooted at the trunk. --- README.md | 2 + cmd/submit.go | 156 +++++++++++ cmd/submit_test.go | 275 ++++++++++++++++++++ docs/src/content/docs/faq.md | 4 + docs/src/content/docs/guides/stacked-prs.md | 2 + docs/src/content/docs/reference/cli.md | 2 + internal/stack/stack.go | 12 + internal/stack/stack_test.go | 20 ++ skills/gh-stack/SKILL.md | 1 + 9 files changed, 474 insertions(+) diff --git a/README.md b/README.md index 70f0fba..35b8494 100644 --- a/README.md +++ b/README.md @@ -372,6 +372,8 @@ Creates a Stacked PR for every branch in the stack, pushing branches to the remo After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs will be added to the top of the stack. +If every PR in the stack has already been merged, that stack is complete and can't be extended — a new PR on top would target the trunk directly rather than chaining onto the merged PRs. In that case `submit` automatically starts a **new** stack rooted at the trunk for your unmerged branches and creates it on GitHub, leaving the merged stack untouched. + In an interactive terminal, `submit` opens a full-screen, mouse- and keyboard-driven editor on a single screen. Every branch without a PR is included by default — deselect any you don't want on the left panel (Ctrl+X). Because each PR builds on the branch below it, deselecting a branch also deselects the ones stacked above it, and re-including a branch re-includes the ones below it. Draft each PR's title, description (with a markdown preview and `$EDITOR` escape), and choose ready-for-review or draft on the right, then submit them all at once with Ctrl+S. Pass `--auto` (or run in CI) to skip the editor and use auto-generated titles. In the editor, new PRs default to ready for review; flip any PR to draft with the ready ↔ draft toggle. With `--auto`, new PRs are created as drafts unless you pass `--open`. diff --git a/cmd/submit.go b/cmd/submit.go index a8c8b0b..7daf067 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -143,6 +143,13 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { // Sync PR state to detect merged/queued PRs before pushing. prDetails := syncStackPRs(cfg, s) + // If the active branches now sit on top of a fully-merged base, they can no + // longer extend the existing remote stack. Fork them into a fresh stack + // rooted at the trunk and continue the submit with that new stack. + if stacksAvailable { + s = maybeForkFromMergedBase(cfg, client, sf, s, gitDir) + } + // Resolve remote for pushing remote, err := pickRemote(cfg, currentBranch, opts.remote) if err != nil { @@ -477,6 +484,144 @@ func humanize(s string) string { }, s) } +// maybeForkFromMergedBase detects when every PR that is officially part of the +// stack on GitHub has already been merged, and forks the remaining local +// branches (new branches, or open PRs that were never part of that remote stack) +// into a brand-new stack rooted at the trunk. +// +// Once all of a stack's remote PRs are merged — especially after their branches +// are deleted upstream — you can no longer add to that stack: a new PR on top +// would target the trunk, breaking the remote stack's "each PR's base ref == +// previous PR's head ref" chain. Rather than failing the stack update on GitHub, +// we lift the survivors into a new local stack with no remote ID so the +// subsequent submit creates a fresh stack on GitHub. The original (fully merged) +// remote stack is left untouched on GitHub. +// +// It returns the stack submit should continue with: the new forked stack when a +// fork happens, or the original stack otherwise. +func maybeForkFromMergedBase(cfg *config.Config, client github.ClientOps, sf *stack.StackFile, s *stack.Stack, gitDir string) *stack.Stack { + // Only meaningful when there is a tracked remote stack to evaluate. A fork + // can only happen if every remote-stack PR is merged, which implies at least + // one locally tracked branch is merged — checking that first avoids an extra + // ListStacks call on the common path. + if s.ID == "" || len(s.MergedBranches()) == 0 { + return s + } + + remotePRs := remoteStackPRs(client, s.ID) + if len(remotePRs) == 0 { + return s + } + + // Every PR officially in the remote stack must be merged. Open PRs that are + // not part of the remote stack do not count. + merged := mergedPRNumbers(s) + for _, n := range remotePRs { + if !merged[n] { + return s // a remote-stack PR is still open — not a fork situation + } + } + + stackIdx := sf.IndexOfStack(s) + if stackIdx < 0 { + return s + } + + // Partition the local branches: those that are part of the merged remote + // stack stay behind; everything else (new branches and open PRs that were + // never part of the remote stack) is forked into a new stack. + remoteSet := make(map[int]bool, len(remotePRs)) + for _, n := range remotePRs { + remoteSet[n] = true + } + var keepBranches, forkBranches []stack.BranchRef + for _, b := range s.Branches { + if b.PullRequest != nil && remoteSet[b.PullRequest.Number] { + keepBranches = append(keepBranches, b) + } else { + forkBranches = append(forkBranches, b) + } + } + if len(forkBranches) == 0 { + return s // nothing new to fork — the whole stack is merged and done + } + + // Capture trunk/prefix before mutating sf.Stacks (RemoveStack/AddStack can + // reallocate the slice and invalidate the s pointer). + trunk := s.Trunk + prefix := s.Prefix + numbered := s.Numbered + + // The bottom surviving branch re-bases onto the trunk. + if base, err := git.MergeBase(forkBranches[0].Branch, trunk.Branch); err == nil { + forkBranches[0].Base = base + } + + cfg.Warningf("Every PR in this stack has already been merged on GitHub") + cfg.Printf("Adding to a fully merged stack isn't supported — starting a new stack for your %d unmerged %s based on %s", + len(forkBranches), plural(len(forkBranches), "branch", "branches"), cfg.ColorCyan(trunk.Branch)) + + // Decide the fate of the original (fully merged) stack: keep it as a record + // only if at least one of its branches still exists locally; otherwise drop + // it. The merged stack is left intact on GitHub either way. + removeOld := true + for _, b := range keepBranches { + if git.BranchExists(b.Branch) { + removeOld = false + break + } + } + + if removeOld { + sf.RemoveStack(stackIdx) + } else { + sf.Stacks[stackIdx].Branches = keepBranches + } + + sf.AddStack(stack.Stack{ + Prefix: prefix, + Numbered: numbered, + Trunk: trunk, + Branches: forkBranches, + }) + + if err := stack.Save(gitDir, sf); err != nil { + // Persisting the split failed, but the in-memory model is correct; + // surface the error and continue so the PRs still get submitted. + _ = handleSaveError(cfg, err) + } + + return &sf.Stacks[len(sf.Stacks)-1] +} + +// remoteStackPRs returns the PR numbers that are officially part of the remote +// stack identified by stackID, or nil if it can't be determined. +func remoteStackPRs(client github.ClientOps, stackID string) []int { + stacks, err := client.ListStacks() + if err != nil { + return nil + } + for _, rs := range stacks { + if strconv.Itoa(rs.ID) == stackID { + return rs.PullRequests + } + } + return nil +} + +// mergedPRNumbers returns the set of PR numbers whose local branch is marked +// merged. Call after syncStackPRs so the merge flags reflect the remote state. +func mergedPRNumbers(s *stack.Stack) map[int]bool { + merged := make(map[int]bool) + for i := range s.Branches { + b := &s.Branches[i] + if b.IsMerged() && b.PullRequest != nil { + merged[b.PullRequest.Number] = true + } + } + return merged +} + // handlePendingModify handles the stack recreation after a modify operation. // It deletes the old remote stack and clears s.ID so syncStack creates a new // one. The state file is NOT cleared here — it is cleared after syncStack @@ -665,6 +810,17 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // immediately try to re-create it. s.ID = "" createNewStack(cfg, client, s, prNumbers) + case 422: + // A merged branch whose ref has been deleted upstream breaks the + // stack's base→head chain, so the update is rejected. This is + // expected once part of the stack has landed; the unmerged PRs + // were still pushed and re-based, so explain it calmly rather + // than alarming the user with a raw API error. + if strings.Contains(httpErr.Message, "must form a stack") && len(s.MergedBranches()) > 0 { + cfg.Infof("Merged PRs have left the stack on GitHub, so it wasn't updated — your unmerged PRs were pushed and re-based onto the trunk") + return + } + cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message) default: cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message) } diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 4676bed..67058b3 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -395,6 +395,281 @@ func TestSubmit_SkipsMergedBranches(t *testing.T) { assert.Equal(t, []string{"b2"}, pushCalls[0].branches) } +// TestSubmit_ForksWhenRemoteStackFullyMerged covers the case where every PR +// officially part of the stack on GitHub has merged and the user has added new +// branches on top. Submit should lift the new branches into a fresh stack rooted +// at the trunk and create a new stack on GitHub, leaving the merged stack alone. +func TestSubmit_ForksWhenRemoteStackFullyMerged(t *testing.T) { + tests := []struct { + name string + branchesExist bool // do the merged branches still exist locally? + wantStackCount int + }{ + {name: "removes old stack when merged branches are gone", branchesExist: false, wantStackCount: 1}, + {name: "keeps old stack when merged branches still exist", branchesExist: true, wantStackCount: 2}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := stack.Stack{ + ID: "42", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2, Merged: true}}, + {Branch: "b3"}, + {Branch: "b4"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var pushCalls []pushCall + var createdPRs []string + var createStackPRs []int + + mock := newSubmitMock(tmpDir, "b4") + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + mock.LogRangeFn = func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{{Subject: "commit for " + head}}, nil + } + mock.MergeBaseFn = func(a, b string) (string, error) { return "basesha", nil } + mock.RevParseFn = func(ref string) (string, error) { return "sha-" + ref, nil } + mock.BranchExistsFn = func(string) bool { return tt.branchesExist } + restore := git.SetOps(mock) + defer restore() + + prCounter := 100 + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 42, PullRequests: []int{1, 2}}}, nil + }, + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + switch n { + case 1: + return &github.PullRequest{Number: 1, HeadRefName: "b1", State: "MERGED", Merged: true}, nil + case 2: + return &github.PullRequest{Number: 2, HeadRefName: "b2", State: "MERGED", Merged: true}, nil + } + return &github.PullRequest{Number: n, State: "OPEN"}, nil + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + CreatePRFn: func(base, head, title, body string, draft bool) (*github.PullRequest, error) { + createdPRs = append(createdPRs, head) + prCounter++ + return &github.PullRequest{ + Number: prCounter, + ID: fmt.Sprintf("PR_%d", prCounter), + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", prCounter), + HeadRefName: head, + }, nil + }, + CreateStackFn: func(prNumbers []int) (int, error) { + createStackPRs = prNumbers + return 99, nil + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + require.NoError(t, err) + + // Only the new branches are pushed; merged ones are left behind. + require.Len(t, pushCalls, 2) + assert.Equal(t, []string{"b3"}, pushCalls[0].branches) + assert.Equal(t, []string{"b4"}, pushCalls[1].branches) + + // Fork messaging. + assert.Contains(t, output, "Every PR in this stack has already been merged") + assert.Contains(t, output, "starting a new stack") + + // PRs are created for the new branches and grouped into a new stack. + assert.Equal(t, []string{"b3", "b4"}, createdPRs) + assert.Equal(t, []int{101, 102}, createStackPRs) + + // The local stack file is split: the new branches form their own + // stack rooted at the trunk with the freshly created remote ID. + reloaded, err := stack.Load(tmpDir) + require.NoError(t, err) + require.Len(t, reloaded.Stacks, tt.wantStackCount) + + forked := reloaded.FindAllStacksForBranch("b4") + require.Len(t, forked, 1) + assert.Equal(t, []string{"b3", "b4"}, forked[0].BranchNames()) + assert.Equal(t, "99", forked[0].ID) + assert.Equal(t, "main", forked[0].Trunk.Branch) + + oldStack := reloaded.FindAllStacksForBranch("b1") + if tt.branchesExist { + require.Len(t, oldStack, 1) + assert.Equal(t, []string{"b1", "b2"}, oldStack[0].BranchNames()) + assert.Equal(t, "42", oldStack[0].ID) + } else { + assert.Empty(t, oldStack) + } + }) + } +} + +// TestSubmit_NoForkWhenRemoteStackHasOpenPR verifies that a normal partially +// merged stack (the remote stack still has an open PR) is NOT forked — that is +// the everyday bottom-up merge flow and must keep working as before. +func TestSubmit_NoForkWhenRemoteStackHasOpenPR(t *testing.T) { + s := stack.Stack{ + ID: "42", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2, Merged: true}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 3}}, + {Branch: "b4"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var pushCalls []pushCall + + mock := newSubmitMock(tmpDir, "b4") + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + mock.LogRangeFn = func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{{Subject: "commit for " + head}}, nil + } + mock.MergeBaseFn = func(a, b string) (string, error) { return "basesha", nil } + mock.RevParseFn = func(ref string) (string, error) { return "sha-" + ref, nil } + mock.BranchExistsFn = func(string) bool { return true } + restore := git.SetOps(mock) + defer restore() + + prCounter := 100 + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 42, PullRequests: []int{1, 2, 3}}}, nil + }, + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + switch n { + case 1: + return &github.PullRequest{Number: 1, HeadRefName: "b1", State: "MERGED", Merged: true}, nil + case 2: + return &github.PullRequest{Number: 2, HeadRefName: "b2", State: "MERGED", Merged: true}, nil + case 3: + return &github.PullRequest{Number: 3, HeadRefName: "b3", State: "OPEN"}, nil + } + return &github.PullRequest{Number: n, State: "OPEN"}, nil + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + CreatePRFn: func(base, head, title, body string, draft bool) (*github.PullRequest, error) { + prCounter++ + return &github.PullRequest{ + Number: prCounter, + ID: fmt.Sprintf("PR_%d", prCounter), + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", prCounter), + HeadRefName: head, + }, nil + }, + UpdateStackFn: func(string, []int) error { + // Merged-and-deleted base branches break the chain on GitHub. + return &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/42"}, + } + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + require.NoError(t, err) + + // No fork happened. + assert.NotContains(t, output, "starting a new stack") + // The broken-chain update is explained calmly, not as a scary failure. + assert.Contains(t, output, "Merged PRs have left the stack") + assert.NotContains(t, output, "Failed to update stack") + + // The local stack file is untouched: still a single stack with all branches. + reloaded, err := stack.Load(tmpDir) + require.NoError(t, err) + require.Len(t, reloaded.Stacks, 1) + assert.Equal(t, []string{"b1", "b2", "b3", "b4"}, reloaded.Stacks[0].BranchNames()) +} + +// TestUpdateStack_BrokenChainAfterMerge verifies the "must form a stack" 422 is +// reported calmly when merged branches are present, but still warns otherwise. +func TestUpdateStack_BrokenChainAfterMerge(t *testing.T) { + mustFormErr := func() error { + return &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/42"}, + } + } + + t.Run("merged branches present is reported calmly", func(t *testing.T) { + s := &stack.Stack{ + ID: "42", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 3}}, + }, + } + mock := &github.MockClient{UpdateStackFn: func(string, []int) error { return mustFormErr() }} + cfg, _, errR := config.NewTestConfig() + updateStack(cfg, mock, s, []int{1, 2, 3}) + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + assert.Contains(t, output, "Merged PRs have left the stack") + assert.NotContains(t, output, "Failed to update stack") + }) + + t.Run("no merged branches still warns", func(t *testing.T) { + s := &stack.Stack{ + ID: "42", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2}}, + }, + } + mock := &github.MockClient{UpdateStackFn: func(string, []int) error { return mustFormErr() }} + cfg, _, errR := config.NewTestConfig() + updateStack(cfg, mock, s, []int{1, 2}) + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + assert.Contains(t, output, "Failed to update stack") + }) +} + func TestSubmit_DefaultPRTitleBody(t *testing.T) { t.Run("single_commit", func(t *testing.T) { restore := git.SetOps(&git.MockOps{ diff --git a/docs/src/content/docs/faq.md b/docs/src/content/docs/faq.md index e047da1..fc18c1e 100644 --- a/docs/src/content/docs/faq.md +++ b/docs/src/content/docs/faq.md @@ -187,6 +187,10 @@ This applies whether or not a merge queue is enabled. With a merge queue, the sa Yes, partial stack merges are supported. After the merge, the lowest unmerged PR is updated to explicitly target the stack base (e.g., `main`). A cascading rebase is also automatically run to rebase the remaining unmerged branches. +### What happens if I add a new PR after the whole stack has merged? + +Once every PR in a stack has merged, the stack is complete and can't be extended — a new PR on top would target the trunk directly, which no longer chains onto the merged PRs. When you run `gh stack submit` with new branches on top of a fully merged stack, the CLI automatically starts a **new** stack rooted at the trunk for those branches and creates it on GitHub. The original, fully merged stack is left untouched. + ### What happens if you close a PR in the middle of the stack? Closing a PR in the middle of the stack will block all PRs above it from being mergeable. The stack relationship is preserved, so if you want to open a different PR or modify the stack, you will need to unstack and then re-create the stack. diff --git a/docs/src/content/docs/guides/stacked-prs.md b/docs/src/content/docs/guides/stacked-prs.md index 1f92493..81e8eac 100644 --- a/docs/src/content/docs/guides/stacked-prs.md +++ b/docs/src/content/docs/guides/stacked-prs.md @@ -30,6 +30,8 @@ Stacks are merged **from the bottom up** — you can merge any number of PRs at 3. The next unmerged PR is now at the bottom and can be reviewed, approved, and merged. 4. Repeat until the entire stack is landed. +Once the entire stack has landed, it is complete and can't be extended. If you add new branches on top and run `gh stack submit`, the CLI automatically starts a **new** stack rooted at the trunk for those branches (a new PR on a fully merged stack would target the trunk directly rather than chaining onto the merged PRs). + For details on merge methods (squash, merge commit, rebase) and merge requirements, see [Merging Stacks](/gh-stack/introduction/overview/#merging-stacks) in the Overview. ## Pushing and Syncing from the CLI diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index a7f0c44..bd21671 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -269,6 +269,8 @@ gh stack submit [flags] Creates a Stacked PR for every branch in the stack, pushing branches to the remote. After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs are added to the existing stack. +If every PR in the stack has already been merged, that stack is complete and can't be extended. In that case `submit` automatically starts a **new** stack rooted at the trunk for your unmerged branches and creates it on GitHub, leaving the merged stack untouched. + In an interactive terminal, `submit` opens a full-screen editor on a single screen: - **Left panel** — every branch without a PR is **included by default**; deselect any you don't want to submit with Ctrl+X. Because each PR builds on the branch below it, deselecting a branch also deselects the ones stacked above it, and re-including a branch re-includes the ones below it that it depends on. Branches that already have a PR (open, draft, queued, or merged) are shown for context but are locked; edit those on the web. diff --git a/internal/stack/stack.go b/internal/stack/stack.go index 75ff71c..7e4043a 100644 --- a/internal/stack/stack.go +++ b/internal/stack/stack.go @@ -216,6 +216,18 @@ func (sf *StackFile) FindAllStacksForBranch(branch string) []*Stack { return stacks } +// IndexOfStack returns the index of the given stack within the file by identity +// (pointer), or -1 if it is not part of this file. Use it to locate a stack +// obtained from FindAllStacksForBranch before mutating the Stacks slice. +func (sf *StackFile) IndexOfStack(s *Stack) int { + for i := range sf.Stacks { + if &sf.Stacks[i] == s { + return i + } + } + return -1 +} + // FindStackByPRNumber returns the first stack and branch whose PR number matches. // Returns nil, nil if no match is found. func (sf *StackFile) FindStackByPRNumber(prNumber int) (*Stack, *BranchRef) { diff --git a/internal/stack/stack_test.go b/internal/stack/stack_test.go index 9d78610..987f25c 100644 --- a/internal/stack/stack_test.go +++ b/internal/stack/stack_test.go @@ -383,6 +383,26 @@ func TestRemoveStackForBranch(t *testing.T) { }) } +// --- IndexOfStack: locating a stack by identity --- + +func TestIndexOfStack(t *testing.T) { + sf := &StackFile{ + Stacks: []Stack{ + makeStack("main", "a1"), + makeStack("main", "b1"), + makeStack("main", "c1"), + }, + } + + assert.Equal(t, 0, sf.IndexOfStack(&sf.Stacks[0])) + assert.Equal(t, 2, sf.IndexOfStack(&sf.Stacks[2])) + + t.Run("not part of the file", func(t *testing.T) { + other := makeStack("main", "z1") + assert.Equal(t, -1, sf.IndexOfStack(&other)) + }) +} + // --- Queued state: transient merge queue support --- func makeQueuedBranch(name string, prNum int) BranchRef { diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 27f26d2..465d79b 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -547,6 +547,7 @@ gh stack submit --auto --open - Pushes all active (non-merged) branches atomically (`--force-with-lease --atomic`) - Creates a new PR for each branch that doesn't have one (base set to the first non-merged ancestor branch) - After creating PRs, links them together as a **Stack** on GitHub (requires the repository to have stacks enabled) +- If every PR in the stack has already been merged, the stack is complete and can't be extended. `submit` automatically forks your unmerged branches into a **new** stack rooted at the trunk and creates it on GitHub, leaving the merged stack untouched. - If stacks are not available (exit code 9), the repository does not have stacked PRs enabled. In interactive mode, `submit` offers to create regular (unstacked) PRs instead. In non-interactive mode, it exits with code 9. - Syncs PR metadata for branches that already have PRs From a9fa57451f0124e35a86740a2b143133115960c3 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 12:05:21 -0400 Subject: [PATCH 2/2] Handle fully merged stacks gracefully in the view and modify TUIs Merged branches (and their PRs) are not selectable, so once an entire stack has landed there is nothing to act on -- yet the TUIs did not reflect that: - `gh stack view` still drew a highlighted cursor on the top branch even though it could not be selected. Navigation, checkout, and the per-branch toggles all silently did nothing, with no indication of why. - `gh stack modify` opened its full editor on a stack with nothing left to restructure, instead of short-circuiting like `gh stack submit` does when there is nothing to submit. Reflect the "nothing actionable" state in both TUIs. View (internal/tui/stackview/model.go): - Hide the cursor when every branch is merged. `New` now starts the cursor at -1 and only lands it on the current or first non-merged branch; when none exists the cursor stays hidden, so no row is rendered as focused. The existing `m.cursor >= 0` guards and merged-skipping `moveCursor` already make every cursor action a no-op in that state, and mouse-wheel scrolling still works for tall merged stacks. - Dim the shortcuts that depend on the cursor. `buildHeaderConfig` marks navigate, commits, files, open PR, and checkout as `Disabled` (rendered gray via the existing ShortcutEntry.Disabled styling) when all branches are merged, leaving only `q quit` active. Modify (cmd/modify.go): - Short-circuit before opening the TUI. After preconditions pass and PR state is synced, `runModify` now returns early when the stack is fully merged, printing "All branches in this stack have been merged" and pointing at `gh stack init`, exiting cleanly (exit 0) like submit's "nothing to submit" path. The linearity and merge-queue precondition checks already skip merged branches, so they do not fire spuriously. Tests: - internal/tui/stackview/model_test.go: the cursor is hidden (-1) when all branches are merged; up/down/enter do not move it or trigger a checkout; View renders without panicking on a hidden cursor; buildHeaderConfig disables every cursor-dependent shortcut (and only those) when all merged, and leaves them all enabled when active branches remain. - cmd/modify_test.go: runModify short-circuits on a fully merged stack, printing the message and returning no error without launching the TUI. --- cmd/modify.go | 8 ++++ cmd/modify_test.go | 45 +++++++++++++++++++ internal/tui/stackview/model.go | 24 +++++----- internal/tui/stackview/model_test.go | 65 ++++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 10 deletions(-) diff --git a/cmd/modify.go b/cmd/modify.go index 3d992bb..9e42aab 100644 --- a/cmd/modify.go +++ b/cmd/modify.go @@ -74,6 +74,14 @@ func runModify(cfg *config.Config) error { s := result.Stack currentBranch := result.CurrentBranch + // A fully merged stack has nothing left to restructure. Short-circuit + // before opening the TUI, mirroring submit's "nothing to submit" behavior. + if s.IsFullyMerged() { + cfg.Warningf("All branches in this stack have been merged") + cfg.Printf("There's nothing to modify — start a new stack with `%s`", cfg.ColorCyan("gh stack init")) + return nil + } + // Load branch data for the TUI viewNodes := stackview.LoadBranchNodes(cfg, s, currentBranch, result.PRDetails) diff --git a/cmd/modify_test.go b/cmd/modify_test.go index 48d0ec7..fefdff5 100644 --- a/cmd/modify_test.go +++ b/cmd/modify_test.go @@ -2,6 +2,7 @@ package cmd import ( "encoding/json" + "io" "os" "path/filepath" "testing" @@ -636,6 +637,50 @@ func TestCheckModifyPreconditions_AllPass(t *testing.T) { assert.Equal(t, "b1", result.CurrentBranch) } +func TestRunModify_FullyMergedStack_ShortCircuits(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2, Merged: true}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + mock := &git.MockOps{ + GitDirFn: func() (string, error) { return tmpDir, nil }, + CurrentBranchFn: func() (string, error) { return "b1", nil }, + IsRebaseInProgressFn: func() bool { return false }, + HasUncommittedChangesFn: func() (bool, error) { return false, nil }, + BranchExistsFn: func(string) bool { return true }, + IsAncestorFn: func(a, d string) (bool, error) { return true, nil }, + LogMergesFn: func(base, head string) ([]git.CommitInfo, error) { return nil, nil }, + } + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.ForceInteractive = true + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + } + + // runModify must short-circuit (and never launch the TUI) on a fully + // merged stack, returning cleanly like submit's "nothing to submit" path. + err := runModify(cfg) + + cfg.Out.Close() + cfg.Err.Close() + out, _ := io.ReadAll(errR) + output := string(out) + + assert.NoError(t, err) + assert.Contains(t, output, "All branches in this stack have been merged") + assert.Contains(t, output, "gh stack init") +} + // --------------------------------------------------------------------------- // 5. State file path / exists edge cases // --------------------------------------------------------------------------- diff --git a/internal/tui/stackview/model.go b/internal/tui/stackview/model.go index a2997ae..2ed2be8 100644 --- a/internal/tui/stackview/model.go +++ b/internal/tui/stackview/model.go @@ -83,17 +83,17 @@ func New(nodes []BranchNode, trunk stack.BranchRef, version string) Model { h := help.New() h.ShowAll = true - // Cursor starts at the current branch, or first non-merged branch - cursor := 0 - found := false + // Cursor starts at the current branch, or the first non-merged branch. + // When every branch is merged there is nothing selectable, so the cursor + // is hidden (-1) and the cursor-dependent shortcuts are disabled. + cursor := -1 for i, n := range nodes { if n.IsCurrent && !n.Ref.IsMerged() { cursor = i - found = true break } } - if !found { + if cursor < 0 { for i, n := range nodes { if !n.Ref.IsMerged() { cursor = i @@ -439,6 +439,10 @@ func (m Model) buildHeaderConfig() shared.HeaderConfig { branchIcon = "●" } + // When every branch is merged there is no selectable branch, so the cursor + // is hidden and the actions that depend on it are dimmed; only quit works. + allMerged := branchCount > 0 && mergedCount == branchCount + return shared.HeaderConfig{ ShowArt: true, Title: "View Stack", @@ -450,11 +454,11 @@ func (m Model) buildHeaderConfig() shared.HeaderConfig { }, ShortcutColumns: 1, Shortcuts: []shared.ShortcutEntry{ - {Key: "↑↓", Desc: "navigate"}, - {Key: "c", Desc: "commits"}, - {Key: "f", Desc: "files"}, - {Key: "o", Desc: "open PR"}, - {Key: "↵", Desc: "checkout"}, + {Key: "↑↓", Desc: "navigate", Disabled: allMerged}, + {Key: "c", Desc: "commits", Disabled: allMerged}, + {Key: "f", Desc: "files", Disabled: allMerged}, + {Key: "o", Desc: "open PR", Disabled: allMerged}, + {Key: "↵", Desc: "checkout", Disabled: allMerged}, {Key: "q", Desc: "quit"}, }, } diff --git a/internal/tui/stackview/model_test.go b/internal/tui/stackview/model_test.go index b351d09..ea4111f 100644 --- a/internal/tui/stackview/model_test.go +++ b/internal/tui/stackview/model_test.go @@ -9,6 +9,7 @@ import ( ghapi "github.com/github/gh-stack/internal/github" "github.com/github/gh-stack/internal/stack" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func makeNodes(branches ...string) []BranchNode { @@ -388,3 +389,67 @@ func TestUpdate_EnterOnMergedDoesNothing(t *testing.T) { assert.Equal(t, "", m.CheckoutBranch(), "enter on merged branch should not set checkout") assert.Nil(t, cmd, "enter on merged branch should not quit") } + +// makeAllMergedNodes returns nodes whose branches are all merged. +func makeAllMergedNodes(branches ...string) []BranchNode { + nodes := makeNodes(branches...) + for i := range nodes { + nodes[i].Ref.PullRequest = &stack.PullRequestRef{Number: i + 1, Merged: true} + } + return nodes +} + +func TestNew_CursorHiddenWhenAllMerged(t *testing.T) { + m := New(makeAllMergedNodes("b1", "b2"), testTrunk, "0.0.1") + assert.Equal(t, -1, m.cursor, "cursor should be hidden when every branch is merged") +} + +func TestUpdate_AllMergedCursorStaysHidden(t *testing.T) { + m := New(makeAllMergedNodes("b1", "b2", "b3"), testTrunk, "0.0.1") + + updated, _ := m.Update(keyMsg("down")) + m = updated.(Model) + assert.Equal(t, -1, m.cursor, "down should not move the hidden cursor") + + updated, _ = m.Update(keyMsg("up")) + m = updated.(Model) + assert.Equal(t, -1, m.cursor, "up should not move the hidden cursor") + + updated, cmd := m.Update(keyMsg("enter")) + m = updated.(Model) + assert.Equal(t, "", m.CheckoutBranch(), "enter should not check out when all merged") + assert.Nil(t, cmd, "enter should not quit when all merged") +} + +func TestView_AllMergedRendersWithoutPanic(t *testing.T) { + m := New(makeAllMergedNodes("b1", "b2"), testTrunk, "0.0.1") + updated, _ := m.Update(tea.WindowSizeMsg{Width: 100, Height: 40}) + m = updated.(Model) + // Should not panic with a hidden (-1) cursor. + view := m.View() + assert.Contains(t, view, "b1") +} + +func TestBuildHeaderConfig_DisablesShortcutsWhenAllMerged(t *testing.T) { + m := New(makeAllMergedNodes("b1", "b2"), testTrunk, "0.0.1") + cfg := m.buildHeaderConfig() + + require.NotEmpty(t, cfg.Shortcuts) + for _, sc := range cfg.Shortcuts { + if sc.Desc == "quit" { + assert.False(t, sc.Disabled, "quit should stay enabled") + } else { + assert.True(t, sc.Disabled, "%q should be disabled when all branches merged", sc.Desc) + } + } +} + +func TestBuildHeaderConfig_ShortcutsEnabledWithActiveBranches(t *testing.T) { + m := New(makeNodes("b1", "b2"), testTrunk, "0.0.1") + cfg := m.buildHeaderConfig() + + require.NotEmpty(t, cfg.Shortcuts) + for _, sc := range cfg.Shortcuts { + assert.False(t, sc.Disabled, "%q should be enabled when there are active branches", sc.Desc) + } +}