can you review the changes in this branch

Claude Code·Opus 4.6[1m]·Soph·5mo ago·377hr 26min·23 Checkpoints·90 file changes·+2806/-961·3.2M tokens

can you review the changes in this branch

5mo ago·2m

Here's my review of the branch changes. The diff adds a new replicate operation mode (source-authoritative relay-only) across the CLI, library API, planner, and a new internal/strategy/replicate package.

Overall

Solid, well-tested addition. The split between sync (reconciliation) and replicate (overwrite) is clean, new code is mostly additive, and there's good test coverage at planner, strategy, session, and CLI levels. A few things worth addressing before merge.

Issues / nitpicks

1. Breaking JSON field rename — execution.mode → execution.transfer_mode (pkg/gitsync/internalbridge/model.go:98,145) ExecutionSummary.Mode → TransferMode is a backwards-incompatible change to the stable embedding surface. If the stability promise matters for pkg/gitsync, consumers parsing execution.mode will silently break. Either (a) keep mode as a deprecated alias, (b) call this out in a CHANGELOG, or (c) confirm no external consumers yet.

2. Dead/confusing branch in defaultOperationMode (cmd/git-sync/main.go:412-424)

The name == "plan" branch is identical to the fallback. Just return operationMode(gitsync.ModeSync). Also — plan passes defaultMode="" intentionally, but sync and replicate always pass a value, so name is never needed. The helper can shrink to if defaultMode != "" { return … }; return operationMode(ModeSync).

3. BuildReplicationPlans mutates its caller's managed map (internal/planner/planner.go:174-185) The prune branch writes back into the passed-in map. The only caller happens not to reuse it after, but other planner functions (BuildPlans) don't do this. Either clone the map internally or document the side effect. This is the kind of thing that becomes a bug in 6 months.

4. dryRun argument in CanReplicateRelay is effectively dead (internal/planner/relay.go:122-124, called at internal/syncer/syncer.go:~632) The only call site is planner.CanReplicateRelay(false, relayPlans, …) — hardcoded. Either drop the parameter or call it from the dry-run path too. Currently it's confusing API surface pretending to support something it doesn't.

5. runReplicate allAbsent ignores managed refs that aren't in desiredRefs (internal/syncer/syncer.go:~555-565) allAbsent only scans desiredRefs. If prune is on and the target has managed refs to delete while every desired ref is absent, we jump into the bootstrap path, which doesn't handle the deletes. Probably low-incidence (empty-target implies empty managed set on first run), but worth either asserting or adding a test for replicate --prune against an "empty except for orphaned managed ref" target.

6. SupportsReplicateRelay called twice (internal/syncer/syncer.go:~548 and again inside CanReplicateRelay at ~632) Minor redundancy — CanReplicateRelay already calls SupportsReplicateRelay. Not wrong, just inefficient readability-wise.

7. closeOnce in the strategy is defensive but unused externally (internal/strategy/replicate/replicate.go:83-104) gitproto.LimitPackReader doesn't wrap in a double-close-safe reader? Worth a quick check whether the _ = packReader.Close() in both the success and error paths + defer pack.Close() in PushPack implementations is the actual duplication hazard. If so, a one-line comment explaining why closeOnce exists would save future readers.

8. Usage string duplication (cmd/git-sync/main.go:~410) The huge fmt.Sprintf with every subcommand's flags is now ~doubled in length. Not new to this PR, but now's a good time to split it into per-subcommand helpers or a usageFor(name) map.

9. Replicate relay vs. --prune delete flow Deletes go through PushCommands without a pack. That's correct, but there's no test covering replicate --prune end-to-end through the syncer (only planner). Worth adding one to catch regressions where pushPlans ordering or relayPlans filtering drifts.

Nice touches

  • Test server now has a reusable rewriteReceivePackAdvertisement helper — cleaner than the previous inline capability mutation.
  • operationModeString defaulting empty → "sync" in both bridges is consistent.
  • replicate correctly rejects --force at session construction with a clear "use sync instead" message; error wrapping throughout runReplicate consistently steers users back to sync on failure.
  • Planner's PlanReplicationRef reasons ("replicate overwrite" vs "replicate tag overwrite") are clear for debugging.

