what test coverage do we have now for replicate? · Entire

Review of Branch Changes

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)

    func defaultOperationMode(name string, defaultMode gitsync.OperationMode) operationMode {
    if defaultMode != "" { return operationMode(defaultMode) }
    if name == "plan" { return operationMode(gitsync.ModeSync) }
    return operationMode(gitsync.ModeSync)
}

    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. 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) The only call site is planner.CanReplicateRelay(false, relayPlans, …) — hardcoded. Drop the parameter or call it from the dry-run path too.

  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. A test for replicate --prune against an "empty except for orphaned managed ref" target is worth adding.

  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) If gitproto.LimitPackReader doesn't wrap in a double-close-safe reader, a 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. Splitting it into per-subcommand helpers is advisable.

  9. Replicate relay vs. --prune delete flow Adds a test covering replicate --prune end-to-end through the syncer.

Nice Touches

Suggested Follow-Ups (not blockers)

Test Coverage for Replicate

Gaps to Flag