can you put it in meaningful commits?

Codex·GPT-5.4·Soph·5mo ago·48min·2 Checkpoints·18 file changes·+936/-39·31.1M tokens

can you look at this project and tell me what we could do differently if:

  • target is read only, we don't need to care abou conflicts, we always overwrite
  • we don't care to much about sending to much data over the wire
5mo ago·5.4s

I’m looking through the repo to understand the current sync design and identify what changes would simplify it under your two assumptions: target is effectively overwrite-only, and network efficiency is not important.

I have the repo layout. Next I’m reading the architecture and the sync/planning paths to see where conflict handling and bandwidth minimization are driving complexity today.

The planner is where most of the safety logic lives today: fast-forward checks, blocked updates, prune semantics, and relay eligibility. I’m reading the strategy implementations now to separate “must have” behavior from complexity that only exists to preserve target history or save bandwidth.

I’ve got the main flow. The current design optimizes for safe incremental mirroring and minimizing transferred objects by advertising target state back to the source and only relaying when the update set is “safe enough.” I’m pulling the exact relay predicates now so I can point to the simplifications cleanly.

If the target is truly derived-only and you’re happy to overwrite it, the project can get much simpler.

The main simplification is to stop treating sync as “safe incremental mirroring” and treat it as “forceful replication.” Right now the complexity comes from three places:

Under your assumptions, I would change the model to this:

  • Always list source refs and target refs.
  • Build desired refs exactly as today.
  • For each desired ref:
    • if source hash == target hash, skip
    • else send an update command using the target’s current hash as old and the source hash as new
  • Fetch from source with no haves, so source sends a full pack for the wanted refs instead of negotiating a minimal delta. The current have-based optimization is in fetchPackV2 and the incremental path passes target refs into it in incremental.Execute.
  • Push that pack directly to target.
  • If you want exact mirroring, keep delete planning; otherwise drop prune too.

That means you can likely remove or collapse:

In practice, the core engine could become:

  • discover source
  • discover target
  • plan create/update/delete by hash comparison only
  • fetch full wanted pack from source
  • force-push commands to target

Two caveats remain:

  • You still need target ref discovery, because receive-pack update commands need the current old hash.
  • If the target has request/body size limits, bootstrap batching may still be useful even if bandwidth efficiency no longer matters. That part is about target acceptance, not conflict safety.

If you want, I can sketch what a “simple overwrite mode” refactor would look like in this repo, including which packages could disappear and what syncer.Run would reduce to.

Fetch from source with no haves, so source sends a full pack for the wanted refs instead of negotiating a minimal delta. The current have-based optimization is in fetchPackV2 ([/Users/soph/Work/entire/devenv/git-sync/internal/gitproto/fetch.go:173]) and the incremental path passes target refs into it in incremental.Execute ([/Users/soph/Work/entire/devenv/git-sync/internal/strategy/incremental/incremental.go:56]).

Why this?

5mo ago·14.7s

Because haves only help answer one question: “what objects can the source omit because the target probably already has them?”

If you do not care about wire size, omitting haves buys you three simplifications:

  • The pack becomes self-contained. Source sends the full object closure for the wanted refs, not “just the delta from target state.” That means the push does not depend on target already having the old base objects.

  • You decouple fetch from target history. Today the relay path uses target refs as haves so source can generate a smaller, target-dependent pack. That is why incremental relay is constrained to cases like fast-forward branch updates and tag creates in CanIncrementalRelay. If you stop sending haves, you no longer need to prove that kind of object overlap.

  • You can rely less on thin-pack assumptions. The current relay logic rejects targets with no-thin because negotiated packs may depend on objects already on the target. A full pack avoids that class of concern.

So the reason is not “we always overwrite, therefore no haves” by itself. The reason is:

  • overwrite-only means you do not need fast-forward safety
  • not caring about bandwidth means you do not need pack minimization
  • once both are true, a full source pack is the simplest thing to reason about

You would still need target ref discovery for the push commands’ expected old hash, but not for fetch negotiation.

One nuance: “no haves” does not mean “send the whole repository,” it means “send the full reachable closure for the refs you want.” If you want all mirrored refs, that may be most of the repo anyway.