Suggested follow-ups (not blockers)

  • CHANGELOG entry for TransferMode rename.
  • Document the "replicate fails → retry with sync" semantics in docs/architecture.md — the code implements it but the doc only hints at it.

Want me to apply the small mechanical fixes (dead branch in defaultOperationMode, dryRun param cleanup, managed map cloning)?

f377186Add relay-only replicate engine mode Entire-Checkpoint: be84133ec5bb+692/-20
3486c1bExpose replicate mode in API and CLI Entire-Checkpoint: ce4b384e68dd+244/-19
307d008Add integration tests for replicate happy paths and --force rejection Previously replicate coverage at the syncer layer only exercised rejection cases (no-thin target, prune-into-bootstrap). This commit adds the missing green-path tests: - Overwrites a non-fast-forward branch rewind via relay pack push - Bootstraps an empty target through the replicate path and carries OperationMode=replicate through the bootstrap result - Retargets a divergent tag end-to-end against the receive-pack server - Deletes an orphaned managed ref under --prune alongside an update push, exercising the separate PushCommands delete path - Rejects --force at session construction before any network I/O, asserting the "use sync instead" guidance text Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: d40ffe7a25bc+264
a9b1834Add CLI tests for replicate subcommand and --force rejection The existing CLI coverage only exercised replicate through 'plan --mode replicate'. This adds tests for the 'replicate' subcommand itself: - End-to-end execution against an empty target, asserting the receive-pack POST happened, target head matches source, and the flat CLI JSON reports operation_mode=replicate plus relay=true with the bootstrap relay_reason - --force rejection at the CLI layer, covering the 'use sync instead' guidance text users see when invoking the subcommand with an incompatible flag Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4186877e71b9+77
b5fdb49Drain go-git upload-pack double-NAK before sideband demux Root cause: go-git's v6 upload-pack server (plumbing/transport/upload_pack.go) emits two NAK pktlines when the client sends haves but none are reachable from the wants. The first NAK is written via ServerResponse.Encode for the "if len(haves) > 0" branch (empty ACKs => NAK), the second via the "no common ack found" branch. The response looks like: 0008NAK\n 0008NAK\n 0009\x01PACK... (sideband channel 1) go-git's ServerResponse.Decode returns after consuming the first NAK, so the second NAK sits in front of the sideband-wrapped pack. The sideband demuxer then reads "NAK\n" as a frame and fails with "unknown channel NAK" (channel byte 'N' = 0x4e) because valid channels are 0x01/0x02/0x03. This reliably hits any fetch where the target's advertised ref is not an ancestor of the source's want - exactly the rewind/overwrite scenario replicate mode is designed for. The same bug affected sync --force via the materialized strategy, but was masked in existing tests because multi-ref scenarios happened to include at least one have whose hash was reachable from a want, producing an ACK and skipping the two-NAK branch. Fix: wrap the post-Decode reader in bufio.Reader and drain any extra "0008NAK\n" pktlines before handing off to the sideband demuxer. Applied to both fetchToStoreV1 (materialized path) and fetchPackV1 (relay path). A short stream that cannot satisfy the 8-byte peek is treated as "no more NAKs" so the downstream reader surfaces the real read error. Also reverts the V1->V2 workaround in TestRun_IntegrationReplicateOverwrites- DivergentBranch now that V1 handles this scenario correctly, and adds TestFetchPackV1DrainsSecondNAK asserting the drainer's behavior directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a8de9a2a91c3+88/-5
416620dDocument replicate API and stable-surface JSON rename Adds a CHANGELOG describing the replicate mode, the renamed execution.mode -> execution.transfer_mode field on the stable SyncResult surface, and the new top-level operation_mode on syncer.Result. Updates the embedding guide to list Replicate alongside Probe/Plan/Sync and to name the new Execution fields (OperationMode, TransferMode) in the result-handling and retry-guidance sections. The rename is breaking for external consumers parsing execution.mode; there is no compatibility alias. This is flagged under "Changed (stable API, breaking)" in the CHANGELOG. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: bab2cea0784a+52/-2
5b7223cDrop unused defaultOperationMode name param, lock in planner copy Two small cleanups flagged during review: - defaultOperationMode took a 'name' argument that was only used by a redundant branch (plan-vs-other that resolved to the same default). The function's real input is the pinned mode, so drop the name parameter and add a comment explaining why an empty defaultMode falls through to ModeSync: that case is 'plan', which accepts --mode. - BuildReplicationPlans already defensively copies the caller's managed map before adding prune-eligible orphans, but nothing tested the non-mutation guarantee. Add TestBuildReplicationPlansDoesNotMutate- Managed so a future refactor can't regress the footgun by dropping the copy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: dd63b36b9c0d+52/-2
1a477f8Let replicate relay against targets advertising no-thin Targets built on go-git's receive-pack (including entire-server) unconditionally advertise the no-thin capability because go-git has a "TODO: support thin-pack" in plumbing/transport/serve.go. Replicate previously rejected such targets with "use sync instead", which made the mode unusable against the most common internal target. Reconsidered the constraint: our upload-pack client (gitproto.fetchPackV1 / fetchPackV2 / fetchToStoreV1) never sets the "thin-pack" capability in the request. By protocol rules the source only emits thin packs when the client explicitly asks for them, so the pack we relay is always self-contained and safe to push to a no-thin receive-pack. The rejection was overcautious. Changes: - planner.SupportsReplicateRelay no longer fails on target.NoThin. It returns ok with reason "replicate-target-capable-no-thin" so callers can still observe the distinction in logs and JSON. - gitproto fetch code gets explicit comments at both the v1 and v2 upload-request build sites documenting that we do not request thin-pack and that SupportsReplicateRelay depends on that invariant. Anyone adding thin-pack support later must update the planner check to gate on target NoThin. - Integration test flipped: replicate against a no-thin target must now succeed and leave source and target heads matching, rather than fail with "use sync instead". - Planner test split into "tolerates no-thin" and "rejects unknown capabilities" so the two branches of SupportsReplicateRelay each have direct coverage. - CHANGELOG and docs/architecture.md updated to explain the new behavior and the thin-pack invariant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e0212dc3cfb8+68/-16
6a219a1Wire max/batch pack byte flags into sync and replicate Replicate against a fresh target for a repo the size of linux currently fails with "decode report-status: invalid pkt-len found: short pkt-line 4" because the receive-pack POST body hits a server limit (or times out) and the target closes the connection before writing a report-status. The fix is batching, which the bootstrap strategy already supports and which replicate's bootstrap-fallback already plumbs to the strategy via Config.BatchMaxPackBytes -- but the flags were only registered on the bootstrap subcommand. Replicate and sync silently ignored them. Changes: - cmd/git-sync/main.go runSyncLike: register --max-pack-bytes and --batch-max-pack-bytes. Update the usage string for sync, replicate, and plan accordingly. - pkg/gitsync/unstable/client.go buildSyncConfig: forward MaxPackBytes and BatchMaxPackBytes from AdvancedOptions to syncer.Config. The fields already existed on AdvancedOptions; only bootstrap was using them. - internal/syncer/integration_test.go: add TestRun_Integration- ReplicateBootstrapBatchesWhenConfigured exercising a replicate call with BatchMaxPackBytes set, asserting it reaches the batched bootstrap path (batching=true, batch_count>=2) and leaves source and target heads matching. This guards the plumbing; regressing the forwarding would silently reintroduce the "flag has no effect" bug. - CHANGELOG.md: document the added flags and the unstable buildSyncConfig forwarding. Typical usage for a large initial replicate push: git-sync replicate \ --batch-max-pack-bytes 268435456 \ --max-pack-bytes 10737418240 \ ... splits the push into ~256 MiB batches, each of which the receive-pack server processes and acknowledges before the next starts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 08a1fb563aa3+54/-1
3b98784Stream sideband progress to stderr when -v is set Long replicate and sync runs against large repos were silent between bootstrap batch checkpoints because both the source upload-pack and the target receive-pack sideband progress channels were being discarded. Changes in internal/gitproto: - push.go: sendReceivePack now takes a verbose flag and wires the sideband demuxer's Progress field to stderr (prefixed "target: ") when set. Added a prefixedLineWriter that splits on both '\n' and '\r' so in-place git progress updates ("Resolving deltas: 12%\r") remain readable when prefixed. progressSink returns nil for non-verbose so the demuxer allocates nothing on the hot path. - fetch.go / refs.go: RefService gains a Verbose field. When true, fetchToStoreV1/V2, fetchPackV1/V2, and buildV1UploadPackBody stop asking the source to suppress progress (drop the "no-progress" upload-request capability and the "no-progress" v2 fetch arg) and wire the sideband demuxer's Progress to stderr (prefixed "source: "). - syncer.go: newSession propagates cfg.Verbose to sourceService.Verbose right after constructing it. Target-side verbose already flowed through gitproto.NewPusher. New tests: - TestPrefixedLineWriter covers line splitting on '\n' and '\r', mid-line writes that don't emit a trailing prefix, and empty writes. - TestProgressSinkNilWhenNotVerbose locks in the "no allocation when quiet" contract. Existing private fetch tests updated to pass the new verbose parameter. Commit-graph fetches (FetchCommitGraph) pass verbose=false because they are short and not user-facing. With -v, a replicate run now looks like: source: Enumerating objects: 120000, done. source: Counting objects: 100% (120000/120000), done. source: Compressing objects: 37% (44400/120000) ... target: Resolving deltas: 58% (69600/120000) target: Updating references: 100% (61/61), done. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: cdd8c35ae583+178/-41
8e4c21cReplace probe-based bootstrap checkpoint planning with estimate The previous checkpoint planner did full FetchPack round-trips per probe candidate to measure actual pack sizes, then binary-searched for the boundary that fit under --batch-max-pack-bytes. For linux/master (75k commits) this took 13+ fetch-and-discard cycles — downloading gigabytes of throwaway data and burning minutes — before any real push started. The precision was rarely needed: the adaptive retry and resume mechanisms already handle batches that turn out too large. Replace with estimate-based planning: 1. Fetch the commit graph (tree:0 filter, one round-trip — unchanged). 2. Walk the first-parent chain to get commit count (unchanged). 3. Estimate total pack size as chainLen × 8 KiB/commit. 4. Divide into ceil(estimated / batchMaxPack) evenly-spaced checkpoints. 5. Done. No probe fetches. For linux at 1 GiB batch limit: planning goes from ~4 minutes / 13 fetches to ~20 seconds / 1 fetch (just the commit graph). The estimate is intentionally conservative (8 KiB vs the old 4 KiB) so it errs toward more batches rather than fewer. If a batch still exceeds the target's limit, the push fails for that batch and bootstrap resume (via temp refs) ensures already-pushed batches aren't re-sent on the next run. Deleted ~535 lines of probe infrastructure: - checkpointPlanner struct and all methods - fetchPackForProbe, probeKey, probeResult, probeBounds - initialCheckpointSpan, adaptiveNextProbeSpan - shouldProbeTipFirst, shouldSelectTipWithoutProbe - nextCheckpointProbeCandidate, searchCheckpointUnderLimit - prefetchedPacks field on plannedBatch and its lookup in packReaderForCheckpoint Added: - estimateBatchCount (ceil division with 8 KiB heuristic) - evenCheckpoints (evenly-spaced placement along the chain) - TestEstimateBatchCount and TestEvenCheckpoints Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: d0974f7b54b6+117/-652
3a8d57cUse MaxPackBytes not BatchMaxPack as per-fetch abort threshold BatchMaxPack controls checkpoint placement (how many batches to plan). MaxPackBytes is the user's hard abort threshold for any single source fetch. The previous code used min(BatchMaxPack, MaxPackBytes) as the per-fetch limit, which meant a batch whose actual pack exceeded the planning estimate was aborted at the source-fetch stage before the target even saw it. With estimate-based planning, the estimate can undercount (8 KiB/commit vs linux's real ~66 KiB/commit), so a batch legitimately exceeds the planning heuristic. The fetch should still complete — if the resulting pack is too large for the target's receive-pack, the push fails and bootstrap resume handles retry. Aborting early at the source side just wastes the transfer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 589898e25466+8/-5
85b386eAuto-subdivide batches on target body-size rejection When a batched bootstrap push is rejected by the target for exceeding its body-size limit, instead of failing immediately, subdivide the remaining checkpoints by inserting midpoints from the stored commit chain and retry. Each failing batch gets split in half, halving the commit range (and roughly halving the pack size) per retry. This converges to batches small enough for the target without requiring the user to manually lower --batch-max-pack-bytes and re-run. The commit chain is already in memory from the planning phase (~1.5 MiB for linux's 75k commits). subdivideCheckpoints walks the remaining checkpoint list, computes chain-index gaps, and inserts a midpoint for each gap > 1 commit. Adjacent commits (gap=1) cannot be split further; if even a single-commit batch is too large, the push fails as before. Example for the linux kernel at 200 MiB batch estimate: - Estimate plans 3 batches (25k commits each) - Batch 3 contains 6.5M objects (~2.3 GiB), target limit is 2 GiB - Push rejected → subdivide inserts midpoint → 4 checkpoints - Retry batch 3a (~12.5k commits, ~1.2 GiB) → succeeds - Continue batch 3b (~12.5k commits, ~1.1 GiB) → succeeds - No user intervention needed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 516fe4e51170+125/-7
fe63356Pre-check PACK header object count before pushing oversized batches When a batch's FetchPack returns, peek at the first 12 bytes of the pack stream to read the PACK header's object count. Multiply by 750 bytes/object (a conservative compressed-object average) to estimate total pack size. If the estimate exceeds BatchMaxPack, subdivide immediately and re-fetch smaller ranges — avoiding a multi-GiB transfer that the target would reject anyway. This is a zero-cost optimization: 12 bytes read + one multiplication. The PACK header is prepended back to the reader via MultiReader so the push receives a valid packfile if the check passes. Combined with the post-push subdivide from the previous commit, the batch loop now has two layers of protection: 1. Pre-push: PACK header estimate catches obviously-oversized batches before any significant transfer (this commit). 2. Post-push: target body-limit rejection triggers subdivide for batches that passed the estimate but still exceeded the target's actual limit (previous commit). For the linux kernel against a 2 GiB target: - Estimate plans 3 batches (even by commit count) - Batch 3 fetches, PACK header shows 6.5M objects × 750 = ~4.9 GiB - 4.9 GiB > 2 GiB BatchMaxPack → subdivide before pushing - Re-fetch two halves (~3.2M objects each, ~2.4 GiB) - If still over, subdivide again — converges in O(log n) splits Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: dbedffba0bc5+146
22f8778Fix subdivide retry loop skipping inserted midpoints, update docs The subdivide-on-failure logic used `continue` inside a `for idx++` loop, which incremented idx past the newly-inserted midpoint checkpoint on every retry. The result: infinite retries of the same oversized range (visible in the linux smoke test as repeated pushes of 6.5M objects at the same from/to hashes). Fix: convert to a manual-index loop (`idx := start; for idx < len; ... idx++` at the bottom). Now `continue` retries at the same idx, where the checkpoint slice has been rewritten to start with the midpoint. Both the PACK-header precheck path and the post-push target-rejection path benefit from the same fix. Also updates docs/bootstrap-batching.md: - Checkpoint Selection section rewritten to describe the estimate-based approach (commit-count heuristic + PACK header precheck + target rejection retry) instead of the removed probe-based binary search. - Practical Risks section updated with the new failure modes (estimate inaccuracy, non-uniform object density, source-side wasted pack builds on PACK header abort). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e00bb1b6c6d8+32/-16
e43f5beRename --batch-max-pack-bytes to --target-max-pack-bytes With estimate-based planning, PACK header precheck, and auto-subdivide, there's no longer a reason for the user to think in terms of "batch sizes." The single meaningful knob is the target's receive-pack body limit. The system estimates checkpoint placement from it, prechecks PACK headers against it, and auto-subdivides when batches exceed it. Rename across CLI flags, internal types, unstable API, docs, and tests: --batch-max-pack-bytes → --target-max-pack-bytes BatchMaxPackBytes → TargetMaxPackBytes BatchMaxPack → TargetMaxPack defaultAutoBatchMaxPackBytes → defaultTargetMaxPackBytes autoBatchMaxPackBytes → autoTargetMaxPackBytes Pure rename, no behavioral changes. All tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: fb6ddecdebbf+66/-66
1af29eaClear stale bootstrap temp refs instead of failing on resume mismatch When a temp ref from a previous run doesn't match any planned checkpoint (e.g., the user changed --target-max-pack-bytes between runs, or a previous subdivide created midpoints that no longer appear in the new plan), bootstrap now deletes the stale temp ref and starts the branch fresh rather than failing with "does not match any planned checkpoint". Temp refs are an internal implementation detail of batched bootstrap, not user-visible state. Treating a mismatch as fatal forced users to manually delete refs on the target before retrying — unnecessary friction for a routine parameter change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 432a6b170dac+36/-40
e185f44Resume from stale temp refs by finding their chain position When a temp ref from a previous run doesn't match any planned checkpoint, look up its hash in the commit chain. If found, use it as the starting point and re-plan remaining checkpoints from there — preserving the data already pushed by previous batches. Only delete the temp ref and start fresh when the hash isn't in the chain at all (truly unrelated data, not just a different checkpoint plan). For the linux case: previous run pushed batches 1-2 (~3 GiB) and left the temp ref at a subdivide midpoint. With different --target-max-pack- bytes, the new plan doesn't include that midpoint as a checkpoint. Instead of re-pushing 3 GiB, we now find the midpoint in the chain (position ~50k of 75k), plan checkpoints for the remaining ~25k commits, and push only what's missing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a474315dec7b+32/-3
3918198Bump estimatedBytesPerCommit from 8 KiB to 64 KiB 8 KiB was inherited from the old probe-based planner where it was just an initial guess that probing would refine. Without probing, it needs to stand on its own as the batch count estimate. At 8 KiB, linux (~66 KiB/commit real) estimated 1 batch when the answer is ~3, forcing the PACK header pre-check to do all the work through repeated subdivisions. 64 KiB is a better general-purpose constant: - Small repos (~5 KiB real): still estimates 1 batch (correct, harmless) - Linux kernel (~66 KiB real): estimates 3 batches at 2 GiB limit (accurate) - Blob-heavy monorepos (~120 KiB real): ~2x under, one PACK header split The PACK header pre-check and target-rejection retry remain as safety nets for the cases where even 64 KiB underestimates, but they should now trigger rarely rather than routinely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ba0e116ded77+7/-1
3c20da5Include completed branch tips as haves when fetching subsequent branches In a multi-branch bootstrap, each branch fetched as if the target was empty — the source had no way to know that previous branches had already pushed shared objects. For repos like linux where master and nocache-cleanup share ~99% of history, this meant re-transferring ~5 GiB of duplicate objects for the second branch. Fix: maintain a completedRefs map across the branch loop. After each branch finishes, add its tip (and temp ref) to the map. When fetching checkpoints for the next branch, merge completedRefs into the haves passed to FetchPack. The source can then delta-compress against objects the target already has, producing a much smaller pack for branches that share history. The tag phase already did this (it built tagTargetRefs from the completed batches). The branch-to-branch loop was the gap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: da38c1b34ae1+16/-2
05d5f1aFree commit graph store immediately after extracting first-parent chain The commit graph fetch for linux stores 1.4M commit objects in memory.NewStorage() (~4.6 GB). We only need the 75k first-parent chain hashes (~3 MB) for checkpoint placement. Previously the store stayed alive for the entire bootstrap run because the function scope held it. Now: extract the chain, nil the store, force GC. The 4.6 GB becomes garbage-collectible immediately after chain extraction (~22s into the run) instead of persisting for the full ~17 minutes. The transient spike during the fetch is unavoidable with current tools: - git protocol v2 has no "first-parent only" fetch command - tree:0 filter gives all 1.4M commits, not just the 75k first-parent - go-git's pack parser needs the full store for delta resolution between commits during pack decoding A streaming parent-map extractor could eliminate the spike entirely but requires go-git packfile internals work (custom storer or scanner integration). The GC approach is the pragmatic fix. Also adds FirstParentChainFromMap to planner/checkpoint.go for future use when a streaming extractor is implemented. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 56d371ce68d4+38
55fe66dFix three issues from PR review 1. evenCheckpoints panic on tiny --target-max-pack-bytes: when numBatches >= len(chain), batchSize truncated to 0 and the index formula produced -1. Guard returns just the tip in this case. 2. Remove stale Mode field from ExecutionSummary: the rename to TransferMode left the old Mode field, causing both "mode" and "transfer_mode" to appear in JSON output. Only TransferMode remains. 3. Make replicate's closeOnceReadCloser thread-safe: switch from plain bool to sync.Once, matching the bootstrap package's implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e6c1525af5a6+21/-11
d851ee0Plan trunk first in batched bootstrap to cut per-branch graph fetches Before: each branch's checkpoint planning fetched the full commit graph from the source independently. For a repo with 152 feature branches that all descend from main, that meant ~152 full-history commit-graph fetches and ~152 independent first-parent walks, each traversing the same shared history. On a linux-sized source this was ~40 s of wall time during planning alone, before any object bytes moved. Now the trunk is identified up front via the source's HEAD symref, planned first, and its commit reachability is captured as a hash set. Each subsequent branch's graph fetch passes trunk's tip as a have so the source returns only the divergence, and the first-parent walk terminates when it enters trunk's ancestry instead of walking back to root. Pieces: - gitproto: expose HEAD symref on RefService.HeadTarget. v1 reads it from the symref capability; v2 adds "symrefs" to ls-refs and a "ref-prefix HEAD" argument so HEAD is actually returned (otherwise ref-prefix refs/heads/ filters it out and HeadTarget stays empty). HEAD itself is consumed for the symref target and not surfaced in the ref slice, matching v1 behavior where symbolic refs are filtered downstream. - gitproto: FetchCommitGraph takes an optional []plumbing.Hash of haves, forwarded into the v2 fetch command. Nil/empty preserves prior behavior. - planner: add FirstParentChainStoppingAt that terminates the walk when a commit is in a caller-supplied stop set. FirstParentChain becomes a thin wrapper with nil stop set for existing callers. - bootstrap: orderTrunkFirst reorders DesiredRefs so the HEAD branch is planned first; collectCommitHashes extracts just the commit hash set (~8 B per commit, ~11 MB for linux) from trunk's graph store before it is GC'd. Subsequent branches plan with trunk's tip as a have and trunk's ancestry as a stop set. Emits one of three diagnostic log lines at planning time so operators can see whether the fast path fired: trunk selected, trunk unset (no HEAD advertised), or trunk unset (HEAD filtered by --branch/--map). Subsumed branches (tip in trunk's ancestry) emit a single-checkpoint batch today — the pack push for those is a follow-up optimization. Empty-chain edge case: when stop-at subsumes the branch tip, FirstParentChainStoppingAt returns nil and planCheckpointsFromChain falls back to [tip] so the existing execute-phase logic still creates the target ref. Falls back to the original per-branch behavior when HeadTarget is empty or when the trunk ref is filtered out of the desired set, so sources without symref advertisement and scoped runs (--branch, --map) are not regressed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 32bf11bd8793+393/-52