From 8eb30674de7d0d41b38a509c689e33a996a3f67a Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Fri, 10 Jul 2026 03:04:36 -0700 Subject: [PATCH] fix: address self-review findings --- .osc-metadata/sync.json | 5 ++ README.md | 2 +- cmd/adopt.go | 28 ++++---- cmd/restack.go | 3 + cmd/submit.go | 26 ++++---- cmd/submit_internal_test.go | 129 ++++++++++++++++++++++++++++++++++++ 6 files changed, 166 insertions(+), 27 deletions(-) create mode 100644 .osc-metadata/sync.json diff --git a/.osc-metadata/sync.json b/.osc-metadata/sync.json new file mode 100644 index 0000000..a568a76 --- /dev/null +++ b/.osc-metadata/sync.json @@ -0,0 +1,5 @@ +{ + "fork_synced_at": "2026-07-10T09:11:21.236884+00:00", + "commits_behind_before_sync": 24, + "action_taken": "synced" +} \ No newline at end of file diff --git a/README.md b/README.md index a604bce..e272e54 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,7 @@ If a rebase conflict occurs, resolve it and run `gh stack continue`. | `-u, --update` | Only update existing PRs, don't create new ones | | `-s, --skip-prs` | Skip PR creation/update, only restack and push | | `-y, --yes` | Skip interactive prompts; use auto-generated PR title/description | -| `--web` | Open created/updated PRs in web browser | +| `--web` | Open newly created PRs in web browser | | `--no-update-refs` | Do not pass `--update-refs` to git (preserves untracked bookmark branches) | ### restack diff --git a/cmd/adopt.go b/cmd/adopt.go index cde1451..e856d40 100644 --- a/cmd/adopt.go +++ b/cmd/adopt.go @@ -9,7 +9,6 @@ import ( "github.com/boneskull/gh-stack/internal/config" "github.com/boneskull/gh-stack/internal/git" "github.com/boneskull/gh-stack/internal/style" - "github.com/boneskull/gh-stack/internal/tree" "github.com/spf13/cobra" ) @@ -85,19 +84,24 @@ func runAdopt(cmd *cobra.Command, args []string) error { } } - // Check for cycles (branch can't be ancestor of parent) - root, err := tree.Build(cfg) - if err != nil { - return err - } + // Check for cycles by walking configured parent links directly. A broken + // parent can disconnect branches from the trunk-rooted tree while their + // stackParent links still form a cycle. + seen := map[string]bool{} + for current := parent; current != trunk; { + if current == branchName { + return errors.New("cannot adopt: would create a cycle") + } + if seen[current] { + return errors.New("cannot adopt: parent chain contains a cycle") + } + seen[current] = true - parentNode := tree.FindNode(root, parent) - if parentNode != nil { - for _, ancestor := range tree.GetAncestors(parentNode) { - if ancestor.Name == branchName { - return errors.New("cannot adopt: would create a cycle") - } + next, parentErr := cfg.GetParent(current) + if parentErr != nil { + break } + current = next } s := style.New() diff --git a/cmd/restack.go b/cmd/restack.go index 9d230d6..526fd12 100644 --- a/cmd/restack.go +++ b/cmd/restack.go @@ -296,6 +296,9 @@ func doRestackWithState(g *git.Git, cfg *config.Config, branches []*tree.Node, o } else { rebaseErr = g.Rebase(parent, updateRefs) } + if rebaseErr != nil && !g.IsRebaseInProgress() { + return rebaseErr + } } if rebaseErr != nil { diff --git a/cmd/submit.go b/cmd/submit.go index 409414d..74790aa 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -84,7 +84,7 @@ func init() { submitCmd.Flags().BoolVarP(&submitUpdateOnlyFlag, "update", "u", false, "only update existing PRs, don't create new ones") submitCmd.Flags().BoolVarP(&submitPushOnlyFlag, "skip-prs", "s", false, "skip PR creation/update, only restack and push") submitCmd.Flags().BoolVarP(&submitYesFlag, "yes", "y", false, "skip interactive prompts and use auto-generated title/description for PRs") - submitCmd.Flags().BoolVar(&submitWebFlag, "web", false, "open created/updated PRs in web browser") + submitCmd.Flags().BoolVar(&submitWebFlag, "web", false, "open newly created PRs in web browser") submitCmd.Flags().StringVarP(&submitFromFlag, "from", "f", "", "submit from this branch toward leaves (default: entire stack; bare --from = current branch)") submitCmd.Flags().Lookup("from").NoOptDefVal = "HEAD" submitCmd.Flags().BoolVar(&submitNoUpdateRefsFlag, "no-update-refs", false, "do not pass --update-refs to git (preserves untracked bookmark branches pointing into the stack)") @@ -251,7 +251,7 @@ type SubmitOptions struct { DryRun bool // UpdateOnly skips creating new PRs; only existing PRs are updated. UpdateOnly bool - // OpenWeb opens created/updated PRs in the browser. + // OpenWeb opens newly created PRs in the browser. OpenWeb bool // PushOnly skips the PR creation/update phase entirely. PushOnly bool @@ -484,9 +484,6 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio if err := ghClient.GenerateAndPostStackComment(root, b.Name, trunk, d.prNum, remoteBranches); err != nil { fmt.Printf("%s failed to update stack comment for PR #%d: %v\n", s.WarningIcon(), d.prNum, err) } - if opts.OpenWeb { - prURLs = append(prURLs, ghClient.PRURL(d.prNum)) - } } } case prActionAdopt: @@ -499,9 +496,6 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio fmt.Printf("%s failed to adopt PR for %s: %v\n", s.WarningIcon(), s.Branch(b.Name), adoptErr) default: fmt.Printf("%s Adopted PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) - if opts.OpenWeb { - prURLs = append(prURLs, ghClient.PRURL(prNum)) - } } } case prActionCreate: @@ -514,15 +508,10 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio fmt.Printf("%s failed to create PR for %s: %v\n", s.WarningIcon(), s.Branch(b.Name), execErr) case adopted: fmt.Printf("%s Adopted PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) - if opts.OpenWeb { - prURLs = append(prURLs, ghClient.PRURL(prNum)) - } default: fmt.Printf("%s Created PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) - if opts.OpenWeb { - prURLs = append(prURLs, ghClient.PRURL(prNum)) - } } + prURLs = collectSubmitPRURL(prURLs, opts, d.action, prNum, adopted, execErr, ghClient) } } } @@ -539,6 +528,15 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio return nil } +// collectSubmitPRURL records only newly created PRs for --web. Updated and +// adopted PRs already existed, and their URLs are printed in the action output. +func collectSubmitPRURL(prURLs []string, opts SubmitOptions, action prAction, prNum int, adopted bool, err error, ghClient *github.Client) []string { + if !opts.OpenWeb || opts.DryRun || err != nil || action != prActionCreate || adopted || prNum <= 0 || ghClient == nil { + return prURLs + } + return append(prURLs, ghClient.PRURL(prNum)) +} + // prContext bundles the shared read-only context that is threaded through the // PR creation and adoption helpers. Grouping these avoids repeating the same // six parameters on every private function that participates in the submit diff --git a/cmd/submit_internal_test.go b/cmd/submit_internal_test.go index 463c81c..07b4c21 100644 --- a/cmd/submit_internal_test.go +++ b/cmd/submit_internal_test.go @@ -10,6 +10,8 @@ import ( "errors" "fmt" "os/exec" + "slices" + "strings" "testing" "github.com/boneskull/gh-stack/internal/config" @@ -533,6 +535,133 @@ func TestApplyMustPushForSkippedAncestors(t *testing.T) { }) } +func TestCollectSubmitPRURL(t *testing.T) { + ghClient := github.NewClientWithREST(nil, "owner", "repo") + + type prResult struct { + action prAction + prNum int + adopted bool + err error + } + + tests := []struct { + name string + opts SubmitOptions + results []prResult + want []string + }{ + { + name: "all branches create new PRs", + opts: SubmitOptions{OpenWeb: true}, + results: []prResult{ + {action: prActionCreate, prNum: 10}, + {action: prActionCreate, prNum: 11}, + }, + want: []string{ + "https://github.com/owner/repo/pull/10", + "https://github.com/owner/repo/pull/11", + }, + }, + { + name: "all PRs already exist and are updated", + opts: SubmitOptions{OpenWeb: true}, + results: []prResult{ + {action: prActionUpdate, prNum: 20}, + {action: prActionUpdate, prNum: 21}, + }, + }, + { + name: "mixed created updated and adopted PRs", + opts: SubmitOptions{OpenWeb: true}, + results: []prResult{ + {action: prActionUpdate, prNum: 30}, + {action: prActionCreate, prNum: 31}, + {action: prActionAdopt, prNum: 32, adopted: true}, + {action: prActionCreate, prNum: 33, adopted: true}, + {action: prActionCreate, prNum: 34}, + }, + want: []string{ + "https://github.com/owner/repo/pull/31", + "https://github.com/owner/repo/pull/34", + }, + }, + { + name: "create failure contributes no URL and later creates still open", + opts: SubmitOptions{OpenWeb: true}, + results: []prResult{ + {action: prActionCreate, prNum: 40, err: errors.New("create failed")}, + {action: prActionCreate, prNum: 41}, + }, + want: []string{"https://github.com/owner/repo/pull/41"}, + }, + { + name: "web disabled collects no URLs", + opts: SubmitOptions{}, + results: []prResult{ + {action: prActionCreate, prNum: 50}, + }, + }, + { + name: "dry run collects no URLs", + opts: SubmitOptions{DryRun: true, OpenWeb: true}, + results: []prResult{ + {action: prActionCreate, prNum: 60}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got []string + for _, result := range tt.results { + got = collectSubmitPRURL(got, tt.opts, result.action, result.prNum, result.adopted, result.err, ghClient) + } + if !slices.Equal(got, tt.want) { + t.Errorf("collectSubmitPRURL() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestRunSubmitRejectsSkipPRsWithWeb(t *testing.T) { + oldDryRun := submitDryRunFlag + oldCurrentOnly := submitCurrentOnlyFlag + oldUpdateOnly := submitUpdateOnlyFlag + oldPushOnly := submitPushOnlyFlag + oldYes := submitYesFlag + oldWeb := submitWebFlag + oldFrom := submitFromFlag + oldNoUpdateRefs := submitNoUpdateRefsFlag + t.Cleanup(func() { + submitDryRunFlag = oldDryRun + submitCurrentOnlyFlag = oldCurrentOnly + submitUpdateOnlyFlag = oldUpdateOnly + submitPushOnlyFlag = oldPushOnly + submitYesFlag = oldYes + submitWebFlag = oldWeb + submitFromFlag = oldFrom + submitNoUpdateRefsFlag = oldNoUpdateRefs + }) + + submitDryRunFlag = false + submitCurrentOnlyFlag = false + submitUpdateOnlyFlag = false + submitPushOnlyFlag = true + submitYesFlag = false + submitWebFlag = true + submitFromFlag = "" + submitNoUpdateRefsFlag = false + + err := runSubmit(nil, nil) + if err == nil { + t.Fatal("expected --skip-prs/--web validation error") + } + if !strings.Contains(err.Error(), "--skip-prs and --web cannot be used together") { + t.Fatalf("unexpected error: %v", err) + } +} + func TestDeleteMergedBranchClearsPRBase(t *testing.T) { cfg, dir := setupTestRepoWithDir(t) g := git.New(dir)