if you fetch a ref without haves aren't you basially getting everything?

5mo ago·9.6s

Yes, for that ref’s reachable history, basically yes.

More precisely:

  • If you request a branch tip with no haves, the source has to send all objects reachable from that tip that are needed to reconstruct it.
  • For a normal branch, that usually means the full branch history, trees, and blobs reachable from the tip.
  • If you request multiple refs at once, you get the union of everything reachable from all of them, with shared objects deduplicated inside the pack.

So “no haves” is effectively:

  • not “the entire repository unconditionally”
  • but “everything needed to materialize the wanted refs from scratch”

That can still be most of the repository if:

  • you mirror the default branch
  • the branches share most history
  • tags point into old history you also want

This is exactly why the current design bothers with haves in fetchPackV2: it lets the source send only the part missing from target instead of a self-contained pack.

So if your assumption is “we don’t care too much about extra wire data,” no-have fetch is simpler, but it is not a small change in transfer volume. It can turn incremental sync from “latest few commits” into “full closure of mirrored refs every run.”

A reasonable middle ground would be:

  • drop fast-forward/conflict logic
  • still overwrite refs
  • keep sending haves so incremental runs stay cheap

That gives you most of the simplification without paying the full-transfer cost on every sync.

yeah, I like the later more.

Can we add this as another mode? replication instead of sync maybe?

5mo ago·6.5s

I’m checking the CLI surface so I can suggest where a replication mode would fit cleanly without colliding with the existing sync and bootstrap behavior.

Yes. A separate mode makes sense, and replicate is a better name than overloading sync.

sync today implies reconciliation with safety rules:

  • fast-forward checks
  • blocked divergent updates
  • relay only when target state makes it safe/cheap
  • materialized fallback when relay is not applicable

replicate would mean:

  • target is downstream only
  • source wins on every differing managed ref
  • overwrite is normal behavior, not an exceptional --force
  • keep haves during fetch so incremental runs stay efficient

That gives you a clean semantic split:

  • sync: conservative mirroring
  • replicate: source-authoritative mirroring

I would scope replicate like this:

  • Planning:
    • same hash => skip
    • missing on target => create
    • different on target => update
    • no ancestry walk, no blocked state
  • Fetch:
    • still advertise target refs as haves
  • Push:
    • send normal update commands with target’s current hash as old
    • no special “force mode” concept at the product level; overwrite is the mode’s default behavior
  • Prune:
    • optional, same as today if you want exact managed-ref replication
  • Bootstrap:
    • can just be an optimization inside replicate, not a separate mental model for users

Concretely in this repo, the main changes would be:

  • add a new command in cmd/git-sync/main.go
  • add a new request/mode field in the library API rather than abusing Force
  • split planning into:
    • current safe planner for sync
    • simpler overwrite planner for replicate
  • let replicate still use the existing relay transport path in incremental.Execute, but relax the eligibility rules substantially
  • likely bypass most of PlanRef for this mode

One subtle point: I would not implement replicate as “sync --force by another name.” --force is currently a per-run escape hatch inside a safety-first model. replicate is a different model, and the code should reflect that or it will stay conceptually muddy.

If you want, I can map out the exact API/CLI shape and the minimal code changes to add replicate without disturbing existing sync behavior.

yeah let's map out replicate

5mo ago·7.1s

I’m mapping the API boundary and result types so the replicate proposal fits the current stable/unstable split instead of being just a CLI alias.

Yes. I’d add replicate as a first-class mode, not an alias.

Shape

replicate should mean:

  • source is authoritative for managed refs
  • differing refs are overwritten by default
  • target refs are still advertised as haves during fetch
  • prune remains optional
  • no blocked divergent updates

That gives a clean split:

  • sync: reconciliation with safety rules
  • replicate: source-authoritative overwrite

CLI

Add:

  • git-sync replicate
  • optionally git-sync plan-replication or keep git-sync plan --mode replicate

I’d avoid overloading plan with a hidden mode unless you want a smaller surface. The most consistent pair is:

  • git-sync replicate
  • git-sync plan-replication

