Skip to content

Commit 75855f3

Browse files
committed
modify: only require submit when changes affect PRs
Previously, `gh stack modify` always transitioned to `PhasePendingSubmit` after completing on any stack with a remote ID (`s.ID != ""`). This blocked the user from running another `modify` until they ran `gh stack submit`, even when the modifications only touched local branches without PRs. This was overly restrictive. If a user is working at the top of their stack with branches that haven't been pushed or had PRs created yet, restructuring those branches is a purely local operation — there is no remote state to reconcile, and no reason to force a submit before allowing further modifies. ## What changed The condition for entering `PhasePendingSubmit` is now `s.ID != "" && affectsPRs` instead of just `s.ID != ""`. A new `affectsPRs` flag is tracked throughout the apply process. It is set to `true` when any of the following occurs: - A **renamed** branch has a `PullRequest` ref - A **folded** branch (source or target) has a `PullRequest` ref - A **dropped** branch has a `PullRequest` ref - A **rebased** branch (during cascading rebase) has a `PullRequest` ref If none of these conditions are met, the modify state file is cleared immediately — no pending-submit lock, no "run `gh stack submit`" prompt. ## Changes by file **`internal/modify/state.go`** - Added `AffectsPRs bool` field to `StateFile`. This persists the flag across conflict boundaries so that `ContinueApply` knows whether actions applied before the conflict already affected PR branches. **`internal/modify/apply.go`** - `ApplyPlan`: tracks `affectsPRs` through each step (rename, fold, drop, rebase). Saves the flag into conflict state when a conflict occurs. Uses `s.ID != "" && affectsPRs` for the pending-submit decision. - `ContinueApply`: initializes `affectsPRs` from the saved state file, then checks the conflict branch and remaining branches for PRs during the cascading rebase. Uses the same combined condition. - Both functions set `result.NeedsSubmit` / show the "run submit" message only when the flag is true. **`internal/tui/modifyview/types.go`** - Added `NeedsSubmit bool` to `ApplyResult` so the caller can use it for the success message. **`cmd/modify.go`** - `printModifySuccess` now takes its cue from `result.NeedsSubmit` instead of `s.ID != ""`. The "run `gh stack submit`" hint is only shown when PR branches were actually affected. **`internal/modify/apply_test.go`** - Updated `TestApplyPlan_PendingSubmitForRemoteStack` to use branches with PRs and trigger an actual rebase, validating the pending-submit path correctly. - Added `TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches`: remote stack where no branches have PRs → state is cleared. - Added `TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected`: remote stack with a mix of PR and non-PR branches, only the non-PR branch is renamed → state is cleared, `NeedsSubmit` is false. ## Behavior summary | Scenario | Before | After | |---|---|---| | Modify on local stack (no remote ID) | State cleared | State cleared (unchanged) | | Modify on remote stack, PR branches affected | `PhasePendingSubmit` | `PhasePendingSubmit` (unchanged) | | Modify on remote stack, only local branches affected | `PhasePendingSubmit` ❌ | State cleared ✅ | The `CheckStateGuard` function (used by `add`, `push`, `sync`, `unstack`, `rebase`) already did not block on `PhasePendingSubmit`, so those commands are unaffected by this change.
1 parent f9fbd25 commit 75855f3

5 files changed

Lines changed: 166 additions & 15 deletions

File tree

cmd/modify.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -149,13 +149,13 @@ func runModify(cfg *config.Config) error {
149149
}
150150

151151
// Print success summary
152-
printModifySuccess(cfg, applyResult, s.ID != "")
152+
printModifySuccess(cfg, applyResult)
153153

154154
return nil
155155
}
156156

157157
// printModifySuccess prints a summary of what was applied.
158-
func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult, hasRemoteStack bool) {
158+
func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult) {
159159
if result == nil {
160160
return
161161
}
@@ -178,7 +178,7 @@ func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult, hasR
178178
}
179179

