diff --git a/cmd/submit.go b/cmd/submit.go index f310c00..66c720a 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -230,10 +230,27 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { drafts = collected } - // Push each branch and create/update its PR in stack order (bottom to top). - // Sequential pushing ensures each branch's base is up-to-date on the - // remote before the next branch is pushed, preventing race conditions. - cfg.Printf("Pushing to %s...", remote) + // Push branches, then create/update each PR in stack order (bottom to top). + // + // Normally all active branches go up in a single atomic push. GitHub then + // receives every rewritten ref in one push event, which lets it synchronize + // each PR once (for its head ref) rather than once for its head and again + // for its base, and the remote never holds a partially-updated stack. + // + // When a modify is pending, the stack's shape may have changed (branches + // reordered, dropped, or folded), so PR bases on GitHub can be stale. In + // that case push branches one at a time and fix each PR's base before + // pushing the next, so no PR is compared against an outdated base. + sequentialPush := pendingModify + if sequentialPush { + cfg.Printf("Pushing to %s...", remote) + } else { + cfg.Printf("Pushing %d %s to %s...", len(activeBranches), plural(len(activeBranches), "branch", "branches"), remote) + if err := git.Push(remote, activeBranches, true, true); err != nil { + cfg.Errorf("failed to push: %s", err) + return ErrSilent + } + } allPRsSubmitted := len(queued) == 0 submittedPRs := make(map[string]int, len(activeBranches)) for i, b := range s.Branches { @@ -241,10 +258,11 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { continue } - // Push this branch - if err := git.Push(remote, []string{b.Branch}, true, false); err != nil { - cfg.Errorf("failed to push %s: %s", b.Branch, err) - return ErrSilent + if sequentialPush { + if err := git.Push(remote, []string{b.Branch}, true, false); err != nil { + cfg.Errorf("failed to push %s: %s", b.Branch, err) + return ErrSilent + } } // Find or create PR, and fix base if needed diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 4918d59..de4dbf0 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -151,11 +151,12 @@ func TestSubmit_CreatesPRsAndStack(t *testing.T) { assert.NoError(t, err) - // Branches should be pushed (sequentially, one per branch) - require.Len(t, pushCalls, 2) + // All branches are pushed together in a single atomic, forced push + require.Len(t, pushCalls, 1) assert.Equal(t, "origin", pushCalls[0].remote) - assert.Equal(t, []string{"b1"}, pushCalls[0].branches) - assert.Equal(t, []string{"b2"}, pushCalls[1].branches) + assert.Equal(t, []string{"b1", "b2"}, pushCalls[0].branches) + assert.True(t, pushCalls[0].force) + assert.True(t, pushCalls[0].atomic) // PRs should be created assert.Equal(t, []string{"b1", "b2"}, createdPRs) @@ -346,6 +347,52 @@ func TestSubmit_PushFailure(t *testing.T) { assert.Contains(t, output, "failed to push") } +// TestSubmit_AtomicPushFailureCreatesNoPRs verifies that, without a pending +// modify, a rejected push aborts before any PR is created or updated. +func TestSubmit_AtomicPushFailureCreatesNoPRs(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var pushCalls []pushCall + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return fmt.Errorf("stale info for b2") + } + + restore := git.SetOps(mock) + defer restore() + + creations := 0 + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + CreatePRFn: func(_, _, _, _ string, _ bool) (*github.PullRequest, error) { + creations++ + return &github.PullRequest{Number: 1}, nil + }, + } + err := runSubmit(cfg, &submitOptions{auto: true}) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, string(errOut), "failed to push") + require.Len(t, pushCalls, 1) + assert.Equal(t, []string{"b1", "b2", "b3"}, pushCalls[0].branches) + assert.True(t, pushCalls[0].atomic) + assert.Zero(t, creations, "no PR should be created after a failed push") +} + func TestSubmit_SkipsMergedBranches(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, @@ -489,9 +536,8 @@ func TestSubmit_ForksWhenRemoteStackFullyMerged(t *testing.T) { require.NoError(t, err) // Only the new branches are pushed; merged ones are left behind. - require.Len(t, pushCalls, 2) - assert.Equal(t, []string{"b3"}, pushCalls[0].branches) - assert.Equal(t, []string{"b4"}, pushCalls[1].branches) + require.Len(t, pushCalls, 1) + assert.Equal(t, []string{"b3", "b4"}, pushCalls[0].branches) // Fork messaging. assert.Contains(t, output, "Every PR in this stack has already been merged") @@ -2059,7 +2105,7 @@ func TestSubmit_PRFailureWithoutPendingModifyRemainsBestEffort(t *testing.T) { require.NoError(t, runSubmit(cfg, &submitOptions{auto: true})) _, output := commandOutput(t, cfg, outR, errR) - assert.Equal(t, 2, pushes) + assert.Equal(t, 1, pushes) assert.Equal(t, 2, creations) assert.Contains(t, output, "failed to create PR for b2") assert.NoFileExists(t, modify.StatePath(dir)) @@ -2563,7 +2609,7 @@ func TestSubmit_FetchesBeforePush(t *testing.T) { assert.NoError(t, err) assert.Equal(t, []string{"b1", "b2"}, fetchedBranches, "should fetch active branches") // fetch must come before all pushes - require.True(t, len(callOrder) >= 3, "expected at least 3 calls (fetch + 2 pushes)") + require.Equal(t, []string{"fetch", "push"}, callOrder, "expected fetch followed by a single push") assert.Equal(t, "fetch", callOrder[0], "fetch must happen before any push") } diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index e96b313..72c99ed 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -341,7 +341,7 @@ gh stack submit [flags] | `--open` | Create new PRs as ready for review instead of drafts, and mark existing PRs as ready for review | | `--remote ` | Remote to push to (defaults to auto-detected remote) | -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. +Creates a Stacked PR for every branch in the stack, pushing branches to the remote. All active branches are pushed together in a single atomic push (with per-branch `--force-with-lease` checks), so either every branch is updated or none are. When finishing a pending `gh stack modify`, branches are instead pushed one at a time so each PR's base can be corrected before the next branch is pushed. 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. 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.