But if you want less command sprawl, this is better:

  • git-sync plan --mode sync|replicate
  • git-sync sync
  • git-sync replicate

Public API

In pkg/gitsync/types.go, extend policy with an explicit mode:

Why this instead of a new request type:

  • sync and replicate share endpoints, scope, tags, prune, protocol, stats
  • the semantic difference belongs in policy
  • stable API stays compact

Then:

  • Client.Sync can require ModeSync or default empty to ModeSync
  • add Client.Replicate
  • Plan works for either mode

For unstable API in pkg/gitsync/unstable/client.go:

  • reuse SyncRequest
  • add Policy.Mode
  • add Client.Replicate(ctx, req)

Internal Config

In internal/syncer/syncer.go, add:

  • Mode string on Config

That lets Run branch early between:

  • current sync path
  • new replicate path

Planner

This is the key design change.

Today PlanRef in internal/planner/planner.go can return block after ancestry checks. For replicate, add a second planner path:

  • BuildReplicationPlans(...)
  • PlanReplicationRef(...)

Rules:

  • same hash => skip
  • absent target => create
  • present and different => update
  • prune handling same as now
  • no ReachesCommit
  • no ActionBlock

That means replicate does not use:

  • ancestry traversal
  • depth-limit errors
  • force-gated tag retargeting

I would keep current planner untouched for sync and add a parallel overwrite planner, rather than threading mode through PlanRef and making it muddy.

Execution

replicate can still use the existing relay transport path, but with different eligibility rules.

Today incremental relay is intentionally narrow in internal/planner/relay.go. For replicate, add something like:

  • CanReplicationRelay(...)

Rules could be:

  • no dry-run
  • no delete actions in pack push
  • allow branch create and update
  • allow tag create and update
  • still reject if target advertises no-thin, unless you later decide to support full-pack replication
  • still pass target refs as haves

Then syncer.Run becomes:

  • build desired refs
  • if mode is replicate, build replication plans
  • split delete plans from create/update plans
  • relay create/update plans with haves
  • send delete plans via PushCommands

That last bit matters because PushPack currently rejects delete commands mixed into a pack push.

Bootstrap

I would not expose bootstrap as a separate concept in replicate.

Internally:

  • if all target refs are absent, reuse current bootstrap relay optimization
  • otherwise use replication planning and replication relay

So bootstrap remains a transport optimization, not the product model.

Force

In replicate, Force should be either:

  • invalid, or
  • ignored

I’d prefer invalid. In this mode, overwrite is already the default behavior, so force has no meaning.

Result Semantics

Existing result shape mostly works, but wording should improve.

Today Blocked exists in results via internalbridge/model.go. In replicate:

  • blocked should always be 0
  • execution summary should surface mode as product mode, not only relay mode

Right now ExecutionSummary.Mode is really relay mode (bootstrap, incremental, etc.). That’s ambiguous once you add replicate.

I’d split it into:

  • OperationMode: sync|replicate
  • TransferMode: bootstrap|incremental|materialized|bootstrap-batch

That is a worthwhile cleanup even without replicate.

Validation

Add validation rules:

  • replicate rejects force
  • replicate allows prune
  • replicate allows tags and mappings exactly as sync does

Minimum Viable Implementation

Smallest coherent version:

  1. Add Mode to policy/config.
  2. Add replicate CLI command.
  3. Add replication planner with no block states.
  4. In Run, branch on mode.
  5. Reuse existing fetch-with-haves and pack relay.
  6. Handle deletes separately with PushCommands.
  7. Keep bootstrap optimization internal.
  8. Leave current sync behavior untouched.

That gets you the product semantics you want without rewriting transport internals.

What I Would Not Do

  • I would not implement replicate as sync --force.
  • I would not remove materialized fallback immediately. Keep it as a fallback if relay eligibility still fails for transport reasons.
  • I would not overload existing RelayMode to mean operation mode.

If you want, the next step can be a concrete implementation plan by file: exact structs, methods, and branch points to add in this repo.

  • I would not remove materialized fallback immediately. Keep it as a fallback if relay eligibility still fails for transport reasons.

