Skip to content

Commit d1e9d14

Browse files
authored
run fetch before push operations (#75)
* run fetch before push operations * ignore if ref doesn't exist on remote * rm fetch from link * more durable fetch by trying all first and falling back to individual fetches * run fetch before sync
1 parent 03fe8ea commit d1e9d14

8 files changed

Lines changed: 209 additions & 8 deletions

File tree

cmd/push.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,12 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
9393
cfg.Printf("No active branches to push (all merged or queued)")
9494
return nil
9595
}
96+
// Best-effort fetch to update tracking refs (helps --force-with-lease
97+
// in shallow clones). Silently ignored if branches don't exist on the
98+
// remote yet.
99+
_ = git.FetchBranches(remote, activeBranches)
96100
cfg.Printf("Pushing %d %s to %s...", len(activeBranches), plural(len(activeBranches), "branch", "branches"), remote)
97-
if err := git.Push(remote, activeBranches, true, true); err != nil {
101+
if err := git.Push(remote, activeBranches, true, false); err != nil {
98102
cfg.Errorf("failed to push: %s", err)
99103
return ErrSilent
100104
}

cmd/push_test.go

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func TestPush_PushesAllBranches(t *testing.T) {
6262
assert.Equal(t, "origin", pushCalls[0].remote)
6363
assert.Equal(t, []string{"b1", "b2"}, pushCalls[0].branches)
6464
assert.True(t, pushCalls[0].force)
65-
assert.True(t, pushCalls[0].atomic)
65+
assert.False(t, pushCalls[0].atomic)
6666
assert.Contains(t, output, "Pushed 2 branches")
6767
assert.Contains(t, output, "gh stack submit", "should hint about submit when branches have no PRs")
6868
}
@@ -182,6 +182,89 @@ func TestPush_PushFailure(t *testing.T) {
182182
assert.Contains(t, output, "failed to push")
183183
}
184184

185+
func TestPush_FetchesBeforePush(t *testing.T) {
186+
s := stack.Stack{
187+
Trunk: stack.BranchRef{Branch: "main"},
188+
Branches: []stack.BranchRef{
189+
{Branch: "b1"},
190+
{Branch: "b2"},
191+
},
192+
}
193+
194+
tmpDir := t.TempDir()
195+
writeStackFile(t, tmpDir, s)
196+
197+
var callOrder []string
198+
199+
mock := newPushMock(tmpDir, "b1")
200+
mock.FetchBranchesFn = func(remote string, branches []string) error {
201+
callOrder = append(callOrder, "fetch")
202+
assert.Equal(t, "origin", remote)
203+
assert.Equal(t, []string{"b1", "b2"}, branches)
204+
return nil
205+
}
206+
mock.PushFn = func(remote string, branches []string, force, atomic bool) error {
207+
callOrder = append(callOrder, "push")
208+
return nil
209+
}
210+
211+
restore := git.SetOps(mock)
212+
defer restore()
213+
214+
cfg, _, errR := config.NewTestConfig()
215+
cfg.GitHubClientOverride = &github.MockClient{}
216+
cmd := PushCmd(cfg)
217+
cmd.SetOut(io.Discard)
218+
cmd.SetErr(io.Discard)
219+
err := cmd.Execute()
220+
221+
cfg.Err.Close()
222+
_, _ = io.ReadAll(errR)
223+
224+
assert.NoError(t, err)
225+
assert.Equal(t, []string{"fetch", "push"}, callOrder, "fetch must happen before push")
226+
}
227+
228+
func TestPush_FetchFailureIsNonFatal(t *testing.T) {
229+
s := stack.Stack{
230+
Trunk: stack.BranchRef{Branch: "main"},
231+
Branches: []stack.BranchRef{
232+
{Branch: "b1"},
233+
},
234+
}
235+
236+
tmpDir := t.TempDir()
237+
writeStackFile(t, tmpDir, s)
238+
239+
pushCalled := false
240+
241+
mock := newPushMock(tmpDir, "b1")
242+
mock.FetchBranchesFn = func(string, []string) error {
243+
return fmt.Errorf("network error")
244+
}
245+
mock.PushFn = func(string, []string, bool, bool) error {
246+
pushCalled = true
247+
return nil
248+
}
249+
250+
restore := git.SetOps(mock)
251+
defer restore()
252+
253+
cfg, _, errR := config.NewTestConfig()
254+
cfg.GitHubClientOverride = &github.MockClient{}
255+
cmd := PushCmd(cfg)
256+
cmd.SetOut(io.Discard)
257+
cmd.SetErr(io.Discard)
258+
err := cmd.Execute()
259+
260+
cfg.Err.Close()
261+
errOut, _ := io.ReadAll(errR)
262+
263+
assert.NoError(t, err, "fetch failure should not abort push")
264+
assert.True(t, pushCalled, "push should proceed after fetch failure")
265+
assert.NotContains(t, string(errOut), "Failed to fetch", "fetch failure should be silent")
266+
}
267+
185268
func TestPush_DoesNotCreatePRs(t *testing.T) {
186269
s := stack.Stack{
187270
Trunk: stack.BranchRef{Branch: "main"},

cmd/submit.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
5959
return ErrNotInStack
6060
}
6161

62+
cfg.Printf("Checking stack state...")
63+
6264
// Find the stack for the current branch without switching branches.
6365
// Submit should never change the user's checked-out branch.
6466
stacks := sf.FindAllStacksForBranch(currentBranch)
@@ -129,7 +131,7 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
129131
return nil
130132
}
131133

132-
// If a modification is pending, delete the old remote stack first so that
134+
// If a modification is pending, delete the old remote stack first so that
133135
// PR base updates are allowed and force-pushes don't trigger auto-merges.
134136
if stacksAvailable {
135137
if err := handlePendingModify(cfg, client, s, gitDir); err != nil {
@@ -141,6 +143,11 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
141143
}
142144
}
143145

146+
// Best-effort fetch to update tracking refs (helps --force-with-lease
147+
// in shallow clones). Silently ignored if branches don't exist on the
148+
// remote yet.
149+
_ = git.FetchBranches(remote, activeBranches)
150+
144151
// Push each branch and create/update its PR in stack order (bottom to top).
145152
// Sequential pushing ensures each branch's base is up-to-date on the
146153
// remote before the next branch is pushed, preventing race conditions.

cmd/submit_test.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1370,3 +1370,68 @@ func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) {
13701370
// State file should be cleared
13711371
assert.False(t, modify.StateExists(tmpDir), "modify state file should be cleared after success")
13721372
}
1373+
1374+
func TestSubmit_FetchesBeforePush(t *testing.T) {
1375+
s := stack.Stack{
1376+
Trunk: stack.BranchRef{Branch: "main"},
1377+
Branches: []stack.BranchRef{
1378+
{Branch: "b1"},
1379+
{Branch: "b2"},
1380+
},
1381+
}
1382+
1383+
tmpDir := t.TempDir()
1384+
writeStackFile(t, tmpDir, s)
1385+
1386+
var callOrder []string
1387+
var fetchedBranches []string
1388+
1389+
mock := newSubmitMock(tmpDir, "b1")
1390+
mock.FetchBranchesFn = func(remote string, branches []string) error {
1391+
callOrder = append(callOrder, "fetch")
1392+
fetchedBranches = branches
1393+
assert.Equal(t, "origin", remote)
1394+
return nil
1395+
}
1396+
mock.PushFn = func(remote string, branches []string, force, atomic bool) error {
1397+
callOrder = append(callOrder, "push")
1398+
return nil
1399+
}
1400+
1401+
restore := git.SetOps(mock)
1402+
defer restore()
1403+
1404+
cfg, _, errR := config.NewTestConfig()
1405+
cfg.GitHubClientOverride = &github.MockClient{
1406+
FindPRForBranchFn: func(branch string) (*github.PullRequest, error) {
1407+
return &github.PullRequest{
1408+
Number: 1,
1409+
URL: "https://github.com/o/r/pull/1",
1410+
BaseRefName: "main",
1411+
HeadRefName: branch,
1412+
State: "OPEN",
1413+
}, nil
1414+
},
1415+
ListStacksFn: func() ([]github.RemoteStack, error) {
1416+
return []github.RemoteStack{}, nil
1417+
},
1418+
CreateStackFn: func(prNumbers []int) (int, error) {
1419+
return 42, nil
1420+
},
1421+
}
1422+
1423+
cmd := SubmitCmd(cfg)
1424+
cmd.SetArgs([]string{"--auto"})
1425+
cmd.SetOut(io.Discard)
1426+
cmd.SetErr(io.Discard)
1427+
err := cmd.Execute()
1428+
1429+
cfg.Err.Close()
1430+
_, _ = io.ReadAll(errR)
1431+
1432+
assert.NoError(t, err)
1433+
assert.Equal(t, []string{"b1", "b2"}, fetchedBranches, "should fetch active branches")
1434+
// fetch must come before all pushes
1435+
require.True(t, len(callOrder) >= 3, "expected at least 3 calls (fetch + 2 pushes)")
1436+
assert.Equal(t, "fetch", callOrder[0], "fetch must happen before any push")
1437+
}

