fix(remotehelper): skip receive-pack POST when send-pack emits no request · Entire
fix(remotehelper): skip receive-pack POST when send-pack emits no request
f93cfcc→main · pjbgf · 19h ago · 3 files · +247 added/-67 removed
On git push --dry-run (and batches where every update is rejected client-side), stateless-rpc send-pack guards the entire request emission with !args->dry_run / cmds_sent: its stdout is just the outer flush followed directly by helper-status lines — no request body and no trailing flush. handlePush unconditionally POSTed the empty body, which the server deliberately rejects with HTTP 400, and then tried to drain a trailing flush that doesn't exist.
Mirror remote-curl.c:rpc_service, which breaks without calling post_rpc when the first packet is a flush: when the wrapped request is empty, skip the POST and the trailing-flush drain and relay the helper-status lines directly.
The two existing send-pack shell stubs emitted an empty request plus a trailing flush — a wire shape real send-pack never produces; they now emit a wrapped one-command request. New invariant tests cover the dry-run and all-rejected shapes via stubs, plus one driving handlePush against the real git binary to pin the wire-shape assumption.
Fixes ENT-1199.
Assisted-by: Claude Opus 4.8 noreply@anthropic.com Signed-off-by: Paulo Gomes paulo@entire.io
Changes
3
internal/remotehelper/githelper
Mconsts_test.go +1
Minvariants_test.go +195/-36
Mpush.go +51/-31
5 unmodified lines
6
7
8
9
10
5 unmodified lines
testRefMain = "refs/heads/main"
testRefFeatureBranch = "refs/heads/feature-branch"
testHeadSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
goosWindows = "windows"
)
Minternal/remotehelper/githelper/consts_test.go +1
20 unmodified lines
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
151 unmodified lines
216
217
218
189
190
191
192
193
194
195
196
197
198
199
200
219
220
221
222
223
224
225
72 unmodified lines
298
299
300
279
280
301
302
303
304
305
283
306
307
308
309
310
311
312
313
314
290
291
292
293
294
315
316
317
318
319
320
321
298
322
323
324
325
2 unmodified lines
328
329
330
307
308
309
310
311
312
313
314
315
316
317
331
332
333
334
25 unmodified lines
360
361
362
349
363
364
365
366
367
368
369
356
357
370
371
372
373
359
374
375
376
377
378
379
29 unmodified lines
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
20 unmodified lines
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"testing"
// wrappedSendPackRequest returns the stateless-rpc outer framing
// git send-pack --stateless-rpc emits for a minimal one-command
// receive-pack request: one outer packet wrapping the inner pkt-line
// stream, then the outer flush. The command line omits the
// NUL-separated capability list a real send-pack appends so the
// result stays shell-safe for printf stubs; handlePush only relays
// the body, and AppendAgentToReceivePackRequest leaves lines without
// a NUL unchanged.
func wrappedSendPackRequest(oldSHA, newSHA, ref string) string {
inner := pktLine(oldSHA+" "+newSHA+" "+ref+" report-status\n") + "0000"
return fmt.Sprintf("%04x%s0000", len(inner)+4, inner)
}
// pushFakeTransport returns a fakeTransport advertising oldSHA at // testRefMain for receive-pack and answering any service RPC with an // empty body. func pushFakeTransport(oldSHA string) *fakeTransport { return &fakeTransport{ infoRefsResp: func() (io.ReadCloser, error) { return stringRC(serviceAnnouncement(serviceReceivePack, oldSHA+" "+testRefMain+"\x00 report-status\n")), nil }, serviceRPCResp: func(string, []byte) (io.ReadCloser, error) { return stringRC(""), nil }, } }
// TestInvariant_NoSpuriousAckOnTransportError: the server's POST // fails before any response body is written. Helper output MUST // contain no "ok " lines and the error MUST propagate. 151 unmodified lines
newSHA := strings.Repeat("b", 40) ref := testRefMain
ft := &fakeTransport{ infoRefsResp: func() (io.ReadCloser, error) { return stringRC(serviceAnnouncement(serviceReceivePack, oldSHA+" "+ref+"\x00 report-status\n")), nil }, serviceRPCResp: func(string, []byte) (io.ReadCloser, error) { // Empty response body — server processed the push but // returned no report-status. Helper must surface this // as "no acknowledgement" rather than synthesise one. return stringRC(""), nil }, }
// The empty ServiceRPC response body means the server processed the // push but returned no report-status. Helper must surface this as // "no acknowledgement" rather than synthesise one. ft := pushFakeTransport(oldSHA)
cmd := oldSHA + " " + newSHA + " " + ref + "\x00 report-status\n" var stdin bytes.Buffer
72 unmodified lines
// reason and leaves the user with bare "send-pack exited with error".
//
// A shell stub on PATH plays the role of git send-pack: it emits the
// minimal wire shape handlePush expects (empty wrapped request, then
// trailing flush + helper-status), and exits 1.
// wire shape of a server-side rejection (a request WAS sent, so the
// wrapped request and the cmds_sent-guarded trailing flush are both
// present before the helper-status), and exits 1.
func TestInvariant_HelperStatusSurfacedOnSendPackError(t *testing.T) {
// No t.Parallel(): t.Setenv("PATH", ...) mutates process-global state.
if runtime.GOOS == "windows" {
if runtime.GOOS == goosWindows {
t.Skip("shell-script PATH stub is POSIX-only")
}
ref := testRefMain oldSHA := strings.Repeat("a", 40) newSHA := strings.Repeat("b", 40) helperStatusLine := "error " + ref + " remote rejected: branch protection violation\n"
// Stub git: emits the outer flush that terminates an empty // send-pack request, drains stdin (refspecs + advertisement + // receive-pack response), then emits the trailing flush + // helper-status line, then exits 1. The exit code mirrors // send-pack's behaviour on per-ref rejection. // Stub git: emits a wrapped one-command request + outer flush, // drains stdin (refspecs + advertisement + receive-pack response), // then emits the trailing flush + helper-status line, then exits 1. stubDir := t.TempDir() stubPath := filepath.Join(stubDir, "git") stub := "#!/bin/sh\n" + "printf '0000'\n" + "printf '%s' '" + wrappedSendPackRequest(oldSHA, newSHA, ref) + "'\n" + "cat > /dev/null\n" + "printf '0000" + helperStatusLine + "'\n" + "exit 1\n" 2 unmodified lines
} t.Setenv("PATH", stubDir+string(os.PathListSeparator)+os.Getenv("PATH"))
oldSHA := strings.Repeat("a", 40)
ft := &fakeTransport{ infoRefsResp: func() (io.ReadCloser, error) { return stringRC(serviceAnnouncement(serviceReceivePack, oldSHA+" "+ref+"\x00 report-status\n")), nil }, serviceRPCResp: func(string, []byte) (io.ReadCloser, error) { return stringRC(""), nil }, } ft := pushFakeTransport(oldSHA)
// handlePush takes the first "push" line plus a bufio.Reader for // the rest of the batch; readPushBatch terminates on the blank 25 unmodified lines
// that phantom ref out of send-pack's view. func TestInvariant_PushReusesListForPushAdvertisement(t *testing.T) { // No t.Parallel(): t.Setenv("PATH", ...) mutates process-global state. if runtime.GOOS == "windows" { if runtime.GOOS == goosWindows { t.Skip("shell-script PATH stub is POSIX-only") }
ref := testRefMain oldSHA := strings.Repeat("a", 40)
// Stub git send-pack: emit the empty-request terminator, drain stdin, // then the trailing flush + a plain "ok" helper-status, exit 0. // Stub git send-pack: emit a wrapped one-command request + outer // flush, drain stdin, then the trailing flush + a plain "ok" // helper-status, exit 0. stubDir := t.TempDir() stub := "#!/bin/sh\nprintf '0000'\ncat > /dev/null\nprintf '0000ok " + ref + "\n'\nexit 0\n" stub := "#!/bin/sh\n" + "printf '%s' '" + wrappedSendPackRequest(oldSHA, strings.Repeat("b", 40), ref) + "'\n" + "cat > /dev/null\nprintf '0000ok " + ref + "\n'\nexit 0\n" if err := os.WriteFile(filepath.Join(stubDir, "git"), []byte(stub), 0o755); err != nil { t.Fatalf("writing stub git: %v", err) } 29 unmodified lines
t.Fatalf("receive-pack info/refs fetched %d times; want 1 (push must reuse the list-for-push advertisement)", receivePackCalls) } }
// TestInvariant_DryRunPushSkipsReceivePackPOST pins the fix for ENT-1199.
// On git push --dry-run, stateless-rpc send-pack emits no receive-pack
// request at all — the whole request emission is guarded by !args->dry_run,
// so its stdout is just the outer flush followed directly by helper-status
// lines (the trailing flush at send-pack.c:759 is guarded by cmds_sent).
// The helper must mirror remote-curl.c:rpc_service, which breaks without
// calling post_rpc when the first packet is a flush: no POST (the server
// deliberately 400s zero-byte receive-pack bodies), no trailing-flush
// drain (reading 4 more bytes would eat "ok r" from the status line).
func TestInvariant_DryRunPushSkipsReceivePackPOST(t *testing.T) {
// No t.Parallel(): t.Setenv("PATH", ...) mutates process-global state.
if runtime.GOOS == "windows" {
t.Skip("shell-script PATH stub is POSIX-only")
}
ref := testRefMain oldSHA := strings.Repeat("a", 40)
// Stub git send-pack in dry-run shape: outer flush terminating the // (empty) request, drain stdin, then helper-status with NO trailing // flush, exit 0. stubDir := t.TempDir() stub := "#!/bin/sh\n" + "printf '0000'\n" + "cat > /dev/null\n" + "printf 'ok " + ref + "\n'\n" + "exit 0\n" if err := os.WriteFile(filepath.Join(stubDir, "git"), []byte(stub), 0o755); err != nil { t.Fatalf("writing stub git: %v", err) } t.Setenv("PATH", stubDir+string(os.PathListSeparator)+os.Getenv("PATH"))
ft := pushFakeTransport(oldSHA)
firstLine := "push " + oldSHA + ":" + ref stdin := bufio.NewReader(strings.NewReader("\n"))
var stdout bytes.Buffer err := handlePush(context.Background(), ft, &refAdvCache{}, firstLine, &Options{dryRun: true}, stdin, &stdout) if err != nil { t.Fatalf("handlePush: %v", err) } if len(ft.rpcCalls) != 0 { t.Errorf("expected no receive-pack POST on dry run, got %d", len(ft.rpcCalls)) } if want := "ok " + ref + "\n\n"; stdout.String() != want { t.Errorf("stdout = %q, want %q", stdout.String(), want) } }
// TestInvariant_DryRunPushRealSendPack drives handlePush with the REAL
// git binary — no shell stub — so the wire-shape assumption behind the
// dry-run skip (send-pack emits no request and no trailing flush when
// cmds_sent is 0) is pinned against the installed git, not our model
// of it. A fast-forward update refs/heads/
ft := pushFakeTransport(oldSHA)
firstLine := "push " + testRefMain + ":" + testRefMain stdin := bufio.NewReader(strings.NewReader("\n"))
var stdout bytes.Buffer err := handlePush(context.Background(), ft, &refAdvCache{}, firstLine, &Options{dryRun: true}, stdin, &stdout) if err != nil { t.Fatalf("handlePush: %v", err) } if len(ft.rpcCalls) != 0 { t.Errorf("expected no receive-pack POST on dry run, got %d", len(ft.rpcCalls)) } if want := "ok " + testRefMain + "\n\n"; stdout.String() != want { t.Errorf("stdout = %q, want %q", stdout.String(), want) } }
// TestInvariant_AllRejectedPushSkipsReceivePackPOST: same empty-request
// wire shape as a dry run, but produced by a batch where send-pack
// rejected every update client-side (e.g. non-fast-forward without
// force) — cmds_sent stays 0, nothing is emitted, and the helper-status
// error lines follow the outer flush directly. The helper must skip
// the POST, relay the error lines, and surface send-pack's exit error.
func TestInvariant_AllRejectedPushSkipsReceivePackPOST(t *testing.T) {
// No t.Parallel(): t.Setenv("PATH", ...) mutates process-global state.
if runtime.GOOS == "windows" {
t.Skip("shell-script PATH stub is POSIX-only")
}
ref := testRefMain oldSHA := strings.Repeat("a", 40) helperStatusLine := "error " + ref + " non-fast-forward\n"
stubDir := t.TempDir() stub := "#!/bin/sh\n" + "printf '0000'\n" + "cat > /dev/null\n" + "printf '" + helperStatusLine + "'\n" + "exit 1\n" if err := os.WriteFile(filepath.Join(stubDir, "git"), []byte(stub), 0o755); err != nil { t.Fatalf("writing stub git: %v", err) } t.Setenv("PATH", stubDir+string(os.PathListSeparator)+os.Getenv("PATH"))
ft := pushFakeTransport(oldSHA)
firstLine := "push " + oldSHA + ":" + ref stdin := bufio.NewReader(strings.NewReader("\n"))
var stdout bytes.Buffer err := handlePush(context.Background(), ft, &refAdvCache{}, firstLine, &Options{}, stdin, &stdout) if err == nil { t.Fatal("expected error from send-pack exit 1") } if len(ft.rpcCalls) != 0 { t.Errorf("expected no receive-pack POST for all-rejected batch, got %d", len(ft.rpcCalls)) } if !strings.Contains(stdout.String(), helperStatusLine) { t.Errorf("stdout missing helper-status line; got %q, want substring %q", stdout.String(), helperStatusLine) } }