# 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](https://git-scm.com/docs/gitprotocol-pack)) 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.