180180
cfg.Printf("")
181-
if hasRemoteStack {
181+
if result.NeedsSubmit {
182182
cfg.Printf("Run `%s` to push your changes and update the stack of PRs on GitHub",
183183
cfg.ColorCyan("gh stack submit"))
184184
}

internal/modify/apply.go

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ func ApplyPlan(
137137

138138
result := &modifyview.ApplyResult{Success: true}
139139

140+
// Track whether any action affects a branch with a PR.
141+
affectsPRs := false
140142
// Collect original refs for rebase --onto, including trunk
141143
branchNames := make([]string, 0, len(s.Branches)+1)
142144
branchNames = append(branchNames, s.Trunk.Branch)
@@ -207,6 +209,9 @@ func ApplyPlan(
207209
OldName: oldName,
208210
NewName: newName,
209211
})
212+
if n.Ref.PullRequest != nil {
213+
affectsPRs = true
214+
}
210215
cfg.Successf("Renamed %s → %s", oldName, newName)
211216
}
212217
}
@@ -255,6 +260,15 @@ func ApplyPlan(
255260

256261
baseBranch := s.ActiveBaseBranch(foldBranch)
257262

263+
// Check if fold source or target has a PR
264+
if n.Ref.PullRequest != nil {
265+
affectsPRs = true
266+
}
267+
targetIdx := s.IndexOf(targetBranch)
268+
if targetIdx >= 0 && s.Branches[targetIdx].PullRequest != nil {
269+
affectsPRs = true
270+
}
271+
258272
if n.PendingAction.Type == modifyview.ActionFoldDown {
259273
// Fold-down: cherry-pick the folded branch's commits onto the target.
260274
commits, err := git.LogRange(baseBranch, foldBranch)
@@ -301,6 +315,7 @@ func ApplyPlan(
301315
stateFile.RemainingBranches = remaining
302316
stateFile.OriginalBranch = currentBranch
303317
stateFile.OriginalRefs = originalParentTips
318+
stateFile.AffectsPRs = affectsPRs
304319
if saveErr := SaveState(gitDir, stateFile); saveErr != nil {
305320
cfg.Warningf("failed to save conflict state: %v", saveErr)
306321
}
@@ -351,6 +366,7 @@ func ApplyPlan(
351366
Branch: dropBranch,
352367
PRNumber: n.Ref.PullRequest.Number,
353368
})
369+
affectsPRs = true
354370
}
355371

356372
s.Branches = append(s.Branches[:dropIdx], s.Branches[dropIdx+1:]...)
@@ -466,6 +482,10 @@ func ApplyPlan(
466482
conflict.ConflictedFiles = files
467483
}
468484

485+
if b.PullRequest != nil {
486+
affectsPRs = true
487+
}
488+
469489
// Save conflict state so --continue can resume
470490
remaining := make([]string, 0)
471491
for j := i + 1; j < len(s.Branches); j++ {
@@ -479,6 +499,7 @@ func ApplyPlan(
479499
stateFile.RemainingBranches = remaining
480500
stateFile.OriginalBranch = currentBranch
481501
stateFile.OriginalRefs = originalParentTips
502+
stateFile.AffectsPRs = affectsPRs
482503
if saveErr := SaveState(gitDir, stateFile); saveErr != nil {
483504
cfg.Warningf("failed to save conflict state: %v", saveErr)
484505
}
@@ -492,6 +513,9 @@ func ApplyPlan(
492513
}
493514

494515
cfg.Successf("Rebased %s onto %s", b.Branch, newBase)
516+
if b.PullRequest != nil {
517+
affectsPRs = true
518+
}
495519
result.MovedBranches++
496520
}
497521

@@ -506,14 +530,15 @@ func ApplyPlan(
506530
// Update base SHAs
507531
updateBaseSHAs(s)
508532

509-
// Update state file phase
510-
if s.ID != "" {
533+
// Update state file phase — only require submit when PRs are affected
534+
needsSubmit := s.ID != "" && affectsPRs
535+
result.NeedsSubmit = needsSubmit
536+
if needsSubmit {
511537
stateFile.Phase = PhasePendingSubmit
512538
if err := SaveState(gitDir, stateFile); err != nil {
513539
cfg.Warningf("failed to update modify state: %s", err)
514540
}
515541
} else {
516-
// No remote stack — clear the state file
517542
ClearState(gitDir)
518543
}
519544

@@ -662,6 +687,14 @@ func ContinueApply(
662687
return fmt.Errorf("stack at index %d not found (stack file may have changed)", state.StackIndex)
663688
}
664689

690+
// Carry forward whether any prior actions already affected PRs
691+
affectsPRs := state.AffectsPRs
692+
693+
// Check the conflict branch itself
694+
if idx := s.IndexOf(state.ConflictBranch); idx >= 0 && s.Branches[idx].PullRequest != nil {
695+
affectsPRs = true
696+
}
697+
665698
// Finish the in-progress git operation (rebase or cherry-pick)
666699
if state.ConflictType == "cherry_pick" {
667700
if err := git.CherryPickContinue(); err != nil {
@@ -739,6 +772,7 @@ func ContinueApply(
739772
}
740773
state.ConflictBranch = branchName
741774
state.RemainingBranches = remaining
775+
state.AffectsPRs = affectsPRs
742776
_ = SaveState(gitDir, state)
743777

744778
cfg.Warningf("Conflict rebasing %s", branchName)
@@ -757,8 +791,10 @@ func ContinueApply(
757791
}
758792

759793
cfg.Successf("Rebased %s onto %s", branchName, newBase)
794+
if b.PullRequest != nil {
795+
affectsPRs = true
796+
}
760797
}
761-
762798
// All rebases done — check out the best branch
763799
if state.OriginalBranch != "" {
764800
targetBranch := resolveCheckoutBranch(state.OriginalBranch, state.Plan, state.Snapshot, s)
@@ -771,8 +807,9 @@ func ContinueApply(
771807
// Update base SHAs
772808
updateBaseSHAs(s)
773809

774-
// Transition to pending_submit or clear
775-
if s.ID != "" {
810+
// Transition to pending_submit only when PRs are affected
811+
needsSubmit := s.ID != "" && affectsPRs
812+
if needsSubmit {
776813
state.Phase = PhasePendingSubmit
777814
state.ConflictBranch = ""
778815
state.RemainingBranches = nil
@@ -790,7 +827,7 @@ func ContinueApply(
790827
}
791828

792829
cfg.Successf("Stack modified successfully")
793-
if state.PriorRemoteStackID != "" {
830+
if needsSubmit {
794831
cfg.Printf("")
795832
cfg.Printf("Run `%s` to push your changes and update the stack of PRs on GitHub",
796833
cfg.ColorCyan("gh stack submit"))

internal/modify/apply_test.go

Lines changed: 113 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1262,8 +1262,8 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) {
12621262
ID: "remote-stack-123",
12631263
Trunk: stack.BranchRef{Branch: "main"},
12641264
Branches: []stack.BranchRef{
1265-
{Branch: "A"},
1266-
{Branch: "B"},
1265+
{Branch: "A", PullRequest: &stack.PullRequestRef{Number: 1}},
1266+
{Branch: "B", PullRequest: &stack.PullRequestRef{Number: 2}},
12671267
},
12681268
}
12691269

@@ -1277,7 +1277,7 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) {
12771277
}
12781278

12791279
mock := newApplyMock(gitDir, branchSHAs)
1280-
mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil }
1280+
mock.IsAncestorFn = func(a, d string) (bool, error) { return false, nil }
12811281
mock.MergeBaseFn = func(a, b string) (string, error) {
12821282
if a == "main" && b == "A" {
12831283
return branchSHAs["main"], nil
@@ -1295,16 +1295,19 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) {
12951295
defer cfg.Out.Close()
12961296
defer cfg.Err.Close()
12971297

1298+
// Reverse nodes so position differs → triggers rebase of PR branches
12981299
nodes := makeNodes(&sf.Stacks[0])
1300+
nodes[0], nodes[1] = nodes[1], nodes[0]
12991301

1300-
_, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs)
1302+
result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs)
13011303
require.NoError(t, err)
13021304

1303-
// Remote stack should transition to "pending_submit"
1305+
// Remote stack with PR branches affected should transition to "pending_submit"
13041306
state, loadErr := LoadState(gitDir)
13051307
require.NoError(t, loadErr)
13061308
require.NotNil(t, state)
13071309
assert.Equal(t, "pending_submit", state.Phase)
1310+
assert.True(t, result.NeedsSubmit, "NeedsSubmit should be true when PR branches are affected")
13081311
}
13091312

13101313
func TestApplyPlan_ClearsStateForLocalStack(t *testing.T) {
@@ -1345,6 +1348,111 @@ func TestApplyPlan_ClearsStateForLocalStack(t *testing.T) {
13451348
assert.False(t, StateExists(gitDir))
13461349
}
13471350

1351+
func TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches(t *testing.T) {
1352+
// Remote stack (has ID) but branches have no PRs — local-only modify
1353+
s := stack.Stack{
1354+
ID: "remote-stack-456",
1355+
Trunk: stack.BranchRef{Branch: "main"},
1356+
Branches: []stack.BranchRef{
1357+
{Branch: "A"},
1358+
{Branch: "B"},
1359+
},
1360+
}
1361+
1362+
gitDir := t.TempDir()
1363+
sf := writeTestStackFile(t, gitDir, s)
1364+
1365+
branchSHAs := map[string]string{
1366+
"main": "sha-main",
1367+
"A": "sha-A",
1368+
"B": "sha-B",
1369+
}
1370+
1371+
mock := newApplyMock(gitDir, branchSHAs)
1372+
mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil }
1373+
mock.MergeBaseFn = func(a, b string) (string, error) {
1374+
if a == "main" && b == "A" {
1375+
return branchSHAs["main"], nil
1376+
}
1377+
if a == "A" && b == "B" {
1378+
return branchSHAs["A"], nil
1379+
}
1380+
return "merge-base", nil
1381+
}
1382+
1383+
restore := git.SetOps(mock)
1384+
defer restore()
1385+
1386+
cfg, _, _ := config.NewTestConfig()
1387+
defer cfg.Out.Close()
1388+
defer cfg.Err.Close()
1389+
1390+
nodes := makeNodes(&sf.Stacks[0])
1391+
1392+
result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs)
1393+
require.NoError(t, err)
1394+
1395+
// Remote stack but no PR branches affected → state should be cleared
1396+
assert.False(t, StateExists(gitDir), "state file should be cleared when no PR branches are affected")
1397+
assert.False(t, result.NeedsSubmit, "NeedsSubmit should be false when no PR branches are affected")
1398+
}
1399+
1400+
func TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected(t *testing.T) {
1401+
// Stack with one PR branch (A) and one local branch (B).
1402+
// Only rename the local branch B — PRs should not be affected.
1403+
s := stack.Stack{
1404+
ID: "remote-stack-789",
1405+
Trunk: stack.BranchRef{Branch: "main"},
1406+
Branches: []stack.BranchRef{
1407+
{Branch: "A", PullRequest: &stack.PullRequestRef{Number: 1}},
1408+
{Branch: "B"},
1409+
},
1410+
}
1411+
1412+
gitDir := t.TempDir()
1413+
sf := writeTestStackFile(t, gitDir, s)
1414+
1415+
branchSHAs := map[string]string{
1416+
"main": "sha-main",
1417+
"A": "sha-A",
1418+
"B": "sha-B",
1419+
}
1420+
1421+
mock := newApplyMock(gitDir, branchSHAs)
1422+
mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil }
1423+
mock.MergeBaseFn = func(a, b string) (string, error) {
1424+
if a == "main" && b == "A" {
1425+
return branchSHAs["main"], nil
1426+
}
1427+
if a == "A" && b == "B" || a == "A" && b == "B-renamed" {
1428+
return branchSHAs["A"], nil
1429+
}
1430+
return "merge-base", nil
1431+
}
1432+
mock.RenameBranchFn = func(old, newName string) error { return nil }
1433+
1434+
restore := git.SetOps(mock)
1435+
defer restore()
1436+
1437+
cfg, _, _ := config.NewTestConfig()
1438+
defer cfg.Out.Close()
1439+
defer cfg.Err.Close()
1440+
1441+
nodes := makeNodes(&sf.Stacks[0])
1442+
// Rename only the non-PR branch B
1443+
nodes[1].PendingAction = &modifyview.PendingAction{
1444+
Type: modifyview.ActionRename,
1445+
NewName: "B-renamed",
1446+
}
1447+
1448+
result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs)
1449+
require.NoError(t, err)
1450+
1451+
// Only non-PR branch was renamed — should clear state, not pending submit
1452+
assert.False(t, StateExists(gitDir), "state file should be cleared when only non-PR branches are renamed")
1453+
assert.False(t, result.NeedsSubmit, "NeedsSubmit should be false when only non-PR branches are affected")
1454+
}
1455+
13481456
// ─── resolveCheckoutBranch ──────────────────────────────────────────────────
13491457

13501458
func TestResolveCheckoutBranch_StillInStack(t *testing.T) {

internal/modify/state.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,11 @@ type StateFile struct {
3939
// Cherry-pick conflict context — which fold was in progress
4040
FoldBranch string `json:"fold_branch,omitempty"` // branch being folded
4141
FoldTarget string `json:"fold_target,omitempty"` // branch receiving the cherry-pick
42+
43+
// AffectsPRs records whether any action so far has affected a branch with
44+
// a PR. Persisted across conflict boundaries so ContinueApply can combine
45+
// it with checks on remaining branches.
46+
AffectsPRs bool `json:"affects_prs,omitempty"`
4247
}
4348

4449
// Snapshot captures the pre-modify state for unwind/recovery.

internal/tui/modifyview/types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ type ApplyResult struct {
4747
DroppedPRs []DroppedPR
4848
RenamedBranches []RenamedBranch
4949
MovedBranches int
50+
NeedsSubmit bool // true when the modify affected branches with PRs
5051
Message string
5152
}
5253

0 commit comments

Comments
 (0)