From 752e16ced9ed772a637d75bcc2b3d00eb02e7f58 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 18 Jun 2026 17:00:47 +0200 Subject: [PATCH 1/4] Batch ref-update commands to stay under receive-pack cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sync of a repo with many refs sent every ref-update command in a single receive-pack request, which entire-server rejects past 25,000 commands ("too many ref-update commands: 55006 (limit 25000)"). The relay/materialize strategies (replicate, incremental, materialized) had no command-count batching. Batch inside the gitproto push primitives so every strategy benefits: - PushPack / PushObjects send the pack with the first batch and the remaining refs as ref-only follow-ups. The pack carries every object for the whole push, and receive-pack commits the entire received pack (entire-server via CommitQuarantinedFanout, canonical git via tmp_objdir_migrate — neither prunes objects unreachable from the pushed tips), so later batches only move ref pointers. - PushCommands chunks all commands under maxRefUpdatesPerPush (20_000, with headroom under the server's 25_000 cap). Works against both entire-server and canonical git/GitHub (GitHub's per-push branch/tag limit is opt-in, default unlimited). Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: 5d4b8f06380e --- internal/gitproto/push.go | 119 ++++++++++++++++++++++++++++++++- internal/gitproto/push_test.go | 117 ++++++++++++++++++++++++++++++++ 2 files changed, 234 insertions(+), 2 deletions(-) diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index bc389cf9..072765e8 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -50,6 +50,44 @@ func NewPusher(conn Conn, adv *packp.AdvRefs, verbose bool) *Pusher { return &Pusher{Conn: conn, Adv: adv, Verbose: verbose} } +// maxRefUpdatesPerPush bounds how many ref-update commands ride in a single +// receive-pack request. entire-server rejects a push carrying more than 25_000 +// commands (server/githttp.maxRefUpdateCommands), and other servers may impose +// their own caps; staying well under that lets a sync of a many-ref repo split +// across several pushes instead of failing outright. +// +// Splitting is safe because the pack accompanying the first batch carries every +// object for the whole push: receive-pack commits the entire received pack into +// the object store (entire-server via CommitQuarantinedFanout, canonical git via +// tmp_objdir_migrate — neither prunes objects unreachable from the pushed tips), +// so the remaining batches only need to move ref pointers and carry no pack. +const maxRefUpdatesPerPush = 20_000 + +// chunkRefUpdates splits commands into batches no larger than +// maxRefUpdatesPerPush. Input that already fits is returned as a single batch +// (including the empty slice, so callers preserve their one-request behavior). +func chunkRefUpdates(commands []PushCommand) [][]PushCommand { + if len(commands) <= maxRefUpdatesPerPush { + return [][]PushCommand{commands} + } + batches := make([][]PushCommand, 0, (len(commands)+maxRefUpdatesPerPush-1)/maxRefUpdatesPerPush) + for start := 0; start < len(commands); start += maxRefUpdatesPerPush { + end := min(start+maxRefUpdatesPerPush, len(commands)) + batches = append(batches, commands[start:end]) + } + return batches +} + +// splitFirstBatch peels off the first batch (up to maxRefUpdatesPerPush) so a +// push can carry the pack with that batch and send the remainder as ref-only +// follow-ups. rest is nil when commands already fit in a single request. +func splitFirstBatch(commands []PushCommand) (first, rest []PushCommand) { + if len(commands) <= maxRefUpdatesPerPush { + return commands, nil + } + return commands[:maxRefUpdatesPerPush], commands[maxRefUpdatesPerPush:] +} + // PushPack streams a pack to the target. func (p *Pusher) PushPack(ctx context.Context, commands []PushCommand, pack io.ReadCloser) error { return PushPack(ctx, p.Conn, p.Adv, commands, pack, p.Verbose, p.OnRejection) @@ -358,6 +396,55 @@ func sendReceivePack( // PushObjects pushes locally-materialized objects to the target. // +// A push within the per-request ref-update cap (maxRefUpdatesPerPush) is a +// single atomic receive-pack request. A larger push is split: the materialized +// pack — which carries every object for the whole push — rides with the first +// batch of object-bearing commands, then the remaining refs (and any deletes) +// move as ref-only updates because the objects are already committed. +func PushObjects( + ctx context.Context, + conn Conn, + adv *packp.AdvRefs, + commands []PushCommand, + store storer.Storer, + hashes []plumbing.Hash, + verbose bool, + onRejection func(plumbing.ReferenceName, string), +) error { + if len(commands) <= maxRefUpdatesPerPush { + return pushObjectsBatch(ctx, conn, adv, commands, store, hashes, verbose, onRejection) + } + + updates := make([]PushCommand, 0, len(commands)) + var deletes []PushCommand + for _, c := range commands { + if c.Delete { + deletes = append(deletes, c) + } else { + updates = append(updates, c) + } + } + + if len(updates) > 0 { + first, rest := splitFirstBatch(updates) + if err := pushObjectsBatch(ctx, conn, adv, first, store, hashes, verbose, onRejection); err != nil { + return err + } + if len(rest) > 0 { + if err := PushCommands(ctx, conn, adv, rest, verbose, onRejection); err != nil { + return err + } + } + } + if len(deletes) > 0 { + return PushCommands(ctx, conn, adv, deletes, verbose, onRejection) + } + return nil +} + +// pushObjectsBatch encodes the selected objects into a pack and sends one +// receive-pack request for commands. +// // Delta selection runs synchronously up front via // packfile.DeltaSelector. The selected objects are then handed back to // a packfile.Encoder behind a passthrough ObjectSelector, so the @@ -366,7 +453,7 @@ func sendReceivePack( // the mid-stream stall that occurs when Encode runs selection itself — // CDN edges treat the resulting idle gap as a stalled upload and close // the connection. See go-git PR #2142 for the API hook. -func PushObjects( +func pushObjectsBatch( ctx context.Context, conn Conn, adv *packp.AdvRefs, @@ -570,7 +657,13 @@ func PushPack( } } - req, _, _, err := buildUpdateRequest(adv, commands, verbose) + // The pack carries every object for all commands, so it rides with the + // first batch; once committed the remaining refs update without re-sending + // objects. This keeps each request under the server's per-push ref-update + // cap (see maxRefUpdatesPerPush). + first, rest := splitFirstBatch(commands) + + req, _, _, err := buildUpdateRequest(adv, first, verbose) if err != nil { _ = pack.Close() return err @@ -584,6 +677,10 @@ func PushPack( if closeErr != nil { return fmt.Errorf("close pack: %w", closeErr) } + + if len(rest) > 0 { + return PushCommands(ctx, conn, adv, rest, verbose, onRejection) + } return nil } @@ -604,6 +701,24 @@ func PushCommands( commands []PushCommand, verbose bool, onRejection func(plumbing.ReferenceName, string), +) error { + for _, batch := range chunkRefUpdates(commands) { + if err := pushCommandsBatch(ctx, conn, adv, batch, verbose, onRejection); err != nil { + return err + } + } + return nil +} + +// pushCommandsBatch sends one receive-pack request for a single batch of +// ref-only commands; the referenced objects must already exist on the target. +func pushCommandsBatch( + ctx context.Context, + conn Conn, + adv *packp.AdvRefs, + commands []PushCommand, + verbose bool, + onRejection func(plumbing.ReferenceName, string), ) error { req, _, hasUpdates, err := buildUpdateRequest(adv, commands, verbose) if err != nil { diff --git a/internal/gitproto/push_test.go b/internal/gitproto/push_test.go index c5921ae2..8513b6a9 100644 --- a/internal/gitproto/push_test.go +++ b/internal/gitproto/push_test.go @@ -12,6 +12,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "testing" "time" @@ -735,3 +736,119 @@ func TestAsRefRejectedError_ToleratesPointerCommandStatusErr(t *testing.T) { t.Fatalf("must classify the pointer form as *RefRejectedError; got %#v", wrapped) } } + +// recordedPush captures one receive-pack request as the server saw it: how +// many ref-update commands it carried and the pack bytes that followed them. +type recordedPush struct { + commands int + pack []byte +} + +// pushRecorder is a receive-pack server that records every request, so a test +// can assert how a single PushPack/PushCommands call split into batches. +type pushRecorder struct { + mu sync.Mutex + pushes []recordedPush +} + +func (rec *pushRecorder) server(t *testing.T) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("read request body: %v", err) + } + _ = r.Body.Close() + + rd := bytes.NewReader(body) + req := &packp.UpdateRequests{} + if err := req.Decode(rd); err != nil { + t.Errorf("decode update requests: %v", err) + } + rest, err := io.ReadAll(rd) + if err != nil { + t.Errorf("read pack remainder: %v", err) + } + + rec.mu.Lock() + rec.pushes = append(rec.pushes, recordedPush{commands: len(req.Commands), pack: rest}) + rec.mu.Unlock() + + w.WriteHeader(http.StatusOK) + })) +} + +func makeCreateCommands(n int) []PushCommand { + h := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + cmds := make([]PushCommand, n) + for i := range cmds { + cmds[i] = PushCommand{ + Name: plumbing.ReferenceName(fmt.Sprintf("refs/heads/b-%d", i)), + New: h, + } + } + return cmds +} + +func TestChunkRefUpdates(t *testing.T) { + require.Len(t, chunkRefUpdates(nil), 1) + require.Len(t, chunkRefUpdates(make([]PushCommand, maxRefUpdatesPerPush)), 1) + + batches := chunkRefUpdates(make([]PushCommand, maxRefUpdatesPerPush+1)) + require.Len(t, batches, 2) + require.Len(t, batches[0], maxRefUpdatesPerPush) + require.Len(t, batches[1], 1) +} + +// TestPushCommandsBatchesOverCap guards that a ref-only push exceeding the +// per-request cap splits into multiple receive-pack requests, each within the +// cap, so the server's too-many-ref-update-commands limit isn't tripped. +func TestPushCommandsBatchesOverCap(t *testing.T) { + rec := &pushRecorder{} + srv := rec.server(t) + defer srv.Close() + + conn := connForServer(t, srv) + adv := &packp.AdvRefs{} + + n := maxRefUpdatesPerPush + 5 + require.NoError(t, PushCommands(context.Background(), conn, adv, makeCreateCommands(n), false, nil)) + + rec.mu.Lock() + defer rec.mu.Unlock() + require.Len(t, rec.pushes, 2) + require.Equal(t, maxRefUpdatesPerPush, rec.pushes[0].commands) + require.Equal(t, 5, rec.pushes[1].commands) + // Every create batch carries a valid empty pack. + require.True(t, bytes.HasSuffix(rec.pushes[0].pack, emptyPack(adv))) + require.True(t, bytes.HasSuffix(rec.pushes[1].pack, emptyPack(adv))) +} + +// TestPushPackBatchesOverCap guards that a pack push exceeding the per-request +// cap sends the pack with the first batch and the remaining refs as ref-only +// follow-up batches (the objects are already committed by the first request). +func TestPushPackBatchesOverCap(t *testing.T) { + rec := &pushRecorder{} + srv := rec.server(t) + defer srv.Close() + + conn := connForServer(t, srv) + adv := &packp.AdvRefs{} + + marker := []byte("REAL-PACK-PAYLOAD-MARKER") + pack := io.NopCloser(bytes.NewReader(marker)) + + n := maxRefUpdatesPerPush + 5 + require.NoError(t, PushPack(context.Background(), conn, adv, makeCreateCommands(n), pack, false, nil)) + + rec.mu.Lock() + defer rec.mu.Unlock() + require.Len(t, rec.pushes, 2) + // First batch: the real pack rides with a full cap's worth of commands. + require.Equal(t, maxRefUpdatesPerPush, rec.pushes[0].commands) + require.Equal(t, marker, rec.pushes[0].pack) + // Remaining refs follow ref-only: an empty pack, no object payload. + require.Equal(t, 5, rec.pushes[1].commands) + require.True(t, bytes.HasSuffix(rec.pushes[1].pack, emptyPack(adv))) + require.False(t, bytes.Contains(rec.pushes[1].pack, marker)) +} From 88a1ebfcd0afdcc57ec9a74d1f75660af0c87eab Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 18 Jun 2026 18:02:46 +0200 Subject: [PATCH 2/4] Default ref-update batch to 5000; make it env-tunable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub returns 500 Internal Server Error when a single push updates ~10k refs at once but accepts 5k — well below entire-server's 25k cap. Lower the default batch to 5_000 so mirroring a many-ref repo works against GitHub out of the box, and add GITSYNC_MAX_REF_UPDATES_PER_PUSH to raise it for targets known to tolerate larger pushes (entire-server, up to 25k) and cut round trips. Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: 7f01a5ec2f19 --- internal/gitproto/push.go | 35 ++++++++++++++++++++++++++++------ internal/gitproto/push_test.go | 14 ++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index 072765e8..5b2e9628 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -9,6 +9,7 @@ import ( "io" "os" "slices" + "strconv" "strings" "sync/atomic" "time" @@ -50,18 +51,40 @@ func NewPusher(conn Conn, adv *packp.AdvRefs, verbose bool) *Pusher { return &Pusher{Conn: conn, Adv: adv, Verbose: verbose} } -// maxRefUpdatesPerPush bounds how many ref-update commands ride in a single -// receive-pack request. entire-server rejects a push carrying more than 25_000 -// commands (server/githttp.maxRefUpdateCommands), and other servers may impose -// their own caps; staying well under that lets a sync of a many-ref repo split -// across several pushes instead of failing outright. +// defaultMaxRefUpdatesPerPush bounds how many ref-update commands ride in a +// single receive-pack request. The default is deliberately conservative: +// GitHub returns 500 Internal Server Error when a single push updates ~10k refs +// at once but accepts 5k, so 5_000 mirrors a many-ref repo there without +// tripping its (undocumented) ceiling. entire-server tolerates far more — its +// hard cap is 25_000 (server/githttp.maxRefUpdateCommands) — so trusted callers +// pushing to entire-server raise this via MaxRefUpdatesEnv to cut round trips. // // Splitting is safe because the pack accompanying the first batch carries every // object for the whole push: receive-pack commits the entire received pack into // the object store (entire-server via CommitQuarantinedFanout, canonical git via // tmp_objdir_migrate — neither prunes objects unreachable from the pushed tips), // so the remaining batches only need to move ref pointers and carry no pack. -const maxRefUpdatesPerPush = 20_000 +const defaultMaxRefUpdatesPerPush = 5_000 + +// MaxRefUpdatesEnv overrides defaultMaxRefUpdatesPerPush with a positive +// integer. Raise it for targets known to accept large ref-update pushes (e.g. +// entire-server, up to its 25_000 cap) to reduce round trips; lower it for a +// provider that rejects even the default. Invalid or non-positive values fall +// back to the default. +const MaxRefUpdatesEnv = "GITSYNC_MAX_REF_UPDATES_PER_PUSH" + +// maxRefUpdatesPerPush is resolved once from the environment so the limit can be +// tuned per target without rebuilding (see MaxRefUpdatesEnv). +var maxRefUpdatesPerPush = resolveMaxRefUpdatesPerPush() + +func resolveMaxRefUpdatesPerPush() int { + if v := os.Getenv(MaxRefUpdatesEnv); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 { + return n + } + } + return defaultMaxRefUpdatesPerPush +} // chunkRefUpdates splits commands into batches no larger than // maxRefUpdatesPerPush. Input that already fits is returned as a single batch diff --git a/internal/gitproto/push_test.go b/internal/gitproto/push_test.go index 8513b6a9..e782e122 100644 --- a/internal/gitproto/push_test.go +++ b/internal/gitproto/push_test.go @@ -790,6 +790,20 @@ func makeCreateCommands(n int) []PushCommand { return cmds } +func TestResolveMaxRefUpdatesPerPush(t *testing.T) { + t.Setenv(MaxRefUpdatesEnv, "") + require.Equal(t, defaultMaxRefUpdatesPerPush, resolveMaxRefUpdatesPerPush()) + + t.Setenv(MaxRefUpdatesEnv, "20000") + require.Equal(t, 20000, resolveMaxRefUpdatesPerPush()) + + // Invalid or non-positive values fall back to the default. + for _, bad := range []string{"0", "-5", "lots"} { + t.Setenv(MaxRefUpdatesEnv, bad) + require.Equal(t, defaultMaxRefUpdatesPerPush, resolveMaxRefUpdatesPerPush(), "value %q", bad) + } +} + func TestChunkRefUpdates(t *testing.T) { require.Len(t, chunkRefUpdates(nil), 1) require.Len(t, chunkRefUpdates(make([]PushCommand, maxRefUpdatesPerPush)), 1) From 6f123838d6caae5ab9e621f95b2a9a5b3095332b Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 18 Jun 2026 18:27:21 +0200 Subject: [PATCH 3/4] Add --target-max-ref-updates flag; quiet ref-only batch progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plumb a per-target ref-update batch size through the Pusher (MaxRefUpdates), the syncer config, the unstable Options, and the replicate/sync/plan/bootstrap commands as --target-max-ref-updates. Zero keeps the env-or-default limit (GITSYNC_MAX_REF_UPDATES_PER_PUSH or 5000); a positive value overrides it — raise it for entire-server targets (up to 25k), lower it for stricter providers. No global mutable state: the value rides on the Pusher. Also stop spewing a bare "target:" sideband line per ref-only follow-up batch: those carry no useful progress, so push them with progress suppressed and, when verbose, emit one concise "pushed ref-update batch N/M (K refs)" line per batch instead. Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: d908aa3f2758 --- cmd/git-sync/bootstrap.go | 1 + cmd/git-sync/syncplan.go | 1 + internal/gitproto/push.go | 104 ++++++++++++++++++++--------- internal/gitproto/push_test.go | 116 ++++++++++++++++++++++++--------- internal/syncer/syncer.go | 2 + unstable/client.go | 9 ++- 6 files changed, 168 insertions(+), 65 deletions(-) diff --git a/cmd/git-sync/bootstrap.go b/cmd/git-sync/bootstrap.go index a9591e89..1dcd2df1 100644 --- a/cmd/git-sync/bootstrap.go +++ b/cmd/git-sync/bootstrap.go @@ -79,6 +79,7 @@ func newBootstrapCmd() *cobra.Command { cmd.Flags().BoolVar(&jsonOutput, "json", false, "print JSON output") cmd.Flags().Int64Var(&req.Options.MaxPackBytes, "max-pack-bytes", 0, "abort bootstrap if the streamed source pack exceeds this many bytes") cmd.Flags().Int64Var(&req.Options.TargetMaxPackBytes, "target-max-pack-bytes", 0, "target receive-pack body size limit; batches are planned and auto-subdivided to fit") + cmd.Flags().IntVar(&req.Options.TargetMaxRefUpdates, "target-max-ref-updates", 0, "max ref-update commands per receive-pack request; 0 uses the default (env GITSYNC_MAX_REF_UPDATES_PER_PUSH or 5000). Raise for entire-server targets (up to 25000); lower for providers that reject large ref pushes") cmd.Flags().StringVar(&req.Options.BootstrapStrategy, "bootstrap-strategy", "", "checkpoint chain ordering: \"first-parent\" (default) or \"topo\". Use \"topo\" for merge-heavy repos where individual first-parent steps drag in unboundedly large side branches; requires the target to allow non-fast-forward updates on the refs/gitsync/ namespace") addProtocolFlag(cmd, &protocolVal) cmd.Flags().BoolVarP(&req.Options.Verbose, "verbose", "v", false, "verbose logging") diff --git a/cmd/git-sync/syncplan.go b/cmd/git-sync/syncplan.go index bd5c5e06..cac81151 100644 --- a/cmd/git-sync/syncplan.go +++ b/cmd/git-sync/syncplan.go @@ -138,6 +138,7 @@ func newSyncLikeCmd(name, short string, dryRun bool, defaultMode gitsync.Operati cmd.Flags().IntVar(&req.Options.MaterializedMaxObjects, "materialized-max-objects", unstable.DefaultMaterializedMaxObjects, "abort non-relay materialized syncs above this many objects") cmd.Flags().Int64Var(&req.Options.MaxPackBytes, "max-pack-bytes", 0, "abort bootstrap-relay push if the streamed source pack exceeds this many bytes") cmd.Flags().Int64Var(&req.Options.TargetMaxPackBytes, "target-max-pack-bytes", 0, "target receive-pack body size limit; batches are planned and auto-subdivided to fit") + cmd.Flags().IntVar(&req.Options.TargetMaxRefUpdates, "target-max-ref-updates", 0, "max ref-update commands per receive-pack request; 0 uses the default (env GITSYNC_MAX_REF_UPDATES_PER_PUSH or 5000). Raise for entire-server targets (up to 25000); lower for providers that reject large ref pushes") cmd.Flags().StringVar(&req.Options.BootstrapStrategy, "bootstrap-strategy", "", "checkpoint chain ordering for bootstrap: \"first-parent\" (default) or \"topo\". Use \"topo\" for merge-heavy repos where individual first-parent steps drag in unboundedly large side branches; requires the target to allow non-fast-forward updates on the refs/gitsync/ namespace") addProtocolFlag(cmd, &protocolVal) cmd.Flags().BoolVarP(&req.Options.Verbose, "verbose", "v", false, "verbose logging") diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index 5b2e9628..102656b4 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -44,6 +44,11 @@ type Pusher struct { Adv *packp.AdvRefs Verbose bool OnRejection func(refName plumbing.ReferenceName, status string) + + // MaxRefUpdates caps ref-update commands per receive-pack request. Zero + // uses the env-or-default limit (see MaxRefUpdatesEnv); a positive value + // overrides it — e.g. from the --target-max-ref-updates flag. + MaxRefUpdates int } // NewPusher builds a target-side push executor. @@ -86,45 +91,70 @@ func resolveMaxRefUpdatesPerPush() int { return defaultMaxRefUpdatesPerPush } -// chunkRefUpdates splits commands into batches no larger than -// maxRefUpdatesPerPush. Input that already fits is returned as a single batch -// (including the empty slice, so callers preserve their one-request behavior). -func chunkRefUpdates(commands []PushCommand) [][]PushCommand { - if len(commands) <= maxRefUpdatesPerPush { +// effectiveMaxRefUpdates resolves a per-push limit: a positive override wins, +// otherwise the env-or-default limit applies. +func effectiveMaxRefUpdates(maxRefUpdates int) int { + if maxRefUpdates > 0 { + return maxRefUpdates + } + return maxRefUpdatesPerPush +} + +// chunkRefUpdates splits commands into batches no larger than limit. Input that +// already fits is returned as a single batch (including the empty slice, so +// callers preserve their one-request behavior). +func chunkRefUpdates(commands []PushCommand, limit int) [][]PushCommand { + if len(commands) <= limit { return [][]PushCommand{commands} } - batches := make([][]PushCommand, 0, (len(commands)+maxRefUpdatesPerPush-1)/maxRefUpdatesPerPush) - for start := 0; start < len(commands); start += maxRefUpdatesPerPush { - end := min(start+maxRefUpdatesPerPush, len(commands)) + batches := make([][]PushCommand, 0, (len(commands)+limit-1)/limit) + for start := 0; start < len(commands); start += limit { + end := min(start+limit, len(commands)) batches = append(batches, commands[start:end]) } return batches } -// splitFirstBatch peels off the first batch (up to maxRefUpdatesPerPush) so a -// push can carry the pack with that batch and send the remainder as ref-only -// follow-ups. rest is nil when commands already fit in a single request. -func splitFirstBatch(commands []PushCommand) (first, rest []PushCommand) { - if len(commands) <= maxRefUpdatesPerPush { +// splitFirstBatch peels off the first batch (up to limit) so a push can carry +// the pack with that batch and send the remainder as ref-only follow-ups. rest +// is nil when commands already fit in a single request. +func splitFirstBatch(commands []PushCommand, limit int) (first, rest []PushCommand) { + if len(commands) <= limit { return commands, nil } - return commands[:maxRefUpdatesPerPush], commands[maxRefUpdatesPerPush:] + return commands[:limit], commands[limit:] +} + +// logRefUpdateBatch reports completion of one ref-update batch to the progress +// writer. Ref-only follow-up batches push with progress suppressed (their +// sideband carries nothing but a bare "target:" line per batch), so this is the +// only per-batch signal; it stays quiet unless verbose and the push actually +// spanned multiple batches. +func logRefUpdateBatch(conn Conn, verbose bool, batchNum, totalBatches, refs int) { + if !verbose || totalBatches <= 1 { + return + } + w := conn.ProgressWriter() + if w == nil { + w = os.Stderr + } + fmt.Fprintf(w, "target: pushed ref-update batch %d/%d (%d refs)\n", batchNum, totalBatches, refs) } // PushPack streams a pack to the target. func (p *Pusher) PushPack(ctx context.Context, commands []PushCommand, pack io.ReadCloser) error { - return PushPack(ctx, p.Conn, p.Adv, commands, pack, p.Verbose, p.OnRejection) + return PushPack(ctx, p.Conn, p.Adv, commands, pack, p.MaxRefUpdates, p.Verbose, p.OnRejection) } // PushCommands sends ref-only updates. Creates/updates carry an empty pack; // delete-only pushes carry no pack. See the package-level PushCommands. func (p *Pusher) PushCommands(ctx context.Context, commands []PushCommand) error { - return PushCommands(ctx, p.Conn, p.Adv, commands, p.Verbose, p.OnRejection) + return PushCommands(ctx, p.Conn, p.Adv, commands, p.MaxRefUpdates, p.Verbose, p.OnRejection) } // PushObjects encodes and pushes locally materialized objects. func (p *Pusher) PushObjects(ctx context.Context, commands []PushCommand, store storer.Storer, hashes []plumbing.Hash) error { - return PushObjects(ctx, p.Conn, p.Adv, commands, store, hashes, p.Verbose, p.OnRejection) + return PushObjects(ctx, p.Conn, p.Adv, commands, store, hashes, p.MaxRefUpdates, p.Verbose, p.OnRejection) } // buildUpdateRequest builds the receive-pack update request. @@ -419,11 +449,12 @@ func sendReceivePack( // PushObjects pushes locally-materialized objects to the target. // -// A push within the per-request ref-update cap (maxRefUpdatesPerPush) is a -// single atomic receive-pack request. A larger push is split: the materialized -// pack — which carries every object for the whole push — rides with the first -// batch of object-bearing commands, then the remaining refs (and any deletes) -// move as ref-only updates because the objects are already committed. +// A push within the per-request ref-update limit (see effectiveMaxRefUpdates) +// is a single atomic receive-pack request. A larger push is split: the +// materialized pack — which carries every object for the whole push — rides +// with the first batch of object-bearing commands, then the remaining refs (and +// any deletes) move as ref-only updates because the objects are already +// committed. func PushObjects( ctx context.Context, conn Conn, @@ -431,10 +462,12 @@ func PushObjects( commands []PushCommand, store storer.Storer, hashes []plumbing.Hash, + maxRefUpdates int, verbose bool, onRejection func(plumbing.ReferenceName, string), ) error { - if len(commands) <= maxRefUpdatesPerPush { + limit := effectiveMaxRefUpdates(maxRefUpdates) + if len(commands) <= limit { return pushObjectsBatch(ctx, conn, adv, commands, store, hashes, verbose, onRejection) } @@ -449,18 +482,18 @@ func PushObjects( } if len(updates) > 0 { - first, rest := splitFirstBatch(updates) + first, rest := splitFirstBatch(updates, limit) if err := pushObjectsBatch(ctx, conn, adv, first, store, hashes, verbose, onRejection); err != nil { return err } if len(rest) > 0 { - if err := PushCommands(ctx, conn, adv, rest, verbose, onRejection); err != nil { + if err := PushCommands(ctx, conn, adv, rest, maxRefUpdates, verbose, onRejection); err != nil { return err } } } if len(deletes) > 0 { - return PushCommands(ctx, conn, adv, deletes, verbose, onRejection) + return PushCommands(ctx, conn, adv, deletes, maxRefUpdates, verbose, onRejection) } return nil } @@ -670,6 +703,7 @@ func PushPack( adv *packp.AdvRefs, commands []PushCommand, pack io.ReadCloser, + maxRefUpdates int, verbose bool, onRejection func(plumbing.ReferenceName, string), ) error { @@ -682,9 +716,9 @@ func PushPack( // The pack carries every object for all commands, so it rides with the // first batch; once committed the remaining refs update without re-sending - // objects. This keeps each request under the server's per-push ref-update - // cap (see maxRefUpdatesPerPush). - first, rest := splitFirstBatch(commands) + // objects. This keeps each request under the target's per-push ref-update + // limit (see effectiveMaxRefUpdates). + first, rest := splitFirstBatch(commands, effectiveMaxRefUpdates(maxRefUpdates)) req, _, _, err := buildUpdateRequest(adv, first, verbose) if err != nil { @@ -702,7 +736,7 @@ func PushPack( } if len(rest) > 0 { - return PushCommands(ctx, conn, adv, rest, verbose, onRejection) + return PushCommands(ctx, conn, adv, rest, maxRefUpdates, verbose, onRejection) } return nil } @@ -722,13 +756,19 @@ func PushCommands( conn Conn, adv *packp.AdvRefs, commands []PushCommand, + maxRefUpdates int, verbose bool, onRejection func(plumbing.ReferenceName, string), ) error { - for _, batch := range chunkRefUpdates(commands) { - if err := pushCommandsBatch(ctx, conn, adv, batch, verbose, onRejection); err != nil { + batches := chunkRefUpdates(commands, effectiveMaxRefUpdates(maxRefUpdates)) + for i, batch := range batches { + // Ref-only batches carry no useful target progress; suppress the empty + // sideband (verbose=false) and report completion ourselves so a large + // push doesn't spew a bare "target:" line per batch. + if err := pushCommandsBatch(ctx, conn, adv, batch, false, onRejection); err != nil { return err } + logRefUpdateBatch(conn, verbose, i+1, len(batches), len(batch)) } return nil } diff --git a/internal/gitproto/push_test.go b/internal/gitproto/push_test.go index e782e122..55b7f9e7 100644 --- a/internal/gitproto/push_test.go +++ b/internal/gitproto/push_test.go @@ -153,7 +153,7 @@ func TestPushPackClosesPackOnSuccess(t *testing.T) { err := PushPack(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - }}, pack, false, nil) + }}, pack, 0, false, nil) if err != nil { t.Fatalf("PushPack returned error: %v", err) } @@ -180,7 +180,7 @@ func TestPushPackClosesPackOnReceivePackError(t *testing.T) { err := PushPack(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - }}, pack, false, nil) + }}, pack, 0, false, nil) if err == nil { t.Fatal("expected PushPack to return an error") } @@ -210,7 +210,7 @@ func TestPushPackClosesPackOnContextCanceled(t *testing.T) { done <- PushPack(ctx, conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - }}, pack, false, nil) + }}, pack, 0, false, nil) }() select { @@ -266,7 +266,7 @@ func TestPushPackStartsHTTPBeforePackFullyRead(t *testing.T) { done <- PushPack(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - }}, pack, false, nil) + }}, pack, 0, false, nil) }() select { @@ -323,7 +323,7 @@ func TestPushObjectsStreamsBody(t *testing.T) { err := PushObjects(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - }}, memory.NewStorage(), nil, false, nil) + }}, memory.NewStorage(), nil, 0, false, nil) if err != nil { t.Fatalf("PushObjects: %v", err) } @@ -419,7 +419,7 @@ func TestPushCommandsSendsEmptyPackForCreate(t *testing.T) { err := PushCommands(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/docs-rules", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - }}, false, nil) + }}, 0, false, nil) require.NoError(t, err) require.True(t, bytes.HasSuffix(awaitBody(t, bodies), emptyPack(adv)), @@ -438,7 +438,7 @@ func TestPushCommandsSendsNoPackForDeleteOnly(t *testing.T) { Name: "refs/gitsync/bootstrap/heads/docs-rules", Old: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), Delete: true, - }}, false, nil) + }}, 0, false, nil) require.NoError(t, err) require.False(t, bytes.Contains(awaitBody(t, bodies), []byte("PACK")), @@ -495,7 +495,7 @@ func TestPushPackRejectsDeletes(t *testing.T) { err = PushPack(context.Background(), conn, adv, []PushCommand{ {Name: "refs/heads/old", Delete: true}, - }, pack, false, nil) + }, pack, 0, false, nil) if err == nil { t.Fatal("expected error for delete in pack push") } @@ -805,18 +805,25 @@ func TestResolveMaxRefUpdatesPerPush(t *testing.T) { } func TestChunkRefUpdates(t *testing.T) { - require.Len(t, chunkRefUpdates(nil), 1) - require.Len(t, chunkRefUpdates(make([]PushCommand, maxRefUpdatesPerPush)), 1) + require.Len(t, chunkRefUpdates(nil, 10), 1) + require.Len(t, chunkRefUpdates(make([]PushCommand, 10), 10), 1) - batches := chunkRefUpdates(make([]PushCommand, maxRefUpdatesPerPush+1)) + batches := chunkRefUpdates(make([]PushCommand, 11), 10) require.Len(t, batches, 2) - require.Len(t, batches[0], maxRefUpdatesPerPush) + require.Len(t, batches[0], 10) require.Len(t, batches[1], 1) } +func TestEffectiveMaxRefUpdates(t *testing.T) { + require.Equal(t, 7, effectiveMaxRefUpdates(7)) + // Zero/negative falls back to the package default (env-or-default). + require.Equal(t, maxRefUpdatesPerPush, effectiveMaxRefUpdates(0)) + require.Equal(t, maxRefUpdatesPerPush, effectiveMaxRefUpdates(-1)) +} + // TestPushCommandsBatchesOverCap guards that a ref-only push exceeding the -// per-request cap splits into multiple receive-pack requests, each within the -// cap, so the server's too-many-ref-update-commands limit isn't tripped. +// per-request limit splits into multiple receive-pack requests, each within the +// limit, so the server's too-many-ref-update-commands cap isn't tripped. func TestPushCommandsBatchesOverCap(t *testing.T) { rec := &pushRecorder{} srv := rec.server(t) @@ -825,21 +832,21 @@ func TestPushCommandsBatchesOverCap(t *testing.T) { conn := connForServer(t, srv) adv := &packp.AdvRefs{} - n := maxRefUpdatesPerPush + 5 - require.NoError(t, PushCommands(context.Background(), conn, adv, makeCreateCommands(n), false, nil)) + // limit=3, 7 refs → batches of 3, 3, 1. + require.NoError(t, PushCommands(context.Background(), conn, adv, makeCreateCommands(7), 3, false, nil)) rec.mu.Lock() defer rec.mu.Unlock() - require.Len(t, rec.pushes, 2) - require.Equal(t, maxRefUpdatesPerPush, rec.pushes[0].commands) - require.Equal(t, 5, rec.pushes[1].commands) + require.Len(t, rec.pushes, 3) + require.Equal(t, []int{3, 3, 1}, []int{rec.pushes[0].commands, rec.pushes[1].commands, rec.pushes[2].commands}) // Every create batch carries a valid empty pack. - require.True(t, bytes.HasSuffix(rec.pushes[0].pack, emptyPack(adv))) - require.True(t, bytes.HasSuffix(rec.pushes[1].pack, emptyPack(adv))) + for _, p := range rec.pushes { + require.True(t, bytes.HasSuffix(p.pack, emptyPack(adv))) + } } // TestPushPackBatchesOverCap guards that a pack push exceeding the per-request -// cap sends the pack with the first batch and the remaining refs as ref-only +// limit sends the pack with the first batch and the remaining refs as ref-only // follow-up batches (the objects are already committed by the first request). func TestPushPackBatchesOverCap(t *testing.T) { rec := &pushRecorder{} @@ -852,17 +859,66 @@ func TestPushPackBatchesOverCap(t *testing.T) { marker := []byte("REAL-PACK-PAYLOAD-MARKER") pack := io.NopCloser(bytes.NewReader(marker)) - n := maxRefUpdatesPerPush + 5 - require.NoError(t, PushPack(context.Background(), conn, adv, makeCreateCommands(n), pack, false, nil)) + // limit=3, 7 refs → first batch of 3 carries the pack, then 3 + 1 ref-only. + require.NoError(t, PushPack(context.Background(), conn, adv, makeCreateCommands(7), pack, 3, false, nil)) rec.mu.Lock() defer rec.mu.Unlock() - require.Len(t, rec.pushes, 2) - // First batch: the real pack rides with a full cap's worth of commands. - require.Equal(t, maxRefUpdatesPerPush, rec.pushes[0].commands) + require.Len(t, rec.pushes, 3) + // First batch: the real pack rides with the first limit's worth of commands. + require.Equal(t, 3, rec.pushes[0].commands) require.Equal(t, marker, rec.pushes[0].pack) // Remaining refs follow ref-only: an empty pack, no object payload. - require.Equal(t, 5, rec.pushes[1].commands) - require.True(t, bytes.HasSuffix(rec.pushes[1].pack, emptyPack(adv))) - require.False(t, bytes.Contains(rec.pushes[1].pack, marker)) + require.Equal(t, 3, rec.pushes[1].commands) + require.Equal(t, 1, rec.pushes[2].commands) + for _, p := range rec.pushes[1:] { + require.True(t, bytes.HasSuffix(p.pack, emptyPack(adv))) + require.False(t, bytes.Contains(p.pack, marker)) + } +} + +// TestPushCommandsVerboseLogsBatches confirms a multi-batch push reports each +// batch to the progress writer when verbose, and stays quiet for one batch. +func TestPushCommandsVerboseLogsBatches(t *testing.T) { + rec := &pushRecorder{} + srv := rec.server(t) + defer srv.Close() + + adv := &packp.AdvRefs{} + + var buf bytes.Buffer + conn := connForServer(t, srv) + conn.ProgressOut = &buf + require.NoError(t, PushCommands(context.Background(), conn, adv, makeCreateCommands(7), 3, true, nil)) + + out := buf.String() + require.Contains(t, out, "pushed ref-update batch 1/3 (3 refs)") + require.Contains(t, out, "pushed ref-update batch 2/3 (3 refs)") + require.Contains(t, out, "pushed ref-update batch 3/3 (1 refs)") + + // Single batch: no per-batch noise. + var single bytes.Buffer + conn2 := connForServer(t, srv) + conn2.ProgressOut = &single + require.NoError(t, PushCommands(context.Background(), conn2, adv, makeCreateCommands(2), 3, true, nil)) + require.NotContains(t, single.String(), "pushed ref-update batch") +} + +// TestPushPackUsesDefaultLimitWhenZero confirms maxRefUpdates=0 falls back to +// the package default (a small push stays a single request). +func TestPushPackUsesDefaultLimitWhenZero(t *testing.T) { + rec := &pushRecorder{} + srv := rec.server(t) + defer srv.Close() + + conn := connForServer(t, srv) + adv := &packp.AdvRefs{} + pack := io.NopCloser(bytes.NewReader([]byte("PACK-PAYLOAD"))) + + require.NoError(t, PushPack(context.Background(), conn, adv, makeCreateCommands(3), pack, 0, false, nil)) + + rec.mu.Lock() + defer rec.mu.Unlock() + require.Len(t, rec.pushes, 1) + require.Equal(t, 3, rec.pushes[0].commands) } diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index b3f41f20..06499e55 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -85,6 +85,7 @@ type Config struct { BestEffort bool MaxPackBytes int64 TargetMaxPackBytes int64 + TargetMaxRefUpdates int MaterializedMaxObjects int ProtocolMode string BootstrapStrategy string // "" | "first-parent" | "topo" @@ -739,6 +740,7 @@ func newSession(ctx context.Context, cfg Config, needTarget bool) (*syncSession, NoThin: targetFeatures.NoThin, } s.target.pusher = gitproto.NewPusher(targetConn, targetAdv, cfg.Verbose) + s.target.pusher.MaxRefUpdates = cfg.TargetMaxRefUpdates if cfg.BestEffort { s.rejections = make(map[plumbing.ReferenceName]string) s.target.pusher.OnRejection = func(name plumbing.ReferenceName, status string) { diff --git a/unstable/client.go b/unstable/client.go index fd9bc658..c17498b7 100644 --- a/unstable/client.go +++ b/unstable/client.go @@ -41,6 +41,7 @@ type AdvancedOptions struct { Progress bool `json:"progress"` MaxPackBytes int64 `json:"maxPackBytes"` TargetMaxPackBytes int64 `json:"targetMaxPackBytes"` + TargetMaxRefUpdates int `json:"targetMaxRefUpdates"` MaterializedMaxObjects int `json:"materializedMaxObjects"` BootstrapStrategy string `json:"bootstrapStrategy,omitempty"` } @@ -279,6 +280,7 @@ func (c *Client) buildSyncConfig(ctx context.Context, req SyncRequest) (syncer.C BestEffort: req.Policy.BestEffort, MaxPackBytes: req.Options.MaxPackBytes, TargetMaxPackBytes: req.Options.TargetMaxPackBytes, + TargetMaxRefUpdates: req.Options.TargetMaxRefUpdates, MaterializedMaxObjects: maxObjects, ProtocolMode: protocolString(req.Policy.Protocol), Verbose: req.Options.Verbose, @@ -308,9 +310,10 @@ func (c *Client) buildBootstrapConfig(ctx context.Context, req BootstrapRequest) ShowStats: req.Options.CollectStats, MeasureMemory: req.Options.MeasureMemory, Progress: req.Options.Progress, - MaxPackBytes: req.Options.MaxPackBytes, - TargetMaxPackBytes: req.Options.TargetMaxPackBytes, - ProtocolMode: protocolString(req.Protocol), + MaxPackBytes: req.Options.MaxPackBytes, + TargetMaxPackBytes: req.Options.TargetMaxPackBytes, + TargetMaxRefUpdates: req.Options.TargetMaxRefUpdates, + ProtocolMode: protocolString(req.Protocol), Verbose: req.Options.Verbose, BootstrapStrategy: req.Options.BootstrapStrategy, }, nil From 85aba74bc6c6fdb130df5dfb8907473d2bae55da Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 18 Jun 2026 18:42:27 +0200 Subject: [PATCH 4/4] gofmt: realign config structs after TargetMaxRefUpdates Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: 3ab7999b3055 --- internal/syncer/syncer.go | 2 +- unstable/client.go | 28 ++++++++++++++-------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 06499e55..390ed8d0 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -740,7 +740,7 @@ func newSession(ctx context.Context, cfg Config, needTarget bool) (*syncSession, NoThin: targetFeatures.NoThin, } s.target.pusher = gitproto.NewPusher(targetConn, targetAdv, cfg.Verbose) - s.target.pusher.MaxRefUpdates = cfg.TargetMaxRefUpdates + s.target.pusher.MaxRefUpdates = cfg.TargetMaxRefUpdates if cfg.BestEffort { s.rejections = make(map[plumbing.ReferenceName]string) s.target.pusher.OnRejection = func(name plumbing.ReferenceName, status string) { diff --git a/unstable/client.go b/unstable/client.go index c17498b7..2c91f633 100644 --- a/unstable/client.go +++ b/unstable/client.go @@ -298,24 +298,24 @@ func (c *Client) buildBootstrapConfig(ctx context.Context, req BootstrapRequest) return syncer.Config{}, err } return syncer.Config{ - Source: source, - Target: target, - HTTPClient: c.httpClient, - Branches: append([]string(nil), req.Scope.Branches...), - Mappings: validationMappings(req.Scope.Mappings), - AllRefs: req.Scope.AllRefs, - ExcludeRefPrefixes: append([]string(nil), req.Scope.ExcludeRefPrefixes...), - IncludeTags: req.IncludeTags, - BestEffort: req.BestEffort, - ShowStats: req.Options.CollectStats, - MeasureMemory: req.Options.MeasureMemory, - Progress: req.Options.Progress, + Source: source, + Target: target, + HTTPClient: c.httpClient, + Branches: append([]string(nil), req.Scope.Branches...), + Mappings: validationMappings(req.Scope.Mappings), + AllRefs: req.Scope.AllRefs, + ExcludeRefPrefixes: append([]string(nil), req.Scope.ExcludeRefPrefixes...), + IncludeTags: req.IncludeTags, + BestEffort: req.BestEffort, + ShowStats: req.Options.CollectStats, + MeasureMemory: req.Options.MeasureMemory, + Progress: req.Options.Progress, MaxPackBytes: req.Options.MaxPackBytes, TargetMaxPackBytes: req.Options.TargetMaxPackBytes, TargetMaxRefUpdates: req.Options.TargetMaxRefUpdates, ProtocolMode: protocolString(req.Protocol), - Verbose: req.Options.Verbose, - BootstrapStrategy: req.Options.BootstrapStrategy, + Verbose: req.Options.Verbose, + BootstrapStrategy: req.Options.BootstrapStrategy, }, nil }