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/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/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/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) + } +} 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