What would that be

5mo ago·10.4s

Mostly cases where overwrite semantics are fine, but direct pack relay still is not.

Examples:

  • Target rejects thin-pack style relay. The current relay gate checks no-thin in CanIncrementalRelay. If the target requires a self-contained push pack, source-to-target relay with target-dependent omission can stop being safe/applicable.

  • Mixed operations where pack relay cannot express the whole update cleanly. PushPack rejects delete commands in a pack push. You can handle deletes separately, but until that path is fully shaped for replicate, materialized fallback is a safe backstop.

  • Relay eligibility remains intentionally narrower than “all overwrites.” Even after adding replicate, you may choose to keep relay limited to branch/tag create-update cases and fall back for odd mappings or future ref kinds.

  • Source/target protocol quirks. If source fetch or target receive-pack behavior makes streamed relay fail, local materialization gives you a more Git-like “fetch objects locally, then encode a fresh push pack” recovery path.

  • Retry/diagnostic value. When a relay attempt fails mid-flight, materialized fallback can keep the operation succeeding while you learn which targets can actually support the pure relay path reliably.

So the point is: replicate removes planning conflicts, not all transport constraints.

If you want the likely end state, I’d expect:

  • replicate uses relay in the common path
  • deletes are handled as separate ref-only pushes
  • materialized fallback becomes rare
  • once tests show relay covers the real targets you care about, then you can decide whether materialized is still worth keeping
  1. the targets I want to handle support thin, like this is a target question?
5mo ago·9.5s

Yes. That specific concern is target-side.

The issue is not “can the source produce a thin pack?” The source usually can. The question is whether the target’s receive-pack is willing to accept a pack that omits objects because the target is expected to already have them.

That is why the current code inspects target capabilities in TargetFeaturesFromAdvRefs and blocks relay when the target advertises no-thin in CanIncrementalRelay.

So if your targets support thin packs, that particular reason for keeping materialized fallback gets much weaker.

Under your assumptions, the remaining reasons to keep it temporarily are mostly practical:

  • you may still want a fallback while rolling out replicate
  • relay currently does not handle delete commands in the same pack push
  • there may be implementation gaps before the replicate relay path is fully generalized

But conceptually, yes: thin here is mainly a target acceptance question.

I want replicate to not use materialize at all, that should be the gooal. If the target rejects thin or does not support it? fail. Basically all that can be handled by sync should fail replicate and suggest using sync instead

5mo ago·12.3s

That makes sense. I’d define replicate as a strict relay-only mode.

So the contract becomes:

  • replicate never materializes objects locally
  • replicate never falls back
  • if relay is not possible, it fails
  • the error should say why and point the caller to sync

That gives you a much cleaner product boundary:

  • sync: general-purpose, safety-first, can fall back to materialized push
  • replicate: source-authoritative, relay-only, no local object-store fallback

For implementation, that means:

  • In replicate, remove the executeMaterialized branch from Run.
  • Add a replication-specific relay gate, separate from CanIncrementalRelay.
  • If that gate fails, return an error like:
    • replicate requires relay-capable target; target advertises no-thin; use sync instead
    • replicate does not support local materialized fallback; use sync instead

I would make the failure reasons explicit and machine-readable where possible. Good candidates:

  • replicate-target-no-thin
  • replicate-missing-target-capabilities
  • replicate-unsupported-plan-kind
  • replicate-delete-with-pack-not-supported if you do not split deletes out
  • replicate-relay-failed

The design implication is important: replicate should have its own execution path, not be bolted onto sync with a flag, because the fallback behavior is fundamentally different.

A clean rule set would be:

  • sync
    • may relay
    • may materialize
    • may block divergent refs unless forced
  • replicate
    • overwrite planner only
    • relay only
    • no materialize
    • no blocked refs
    • fail on any transport/path limitation and suggest sync

That is a solid product definition. If you want, I can turn this into a concrete file-by-file implementation plan next.

yes, do this

5mo ago·34.4s

Implementation Plan

