diff --git a/cmd/modify.go b/cmd/modify.go index 4e1456c..3842e38 100644 --- a/cmd/modify.go +++ b/cmd/modify.go @@ -33,7 +33,8 @@ Operations available: • Rename branches All changes are staged in the TUI and applied together when you press Ctrl+S. -After applying, run 'gh stack submit' to push changes and recreate the stack on GitHub.`, +If your changes affect branches with pull requests, run 'gh stack submit' +afterward to push changes, update PRs, and recreate the stack on GitHub.`, Example: ` # Open the interactive TUI to restructure the stack $ gh stack modify @@ -149,13 +150,13 @@ func runModify(cfg *config.Config) error { } // Print success summary - printModifySuccess(cfg, applyResult, s.ID != "") + printModifySuccess(cfg, applyResult) return nil } // printModifySuccess prints a summary of what was applied. -func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult, hasRemoteStack bool) { +func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult) { if result == nil { return } @@ -178,7 +179,7 @@ func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult, hasR } cfg.Printf("") - if hasRemoteStack { + if result.NeedsSubmit { cfg.Printf("Run `%s` to push your changes and update the stack of PRs on GitHub", cfg.ColorCyan("gh stack submit")) } diff --git a/internal/modify/apply.go b/internal/modify/apply.go index b39ecd4..a95e292 100644 --- a/internal/modify/apply.go +++ b/internal/modify/apply.go @@ -137,6 +137,8 @@ func ApplyPlan( result := &modifyview.ApplyResult{Success: true} + // Track whether any action affects a branch with a PR. + affectsPRs := false // Collect original refs for rebase --onto, including trunk branchNames := make([]string, 0, len(s.Branches)+1) branchNames = append(branchNames, s.Trunk.Branch) @@ -207,6 +209,9 @@ func ApplyPlan( OldName: oldName, NewName: newName, }) + if n.Ref.PullRequest != nil { + affectsPRs = true + } cfg.Successf("Renamed %s → %s", oldName, newName) } } @@ -255,6 +260,15 @@ func ApplyPlan( baseBranch := s.ActiveBaseBranch(foldBranch) + // Check if fold source or target has a PR + if n.Ref.PullRequest != nil { + affectsPRs = true + } + targetIdx := s.IndexOf(targetBranch) + if targetIdx >= 0 && s.Branches[targetIdx].PullRequest != nil { + affectsPRs = true + } + if n.PendingAction.Type == modifyview.ActionFoldDown { // Fold-down: cherry-pick the folded branch's commits onto the target. commits, err := git.LogRange(baseBranch, foldBranch) @@ -301,6 +315,7 @@ func ApplyPlan( stateFile.RemainingBranches = remaining stateFile.OriginalBranch = currentBranch stateFile.OriginalRefs = originalParentTips + stateFile.AffectsPRs = affectsPRs if saveErr := SaveState(gitDir, stateFile); saveErr != nil { cfg.Warningf("failed to save conflict state: %v", saveErr) } @@ -351,6 +366,7 @@ func ApplyPlan( Branch: dropBranch, PRNumber: n.Ref.PullRequest.Number, }) + affectsPRs = true } s.Branches = append(s.Branches[:dropIdx], s.Branches[dropIdx+1:]...) @@ -466,6 +482,10 @@ func ApplyPlan( conflict.ConflictedFiles = files } + if b.PullRequest != nil { + affectsPRs = true + } + // Save conflict state so --continue can resume remaining := make([]string, 0) for j := i + 1; j < len(s.Branches); j++ { @@ -479,6 +499,7 @@ func ApplyPlan( stateFile.RemainingBranches = remaining stateFile.OriginalBranch = currentBranch stateFile.OriginalRefs = originalParentTips + stateFile.AffectsPRs = affectsPRs if saveErr := SaveState(gitDir, stateFile); saveErr != nil { cfg.Warningf("failed to save conflict state: %v", saveErr) } @@ -492,6 +513,9 @@ func ApplyPlan( } cfg.Successf("Rebased %s onto %s", b.Branch, newBase) + if b.PullRequest != nil { + affectsPRs = true + } result.MovedBranches++ } @@ -507,15 +531,14 @@ func ApplyPlan( // Update base SHAs updateBaseSHAs(s) - // Update state file phase - if s.ID != "" { + // Update state file phase — only require submit when PRs are affected + needsSubmit := s.ID != "" && affectsPRs + result.NeedsSubmit = needsSubmit + if needsSubmit { stateFile.Phase = PhasePendingSubmit if err := SaveState(gitDir, stateFile); err != nil { cfg.Warningf("failed to update modify state: %s", err) } - } else { - // No remote stack — clear the state file - ClearState(gitDir) } // Save stack metadata — this must succeed since git refs have been rewritten @@ -523,6 +546,11 @@ func ApplyPlan( return nil, nil, fmt.Errorf("saving stack metadata: %w", err) } + // Clear state after metadata save succeeds to preserve --abort recovery + if !needsSubmit { + ClearState(gitDir) + } + return result, nil, nil } @@ -686,6 +714,14 @@ func ContinueApply( return fmt.Errorf("stack at index %d not found (stack file may have changed)", state.StackIndex) } + // Carry forward whether any prior actions already affected PRs + affectsPRs := state.AffectsPRs + + // Check the conflict branch itself + if idx := s.IndexOf(state.ConflictBranch); idx >= 0 && s.Branches[idx].PullRequest != nil { + affectsPRs = true + } + // Finish the in-progress git operation (rebase or cherry-pick) if state.ConflictType == "cherry_pick" { if err := git.CherryPickContinue(); err != nil { @@ -763,6 +799,7 @@ func ContinueApply( } state.ConflictBranch = branchName state.RemainingBranches = remaining + state.AffectsPRs = affectsPRs _ = SaveState(gitDir, state) cfg.Warningf("Conflict rebasing %s", branchName) @@ -781,8 +818,10 @@ func ContinueApply( } cfg.Successf("Rebased %s onto %s", branchName, newBase) + if b.PullRequest != nil { + affectsPRs = true + } } - // All rebases done — check out the best branch if state.OriginalBranch != "" { targetBranch := resolveCheckoutBranch(state.OriginalBranch, state.Plan, state.Snapshot, s) @@ -796,8 +835,9 @@ func ContinueApply( // Update base SHAs updateBaseSHAs(s) - // Transition to pending_submit or clear - if s.ID != "" { + // Transition to pending_submit only when PRs are affected + needsSubmit := s.ID != "" && affectsPRs + if needsSubmit { state.Phase = PhasePendingSubmit state.ConflictBranch = "" state.RemainingBranches = nil @@ -805,8 +845,6 @@ func ContinueApply( if err := SaveState(gitDir, state); err != nil { cfg.Warningf("failed to update modify state: %s", err) } - } else { - ClearState(gitDir) } // Save stack metadata @@ -814,8 +852,13 @@ func ContinueApply( cfg.Warningf("failed to save stack: %v", err) } + // Clear state after metadata save succeeds to preserve --abort recovery + if !needsSubmit { + ClearState(gitDir) + } + cfg.Successf("Stack modified successfully") - if state.PriorRemoteStackID != "" { + if needsSubmit { cfg.Printf("") cfg.Printf("Run `%s` to push your changes and update the stack of PRs on GitHub", cfg.ColorCyan("gh stack submit")) diff --git a/internal/modify/apply_test.go b/internal/modify/apply_test.go index 084bc92..9921424 100644 --- a/internal/modify/apply_test.go +++ b/internal/modify/apply_test.go @@ -1262,8 +1262,8 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) { ID: "remote-stack-123", Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "A"}, - {Branch: "B"}, + {Branch: "A", PullRequest: &stack.PullRequestRef{Number: 1}}, + {Branch: "B", PullRequest: &stack.PullRequestRef{Number: 2}}, }, } @@ -1277,7 +1277,7 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) { } mock := newApplyMock(gitDir, branchSHAs) - mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil } + mock.IsAncestorFn = func(a, d string) (bool, error) { return false, nil } mock.MergeBaseFn = func(a, b string) (string, error) { if a == "main" && b == "A" { return branchSHAs["main"], nil @@ -1295,16 +1295,19 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) { defer cfg.Out.Close() defer cfg.Err.Close() + // Reverse nodes so position differs → triggers rebase of PR branches nodes := makeNodes(&sf.Stacks[0]) + nodes[0], nodes[1] = nodes[1], nodes[0] - _, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) + result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) require.NoError(t, err) - // Remote stack should transition to "pending_submit" + // Remote stack with PR branches affected should transition to "pending_submit" state, loadErr := LoadState(gitDir) require.NoError(t, loadErr) require.NotNil(t, state) assert.Equal(t, "pending_submit", state.Phase) + assert.True(t, result.NeedsSubmit, "NeedsSubmit should be true when PR branches are affected") } func TestApplyPlan_ClearsStateForLocalStack(t *testing.T) { @@ -1345,6 +1348,111 @@ func TestApplyPlan_ClearsStateForLocalStack(t *testing.T) { assert.False(t, StateExists(gitDir)) } +func TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches(t *testing.T) { + // Remote stack (has ID) but branches have no PRs — local-only modify + s := stack.Stack{ + ID: "remote-stack-456", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "A"}, + {Branch: "B"}, + }, + } + + gitDir := t.TempDir() + sf := writeTestStackFile(t, gitDir, s) + + branchSHAs := map[string]string{ + "main": "sha-main", + "A": "sha-A", + "B": "sha-B", + } + + mock := newApplyMock(gitDir, branchSHAs) + mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil } + mock.MergeBaseFn = func(a, b string) (string, error) { + if a == "main" && b == "A" { + return branchSHAs["main"], nil + } + if a == "A" && b == "B" { + return branchSHAs["A"], nil + } + return "merge-base", nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + defer cfg.Out.Close() + defer cfg.Err.Close() + + nodes := makeNodes(&sf.Stacks[0]) + + result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) + require.NoError(t, err) + + // Remote stack but no PR branches affected → state should be cleared + assert.False(t, StateExists(gitDir), "state file should be cleared when no PR branches are affected") + assert.False(t, result.NeedsSubmit, "NeedsSubmit should be false when no PR branches are affected") +} + +func TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected(t *testing.T) { + // Stack with one PR branch (A) and one local branch (B). + // Only rename the local branch B — PRs should not be affected. + s := stack.Stack{ + ID: "remote-stack-789", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "A", PullRequest: &stack.PullRequestRef{Number: 1}}, + {Branch: "B"}, + }, + } + + gitDir := t.TempDir() + sf := writeTestStackFile(t, gitDir, s) + + branchSHAs := map[string]string{ + "main": "sha-main", + "A": "sha-A", + "B": "sha-B", + } + + mock := newApplyMock(gitDir, branchSHAs) + mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil } + mock.MergeBaseFn = func(a, b string) (string, error) { + if a == "main" && b == "A" { + return branchSHAs["main"], nil + } + if a == "A" && b == "B" || a == "A" && b == "B-renamed" { + return branchSHAs["A"], nil + } + return "merge-base", nil + } + mock.RenameBranchFn = func(old, newName string) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + defer cfg.Out.Close() + defer cfg.Err.Close() + + nodes := makeNodes(&sf.Stacks[0]) + // Rename only the non-PR branch B + nodes[1].PendingAction = &modifyview.PendingAction{ + Type: modifyview.ActionRename, + NewName: "B-renamed", + } + + result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) + require.NoError(t, err) + + // Only non-PR branch was renamed — should clear state, not pending submit + assert.False(t, StateExists(gitDir), "state file should be cleared when only non-PR branches are renamed") + assert.False(t, result.NeedsSubmit, "NeedsSubmit should be false when only non-PR branches are affected") +} + // ─── resolveCheckoutBranch ────────────────────────────────────────────────── func TestResolveCheckoutBranch_StillInStack(t *testing.T) { diff --git a/internal/modify/state.go b/internal/modify/state.go index d4e243a..2660cba 100644 --- a/internal/modify/state.go +++ b/internal/modify/state.go @@ -39,6 +39,11 @@ type StateFile struct { // Cherry-pick conflict context — which fold was in progress FoldBranch string `json:"fold_branch,omitempty"` // branch being folded FoldTarget string `json:"fold_target,omitempty"` // branch receiving the cherry-pick + + // AffectsPRs records whether any action so far has affected a branch with + // a PR. Persisted across conflict boundaries so ContinueApply can combine + // it with checks on remaining branches. + AffectsPRs bool `json:"affects_prs,omitempty"` } // Snapshot captures the pre-modify state for unwind/recovery. diff --git a/internal/tui/modifyview/types.go b/internal/tui/modifyview/types.go index e3f995d..4598c6b 100644 --- a/internal/tui/modifyview/types.go +++ b/internal/tui/modifyview/types.go @@ -47,6 +47,7 @@ type ApplyResult struct { DroppedPRs []DroppedPR RenamedBranches []RenamedBranch MovedBranches int + NeedsSubmit bool // true when the modify affected branches with PRs Message string }