cmd/sync.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,11 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
7676
return ErrSilent
7777
}
7878

79-
if err := git.Fetch(remote); err != nil {
80-
cfg.Warningf("Failed to fetch %s: %v", remote, err)
81-
} else {
82-
cfg.Successf("Fetched latest changes from %s", remote)
83-
}
79+
// Fetch trunk + active branches so tracking refs are current for
80+
// fast-forward detection (Step 2) and --force-with-lease (Step 4).
81+
fetchTargets := append([]string{s.Trunk.Branch}, activeBranchNames(s)...)
82+
_ = git.FetchBranches(remote, fetchTargets)
83+
cfg.Successf("Fetched latest changes from %s", remote)
8484

8585
// --- Step 2: Fast-forward trunk ---
8686
trunk := s.Trunk.Branch

internal/git/git.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,12 @@ func Fetch(remote string) error {
124124
return ops.Fetch(remote)
125125
}
126126

127+
// FetchBranches fetches specific branches from a remote,
128+
// updating their tracking refs.
129+
func FetchBranches(remote string, branches []string) error {
130+
return ops.FetchBranches(remote, branches)
131+
}
132+
127133
// DefaultBranch returns the HEAD branch from origin.
128134
func DefaultBranch() (string, error) {
129135
return ops.DefaultBranch()

internal/git/gitops.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ type Ops interface {
2121
BranchExists(name string) bool
2222
CheckoutBranch(name string) error
2323
Fetch(remote string) error
24+
FetchBranches(remote string, branches []string) error
2425
DefaultBranch() (string, error)
2526
CreateBranch(name, base string) error
2627
Push(remote string, branches []string, force, atomic bool) error
@@ -106,6 +107,33 @@ func (d *defaultOps) Fetch(remote string) error {
106107
return client.Fetch(context.Background(), remote, "")
107108
}
108109

110+
func (d *defaultOps) FetchBranches(remote string, branches []string) error {
111+
// Only fetch branches that already have a remote tracking ref.
112+
var tracked []string
113+
for _, b := range branches {
114+
ref := fmt.Sprintf("refs/remotes/%s/%s", remote, b)
115+
if err := runSilent("rev-parse", "--verify", "--quiet", ref); err == nil {
116+
tracked = append(tracked, b)
117+
}
118+
}
119+
if len(tracked) == 0 {
120+
return nil
121+
}
122+
// Fast path: fetch all tracked branches in a single call.
123+
args := []string{"fetch", remote}
124+
args = append(args, tracked...)
125+
if err := runSilent(args...); err == nil {
126+
return nil
127+
}
128+
// Fallback: a ref may have been deleted on the remote while the
129+
// local tracking ref still exists. Fetch branches individually so
130+
// one missing ref doesn't block the others.
131+
for _, b := range tracked {
132+
_ = runSilent("fetch", remote, b)
133+
}
134+
return nil
135+
}
136+
109137
func (d *defaultOps) DefaultBranch() (string, error) {
110138
ref, err := run("symbolic-ref", "refs/remotes/origin/HEAD")
111139
if err != nil {

internal/git/mock_ops.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ type MockOps struct {
99
BranchExistsFn func(string) bool
1010
CheckoutBranchFn func(string) error
1111
FetchFn func(string) error
12+
FetchBranchesFn func(string, []string) error
1213
DefaultBranchFn func() (string, error)
1314
CreateBranchFn func(string, string) error
1415
PushFn func(string, []string, bool, bool) error
@@ -87,6 +88,13 @@ func (m *MockOps) Fetch(remote string) error {
8788
return nil
8889
}
8990

91+
func (m *MockOps) FetchBranches(remote string, branches []string) error {
92+
if m.FetchBranchesFn != nil {
93+
return m.FetchBranchesFn(remote, branches)
94+
}
95+
return nil
96+
}
97+
9098
func (m *MockOps) DefaultBranch() (string, error) {
9199
if m.DefaultBranchFn != nil {
92100
return m.DefaultBranchFn()

0 commit comments

Comments
 (0)