Add replicate as a first-class relay-only mode with a separate planner and execution branch. sync stays unchanged.

  1. Add Operation Mode Types

    • Update pkg/gitsync/types.go to add:
      • type OperationMode string
      • ModeSync
      • ModeReplicate
    • Add Mode OperationMode to gitsync.SyncPolicy.
    • Default empty mode to sync for backward compatibility.
  2. Bridge Mode Through Public and Unstable APIs

    • Update pkg/gitsync/client.go so bridgePolicy includes mode.
    • Add Client.Replicate(ctx, req SyncRequest) as a thin wrapper that sets Policy.Mode = ModeReplicate and calls internal run.
    • Update pkg/gitsync/internalbridge/model.go result summaries to distinguish:
      • operation mode: sync|replicate
      • transfer mode: bootstrap|incremental|materialized|bootstrap-batch
    • Update pkg/gitsync/unstable/client.go to pass mode through config and add Replicate.
  3. Add CLI Command

    • Update cmd/git-sync/main.go with:
      • case "replicate": return runSyncLike(ctx, "replicate", args[1:], false, gitsync.ModeReplicate)
    • Refactor runSyncLike to accept mode.
    • For plan, either:
      • keep current behavior as sync by default, plus --mode replicate, or
      • add plan-replication.
    • I recommend plan --mode sync|replicate to avoid another top-level command.
  4. Add Internal Config Flag

    • Update internal/syncer/syncer.go Config with:
      • Mode string
    • Normalize empty mode to sync in session setup or validation.
    • Add validation:
      • replicate rejects Force
      • replicate ignores or rejects MaterializedMaxObjects
      • replicate allows Prune
  5. Add Replication Planner

    • Keep current planner untouched for sync.
    • Add new functions in internal/planner/planner.go:
      • BuildReplicationPlans(...)
      • PlanReplicationRef(...)
    • Rules:
      • same hash => ActionSkip
      • target missing => ActionCreate
      • target different => ActionUpdate
      • prune => ActionDelete
      • never return ActionBlock
    • Do not call ReachesCommit.
  6. Add Replication Relay Eligibility

    • In internal/planner/relay.go, add:
      • CanReplicateRelay(...)
      • ReplicateFallbackReason(...) or better ReplicateFailureReason(...)
    • Rules:
      • fail on dry-run only in execution, not planning
      • require known target capabilities
      • fail if target advertises no-thin
      • allow branch create/update
      • allow tag create/update
      • reject unsupported ref kinds
    • This is not a fallback reason in product terms; it is an execution eligibility reason.
  7. Add Replicate Execution Path

    • In internal/syncer/syncer.go, split Run into:
      • runSync(...)
      • runReplicate(...)
    • runReplicate(...) flow:
      • discover source/target
      • build desired refs
      • if bootstrap eligible, reuse bootstrap relay optimization
      • build replication plans
      • if dry-run, return plan result
      • partition plans into:
        • create/update plans
        • delete plans
      • validate relay eligibility for create/update plans
      • if not eligible: fail with “use sync instead”
      • relay create/update plans with target refs as haves
      • apply deletes via PushCommands
      • never call executeMaterialized
  8. Generalize Relay Execution as Needed

    • Current internal/strategy/incremental/incremental.go is shaped around sync semantics.
    • Either:
      • add a new internal/strategy/replicate package, or
      • rename/generalize incremental to accept replication rules.
    • I recommend a new replicate strategy package to keep semantics clean.
    • Behavior:
      • fetch pack from source using haves from target refs
      • push create/update commands via PushPack
      • if delete plans exist, push them separately with PushCommands
  9. Fail Hard Instead of Falling Back

    • In replicate mode, any relay ineligibility or relay failure returns an error.
    • Error text should be explicit, for example:
      • replicate requires relay-capable target: target advertises no-thin; use sync instead
      • replicate does not support ref kind X; use sync instead
      • replicate relay failed: <reason>; use sync instead
    • Do not silently downgrade to sync behavior.
  10. Result Model Cleanup

  • Update result types so users can tell:
    • product mode: sync|replicate
    • transport mode: bootstrap|incremental|bootstrap-batch|materialized
  • In replicate:
    • Blocked must always be 0
    • materialized must never appear as transfer mode
  1. Tests
  • Add planner tests:
    • divergent branch becomes update, not block
    • divergent tag becomes update, not block
    • prune still works
  • Add relay eligibility tests:
    • no-thin target fails replicate
    • create/update branch/tag allowed
    • unsupported plan kinds fail
  • Add integration tests in internal/syncer/integration_test.go:
    • empty target replicate uses bootstrap relay
    • divergent target branch is overwritten
    • divergent tag is overwritten
    • target with no-thin fails and suggests sync
    • relay failure does not materialize
    • delete+update replication works with split push behavior
  • Add CLI tests in cmd/git-sync/main_test.go for replicate and plan --mode replicate.
  1. Docs
  • Update README.md and docs/architecture.md:
    • define replicate
    • explain that it is relay-only
    • explain failure behavior and when to use sync instead

