Skip to content

Commit da3dcda

Browse files
committed
fix(submit): only prompt to publish draft when base transitions to trunk
Resolves #95. Previously, `gh stack submit` would prompt to publish a draft PR every time it ran, as long as the PR was a draft and targeted trunk. This got annoying fast. The fix adds a `stackPRBase` key to `.git/config` (per branch) that persists the last-known remote base after each successful submit run. The publish prompt now fires only when the stored base differs from trunk — i.e., the PR's base is genuinely moving to trunk for the first time since the last submit. Cleanup is consistent: `unlink`, `orphan`, and sync's merged-branch handlers all clear `stackPRBase` alongside the other per-branch config keys. Sync's retarget-to-trunk path also persists the new base so subsequent submit runs don't re-prompt.
1 parent 7ea4291 commit da3dcda

9 files changed

Lines changed: 401 additions & 4 deletions

File tree

cmd/orphan.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ func runOrphan(cmd *cobra.Command, args []string) error {
7676
_ = cfg.RemoveParent(desc.Name) //nolint:errcheck // best effort cleanup
7777
_ = cfg.RemovePR(desc.Name) //nolint:errcheck // best effort cleanup
7878
_ = cfg.RemoveForkPoint(desc.Name) //nolint:errcheck // best effort cleanup
79+
_ = cfg.RemovePRBase(desc.Name) //nolint:errcheck // best effort cleanup
7980
fmt.Printf("%s Orphaned %s\n", s.SuccessIcon(), s.Branch(desc.Name))
8081
}
8182
}
@@ -84,6 +85,7 @@ func runOrphan(cmd *cobra.Command, args []string) error {
8485
_ = cfg.RemoveParent(branchName) //nolint:errcheck // best effort cleanup
8586
_ = cfg.RemovePR(branchName) //nolint:errcheck // best effort cleanup
8687
_ = cfg.RemoveForkPoint(branchName) //nolint:errcheck // best effort cleanup
88+
_ = cfg.RemovePRBase(branchName) //nolint:errcheck // best effort cleanup
8789
fmt.Printf("%s Orphaned %s\n", s.SuccessIcon(), s.Branch(branchName))
8890

8991
return nil

cmd/submit.go

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -468,14 +468,15 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio
468468
fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), d.prNum, err)
469469
} else {
470470
fmt.Println(s.Success("ok"))
471+
_ = cfg.SetPRBase(b.Name, parent) //nolint:errcheck // best effort — used for transition detection only
471472
if opts.OpenWeb {
472473
prURLs = append(prURLs, ghClient.PRURL(d.prNum))
473474
}
474475
}
475476
if err := ghClient.GenerateAndPostStackComment(root, b.Name, trunk, d.prNum, remoteBranches); err != nil {
476477
fmt.Printf("%s failed to update stack comment for PR #%d: %v\n", s.WarningIcon(), d.prNum, err)
477478
}
478-
maybeMarkPRReady(ghClient, d.prNum, b.Name, parent, trunk, s)
479+
maybeMarkPRReady(ghClient, cfg, d.prNum, b.Name, parent, trunk, s)
479480
}
480481
case prActionAdopt:
481482
if opts.DryRun {
@@ -561,6 +562,9 @@ func executePRCreate(pCtx prContext, branch, base, title, body string, draft boo
561562
return pr.Number, false, fmt.Errorf("PR created but failed to store number: %w", err)
562563
}
563564

565+
// Persist the base so future submit runs can detect trunk transitions.
566+
_ = pCtx.cfg.SetPRBase(branch, base) //nolint:errcheck // best effort
567+
564568
if node := tree.FindNode(pCtx.root, branch); node != nil {
565569
node.PR = pr.Number
566570
}
@@ -711,23 +715,51 @@ func adoptExistingPRDirect(pCtx prContext, branch, base string, existingPR *gith
711715
}
712716
}
713717

718+
// Persist the new base for transition detection on future submit runs.
719+
_ = pCtx.cfg.SetPRBase(branch, base) //nolint:errcheck // best effort
720+
714721
// Add/update stack navigation comment
715722
if err := pCtx.ghClient.GenerateAndPostStackComment(pCtx.root, branch, pCtx.trunk, existingPR.Number, pCtx.remoteBranches); err != nil {
716723
fmt.Printf("%s failed to update stack comment: %v\n", pCtx.s.WarningIcon(), err)
717724
}
718725

719-
// If adopted PR is a draft and targets trunk, offer to publish
720-
if existingPR.Draft && base == pCtx.trunk {
726+
// Prompt to publish if the PR is a draft and its base is transitioning to trunk.
727+
// We use the live pre-adoption base (existingPR.Base.Ref) here since stored base
728+
// metadata may not exist yet for a freshly adopted PR.
729+
if existingPR.Draft && base == pCtx.trunk && existingPR.Base.Ref != pCtx.trunk {
721730
promptMarkPRReady(pCtx.ghClient, existingPR.Number, branch, pCtx.trunk, pCtx.s)
722731
}
723732

724733
return existingPR.Number, nil
725734
}
726735

