diff --git a/cmd/add.go b/cmd/add.go index ddcaa568..f097a970 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -13,6 +13,7 @@ import ( ) type addOptions struct { + fork bool stageAll bool stageTracked bool message string @@ -26,6 +27,9 @@ func AddCmd(cfg *config.Config) *cobra.Command { Short: "Add a new branch on top of the current stack", Long: `Add a new branch on top of the current stack. +Use --fork to start a separate stack based on the current branch instead. +When run in the middle of a stack, offers to fork in an interactive terminal. + When -m is omitted but -A or -u is used, your editor opens for the commit message. When -m is provided without an explicit branch name, the branch name is auto-generated from the commit message.`, @@ -37,6 +41,9 @@ the branch name is auto-generated from the commit message.`, # Auto-generate branch name from the commit message $ gh stack add -m "Fix login bug" + + # Start a separate stack based on the current branch + $ gh stack add --fork parallel-feature # Add a branch and open editor to write commit message $ gh stack add -A my-feature`, @@ -49,6 +56,7 @@ the branch name is auto-generated from the commit message.`, cmd.Flags().BoolVarP(&opts.stageAll, "all", "A", false, "Stage all changes including untracked files") cmd.Flags().BoolVarP(&opts.stageTracked, "update", "u", false, "Stage changes to tracked files only") cmd.Flags().StringVarP(&opts.message, "message", "m", "", "Create a commit with this message") + cmd.Flags().BoolVar(&opts.fork, "fork", false, "Start a new stack based on the current branch") return cmd } @@ -73,13 +81,35 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { return err } } - result, err := loadStackOptional(cfg, "") + var result *loadStackResult + if opts.fork { + // A fork needs the current branch, not a selected stack or a checkout. + dir, err := stackStateDir(cfg) + if err != nil { + return err + } + sf, err := stack.Load(dir) + if err != nil { + return fmt.Errorf("loading stack state: %w", err) + } + current, err := git.CurrentBranch() + if err != nil || current == "" { + if err != nil { + cfg.Errorf("could not determine the current branch: %s", err) + } + cfg.Errorf("check out a branch before starting a new stack") + return ErrInvalidArgs + } + result = &loadStackResult{GitDir: dir, StackFile: sf, CurrentBranch: current} + } else { + result, err = loadStackOptional(cfg, "") + } if err != nil { return stackLookupError(err) } gitDir := result.GitDir - if result.Stack == nil { + if result.Stack == nil && !opts.fork { branchName, err := addBranchNameFromArgs(cfg, opts, args) if err != nil { return err @@ -90,27 +120,59 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { sf := result.StackFile s := result.Stack currentBranch := result.CurrentBranch + fork := opts.fork - if s.IsFullyMerged() { + if !fork && s.IsFullyMerged() { cfg.Warningf("All branches in this stack have been merged") cfg.Printf("Consider creating a new stack with `%s`", cfg.ColorCyan("gh stack init")) return nil } - idx := s.IndexOf(currentBranch) + idx := -1 + if s != nil { + idx = s.IndexOf(currentBranch) + } // idx < 0 means we're on the trunk — that's allowed (we'll create // a new branch from it). Only block if we're in the middle of the stack. - if idx >= 0 && idx < len(s.Branches)-1 { - cfg.Errorf("can only add branches to the top of the stack; run `%s` then `%s`", cfg.ColorCyan("gh stack top"), cfg.ColorCyan("gh stack add")) - cfg.Printf("Or to restructure your stack and insert a branch, use `%s`", cfg.ColorCyan("gh stack modify")) - return ErrInvalidArgs + if !fork && idx >= 0 && idx < len(s.Branches)-1 { + if cfg.IsInteractive() { + cfg.Infof("%s is in the middle of the stack%s", currentBranch, linkedStackLabel(cfg, s.Number)) + prompt := "Start a new stack based on " + currentBranch + "?" + var promptErr error + if cfg.ConfirmFn != nil { + fork, promptErr = cfg.ConfirmFn(prompt, true) + } else { + fork, promptErr = prompter.New(cfg.In, cfg.Out, cfg.Err).Confirm(prompt, true) + } + if promptErr != nil { + if !isInterruptError(promptErr) { + return fmt.Errorf("reading fork confirmation: %w", promptErr) + } + printInterrupt(cfg) + fork = false + } + } + if !fork { + cfg.Errorf("can only add branches to the top of the stack; run `%s` then `%s`", cfg.ColorCyan("gh stack top"), cfg.ColorCyan("gh stack add")) + cfg.Printf("Or to restructure your stack and insert a branch, use `%s`", cfg.ColorCyan("gh stack modify")) + cfg.Printf("To start a separate stack here, use `%s`", cfg.ColorCyan("gh stack add --fork ")) + return ErrInvalidArgs + } + } + if fork { + for _, parent := range sf.StacksForBranch(currentBranch) { + if i := parent.IndexOf(currentBranch); i >= 0 && parent.Branches[i].IsMerged() { + cfg.Errorf("branch %q has already merged; check out its updated base before starting a new stack", currentBranch) + return ErrInvalidArgs + } + } } // Check if the current branch is a stack branch with no unique commits // relative to its parent. If so, the commit should land on this branch // without creating a new one (e.g., right after init). var branchIsEmpty bool - if wantsCommit && idx >= 0 { + if !fork && wantsCommit && idx >= 0 { parentBranch := s.ActiveBaseBranch(currentBranch) shas, err := git.RevParseMulti([]string{parentBranch, currentBranch}) if err == nil { @@ -143,6 +205,10 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { return err } if branchName == "" { + if fork && !cfg.IsInteractive() { + cfg.Errorf("provide a branch name or -m when using --fork without an interactive terminal") + return ErrInvalidArgs + } // No -m and no explicit name — prompt for one. for { input, err := promptInput(cfg, "Enter a name for the new branch:") @@ -167,6 +233,14 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { return ErrInvalidArgs } + if err := git.ValidateRefName(branchName); err != nil { + cfg.Errorf("invalid branch name %q: must be a valid git ref", branchName) + return ErrInvalidArgs + } + if fork && branchName == currentBranch { + cfg.Errorf("the new stack's branch must differ from its base %q", currentBranch) + return ErrInvalidArgs + } if err := sf.ValidateNoDuplicateBranch(branchName); err != nil { cfg.Errorf("branch %q already exists in the stack", branchName) return ErrInvalidArgs @@ -189,9 +263,21 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { cfg.Errorf("commit and staging flags cannot be used when adopting a branch checked out in another worktree") return ErrInvalidArgs } - var adoptedBase string + var adoptedBase, trunkSHA string + baseRef := currentBranch + if fork { + trunkSHA, err = git.RevParse("refs/heads/" + currentBranch) + if err != nil { + return fmt.Errorf("resolving the new stack's base %s: %w", currentBranch, err) + } + baseRef = trunkSHA + } if adopted { - adoptedBase, err = git.MergeBase(currentBranch, branchName) + branchRef := branchName + if fork { + branchRef = "refs/heads/" + branchName + } + adoptedBase, err = git.MergeBase(baseRef, branchRef) if err != nil { cfg.Errorf("failed to determine the common base of %s and %s: %s", currentBranch, branchName, err) return ErrSilent @@ -208,7 +294,7 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { if !adopted { // Create the new branch from the current HEAD and check it out - if err := git.CreateBranch(branchName, currentBranch); err != nil { + if err := git.CreateBranch(branchName, baseRef); err != nil { cfg.Errorf("failed to create branch: %s", err) return ErrSilent } @@ -227,12 +313,23 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { base := adoptedBase if !adopted { - base, err = git.RevParse(currentBranch) - if err != nil { - cfg.Warningf("could not resolve base SHA for %s: %s", currentBranch, err) + base = trunkSHA + if !fork { + base, err = git.RevParse(currentBranch) + if err != nil { + cfg.Warningf("could not resolve base SHA for %s: %s", currentBranch, err) + } } } - s.Branches = append(s.Branches, stack.BranchRef{Branch: branchName, Base: base}) + if fork { + sf.AddStack(stack.Stack{ + Trunk: stack.BranchRef{Branch: currentBranch, Head: trunkSHA}, + Branches: []stack.BranchRef{{Branch: branchName, Base: base}}, + }) + s = &sf.Stacks[len(sf.Stacks)-1] + } else { + s.Branches = append(s.Branches, stack.BranchRef{Branch: branchName, Base: base}) + } // Commit on the NEW branch (staging already done above) var commitSHA string @@ -250,6 +347,9 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error { } // Print summary + if fork { + cfg.Successf("Started a new stack based on %s", cfg.ColorBold(currentBranch)) + } position := len(s.Branches) if adopted { if commitSHA != "" { diff --git a/cmd/add_test.go b/cmd/add_test.go index 2c9b5453..eaeeeb26 100644 --- a/cmd/add_test.go +++ b/cmd/add_test.go @@ -220,6 +220,265 @@ func TestAdd_CreatesNewBranch(t *testing.T) { assert.Equal(t, "newbranch", names[len(names)-1], "top branch") } +func TestAdd_Fork(t *testing.T) { + for _, tt := range []struct { + name, current string + opts addOptions + adopted, foreign, generate bool + noCatalog, prompt bool + }{ + {name: "middle", current: "b1"}, + {name: "top", current: "b2"}, + {name: "shared trunk", current: "main"}, + {name: "untracked", current: "outside"}, + {name: "first stack", current: "main", noCatalog: true}, + {name: "prompted name", current: "b1", prompt: true}, + {name: "adopt", current: "b1", adopted: true}, + {name: "foreign adoption", current: "b1", adopted: true, foreign: true}, + {name: "stage all", current: "b1", opts: addOptions{stageAll: true, message: "New layer"}}, + {name: "stage tracked", current: "b1", opts: addOptions{stageTracked: true, message: "New layer"}}, + {name: "staged commit", current: "b1", opts: addOptions{message: "New layer"}}, + {name: "editor", current: "b1", opts: addOptions{stageAll: true}}, + {name: "generated name", current: "b1", opts: addOptions{message: "New layer"}, generate: true}, + {name: "adopt and commit", current: "b1", opts: addOptions{message: "New layer"}, adopted: true}, + } { + t.Run(tt.name, func(t *testing.T) { + dir, owner := t.TempDir(), t.TempDir() + if !tt.noCatalog { + writeStackFileMulti(t, dir, + stack.Stack{Trunk: stack.BranchRef{Branch: "b1"}, Branches: []stack.BranchRef{{Branch: "old-child"}}}, + stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}}, + stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "other"}}}, + ) + } + before, err := stack.Load(dir) + require.NoError(t, err) + current := tt.current + name := "new-child" + args := []string{name} + if tt.generate { + args = nil + name = time.Now().Format("01-02") + "-new_layer" + } else if tt.prompt { + args = nil + } + var staged, committed, created bool + commit := func() (string, error) { + assert.Equal(t, name, current, "never commit on the parent, even when it is empty") + committed = true + return "commit", nil + } + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return dir, nil }, + CurrentBranchFn: func() (string, error) { return current, nil }, + BranchExistsFn: func(branch string) (bool, error) { return tt.adopted && branch == name, nil }, + RevParseFn: func(ref string) (string, error) { + assert.Equal(t, "refs/heads/"+tt.current, ref) + return "parent-sha", nil + }, + RevParseMultiFn: func([]string) ([]string, error) { + t.Fatal("fork must bypass the empty-layer shortcut") + return nil, nil + }, + MergeBaseFn: func(base, branch string) (string, error) { + assert.Equal(t, "parent-sha", base) + assert.Equal(t, "refs/heads/"+name, branch) + return "common-base", nil + }, + WorktreesFn: func() ([]git.Worktree, error) { + if tt.foreign { + return []git.Worktree{{Path: owner, Branch: name}}, nil + } + return nil, nil + }, + CreateBranchFn: func(branch, base string) error { + assert.Equal(t, name, branch) + assert.Equal(t, "parent-sha", base) + created = true + return nil + }, + CheckoutBranchFn: func(branch string) error { current = branch; return nil }, + StageAllFn: func() error { staged = true; return nil }, + StageTrackedFn: func() error { staged = true; return nil }, + HasStagedChangesFn: func() (bool, error) { return true, nil }, + CommitFn: func(string) (string, error) { return commit() }, + CommitInteractiveFn: commit, + }) + defer restore() + cfg, outR, errR := config.NewTestConfig() + cfg.ForceInteractive = tt.prompt + cfg.InputFn = func(string) (string, error) { + assert.True(t, tt.prompt) + return name, nil + } + cfg.ConfirmFn = func(string, bool) (bool, error) { + t.Fatal("explicit fork does not require confirmation") + return false, nil + } + cfg.SelectFn = func(string, string, []string) (int, error) { + t.Fatal("explicit fork must not select an existing stack") + return 0, nil + } + cfg.GitHubClientOverride = &github.MockClient{FindPRForBranchFn: func(string) (*github.PullRequest, error) { + t.Fatal("fork is local-only") + return nil, nil + }} + tt.opts.fork = true + require.NoError(t, runAdd(cfg, &tt.opts, args)) + _, output := commandOutput(t, cfg, outR, errR) + after, err := stack.Load(dir) + require.NoError(t, err) + require.Len(t, after.Stacks, len(before.Stacks)+1) + assert.Equal(t, before.Stacks, after.Stacks[:len(before.Stacks)]) + child := after.Stacks[len(before.Stacks)] + assert.Equal(t, stack.BranchRef{Branch: tt.current, Head: "parent-sha"}, child.Trunk) + assert.Equal(t, []string{name}, child.BranchNames()) + base := "parent-sha" + if tt.adopted { + base = "common-base" + } + assert.Equal(t, base, child.Branches[0].Base) + assert.Equal(t, !tt.adopted, created) + assert.Equal(t, tt.opts.stageAll || tt.opts.stageTracked, staged) + assert.Equal(t, tt.opts.message != "" || staged, committed) + if tt.foreign { + assert.Equal(t, tt.current, current) + assert.Contains(t, output, owner) + } else { + assert.Equal(t, name, current) + } + assert.Contains(t, output, "Started a new stack based on "+tt.current) + }) + } +} + +func TestAdd_MidStackForkPrompt(t *testing.T) { + for _, mode := range []string{"yes", "no", "interrupt", "error", "noninteractive"} { + t.Run(mode, func(t *testing.T) { + dir := t.TempDir() + saveStack(t, dir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + }) + confirmed, created := false, false + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return dir, nil }, + CurrentBranchFn: func() (string, error) { return "b1", nil }, + CreateBranchFn: func(string, string) error { + assert.Equal(t, "yes", mode) + created = true + return nil + }, + }) + defer restore() + cfg, outR, errR := config.NewTestConfig() + cfg.ForceInteractive = mode != "noninteractive" + cfg.ConfirmFn = func(prompt string, def bool) (bool, error) { + assert.True(t, def) + assert.Contains(t, prompt, "b1") + assert.NotEqual(t, "noninteractive", mode) + confirmed = true + if mode == "interrupt" { + return false, terminal.InterruptErr + } + if mode == "error" { + return false, fmt.Errorf("prompt failed") + } + return mode == "yes", nil + } + cfg.InputFn = func(string) (string, error) { + assert.True(t, confirmed, "confirm the fork before asking for a name") + assert.Equal(t, "yes", mode) + return "child", nil + } + err := runAdd(cfg, &addOptions{}, nil) + _, output := commandOutput(t, cfg, outR, errR) + sf, loadErr := stack.Load(dir) + require.NoError(t, loadErr) + if mode == "yes" { + require.NoError(t, err) + require.Len(t, sf.Stacks, 2) + assert.Equal(t, "b1", sf.Stacks[1].Trunk.Branch) + } else { + require.Error(t, err) + assert.Len(t, sf.Stacks, 1) + if mode != "error" { + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "gh stack add --fork") + assert.Contains(t, output, "gh stack modify") + } + } + assert.Equal(t, mode == "yes", created) + }) + } +} + +func TestAdd_ForkRefusals(t *testing.T) { + for _, mode := range []string{"merged", "duplicate", "self", "invalid name", "missing name", "detached", "base lookup", "foreign commit", "nothing staged"} { + t.Run(mode, func(t *testing.T) { + dir, owner := t.TempDir(), t.TempDir() + s := stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "parent"}}} + if mode == "merged" { + s.Branches[0].PullRequest = &stack.PullRequestRef{Number: 1, Merged: true} + } + saveStack(t, dir, s) + opts := &addOptions{fork: true} + args := []string{"child"} + switch mode { + case "duplicate": + args[0] = "main" + case "self": + args[0] = "parent" + case "invalid name": + args[0] = "invalid name" + case "missing name": + args = nil + case "foreign commit", "nothing staged": + opts.message = "commit" + } + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return dir, nil }, + ValidateRefNameFn: func(string) error { + if mode == "invalid name" { + return fmt.Errorf("invalid ref") + } + return nil + }, + CurrentBranchFn: func() (string, error) { + if mode == "detached" { + return "", fmt.Errorf("detached HEAD") + } + return "parent", nil + }, + RevParseFn: func(string) (string, error) { + if mode == "base lookup" { + return "", fmt.Errorf("base lookup failed") + } + return "parent-sha", nil + }, + WorktreesFn: func() ([]git.Worktree, error) { + if mode == "foreign commit" { + return []git.Worktree{{Path: owner, Branch: "child"}}, nil + } + return nil, nil + }, + CreateBranchFn: func(string, string) error { t.Fatal("must not create a branch"); return nil }, + CheckoutBranchFn: func(string) error { t.Fatal("must not change checkout"); return nil }, + StageAllFn: func() error { t.Fatal("must not stage"); return nil }, + CommitFn: func(string) (string, error) { t.Fatal("must not commit"); return "", nil }, + }) + defer restore() + cfg, outR, errR := config.NewTestConfig() + cfg.InputFn = func(string) (string, error) { t.Fatal("must not prompt"); return "", nil } + require.Error(t, runAdd(cfg, opts, args)) + _, output := commandOutput(t, cfg, outR, errR) + assert.NotContains(t, output, "Started a new stack") + sf, err := stack.Load(dir) + require.NoError(t, err) + assert.Equal(t, []stack.Stack{s}, sf.Stacks) + }) + } +} + func TestAdd_OnlyAllowedOnTopOfStack(t *testing.T) { gitDir := t.TempDir() saveStack(t, gitDir, stack.Stack{ diff --git a/cmd/checkout.go b/cmd/checkout.go index 64675b9b..4331e420 100644 --- a/cmd/checkout.go +++ b/cmd/checkout.go @@ -137,7 +137,7 @@ func runCheckout(cfg *config.Config, opts *checkoutOptions) error { } } else { // Non-numeric target — resolve locally before checking GitHub. - if len(sf.FindAllStacksForBranch(opts.target)) > 1 { + if len(sf.StacksForBranch(opts.target)) > 1 { cfg.Errorf("branch %q belongs to multiple stacks; use a stack or PR number to choose one", opts.target) return ErrDisambiguate } @@ -225,7 +225,7 @@ func resolveNumericTarget(cfg *config.Config, sf *stack.StackFile, gitDir string remoteErr := err // 4. Fall back to local branch name lookup (handles numeric branch names). - stacks := sf.FindAllStacksForBranch(raw) + stacks := sf.StacksForBranch(raw) if len(stacks) > 1 { cfg.Errorf("branch %q belongs to multiple stacks; use a stack or PR number to choose one", raw) return nil, "", ErrDisambiguate diff --git a/cmd/checkout_test.go b/cmd/checkout_test.go index 7c3fcd1a..0c94355d 100644 --- a/cmd/checkout_test.go +++ b/cmd/checkout_test.go @@ -123,6 +123,32 @@ func TestCheckout_PrintPathAmbiguousBranch(t *testing.T) { assert.Contains(t, diagnostics, "multiple stacks") } +func TestCheckout_NumericMemberBranch(t *testing.T) { + dir := t.TempDir() + writeStackFileMulti(t, dir, + stack.Stack{Trunk: stack.BranchRef{Branch: "123"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "123"}}}, + ) + var checkedOut string + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return dir, nil }, + BranchExistsFn: func(string) (bool, error) { return true, nil }, + CheckoutBranchFn: func(branch string) error { + checkedOut = branch + return nil + }, + }) + defer restore() + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(int) (*github.RemoteStack, error) { return nil, notFoundErr() }, + } + require.NoError(t, runCheckout(cfg, &checkoutOptions{target: "123"})) + assert.Equal(t, "123", checkedOut) + _, diagnostics := commandOutput(t, cfg, outR, errR) + assert.Contains(t, diagnostics, "(main) <- 123") +} + func TestCheckout_RemotePrintPathDoesNotImportDuringRecovery(t *testing.T) { t.Setenv("GH_STACK_HYPERLINKS", "1") common, root, owner := t.TempDir(), t.TempDir(), t.TempDir() diff --git a/cmd/init.go b/cmd/init.go index 35af9ea9..63c00639 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -116,6 +116,7 @@ func runInit(cfg *config.Config, opts *initOptions) error { for _, s := range sf.FindAllStacksForBranch(currentBranch) { if s.IndexOf(currentBranch) >= 0 { cfg.Errorf("current branch %q is already part of a stack", currentBranch) + cfg.Printf("To start a separate stack based on it, use `%s`", cfg.ColorCyan("gh stack add --fork ")) return ErrInvalidArgs } } diff --git a/cmd/merge.go b/cmd/merge.go index 306c1f33..b8415625 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -261,7 +261,7 @@ func resolveActiveRemoteStack(cfg *config.Config, client github.ClientOps) (*git return nil, ErrNotInStack } - stacks := sf.FindAllStacksForBranch(currentBranch) + stacks := sf.StacksForBranch(currentBranch) if len(stacks) == 0 { cfg.Errorf("current branch %q is not part of a stack", currentBranch) cfg.Printf("Checkout a stack first, or specify which stack or pull request to merge with `%s`", cfg.ColorCyan("gh stack merge [number]")) diff --git a/cmd/merge_test.go b/cmd/merge_test.go index 61a21666..ee0d7056 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -77,6 +77,30 @@ func setupLocalStack(t *testing.T, number int, currentBranch string, branches .. }) } +func TestResolveActiveRemoteStack_MemberBeforeTrunk(t *testing.T) { + dir := t.TempDir() + writeStackFileMulti(t, dir, + stack.Stack{Number: 8, Trunk: stack.BranchRef{Branch: "parent"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + stack.Stack{Number: 7, Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "parent"}}}, + ) + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return dir, nil }, + CurrentBranchFn: func() (string, error) { return "parent", nil }, + }) + defer restore() + cfg, outR, errR := config.NewTestConfig() + expected := remoteStack(7, "main", openStackPR(1, "parent")) + resolved, err := resolveActiveRemoteStack(cfg, &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + assert.Equal(t, 7, n) + return expected, nil + }, + }) + require.NoError(t, err) + assert.Same(t, expected, resolved) + commandOutput(t, cfg, outR, errR) +} + func TestResolveMergeStack_FetchFailureLinks(t *testing.T) { for _, mode := range []string{"explicit", "active"} { for _, force := range []string{"0", "1"} { diff --git a/cmd/modify.go b/cmd/modify.go index c4b6d53e..6c1d83ec 100644 --- a/cmd/modify.go +++ b/cmd/modify.go @@ -9,6 +9,7 @@ import ( "github.com/github/gh-stack/internal/config" "github.com/github/gh-stack/internal/git" "github.com/github/gh-stack/internal/modify" + "github.com/github/gh-stack/internal/stack" "github.com/github/gh-stack/internal/tui/modifyview" "github.com/github/gh-stack/internal/tui/stackview" "github.com/spf13/cobra" @@ -152,6 +153,9 @@ func runModify(cfg *config.Config) error { return fmt.Errorf("the current branch changed while modify was open; reopen modify before applying") } + if err := validateModifyDependents(cfg, sf, s, reordered); err != nil { + return err + } applyResult, conflict, applyErr := modify.ApplyPlan(cfg, gitDir, s, sf, reordered, currentBranch, updateBaseSHAs) if conflict != nil { @@ -192,6 +196,29 @@ func runModify(cfg *config.Config) error { return nil } +func validateModifyDependents(cfg *config.Config, sf *stack.StackFile, s *stack.Stack, nodes []modifyview.ModifyBranchNode) error { + for _, n := range nodes { + if n.PendingAction == nil { + continue + } + switch n.PendingAction.Type { + case modifyview.ActionRename, modifyview.ActionDrop, modifyview.ActionFoldDown, modifyview.ActionFoldUp: + default: + continue + } + for i := range sf.Stacks { + dependent := &sf.Stacks[i] + if dependent != s && dependent.Trunk.Branch == n.Ref.Branch { + cfg.Errorf("cannot %s branch %q: it is the base of another local stack%s", + n.PendingAction.Type, n.Ref.Branch, linkedStackLabel(cfg, dependent.Number)) + cfg.Printf("Dependent stack: %s", dependent.DisplayChain()) + return ErrInvalidArgs + } + } + } + return nil +} + // printModifySuccess prints a summary of what was applied. func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult) { if result == nil { diff --git a/cmd/modify_test.go b/cmd/modify_test.go index a0a232b8..547d81a8 100644 --- a/cmd/modify_test.go +++ b/cmd/modify_test.go @@ -476,6 +476,51 @@ func TestBuildModifySnapshot(t *testing.T) { assert.Equal(t, "b3", restoredStack.Branches[2].Branch) } +func TestModify_DependentBaseGuard(t *testing.T) { + for _, number := range []int{0, 9} { + for _, action := range []modifyview.ActionType{ + modifyview.ActionRename, modifyview.ActionDrop, modifyview.ActionFoldDown, modifyview.ActionFoldUp, + modifyview.ActionMove, modifyview.ActionInsertAbove, modifyview.ActionInsertBelow, + } { + t.Run(fmt.Sprintf("%d/%s", number, action), func(t *testing.T) { + sf := &stack.StackFile{Stacks: []stack.Stack{ + {Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "parent"}, {Branch: "other"}}}, + {Number: number, Trunk: stack.BranchRef{Branch: "parent"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + }} + before, err := json.Marshal(sf) + require.NoError(t, err) + nodes := []modifyview.ModifyBranchNode{ + { + BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "other"}}, + PendingAction: &modifyview.PendingAction{Type: modifyview.ActionRename, NewName: "other-new"}, + }, + { + BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "parent"}}, + PendingAction: &modifyview.PendingAction{Type: action, NewName: "parent-new"}, + }, + } + cfg, outR, errR := config.NewTestConfig() + err = validateModifyDependents(cfg, sf, &sf.Stacks[0], nodes) + _, output := commandOutput(t, cfg, outR, errR) + blocked := action == modifyview.ActionRename || action == modifyview.ActionDrop || + action == modifyview.ActionFoldDown || action == modifyview.ActionFoldUp + if blocked { + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, `"parent"`) + assert.Contains(t, output, "(parent) <- child") + cfg := issue250TestConfig(t) + require.NoError(t, validateModifyDependents(cfg, sf, &sf.Stacks[0], nodes[:1])) + } else { + require.NoError(t, err) + } + after, err := json.Marshal(sf) + require.NoError(t, err) + assert.Equal(t, string(before), string(after), "the entire plan is validated without changing either stack") + }) + } + } +} + func TestBuildModifyPlan(t *testing.T) { t.Run("drop action", func(t *testing.T) { nodes := []modifyview.ModifyBranchNode{ diff --git a/cmd/navigate_test.go b/cmd/navigate_test.go index 0e575c86..55b6ef39 100644 --- a/cmd/navigate_test.go +++ b/cmd/navigate_test.go @@ -42,9 +42,11 @@ func TestNavigation_PrintPath(t *testing.T) { } t.Run(strconv.FormatBool(foreign), func(t *testing.T) { common, root, owner := t.TempDir(), t.TempDir(), filepath.Join(t.TempDir(), "other tree") - writeStackFile(t, common, stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, - }) + writeStackFileMulti(t, common, + stack.Stack{Trunk: stack.BranchRef{Branch: "b1"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + stack.Stack{Trunk: stack.BranchRef{Branch: "b2"}, Branches: []stack.BranchRef{{Branch: "sibling"}}}, + stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}}, + ) var checkouts []string restore := git.SetOps(&git.MockOps{ GitDirFn: func() (string, error) { return common, nil }, diff --git a/cmd/push.go b/cmd/push.go index 38529f41..72c35217 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -65,7 +65,7 @@ func runPush(cfg *config.Config, opts *pushOptions) error { // Find the stack for the current branch without switching branches. // Push should never change the user's checked-out branch. - stacks := sf.FindAllStacksForBranch(currentBranch) + stacks := sf.StacksForBranch(currentBranch) if len(stacks) == 0 { cfg.Errorf("current branch %q is not part of a stack", currentBranch) return ErrNotInStack @@ -84,11 +84,9 @@ func runPush(cfg *config.Config, opts *pushOptions) error { } return ErrSilent } - if err := requireCurrentBase(cfg, s, remote); err != nil { + if _, err := requireCurrentBase(cfg, sf, s, remote, false); err != nil { return err } - // Sync PR state to detect merged/queued PRs before pushing. - _ = syncStackPRs(cfg, s) merged := s.MergedBranches() if len(merged) > 0 { diff --git a/cmd/push_test.go b/cmd/push_test.go index 15fa1875..c788b569 100644 --- a/cmd/push_test.go +++ b/cmd/push_test.go @@ -33,7 +33,10 @@ func TestPush_PushesAllBranches(t *testing.T) { } tmpDir := t.TempDir() - writeStackFile(t, tmpDir, s) + writeStackFileMulti(t, tmpDir, + stack.Stack{Trunk: stack.BranchRef{Branch: "b1"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + s, + ) var pushCalls []pushCall @@ -424,3 +427,53 @@ func TestPush_BaseRetarget(t *testing.T) { assert.ErrorIs(t, runPush(cfg, &pushOptions{remote: "origin"}), ErrSilent) } + +func TestPush_RetargetedClosedPRRequiresBase(t *testing.T) { + dir := t.TempDir() + writeStackFile(t, dir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "parent"}, + Branches: []stack.BranchRef{{Branch: "child", PullRequest: &stack.PullRequestRef{Number: 11}}}, + }) + mock := newPushMock(dir, "child") + mock.CommitExistsFn = func(string) (bool, error) { return false, nil } + mock.RemoteBranchExistsFn = func(remote, branch string) (bool, error) { + assert.Equal(t, "origin", remote) + assert.Equal(t, "parent", branch) + return false, nil + } + mock.PushFn = func(string, []string, bool, bool) error { + t.Fatal("a closed associated PR must not use the local-only exemption") + return nil + } + restore := git.SetOps(mock) + defer restore() + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(int) (*github.PullRequest, error) { + return &github.PullRequest{Number: 11, State: "CLOSED", BaseRefName: "main"}, nil + }, + } + require.ErrorIs(t, runPush(cfg, &pushOptions{remote: "origin"}), ErrInvalidArgs) + _, output := commandOutput(t, cfg, outR, errR) + assert.Contains(t, output, "PR #11 is closed") +} + +func TestPush_SinglePRRetarget(t *testing.T) { + for _, closed := range []bool{false, true} { + t.Run(fmt.Sprint(closed), func(t *testing.T) { + repo := setupSinglePRRetargetRepo(t, true, closed) + withIssue250Repo(t, repo.dir) + before := issue250Git(t, repo.dir, "rev-parse", "origin/child") + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = repo.client + require.Error(t, runPush(cfg, &pushOptions{remote: "origin"})) + _, output := commandOutput(t, cfg, outR, errR) + assert.Equal(t, before, issue250Git(t, repo.dir, "rev-parse", "origin/child")) + if closed { + assert.Contains(t, output, "PR #11 is closed") + } else { + assert.Contains(t, output, "gh stack sync") + } + }) + } +} diff --git a/cmd/rebase.go b/cmd/rebase.go index 6e29d65e..5664d7cf 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -185,6 +185,22 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { cfg.Printf("No branches to rebase") return nil } + var remote string + var serverBase remoteBase + if !opts.noTrunk { + remote, err = pickRemote(cfg, anchor, opts.remote) + if err != nil { + if !errors.Is(err, errInterrupt) { + cfg.Errorf("%s", err) + } + return ErrSilent + } + serverBase, err = retargetedBase(cfg, sf, s, remote) + if err != nil { + cfg.Errorf("failed to read the stack's base on GitHub: %v", err) + return ErrAPIFailure + } + } _ = syncStackPRs(cfg, s) ctx, err := worktree.New() if err != nil { @@ -199,20 +215,9 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { } var trunk trunkTarget - var newBase, cutoff string + newBase := serverBase.Branch + var cutoff string if !opts.noTrunk { - // Resolve remote for fetch and trunk comparison - remote, err := pickRemote(cfg, anchor, opts.remote) - if err != nil { - if !errors.Is(err, errInterrupt) { - cfg.Errorf("%s", err) - } - return ErrSilent - } - if newBase, err = retargetedBase(cfg, s, remote); err != nil { - cfg.Errorf("failed to read the stack's base on GitHub: %v", err) - return ErrAPIFailure - } if i := s.FirstActiveBranchIndex(); newBase != "" && i >= 0 && (i < startIdx || i >= endIdx) { cfg.Errorf("rebase the whole stack to move it onto %s", newBase) return ErrInvalidArgs @@ -231,12 +236,15 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { cfg.Errorf("%s", err) return ErrSilent } - trunk, err = resolveTrunkTarget(cfg, s, remote, currentBranch, trunkResolveOptions{Worktrees: ctx, Branch: newBase}) + trunk, err = resolveTrunkTarget(cfg, s, remote, currentBranch, trunkResolveOptions{ + Worktrees: ctx, Branch: newBase, Catalog: sf, + RequireRemote: serverBase.Published, ClosedPR: serverBase.ClosedPR, + }) if err != nil { return err } - if newBase != "" { - if cutoff, err = retargetCutoff(cfg, s, trunk, planned, opts.acceptBaseChange); err != nil { + if newBase != "" || startIdx == 0 { + if cutoff, err = bottomRebaseCutoff(cfg, s, trunk, planned, newBase != "", opts.acceptBaseChange); err != nil { return err } } diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 3e2d4e79..e03be60c 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -185,7 +185,9 @@ func TestRebase_CascadeRebase(t *testing.T) { } tmpDir := t.TempDir() - writeStackFile(t, tmpDir, s) + writeStackFileMulti(t, tmpDir, s, + stack.Stack{Trunk: stack.BranchRef{Branch: "b2"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + ) var allRebaseCalls []rebaseCall var currentCheckedOut string @@ -869,7 +871,7 @@ func TestRebase_QueuedBranch_DownstreamStaysStacked(t *testing.T) { assert.NoError(t, err) assert.Contains(t, output, "Skipping b1") assert.Contains(t, output, "queued") - assert.NotContains(t, output, "adjusted for merged PR", + assert.NotContains(t, output, "adjusted for changed base", "queued branches must not trigger the merged --onto path") // b2 stays stacked on the queued b1 (not rebased --onto main); b3 onto b2. @@ -932,7 +934,7 @@ func TestRebase_MergedBelowQueued_KeepsStackedOnQueued(t *testing.T) { require.Len(t, rebaseCalls, 1) assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[0], "b3 should rebase onto the queued b2, not --onto main") - assert.NotContains(t, output, "adjusted for merged PR") + assert.NotContains(t, output, "adjusted for changed base") } // TestRebase_UpstackAboveQueuedBranch verifies the onto-seed fix: with --upstack @@ -982,7 +984,7 @@ func TestRebase_UpstackAboveQueuedBranch(t *testing.T) { "b2 should rebase onto the queued predecessor b1, not --onto main") assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1], "b3 should rebase onto b2") - assert.NotContains(t, output, "adjusted for merged PR") + assert.NotContains(t, output, "adjusted for changed base") } // TestRebase_StateRoundTrip verifies that rebase state can be saved and loaded @@ -2228,6 +2230,74 @@ func TestResolveRebaseOldBase(t *testing.T) { }) } +func TestBottomRebaseCutoff(t *testing.T) { + for _, tt := range []struct { + name string + base string + valid, landed, planned, older bool + skipped bool + fail bool + want string + }{ + {name: "rewritten", base: "old", valid: true, want: "old"}, + {name: "forward only", base: "old", valid: true, landed: true}, + {name: "empty recorded base"}, + {name: "imported nonancestor base", base: "old"}, + {name: "planned child fast-forward", base: "old", valid: true, planned: true, want: "old"}, + {name: "older local tip", base: "old", valid: true, older: true, want: "old"}, + {name: "queued bottom", base: "old", valid: true, skipped: true}, + {name: "lookup error", base: "old", fail: true}, + } { + t.Run(tt.name, func(t *testing.T) { + lookupErr := errors.New("lookup failed") + restore := git.SetOps(&git.MockOps{ + IsAncestorFn: func(a, b string) (bool, error) { + if tt.fail { + return false, lookupErr + } + switch a + ":" + b { + case "old:child": + return tt.valid && !tt.planned, nil + case "old:remote-child": + return tt.valid, nil + case "old:new": + return tt.landed, nil + case "previous:old", "previous:child": + return tt.older, nil + } + return false, nil + }, + CommitExistsFn: func(string) (bool, error) { return true, nil }, + }) + defer restore() + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "parent"}, + Branches: []stack.BranchRef{ + {Branch: "child", Base: tt.base, Queued: tt.skipped}, + {Branch: "top", Base: "child"}, + }, + } + var planned []branchFastForward + if tt.planned { + planned = []branchFastForward{{Branch: "child", NewSHA: "remote-child"}} + } + cfg := issue250TestConfig(t) + cfg.ConfirmFn = func(string, bool) (bool, error) { + t.Fatal("following a rewritten parent does not require retarget consent") + return false, nil + } + cutoff, err := bottomRebaseCutoff(cfg, s, + trunkTarget{Branch: "parent", Ref: "parent", SHA: "new", OldSHA: "previous"}, planned, false, false) + if tt.fail { + require.ErrorIs(t, err, lookupErr) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, cutoff) + } + }) + } +} + type amendedParentRepo struct { dir string gitDir string @@ -2801,6 +2871,127 @@ func assertIssue250History(t *testing.T, repo amendedParentRepo) { assert.NoError(t, newErr) } +func setupLinkedParentRepo(t *testing.T) amendedParentRepo { + t.Helper() + repo := setupAmendedParentRepo(t, false) + issue250Git(t, repo.dir, "push", "--force", "origin", "parent") + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + child := sf.Stacks[0].Branches[1] + sf.Stacks[0].Branches = sf.Stacks[0].Branches[:1] + sf.AddStack(stack.Stack{ + Trunk: stack.BranchRef{Branch: "parent", Head: repo.oldParent}, + Branches: []stack.BranchRef{child}, + }) + require.NoError(t, stack.Save(repo.gitDir, sf)) + return repo +} + +func testLinkedParentRewrite(t *testing.T, run func(*config.Config) error) { + t.Helper() + for _, mode := range []string{"amended", "partial squash", "local-only"} { + t.Run(mode, func(t *testing.T) { + repo := setupLinkedParentRepo(t) + parentRef := "origin/parent" + if mode == "local-only" { + issue250Git(t, repo.dir, "push", "origin", "--delete", "parent") + issue250Git(t, repo.dir, "config", "--unset", "branch.parent.remote") + issue250Git(t, repo.dir, "config", "--unset", "branch.parent.merge") + parentRef = "parent" + } + if mode == "partial squash" { + issue250Git(t, repo.dir, "checkout", "-B", "parent", repo.oldParent) + issue250WriteFile(t, repo.dir, "second.txt", "second parent commit\n") + issue250Git(t, repo.dir, "add", ".") + issue250Git(t, repo.dir, "commit", "-m", "parent second") + oldTip := issue250Git(t, repo.dir, "rev-parse", "parent") + issue250Git(t, repo.dir, "rebase", "--onto", "parent", repo.oldParent, "child") + issue250Git(t, repo.dir, "push", "--force", "origin", "child") + issue250Git(t, repo.dir, "checkout", "main") + issue250Git(t, repo.dir, "merge", "--squash", repo.oldParent) + issue250Git(t, repo.dir, "commit", "-m", "squashed first parent") + issue250Git(t, repo.dir, "rebase", "--onto", "main", repo.oldParent, "parent") + repo.newParent = issue250Git(t, repo.dir, "rev-parse", "parent") + issue250Git(t, repo.dir, "push", "--force", "origin", "main", "parent") + issue250Git(t, repo.dir, "checkout", "child") + issue250Git(t, repo.dir, "branch", "-f", "parent", oldTip) + repo.oldParent = oldTip + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + sf.Stacks[1].Branches[0].Base = oldTip + require.NoError(t, stack.Save(repo.gitDir, sf)) + } + issue250Git(t, repo.dir, "branch", "sibling", "child") + parent := issue250Git(t, repo.dir, "rev-parse", "parent") + sibling := issue250Git(t, repo.dir, "rev-parse", "sibling") + before, err := stack.Load(repo.gitDir) + require.NoError(t, err) + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + + require.NoError(t, run(cfg)) + + assert.Equal(t, "child commit", issue250Git(t, repo.dir, "log", "--format=%s", parentRef+"..child")) + assert.Error(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", repo.oldParent, "child")) + require.NoError(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", repo.newParent, "child")) + assert.Equal(t, parent, issue250Git(t, repo.dir, "rev-parse", "parent")) + assert.Equal(t, sibling, issue250Git(t, repo.dir, "rev-parse", "sibling")) + after, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, before.Stacks[0], after.Stacks[0], "child repair leaves the parent catalog alone") + assert.Equal(t, "parent", after.Stacks[1].Trunk.Branch) + assert.Equal(t, repo.newParent, after.Stacks[1].Branches[0].Base) + child := issue250Git(t, repo.dir, "rev-parse", "child") + require.NoError(t, run(cfg)) + assert.Equal(t, child, issue250Git(t, repo.dir, "rev-parse", "child")) + }) + } +} + +func TestRebase_LinkedParentRewrite(t *testing.T) { + testLinkedParentRewrite(t, func(cfg *config.Config) error { + return runRebase(cfg, &rebaseOptions{remote: "origin"}) + }) +} + +func TestRebase_LinkedParentRecovery(t *testing.T) { + for _, action := range []string{"continue", "abort", "sync"} { + t.Run(action, func(t *testing.T) { + repo := setupLinkedParentRepo(t) + issue250Git(t, repo.dir, "checkout", "parent") + issue250WriteFile(t, repo.dir, "child.txt", "conflicting parent change\n") + issue250Git(t, repo.dir, "add", ".") + issue250Git(t, repo.dir, "commit", "--amend", "--no-edit") + issue250Git(t, repo.dir, "push", "--force", "origin", "parent") + issue250Git(t, repo.dir, "checkout", "child") + original := issue250Git(t, repo.dir, "rev-parse", "child") + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + if action == "sync" { + require.ErrorIs(t, runSync(cfg, &syncOptions{remote: "origin"}), ErrConflict) + assert.Equal(t, original, issue250Git(t, repo.dir, "rev-parse", "child")) + assert.Equal(t, original, issue250Git(t, repo.dir, "rev-parse", "origin/child")) + } else { + require.ErrorIs(t, runRebase(cfg, &rebaseOptions{remote: "origin"}), ErrConflict) + state, err := loadRebaseState(repo.gitDir) + require.NoError(t, err) + assert.Empty(t, state.TargetTrunk) + assert.Equal(t, repo.oldParent, state.RebaseOldBase) + if action == "continue" { + issue250WriteFile(t, repo.dir, "child.txt", "resolved child\n") + issue250Git(t, repo.dir, "add", "child.txt") + require.NoError(t, runRebase(cfg, &rebaseOptions{cont: true})) + assert.Error(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", repo.oldParent, "child")) + } else { + require.NoError(t, runRebase(cfg, &rebaseOptions{abort: true})) + assert.Equal(t, original, issue250Git(t, repo.dir, "rev-parse", "child")) + } + } + assert.NoFileExists(t, filepath.Join(repo.gitDir, rebaseStateFile)) + }) + } +} + func TestIntegration_AmendedParentPushThenRebase(t *testing.T) { repo := setupAmendedParentRepo(t, false) withIssue250Repo(t, repo.dir) @@ -3881,6 +4072,135 @@ func setupBaseRetargetRepo(t *testing.T, target string, merged bool) baseRetarge return baseRetargetRepo{repo, target, remote, client} } +func setupSinglePRRetargetRepo(t *testing.T, merged, closed bool) baseRetargetRepo { + t.Helper() + repo := setupBaseRetargetRepo(t, "main", merged) + issue250Git(t, repo.dir, "checkout", "child") + issue250Git(t, repo.dir, "config", "--unset", "branch.parent.remote") + issue250Git(t, repo.dir, "config", "--unset", "branch.parent.merge") + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + sf.Stacks[0].ID, sf.Stacks[0].Number = "", 0 + sf.Stacks[0].Branches = sf.Stacks[0].Branches[:1] + require.NoError(t, stack.Save(repo.gitDir, sf)) + pr := &github.PullRequest{Number: 11, HeadRefName: "child", BaseRefName: "main", State: "OPEN"} + if closed { + pr.BaseRefName, pr.State = "parent", "CLOSED" + } + repo.client.FindPRByNumberFn = func(n int) (*github.PullRequest, error) { + assert.Equal(t, 11, n) + return pr, nil + } + repo.client.FindPRForBranchFn = func(branch string) (*github.PullRequest, error) { + if branch == "child" && !closed { + return pr, nil + } + return nil, nil + } + parentDetails := repo.client.FindPRDetailsForBranchFn + repo.client.FindPRDetailsForBranchFn = func(branch string) (*github.PRDetails, error) { + if branch == "child" { + return &github.PRDetails{Number: 11, State: pr.State, BaseRefName: pr.BaseRefName}, nil + } + return parentDetails(branch) + } + repo.client.GetStackFn = func(int) (*github.RemoteStack, error) { + t.Fatal("a single PR has no remote stack to fetch") + return nil, nil + } + return repo +} + +func TestRebase_UnrelatedClosedPRKeepsLocalBase(t *testing.T) { + repo := setupLinkedParentRepo(t) + issue250Git(t, repo.dir, "push", "origin", "--delete", "parent") + issue250Git(t, repo.dir, "config", "--unset", "branch.parent.remote") + issue250Git(t, repo.dir, "config", "--unset", "branch.parent.merge") + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + cfg.GitHubClientOverride = &github.MockClient{ + FindPRDetailsForBranchFn: func(branch string) (*github.PRDetails, error) { + require.Equal(t, "child", branch) + return &github.PRDetails{Number: 11, State: "CLOSED", BaseRefName: "other-parent"}, nil + }, + } + require.NoError(t, runRebase(cfg, &rebaseOptions{remote: "origin"})) + assert.Equal(t, "child commit", issue250Git(t, repo.dir, "log", "--format=%s", "parent..child")) + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, "parent", sf.Stacks[1].Trunk.Branch) + assert.Nil(t, sf.Stacks[1].Branches[0].PullRequest) +} + +func testSinglePRRetarget(t *testing.T, run func(*config.Config, bool) error) { + t.Helper() + for _, mode := range []string{"merged parent", "unmerged refusal", "unmerged accepted", "closed", "closed after view", "closed after prune", "replacement"} { + t.Run(mode, func(t *testing.T) { + closed := strings.HasPrefix(mode, "closed") + repo := setupSinglePRRetargetRepo(t, mode == "merged parent" || mode == "replacement", closed || mode == "replacement") + if mode == "replacement" { + pr := &github.PullRequest{Number: 13, State: "OPEN", HeadRefName: "child", BaseRefName: "main"} + oldLookup := repo.client.FindPRByNumberFn + repo.client.FindPRByNumberFn = func(n int) (*github.PullRequest, error) { + if n == 13 { + return pr, nil + } + return oldLookup(n) + } + repo.client.FindPRForBranchFn = func(string) (*github.PullRequest, error) { return pr, nil } + } + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + cfg.GitHubClientOverride = repo.client + if strings.HasPrefix(mode, "closed after") { + require.NoError(t, runViewJSON(cfg)) + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Nil(t, sf.Stacks[0].Branches[0].PullRequest) + } + if mode == "closed after prune" { + issue250Git(t, repo.dir, "push", "origin", "--delete", "child") + issue250Git(t, repo.dir, "fetch", "--prune", "origin") + } + before := issue250Git(t, repo.dir, "rev-parse", "child") + catalog, err := os.ReadFile(filepath.Join(repo.gitDir, "gh-stack")) + require.NoError(t, err) + err = run(cfg, mode == "unmerged accepted") + if closed || mode == "unmerged refusal" { + require.Error(t, err) + assert.Equal(t, before, issue250Git(t, repo.dir, "rev-parse", "child")) + if mode == "closed after prune" { + assert.Error(t, issue250GitMayFail(t, repo.dir, "show-ref", "--verify", "refs/remotes/origin/child")) + } else { + assert.Equal(t, before, issue250Git(t, repo.dir, "rev-parse", "origin/child")) + } + after, err := os.ReadFile(filepath.Join(repo.gitDir, "gh-stack")) + require.NoError(t, err) + assert.Equal(t, catalog, after) + } else { + require.NoError(t, err) + assert.Equal(t, "child commit", issue250Git(t, repo.dir, "log", "--format=%s", "main..child")) + assert.Error(t, issue250GitMayFail(t, repo.dir, "merge-base", "--is-ancestor", repo.oldParent, "child")) + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Empty(t, sf.Stacks[0].ID) + assert.Equal(t, "main", sf.Stacks[0].Trunk.Branch) + child := issue250Git(t, repo.dir, "rev-parse", "child") + require.NoError(t, run(cfg, false)) + assert.Equal(t, child, issue250Git(t, repo.dir, "rev-parse", "child")) + } + assert.Equal(t, repo.newParent, issue250Git(t, repo.dir, "rev-parse", "parent")) + assert.NoFileExists(t, filepath.Join(repo.gitDir, rebaseStateFile)) + }) + } +} + +func TestRebase_SinglePRRetarget(t *testing.T) { + testSinglePRRetarget(t, func(cfg *config.Config, accept bool) error { + return runRebase(cfg, &rebaseOptions{remote: "origin", acceptBaseChange: accept}) + }) +} + func assertBaseRetargetHistory(t *testing.T, repo baseRetargetRepo) { t.Helper() assert.Equal(t, "top commit\nchild commit", issue250Git(t, repo.dir, "log", "--format=%s", repo.target+"..top")) diff --git a/cmd/submit.go b/cmd/submit.go index 4151efdf..13c6331b 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -101,7 +101,7 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { // Find the stack for the current branch without switching branches. // Submit should never change the user's checked-out branch. - stacks := sf.FindAllStacksForBranch(currentBranch) + stacks := sf.StacksForBranch(currentBranch) if len(stacks) == 0 { cfg.Errorf("current branch %q is not part of a stack", currentBranch) return ErrNotInStack @@ -145,9 +145,6 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { } } - // Sync PR state to detect merged/queued PRs before pushing. - prDetails := syncStackPRs(cfg, s) - // Resolve remote for pushing remote, err := pickRemote(cfg, currentBranch, opts.remote) if err != nil { @@ -156,7 +153,8 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { } return ErrSilent } - if err := requireCurrentBase(cfg, s, remote); err != nil { + prDetails, err := requireCurrentBase(cfg, sf, s, remote, true) + if err != nil { return err } merged := s.MergedBranches() diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 62868390..c03cce8d 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -102,7 +102,9 @@ func TestSubmit_CreatesPRsAndStack(t *testing.T) { } tmpDir := t.TempDir() - writeStackFile(t, tmpDir, s) + writeStackFileMulti(t, tmpDir, s, + stack.Stack{Trunk: stack.BranchRef{Branch: "b1"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + ) var pushCalls []pushCall var createdPRs []string @@ -1367,6 +1369,69 @@ func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs") } +func TestSubmit_AdoptedPRKeepsChosenBase(t *testing.T) { + for _, method := range []string{"fork", "init"} { + for _, published := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/published=%t", method, published), func(t *testing.T) { + repo := setupAmendedParentRepo(t, false) + issue250Git(t, repo.dir, "branch", "-f", "parent", repo.oldParent) + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + sf.Stacks[0].Branches = sf.Stacks[0].Branches[:1] + require.NoError(t, stack.Save(repo.gitDir, sf)) + base := "parent" + if !published { + base = "local-parent" + issue250Git(t, repo.dir, "branch", base, "parent") + } + withIssue250Repo(t, repo.dir) + cfg := issue250TestConfig(t) + pr := &github.PullRequest{Number: 11, ID: "PR_11", State: "OPEN", HeadRefName: "child", BaseRefName: "main"} + updates := 0 + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(int) (*github.PullRequest, error) { return pr, nil }, + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + if branch == "child" { + return pr, nil + } + return nil, nil + }, + UpdatePRBaseFn: func(number int, target string) error { + assert.Equal(t, 11, number) + assert.Equal(t, base, target) + updates++ + pr.BaseRefName = target + return nil + }, + } + if method == "fork" { + issue250Git(t, repo.dir, "checkout", base) + require.NoError(t, runAdd(cfg, &addOptions{fork: true}, []string{"child"})) + } else { + issue250Git(t, repo.dir, "checkout", "main") + require.NoError(t, runInit(cfg, &initOptions{base: base, branches: []string{"child"}})) + } + before := issue250Git(t, repo.dir, "rev-parse", "child") + require.NoError(t, runRebase(cfg, &rebaseOptions{remote: "origin"})) + assert.Equal(t, before, issue250Git(t, repo.dir, "rev-parse", "child")) + assert.Zero(t, updates, "rebase must not change the PR base") + if !published { + require.ErrorIs(t, runSubmit(cfg, &submitOptions{auto: true, remote: "origin"}), ErrInvalidArgs) + assert.Zero(t, updates) + issue250Git(t, repo.dir, "push", "origin", base) + } + require.NoError(t, runSync(cfg, &syncOptions{remote: "origin"})) + require.NoError(t, runSubmit(cfg, &submitOptions{auto: true, remote: "origin"})) + assert.Equal(t, 1, updates) + after, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, base, after.Stacks[1].Trunk.Branch) + assert.Equal(t, before, issue250Git(t, repo.dir, "rev-parse", "child")) + }) + } + } +} + func TestSubmit_UpdatesBaseBranch(t *testing.T) { // b1's PR has base "main" but it should be "main" (correct). // b2's PR has base "main" but it should be "b1" (wrong — needs update). @@ -1886,6 +1951,7 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { {name: "one PR failed creation", branches: 1, newPR: true, failure: "create"}, {name: "one PR invalid creation result", branches: 1, newPR: true, failure: "invalid-create"}, {name: "one PR failed base update", branches: 1, failure: "base"}, + {name: "retry one PR base update", branches: 1, failure: "base", retry: true}, {name: "one PR failed auto-merge update", branches: 1, failure: "auto-merge"}, {name: "one PR failed ready update", branches: 1, failure: "ready"}, {name: "one PR failed catalog save", branches: 1, failure: "save"}, @@ -2118,7 +2184,13 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { require.NoError(t, loadErr) require.Len(t, saved.Stacks, 1) assert.Equal(t, s.BranchNames(), saved.Stacks[0].BranchNames()) - assert.Equal(t, "77", saved.Stacks[0].ID) + if tc.branches == 1 { + assert.Empty(t, saved.Stacks[0].ID) + assert.Nil(t, replacement) + assert.Equal(t, 2, baseUpdates) + } else { + assert.Equal(t, "77", saved.Stacks[0].ID) + } if tc.mergeOnRetry { assert.Equal(t, []string{"b1", "b3"}, pushes[pushStart:]) assert.Zero(t, createdPRs) @@ -3175,3 +3247,121 @@ func TestSubmit_BaseRetarget(t *testing.T) { }) } } + +func TestSubmit_SelectedRemoteMissingDefault(t *testing.T) { + repo := setupAmendedParentRepo(t, false) + upstream := filepath.Join(t.TempDir(), "upstream.git") + issue250Git(t, ".", "-c", "safe.bareRepository=all", "init", "--bare", "-b", "release", upstream) + issue250Git(t, repo.dir, "remote", "add", "upstream", upstream) + issue250Git(t, repo.dir, "push", "upstream", "main:release") + issue250Git(t, repo.dir, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/main") + writeStackFile(t, repo.gitDir, stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "child"}}, + }) + withIssue250Repo(t, repo.dir) + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + CreatePRFn: func(string, string, string, string, bool) (*github.PullRequest, error) { + t.Fatal("must reject the selected remote's missing base before opening PRs") + return nil, nil + }, + } + before, err := os.ReadFile(filepath.Join(repo.gitDir, "gh-stack")) + require.NoError(t, err) + require.ErrorIs(t, runSubmit(cfg, &submitOptions{auto: true, remote: "upstream"}), ErrInvalidArgs) + _, output := commandOutput(t, cfg, outR, errR) + assert.Contains(t, output, "main does not exist on upstream") + assert.Empty(t, issue250Git(t, repo.dir, "ls-remote", "--heads", "upstream", "refs/heads/child")) + after, err := os.ReadFile(filepath.Join(repo.gitDir, "gh-stack")) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +func TestSubmit_BasePreflight(t *testing.T) { + for _, mode := range []string{"unpublished", "transport error", "published", "default", "missing selected default", "retargeted", "closed"} { + t.Run(mode, func(t *testing.T) { + dir := t.TempDir() + trunk := "parent" + if mode == "default" || mode == "missing selected default" { + trunk = "main" + } + s := stack.Stack{Trunk: stack.BranchRef{Branch: trunk}, Branches: []stack.BranchRef{{Branch: "child"}}} + pr := &github.PullRequest{Number: 11, State: "OPEN", HeadRefName: "child", BaseRefName: trunk} + if mode != "unpublished" { + s.Branches[0].PullRequest = &stack.PullRequestRef{Number: 11} + } + if mode == "retargeted" { + pr.BaseRefName = "main" + } + if mode == "closed" { + pr.State = "CLOSED" + } + writeStackFile(t, dir, s) + var checked, pushed int + mock := newSubmitMock(dir, "child") + mock.CommitExistsFn = func(ref string) (bool, error) { + return mode == "retargeted" && ref == "refs/remotes/upstream/parent", nil + } + mock.RemoteBranchExistsFn = func(remote, branch string) (bool, error) { + assert.Equal(t, "upstream", remote) + assert.Equal(t, trunk, branch) + checked++ + if mode == "unpublished" || mode == "closed" || mode == "missing selected default" || mode == "retargeted" { + return false, nil + } + if mode == "transport error" { + return false, fmt.Errorf("network unavailable") + } + return true, nil + } + mock.PushFn = func(_ string, branches []string, _, _ bool) error { + assert.Contains(t, []string{"published", "default"}, mode) + assert.Equal(t, []string{"child"}, branches) + pushed++ + return nil + } + restore := git.SetOps(mock) + defer restore() + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(int) (*github.PullRequest, error) { return pr, nil }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { + if mode == "unpublished" || mode == "closed" { + return nil, nil + } + return pr, nil + }, + CreatePRFn: func(string, string, string, string, bool) (*github.PullRequest, error) { + t.Fatal("must not create a PR during base preflight") + return nil, nil + }, + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + t.Fatal("must not unstack before base preflight") + return nil, false, nil + }, + } + err := runSubmit(cfg, &submitOptions{auto: true, remote: "upstream"}) + _, output := commandOutput(t, cfg, outR, errR) + if mode == "published" || mode == "default" { + require.NoError(t, err) + assert.Equal(t, 1, pushed) + } else { + require.Error(t, err) + assert.Zero(t, pushed) + sf, loadErr := stack.Load(dir) + require.NoError(t, loadErr) + assert.Equal(t, s, sf.Stacks[0]) + switch mode { + case "unpublished", "missing selected default": + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "Publish the parent stack") + case "retargeted": + assert.Contains(t, output, "gh stack sync") + case "closed": + assert.Contains(t, output, "PR #11 is closed") + } + } + assert.Equal(t, 1, checked) + }) + } +} diff --git a/cmd/sync.go b/cmd/sync.go index c2c058a2..6bed9d97 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -156,12 +156,13 @@ func runSync(cfg *config.Config, opts *syncOptions) error { if cb, cbErr := git.CurrentBranch(); cbErr == nil { currentBranch = cb } - _ = syncStackPRs(cfg, s) - newBase, err := retargetedBase(cfg, s, remote) + serverBase, err := retargetedBase(cfg, sf, s, remote) if err != nil { cfg.Errorf("failed to read the stack's base on GitHub: %v", err) return ErrAPIFailure } + newBase := serverBase.Branch + _ = syncStackPRs(cfg, s) ctx, err := worktree.New() if err != nil { cfg.Errorf("%s", err) @@ -171,8 +172,11 @@ func runSync(cfg *config.Config, opts *syncOptions) error { // --- Step 2: Resolve trunk --- trunk, err := resolveTrunkTarget(cfg, s, remote, currentBranch, trunkResolveOptions{ - Worktrees: ctx, - Branch: newBase, + Worktrees: ctx, + Branch: newBase, + Catalog: sf, + RequireRemote: serverBase.Published, + ClosedPR: serverBase.ClosedPR, Preflight: func(sha string, moved bool) error { var required []string for _, forward := range planned { @@ -191,10 +195,14 @@ func runSync(cfg *config.Config, opts *syncOptions) error { if err != nil { return err } - var cutoff string - if newBase != "" { - if cutoff, err = retargetCutoff(cfg, s, trunk, planned, opts.acceptBaseChange); err != nil { - return err + cutoff, err := bottomRebaseCutoff(cfg, s, trunk, planned, newBase != "", opts.acceptBaseChange) + if err != nil { + return err + } + if cutoff != "" { + if err := ctx.Preflight(activeBranchNames(s)); err != nil { + cfg.Errorf("%s", err) + return ErrSilent } } @@ -206,7 +214,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } // --- Step 3: Cascade rebase --- - needsRebase := newBase != "" || trunk.Moved || len(updatedBranches) > 0 || stackNeedsRebase(s, trunk.Ref) + needsRebase := newBase != "" || cutoff != "" || trunk.Moved || len(updatedBranches) > 0 || stackNeedsRebase(s, trunk.Ref) rebased := false var originalRefs map[string]string var state *rebaseState diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 2682c761..52bcbc44 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -85,7 +85,9 @@ func TestSync_TrunkAlreadyUpToDate(t *testing.T) { } tmpDir := t.TempDir() - writeStackFile(t, tmpDir, s) + writeStackFileMulti(t, tmpDir, s, + stack.Stack{Trunk: stack.BranchRef{Branch: "b1"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + ) var rebaseCalls []rebaseCall var pushCalls []pushCall @@ -134,6 +136,7 @@ func TestSync_TrunkAlreadyUpToDate(t *testing.T) { // Push should happen without force require.Len(t, pushCalls, 1) assert.False(t, pushCalls[0].force, "push should not use force when no rebase occurred") + assert.Equal(t, []string{"b1", "b2"}, pushCalls[0].branches) } // TestSync_TrunkUpToDate_StackStale verifies that when trunk is already up to @@ -2876,6 +2879,18 @@ func TestSync_BaseRetarget(t *testing.T) { } } +func TestSync_LinkedParentRewrite(t *testing.T) { + testLinkedParentRewrite(t, func(cfg *config.Config) error { + return runSync(cfg, &syncOptions{remote: "origin"}) + }) +} + +func TestSync_SinglePRRetarget(t *testing.T) { + testSinglePRRetarget(t, func(cfg *config.Config, accept bool) error { + return runSync(cfg, &syncOptions{remote: "origin", acceptBaseChange: accept}) + }) +} + func TestSync_BaseRetargetConflictKeepsOldBase(t *testing.T) { repo := setupBaseRetargetRepo(t, "main", true) issue250Git(t, repo.dir, "checkout", "main") diff --git a/cmd/trunk_target_test.go b/cmd/trunk_target_test.go index b39678c9..0d346747 100644 --- a/cmd/trunk_target_test.go +++ b/cmd/trunk_target_test.go @@ -118,10 +118,11 @@ func TestResolveTrunkTarget(t *testing.T) { t.Run("uses intentional local-only trunk", func(t *testing.T) { mock := trunkTargetMock("local", "") + mock.CommitExistsFn = func(string) (bool, error) { return false, nil } mock.FetchBranchFn = func(string, string) error { return git.ErrRemoteBranchNotFound } - mock.UpstreamRemoteFn = func(string) (string, error) { return "", errors.New("unset") } + mock.UpstreamRemoteFn = func(string) (string, error) { return "", nil } restore := git.SetOps(mock) defer restore() diff --git a/cmd/utils.go b/cmd/utils.go index d8aa476b..8ebf5e6b 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -10,6 +10,7 @@ import ( "sync" "github.com/AlecAivazis/survey/v2/terminal" + cligit "github.com/cli/cli/v2/git" "github.com/cli/go-gh/v2/pkg/api" "github.com/cli/go-gh/v2/pkg/prompter" "github.com/github/gh-stack/internal/config" @@ -419,7 +420,7 @@ func handleSaveError(cfg *config.Config, err error) error { // checkout; other mutating callers switch to the selected stack's top branch. // Returns nil with no error if no stack contains the branch. func resolveStack(sf *stack.StackFile, branch string, cfg *config.Config) (*stack.Stack, error) { - stacks := sf.FindAllStacksForBranch(branch) + stacks := sf.StacksForBranch(branch) switch len(stacks) { case 0: @@ -1087,56 +1088,201 @@ func normalizeStackTrunk(cfg *config.Config, s *stack.Stack, remote string) erro return nil } -// retargetedBase returns the branch GitHub moved a tracked stack onto, or "" -// if the base is unchanged. GitHub retargets a stack when its base branch is -// deleted but does not rebase the stack's branches. -func retargetedBase(cfg *config.Config, s *stack.Stack, remote string) (string, error) { +type remoteBase struct { + Branch string + Published bool + ClosedPR int +} + +// retargetedBase reads the server base without discarding closed-PR evidence. +// GitHub can retarget either a server stack or an individual PR. +func retargetedBase(cfg *config.Config, sf *stack.StackFile, s *stack.Stack, remote string) (remoteBase, error) { + result := remoteBase{Published: s.ID != ""} + trunk, err := normalizeTrunkBranch(s.Trunk.Branch, remote) + if err != nil { + return result, err + } + var base string if s.ID == "" { - return "", nil + pr, tracked, err := stackBasePR(cfg, s) + if err != nil || pr == nil || s.IndexOf(pr.BaseRefName) >= 0 { + return result, err + } + result.Published = pr.BaseRefName == trunk + if pr.State == "CLOSED" { + if tracked || result.Published { + result.ClosedPR = pr.Number + } + return result, nil + } + base = pr.BaseRefName + } else { + client, err := cfg.GitHubClient() + if err != nil { + return result, err + } + number, err := ensureStackNumber(client, s) + if err != nil || number == 0 { + return result, err + } + rs, err := client.GetStack(number) + if isNotFound(err) { + return result, nil + } + if err != nil || rs == nil { + return result, err + } + base = rs.Base.Ref } - client, err := cfg.GitHubClient() - if err != nil { - return "", err + if base != "" && base != trunk { + if s.ID == "" { + exists, err := git.RemoteBranchExists(remote, trunk) + if err != nil { + return result, err + } + if exists { + result.Published = true + return result, nil + } + published, err := baseWasPublished(sf, trunk, remote) + if err != nil { + return result, err + } + if !published { + client, err := cfg.GitHubClient() + if err != nil { + return result, err + } + parent, err := client.FindPRDetailsForBranch(trunk) + if err != nil { + return result, err + } + published = parent != nil && parent.Number > 0 + } + if !published { + return result, nil // an unpublished local base is an intentional choice + } + } + result.Branch = base + result.Published = true + cfg.Warningf("GitHub retargeted this stack%s from %s to %s", linkedStackLabel(cfg, s.Number), s.Trunk.Branch, base) } - number, err := ensureStackNumber(client, s) - if err != nil || number == 0 { - return "", err + return result, nil +} + +func stackBasePR(cfg *config.Config, s *stack.Stack) (*github.PullRequest, bool, error) { + for _, b := range s.Branches { + if b.IsMerged() { + continue + } + client, err := cfg.GitHubClient() + var pr *github.PullRequest + if err == nil && b.PullRequest != nil && b.PullRequest.Number > 0 { + pr, err = client.FindPRByNumber(b.PullRequest.Number) + } + if err == nil && (pr == nil || pr.State == "CLOSED") { + var open *github.PullRequest + open, err = client.FindPRForBranch(b.Branch) + if open != nil { + pr = open + } + } + if err == nil && pr == nil { + var details *github.PRDetails + details, err = client.FindPRDetailsForBranch(b.Branch) + if details != nil { + pr = &github.PullRequest{Number: details.Number, State: details.State, Merged: details.Merged, BaseRefName: details.BaseRefName} + } + } + if err != nil { + if b.PullRequest == nil { + cfg.Warningf("Could not look up a PR for %s: %v", b.Branch, err) + return nil, false, nil // discovery remains best-effort for untracked PRs + } + return nil, false, err + } + if pr != nil && pr.Merged { + continue + } + tracked := pr != nil && pr.Number > 0 && b.PullRequest != nil && b.PullRequest.Number == pr.Number + return pr, tracked, nil } - rs, err := client.GetStack(number) - if isNotFound(err) { - return "", nil + return nil, false, nil +} + +func baseWasPublished(sf *stack.StackFile, trunk, remote string) (bool, error) { + if sf != nil { + for _, s := range sf.StacksForBranch(trunk) { + if i := s.IndexOf(trunk); i >= 0 && s.Branches[i].PullRequest != nil && s.Branches[i].PullRequest.Number > 0 { + return true, nil + } + } } - if err != nil || rs == nil || rs.Base.Ref == "" { - return "", err + exists, err := git.CommitExists("refs/remotes/" + remote + "/" + trunk) + if err != nil || exists { + return exists, err } - trunk, err := normalizeTrunkBranch(s.Trunk.Branch, remote) - if err != nil || rs.Base.Ref == trunk { - return "", err + upstream, err := git.UpstreamRemote(trunk) + var gitErr *cligit.GitError + if errors.As(err, &gitErr) && gitErr.ExitCode == 1 { + return false, nil // unset branch..remote } - cfg.Warningf("GitHub retargeted this stack%s from %s to %s", linkedStackLabel(cfg, s.Number), s.Trunk.Branch, rs.Base.Ref) - return rs.Base.Ref, nil + return upstream == remote, err } -// requireCurrentBase stops publishing a stack that GitHub retargeted until it -// has been rebased onto its new base. -func requireCurrentBase(cfg *config.Config, s *stack.Stack, remote string) error { - newBase, err := retargetedBase(cfg, s, remote) +func reportMissingBase(cfg *config.Config, trunk, remote string, closedPR int) { + cfg.Errorf("base branch %s does not exist on %s", trunk, remote) + if closedPR > 0 { + cfg.Printf("PR #%d is closed; repair its base and PR before retrying.", closedPR) + } else { + cfg.Printf("Publish the parent stack or push its base branch before retrying.") + } +} + +// requireCurrentBase stops publishing until a retarget is repaired. Submit +// additionally requires even a previously local-only base to exist remotely. +func requireCurrentBase(cfg *config.Config, sf *stack.StackFile, s *stack.Stack, remote string, submitting bool) (map[string]*github.PRDetails, error) { + base, err := retargetedBase(cfg, sf, s, remote) if err != nil { cfg.Errorf("failed to read the stack's base on GitHub: %v", err) - return ErrAPIFailure + return nil, ErrAPIFailure } - if newBase != "" { - cfg.Errorf("run `%s` to rebase the stack onto %s before publishing", cfg.ColorCyan("gh stack sync"), newBase) - return ErrSilent + if base.Branch != "" { + cfg.Errorf("run `%s` to rebase the stack onto %s before publishing", cfg.ColorCyan("gh stack sync"), base.Branch) + return nil, ErrSilent } - return nil + // Refresh only after reading the base: closed PRs may lose their association. + details := syncStackPRs(cfg, s) + if len(s.ActiveBranches()) == 0 { + return details, nil + } + trunk, err := normalizeTrunkBranch(s.Trunk.Branch, remote) + if err != nil { + return nil, err + } + if !submitting && !base.Published && base.ClosedPR == 0 { + known, err := baseWasPublished(sf, trunk, remote) + if err != nil { + return nil, err + } + if !known { + return details, nil + } + } + exists, err := git.RemoteBranchExists(remote, trunk) + if err != nil { + return nil, fmt.Errorf("checking base branch %s on %s: %w", trunk, remote, err) + } + if !exists { + reportMissingBase(cfg, trunk, remote, base.ClosedPR) + return nil, ErrInvalidArgs + } + return details, nil } -// retargetCutoff returns the old base commit for rebasing the bottom branch -// onto a retargeted base, or "" when merged branches below it already set the -// cutoff. Dropping commits that are not known to have landed in the new base -// requires consent. -func retargetCutoff(cfg *config.Config, s *stack.Stack, target trunkTarget, planned []branchFastForward, accept bool) (string, error) { +// bottomRebaseCutoff selects --onto for a retargeted or rewritten trunk. +// Only retargeting away from unlanded commits requires consent. +func bottomRebaseCutoff(cfg *config.Config, s *stack.Stack, target trunkTarget, planned []branchFastForward, retarget, accept bool) (string, error) { i := s.FirstActiveBranchIndex() if i < 0 { return "", nil @@ -1148,6 +1294,30 @@ func retargetCutoff(cfg *config.Config, s *stack.Stack, target trunkTarget, plan head = forward.NewSHA } } + if !retarget { + if i != 0 { + return "", nil + } + valid, err := rebaseBoundaryContains(b.Base, head) + if err != nil || !valid { + return "", err + } + landed, err := git.IsAncestor(b.Base, target.SHA) + if err != nil || landed { + return "", err + } + oldTip := target.OldSHA + if oldTip != "" { + older, err := git.IsAncestor(oldTip, b.Base) + if err != nil { + return "", err + } + if older { + oldTip = "" // do not prefer a local tip older than the recorded boundary + } + } + return resolveRebaseOldBase(oldTip, b.Base, target.Ref, head) + } var cutoff string var err error if i == 0 { @@ -1240,13 +1410,17 @@ type trunkTarget struct { Branch string Ref string SHA string + OldSHA string Moved bool } type trunkResolveOptions struct { - Worktrees *worktree.Context - Preflight func(string, bool) error - Branch string // overrides the stack's trunk + Worktrees *worktree.Context + Preflight func(string, bool) error + Branch string // overrides the stack's trunk + Catalog *stack.StackFile + RequireRemote bool + ClosedPR int } func (t trunkTarget) Describe() string { @@ -1277,10 +1451,18 @@ func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranc trunk = opts.Branch } remoteRef := remote + "/" + trunk + knownRemote := opts.RequireRemote + if !knownRemote { + var err error + knownRemote, err = baseWasPublished(opts.Catalog, trunk, remote) + if err != nil { + return trunkTarget{}, fmt.Errorf("checking previous publication of %s: %w", trunk, err) + } + } if err := git.FetchBranch(remote, trunk); err != nil { if errors.Is(err, git.ErrRemoteBranchNotFound) { - target, err := trunkWithoutRemote(cfg, trunk, remote) + target, err := trunkWithoutRemote(cfg, trunk, remote, knownRemote, opts.ClosedPR) if err == nil { err = preflight(target.SHA, target.Moved) } @@ -1323,7 +1505,7 @@ func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranc return trunkTarget{}, err } cfg.Successf("Trunk %s is already up to date", trunk) - return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA, OldSHA: localSHA}, nil } canFastForward, ffErr := git.IsAncestor(localSHA, remoteSHA) @@ -1351,7 +1533,7 @@ func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranc } if updateErr == nil { cfg.Successf("Trunk %s fast-forwarded to %s", trunk, short(remoteSHA)) - return trunkTarget{Branch: trunk, Ref: trunk, SHA: remoteSHA, Moved: true}, nil + return trunkTarget{Branch: trunk, Ref: trunk, SHA: remoteSHA, OldSHA: localSHA, Moved: true}, nil } cfg.Warningf("Could not update local %s: %v", trunk, updateErr) } else if ffErr != nil { @@ -1363,7 +1545,7 @@ func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranc return trunkTarget{}, err } cfg.Successf("Trunk %s is ahead of %s — using the local branch", trunk, remoteRef) - return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA, OldSHA: localSHA}, nil } else { cfg.Warningf("Local %s has diverged from %s", trunk, remoteRef) } @@ -1372,10 +1554,14 @@ func resolveTrunkTarget(cfg *config.Config, s *stack.Stack, remote, currentBranc return trunkTarget{}, err } cfg.Printf(" Rebasing the stack onto %s instead; local %s is unchanged.", remoteRef, trunk) - return trunkTarget{Branch: trunk, Ref: remoteRef, SHA: remoteSHA}, nil + return trunkTarget{Branch: trunk, Ref: remoteRef, SHA: remoteSHA, OldSHA: localSHA}, nil } -func trunkWithoutRemote(cfg *config.Config, trunk, remote string) (trunkTarget, error) { +func trunkWithoutRemote(cfg *config.Config, trunk, remote string, knownRemote bool, closedPR int) (trunkTarget, error) { + if knownRemote || closedPR > 0 { + reportMissingBase(cfg, trunk, remote, closedPR) + return trunkTarget{}, ErrSilent + } exists, err := git.BranchExists(trunk) if err != nil { return trunkTarget{}, fmt.Errorf("checking trunk branch %s: %w", trunk, err) @@ -1384,20 +1570,13 @@ func trunkWithoutRemote(cfg *config.Config, trunk, remote string) (trunkTarget, cfg.Errorf("trunk branch %s exists neither locally nor on %s", trunk, remote) return trunkTarget{}, ErrSilent } - if trackedRemote, err := git.UpstreamRemote(trunk); err == nil && trackedRemote != "" { - cfg.Errorf("%s no longer exists on %s", trunk, remote) - cfg.Printf(" Re-point the stack at an existing trunk branch, or use `%s`.", - cfg.ColorCyan("gh stack rebase --no-trunk")) - return trunkTarget{}, ErrSilent - } - localSHA, err := git.RevParse(trunk) if err != nil { cfg.Errorf("could not resolve local trunk branch %s: %v", trunk, err) return trunkTarget{}, ErrSilent } cfg.Warningf("Trunk %s only exists locally — %s has no such branch", trunk, remote) - return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA}, nil + return trunkTarget{Branch: trunk, Ref: trunk, SHA: localSHA, OldSHA: localSHA}, nil } // cascadeRebaseOpts holds parameters for a cascade rebase across a range of @@ -1705,7 +1884,7 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { } } - cfg.Successf("Rebased %s onto %s (adjusted for merged PR)", br.Branch, newBase) + cfg.Successf("Rebased %s onto %s (adjusted for changed base)", br.Branch, newBase) result.Rebased = true ontoOldBase = originalRefs[br.Branch] } else { @@ -1819,7 +1998,7 @@ func resolvePR(cfg *config.Config, sf *stack.StackFile, target string) (*stack.S } // Try matching as a branch name. - stacks := sf.FindAllStacksForBranch(target) + stacks := sf.StacksForBranch(target) if len(stacks) > 0 { s := stacks[0] idx := s.IndexOf(target) diff --git a/cmd/utils_test.go b/cmd/utils_test.go index 48780835..b65bf4a3 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -78,6 +78,38 @@ func TestResolveStack_ReadOnlySelectionDoesNotCheckout(t *testing.T) { commandOutput(t, cfg, outR, errR) } +func TestResolveStack_MemberBeforeTrunk(t *testing.T) { + for _, interactive := range []bool{false, true} { + t.Run(fmt.Sprint(interactive), func(t *testing.T) { + sf := &stack.StackFile{Stacks: []stack.Stack{ + {Trunk: stack.BranchRef{Branch: "parent"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + {Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "parent"}, {Branch: "top"}}}, + {Trunk: stack.BranchRef{Branch: "parent"}, Branches: []stack.BranchRef{{Branch: "sibling"}}}, + }} + cfg, outR, errR := config.NewTestConfig() + cfg.ForceInteractive = interactive + cfg.StackMutation = &config.StackMutationContext{} + cfg.SelectFn = func(string, string, []string) (int, error) { + t.Fatal("a member is not ambiguous with dependent trunks") + return 0, nil + } + restore := git.SetOps(&git.MockOps{CheckoutBranchFn: func(string) error { + t.Fatal("member resolution must not change checkout") + return nil + }}) + defer restore() + selected, err := resolveStack(sf, "parent", cfg) + require.NoError(t, err) + assert.Same(t, &sf.Stacks[1], selected) + selected, branch, err := resolvePR(cfg, sf, "parent") + require.NoError(t, err) + assert.Same(t, &sf.Stacks[1], selected) + assert.Equal(t, "parent", branch.Branch) + commandOutput(t, cfg, outR, errR) + }) + } +} + func TestResolveStack_RewriteSelectionPreservesCheckout(t *testing.T) { for _, kind := range []string{"rebase", "rebase-continue", "rebase-abort", "sync", "modify", "modify-continue", "modify-abort", "add", "checkout", "submit"} { t.Run(kind, func(t *testing.T) { @@ -1925,6 +1957,149 @@ func TestTrunkLookupFailureStopsBeforeMutation(t *testing.T) { require.ErrorIs(t, err, lookupErr) } +func TestRetargetedBase_SinglePR(t *testing.T) { + for _, mode := range []string{"tracked", "discovered", "replacement", "no PR", "closed", "closed without tracking", "unrelated closed", "tracked closed retargeted", "queued bottom", "internal base", "discovery failure", "tracked failure"} { + t.Run(mode, func(t *testing.T) { + s := &stack.Stack{Trunk: stack.BranchRef{Branch: "parent"}, Branches: []stack.BranchRef{ + {Branch: "child", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "top", PullRequest: &stack.PullRequestRef{Number: 12}}, + }} + pr := &github.PullRequest{Number: 11, BaseRefName: "main", State: "OPEN"} + switch mode { + case "discovered", "no PR", "discovery failure": + s.Branches[0].PullRequest = nil + case "closed": + pr.BaseRefName, pr.State = "parent", "CLOSED" + case "closed without tracking", "unrelated closed": + s.Branches[0].PullRequest = nil + pr.BaseRefName, pr.State = "parent", "CLOSED" + if mode == "unrelated closed" { + pr.BaseRefName = "other-parent" + } + case "tracked closed retargeted": + pr.State = "CLOSED" + case "queued bottom": + pr.BaseRefName = "parent" + pr.MergeQueueEntry = &github.MergeQueueEntry{ID: "queued"} + s.Branches[0].Queued = true + case "internal base": + pr.BaseRefName = "top" + } + restore := git.SetOps(&git.MockOps{ + CommitExistsFn: func(ref string) (bool, error) { + return ref == "refs/remotes/origin/parent" && + (mode == "tracked" || mode == "discovered" || mode == "replacement"), nil + }, + RemoteBranchExistsFn: func(string, string) (bool, error) { return false, nil }, + }) + defer restore() + cfg, outR, errR := config.NewTestConfig() + lookupErr := fmt.Errorf("API unavailable") + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + assert.Equal(t, 11, n, "a queued bottom must not promote the next PR's internal base") + if mode == "replacement" { + return &github.PullRequest{Number: 11, State: "CLOSED", BaseRefName: "parent"}, nil + } + if mode == "tracked failure" { + return nil, lookupErr + } + return pr, nil + }, + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + assert.Equal(t, "child", branch) + if mode == "discovery failure" { + return nil, lookupErr + } + if mode == "no PR" || pr.State == "CLOSED" { + return nil, nil + } + return pr, nil + }, + FindPRDetailsForBranchFn: func(branch string) (*github.PRDetails, error) { + assert.Equal(t, "child", branch) + if pr.State == "CLOSED" { + return &github.PRDetails{Number: pr.Number, State: pr.State, BaseRefName: pr.BaseRefName}, nil + } + return nil, nil + }, + GetStackFn: func(int) (*github.RemoteStack, error) { + t.Fatal("ID-less lookup must not fetch a server stack") + return nil, nil + }, + } + base, err := retargetedBase(cfg, nil, s, "origin") + _, output := commandOutput(t, cfg, outR, errR) + if mode == "tracked failure" { + require.ErrorIs(t, err, lookupErr) + } else { + require.NoError(t, err) + } + if mode == "tracked" || mode == "discovered" || mode == "replacement" { + assert.Equal(t, "main", base.Branch) + } else { + assert.Empty(t, base.Branch) + } + if mode == "closed" || mode == "closed without tracking" { + assert.True(t, base.Published) + assert.Equal(t, 11, base.ClosedPR) + } + if mode == "unrelated closed" { + assert.False(t, base.Published) + assert.Zero(t, base.ClosedPR) + } + if mode == "tracked closed retargeted" { + assert.False(t, base.Published) + assert.Equal(t, 11, base.ClosedPR) + } + if mode == "discovery failure" { + assert.Contains(t, output, "Could not look up a PR") + } + }) + } +} + +func TestBaseWasPublished(t *testing.T) { + for _, mode := range []string{"local", "catalog", "remote ref", "upstream", "other remote", "lookup error"} { + t.Run(mode, func(t *testing.T) { + sf := &stack.StackFile{Stacks: []stack.Stack{ + {Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "parent"}}}, + {Trunk: stack.BranchRef{Branch: "parent", Head: "local-only-sha"}, Branches: []stack.BranchRef{{Branch: "child"}}}, + }} + if mode == "catalog" { + sf.Stacks[0].Branches[0].PullRequest = &stack.PullRequestRef{Number: 10, Merged: true} + } + lookupErr := fmt.Errorf("lookup failed") + restore := git.SetOps(&git.MockOps{ + CommitExistsFn: func(ref string) (bool, error) { + assert.Equal(t, "refs/remotes/origin/parent", ref) + if mode == "lookup error" { + return false, lookupErr + } + return mode == "remote ref", nil + }, + UpstreamRemoteFn: func(string) (string, error) { + if mode == "upstream" { + return "origin", nil + } + if mode == "other remote" { + return "upstream", nil + } + return "", nil + }, + }) + defer restore() + published, err := baseWasPublished(sf, "parent", "origin") + if mode == "lookup error" { + require.ErrorIs(t, err, lookupErr) + } else { + require.NoError(t, err) + assert.Equal(t, mode == "catalog" || mode == "remote ref" || mode == "upstream", published) + } + }) + } +} + func TestEnsureLocalTrunk_FetchesAndCreates(t *testing.T) { var fetchedBranches []string var createdBranch, createdBase string diff --git a/cmd/view.go b/cmd/view.go index eca65e8f..2fd36ec3 100644 --- a/cmd/view.go +++ b/cmd/view.go @@ -75,7 +75,7 @@ func runView(cfg *config.Config, opts *viewOptions) error { sf := result.StackFile s := result.Stack currentBranch := result.CurrentBranch - warnRetargeted(cfg, s) + warnRetargeted(cfg, sf, s) // Show loading indicator for interactive TUI mode. showingLoader := false @@ -120,7 +120,7 @@ func runViewJSON(cfg *config.Config) error { return ErrNotInStack } - stacks := sf.FindAllStacksForBranch(currentBranch) + stacks := sf.StacksForBranch(currentBranch) if len(stacks) == 0 { cfg.Errorf("current branch %q is not part of a stack", currentBranch) return ErrNotInStack @@ -131,7 +131,7 @@ func runViewJSON(cfg *config.Config) error { } s := stacks[0] - retargeted := warnRetargeted(cfg, s) + retargeted := warnRetargeted(cfg, sf, s) syncStackPRs(cfg, s) stack.SaveNonBlocking(gitDir, sf) @@ -139,13 +139,13 @@ func runViewJSON(cfg *config.Config) error { } // warnRetargeted reports a GitHub retarget that has not been rebased onto yet. -func warnRetargeted(cfg *config.Config, s *stack.Stack) bool { +func warnRetargeted(cfg *config.Config, sf *stack.StackFile, s *stack.Stack) bool { remote, _ := git.ResolveRemote(s.Trunk.Branch) - newBase, _ := retargetedBase(cfg, s, remote) - if newBase != "" { - cfg.Printf(" Run `%s` to rebase the stack onto %s", cfg.ColorCyan("gh stack sync"), newBase) + base, _ := retargetedBase(cfg, sf, s, remote) + if base.Branch != "" { + cfg.Printf(" Run `%s` to rebase the stack onto %s", cfg.ColorCyan("gh stack sync"), base.Branch) } - return newBase != "" + return base.Branch != "" } func viewShort(cfg *config.Config, s *stack.Stack, currentBranch string) error { diff --git a/cmd/view_test.go b/cmd/view_test.go index 086952ee..813e593f 100644 --- a/cmd/view_test.go +++ b/cmd/view_test.go @@ -547,9 +547,28 @@ func TestRunViewJSON_MultipleStacks(t *testing.T) { assert.Contains(t, string(errOut), "belongs to multiple stacks") } +func TestView_SinglePRRetarget(t *testing.T) { + repo := setupSinglePRRetargetRepo(t, true, false) + issue250Git(t, repo.dir, "branch", "-f", "parent", repo.oldParent) + withIssue250Repo(t, repo.dir) + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = repo.client + require.NoError(t, runViewJSON(cfg)) + out, diagnostics := commandOutput(t, cfg, outR, errR) + var result viewJSONOutput + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, "parent", result.Trunk) + require.Len(t, result.Branches, 1) + assert.True(t, result.Branches[0].NeedsRebase) + assert.Contains(t, diagnostics, "gh stack sync") + sf, err := stack.Load(repo.gitDir) + require.NoError(t, err) + assert.Equal(t, "parent", sf.Stacks[0].Trunk.Branch) +} + func TestRunViewJSON_SingleStack(t *testing.T) { tmpDir := t.TempDir() - writeStackFile(t, tmpDir, stack.Stack{ + writeStackFileMulti(t, tmpDir, stack.Stack{ Trunk: stack.BranchRef{Branch: "main", Head: "aaa"}, Branches: []stack.BranchRef{ { @@ -559,7 +578,7 @@ func TestRunViewJSON_SingleStack(t *testing.T) { PullRequest: &stack.PullRequestRef{Number: 10, URL: "https://github.com/o/r/pull/10"}, }, }, - }) + }, stack.Stack{Trunk: stack.BranchRef{Branch: "feat/01"}, Branches: []stack.BranchRef{{Branch: "child"}}}) restore := git.SetOps(&git.MockOps{ GitDirFn: func() (string, error) { return tmpDir, nil }, diff --git a/cmd/worktree_utils.go b/cmd/worktree_utils.go index a96e56a7..cf474a56 100644 --- a/cmd/worktree_utils.go +++ b/cmd/worktree_utils.go @@ -319,7 +319,7 @@ func loadNavigationStack(cfg *config.Config, printPath bool) (*loadStackResult, cfg.Errorf("finding current branch: %s", err) return nil, ErrNotInStack } - stacks := sf.FindAllStacksForBranch(current) + stacks := sf.StacksForBranch(current) if len(stacks) == 0 { reportBranchNotInStack(cfg, current, false) return nil, ErrNotInStack diff --git a/docs/src/content/docs/faq.md b/docs/src/content/docs/faq.md index 37e1b2c0..14db8a1c 100644 --- a/docs/src/content/docs/faq.md +++ b/docs/src/content/docs/faq.md @@ -75,6 +75,8 @@ Yes. A stack's **trunk** (the base branch of the bottom PR) can be any branch in The same behavior applies to whatever trunk your stack targets — branch protection rules, required checks, and CI are all evaluated against your stack's base branch. +The base may also be a member of another stack. Check out that branch and run `gh stack add --fork ` to start a separate dependent stack. Publish the parent before submitting the child, and sync each stack separately after parent changes. This does not add tree-wide operations or allow a branch to be a member of multiple stacks. + ## Checks, Rules & Requirements ### How are branch protection rules evaluated for Stacked PRs? diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index 297b666d..8f2f1752 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -175,6 +175,29 @@ gh stack top This keeps each branch focused on one concern and avoids muddying the diff for reviewers. +## Branching into a Separate Stack + +Use `add --fork` when new work depends on an existing layer but should be reviewed as a separate stack: + +```sh +# Publish the existing stack first +gh stack submit --auto + +# Branch from the layer this work depends on +gh stack checkout auth +gh stack add --fork alternative-ui +# Make and commit the child changes +gh stack submit --auto +``` + +Each stack remains linear, and a branch can be a member of only one stack. That branch can also be the base of other stacks. Ordinary `add` at the top still extends the current stack; in the middle, an interactive terminal offers to fork instead. + +Commands stay within the selected stack. On a branch that is both a member and another stack's base, the member stack wins. To work on the child, check out one of its own branches. + +After rewriting the parent, sync the parent first, then sync each child separately. If GitHub changes a child's base after deleting the old base, child `sync`/`rebase` repairs its history; this also works when the child has only one PR. Dropping an unmerged old base still requires consent. A closed PR with a missing base needs manual repair rather than an inferred new target. + +There is no tree-wide sync, view, navigation, or merge ordering. `merge` still merges the selected stack into its own base, not automatically into the repository's default branch. `modify` refuses to rename, drop, or fold a branch used as another locally tracked stack's base; reordering and inserting layers remain available. + ## Responding to Review Feedback When a reviewer requests changes on a PR mid-stack: diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index 6368d0c5..0b05471b 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -84,6 +84,8 @@ When explicit branch names are given, existing branches are adopted automaticall Branches checked out in other worktrees can be adopted. If the final branch is occupied elsewhere, `init` leaves your current checkout unchanged and reports the owner instead. +If your current branch already belongs to a stack, use `gh stack add --fork ` to start a separate stack based on it. + Enables `git rerere` automatically so that conflict resolutions are remembered across rebases. **Examples:** @@ -112,13 +114,16 @@ gh stack add [flags] [branch] | Flag | Description | |------|-------------| -| `-A, --all` | Stage all changes (including untracked files); requires `-m` | -| `-u, --update` | Stage changes to tracked files only; requires `-m` | -| `-m, --message ` | Create a commit with this message before creating the branch | +| `--fork` | Start a separate stack based on the current branch | +| `-A, --all` | Stage all changes (including untracked files); opens the editor if `-m` is omitted | +| `-u, --update` | Stage changes to tracked files only; opens the editor if `-m` is omitted | +| `-m, --message ` | Commit staged changes with this message on the added branch | > **Note:** `-A` and `-u` are mutually exclusive. -For an existing stack, creates a new branch at the current HEAD, adds it to the top of the stack, and checks it out. Must be run while on the topmost branch of a stack. If no branch name is given, prompts for one. +Normally creates a branch at the current HEAD, adds it to the top of the stack, and checks it out. From a middle layer, an interactive terminal offers to start a separate stack based on that layer instead; the default is Yes. Without an interactive terminal, use `--fork` explicitly or move to the top to extend the existing stack. + +`--fork` always creates a separate local stack, even at the top of a stack or on a shared trunk. The current branch becomes its base, not a member of both stacks. It does not contact GitHub or publish the parent. Supply a branch name or `-m` without a TTY; otherwise, a missing name is prompted for. Existing foreign-owned branches are adopted without switching either checkout. Commit/stage shortcuts (`-m`, `-A`, `-u`) are incompatible with that adoption and fail before staging or changing membership. @@ -126,12 +131,20 @@ When run interactively from a branch that is not part of a stack, `add` offers t You can optionally stage changes and create a commit as part of the `add` flow. When `-m` is provided without an explicit branch name, the branch name is auto-generated in date+slug format (e.g., `03-24-add_login`). +Ordinary `add` commits in place when the current layer is empty. `--fork` bypasses that shortcut: the commit goes on the new branch, never on its base. A base recorded as merged is refused; sync and choose its updated base first. + **Examples:** ```sh # Create a branch by name gh stack add api-routes +# Start an independent stack based on the current branch +gh stack add --fork alternative-ui + +# Fork and commit on the new branch +gh stack add --fork -Am "Add alternative UI" alternative-ui + # Prompt for a branch name interactively gh stack add @@ -351,6 +364,8 @@ gh stack submit [flags] Creates a Stacked PR for every branch in the stack, pushing branches to the remote. After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs are added to the existing stack. +Publish the parent stack or base branch first. `submit` checks that the base exists on the selected remote before pushing any child branches, including when its name matches the default branch; it never publishes another stack for you. If GitHub has retargeted this stack or its single PR, run `gh stack sync` before publishing. + If every PR in the stack has already been merged, that stack is complete and can't be extended. In that case `submit` automatically starts a **new** stack rooted at the trunk for your unmerged branches and creates it on GitHub, leaving the merged stack untouched. In an interactive terminal, `submit` opens a full-screen editor on a single screen: diff --git a/internal/git/git.go b/internal/git/git.go index 37bd7c4e..ee9e0c79 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -276,6 +276,11 @@ func BranchExists(name string) (bool, error) { return ops.BranchExists(name) } +// RemoteBranchExists checks the selected remote without updating local refs. +func RemoteBranchExists(remote, branch string) (bool, error) { + return ops.RemoteBranchExists(remote, branch) +} + // CheckoutBranch switches to the specified branch. func CheckoutBranch(name string) error { return ops.CheckoutBranch(name) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index 0dd9a96f..92be26e7 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -41,6 +41,7 @@ type Ops interface { RootDir() (string, error) CurrentBranch() (string, error) BranchExists(name string) (bool, error) + RemoteBranchExists(remote, branch string) (bool, error) CheckoutBranch(name string) error Fetch(remote string) error FetchBranch(remote, branch string) error @@ -165,6 +166,20 @@ func (d *defaultOps) BranchExists(name string) (bool, error) { return true, nil } +func (d *defaultOps) RemoteBranchExists(remote, branch string) (bool, error) { + ref := "refs/heads/" + branch + out, err := d.run("ls-remote", "--heads", remote, ref) + if err != nil { + return false, err + } + for _, line := range strings.Split(out, "\n") { + if _, name, ok := strings.Cut(line, "\t"); ok && name == ref { + return true, nil + } + } + return false, nil +} + func (d *defaultOps) CheckoutBranch(name string) error { return d.runSilent("checkout", name) } diff --git a/internal/git/gitops_test.go b/internal/git/gitops_test.go index 71b7edcf..7e227c71 100644 --- a/internal/git/gitops_test.go +++ b/internal/git/gitops_test.go @@ -95,6 +95,40 @@ func setupLocalRepo(t *testing.T) string { return dir } +func TestIntegration_RemoteBranchExists(t *testing.T) { + bare, clone := setupBareAndClone(t) + gitExec(t, clone, "branch", "feature") + gitExec(t, clone, "branch", "refs/heads/missing") + gitExec(t, clone, "tag", "missing") + gitExec(t, clone, "push", "origin", "feature", "refs/heads/refs/heads/missing", "refs/tags/missing") + other := filepath.Join(t.TempDir(), "other") + gitExec(t, ".", "clone", bare, other) + gitExec(t, other, "push", "origin", "--delete", "feature") + before := gitExec(t, clone, "for-each-ref", "--format=%(refname) %(objectname)", "refs/remotes") + restore := withGitDir(t, clone) + defer restore() + for _, tc := range []struct { + name string + exists bool + }{ + {"main", true}, + {"feature", false}, + {"missing", false}, + {"refs/heads/missing", true}, + } { + exists, err := RemoteBranchExists("origin", tc.name) + require.NoError(t, err) + assert.Equal(t, tc.exists, exists, tc.name) + } + assert.Equal(t, before, gitExec(t, clone, "for-each-ref", "--format=%(refname) %(objectname)", "refs/remotes")) + _, err := RemoteBranchExists("unavailable", "main") + require.Error(t, err, "transport errors are not missing branches") + scoped := requireWorktree(t, &defaultOps{}, clone) + exists, err := scoped.RemoteBranchExists("origin", "main") + require.NoError(t, err) + assert.True(t, exists) +} + func TestIntegration_RebaseBoundaryLookups(t *testing.T) { dir := setupLocalRepo(t) restore := withGitDir(t, dir) diff --git a/internal/git/mock_ops.go b/internal/git/mock_ops.go index a3f894ce..e9fd9739 100644 --- a/internal/git/mock_ops.go +++ b/internal/git/mock_ops.go @@ -18,6 +18,7 @@ type MockOps struct { RootDirFn func() (string, error) CurrentBranchFn func() (string, error) BranchExistsFn func(string) (bool, error) + RemoteBranchExistsFn func(string, string) (bool, error) CheckoutBranchFn func(string) error FetchFn func(string) error FetchBranchFn func(string, string) error @@ -132,6 +133,13 @@ func (m *MockOps) BranchExists(name string) (bool, error) { return false, nil } +func (m *MockOps) RemoteBranchExists(remote, branch string) (bool, error) { + if m.RemoteBranchExistsFn != nil { + return m.RemoteBranchExistsFn(remote, branch) + } + return true, nil +} + func (m *MockOps) CheckoutBranch(name string) error { if m.CheckoutBranchFn != nil { return m.CheckoutBranchFn(name) diff --git a/internal/github/github.go b/internal/github/github.go index 88d3f6f5..35441eeb 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -301,6 +301,8 @@ type PRDetails struct { Merged bool IsQueued bool + BaseRefName string + // Merge evidence for a stack retargeted off this PR's branch. HeadRefOID string MergeCommitOID string @@ -320,6 +322,7 @@ func (c *Client) FindPRDetailsForBranch(branch string) (*PRDetails, error) { Merged bool `graphql:"merged"` MergeQueueEntry *MergeQueueEntry `graphql:"mergeQueueEntry"` HeadRefOID string `graphql:"headRefOid"` + BaseRefName string `graphql:"baseRefName"` MergeCommit struct { OID string `graphql:"oid"` } `graphql:"mergeCommit"` @@ -354,6 +357,7 @@ func (c *Client) FindPRDetailsForBranch(branch string) (*PRDetails, error) { HeadRefOID: n.HeadRefOID, MergeCommitOID: n.MergeCommit.OID, + BaseRefName: n.BaseRefName, }, nil } diff --git a/internal/stack/stack.go b/internal/stack/stack.go index 2881ceef..5e1c6385 100644 --- a/internal/stack/stack.go +++ b/internal/stack/stack.go @@ -249,6 +249,22 @@ func (sf *StackFile) FindAllStacksForBranch(branch string) []*Stack { return stacks } +// StacksForBranch prefers stacks containing branch as a member over stacks +// that only use it as their trunk. +func (sf *StackFile) StacksForBranch(branch string) []*Stack { + stacks := sf.FindAllStacksForBranch(branch) + var members []*Stack + for _, s := range stacks { + if s.IndexOf(branch) >= 0 { + members = append(members, s) + } + } + if len(members) > 0 { + return members + } + return stacks +} + // IndexOfStack returns the index of the given stack within the file by identity // (pointer), or -1 if it is not part of this file. Use it to locate a stack // obtained from FindAllStacksForBranch before mutating the Stacks slice. diff --git a/internal/stack/stack_test.go b/internal/stack/stack_test.go index 71d8105d..69f524cc 100644 --- a/internal/stack/stack_test.go +++ b/internal/stack/stack_test.go @@ -25,6 +25,35 @@ func makeMergedBranch(name string, prNum int) BranchRef { return BranchRef{Branch: name, PullRequest: &PullRequestRef{Number: prNum, Merged: true}} } +func TestStacksForBranch(t *testing.T) { + sf := &StackFile{Stacks: []Stack{ + makeStack("parent", "child"), + makeStack("main", "parent", "top"), + makeStack("parent", "sibling"), + makeStack("main", "other"), + }} + for _, tt := range []struct { + branch string + indices []int + }{ + {"parent", []int{1}}, + {"main", []int{1, 3}}, + {"child", []int{0}}, + {"missing", nil}, + } { + t.Run(tt.branch, func(t *testing.T) { + var indices []int + for _, s := range sf.StacksForBranch(tt.branch) { + indices = append(indices, sf.IndexOfStack(s)) + } + assert.Equal(t, tt.indices, indices) + }) + } + assert.Len(t, sf.FindAllStacksForBranch("parent"), 3, "all-matches scans still include trunks") + sf.AddStack(makeStack("main", "parent")) + assert.Len(t, sf.StacksForBranch("parent"), 2, "duplicate members remain ambiguous") +} + // --- ActiveBaseBranch: skipping merged ancestors for rebase --- func TestActiveBaseBranch(t *testing.T) { diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 8e390246..7266bb16 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -54,6 +54,7 @@ Agent harnesses differ, so always pass the flags below instead of relying on tha | `gh stack merge --yes` | `gh pr merge` | `gh pr merge` cannot merge a stack | | `gh stack init ...` | `gh stack init` | prompts for branch names | | `gh stack add ` | `gh stack add` | prompts for a name, and fails even when piped | +| `gh stack add --fork ` | mid-stack `gh stack add` | starts a separate stack without a confirmation prompt | | `gh stack checkout ` | `gh stack checkout` | opens a selection menu | | `gh stack up` / `down` / `top` / `bottom` | `gh stack switch` | `switch` is menu-only | | — | `gh stack modify` | TUI-only, no non-interactive path | @@ -97,6 +98,10 @@ gh stack view --json # confirm Add `--open` to `submit` to create PRs ready for review instead of drafts. Branch names are verbatim — `gh stack add refactor/foo` creates `refactor/foo`. +For a separate stack based on the current branch, use `gh stack add --fork `. +Publish the parent first: child `submit` refuses an unpublished base and never pushes the parent. +Fork commit flags always commit on the new branch, including when the parent is empty. + ## Staying in sync ```bash @@ -108,6 +113,11 @@ Pruning never happens without `--prune` when non-interactive. If the local and r diverged, `sync` prints both chains, makes no changes, and exits 0 with `Sync aborted` — see `references/troubleshooting.md`. +For linked stacks, sync the parent first, then check out and sync each child. Parent rewrites and +server base retargets are repaired within the child stack, including one-PR children. Retargeting +away from unmerged commits still needs confirmation or `--accept-base-change`. A closed PR with a +missing base requires manual repair; do not assume it moved to the default branch. + ## Merging Scope the merge with an argument: @@ -167,7 +177,11 @@ an ancestor of the branch. ## Constraints -- Stacks are strictly linear: one parent, at most one child. Use separate stacks for parallel work. +- Each stack is linear, but its base may belong to another stack. A branch may be a member of only + one stack. On a member that is also another stack's base, commands select the member stack. +- Operations do not traverse dependent stacks. There is no tree view or merge ordering; merge + targets the selected stack's own base. Modify refuses rename/drop/fold of locally known child + stack bases, while still allowing reorder/insert. - `rebase` and `sync` automatically update affected clean worktrees; they never auto-stash or create/remove worktrees. Mutations serialize across the clone, and paused operations require recovery in their recorded owners. diff --git a/skills/gh-stack/references/stack-design.md b/skills/gh-stack/references/stack-design.md index f64543ac..2e9912ff 100644 --- a/skills/gh-stack/references/stack-design.md +++ b/skills/gh-stack/references/stack-design.md @@ -93,5 +93,10 @@ independent refactor. Do not mix efforts into one stack just because you happene Use `gh stack init` for the new effort, or `gh stack checkout ` to move between existing stacks. +If the separate effort depends on a layer in the current stack, check out that layer and run +`gh stack add --fork `. The layer becomes the new stack's base without becoming a member +of both stacks. Publish the parent before submitting the child, and sync parent then child after +rewrites. Each stack stays linear; commands do not traverse all dependent stacks. + A trivial incidental fix can ride along in the current stack. Once it grows into its own project, it deserves its own stack.