Summary of Issue #46 and Related Discussions

I've read through issue #46. Quick summary of how I'm reading it:

• The core ask is to use receive-pack's atomic capability so a single push is all-or-nothing.
• The honest finding in the issue itself: atomic only helps within one push. The real non-atomicity comes from the multi-phase shapes — batched bootstrap (branches → tail) and replicate (relay push → separate delete PushCommands).
• The two phase-shape issues each have a different answer: replicate can fold deletes into the same push, but batched bootstrap is multi-transaction by design.
• And atomic + BestEffort cancel each other out — they need an explicit interaction rule.

A few things I'd want to chat about before touching code:

1. Scope. Is the goal here just "negotiate and set atomic where pushes are already one-shot" (cheap, partial win), or also "restructure replicate to one push" (more invasive, actually closes the gap)? They're separable.
2. BestEffort policy. Your sketch suggests "skip atomic when BestEffort is set." That seems right to me — silent skip with a debug log, error only if a user explicitly asks for both. Agree?
3. Batched bootstrap. Are you okay leaving that as documented-multi-transaction, or is there appetite to make the tail push optional / fold it into the last batch when small?

What's your priority on this — quick win first, or do you want to design the replicate change too?

Further Clarifications

Got it — that reframes #46 nicely. Reading the two together:

• #47 is the safety net. Every PushCommand already carries Old: TargetHash captured at session start, so receive-pack rejects any update where the target moved underneath us. That's the lease.
• #46 atomic is for "this single push is naturally all-or-nothing", not "make the whole sync run atomic."

Scope Reiteration

So the scope you're describing is:

1. Negotiate atomic and set it on the pushes that are already one transaction: one-shot bootstrap, sync relay, materialized fallback, sync+prune.
2. For batched bootstrap: set atomic on each batch individually. We assume no concurrent target changes for the initial flow; if that assumption breaks, the next batch's lease check (#47) catches it because its Old won't match anymore.
3. Don't restructure replicate's two-push shape. Atomic still applies to the relay push; the delete push remains its own transaction.
4. BestEffort + atomic: skip atomic when BestEffort is set; error only if a user explicitly passes both.

Two things to nail down before I touch code:

• Default on or opt-in? My read: on by default when the server advertises the capability — it's strictly safer and matches git push --mirror parity. Any reason to gate it behind a flag?
• Per-batch atomic in batched bootstrap — confirming you want this. It adds nothing for the cross-batch story, but it does mean a single bad ref in batch N doesn't half-land that batch.

Operation Dynamics

How would that work with something like GitHub pull refs that should be warn skipped now if fail to create?

Pull refs today are handled one of two ways:

1. --exclude-ref-prefix refs/pull/ — they never enter the push at all.
2. --all-refs which bundles BestEffort — they hit the push, server returns ng, BestEffort downgrades each one to a warning.

Atomic Conflicts Resolution

Atomic conflicts only with (2). If BestEffort is on, even one ng from a pull ref under atomic fails the whole transaction → no refs land → the user gets nothing instead of "most things plus warnings." That defeats the BestEffort UX.

Resolution: atomic and BestEffort are mutually exclusive. Concretely:

• BestEffort on → never set atomic, regardless of server capability. Per-ref ng continues to come back individually, warn-skip works.
• BestEffort off → atomic safe to negotiate. If a pull ref (or anything else) gets ng, the user already wants the whole sync to fail; atomic just makes it fail cleanly with nothing partially landed.

Future Work Consideration

Want me to write that scope clarification into the issue, or leave it and just implement?

Final Thoughts: I also tried to run go test ./internal/gitproto ./internal/syncer ./cmd/git-sync, but the sandbox blocks httptest from binding a local port, so I couldn’t use test execution to validate these paths here.

Proposed New Body for Issue:

Status

git-sync doesn't negotiate or set the receive-pack atomic capability today. Logging this so we don't lose track of the option.

Background

The atomic capability ( gitprotocol-pack.adoc) lets a client signal that all ref commands in one update-request apply or none do. It's what git push --atomic sets — not what git push --mirror sets. Default --mirror is per-ref: each command gets its own ok/ng, partial application is normal.

Current Behavior Comparison

Where git-sync stands relative to git push:

• Default git push --mirror → per-ref outcomes, partial apply. git-sync today matches this.
• git push --mirror --atomic → all-or-nothing within one push. git-sync has no equivalent.

Foundational Reasons for Not Building it

1. No demand yet. No one has asked for transactional pushes; the per-ref model has been fine.
2. Conflicts with BestEffort.--all-refs on sync/plan bundles BestEffort, which downgrades per-ref ng to warnings — atomic would defeat that, so atomic would have to be off for the most common GitHub-mirror config anyway.
3. Phase shape, not capability, is the real gap. Where git-sync is non-atomic across phases (batched bootstrap tail, replicate's separate delete push), the atomic capability alone doesn't help — those are multi-transaction by design.

If We Revisit

• Add an explicit --atomic flag (opt-in, matching git's UX).
• Negotiate via gitproto.TargetFeaturesFromAdvRefs, set on packp.UpdateRequests in buildUpdateRequest.
• Mutually exclusive with BestEffort: error if both are set explicitly.
• Per-batch only in batched bootstrap; the cross-batch story stays the lease check from #47.