can you put it in meaningful commits?

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
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:
- fast-forward / conflict planning in
PlanRef - relay eligibility rules in
CanIncrementalRelay - the materialized fallback path that computes object closure and repacks locally in
materialized.Execute
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
oldand the source hash asnew
- Fetch from source with no
haves, so source sends a full pack for the wanted refs instead of negotiating a minimal delta. The currenthave-based optimization is infetchPackV2and the incremental path passes target refs into it inincremental.Execute. - Push that pack directly to target.
- If you want exact mirroring, keep delete planning; otherwise drop
prunetoo.
That means you can likely remove or collapse:
- bootstrap special-casing in
Runand most ofbootstrap.Execute - incremental relay eligibility logic in
relay.go - ancestry traversal and block states in
planner.go - materialized fallback and its object-count guardrail in
materialized.go
In practice, the core engine could become:
discover sourcediscover targetplan create/update/delete by hash comparison onlyfetch full wanted pack from sourceforce-push commands to target
Two caveats remain:
- You still need target ref discovery, because
receive-packupdate commands need the currentoldhash. - 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?
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 inCanIncrementalRelay. If you stop sendinghaves, 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-thinbecause 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.
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?
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 mirroringreplicate: source-authoritative mirroring
I would scope replicate like this:
- Planning:
same hash=>skipmissing on target=>createdifferent on target=>update- no ancestry walk, no blocked state
- Fetch:
- still advertise target refs as
haves
- still advertise target refs as
- 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
- send normal update commands with target’s current hash as
- 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
- can just be an optimization inside
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
- current safe planner for
- let
replicatestill use the existing relay transport path inincremental.Execute, but relax the eligibility rules substantially - likely bypass most of
PlanReffor 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.
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 pruneremains optional- no blocked divergent updates
That gives a clean split:
sync: reconciliation with safety rulesreplicate: source-authoritative overwrite
CLI
Add:
git-sync replicate- optionally
git-sync plan-replicationor keepgit-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 replicategit-sync plan-replication
But if you want less command sprawl, this is better:
git-sync plan --mode sync|replicategit-sync syncgit-sync replicate
Public API
In pkg/gitsync/types.go, extend policy with an explicit mode:
Why this instead of a new request type:
syncandreplicateshare endpoints, scope, tags, prune, protocol, stats- the semantic difference belongs in policy
- stable API stays compact
Then:
Client.Synccan requireModeSyncor default empty toModeSync- add
Client.Replicate Planworks 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 stringonConfig
That lets Run branch early between:
- current
syncpath - new
replicatepath
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
createandupdate - allow tag
createandupdate - 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:
blockedshould always be0- 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|replicateTransferMode:bootstrap|incremental|materialized|bootstrap-batch
That is a worthwhile cleanup even without replicate.
Validation
Add validation rules:
replicaterejectsforcereplicateallowsprunereplicateallows tags and mappings exactly assyncdoes
Minimum Viable Implementation
Smallest coherent version:
- Add
Modeto policy/config. - Add
replicateCLI command. - Add replication planner with no block states.
- In
Run, branch on mode. - Reuse existing fetch-with-haves and pack relay.
- Handle deletes separately with
PushCommands. - Keep bootstrap optimization internal.
- Leave current
syncbehavior untouched.
That gets you the product semantics you want without rewriting transport internals.
What I Would Not Do
- I would not implement
replicateassync --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
RelayModeto 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
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-thininCanIncrementalRelay. 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.
PushPackrejects delete commands in a pack push. You can handle deletes separately, but until that path is fully shaped forreplicate, 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:
replicateuses 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
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
That makes sense. I’d define replicate as a strict relay-only mode.
So the contract becomes:
replicatenever materializes objects locallyreplicatenever 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 pushreplicate: source-authoritative, relay-only, no local object-store fallback
For implementation, that means:
- In
replicate, remove theexecuteMaterializedbranch fromRun. - 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 insteadreplicate 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-thinreplicate-missing-target-capabilitiesreplicate-unsupported-plan-kindreplicate-delete-with-pack-not-supportedif you do not split deletes outreplicate-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.
Implementation Plan
Add replicate as a first-class relay-only mode with a separate planner and execution branch. sync stays unchanged.
-
Add Operation Mode Types
- Update
pkg/gitsync/types.goto add:type OperationMode stringModeSyncModeReplicate
- Add
Mode OperationModetogitsync.SyncPolicy. - Default empty mode to
syncfor backward compatibility.
- Update
-
Bridge Mode Through Public and Unstable APIs
- Update
pkg/gitsync/client.gosobridgePolicyincludes mode. - Add
Client.Replicate(ctx, req SyncRequest)as a thin wrapper that setsPolicy.Mode = ModeReplicateand calls internal run. - Update
pkg/gitsync/internalbridge/model.goresult summaries to distinguish:- operation mode:
sync|replicate - transfer mode:
bootstrap|incremental|materialized|bootstrap-batch
- operation mode:
- Update
pkg/gitsync/unstable/client.goto pass mode through config and addReplicate.
- Update
-
Add CLI Command
- Update
cmd/git-sync/main.gowith:case "replicate": return runSyncLike(ctx, "replicate", args[1:], false, gitsync.ModeReplicate)
- Refactor
runSyncLiketo accept mode. - For
plan, either:- keep current behavior as
syncby default, plus--mode replicate, or - add
plan-replication.
- keep current behavior as
- I recommend
plan --mode sync|replicateto avoid another top-level command.
- Update
-
Add Internal Config Flag
- Update
internal/syncer/syncer.goConfigwith:Mode string
- Normalize empty mode to
syncin session setup or validation. - Add validation:
replicaterejectsForcereplicateignores or rejectsMaterializedMaxObjectsreplicateallowsPrune
- Update
-
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
- same hash =>
- Do not call
ReachesCommit.
- Keep current planner untouched for
-
Add Replication Relay Eligibility
- In
internal/planner/relay.go, add:CanReplicateRelay(...)ReplicateFallbackReason(...)or betterReplicateFailureReason(...)
- 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.
- In
-
Add Replicate Execution Path
- In
internal/syncer/syncer.go, splitRuninto: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
- In
-
Generalize Relay Execution as Needed
- Current
internal/strategy/incremental/incremental.gois shaped around sync semantics. - Either:
- add a new
internal/strategy/replicatepackage, or - rename/generalize incremental to accept replication rules.
- add a new
- I recommend a new
replicatestrategy 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
- fetch pack from source using
- Current
-
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 insteadreplicate does not support ref kind X; use sync insteadreplicate relay failed: <reason>; use sync instead
- Do not silently downgrade to sync behavior.
-
Result Model Cleanup
- Update result types so users can tell:
- product mode:
sync|replicate - transport mode:
bootstrap|incremental|bootstrap-batch|materialized
- product mode:
- In replicate:
Blockedmust always be0materializedmust never appear as transfer mode
- Tests
- Add planner tests:
- divergent branch becomes
update, notblock - divergent tag becomes
update, notblock - prune still works
- divergent branch becomes
- Add relay eligibility tests:
no-thintarget 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-thinfails and suggestssync - relay failure does not materialize
- delete+update replication works with split push behavior
- Add CLI tests in
cmd/git-sync/main_test.goforreplicateandplan --mode replicate.
- Docs
- Update
README.mdanddocs/architecture.md:- define
replicate - explain that it is relay-only
- explain failure behavior and when to use
syncinstead
- define
Recommended Implementation Order
- Add mode to API/config.
- Add replication planner.
- Add
runReplicate. - Add relay-only replicate strategy with delete splitting.
- Add CLI command.
- Add tests.
- Update docs.
One design choice to settle before coding
For planning UX, choose one:
git-sync plan --mode replicategit-sync plan-replication
I recommend plan --mode replicate. It keeps the surface smaller and matches the internal model better.
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
--modetorunSyncLikeincmd/git-sync/main.go - default it to
syncforplan - hardcode
replicatefor thereplicatecommand - 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
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, andinternal/syncer. - Added
git-sync replicateandgit-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
- relays create/update refs with target
- Split result reporting into
operation_modeandtransfer_mode. - Updated README and architecture docs.
Behaviorally:
synckeeps the old safety-first semantics.replicateis source-authoritative and relay-only.replicaterejects--force.replicatedoes not fall back to materialized push.
Verification:
- Ran
gofmt -won 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.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
rewriteReceivePackAdvertisementhelper — cleaner than the previous inline capability mutation. operationModeStringdefaulting empty →"sync"in both bridges is consistent.replicatecorrectly rejects--forceat session construction with a clear "use sync instead" message; error wrapping throughoutrunReplicateconsistently steers users back tosyncon failure.- Planner's
PlanReplicationRefreasons ("replicate overwrite"vs"replicate tag overwrite") are clear for debugging.
Suggested follow-ups (not blockers)
- CHANGELOG entry for
TransferModerename. - 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)?
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.modeas a deprecated alias alongsideexecution.transfer_modeinpkg/gitsync/internalbridge/model.go. - Simplified
defaultOperationModeincmd/git-sync/main.go. - Made
BuildReplicationPlansside-effect-free by cloning themanagedmap ininternal/planner/planner.go. - Removed the dead
dryRunparameter by splitting target capability checks from plan-shape checks: - Hardened the replicate bootstrap shortcut so
--prunewon’t incorrectly jump into bootstrap when managed orphan refs need deletion ininternal/syncer/syncer.go. - Added a short comment explaining the
closeOncewrapper ininternal/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
replicatefails and tells users to retry withsync.
Verification:
gofmt -won touched Go filesgo test ./...passed
I did not refactor the big CLI usage string yet. That still stands as cleanup, but it’s non-blocking.
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:
f377186Add relay-only replicate engine mode3486c1bExpose replicate mode in API and CLI
Worktree is clean.