736+
// isTransitionToTrunk reports whether a PR's base branch is moving to trunk for
737+
// the first time as tracked by gh-stack. It uses the stored last-known PR base
738+
// (persisted after each successful submit) to detect the transition, avoiding
739+
// any additional GitHub API calls.
740+
//
741+
// Returns true when:
742+
// - no stored base exists yet (first run after PR creation/adoption), or
743+
// - stored base is not trunk (base is changing from something else to trunk).
744+
//
745+
// Returns false when the stored base is already trunk, meaning the PR was
746+
// already targeting trunk on the previous submit run.
747+
func isTransitionToTrunk(cfg *config.Config, branch, trunk string) bool {
748+
storedBase, err := cfg.GetPRBase(branch)
749+
if err == nil && storedBase == trunk {
750+
return false
751+
}
752+
return true
753+
}
754+
727755
// maybeMarkPRReady checks if a PR is a draft targeting trunk and offers to publish it.
728756
// This handles the case where a PR was created as a draft (middle of stack) but now
729757
// targets trunk because its parent was merged.
730-
func maybeMarkPRReady(ghClient *github.Client, prNumber int, branch, base, trunk string, s *style.Style) {
758+
//
759+
// The prompt fires only when the base branch is transitioning to trunk for the first
760+
// time — i.e., the stored last-known base is not already trunk. This prevents the
761+
// prompt from appearing on every submit run once the PR already targets trunk.
762+
func maybeMarkPRReady(ghClient *github.Client, cfg *config.Config, prNumber int, branch, base, trunk string, s *style.Style) {
731763
// Only relevant if PR now targets trunk
732764
if base != trunk {
733765
return
@@ -739,6 +771,11 @@ func maybeMarkPRReady(ghClient *github.Client, prNumber int, branch, base, trunk
739771
return
740772
}
741773

774+
// Only prompt when the base is transitioning to trunk this run.
775+
if !isTransitionToTrunk(cfg, branch, trunk) {
776+
return
777+
}
778+
742779
promptMarkPRReady(ghClient, prNumber, branch, trunk, s)
743780
}
744781

cmd/submit_internal_test.go

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,13 @@ package cmd
99
import (
1010
"errors"
1111
"fmt"
12+
"os/exec"
1213
"testing"
1314

15+
"github.com/boneskull/gh-stack/internal/config"
16+
"github.com/boneskull/gh-stack/internal/git"
1417
"github.com/boneskull/gh-stack/internal/github"
18+
"github.com/boneskull/gh-stack/internal/style"
1519
"github.com/boneskull/gh-stack/internal/tree"
1620
)
1721

@@ -319,6 +323,107 @@ func TestIsBaseBranchInvalidError(t *testing.T) {
319323
}
320324
}
321325

326+
func setupTestRepo(t *testing.T) *config.Config {
327+
t.Helper()
328+
dir := t.TempDir()
329+
if err := exec.Command("git", "init", dir).Run(); err != nil {
330+
t.Fatalf("git init failed: %v", err)
331+
}
332+
exec.Command("git", "-C", dir, "config", "user.email", "test@test.com").Run() //nolint:errcheck
333+
exec.Command("git", "-C", dir, "config", "user.name", "Test").Run() //nolint:errcheck
334+
cfg, err := config.Load(dir)
335+
if err != nil {
336+
t.Fatalf("config.Load failed: %v", err)
337+
}
338+
return cfg
339+
}
340+
341+
// setupTestRepoWithDir is like setupTestRepo but also returns the directory path
342+
// for callers that need to run git commands directly or construct a git.Git instance.
343+
func setupTestRepoWithDir(t *testing.T) (*config.Config, string) {
344+
t.Helper()
345+
dir := t.TempDir()
346+
if err := exec.Command("git", "init", dir).Run(); err != nil {
347+
t.Fatalf("git init failed: %v", err)
348+
}
349+
exec.Command("git", "-C", dir, "config", "user.email", "test@test.com").Run() //nolint:errcheck
350+
exec.Command("git", "-C", dir, "config", "user.name", "Test").Run() //nolint:errcheck
351+
// Create an initial commit so the repo has a HEAD and we can create branches.
352+
exec.Command("git", "-C", dir, "commit", "--allow-empty", "-m", "init").Run() //nolint:errcheck
353+
cfg, err := config.Load(dir)
354+
if err != nil {
355+
t.Fatalf("config.Load failed: %v", err)
356+
}
357+
return cfg, dir
358+
}
359+
360+
func TestIsTransitionToTrunk(t *testing.T) {
361+
trunk := "main"
362+
363+
t.Run("no_stored_base_returns_true", func(t *testing.T) {
364+
cfg := setupTestRepo(t)
365+
// No stored base — first run after PR creation; should prompt.
366+
if !isTransitionToTrunk(cfg, "feat-a", trunk) {
367+
t.Error("expected true when no stored base exists")
368+
}
369+
})
370+
371+
t.Run("stored_base_is_not_trunk_returns_true", func(t *testing.T) {
372+
cfg := setupTestRepo(t)
373+
// Branch previously targeted a non-trunk parent; now it targets trunk.
374+
if err := cfg.SetPRBase("feat-a", "feat-parent"); err != nil {
375+
t.Fatalf("SetPRBase failed: %v", err)
376+
}
377+
if !isTransitionToTrunk(cfg, "feat-a", trunk) {
378+
t.Error("expected true when stored base is not trunk")
379+
}
380+
})
381+
382+
t.Run("stored_base_is_trunk_returns_false", func(t *testing.T) {
383+
cfg := setupTestRepo(t)
384+
// Branch was already targeting trunk on the previous run; don't re-prompt.
385+
if err := cfg.SetPRBase("feat-a", trunk); err != nil {
386+
t.Fatalf("SetPRBase failed: %v", err)
387+
}
388+
if isTransitionToTrunk(cfg, "feat-a", trunk) {
389+
t.Error("expected false when stored base is already trunk")
390+
}
391+
})
392+
393+
t.Run("different_branches_are_independent", func(t *testing.T) {
394+
cfg := setupTestRepo(t)
395+
// feat-a already targeting trunk; feat-b has no stored base.
396+
if err := cfg.SetPRBase("feat-a", trunk); err != nil {
397+
t.Fatalf("SetPRBase failed: %v", err)
398+
}
399+
if isTransitionToTrunk(cfg, "feat-a", trunk) {
400+
t.Error("feat-a: expected false when stored base is trunk")
401+
}
402+
if !isTransitionToTrunk(cfg, "feat-b", trunk) {
403+
t.Error("feat-b: expected true when no stored base exists")
404+
}
405+
})
406+
407+
t.Run("custom_trunk_name_works", func(t *testing.T) {
408+
cfg := setupTestRepo(t)
409+
customTrunk := "master"
410+
// Stored as "master"; should not prompt.
411+
if err := cfg.SetPRBase("feat-a", customTrunk); err != nil {
412+
t.Fatalf("SetPRBase failed: %v", err)
413+
}
414+
if isTransitionToTrunk(cfg, "feat-a", customTrunk) {
415+
t.Error("expected false with custom trunk name already stored")
416+
}
417+
// Stored as something else; should prompt.
418+
if err := cfg.SetPRBase("feat-b", "other-branch"); err != nil {
419+
t.Fatalf("SetPRBase failed: %v", err)
420+
}
421+
if !isTransitionToTrunk(cfg, "feat-b", customTrunk) {
422+
t.Error("expected true when stored base is not the custom trunk")
423+
}
424+
})
425+
}
426+
322427
func TestApplyMustPushForSkippedAncestors(t *testing.T) {
323428
main := &tree.Node{Name: "main"}
324429
featA := &tree.Node{Name: "feat-a", Parent: main}
@@ -392,3 +497,84 @@ func TestApplyMustPushForSkippedAncestors(t *testing.T) {
392497
}
393498
})
394499
}
500+
501+
func TestDeleteMergedBranchClearsPRBase(t *testing.T) {
502+
cfg, dir := setupTestRepoWithDir(t)
503+
g := git.New(dir)
504+
s := style.New()
505+
506+
trunk, err := g.CurrentBranch()
507+
if err != nil {
508+
t.Fatalf("CurrentBranch failed: %v", err)
509+
}
510+
if err := cfg.SetTrunk(trunk); err != nil {
511+
t.Fatalf("SetTrunk failed: %v", err)
512+
}
513+
514+
// Create feature-a with a commit so git can delete it later.
515+
if err := exec.Command("git", "-C", dir, "checkout", "-b", "feature-a").Run(); err != nil {
516+
t.Fatalf("create branch failed: %v", err)
517+
}
518+
if err := exec.Command("git", "-C", dir, "commit", "--allow-empty", "-m", "feat").Run(); err != nil {
519+
t.Fatalf("commit failed: %v", err)
520+
}
521+
if err := exec.Command("git", "-C", dir, "checkout", trunk).Run(); err != nil {
522+
t.Fatalf("checkout trunk failed: %v", err)
523+
}
524+
525+
if err := cfg.SetParent("feature-a", trunk); err != nil {
526+
t.Fatalf("SetParent failed: %v", err)
527+
}
528+
if err := cfg.SetPR("feature-a", 10); err != nil {
529+
t.Fatalf("SetPR failed: %v", err)
530+
}
531+
if err := cfg.SetPRBase("feature-a", trunk); err != nil {
532+
t.Fatalf("SetPRBase failed: %v", err)
533+
}
534+
535+
currentBranch := trunk
536+
deleteMergedBranch(g, cfg, "feature-a", trunk, &currentBranch, s)
537+
538+
if v, err := cfg.GetPRBase("feature-a"); err == nil {
539+
t.Errorf("expected stackPRBase to be removed after deleteMergedBranch, got %q", v)
540+
}
541+
if v, err := cfg.GetPR("feature-a"); err == nil {
542+
t.Errorf("expected stackPR to be removed after deleteMergedBranch, got %d", v)
543+
}
544+
}
545+
546+
func TestOrphanMergedBranchClearsPRBase(t *testing.T) {
547+
cfg, dir := setupTestRepoWithDir(t)
548+
g := git.New(dir)
549+
s := style.New()
550+
551+
trunk, err := g.CurrentBranch()
552+
if err != nil {
553+
t.Fatalf("CurrentBranch failed: %v", err)
554+
}
555+
if err := cfg.SetTrunk(trunk); err != nil {
556+
t.Fatalf("SetTrunk failed: %v", err)
557+
}
558+
559+
if err := cfg.SetParent("feature-a", trunk); err != nil {
560+
t.Fatalf("SetParent failed: %v", err)
561+
}
562+
if err := cfg.SetPR("feature-a", 11); err != nil {
563+
t.Fatalf("SetPR failed: %v", err)
564+
}
565+
if err := cfg.SetPRBase("feature-a", trunk); err != nil {
566+
t.Fatalf("SetPRBase failed: %v", err)
567+
}
568+
569+
orphanMergedBranch(cfg, "feature-a", s)
570+
571+
if v, err := cfg.GetPRBase("feature-a"); err == nil {
572+
t.Errorf("expected stackPRBase to be removed after orphanMergedBranch, got %q", v)
573+
}
574+
if v, err := cfg.GetPR("feature-a"); err == nil {
575+
t.Errorf("expected stackPR to be removed after orphanMergedBranch, got %d", v)
576+
}
577+
if v, err := cfg.GetParent("feature-a"); err == nil {
578+
t.Errorf("expected stackParent to be removed after orphanMergedBranch, got %q", v)
579+
}
580+
}