Recommended Implementation Order

  1. Add mode to API/config.
  2. Add replication planner.
  3. Add runReplicate.
  4. Add relay-only replicate strategy with delete splitting.
  5. Add CLI command.
  6. Add tests.
  7. Update docs.

One design choice to settle before coding

For planning UX, choose one:

  • git-sync plan --mode replicate
  • git-sync plan-replication

I recommend plan --mode replicate. It keeps the surface smaller and matches the internal model better.

yes plan --mode replicate

5mo ago·8.4s

That’s the right choice.

plan --mode replicate keeps planning as one concept and makes the operation mode explicit without adding another command family.

So the CLI shape becomes:

  • git-sync sync ...
  • git-sync replicate ...
  • git-sync plan --mode sync ...
  • git-sync plan --mode replicate ...

Implementation-wise, that means:

  • add --mode to runSyncLike in cmd/git-sync/main.go
  • default it to sync for plan
  • hardcode replicate for the replicate command
  • pass mode through SyncPolicy

If you want, I can start implementing this now.

yes, start implementing and keep reviewing after each phase, keep going until the end only stop if you need feedback from me

5mo ago·2.8s

I’m implementing replicate as a relay-only mode with plan --mode replicate, starting at the API/config boundary so the semantics are explicit all the way through instead of hidden in CLI flags.

I’m making the mode explicit through the stable API, unstable API, bridge layer, and syncer config first. That keeps the later execution split isolated instead of threading ad hoc flags through the planner.

The mode plumbing is in. I’m checking compile impact now before I split planner/execution; this phase should settle the API boundary and result shape cleanly.

The boundary changes compile cleanly. Next I’m splitting planning and execution so replicate can overwrite divergent refs, stay relay-only, and fail instead of falling back.

I’m defining replicate as relay-only in code now: separate overwrite planning, separate relay gate, and an explicit failure path instead of falling into materialized push.

I have the execution split in place, but there’s one piece I’m tightening now: the early target-capability check should be explicit, not inferred through a synthetic plan. After that I’ll wire the CLI and then drive failures through tests.

The relay-only path compiles. I’m wiring the user surface now: replicate as a command and plan --mode replicate, then I’ll fill in behavior tests around overwrite and relay-only failure.

The CLI/API surface is wired. I’m adding the actual behavior tests now: overwrite planning, relay-only rejection, and the new CLI mode selection.

I’ve added behavior tests around the new mode. Running targeted suites now to catch semantic mismatches before I touch docs and broader verification.

The test targets are advertising no-thin by default, so replicate is correctly failing in tests. I’m adjusting the local test servers to model both cases explicitly: a thin-capable target for normal replication tests and a no-thin target for the failure case.

The capability rewrite still isn’t taking effect, so I’m making the test server fail loudly on rewrite errors instead of silently serving the original advertisement. That will tell me whether the issue is parsing or capability handling.

The in-memory Smart HTTP test server is not a good stand-in for a thin-capable target, so I’m moving the positive overwrite coverage down one level: unit-testing the replicate relay path directly, while keeping the no-thin integration failure at the syncer level. That preserves the product contract without baking in a misleading fake target.

