From 6e60ae6510f9af84b95a32fe38841435b909bf25 Mon Sep 17 00:00:00 2001 From: Mo Valipour Date: Mon, 2 Nov 2026 13:10:57 +0000 Subject: [PATCH 1/3] Add git stack get to pull a pull request's stack down Reviewing someone else's stacked PRs meant creating each branch by hand and tracking them one at a time. get takes a PR number, URL or branch name and brings its stack down: gh stack checkout does the work when GitHub has the PR in a stack, and for a lone PR get walks down the open PRs it is based on and tracks them as a new stack. Branches that already exist are fast forwarded with sync's code, never rebased or pushed, and forks are refused since their branches are not on our remote. Also adds the get alias, the stack_get MCP tool, and the matching docs and skill entries. --- README.md | 28 ++- cmd/complete.go | 27 +++ cmd/get.go | 97 ++++++++ cmd/get_test.go | 42 ++++ cmd/root.go | 1 + docs/architecture.md | 12 + docs/mcp.md | 3 +- pkg/app/get.go | 352 ++++++++++++++++++++++++++++ pkg/app/get_test.go | 243 +++++++++++++++++++ pkg/app/mutate_test.go | 4 + pkg/app/prs_test.go | 14 ++ pkg/app/submit_test.go | 4 + pkg/backend/ghstack/ghstack_test.go | 28 +++ pkg/backend/ghstack/tracker.go | 26 ++ pkg/forge/forge.go | 5 + pkg/forge/github/github.go | 14 ++ pkg/forge/github/github_test.go | 15 ++ pkg/git/branch.go | 14 ++ pkg/gitconfig/aliases.go | 1 + pkg/install/install_test.go | 2 +- pkg/install/skill/SKILL.md | 5 +- pkg/mcp/instructions.go | 2 +- pkg/mcp/server.go | 1 + pkg/mcp/server_test.go | 37 ++- pkg/mcp/tools.go | 30 +++ pkg/stack/ports.go | 6 + pkg/stack/stacktest/backend.go | 17 ++ 27 files changed, 1022 insertions(+), 8 deletions(-) create mode 100644 cmd/get.go create mode 100644 cmd/get_test.go create mode 100644 pkg/app/get.go create mode 100644 pkg/app/get_test.go diff --git a/README.md b/README.md index 3bd2312..8c6ec11 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ Claude Code and Codex if you have them installed**. Each of those is on by defau change before it happens, and `--uninstall` removes what it recorded installing and nothing else (that record lives in `git config --global stack.managed*`). -- **Aliases** (`--aliases`): `git create|modify|absorb|reorder|move|fold|rename|untrack|track|pop|restack|continue|abort|undo|submit|sync|up|down|top|bottom|pr` +- **Aliases** (`--aliases`): `git create|modify|absorb|reorder|move|fold|rename|untrack|track|pop|restack|continue|abort|undo|submit|sync|up|down|top|bottom|pr|get` and the short ones `git c|m|ab|rs|ss|u|d|t|b|co`, written with `git config --global`. Anything that clashes with a git builtin or an installed command is skipped, and an alias you already have is only replaced if you pass `--force` or say yes when asked. @@ -593,6 +593,30 @@ Flags: - `-s` / `--stack` only show the current stack - `-t` / `--trunk` check out the trunk of the current stack +### `git stack get ` + +> Alias: `git get` + +Pulls a pull request down from GitHub so you can work on it, by number (`418` or `#418`), URL or +branch name. Its branch and the rest of its stack are created locally where you don't have them, +fast forwarded where the remote is ahead, tracked as a stack and its branch is checked out. + +``` +❯ git get 418 +✔ Pulled down a stack of 3 branches on main and checked out payments-retry-queue + created payments-db, payments-api and payments-retry-queue from the remote +``` + +A pull request in a stack on GitHub brings the whole stack (through `gh stack checkout`). One that +isn't, or a repository without stacked PRs, gets the pull request and the open ones it's based on, +down to the first base that has none, which becomes the trunk. If you already have the stack it's +just brought up to date. + +Nothing is rebased or pushed. A branch that has diverged from the remote is left as it is with a +warning, the same as `sync` does. Pull requests from forks can't be got this way (`gh pr checkout` +checks one out without tracking it), and neither can one based on a branch that's already in +another of your stacks. + ### `git stack pr [branch]` > Alias: `git pr` @@ -712,7 +736,7 @@ git modify ``` It covers create (`--insert` too), modify, absorb, restack, reorder, move, fold, pop, untrack, -init (`track`), sync and a rename that stayed on your machine. Every branch they touched goes back +init (`track`), get, sync and a rename that stayed on your machine. Every branch they touched goes back to where it was (a branch sync or pop deleted comes back, one create made goes away), the stack metadata is put back exactly, and so is what you had checked out. After create, modify or absorb the changes they committed come back as staged changes, and after a pop you're back on the popped diff --git a/cmd/complete.go b/cmd/complete.go index ffa8ff3..863fd4b 100644 --- a/cmd/complete.go +++ b/cmd/complete.go @@ -10,6 +10,7 @@ import ( "github.com/spf13/cobra" "github.com/DomBlack/git-stack/pkg/app" + "github.com/DomBlack/git-stack/pkg/forge" "github.com/DomBlack/git-stack/pkg/stack" ) @@ -286,3 +287,29 @@ func (c *cli) completeTrunks(cmd *cobra.Command, _ []string, toComplete string) } return filterPrefix(items, toComplete), cobra.ShellCompDirectiveNoFileComp } + +// completeOpenPRs offers the open pull requests the PR cache knows about, +// by number with their branch and title, newest first. It never refreshes +// the cache. +func (c *cli) completeOpenPRs(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) { + if len(args) > 0 { + return nil, cobra.ShellCompDirectiveNoFileComp + } + ctx := cmd.Context() + if ctx == nil { + ctx = context.Background() + } + a, repo, err := c.completionApp(ctx) + if err != nil { + return nil, cobra.ShellCompDirectiveNoFileComp + } + prs, _, _ := a.CachedPRs(repo) + var items []cobra.Completion + for _, pr := range prs { + if pr.Fork || (pr.State != forge.StateOpen && pr.State != forge.StateDraft) { + continue + } + items = append(items, cobra.CompletionWithDesc(strconv.Itoa(pr.Number), pr.Head+" · "+pr.Title)) + } + return filterPrefix(items, toComplete), cobra.ShellCompDirectiveNoFileComp | cobra.ShellCompDirectiveKeepOrder +} diff --git a/cmd/get.go b/cmd/get.go new file mode 100644 index 0000000..c96f998 --- /dev/null +++ b/cmd/get.go @@ -0,0 +1,97 @@ +package cmd + +import ( + "fmt" + "strings" + + "github.com/spf13/cobra" + + "github.com/DomBlack/git-stack/pkg/app" + "github.com/DomBlack/git-stack/pkg/ui" +) + +func newGetCmd(c *cli) *cobra.Command { + cmd := &cobra.Command{ + Use: "get ", + Short: "Pull a pull request's stack down from GitHub and check it out", + Long: `Pulls a pull request down from GitHub so you can work on it: its branch and +the branches of its stack are created locally (or fast forwarded where the +remote is ahead), tracked as a stack, and its branch is checked out. Name it +by number (123 or #123), URL or branch name. + +A pull request in a stack on GitHub brings the whole stack, through gh stack. +One that isn't comes with the open pull requests it's based on, down to the +first base that has none, which becomes the trunk. A stack you already have +is just brought up to date. + +Nothing is rebased or pushed, and a branch that has diverged from the remote +is left alone with a warning. Pull requests from forks can't be got this way.`, + Example: ` # a colleague's pull request, by number + git get 418 + + # or by URL, or by branch name + git get https://github.com/acme/payments/pull/418 + git get payments-retry-queue`, + Args: cobra.ExactArgs(1), + ValidArgsFunction: c.completeOpenPRs, + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + a, repo, err := c.app(ctx) + if err != nil { + return err + } + res, err := a.Get(ctx, repo, app.GetOptions{Target: args[0]}) + rep := c.report() + if err == nil { + rep.Success("%s", getSummary(rep, res)) + } + if res.Branch == "" { + return err + } + renderGet(rep, res) + return err + }, + } + return cmd +} + +// getSummary is the result line: what was got and what's checked out. +func getSummary(rep *ui.Reporter, res app.GetResult) string { + n := len(res.Branches) + switch res.Source { + case app.GetLocal: + return fmt.Sprintf("Checked out %s; its stack was already here", rep.Branch(res.Branch)) + case app.GetPRs: + return fmt.Sprintf("Tracking %d %s on %s and checked out %s", n, branchNoun(n), rep.Branch(res.Trunk), rep.Branch(res.Branch)) + default: + return fmt.Sprintf("Pulled down a stack of %d %s on %s and checked out %s", n, branchNoun(n), rep.Branch(res.Trunk), rep.Branch(res.Branch)) + } +} + +// renderGet prints what get did underneath the result line. +func renderGet(rep *ui.Reporter, res app.GetResult) { + if res.Source == app.GetPRs { + rep.Info("the pull request isn't in a stack on GitHub, so it was tracked as a stack of its own") + } + if len(res.Created) > 0 { + names := make([]string, len(res.Created)) + for i, name := range res.Created { + names[i] = rep.Branch(name) + } + rep.Info("created %s from the remote", joinAnd(names)) + } + for _, u := range res.Updated { + rep.Info("%s fast forwarded to %s", rep.Branch(u.Name), rep.SHA(u.To)) + } + for _, n := range res.NotUpdated { + if !n.Failed() { + renderBlocked(rep, n) + } + } + if len(res.NeedsRestack) > 0 { + rep.Warn("%s %s not on the branch below; run git stack restack", strings.Join(res.NeedsRestack, ", "), isAre(len(res.NeedsRestack))) + } + for _, n := range res.Notices { + rep.Warn("%s", n) + } +} diff --git a/cmd/get_test.go b/cmd/get_test.go new file mode 100644 index 0000000..4ab2889 --- /dev/null +++ b/cmd/get_test.go @@ -0,0 +1,42 @@ +package cmd + +import ( + "strings" + "testing" + + "github.com/DomBlack/git-stack/pkg/git/gittest" +) + +func TestGetCommand(t *testing.T) { + gittest.Isolate(t) + dir := gittest.InitRepo(t) + gittest.Run(t, dir, "switch", "-q", "-c", "feat-a") + gittest.Commit(t, dir, "a.txt", "a", "feat: a") + gittest.InitRemote(t, dir) + gittest.Run(t, dir, "switch", "-q", "main") + gittest.Run(t, dir, "branch", "-q", "-D", "feat-a") + + f := fakeGh(t, dir) + f.On("gh", "stack", "checkout").Fail(2, "PR #7 is not part of a stack on GitHub") + f.On("gh", "pr", "view", "7").Reply(`{"number":7,"url":"u/7","title":"A","state":"OPEN","isDraft":false,"isCrossRepository":false,"headRefName":"feat-a","baseRefName":"main"}`) + run := func(args ...string) (string, string, error) { + t.Helper() + return runWith(t, f, append([]string{"--cwd", dir}, args...)...) + } + + out, errOut, err := run("get", "#7") + if err != nil || !strings.Contains(out, "ok: Tracking 1 branch on main and checked out feat-a") || + !strings.Contains(out, "isn't in a stack on GitHub") || !strings.Contains(out, "created feat-a from the remote") { + t.Errorf("get: %q %q %v", out, errOut, err) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "feat-a" { + t.Errorf("checked out %s", cur) + } + out, _, err = run("get", "feat-a") + if err != nil || !strings.Contains(out, "ok: Checked out feat-a; its stack was already here") { + t.Errorf("again: %q %v", out, err) + } + if _, _, err = run("get"); err == nil { + t.Error("no target should fail") + } +} diff --git a/cmd/root.go b/cmd/root.go index 5675a46..f466689 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -363,6 +363,7 @@ func newRootCmd(c *cli) *cobra.Command { newTopCmd(c), newBottomCmd(c), newCheckoutCmd(c), + newGetCmd(c), newLogCmd(c), newPRCmd(c), newInstallCmd(c), diff --git a/docs/architecture.md b/docs/architecture.md index 2cc80f8..2e22ac3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -201,6 +201,18 @@ bottom branch lands on trunk, overshooting clamps silently, merged branches are also check `worktreepath` before switching so you get told which worktree has the branch rather than a confusing git error. +**`get` lets gh stack find the stack, and does lone pull requests itself.** `gh stack checkout +` already asks GitHub which stack a pull request is in, creates the missing branches and +records the stack, so `get` hands it any target that isn't tracked here. It has gaps we fill: +a branch that already exists locally is used as it is, never updated, so `get` fetches first and +then fast forwards the stack's branches with sync's own code (diverged ones get sync's notice); +and a pull request in no stack on GitHub (or a repository without stacked PRs) is exit 2 or 9, so +`get` looks the pull request up through the forge, walks down the open pull requests it's based +on to the first base without one, creates those branches from the remote and records them the way +`init` does. A target that's already tracked here never reaches gh stack, since its composition +check would refuse a stack reordered locally but not yet submitted. Forks are refused rather than +guessed at: their branches aren't on our remote, and submit couldn't push them back anyway. + **`create` from trunk runs `gh stack init`, from the top runs `gh stack add`, and we always commit natively.** This sidesteps both `add` quirks above. From the middle of a stack you get a `not_at_top` error with next steps (unless you pass `--insert`, see below); diff --git a/docs/mcp.md b/docs/mcp.md index 28446b5..439047a 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -33,9 +33,10 @@ path is resolved with `git rev-parse`; anything outside a repository returns `no | `stack_pop` | Delete the current branch and take it out of its stack, leaving its changes staged on its parent; the branches above are restacked onto the parent first, and its open PR is closed by the next `stack_submit`; refuses with nothing changed when tracked files have changes, the branch isn't on its parent or the parent is checked out in another worktree | | destructive | | `stack_continue` | Finish an interrupted restack, modify, reorder, move or pop once the conflicts are resolved and git added | `stage_all` | destructive | | `stack_abort` | Give up an interrupted restack and put every moved branch back (and, after a reorder or move, the old stack order) | | destructive | -| `stack_undo` | Undo the newest command run in this worktree that changed local state (create, modify, absorb, restack, reorder, move, fold, pop, untrack, init, sync, a local rename); call again to step further back, up to 20. After create, modify or absorb the changes come back staged (`staged: true`). A successful `stack_submit` or `stack_merge` clears the history. Refuses with `invalid_args` when nothing is left, when a branch moved since (`force` throws that away; uncommitted changes always stay) or when local changes are in the way, with nothing changed | `list` (the history with what each would put back, changing nothing), `force` (only with the user's say so) | destructive | +| `stack_undo` | Undo the newest command run in this worktree that changed local state (create, modify, absorb, restack, reorder, move, fold, pop, untrack, init, get, sync, a local rename); call again to step further back, up to 20. After create, modify or absorb the changes come back staged (`staged: true`). A successful `stack_submit` or `stack_merge` clears the history. Refuses with `invalid_args` when nothing is left, when a branch moved since (`force` throws that away; uncommitted changes always stay) or when local changes are in the way, with nothing changed | `list` (the history with what each would put back, changing nothing), `force` (only with the user's say so) | destructive | | `stack_restack` | Local rebase of the stack onto its parents, bottom branch onto the local trunk included; only checks out a branch to resolve a conflict through one git rebase | `scope` = `all`/`upstack`/`downstack`/`only`, `branch`, `continue`, `abort`, `stage_all` | destructive | | `stack_navigate` | `up`/`down`/`top`/`bottom` with `steps`, or `branch` | `direction`, `steps`, `branch` | idempotent | +| `stack_get` | Pull a pull request and its stack down from GitHub and check out its branch: the whole stack when it's in one on GitHub, else the open pull requests it's based on as a new stack; a stack already here is only brought up to date. Nothing is rebased or pushed | `target` (number, `#123`, URL or branch) | open world (fetches and asks GitHub) | | `stack_submit` | Push and create/update chained PRs (with `down`, only up to the current branch); ready for review by default; when a PR of the stack has merged since the last sync it syncs first (`merged` names the branches, `sync` says what happened) | `down`, `branch`, `draft`, `publish`, `dry_run`, `pull_requests` `{branch: {title, body}}`, `use_ai`, `force` | destructive, open-world | | `stack_sync` | Fetch, update trunk and restack every stack (checked out or not, all worktrees), never pushing; merged branches deleted per `stack.sync.prune` | `prune` (force deletion) | destructive, open-world | | `stack_merge` | Merge the stack's PRs up to a branch into trunk, all or nothing, then sync; refuses while a check on any of them failed or is still running | `branch` (default current), `method` = `merge`/`squash`/`rebase`, `no_sync`, `force` (skip the checks check; only with the user's say so) | destructive, open-world | diff --git a/pkg/app/get.go b/pkg/app/get.go new file mode 100644 index 0000000..8557b91 --- /dev/null +++ b/pkg/app/get.go @@ -0,0 +1,352 @@ +package app + +import ( + "context" + "errors" + "fmt" + "regexp" + "slices" + "strconv" + "strings" + + "github.com/DomBlack/git-stack/pkg/forge" + "github.com/DomBlack/git-stack/pkg/git" + "github.com/DomBlack/git-stack/pkg/stack" +) + +// GetOptions names what to pull down. +type GetOptions struct { + // Target is a pull request number (123 or #123), a pull request URL or + // a branch name. + Target string +} + +// Where Get found its target. +const ( + // GetLocal: the stack was already tracked here; it was only brought up + // to date with the remote. + GetLocal = "local" + // GetStack: the stack was pulled down from GitHub's stacks. + GetStack = "stack" + // GetPRs: the pull request is in no stack on GitHub, so it and the + // open pull requests it is based on were tracked as a new stack. + GetPRs = "prs" +) + +// GetResult reports what Get did. +type GetResult struct { + // Branch is the branch now checked out. + Branch string `json:"branch"` + Trunk string `json:"trunk"` + // Branches is the stack, bottom to top. + Branches []string `json:"branches"` + Source string `json:"source"` + // Created lists the branches that only existed on the remote before. + Created []string `json:"created,omitempty"` + // Updated lists branches fast forwarded to the remote. + Updated []BranchMove `json:"updated,omitempty"` + // Behind lists branches that have diverged from the remote, with the + // number of commits there that aren't here; they were left alone. + Behind []RemoteAhead `json:"behind,omitempty"` + // NotUpdated lists branches the remote is ahead of that couldn't be + // moved. + NotUpdated []NotUpdated `json:"notUpdated,omitempty"` + // NeedsRestack lists branches not on the tip of the one below them. + NeedsRestack []string `json:"needsRestack,omitempty"` + Notices []string `json:"notices,omitempty"` +} + +// rePRURL matches a pull request's web address and captures its number. +var rePRURL = regexp.MustCompile(`^https?://[^/]+/[^/]+/[^/]+/pull/(\d+)(?:[/?#].*)?$`) + +// parseGetTarget splits a target into a pull request number or a branch +// name; exactly one is set. +func parseGetTarget(s string) (int, string, error) { + s = strings.TrimSpace(s) + if s == "" { + return 0, "", stack.New(stack.KindInvalidArgs, "name a pull request number, URL or branch to get") + } + if m := rePRURL.FindStringSubmatch(s); m != nil { + s = m[1] + } + if n, err := strconv.Atoi(strings.TrimPrefix(s, "#")); err == nil { + if n <= 0 { + return 0, "", stack.Newf(stack.KindInvalidArgs, "%s is not a pull request number", s) + } + return n, "", nil + } + return 0, s, nil +} + +// Get pulls a pull request's stack down from the remote and checks out its +// branch: every branch below it in the stack (and above it, when the stack +// is on GitHub) is created locally if it isn't already, tracked, and fast +// forwarded where the remote is ahead. A stack tracked here already is only +// brought up to date. Nothing is rebased or pushed. +func (a *App) Get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, error) { + return undoableResult(a, ctx, repo, "git stack get", undoKeep, func(ctx context.Context) (GetResult, error) { + return a.get(ctx, repo, o) + }) +} + +// get is Get's body. +func (a *App) get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, error) { + var res GetResult + number, branch, err := parseGetTarget(o.Target) + if err != nil { + return res, err + } + if err := a.noRebaseActive(ctx, repo); err != nil { + return res, err + } + before, err := a.localBranches(ctx, repo) + if err != nil { + return res, err + } + st, err := a.gatherSync(ctx, repo) + if err != nil { + return res, err + } + err = a.progress(ctx, PhaseSync, "Fetching "+st.remote, func(ctx context.Context) error { + return a.d.Git.Fetch(ctx, repo, st.remote) + }) + if err != nil { + return res, stack.Newf(stack.KindAPIFailure, "could not fetch from %s", st.remote). + WithDetail(err.Error()).WithCause(err). + WithSteps("check the remote and your network, then run git stack get again") + } + + target := a.trackedTarget(repo, st.graph, number, branch) + if target != "" { + res.Source = GetLocal + } else { + if target, err = a.importTarget(ctx, repo, number, branch); err != nil { + return res, err + } + if target != "" { + res.Source = GetStack + } else { + res.Source = GetPRs + if target, err = a.getPRs(ctx, st, number, branch, &res); err != nil { + return res, err + } + } + } + + // Bring the stack up to date with the remote, the way sync does. + if st.graph, err = a.d.Meta.Load(ctx, repo); err != nil { + return res, err + } + s, _, ok := st.graph.StackOf(target) + if !ok { + return res, fmt.Errorf("%s is not in a stack after getting it", target) + } + only := *s + st.graph = stack.NewGraph([]stack.Stack{only}) + if err := a.loadCheckouts(ctx, st); err != nil { + return res, err + } + var sres SyncResult + if err := a.syncRemote(ctx, st, &sres); err != nil { + return res, err + } + res.Updated, res.Behind, res.NotUpdated = sres.Updated, sres.Behind, sres.NotUpdated + res.Notices = append(res.Notices, sres.Notices...) + + if err := a.Checkout(ctx, repo, target); err != nil { + return res, err + } + res.Branch, res.Trunk, res.Branches = target, only.Trunk, only.Names() + for _, name := range append([]string{only.Trunk}, res.Branches...) { + if _, ok := before[name]; !ok { + res.Created = append(res.Created, name) + } + } + + var parts, steps []string + for _, n := range res.NotUpdated { + if n.Failed() { + part, step := n.explain() + parts = append(parts, part) + steps = append(steps, step...) + } + } + if len(parts) > 0 { + return res, stack.Newf(stack.KindPartial, "%s; everything else is up to date", strings.Join(parts, "; ")).WithSteps(steps...) + } + return res, nil +} + +// trackedTarget is the branch the target names when it is already in a +// stack here, else "". +func (a *App) trackedTarget(repo git.Repo, graph *stack.Graph, number int, branch string) string { + if number == 0 { + if _, _, ok := graph.StackOf(branch); ok { + return branch + } + return "" + } + for _, s := range graph.Stacks { + for _, b := range s.Branches { + if b.PR != nil && b.PR.Number == number { + return b.Name + } + } + } + // A stack git-stack made itself has no PR records until gh stack + // submits it, so fall back to what the forge last said. + prs, _, _ := a.CachedPRs(repo) + for _, pr := range prs { + if pr.Number == number && !pr.Fork { + if _, _, ok := graph.StackOf(pr.Head); ok { + return pr.Head + } + } + } + return "" +} + +// importTarget pulls the target's stack down from GitHub and returns the +// branch it checked out, or "" when the target is in no stack there (or +// the repository doesn't have stacks), for getPRs to handle. +func (a *App) importTarget(ctx context.Context, repo git.Repo, number int, branch string) (string, error) { + if a.d.Tracker == nil { + return "", nil + } + arg := branch + if number > 0 { + arg = strconv.Itoa(number) + } + err := a.progress(ctx, PhaseSync, "Looking for "+getLabel(number, branch)+" in GitHub's stacks", func(ctx context.Context) error { + return a.d.Tracker.ImportStack(ctx, repo, arg) + }) + if se, ok := errors.AsType[*stack.Error](err); ok && (se.Kind == stack.KindNotInStack || se.Kind == stack.KindStacksUnavailable) { + a.d.Log.Debug("not in a stack on GitHub", "target", arg, "err", err) + return "", nil + } + if err != nil { + return "", err + } + return a.d.Git.CurrentBranch(ctx, repo) +} + +// getPRs tracks a pull request that is in no stack on GitHub, together +// with the open pull requests it is based on, as a new stack on the first +// base that has none. It returns the pull request's branch. +func (a *App) getPRs(ctx context.Context, st *syncState, number int, branch string, res *GetResult) (string, error) { + if a.d.Forge == nil { + return "", ErrNoForge + } + repo := st.repo + var byHead map[string]forge.PullRequest + heads := func() error { + if byHead != nil { + return nil + } + prs, err := a.RefreshPRs(ctx, repo) + if err != nil { + return err + } + byHead = PRsFor(prs) + return nil + } + + var top forge.PullRequest + err := a.progress(ctx, PhaseRefresh, "Looking up "+getLabel(number, branch), func(ctx context.Context) error { + if number > 0 { + var err error + top, err = a.d.Forge.PR(ctx, repo, number) + return err + } + if err := heads(); err != nil { + return err + } + pr, ok := byHead[branch] + if !ok || !prOpen(pr) { + return stack.Newf(stack.KindInvalidArgs, "%s has no open pull request and isn't in a stack on GitHub", branch). + WithSteps("check the name, or pass the pull request's number or URL") + } + top = pr + return nil + }) + if err != nil { + return "", err + } + if top.Fork { + return "", stack.Newf(stack.KindUnsupported, "#%d comes from a fork; git stack get only pulls branches from this repository", top.Number). + WithSteps(fmt.Sprintf("gh pr checkout %d checks it out without tracking it", top.Number)) + } + if !prOpen(top) { + return "", stack.Newf(stack.KindInvalidArgs, "#%d is %s, so there's nothing to get", top.Number, top.State) + } + + // Walk down through the open pull requests it is based on. + chain := []forge.PullRequest{top} + for len(chain) < 100 { + base := chain[0].Base + if st.graph.IsTrunk(base) || slices.Contains(st.trunks, base) { + break + } + if s, _, ok := st.graph.StackOf(base); ok { + return "", stack.Newf(stack.KindUnsupported, "#%d is based on %s, which is already in the stack starting at %s", chain[0].Number, base, s.Bottom()). + WithSteps("git stack get only adds pull requests as a stack of their own; ask for the pull request to be added to that stack on GitHub") + } + if err := heads(); err != nil { + return "", err + } + pr, ok := byHead[base] + if !ok || !prOpen(pr) || slices.ContainsFunc(chain, func(c forge.PullRequest) bool { return c.Head == pr.Head }) { + break + } + chain = slices.Insert(chain, 0, pr) + } + + trunk := chain[0].Base + names := make([]string, len(chain)) + for i, pr := range chain { + names[i] = pr.Head + } + for _, name := range append([]string{trunk}, names...) { + if err := a.ensureLocal(ctx, st, name); err != nil { + return "", err + } + } + ir, err := a.initStack(ctx, repo, InitOptions{Branches: names, Base: trunk}) + if err != nil { + return "", err + } + res.NeedsRestack = ir.NeedsRestack + return top.Head, nil +} + +// ensureLocal creates name from the remote's copy when there's no local +// branch of that name; one that exists is left for syncRemote to bring up +// to date. +func (a *App) ensureLocal(ctx context.Context, st *syncState, name string) error { + if _, ok := st.local[name]; ok { + return nil + } + if _, ok, err := a.d.Git.Tip(ctx, st.repo, "refs/remotes/"+st.remote+"/"+name); err != nil { + return err + } else if !ok { + return stack.Newf(stack.KindInvalidArgs, "%s isn't on %s", name, st.remote). + WithSteps("it may have been deleted; check the pull request on GitHub") + } + if err := a.d.Git.CreateTrackingBranch(ctx, st.repo, name, st.remote); err != nil { + return err + } + return a.loadCheckouts(ctx, st) +} + +// prOpen reports whether a pull request can still be stacked on. +func prOpen(pr forge.PullRequest) bool { + return pr.State == forge.StateOpen || pr.State == forge.StateDraft +} + +// getLabel names the target in progress lines. +func getLabel(number int, branch string) string { + if number > 0 { + return fmt.Sprintf("#%d", number) + } + return branch +} diff --git a/pkg/app/get_test.go b/pkg/app/get_test.go new file mode 100644 index 0000000..095b212 --- /dev/null +++ b/pkg/app/get_test.go @@ -0,0 +1,243 @@ +package app_test + +import ( + "context" + "errors" + "slices" + "testing" + + "github.com/DomBlack/git-stack/pkg/app" + "github.com/DomBlack/git-stack/pkg/cache" + "github.com/DomBlack/git-stack/pkg/config" + "github.com/DomBlack/git-stack/pkg/exec" + "github.com/DomBlack/git-stack/pkg/forge" + "github.com/DomBlack/git-stack/pkg/git" + "github.com/DomBlack/git-stack/pkg/git/gittest" + "github.com/DomBlack/git-stack/pkg/stack" + "github.com/DomBlack/git-stack/pkg/stack/stacktest" +) + +// getFixture is a clone that has only main, with origin holding a (on +// main) and b (on a) and the forge knowing #7 for a and #8 for b. Nothing +// is tracked. +type getFixture struct { + app *app.App + be *stacktest.Backend + forge *recordingForge + repo git.Repo + dir string +} + +func newGetFixture(t *testing.T) *getFixture { + t.Helper() + gittest.Isolate(t) + dir := gittest.InitRepo(t) + gittest.Run(t, dir, "switch", "-q", "-c", "a") + gittest.Commit(t, dir, "a.txt", "a", "feat: a") + gittest.Run(t, dir, "switch", "-q", "-c", "b") + gittest.Commit(t, dir, "b.txt", "b", "feat: b") + gittest.InitRemote(t, dir) + gittest.Run(t, dir, "switch", "-q", "main") + gittest.Run(t, dir, "branch", "-q", "-D", "a", "b") + + g := git.New(exec.New()) + repo, err := g.Discover(context.Background(), dir) + if err != nil { + t.Fatal(err) + } + fg := &recordingForge{prs: []forge.PullRequest{ + {Number: 7, Head: "a", Base: "main", State: forge.StateOpen, URL: "u/7"}, + {Number: 8, Head: "b", Base: "a", State: forge.StateDraft, URL: "u/8"}, + }} + be := stacktest.New(g, nil) + cfg := config.Defaults() + cfg.CacheTTL = 0 + a := app.New(app.Deps{Git: g, Meta: be, Tracker: be, Forge: fg, Cache: cache.New(repo), Config: cfg}) + return &getFixture{app: a, be: be, forge: fg, repo: repo, dir: dir} +} + +func (f *getFixture) get(t *testing.T, target string) (app.GetResult, error) { + t.Helper() + return f.app.Get(context.Background(), f.repo, app.GetOptions{Target: target}) +} + +func (f *getFixture) current(t *testing.T) string { + t.Helper() + return gittest.Run(t, f.dir, "branch", "--show-current") +} + +// pushRemote commits file on top of origin/ and pushes it, leaving +// the local branch (if any) and the checkout where they were. +func (f *getFixture) pushRemote(t *testing.T, branch, file string) string { + t.Helper() + cur := f.current(t) + gittest.Run(t, f.dir, "fetch", "-q", "origin") + gittest.Run(t, f.dir, "switch", "-q", "--detach", "origin/"+branch) + sha := gittest.Commit(t, f.dir, file, file, "remote: "+file) + gittest.Run(t, f.dir, "push", "-q", "origin", "HEAD:refs/heads/"+branch) + gittest.Run(t, f.dir, "switch", "-q", cur) + return sha +} + +func TestGetPullRequestInNoStack(t *testing.T) { + f := newGetFixture(t) + res, err := f.get(t, "#8") + if err != nil { + t.Fatal(err) + } + if res.Source != app.GetPRs || res.Branch != "b" || res.Trunk != "main" || + !slices.Equal(res.Branches, []string{"a", "b"}) || !slices.Equal(res.Created, []string{"a", "b"}) { + t.Errorf("result = %+v", res) + } + if !slices.Equal(f.be.Imports, []string{"8"}) { + t.Errorf("gh stack was asked for %q", f.be.Imports) + } + s, i, ok := f.be.Graph().StackOf("b") + if !ok || i != 1 || s.Trunk != "main" || s.Branches[0].Name != "a" { + t.Errorf("stack = %+v", s) + } + if f.current(t) != "b" { + t.Errorf("checked out %s", f.current(t)) + } + for _, b := range []string{"a", "b"} { + if got := gittest.Run(t, f.dir, "rev-parse", b); got != gittest.Run(t, f.dir, "rev-parse", "origin/"+b) { + t.Errorf("%s is not at origin/%s", b, b) + } + if up := gittest.Run(t, f.dir, "rev-parse", "--abbrev-ref", b+"@{upstream}"); up != "origin/"+b { + t.Errorf("%s tracks %q", b, up) + } + } + + // Asking again finds it here and only brings it up to date. + tip := f.pushRemote(t, "a", "more.txt") + gittest.Run(t, f.dir, "switch", "-q", "main") + res, err = f.get(t, "https://github.com/acme/app/pull/8/files") + if err != nil { + t.Fatal(err) + } + if res.Source != app.GetLocal || res.Branch != "b" || len(res.Created) != 0 || + len(res.Updated) != 1 || res.Updated[0].Name != "a" || res.Updated[0].To != tip { + t.Errorf("again: %+v", res) + } + if len(f.be.Imports) != 1 { + t.Errorf("a tracked stack went to gh stack: %q", f.be.Imports) + } + // b is no longer on a: not get's job to fix, sync's or restack's. + if f.current(t) != "b" { + t.Errorf("checked out %s", f.current(t)) + } +} + +func TestGetByBranchName(t *testing.T) { + f := newGetFixture(t) + res, err := f.get(t, "a") + if err != nil { + t.Fatal(err) + } + if res.Source != app.GetPRs || !slices.Equal(res.Branches, []string{"a"}) || f.current(t) != "a" { + t.Errorf("result = %+v on %s", res, f.current(t)) + } + if !slices.Equal(f.be.Imports, []string{"a"}) { + t.Errorf("gh stack was asked for %q", f.be.Imports) + } + + // b sits on a, which is now in a stack of its own here. + _, err = f.get(t, "b") + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != stack.KindUnsupported { + t.Errorf("b on a tracked branch: %v", err) + } +} + +func TestGetFromGitHubStack(t *testing.T) { + f := newGetFixture(t) + // a exists locally but is behind origin/a, which gh stack leaves alone. + gittest.Run(t, f.dir, "branch", "a", "origin/a") + tip := f.pushRemote(t, "a", "more.txt") + f.be.ImportFn = func(target string) error { + ctx := context.Background() + g := git.New(exec.New()) + if err := g.CreateTrackingBranch(ctx, f.repo, "b", "origin"); err != nil { + return err + } + f.be.SetGraph(stack.NewGraph([]stack.Stack{{Trunk: "main", Number: 3, Branches: []stack.Branch{ + {Name: "a", PR: &stack.PRRef{Number: 7}}, {Name: "b", PR: &stack.PRRef{Number: 8}}, + }}})) + return g.Switch(ctx, f.repo, "a") + } + res, err := f.get(t, "7") + if err != nil { + t.Fatal(err) + } + if res.Source != app.GetStack || res.Branch != "a" || !slices.Equal(res.Branches, []string{"a", "b"}) || + !slices.Equal(res.Created, []string{"b"}) || len(res.Updated) != 1 || res.Updated[0].To != tip { + t.Errorf("result = %+v", res) + } + if got := gittest.Run(t, f.dir, "rev-parse", "a"); got != tip { + t.Errorf("a = %s, want %s", got, tip) + } + if len(f.forge.prs) != 2 || f.forge.lists != 0 { + t.Errorf("the forge was asked about a stack gh stack found (%d lists)", f.forge.lists) + } +} + +func TestGetPassesOtherBackendErrorsOn(t *testing.T) { + f := newGetFixture(t) + f.be.ImportFn = func(string) error { return stack.New(stack.KindAuthRequired, "gh is not logged in") } + _, err := f.get(t, "8") + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != stack.KindAuthRequired { + t.Errorf("err = %v", err) + } + if _, _, ok := f.be.Graph().StackOf("b"); ok { + t.Error("tracked b anyway") + } + + // A repository without stacked PRs falls back like a lone PR. + f.be.ImportFn = func(string) error { return stack.New(stack.KindStacksUnavailable, "no stacks") } + if res, err := f.get(t, "8"); err != nil || res.Source != app.GetPRs { + t.Errorf("stacks unavailable: %+v %v", res, err) + } +} + +func TestGetRefusals(t *testing.T) { + f := newGetFixture(t) + f.forge.prs = append(f.forge.prs, + forge.PullRequest{Number: 9, Head: "x", Base: "main", State: forge.StateOpen, Fork: true}, + forge.PullRequest{Number: 10, Head: "y", Base: "main", State: forge.StateMerged}, + forge.PullRequest{Number: 11, Head: "gone", Base: "main", State: forge.StateOpen}, + ) + for target, kind := range map[string]stack.Kind{ + "": stack.KindInvalidArgs, + "0": stack.KindInvalidArgs, + "9": stack.KindUnsupported, + "10": stack.KindInvalidArgs, + "12": stack.KindInvalidArgs, + "11": stack.KindInvalidArgs, + "nope": stack.KindInvalidArgs, + "#9999": stack.KindInvalidArgs, + } { + _, err := f.get(t, target) + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != kind { + t.Errorf("%q: %v, want %s", target, err, kind.Code()) + } + } + if len(f.be.Graph().Stacks) != 0 || f.current(t) != "main" { + t.Errorf("a refusal changed something: %+v on %s", f.be.Graph().Stacks, f.current(t)) + } +} + +func TestGetUndo(t *testing.T) { + f := newGetFixture(t) + if _, err := f.get(t, "8"); err != nil { + t.Fatal(err) + } + res, err := f.app.Undo(context.Background(), f.repo, app.UndoOptions{}) + if err != nil || res.Undone.Command != "git stack get" { + t.Fatalf("undo: %+v %v", res, err) + } + if f.current(t) != "main" || len(f.be.Graph().Stacks) != 0 { + t.Errorf("after undo: on %s with %+v", f.current(t), f.be.Graph().Stacks) + } + if out, _ := gittest.RunErr(t, f.dir, "branch", "--list", "a", "b"); out != "" { + t.Errorf("branches left behind: %q", out) + } +} diff --git a/pkg/app/mutate_test.go b/pkg/app/mutate_test.go index d4642d4..2f7f0f5 100644 --- a/pkg/app/mutate_test.go +++ b/pkg/app/mutate_test.go @@ -82,6 +82,10 @@ func (f *fakeBackend) AddTop(ctx context.Context, repo git.Repo, name string) er return f.git.Switch(ctx, repo, name) } +func (f *fakeBackend) ImportStack(_ context.Context, _ git.Repo, target string) error { + return stack.Newf(stack.KindNotInStack, "%s is not in a stack on GitHub", target) +} + type fakeAI struct { commit ai.Commit inputs []ai.CommitInput diff --git a/pkg/app/prs_test.go b/pkg/app/prs_test.go index 1dc6c10..8175cfd 100644 --- a/pkg/app/prs_test.go +++ b/pkg/app/prs_test.go @@ -26,6 +26,20 @@ func (f *fakeForge) ListPRs(context.Context, git.Repo) ([]forge.PullRequest, err f.calls++ return f.prs, f.err } +func (f *fakeForge) PR(_ context.Context, _ git.Repo, n int) (forge.PullRequest, error) { + return findPR(f.prs, n) +} + +// findPR is the forge's PR lookup over a fake's list. +func findPR(prs []forge.PullRequest, n int) (forge.PullRequest, error) { + for _, pr := range prs { + if pr.Number == n { + return pr, nil + } + } + return forge.PullRequest{}, stack.Newf(stack.KindInvalidArgs, "there is no pull request #%d", n) +} + func (f *fakeForge) CreatePR(context.Context, git.Repo, forge.CreatePR) (forge.PullRequest, error) { return forge.PullRequest{}, nil } diff --git a/pkg/app/submit_test.go b/pkg/app/submit_test.go index 5755948..3198ec7 100644 --- a/pkg/app/submit_test.go +++ b/pkg/app/submit_test.go @@ -122,6 +122,10 @@ func (f *recordingForge) ClosePR(_ context.Context, _ git.Repo, n int, comment s return nil } +func (f *recordingForge) PR(_ context.Context, _ git.Repo, n int) (forge.PullRequest, error) { + return findPR(f.prs, n) +} + func (f *recordingForge) RenameBranch(context.Context, git.Repo, string, string) error { return nil } func (f *recordingForge) CreateStack(_ context.Context, _ git.Repo, numbers []int) (forge.StackRef, error) { diff --git a/pkg/backend/ghstack/ghstack_test.go b/pkg/backend/ghstack/ghstack_test.go index 9e53214..8125df4 100644 --- a/pkg/backend/ghstack/ghstack_test.go +++ b/pkg/backend/ghstack/ghstack_test.go @@ -191,3 +191,31 @@ func TestGhEnvIsNonInteractive(t *testing.T) { t.Errorf("call = %+v", call) } } + +func TestImportStack(t *testing.T) { + ext := "gh stack\tgithub/gh-stack\tv0.9.0\n" + repo := git.Repo{TopLevel: "/repo"} + f := exectest.New() + f.On("gh", "extension", "list").Reply(ext) + f.On("gh", "stack", "checkout", "418").Reply("") + if err := New(f, nil).ImportStack(context.Background(), repo, "418"); err != nil { + t.Fatal(err) + } + if call := f.CallsTo("gh")[1]; call.Mode != exec.Capture || !slices.Equal(call.Args, []string{"stack", "checkout", "418"}) { + t.Errorf("call = %+v", call) + } + + for code, kind := range map[int]stack.Kind{ + 2: stack.KindNotInStack, // not in a stack on GitHub + 3: stack.KindInvalidArgs, // a local stack composed differently, not a rebase + 9: stack.KindStacksUnavailable, // the repository has no stacks + } { + f := exectest.New() + f.On("gh", "extension", "list").Reply(ext) + f.On("gh", "stack", "checkout").Fail(code, "nope") + err := New(f, nil).ImportStack(context.Background(), repo, "418") + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != kind || se.Detail != "nope" { + t.Errorf("exit %d: %v, want %s", code, err, kind.Code()) + } + } +} diff --git a/pkg/backend/ghstack/tracker.go b/pkg/backend/ghstack/tracker.go index 72e5c5a..ab316f7 100644 --- a/pkg/backend/ghstack/tracker.go +++ b/pkg/backend/ghstack/tracker.go @@ -38,3 +38,29 @@ func (b *Backend) AddTop(ctx context.Context, repo git.Repo, name string) error } return nil } + +// ImportStack runs `gh stack checkout `. It only ever gets a target +// git-stack doesn't already track, so gh stack's own local lookups miss and +// it asks GitHub for the stack. +func (b *Backend) ImportStack(ctx context.Context, repo git.Repo, target string) error { + if err := b.Preflight(ctx); err != nil { + return err + } + err := b.gh(ctx, repo, "checkout", target) + se, ok := errors.AsType[*stack.Error](err) + if !ok { + return err + } + switch se.Kind { + case stack.KindNotInStack: + return stack.Newf(stack.KindNotInStack, "%s is not in a stack on GitHub", target). + WithDetail(se.Detail).WithCause(se.Cause) + case stack.KindConflict: + // Exit 3 from checkout is a local stack holding some of the + // branches in a different order, not a rebase. + return stack.New(stack.KindInvalidArgs, "a local stack holds some of these branches in a different order than GitHub"). + WithSteps("git stack untrack the local branches, then run git stack get again"). + WithDetail(se.Detail).WithCause(se.Cause) + } + return err +} diff --git a/pkg/forge/forge.go b/pkg/forge/forge.go index f728f9f..cdd3057 100644 --- a/pkg/forge/forge.go +++ b/pkg/forge/forge.go @@ -36,6 +36,8 @@ type PullRequest struct { // MergeCommit is the commit the PR was merged as, "" unless merged. MergeCommit string `json:"mergeCommit,omitempty"` UpdatedAt time.Time `json:"updatedAt"` + // Fork is set when the head branch lives in another repository. + Fork bool `json:"fork,omitempty"` } // CreatePR describes a new pull request. @@ -111,6 +113,9 @@ type Forge interface { // ListPRs returns every pull request whose head is a branch of this // repository, in one call. ListPRs(ctx context.Context, repo git.Repo) ([]PullRequest, error) + // PR looks up one pull request by number, forks included. One that + // doesn't exist is a *stack.Error of kind invalid_args. + PR(ctx context.Context, repo git.Repo, number int) (PullRequest, error) CreatePR(ctx context.Context, repo git.Repo, in CreatePR) (PullRequest, error) UpdatePR(ctx context.Context, repo git.Repo, number int, in UpdatePR) error // MergeStack merges pull request number together with every open pull diff --git a/pkg/forge/github/github.go b/pkg/forge/github/github.go index c9ad165..e4746d6 100644 --- a/pkg/forge/github/github.go +++ b/pkg/forge/github/github.go @@ -140,6 +140,7 @@ func (p prJSON) toForge() forge.PullRequest { Head: p.HeadRefName, Base: p.BaseRefName, UpdatedAt: p.UpdatedAt, } pr.HeadSHA = p.HeadRefOid + pr.Fork = p.IsCrossRepository if p.MergeCommit != nil { pr.MergeCommit = p.MergeCommit.OID } @@ -203,6 +204,19 @@ func (f *Forge) view(ctx context.Context, repo git.Repo, selector string) (forge return p.toForge(), nil } +// reNoSuchPR is gh's answer for a pull request number that doesn't exist. +var reNoSuchPR = regexp.MustCompile(`(?i)could not resolve to a pullrequest|no pull requests? found`) + +// PR looks up pull request number through gh pr view. +func (f *Forge) PR(ctx context.Context, repo git.Repo, number int) (forge.PullRequest, error) { + pr, err := f.view(ctx, repo, strconv.Itoa(number)) + if se, ok := errors.AsType[*stack.Error](err); ok && reNoSuchPR.MatchString(se.Detail) { + return forge.PullRequest{}, stack.Newf(stack.KindInvalidArgs, "there is no pull request #%d", number). + WithDetail(se.Detail).WithCause(err) + } + return pr, err +} + // reAlreadyClosed is gh pr close's refusal for a pull request that is // already closed or merged. var reAlreadyClosed = regexp.MustCompile(`(?i)already closed|can't be closed because it was already merged|is already merged`) diff --git a/pkg/forge/github/github_test.go b/pkg/forge/github/github_test.go index cc37693..4d9f2ae 100644 --- a/pkg/forge/github/github_test.go +++ b/pkg/forge/github/github_test.go @@ -156,3 +156,18 @@ func TestClosePR(t *testing.T) { t.Error("a real failure should be an error") } } + +func TestPR(t *testing.T) { + f := exectest.New() + f.On("gh", "pr", "view", "5").Reply(`{"number":5,"url":"u","title":"fork","state":"OPEN","isDraft":false,"isCrossRepository":true,"headRefName":"feat/api","baseRefName":"main"}`) + f.On("gh", "pr", "view", "6").Fail(1, "GraphQL: Could not resolve to a PullRequest with the number of 6. (repository.pullRequest)") + fg := New(f) + pr, err := fg.PR(context.Background(), git.Repo{}, 5) + if err != nil || pr.Number != 5 || !pr.Fork || pr.Head != "feat/api" || pr.State != forge.StateOpen { + t.Errorf("PR 5 = %+v %v", pr, err) + } + _, err = fg.PR(context.Background(), git.Repo{}, 6) + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != stack.KindInvalidArgs || se.Msg != "there is no pull request #6" { + t.Errorf("PR 6: %v", err) + } +} diff --git a/pkg/git/branch.go b/pkg/git/branch.go index 3640c1c..d5d17ab 100644 --- a/pkg/git/branch.go +++ b/pkg/git/branch.go @@ -94,6 +94,20 @@ func (c *Client) CreateBranch(ctx context.Context, repo Repo, name, start string return nil } +// CreateTrackingBranch creates name from remote's copy of it, with remote +// as its upstream, the way git switch does for a branch that only exists on +// a remote. +func (c *Client) CreateTrackingBranch(ctx context.Context, repo Repo, name, remote string) error { + _, err := c.gitIn(ctx, repo, "branch", "--track", name, "refs/remotes/"+remote+"/"+name) + if err != nil { + if ee, ok := errors.AsType[*exec.ExitError](err); ok { + return fmt.Errorf("create branch %q from %s: %s", name, remote, ee.Result.Err()) + } + return err + } + return nil +} + // DefaultBranch guesses the repository's trunk the way gh-stack does: the // branch origin/HEAD points at, else a local main or master. The boolean is // false when nothing matches. diff --git a/pkg/gitconfig/aliases.go b/pkg/gitconfig/aliases.go index 26ad8c2..2583601 100644 --- a/pkg/gitconfig/aliases.go +++ b/pkg/gitconfig/aliases.go @@ -47,6 +47,7 @@ var DefaultAliases = []Alias{ {"abort", "stack abort"}, {"pr", "stack pr"}, {"undo", "stack undo"}, + {"get", "stack get"}, {"c", "stack create"}, {"m", "stack modify"}, {"ab", "stack absorb"}, diff --git a/pkg/install/install_test.go b/pkg/install/install_test.go index 390535a..e7d3c5f 100644 --- a/pkg/install/install_test.go +++ b/pkg/install/install_test.go @@ -192,7 +192,7 @@ func TestInstallAndUninstall(t *testing.T) { } func gitconfigDefaults() []string { - return []string{"create", "modify", "absorb", "reorder", "move", "fold", "rename", "untrack", "track", "pop", "restack", "submit", "sync", "up", "down", "top", "bottom", "continue", "abort", "pr", "undo", "c", "m", "ab", "rs", "ss", "u", "d", "t", "b", "co"} + return []string{"create", "modify", "absorb", "reorder", "move", "fold", "rename", "untrack", "track", "pop", "restack", "submit", "sync", "up", "down", "top", "bottom", "continue", "abort", "pr", "undo", "get", "c", "m", "ab", "rs", "ss", "u", "d", "t", "b", "co"} } func TestSkillOptInIsAsked(t *testing.T) { diff --git a/pkg/install/skill/SKILL.md b/pkg/install/skill/SKILL.md index dbaf111..8b957a5 100644 --- a/pkg/install/skill/SKILL.md +++ b/pkg/install/skill/SKILL.md @@ -30,6 +30,7 @@ that touches a stack: | Take the current branch out of its stack, keeping its changes staged on its parent | `stack_pop` | | Stop tracking a branch and what's above it, leaving the branches alone | `stack_untrack` (`only: true` drops just that branch and restacks the rest onto its parent) | | Move around | `stack_navigate` (`up`/`down`/`top`/`bottom` or `branch`) | +| Pull someone's pull request (and its stack) down to work on it | `stack_get` (`target`: number, URL or branch) | | Push and open/update PRs | `stack_submit`; you write `pull_requests` `{branch: {title, body}}`; ready for review unless `draft: true` | | Pull trunk, restack, prune merged branches | `stack_sync` | @@ -64,6 +65,6 @@ the one calling. `git stack create -m "…"`, `git stack modify -a`, `git stack absorb`, `git stack reorder a b c`, `git stack move --onto `, `git stack fold`, `git stack rename `, `git stack untrack`, `git stack init a b`, `git stack pop`, `git stack restack`, -`git stack up/down/top/bottom`, `git stack checkout`, `git stack submit --no-edit`, +`git stack up/down/top/bottom`, `git stack checkout`, `git stack get 123`, `git stack submit --no-edit`, `git stack sync -f`, `git stack undo`, `git stack pr --print` (a branch's PR URL; `stack_view` has them all). Aliases: `git c`, `git m`, `git ab`, `git reorder`, `git move`, `git fold`, `git rename`, `git untrack`, `git track`, `git pop`, -`git rs`, `git u`, `git d`, `git t`, `git b`, `git co`, `git ss`, `git pr`, `git undo`. +`git rs`, `git u`, `git d`, `git t`, `git b`, `git co`, `git ss`, `git pr`, `git undo`, `git get`. diff --git a/pkg/mcp/instructions.go b/pkg/mcp/instructions.go index a526d08..ca10dfb 100644 --- a/pkg/mcp/instructions.go +++ b/pkg/mcp/instructions.go @@ -2,7 +2,7 @@ package mcp // Instructions is sent to clients at initialisation. Codex weighs the start // of it most, so the key guidance comes first. -const Instructions = `git-stack manages stacked branches and stacked pull requests on top of GitHub's gh stack. Prefer these tools over raw git whenever the user works in a stack: stack_view shows every stack (branches, parents, PR state, needs_restack); stack_create starts a new branch on top of the current one (you write the commit message), or with insert: true right above the current branch in the middle of its stack, restacking the branches above onto it; stack_modify amends the current branch (or adds a commit) and restacks everything above it; stack_absorb folds each staged hunk into the commit of the stack that last changed those lines and rewrites that commit and everything above it (dry_run shows the plan first); use it to fix something lower down without navigating there; stack_reorder changes the order of the current stack's branches (order, bottom to top) and restacks them; stack_move puts a branch and what's above it on top of another stack or on a trunk as a new stack (only: true leaves the branches above behind); stack_fold merges the current branch into the branch below it, one branch and one pull request (keep: true keeps the current branch's name and pull request instead of the parent's), and the next stack_submit closes the pull request of the branch that went; stack_rename gives the current branch a new name, renaming it on GitHub first once it has been pushed so its pull request follows (local: true renames it only in this repository); stack_init tracks branches you already have as a new stack, without rebasing; stack_untrack is its opposite, taking a branch and everything above it out of its stack without touching the branches (only: true takes just that branch and restacks the ones above onto its parent); stack_pop deletes the current branch and takes it out of its stack, leaving its changes staged on its parent, and restacks the branches above (its pull request is closed by the next stack_submit). stack_reorder, stack_move and stack_create with insert leave the stack's pull requests to be regrouped on GitHub: the next stack_submit dissolves every stale GitHub stack and creates the ones it did not submit again, its notices naming each (a stack with fewer than two pull requests has nothing to group; a branch without an open pull request, or a failed create, gets a notice saying to submit from that stack); stack_restack rebases the stack locally without checking anything out, bottom branch onto the local trunk included; on a conflict it leaves one git rebase in progress for the files to be resolved; stack_continue finishes an interrupted restack once the conflicts are resolved and git added; stack_abort gives it up and puts every moved branch back; stack_undo takes back the last of those commands run in this worktree (call it again to step further back; list: true shows what it would undo; a successful stack_submit or stack_merge clears it; only pass force: true when the user said to throw away commits made since); stack_navigate moves up/down/top/bottom or checks out a branch; stack_submit pushes the stack (or with down only the current branch and the ones below it) and creates or updates PRs (you write titles and bodies; ready for review unless draft is true), syncing first when a pull request of the stack has merged since the last sync; stack_sync fetches, fast forwards trunk, deletes branches whose pull requests merged or closed, fast forwards branches that moved on the remote and restacks every stack without checking anything out; it never pushes. stack_merge merges the stack's pull requests up to a branch into trunk in one all or nothing operation (drafts must be published first) and then syncs; it refuses while any of those pull requests has a check that failed (code checks_failing) or is still running (code checks_pending), listing them in the error's checks field. Fix the failures and push with stack_submit, or wait for running checks and call it again; only pass force: true when the user has said to merge anyway. +const Instructions = `git-stack manages stacked branches and stacked pull requests on top of GitHub's gh stack. Prefer these tools over raw git whenever the user works in a stack: stack_view shows every stack (branches, parents, PR state, needs_restack); stack_create starts a new branch on top of the current one (you write the commit message), or with insert: true right above the current branch in the middle of its stack, restacking the branches above onto it; stack_modify amends the current branch (or adds a commit) and restacks everything above it; stack_absorb folds each staged hunk into the commit of the stack that last changed those lines and rewrites that commit and everything above it (dry_run shows the plan first); use it to fix something lower down without navigating there; stack_reorder changes the order of the current stack's branches (order, bottom to top) and restacks them; stack_move puts a branch and what's above it on top of another stack or on a trunk as a new stack (only: true leaves the branches above behind); stack_fold merges the current branch into the branch below it, one branch and one pull request (keep: true keeps the current branch's name and pull request instead of the parent's), and the next stack_submit closes the pull request of the branch that went; stack_rename gives the current branch a new name, renaming it on GitHub first once it has been pushed so its pull request follows (local: true renames it only in this repository); stack_init tracks branches you already have as a new stack, without rebasing; stack_untrack is its opposite, taking a branch and everything above it out of its stack without touching the branches (only: true takes just that branch and restacks the ones above onto its parent); stack_pop deletes the current branch and takes it out of its stack, leaving its changes staged on its parent, and restacks the branches above (its pull request is closed by the next stack_submit). stack_reorder, stack_move and stack_create with insert leave the stack's pull requests to be regrouped on GitHub: the next stack_submit dissolves every stale GitHub stack and creates the ones it did not submit again, its notices naming each (a stack with fewer than two pull requests has nothing to group; a branch without an open pull request, or a failed create, gets a notice saying to submit from that stack); stack_restack rebases the stack locally without checking anything out, bottom branch onto the local trunk included; on a conflict it leaves one git rebase in progress for the files to be resolved; stack_continue finishes an interrupted restack once the conflicts are resolved and git added; stack_abort gives it up and puts every moved branch back; stack_undo takes back the last of those commands run in this worktree (call it again to step further back; list: true shows what it would undo; a successful stack_submit or stack_merge clears it; only pass force: true when the user said to throw away commits made since); stack_navigate moves up/down/top/bottom or checks out a branch; stack_get pulls a pull request (number, URL or branch) and its stack down from GitHub and checks it out, creating or fast forwarding the branches and tracking them; stack_submit pushes the stack (or with down only the current branch and the ones below it) and creates or updates PRs (you write titles and bodies; ready for review unless draft is true), syncing first when a pull request of the stack has merged since the last sync; stack_sync fetches, fast forwards trunk, deletes branches whose pull requests merged or closed, fast forwards branches that moved on the remote and restacks every stack without checking anything out; it never pushes. stack_merge merges the stack's pull requests up to a branch into trunk in one all or nothing operation (drafts must be published first) and then syncs; it refuses while any of those pull requests has a check that failed (code checks_failing) or is still running (code checks_pending), listing them in the error's checks field. Fix the failures and push with stack_submit, or wait for running checks and call it again; only pass force: true when the user has said to merge anyway. Never run git rebase or git commit --amend by hand inside a stack: descendants would not be restacked. Use stack_modify / stack_restack instead. diff --git a/pkg/mcp/server.go b/pkg/mcp/server.go index 5b35a6a..4f0b30f 100644 --- a/pkg/mcp/server.go +++ b/pkg/mcp/server.go @@ -240,6 +240,7 @@ var cliToTool = []struct{ from, to string }{ {"git stack checkout --trunk", "stack_navigate {direction: \"bottom\"} then {direction: \"down\"}"}, {"git stack checkout", "stack_navigate {branch: ...}"}, {"git stack create", "stack_create"}, + {"git stack get", "stack_get"}, {"git stack sync", "stack_sync"}, {"git stack merge --force", "stack_merge with force: true (only with the user's say so)"}, {"git stack merge ", "stack_merge {branch: ...}"}, diff --git a/pkg/mcp/server_test.go b/pkg/mcp/server_test.go index 047d287..b20ff8c 100644 --- a/pkg/mcp/server_test.go +++ b/pkg/mcp/server_test.go @@ -54,6 +54,14 @@ func (f *fakeForge) RenameBranch(_ context.Context, _ git.Repo, from, to string) func (f *fakeForge) CreateStack(context.Context, git.Repo, []int) (forge.StackRef, error) { return forge.StackRef{}, nil } +func (f *fakeForge) PR(_ context.Context, _ git.Repo, n int) (forge.PullRequest, error) { + for _, pr := range f.prs { + if pr.Number == n { + return pr, nil + } + } + return forge.PullRequest{}, stack.Newf(stack.KindInvalidArgs, "there is no pull request #%d", n) +} func (f *fakeForge) CreatePR(context.Context, git.Repo, forge.CreatePR) (forge.PullRequest, error) { return forge.PullRequest{}, nil } @@ -229,7 +237,7 @@ func TestToolsAreListedWithAnnotationsAndSchemas(t *testing.T) { for _, tool := range res.Tools { got[tool.Name] = tool } - for _, name := range []string{"stack_view", "stack_create", "stack_modify", "stack_absorb", "stack_reorder", "stack_move", "stack_fold", "stack_rename", "stack_untrack", "stack_doctor", "stack_init", "stack_pop", "stack_restack", "stack_undo", "stack_navigate", "stack_submit", "stack_sync"} { + for _, name := range []string{"stack_view", "stack_create", "stack_modify", "stack_absorb", "stack_reorder", "stack_move", "stack_fold", "stack_rename", "stack_untrack", "stack_doctor", "stack_init", "stack_pop", "stack_restack", "stack_undo", "stack_navigate", "stack_get", "stack_submit", "stack_sync"} { if got[name] == nil { t.Errorf("missing tool %s", name) } @@ -937,6 +945,33 @@ func TestRename(t *testing.T) { } } +func TestGet(t *testing.T) { + h := newHarness(t) + defer h.assertNoStdout() + // c is on origin only, with a pull request in no stack. + gittest.Run(t, h.dir, "switch", "-q", "-c", "c", "main") + gittest.Commit(t, h.dir, "c.txt", "c", "feat: c") + gittest.Run(t, h.dir, "push", "-q", "origin", "c") + gittest.Run(t, h.dir, "switch", "-q", "b") + gittest.Run(t, h.dir, "branch", "-q", "-D", "c") + h.forge.prs = append(h.forge.prs, + forge.PullRequest{Number: 9, Head: "c", Base: "main", State: forge.StateOpen}, + forge.PullRequest{Number: 11, Head: "f", Base: "main", State: forge.StateOpen, Fork: true}) + + var res app.GetResult + h.call("stack_get", map[string]any{"target": "#9"}, &res) + if res.Source != app.GetPRs || res.Branch != "c" || len(res.Created) != 1 || h.current() != "c" { + t.Fatalf("get = %+v on %s", res, h.current()) + } + h.call("stack_get", map[string]any{"target": "a"}, &res) + if res.Source != app.GetLocal || res.Branch != "a" || h.current() != "a" { + t.Errorf("tracked = %+v on %s", res, h.current()) + } + if te := h.toolErr("stack_get", map[string]any{"target": "11"}); te.Code != "unsupported" || len(te.NextSteps) == 0 { + t.Errorf("fork = %+v", te) + } +} + func TestDoctor(t *testing.T) { h := newHarness(t) defer h.assertNoStdout() diff --git a/pkg/mcp/tools.go b/pkg/mcp/tools.go index c7099c4..7066564 100644 --- a/pkg/mcp/tools.go +++ b/pkg/mcp/tools.go @@ -383,6 +383,30 @@ func (s *Server) initStack(ctx context.Context, req *mcp.CallToolRequest, in ini return nil, res, nil } +// --- stack_get --------------------------------------------------------- + +type getInput struct { + repoArg + Target string `json:"target" jsonschema:"the pull request to get: its number (123 or #123), its URL or its branch name"` +} + +func (s *Server) get(ctx context.Context, req *mcp.CallToolRequest, in getInput) (*mcp.CallToolResult, app.GetResult, error) { + repo, a, err := s.appFor(ctx, req, in.RepoPath) + if err != nil { + return nil, app.GetResult{}, wrapErr(err) + } + res, err := a.Get(ctx, repo, app.GetOptions{Target: in.Target}) + if err != nil && res.Branch != "" && errors.Is(err, &stack.Error{Kind: stack.KindPartial}) { + // Checked out, but a branch the remote is ahead of couldn't move: + // keep the result so notUpdated says which. + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: wrapErr(err).Error()}}}, res, nil + } + if err != nil { + return nil, app.GetResult{}, wrapErr(err) + } + return nil, res, nil +} + // --- stack_pop --------------------------------------------------------- type popInput struct{ repoArg } @@ -740,6 +764,12 @@ func (s *Server) registerTools() { Description: "Track existing branches as a new stack on a trunk, bottom to top; nothing is committed, rebased or checked out. The result's needsRestack lists branches that aren't on their parent; stack_restack fixes them.", Annotations: &mcp.ToolAnnotations{DestructiveHint: boolp(true), OpenWorldHint: boolp(false)}, }, s.initStack) + mcp.AddTool(s.mcp, &mcp.Tool{ + Name: "stack_get", + Title: "Get a pull request and its stack", + Description: "Pull a pull request down from GitHub and check out its branch, by number, URL or branch name (target). A pull request in a stack on GitHub brings the whole stack; one that isn't comes with the open pull requests it is based on, tracked as a new stack on the first base with none (source says which: stack, prs, or local for a stack already tracked here, which is only brought up to date). Missing branches are created from the remote (created), ones the remote is ahead of are fast forwarded (updated) and diverged ones are left alone (behind); nothing is rebased or pushed. Code partial means it is checked out but a branch in notUpdated couldn't be moved. Refuses with unsupported for a pull request from a fork or one based on a branch already in another local stack, and invalid_args for one that is merged, closed or doesn't exist.", + Annotations: &mcp.ToolAnnotations{DestructiveHint: boolp(false), OpenWorldHint: boolp(true)}, + }, s.get) mcp.AddTool(s.mcp, &mcp.Tool{ Name: "stack_pop", Title: "Pop the current branch out of its stack", diff --git a/pkg/stack/ports.go b/pkg/stack/ports.go index 83a2986..df421ab 100644 --- a/pkg/stack/ports.go +++ b/pkg/stack/ports.go @@ -69,6 +69,12 @@ type Tracker interface { // AddTop creates (or adopts) name on top of the current stack and checks // it out. The current branch must be the top of its stack. AddTop(ctx context.Context, repo git.Repo, name string) error + // ImportStack pulls down the forge stack holding target, a pull + // request number or a branch name: it creates the local branches it + // is missing (leaving ones that exist alone), records the stack and + // checks out target's branch. A target in no stack on the forge is a + // *Error of KindNotInStack. + ImportStack(ctx context.Context, repo git.Repo, target string) error } // SubmitOptions controls Submitter.Submit. diff --git a/pkg/stack/stacktest/backend.go b/pkg/stack/stacktest/backend.go index 2e9d6b4..f8dc942 100644 --- a/pkg/stack/stacktest/backend.go +++ b/pkg/stack/stacktest/backend.go @@ -29,6 +29,12 @@ type Backend struct { // its result. Unstacked []int UnstackFn func(number int) error + + // Imports records every ImportStack target; ImportFn, when set, does + // the import (it may call SetGraph). Without it every target is in no + // stack on the forge. + Imports []string + ImportFn func(target string) error } // New returns a Backend over g with the given initial graph. @@ -146,6 +152,17 @@ func (b *Backend) AddTop(ctx context.Context, repo git.Repo, name string) error return b.git.Switch(ctx, repo, name) } +func (b *Backend) ImportStack(_ context.Context, _ git.Repo, target string) error { + b.mu.Lock() + b.Imports = append(b.Imports, target) + fn := b.ImportFn + b.mu.Unlock() + if fn != nil { + return fn(target) + } + return stack.Newf(stack.KindNotInStack, "%s is not in a stack on GitHub", target) +} + func (b *Backend) Submit(_ context.Context, _ git.Repo, o stack.SubmitOptions) (stack.SubmitResult, error) { b.mu.Lock() b.Submits = append(b.Submits, o) From 5fadd98855506fc54b4d628b916695677c5834f3 Mon Sep 17 00:00:00 2001 From: Mo Valipour Date: Mon, 2 Nov 2026 14:16:10 +0000 Subject: [PATCH 2/3] Address review comments on git stack get - Refuse a pull request URL for another repository instead of reading its number as one of ours. - Report a failed checkout after branches moved as a partial result, so undo records it; the result's checkedOut says whether it happened. - Look an untracked pull request number up on the forge and check its branch against the graph, so getting a lone PR again finds it. - Work out needsRestack after the fast forwards, for every source. - Find base pull requests by branch with Forge.OpenPRFor rather than in the 500 most recent, which also runs the lookup on the step's context. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +- cmd/get.go | 2 +- docs/architecture.md | 12 +- pkg/app/get.go | 265 +++++++++++++++++++++----------- pkg/app/get_test.go | 57 ++++++- pkg/app/prs_test.go | 15 ++ pkg/app/submit_test.go | 8 + pkg/forge/forge.go | 5 + pkg/forge/github/github.go | 19 +++ pkg/forge/github/github_test.go | 22 +++ pkg/mcp/server_test.go | 8 + pkg/mcp/tools.go | 7 +- 12 files changed, 327 insertions(+), 97 deletions(-) diff --git a/README.md b/README.md index 8c6ec11..e36a2c6 100644 --- a/README.md +++ b/README.md @@ -612,7 +612,9 @@ isn't, or a repository without stacked PRs, gets the pull request and the open o down to the first base that has none, which becomes the trunk. If you already have the stack it's just brought up to date. -Nothing is rebased or pushed. A branch that has diverged from the remote is left as it is with a +Nothing is rebased or pushed, and a URL for another repository is refused. If the branch can't be +checked out (files in the way, or it's checked out in another worktree) the stack is still got +and you're told why. A branch that has diverged from the remote is left as it is with a warning, the same as `sync` does. Pull requests from forks can't be got this way (`gh pr checkout` checks one out without tracking it), and neither can one based on a branch that's already in another of your stacks. diff --git a/cmd/get.go b/cmd/get.go index c96f998..7bebef2 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -45,7 +45,7 @@ is left alone with a warning. Pull requests from forks can't be got this way.`, if err == nil { rep.Success("%s", getSummary(rep, res)) } - if res.Branch == "" { + if len(res.Branches) == 0 { return err } renderGet(rep, res) diff --git a/docs/architecture.md b/docs/architecture.md index 2e22ac3..dfc784b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -208,9 +208,15 @@ a branch that already exists locally is used as it is, never updated, so `get` f then fast forwards the stack's branches with sync's own code (diverged ones get sync's notice); and a pull request in no stack on GitHub (or a repository without stacked PRs) is exit 2 or 9, so `get` looks the pull request up through the forge, walks down the open pull requests it's based -on to the first base without one, creates those branches from the remote and records them the way -`init` does. A target that's already tracked here never reaches gh stack, since its composition -check would refuse a stack reordered locally but not yet submitted. Forks are refused rather than +on to the first base without one (asking for each base's open pull request by branch, since the +list of recent pull requests sync uses stops at 500), creates those branches from the remote and records them the way +`init` does. A pull request number nothing here records is looked up first and its branch checked against +the graph, because a stack git-stack tracked itself has no PR records until gh stack submits it. +A target that's already tracked here never reaches gh stack, since its composition +check would refuse a stack reordered locally but not yet submitted. Once branches have moved, a failed +checkout is a partial result rather than an error, so undo still records it. A URL is checked +against the repository the forge works out from the remotes, as its number means a different +pull request anywhere else. Forks are refused rather than guessed at: their branches aren't on our remote, and submit couldn't push them back anyway. **`create` from trunk runs `gh stack init`, from the top runs `gh stack add`, and we diff --git a/pkg/app/get.go b/pkg/app/get.go index 8557b91..313d360 100644 --- a/pkg/app/get.go +++ b/pkg/app/get.go @@ -35,9 +35,12 @@ const ( // GetResult reports what Get did. type GetResult struct { - // Branch is the branch now checked out. + // Branch is the pull request's branch. Branch string `json:"branch"` - Trunk string `json:"trunk"` + // CheckedOut is set once Branch is checked out; when it isn't, the + // error says why. + CheckedOut bool `json:"checkedOut"` + Trunk string `json:"trunk"` // Branches is the stack, bottom to top. Branches []string `json:"branches"` Source string `json:"source"` @@ -56,26 +59,57 @@ type GetResult struct { Notices []string `json:"notices,omitempty"` } -// rePRURL matches a pull request's web address and captures its number. -var rePRURL = regexp.MustCompile(`^https?://[^/]+/[^/]+/[^/]+/pull/(\d+)(?:[/?#].*)?$`) +// rePRURL matches a pull request's web address, capturing its repository +// (host/owner/name) and number. +var rePRURL = regexp.MustCompile(`^https?://([^/]+/[^/]+/[^/]+)/pull/(\d+)(?:[/?#].*)?$`) -// parseGetTarget splits a target into a pull request number or a branch -// name; exactly one is set. -func parseGetTarget(s string) (int, string, error) { +// getTarget is a parsed target: a pull request number or a branch name, +// exactly one of them set. Repo is the host/owner/name of a URL's +// repository, "" for anything else. +type getTarget struct { + number int + branch string + repo string +} + +// parseGetTarget parses a target. +func parseGetTarget(s string) (getTarget, error) { s = strings.TrimSpace(s) if s == "" { - return 0, "", stack.New(stack.KindInvalidArgs, "name a pull request number, URL or branch to get") + return getTarget{}, stack.New(stack.KindInvalidArgs, "name a pull request number, URL or branch to get") } + var t getTarget if m := rePRURL.FindStringSubmatch(s); m != nil { - s = m[1] + t.repo, s = m[1], m[2] } if n, err := strconv.Atoi(strings.TrimPrefix(s, "#")); err == nil { if n <= 0 { - return 0, "", stack.Newf(stack.KindInvalidArgs, "%s is not a pull request number", s) + return getTarget{}, stack.Newf(stack.KindInvalidArgs, "%s is not a pull request number", s) } - return n, "", nil + t.number = n + return t, nil } - return 0, s, nil + t.branch = s + return t, nil +} + +// checkURLRepo refuses a pull request URL for another repository than the +// one we're in: its number means a different pull request here. When the +// forge can't tell which repository this is, the URL is taken on trust. +func (a *App) checkURLRepo(ctx context.Context, repo git.Repo, t getTarget) error { + if t.repo == "" || a.d.Forge == nil { + return nil + } + here, err := a.d.Forge.PullRequestURL(ctx, repo, t.number) + if err != nil || here == "" { + return nil + } + m := rePRURL.FindStringSubmatch(here) + if m == nil || strings.EqualFold(m[1], t.repo) { + return nil + } + return stack.Newf(stack.KindInvalidArgs, "that pull request is in %s, not this repository (%s)", t.repo, m[1]). + WithSteps("run git stack get from a clone of " + t.repo) } // Get pulls a pull request's stack down from the remote and checks out its @@ -92,10 +126,13 @@ func (a *App) Get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, // get is Get's body. func (a *App) get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, error) { var res GetResult - number, branch, err := parseGetTarget(o.Target) + t, err := parseGetTarget(o.Target) if err != nil { return res, err } + if err := a.checkURLRepo(ctx, repo, t); err != nil { + return res, err + } if err := a.noRebaseActive(ctx, repo); err != nil { return res, err } @@ -116,18 +153,21 @@ func (a *App) get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, WithSteps("check the remote and your network, then run git stack get again") } - target := a.trackedTarget(repo, st.graph, number, branch) + target, pr, err := a.trackedTarget(ctx, repo, st.graph, t) + if err != nil { + return res, err + } if target != "" { res.Source = GetLocal } else { - if target, err = a.importTarget(ctx, repo, number, branch); err != nil { + if target, err = a.importTarget(ctx, repo, t); err != nil { return res, err } if target != "" { res.Source = GetStack } else { res.Source = GetPRs - if target, err = a.getPRs(ctx, st, number, branch, &res); err != nil { + if target, err = a.getPRs(ctx, st, t, pr); err != nil { return res, err } } @@ -152,18 +192,28 @@ func (a *App) get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, } res.Updated, res.Behind, res.NotUpdated = sres.Updated, sres.Behind, sres.NotUpdated res.Notices = append(res.Notices, sres.Notices...) - - if err := a.Checkout(ctx, repo, target); err != nil { - return res, err - } res.Branch, res.Trunk, res.Branches = target, only.Trunk, only.Names() for _, name := range append([]string{only.Trunk}, res.Branches...) { if _, ok := before[name]; !ok { res.Created = append(res.Created, name) } } + if res.NeedsRestack, err = a.getNeedsRestack(ctx, repo, only); err != nil { + return res, err + } + // From here on the branches have moved, so a failure is partial: the + // result says what was done, and undo can take it back. var parts, steps []string + if err := a.Checkout(ctx, repo, target); err != nil { + parts = append(parts, fmt.Sprintf("%s was not checked out: %s", target, errMessage(err))) + if se, ok := errors.AsType[*stack.Error](err); ok && len(se.NextSteps) > 0 { + steps = append(steps, se.NextSteps...) + } + steps = append(steps, "then git stack checkout "+target) + } else { + res.CheckedOut = true + } for _, n := range res.NotUpdated { if n.Failed() { part, step := n.explain() @@ -177,47 +227,95 @@ func (a *App) get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, return res, nil } +// errMessage is an error's message without a *stack.Error's steps. +func errMessage(err error) string { + if se, ok := errors.AsType[*stack.Error](err); ok { + return se.Msg + } + return err.Error() +} + +// getNeedsRestack lists the branches of s that aren't on the local tip of +// the one below them (trunk for the bottom one), as they are once get has +// fast forwarded what it could. Merged branches and ones missing locally +// are skipped, and so is a branch above one that is missing. +func (a *App) getNeedsRestack(ctx context.Context, repo git.Repo, s stack.Stack) ([]string, error) { + local, err := a.localBranches(ctx, repo) + if err != nil { + return nil, err + } + var out []string + parent := s.Trunk + for _, b := range s.Branches { + if b.Merged() { + continue + } + pb, pok := local[parent] + lb, ok := local[b.Name] + if pok && ok && !a.isAncestor(ctx, repo, pb.Head, lb.Head) { + out = append(out, b.Name) + } + parent = b.Name + } + return out, nil +} + // trackedTarget is the branch the target names when it is already in a -// stack here, else "". -func (a *App) trackedTarget(repo git.Repo, graph *stack.Graph, number int, branch string) string { - if number == 0 { - if _, _, ok := graph.StackOf(branch); ok { - return branch +// stack here, else "". A pull request number nothing here records is looked +// up on the forge, since a stack git-stack made itself has no PR records +// until gh stack submits it; the pull request found (nil when there was no +// need, or no forge) is returned for getPRs. +func (a *App) trackedTarget(ctx context.Context, repo git.Repo, graph *stack.Graph, t getTarget) (string, *forge.PullRequest, error) { + if t.number == 0 { + if _, _, ok := graph.StackOf(t.branch); ok { + return t.branch, nil, nil } - return "" + return "", nil, nil } for _, s := range graph.Stacks { for _, b := range s.Branches { - if b.PR != nil && b.PR.Number == number { - return b.Name + if b.PR != nil && b.PR.Number == t.number { + return b.Name, nil, nil } } } - // A stack git-stack made itself has no PR records until gh stack - // submits it, so fall back to what the forge last said. - prs, _, _ := a.CachedPRs(repo) - for _, pr := range prs { - if pr.Number == number && !pr.Fork { - if _, _, ok := graph.StackOf(pr.Head); ok { - return pr.Head - } + if a.d.Forge == nil { + return "", nil, nil + } + var pr forge.PullRequest + err := a.progress(ctx, PhaseRefresh, "Looking up "+getLabel(t), func(ctx context.Context) error { + var err error + pr, err = a.d.Forge.PR(ctx, repo, t.number) + return err + }) + if se, ok := errors.AsType[*stack.Error](err); ok && se.Kind == stack.KindInvalidArgs { + // No such pull request; gh stack may still know the number as a + // stack's. + return "", nil, nil + } + if err != nil { + return "", nil, err + } + if !pr.Fork { + if _, _, ok := graph.StackOf(pr.Head); ok { + return pr.Head, nil, nil } } - return "" + return "", &pr, nil } // importTarget pulls the target's stack down from GitHub and returns the // branch it checked out, or "" when the target is in no stack there (or // the repository doesn't have stacks), for getPRs to handle. -func (a *App) importTarget(ctx context.Context, repo git.Repo, number int, branch string) (string, error) { +func (a *App) importTarget(ctx context.Context, repo git.Repo, t getTarget) (string, error) { if a.d.Tracker == nil { return "", nil } - arg := branch - if number > 0 { - arg = strconv.Itoa(number) + arg := t.branch + if t.number > 0 { + arg = strconv.Itoa(t.number) } - err := a.progress(ctx, PhaseSync, "Looking for "+getLabel(number, branch)+" in GitHub's stacks", func(ctx context.Context) error { + err := a.progress(ctx, PhaseSync, "Looking for "+getLabel(t)+" in GitHub's stacks", func(ctx context.Context) error { return a.d.Tracker.ImportStack(ctx, repo, arg) }) if se, ok := errors.AsType[*stack.Error](err); ok && (se.Kind == stack.KindNotInStack || se.Kind == stack.KindStacksUnavailable) { @@ -232,56 +330,47 @@ func (a *App) importTarget(ctx context.Context, repo git.Repo, number int, branc // getPRs tracks a pull request that is in no stack on GitHub, together // with the open pull requests it is based on, as a new stack on the first -// base that has none. It returns the pull request's branch. -func (a *App) getPRs(ctx context.Context, st *syncState, number int, branch string, res *GetResult) (string, error) { +// base that has none. top is the pull request when trackedTarget already +// looked it up. It returns the pull request's branch. +func (a *App) getPRs(ctx context.Context, st *syncState, t getTarget, top *forge.PullRequest) (string, error) { if a.d.Forge == nil { return "", ErrNoForge } repo := st.repo - var byHead map[string]forge.PullRequest - heads := func() error { - if byHead != nil { + if top == nil { + var pr forge.PullRequest + err := a.progress(ctx, PhaseRefresh, "Looking up "+getLabel(t), func(ctx context.Context) error { + if t.number > 0 { + var err error + pr, err = a.d.Forge.PR(ctx, repo, t.number) + return err + } + found, ok, err := a.d.Forge.OpenPRFor(ctx, repo, t.branch) + if err != nil { + return err + } + if !ok { + return stack.Newf(stack.KindInvalidArgs, "%s has no open pull request and isn't in a stack on GitHub", t.branch). + WithSteps("check the name, or pass the pull request's number or URL") + } + pr = found return nil - } - prs, err := a.RefreshPRs(ctx, repo) + }) if err != nil { - return err - } - byHead = PRsFor(prs) - return nil - } - - var top forge.PullRequest - err := a.progress(ctx, PhaseRefresh, "Looking up "+getLabel(number, branch), func(ctx context.Context) error { - if number > 0 { - var err error - top, err = a.d.Forge.PR(ctx, repo, number) - return err - } - if err := heads(); err != nil { - return err - } - pr, ok := byHead[branch] - if !ok || !prOpen(pr) { - return stack.Newf(stack.KindInvalidArgs, "%s has no open pull request and isn't in a stack on GitHub", branch). - WithSteps("check the name, or pass the pull request's number or URL") + return "", err } - top = pr - return nil - }) - if err != nil { - return "", err + top = &pr } if top.Fork { return "", stack.Newf(stack.KindUnsupported, "#%d comes from a fork; git stack get only pulls branches from this repository", top.Number). WithSteps(fmt.Sprintf("gh pr checkout %d checks it out without tracking it", top.Number)) } - if !prOpen(top) { + if !prOpen(*top) { return "", stack.Newf(stack.KindInvalidArgs, "#%d is %s, so there's nothing to get", top.Number, top.State) } // Walk down through the open pull requests it is based on. - chain := []forge.PullRequest{top} + chain := []forge.PullRequest{*top} for len(chain) < 100 { base := chain[0].Base if st.graph.IsTrunk(base) || slices.Contains(st.trunks, base) { @@ -291,11 +380,17 @@ func (a *App) getPRs(ctx context.Context, st *syncState, number int, branch stri return "", stack.Newf(stack.KindUnsupported, "#%d is based on %s, which is already in the stack starting at %s", chain[0].Number, base, s.Bottom()). WithSteps("git stack get only adds pull requests as a stack of their own; ask for the pull request to be added to that stack on GitHub") } - if err := heads(); err != nil { + var pr forge.PullRequest + var ok bool + err := a.progress(ctx, PhaseRefresh, "Looking up the pull request for "+base, func(ctx context.Context) error { + var err error + pr, ok, err = a.d.Forge.OpenPRFor(ctx, repo, base) + return err + }) + if err != nil { return "", err } - pr, ok := byHead[base] - if !ok || !prOpen(pr) || slices.ContainsFunc(chain, func(c forge.PullRequest) bool { return c.Head == pr.Head }) { + if !ok || slices.ContainsFunc(chain, func(c forge.PullRequest) bool { return c.Head == pr.Head }) { break } chain = slices.Insert(chain, 0, pr) @@ -311,11 +406,9 @@ func (a *App) getPRs(ctx context.Context, st *syncState, number int, branch stri return "", err } } - ir, err := a.initStack(ctx, repo, InitOptions{Branches: names, Base: trunk}) - if err != nil { + if _, err := a.initStack(ctx, repo, InitOptions{Branches: names, Base: trunk}); err != nil { return "", err } - res.NeedsRestack = ir.NeedsRestack return top.Head, nil } @@ -344,9 +437,9 @@ func prOpen(pr forge.PullRequest) bool { } // getLabel names the target in progress lines. -func getLabel(number int, branch string) string { - if number > 0 { - return fmt.Sprintf("#%d", number) +func getLabel(t getTarget) string { + if t.number > 0 { + return fmt.Sprintf("#%d", t.number) } - return branch + return t.branch } diff --git a/pkg/app/get_test.go b/pkg/app/get_test.go index 095b212..07403c8 100644 --- a/pkg/app/get_test.go +++ b/pkg/app/get_test.go @@ -92,6 +92,11 @@ func TestGetPullRequestInNoStack(t *testing.T) { if !slices.Equal(f.be.Imports, []string{"8"}) { t.Errorf("gh stack was asked for %q", f.be.Imports) } + // The base was found by a lookup for its branch, not in the list of + // recent pull requests. + if !slices.Equal(f.forge.heads, []string{"a"}) || f.forge.lists != 0 { + t.Errorf("looked up heads %q, listed %d times", f.forge.heads, f.forge.lists) + } s, i, ok := f.be.Graph().StackOf("b") if !ok || i != 1 || s.Trunk != "main" || s.Branches[0].Name != "a" { t.Errorf("stack = %+v", s) @@ -111,21 +116,67 @@ func TestGetPullRequestInNoStack(t *testing.T) { // Asking again finds it here and only brings it up to date. tip := f.pushRemote(t, "a", "more.txt") gittest.Run(t, f.dir, "switch", "-q", "main") - res, err = f.get(t, "https://github.com/acme/app/pull/8/files") + res, err = f.get(t, "https://forge.test/o/r/pull/8/files") if err != nil { t.Fatal(err) } - if res.Source != app.GetLocal || res.Branch != "b" || len(res.Created) != 0 || + if res.Source != app.GetLocal || res.Branch != "b" || !res.CheckedOut || len(res.Created) != 0 || len(res.Updated) != 1 || res.Updated[0].Name != "a" || res.Updated[0].To != tip { t.Errorf("again: %+v", res) } + // a moved on, so b is no longer on it. + if !slices.Equal(res.NeedsRestack, []string{"b"}) { + t.Errorf("needs restack after the fast forward: %q", res.NeedsRestack) + } if len(f.be.Imports) != 1 { t.Errorf("a tracked stack went to gh stack: %q", f.be.Imports) } - // b is no longer on a: not get's job to fix, sync's or restack's. if f.current(t) != "b" { t.Errorf("checked out %s", f.current(t)) } + + // A URL for another repository is refused before anything happens. + _, err = f.get(t, "https://github.com/acme/app/pull/8") + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != stack.KindInvalidArgs { + t.Errorf("other repository: %v", err) + } +} + +func TestGetAgainWithNothingCached(t *testing.T) { + f := newGetFixture(t) + // #7 is on main, so nothing lists pull requests and the cache stays + // empty; the second get has to find a by asking the forge about #7. + if res, err := f.get(t, "7"); err != nil || res.Source != app.GetPRs { + t.Fatalf("first: %+v %v", res, err) + } + gittest.Run(t, f.dir, "switch", "-q", "main") + res, err := f.get(t, "7") + if err != nil || res.Source != app.GetLocal || res.Branch != "a" || f.current(t) != "a" { + t.Errorf("again: %+v %v on %s", res, err, f.current(t)) + } + if len(f.be.Imports) != 1 { + t.Errorf("a tracked stack went to gh stack: %q", f.be.Imports) + } +} + +func TestGetCheckoutFailureIsPartialAndUndoable(t *testing.T) { + f := newGetFixture(t) + // An untracked a.txt with other content is in the way of switching to a. + gittest.WriteFile(t, f.dir, "a.txt", "mine") + res, err := f.get(t, "7") + se, ok := errors.AsType[*stack.Error](err) + if !ok || se.Kind != stack.KindPartial || len(se.NextSteps) == 0 { + t.Fatalf("err = %v", err) + } + if res.CheckedOut || res.Branch != "a" || !slices.Equal(res.Created, []string{"a"}) || f.current(t) != "main" { + t.Errorf("result = %+v on %s", res, f.current(t)) + } + if _, _, ok := f.be.Graph().StackOf("a"); !ok { + t.Error("a is not tracked") + } + if h := app.UndoHistory(f.repo); len(h) == 0 || h[0] != "git stack get (keep)" { + t.Errorf("undo history = %q", h) + } } func TestGetByBranchName(t *testing.T) { diff --git a/pkg/app/prs_test.go b/pkg/app/prs_test.go index 8175cfd..86ccdca 100644 --- a/pkg/app/prs_test.go +++ b/pkg/app/prs_test.go @@ -30,6 +30,21 @@ func (f *fakeForge) PR(_ context.Context, _ git.Repo, n int) (forge.PullRequest, return findPR(f.prs, n) } +func (f *fakeForge) OpenPRFor(_ context.Context, _ git.Repo, branch string) (forge.PullRequest, bool, error) { + pr, ok := findOpenPR(f.prs, branch) + return pr, ok, nil +} + +// findOpenPR is the forge's open PR lookup by head over a fake's list. +func findOpenPR(prs []forge.PullRequest, branch string) (forge.PullRequest, bool) { + for _, pr := range prs { + if pr.Head == branch && !pr.Fork && (pr.State == forge.StateOpen || pr.State == forge.StateDraft) { + return pr, true + } + } + return forge.PullRequest{}, false +} + // findPR is the forge's PR lookup over a fake's list. func findPR(prs []forge.PullRequest, n int) (forge.PullRequest, error) { for _, pr := range prs { diff --git a/pkg/app/submit_test.go b/pkg/app/submit_test.go index 3198ec7..7224ff9 100644 --- a/pkg/app/submit_test.go +++ b/pkg/app/submit_test.go @@ -102,6 +102,8 @@ type recordingForge struct { // closed records every ClosePR call; closeErr is what it returns. closed []closeCall closeErr error + // heads records every OpenPRFor branch. + heads []string } type closeCall struct { @@ -126,6 +128,12 @@ func (f *recordingForge) PR(_ context.Context, _ git.Repo, n int) (forge.PullReq return findPR(f.prs, n) } +func (f *recordingForge) OpenPRFor(_ context.Context, _ git.Repo, branch string) (forge.PullRequest, bool, error) { + f.heads = append(f.heads, branch) + pr, ok := findOpenPR(f.prs, branch) + return pr, ok, nil +} + func (f *recordingForge) RenameBranch(context.Context, git.Repo, string, string) error { return nil } func (f *recordingForge) CreateStack(_ context.Context, _ git.Repo, numbers []int) (forge.StackRef, error) { diff --git a/pkg/forge/forge.go b/pkg/forge/forge.go index cdd3057..74616fd 100644 --- a/pkg/forge/forge.go +++ b/pkg/forge/forge.go @@ -116,6 +116,11 @@ type Forge interface { // PR looks up one pull request by number, forks included. One that // doesn't exist is a *stack.Error of kind invalid_args. PR(ctx context.Context, repo git.Repo, number int) (PullRequest, error) + // OpenPRFor finds the open (or draft) pull request whose head is + // branch in this repository, however old it is; false when there is + // none. Pull requests from forks with a branch of the same name don't + // count. + OpenPRFor(ctx context.Context, repo git.Repo, branch string) (PullRequest, bool, error) CreatePR(ctx context.Context, repo git.Repo, in CreatePR) (PullRequest, error) UpdatePR(ctx context.Context, repo git.Repo, number int, in UpdatePR) error // MergeStack merges pull request number together with every open pull diff --git a/pkg/forge/github/github.go b/pkg/forge/github/github.go index e4746d6..3c07f22 100644 --- a/pkg/forge/github/github.go +++ b/pkg/forge/github/github.go @@ -217,6 +217,25 @@ func (f *Forge) PR(ctx context.Context, repo git.Repo, number int) (forge.PullRe return pr, err } +// OpenPRFor asks gh for the open pull requests with branch as their head, +// so it isn't limited to the newest ones the way ListPRs is. +func (f *Forge) OpenPRFor(ctx context.Context, repo git.Repo, branch string) (forge.PullRequest, bool, error) { + res, err := f.gh(ctx, repo, "", "pr", "list", "--state", "open", "--head", branch, "--limit", "20", "--json", prFields) + if err != nil { + return forge.PullRequest{}, false, err + } + var raw []prJSON + if err := json.Unmarshal(res.Stdout, &raw); err != nil { + return forge.PullRequest{}, false, fmt.Errorf("parse gh pr list output: %w", err) + } + for _, p := range raw { + if !p.IsCrossRepository && p.HeadRefName == branch { + return p.toForge(), true, nil + } + } + return forge.PullRequest{}, false, nil +} + // reAlreadyClosed is gh pr close's refusal for a pull request that is // already closed or merged. var reAlreadyClosed = regexp.MustCompile(`(?i)already closed|can't be closed because it was already merged|is already merged`) diff --git a/pkg/forge/github/github_test.go b/pkg/forge/github/github_test.go index 4d9f2ae..eed59df 100644 --- a/pkg/forge/github/github_test.go +++ b/pkg/forge/github/github_test.go @@ -171,3 +171,25 @@ func TestPR(t *testing.T) { t.Errorf("PR 6: %v", err) } } + +func TestOpenPRFor(t *testing.T) { + f := exectest.New() + f.On("gh", "pr", "list").Reply(`[ + {"number":5,"url":"u/5","title":"fork","state":"OPEN","isCrossRepository":true,"headRefName":"feat/api","baseRefName":"main"}, + {"number":12,"url":"u/12","title":"API","state":"OPEN","isDraft":true,"isCrossRepository":false,"headRefName":"feat/api","baseRefName":"main"} +]`) + pr, ok, err := New(f).OpenPRFor(context.Background(), git.Repo{}, "feat/api") + if err != nil || !ok || pr.Number != 12 || pr.State != forge.StateDraft { + t.Errorf("found %+v %v %v", pr, ok, err) + } + args := f.Calls()[0].Args + if !slices.Contains(args, "--head") || !slices.Contains(args, "feat/api") || !slices.Contains(args, "open") { + t.Errorf("args = %q", args) + } + + f = exectest.New() + f.On("gh", "pr", "list").Reply(`[]`) + if _, ok, err := New(f).OpenPRFor(context.Background(), git.Repo{}, "gone"); ok || err != nil { + t.Errorf("none: %v %v", ok, err) + } +} diff --git a/pkg/mcp/server_test.go b/pkg/mcp/server_test.go index b20ff8c..753f135 100644 --- a/pkg/mcp/server_test.go +++ b/pkg/mcp/server_test.go @@ -62,6 +62,14 @@ func (f *fakeForge) PR(_ context.Context, _ git.Repo, n int) (forge.PullRequest, } return forge.PullRequest{}, stack.Newf(stack.KindInvalidArgs, "there is no pull request #%d", n) } +func (f *fakeForge) OpenPRFor(_ context.Context, _ git.Repo, branch string) (forge.PullRequest, bool, error) { + for _, pr := range f.prs { + if pr.Head == branch && !pr.Fork && (pr.State == forge.StateOpen || pr.State == forge.StateDraft) { + return pr, true, nil + } + } + return forge.PullRequest{}, false, nil +} func (f *fakeForge) CreatePR(context.Context, git.Repo, forge.CreatePR) (forge.PullRequest, error) { return forge.PullRequest{}, nil } diff --git a/pkg/mcp/tools.go b/pkg/mcp/tools.go index 7066564..bbd836e 100644 --- a/pkg/mcp/tools.go +++ b/pkg/mcp/tools.go @@ -397,8 +397,9 @@ func (s *Server) get(ctx context.Context, req *mcp.CallToolRequest, in getInput) } res, err := a.Get(ctx, repo, app.GetOptions{Target: in.Target}) if err != nil && res.Branch != "" && errors.Is(err, &stack.Error{Kind: stack.KindPartial}) { - // Checked out, but a branch the remote is ahead of couldn't move: - // keep the result so notUpdated says which. + // Got, but not checked out or a branch the remote is ahead of + // couldn't move: keep the result so checkedOut and notUpdated say + // which. return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: wrapErr(err).Error()}}}, res, nil } if err != nil { @@ -767,7 +768,7 @@ func (s *Server) registerTools() { mcp.AddTool(s.mcp, &mcp.Tool{ Name: "stack_get", Title: "Get a pull request and its stack", - Description: "Pull a pull request down from GitHub and check out its branch, by number, URL or branch name (target). A pull request in a stack on GitHub brings the whole stack; one that isn't comes with the open pull requests it is based on, tracked as a new stack on the first base with none (source says which: stack, prs, or local for a stack already tracked here, which is only brought up to date). Missing branches are created from the remote (created), ones the remote is ahead of are fast forwarded (updated) and diverged ones are left alone (behind); nothing is rebased or pushed. Code partial means it is checked out but a branch in notUpdated couldn't be moved. Refuses with unsupported for a pull request from a fork or one based on a branch already in another local stack, and invalid_args for one that is merged, closed or doesn't exist.", + Description: "Pull a pull request down from GitHub and check out its branch, by number, URL or branch name (target). A pull request in a stack on GitHub brings the whole stack; one that isn't comes with the open pull requests it is based on, tracked as a new stack on the first base with none (source says which: stack, prs, or local for a stack already tracked here, which is only brought up to date). Missing branches are created from the remote (created), ones the remote is ahead of are fast forwarded (updated) and diverged ones are left alone (behind); nothing is rebased or pushed. Code partial means the stack was got but either its branch couldn't be checked out (checkedOut is false; next_steps say why) or a branch in notUpdated couldn't be moved; undo still covers it. A URL for another repository is refused with invalid_args. Refuses with unsupported for a pull request from a fork or one based on a branch already in another local stack, and invalid_args for one that is merged, closed or doesn't exist.", Annotations: &mcp.ToolAnnotations{DestructiveHint: boolp(false), OpenWorldHint: boolp(true)}, }, s.get) mcp.AddTool(s.mcp, &mcp.Tool{ From cd29fee0958d608bdd61ded6269bfeda57e56c05 Mon Sep 17 00:00:00 2001 From: Mo Valipour Date: Wed, 4 Nov 2026 17:12:57 +0000 Subject: [PATCH 3/3] Handle failed imports, closed PRs and missing branches in git stack get - When gh stack checkout fails after saving the imported stack, carry on with the target from that stack so the failed checkout is a partial, undoable result. - Refuse a closed, merged or fork pull request before gh stack sees it. - Check every branch is on the remote before creating any for a lone pull request, and delete the ones created if tracking them fails. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +- docs/architecture.md | 8 ++- pkg/app/get.go | 127 ++++++++++++++++++++++++++++++++++--------- pkg/app/get_test.go | 42 ++++++++++++++ 4 files changed, 151 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index e36a2c6..125eb64 100644 --- a/README.md +++ b/README.md @@ -616,8 +616,8 @@ Nothing is rebased or pushed, and a URL for another repository is refused. If th checked out (files in the way, or it's checked out in another worktree) the stack is still got and you're told why. A branch that has diverged from the remote is left as it is with a warning, the same as `sync` does. Pull requests from forks can't be got this way (`gh pr checkout` -checks one out without tracking it), and neither can one based on a branch that's already in -another of your stacks. +checks one out without tracking it), and neither can a closed or merged one, or one based on a +branch that's already in another of your stacks. ### `git stack pr [branch]` diff --git a/docs/architecture.md b/docs/architecture.md index dfc784b..387669b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -214,7 +214,13 @@ list of recent pull requests sync uses stops at 500), creates those branches fro the graph, because a stack git-stack tracked itself has no PR records until gh stack submits it. A target that's already tracked here never reaches gh stack, since its composition check would refuse a stack reordered locally but not yet submitted. Once branches have moved, a failed -checkout is a partial result rather than an error, so undo still records it. A URL is checked +checkout is a partial result rather than an error, so undo still records it. That includes gh stack +failing after it saved the stack (it creates the branches and saves before it checks out): `get` +finds the target in the newly saved stack and carries on, and its own checkout reports the failure. +A pull request looked up through the forge is refused when closed, merged or from a fork before +gh stack sees it, as gh stack would take a merged one still in a stack with open ones. The lone pull +request path checks every branch is on the remote before creating any, and deletes the ones it +created if tracking them fails. A URL is checked against the repository the forge works out from the remotes, as its number means a different pull request anywhere else. Forks are refused rather than guessed at: their branches aren't on our remote, and submit couldn't push them back anyway. diff --git a/pkg/app/get.go b/pkg/app/get.go index 313d360..9519eb6 100644 --- a/pkg/app/get.go +++ b/pkg/app/get.go @@ -160,7 +160,15 @@ func (a *App) get(ctx context.Context, repo git.Repo, o GetOptions) (GetResult, if target != "" { res.Source = GetLocal } else { - if target, err = a.importTarget(ctx, repo, t); err != nil { + // Refuse what can't be got before gh stack sees it: it would take a + // merged pull request whose branch is still there, in a stack with + // open ones. + if pr != nil { + if err := gettable(*pr); err != nil { + return res, err + } + } + if target, err = a.importTarget(ctx, repo, st.graph, t, pr); err != nil { return res, err } if target != "" { @@ -306,8 +314,13 @@ func (a *App) trackedTarget(ctx context.Context, repo git.Repo, graph *stack.Gra // importTarget pulls the target's stack down from GitHub and returns the // branch it checked out, or "" when the target is in no stack there (or -// the repository doesn't have stacks), for getPRs to handle. -func (a *App) importTarget(ctx context.Context, repo git.Repo, t getTarget) (string, error) { +// the repository doesn't have stacks), for getPRs to handle. gh stack +// creates the branches and saves the stack before it checks the branch +// out, so when it fails after that the target is taken from the stack it +// saved: get carries on, and its own checkout reports the failure as a +// partial result that undo can take back. before is the graph from before +// the import; pr is the pull request when trackedTarget looked it up. +func (a *App) importTarget(ctx context.Context, repo git.Repo, before *stack.Graph, t getTarget, pr *forge.PullRequest) (string, error) { if a.d.Tracker == nil { return "", nil } @@ -323,11 +336,47 @@ func (a *App) importTarget(ctx context.Context, repo git.Repo, t getTarget) (str return "", nil } if err != nil { + if target := a.importedTarget(ctx, repo, before, t, pr); target != "" { + a.d.Log.Debug("gh stack failed after importing the stack", "target", target, "err", err) + return target, nil + } return "", err } return a.d.Git.CurrentBranch(ctx, repo) } +// importedTarget is the branch a failed import was getting when it saved a +// new stack holding it (one none of whose branches was tracked before), +// else "". A stack number names the stack's top open branch, the one gh +// stack checks out. +func (a *App) importedTarget(ctx context.Context, repo git.Repo, before *stack.Graph, t getTarget, pr *forge.PullRequest) string { + after, err := a.d.Meta.Load(ctx, repo) + if err != nil { + return "" + } + for _, s := range after.Stacks { + if slices.ContainsFunc(s.Branches, func(b stack.Branch) bool { + _, _, ok := before.StackOf(b.Name) + return ok + }) { + continue + } + switch { + case pr != nil && s.Index(pr.Head) >= 0: + return pr.Head + case t.number == 0 && s.Index(t.branch) >= 0: + return t.branch + case pr == nil && t.number > 0 && s.Number == t.number: + for _, b := range slices.Backward(s.Branches) { + if !b.Merged() { + return b.Name + } + } + } + } + return "" +} + // getPRs tracks a pull request that is in no stack on GitHub, together // with the open pull requests it is based on, as a new stack on the first // base that has none. top is the pull request when trackedTarget already @@ -361,12 +410,8 @@ func (a *App) getPRs(ctx context.Context, st *syncState, t getTarget, top *forge } top = &pr } - if top.Fork { - return "", stack.Newf(stack.KindUnsupported, "#%d comes from a fork; git stack get only pulls branches from this repository", top.Number). - WithSteps(fmt.Sprintf("gh pr checkout %d checks it out without tracking it", top.Number)) - } - if !prOpen(*top) { - return "", stack.Newf(stack.KindInvalidArgs, "#%d is %s, so there's nothing to get", top.Number, top.State) + if err := gettable(*top); err != nil { + return "", err } // Walk down through the open pull requests it is based on. @@ -401,34 +446,62 @@ func (a *App) getPRs(ctx context.Context, st *syncState, t getTarget, top *forge for i, pr := range chain { names[i] = pr.Head } - for _, name := range append([]string{trunk}, names...) { - if err := a.ensureLocal(ctx, st, name); err != nil { + // Check every branch is on the remote before creating any, so a missing + // one leaves nothing behind. + all := append([]string{trunk}, names...) + var missing []string + for _, name := range all { + if _, ok := st.local[name]; ok { + continue + } + _, ok, err := a.d.Git.Tip(ctx, repo, "refs/remotes/"+st.remote+"/"+name) + if err != nil { return "", err } + if !ok { + return "", stack.Newf(stack.KindInvalidArgs, "%s isn't on %s", name, st.remote). + WithSteps("it may have been deleted; check the pull request on GitHub") + } + missing = append(missing, name) } - if _, err := a.initStack(ctx, repo, InitOptions{Branches: names, Base: trunk}); err != nil { + var created []string + err := func() error { + for _, name := range missing { + if err := a.d.Git.CreateTrackingBranch(ctx, repo, name, st.remote); err != nil { + return err + } + created = append(created, name) + } + if err := a.loadCheckouts(ctx, st); err != nil { + return err + } + _, err := a.initStack(ctx, repo, InitOptions{Branches: names, Base: trunk}) + return err + }() + if err != nil { + // Nothing is tracked yet, so this run's branches can simply go. The + // context may be what failed, so don't let it stop the cleanup. + for _, name := range created { + if derr := a.d.Git.DeleteBranch(context.WithoutCancel(ctx), repo, name); derr != nil { + a.d.Log.Warn("could not delete a branch git stack get created", "branch", name, "err", derr) + } + } return "", err } return top.Head, nil } -// ensureLocal creates name from the remote's copy when there's no local -// branch of that name; one that exists is left for syncRemote to bring up -// to date. -func (a *App) ensureLocal(ctx context.Context, st *syncState, name string) error { - if _, ok := st.local[name]; ok { - return nil - } - if _, ok, err := a.d.Git.Tip(ctx, st.repo, "refs/remotes/"+st.remote+"/"+name); err != nil { - return err - } else if !ok { - return stack.Newf(stack.KindInvalidArgs, "%s isn't on %s", name, st.remote). - WithSteps("it may have been deleted; check the pull request on GitHub") +// gettable refuses a pull request get can't pull down: one from a fork, or +// one that is closed or merged. +func gettable(pr forge.PullRequest) error { + if pr.Fork { + return stack.Newf(stack.KindUnsupported, "#%d comes from a fork; git stack get only pulls branches from this repository", pr.Number). + WithSteps(fmt.Sprintf("gh pr checkout %d checks it out without tracking it", pr.Number)) } - if err := a.d.Git.CreateTrackingBranch(ctx, st.repo, name, st.remote); err != nil { - return err + if !prOpen(pr) { + return stack.Newf(stack.KindInvalidArgs, "#%d is %s, so there's nothing to get", pr.Number, pr.State) } - return a.loadCheckouts(ctx, st) + return nil } // prOpen reports whether a pull request can still be stacked on. diff --git a/pkg/app/get_test.go b/pkg/app/get_test.go index 07403c8..91b63f0 100644 --- a/pkg/app/get_test.go +++ b/pkg/app/get_test.go @@ -255,6 +255,8 @@ func TestGetRefusals(t *testing.T) { forge.PullRequest{Number: 9, Head: "x", Base: "main", State: forge.StateOpen, Fork: true}, forge.PullRequest{Number: 10, Head: "y", Base: "main", State: forge.StateMerged}, forge.PullRequest{Number: 11, Head: "gone", Base: "main", State: forge.StateOpen}, + // Based on #7, whose branch a is on origin, but c isn't. + forge.PullRequest{Number: 14, Head: "c", Base: "a", State: forge.StateOpen}, ) for target, kind := range map[string]stack.Kind{ "": stack.KindInvalidArgs, @@ -263,6 +265,7 @@ func TestGetRefusals(t *testing.T) { "10": stack.KindInvalidArgs, "12": stack.KindInvalidArgs, "11": stack.KindInvalidArgs, + "14": stack.KindInvalidArgs, "nope": stack.KindInvalidArgs, "#9999": stack.KindInvalidArgs, } { @@ -274,6 +277,45 @@ func TestGetRefusals(t *testing.T) { if len(f.be.Graph().Stacks) != 0 || f.current(t) != "main" { t.Errorf("a refusal changed something: %+v on %s", f.be.Graph().Stacks, f.current(t)) } + if got := gittest.Run(t, f.dir, "branch", "--format=%(refname:short)"); got != "main" { + t.Errorf("a refusal left branches behind: %q", got) + } + // gh stack never saw the fork or the merged pull request. + if slices.Contains(f.be.Imports, "9") || slices.Contains(f.be.Imports, "10") { + t.Errorf("imports = %q", f.be.Imports) + } +} + +func TestGetFailedImportCheckoutIsPartial(t *testing.T) { + f := newGetFixture(t) + // gh stack creates the branches and saves the stack, then fails to + // check a out because an untracked a.txt is in the way. + gittest.WriteFile(t, f.dir, "a.txt", "mine") + f.be.ImportFn = func(target string) error { + ctx := context.Background() + g := git.New(exec.New()) + for _, name := range []string{"a", "b"} { + if err := g.CreateTrackingBranch(ctx, f.repo, name, "origin"); err != nil { + return err + } + } + f.be.SetGraph(stack.NewGraph([]stack.Stack{{Trunk: "main", Number: 3, Branches: []stack.Branch{ + {Name: "a", PR: &stack.PRRef{Number: 7}}, {Name: "b", PR: &stack.PRRef{Number: 8}}, + }}})) + return stack.New(stack.KindUnknown, "gh stack checkout failed") + } + res, err := f.get(t, "7") + se, ok := errors.AsType[*stack.Error](err) + if !ok || se.Kind != stack.KindPartial { + t.Fatalf("err = %v", err) + } + if res.Source != app.GetStack || res.CheckedOut || res.Branch != "a" || + !slices.Equal(res.Created, []string{"a", "b"}) || f.current(t) != "main" { + t.Errorf("result = %+v on %s", res, f.current(t)) + } + if h := app.UndoHistory(f.repo); len(h) == 0 || h[0] != "git stack get (keep)" { + t.Errorf("undo history = %q", h) + } } func TestGetUndo(t *testing.T) {