cmd/sync.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,10 @@ func runSync(cmd *cobra.Command, args []string) error {
318318
if rt.childPR > 0 {
319319
if updateErr := gh.UpdatePRBase(rt.childPR, trunk); updateErr != nil {
320320
fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), rt.childPR, updateErr)
321+
} else {
322+
// Persist the new base so the submit publish-prompt knows this
323+
// PR is already targeting trunk on its next run.
324+
_ = cfg.SetPRBase(rt.childName, trunk) //nolint:errcheck // best effort
321325
}
322326
}
323327

@@ -473,6 +477,7 @@ func deleteMergedBranch(g *git.Git, cfg *config.Config, branch, trunk string, cu
473477
_ = cfg.RemoveParent(branch) //nolint:errcheck // best effort cleanup
474478
_ = cfg.RemovePR(branch) //nolint:errcheck // best effort cleanup
475479
_ = cfg.RemoveForkPoint(branch) //nolint:errcheck // best effort cleanup
480+
_ = cfg.RemovePRBase(branch) //nolint:errcheck // best effort cleanup
476481
if err := g.DeleteBranch(branch); err != nil {
477482
fmt.Printf("%s could not delete branch %s: %v\n", s.WarningIcon(), s.Branch(branch), err)
478483
}
@@ -485,5 +490,6 @@ func orphanMergedBranch(cfg *config.Config, branch string, s *style.Style) merge
485490
_ = cfg.RemoveParent(branch) //nolint:errcheck // best effort cleanup
486491
_ = cfg.RemovePR(branch) //nolint:errcheck // best effort cleanup
487492
_ = cfg.RemoveForkPoint(branch) //nolint:errcheck // best effort cleanup
493+
_ = cfg.RemovePRBase(branch) //nolint:errcheck // best effort cleanup
488494
return mergedActionOrphan
489495
}

cmd/unlink.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ func runUnlink(cmd *cobra.Command, args []string) error {
4242
if err := cfg.RemovePR(branch); err != nil {
4343
return err
4444
}
45+
_ = cfg.RemovePRBase(branch) //nolint:errcheck // best effort cleanup
4546

4647
s := style.New()
4748
fmt.Printf("%s Unlinked PR from branch %s\n", s.SuccessIcon(), s.Branch(branch))

0 commit comments

Comments
 (0)