The mode behavior is covered now. I’m doing the cleanup pass next: gofmt, broader test runs, and docs so the new semantics are visible from the CLI and architecture docs.

I’ve finished the code and docs changes. Final pass is a broader test run across the repo to catch any shape changes outside the targeted suites.

Implemented replicate end-to-end as a first-class relay-only mode.

Main changes:

  • Added explicit operation mode plumbing through pkg/gitsync, pkg/gitsync/unstable, the bridge layer, and internal/syncer.
  • Added git-sync replicate and git-sync plan --mode replicate.
  • Added overwrite-oriented replication planning that never blocks divergent refs.
  • Added a dedicated relay-only replicate strategy that:
    • relays create/update refs with target haves
    • sends deletes separately as ref-only commands
    • never materializes locally
    • fails with “use sync instead” when the target is not relay-capable, including no-thin
  • Split result reporting into operation_mode and transfer_mode.
  • Updated README and architecture docs.

Behaviorally:

  • sync keeps the old safety-first semantics.
  • replicate is source-authoritative and relay-only.
  • replicate rejects --force.
  • replicate does not fall back to materialized push.

Verification:

  • Ran gofmt -w on modified Go files.
  • Ran go test ./... successfully.

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.modeexecution.transfer_mode (pkg/gitsync/internalbridge/model.go:98,145) ExecutionSummary.ModeTransferMode 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)?

5mo ago·8.9s

I’m taking the review points as action items and applying the mechanical fixes now. I’ll also preserve the old execution.mode field as a deprecated alias and add the missing replicate --prune coverage so the branch is safer to merge.

I’m cleaning up the internal APIs now: removing the dead dryRun parameter, making replication planning side-effect-free, preserving the old JSON field as an alias, and tightening the bootstrap shortcut so replicate --prune can’t skip orphan deletions.

I’ve fixed the API and planner cleanup items. I’m adding one targeted syncer test for the replicate --prune bootstrap edge case now, plus a brief doc note about the “retry with sync” contract.

There’s one test cleanup left from the relay API change: an unused plans variable in the planner tests. I’m fixing that and then rerunning the focused suite plus the full repo.

The review fixes are in and the focused suites are clean. I’m running the full repo one last time so the branch lands with the compatibility alias and the prune/bootstrap hardening verified.

Applied the review fixes.

What changed:

  • Preserved backward compatibility in the stable JSON surface by restoring execution.mode as a deprecated alias alongside execution.transfer_mode in pkg/gitsync/internalbridge/model.go.
  • Simplified defaultOperationMode in cmd/git-sync/main.go.
  • Made BuildReplicationPlans side-effect-free by cloning the managed map in internal/planner/planner.go.
  • Removed the dead dryRun parameter by splitting target capability checks from plan-shape checks:
  • Hardened the replicate bootstrap shortcut so --prune won’t incorrectly jump into bootstrap when managed orphan refs need deletion in internal/syncer/syncer.go.
  • Added a short comment explaining the closeOnce wrapper in internal/strategy/replicate/replicate.go.
  • Added the missing coverage:
    • plan/relay tests in planner
    • prune/bootstrap edge test in syncer
    • existing replicate strategy test already covers split update/delete flow
  • Added a README note that replicate fails and tells users to retry with sync.

Verification:

  • gofmt -w on touched Go files
  • go test ./... passed

I did not refactor the big CLI usage string yet. That still stands as cleanup, but it’s non-blocking.

can you put it in meaningful commits?

5mo ago·5.4s

I’m grouping the work into a few coherent commits: API/mode plumbing, relay-only replicate execution, and follow-up compatibility/test/docs fixes. First I’m checking the current diff and staging boundaries so the commits stay meaningful.

I can split this cleanly into two substantive commits: engine support for relay-only replication, then public/API/CLI exposure plus compatibility/docs. I’m staging the engine layer first so each commit has a clear boundary.

The engine commit is in. I’m staging the API/CLI/docs layer now, including the compatibility alias for execution.mode and the new plan --mode replicate surface.

Committed in two chunks:

  • f377186 Add relay-only replicate engine mode
  • 3486c1b Expose replicate mode in API and CLI

Worktree is clean.