do we have integration tests? · Entire

Home

Log in

does git-sync handle any ref or just branches

2mo ago·30.6s·8 calls

I have a clear answer.

Short answer: branches and tags only.

git-sync rejects refs outside refs/heads/ and refs/tags/. The validator at internal/validation/validation.go:184-193 returns an empty kind for anything that isn't IsBranch() or IsTag(), and NormalizeMapping errors out with unsupported source/target ref kind (validation.go:139-144).

Concretely:

how does git --mirror handle this?

2mo ago·14.3s

git clone --mirror / git push --mirror is namespace-agnostic. It sets remote.<name>.mirror = true with the refspec +refs/*:refs/*, so it copies everything under refs/ 1:1:

The leading + forces non-fast-forwards, and push --mirror also deletes refs on the target that don't exist on the source — equivalent to git-sync's --force plus --prune, but applied to the whole ref namespace rather than just branches/tags.

So git-sync's scope is a deliberate subset: --mirror is "copy the entire ref graph verbatim," whereas git-sync only manages refs/heads/* and (opt-in) refs/tags/* and validates everything else away.

but it should be straight forward to add, right?

2mo ago·19.7s

Mechanically yes — the gate is one function (refKind in internal/validation/validation.go:184) plus loosening the RefKind enum and the few code paths that branch on it. The wire protocol doesn't care about namespaces; it's mostly a validation/policy question.

The real tradeoff is semantic, not technical: branches have fast-forward + force semantics, tags have retarget-vs-immutable rules, but refs/notes/*, refs/pull/*, refs/changes/* etc. each have their own conventions (notes are commits with merge-style updates; pull/MR refs are read-only on most hosts; Gerrit changes are immutable). So the choice is either a permissive --mirror-style "copy whatever, treat unknowns like branches" mode, or a more careful "opt-in per namespace with appropriate semantics" design — the second is the one that fits git-sync's current style but is more work than just relaxing the validator.

but the distinction only matters for consequtive syncs, right? the inital sync (empty target) could just work?

2mo ago·18.0s

Right — for an empty target, every ref is a create, so fast-forward/force/retarget/immutability questions don't arise; the protocol just sends 0000…→<hash> <refname> for each, and the target either accepts it or doesn't. That maps cleanly onto git-sync's existing bootstrap relay path, which already streams the source pack into receive-pack without interpreting ref kinds.

Two caveats worth flagging: (1) acceptance is still a target-server policy — pushing refs/pull/* to GitHub will be refused regardless of how you frame it, but a bare repo or your own host takes anything; (2) the second sync is where you'd have to commit to per-namespace semantics, so shipping "mirror-on-bootstrap only" is a real feature but it's a one-way door unless you're willing to error on subsequent syncs that touch non-branch/non-tag refs.

how does a mirror push to github does this btw? I think I tried that and it did not error, but how did it handle the pull refs?

2mo ago·32.5s

Honest answer: I'm not 100% confident on GitHub's exact behavior here and don't want to guess at specifics. The architectural picture is that refs/pull/* are server-managed "hidden refs" — GitHub typically advertises them on fetch (so a clone --mirror does pick them up) but rejects writes to them on receive, usually with a per-ref deny updating a hidden ref message rather than failing the whole push. Per-ref rejections in the receive-pack response don't always make git push exit non-zero in a way that's visually obvious, especially among hundreds of OK refs scrolling by, so "it didn't error" is consistent with "the pull-ref updates were silently rejected and the branch/tag updates went through."

Easy way to verify on your repo: git push --mirror 2>&1 | grep -E 'rejected|hidden' — if any pull refs got refused, they'll show up there even if echo $? looks fine.

I'd suggest we add an "--all-refs" flag and the default behaviour is "warn" when we can't set/update it.

2mo ago·20.8s

Reasonable shape. Two things to nail down before committing: (1) "warn" needs to be a first-class result, not just a log line — RefResult should grow a WarnRejected action (or similar) so JSON consumers and exit codes can see per-ref outcomes, otherwise users discover failures the same way GitHub mirror-push hides them today; (2) decide whether --all-refs implies warn-on-rejection or whether they're orthogonal — I'd keep them separate (--all-refs for scope, plus a per-ref failure policy that defaults to warn only when --all-refs is set) so the existing branches+tags flow keeps its loud failures for refs the user explicitly named.

The other thing worth being deliberate about: warn-on-rejection only makes a difference on non-empty targets, so the bootstrap-only path you sketched earlier composes cleanly with this — --all-refs on first sync just works, and the warn machinery only earns its keep on subsequent syncs into hostile targets like GitHub.

1: yes, 2: yes, basically "--all-refs" is best effort, specifying --tags means "I want all tags 100%"

--all-refs on first sync wouldn't work for pull refs when target and source is GitHub (or a flavour of GitHub)

2mo ago·16.2s

Correct — I was sloppy on that. Hidden-ref rejection is a server policy, not a repo-state property, so GitHub→GitHub mirroring rejects refs/pull/* even into an empty target. That tightens the design: warn-on-reject isn't a "subsequent sync" concern that bootstrap-only mode lets you punt on — it's load-bearing on day one whenever the target is GitHub-flavored, which means --all-refs and the warn machinery genuinely ship together rather than in phases.

Net: --all-refs = best-effort scope + permissive failure mode, explicit --tags / named branches = strict. Clean split.

ok, let's go, make a new branch, commit meaningful on the go

2mo ago·2h 50m·243 calls

All four commits land cleanly on soph/all-refs. Summary:

07d05d5 Wire --all-refs into the CLI surface
0990568 Add BestEffort policy and ActionWarn for per-ref reject downgrades
64421e4 Push other-kind refs through bootstrap and add end-to-end coverage
6814963 Add RefScope.AllRefs to broaden mirroring beyond branches and tags

What's working end-to-end:

Tests added: validation unit (NormalizeMappingAllowOther), planner unit (BuildDesiredRefsAllRefs), syncer integration (AllRefsBootstrapsCustomNamespace, AllRefsBestEffortDowngradesNgToWarn, AllRefsRejectsCustomMappingWithoutAllRefs).

Want me to push the branch and open a PR, or hold off?

64421e4Push other-kind refs through bootstrap and add end-to-end coverage\ \ Batched bootstrap previously errored on anything that wasn't a branch or\ create-only tag. Other-kind refs (refs/notes/, refs/pull/, custom\ namespaces enabled by AllRefs) need the same create-only path: rename\ the tag-phase to a tail phase and route both tags and other-kind plans\ through it. Tags still request include-tag in the fetch (IsTag=true);\ other-kind refs fetch by exact hash.\ \ The integration test seeds refs/notes/commits on a source with two\ commits, runs an AllRefs sync into an empty target, and asserts the\ notes ref shows up at the expected hash. A second test pins the\ validator gate: mapping refs/notes/* without AllRefs errors out.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: ccbb25706390+89/-18

0990568Add BestEffort policy and ActionWarn for per-ref reject downgrades\ \ The receive-pack response is per-ref: the server can accept some refs\ and reject others in the same push (GitHub's hidden-ref refusals are\ the load-bearing case). Previously sendReceivePack treated any ng as a\ fatal error via report.Error(); this commit threads an OnRejection\ callback down through Pusher and the free Push* functions so callers\ can opt into receiving per-ref ng statuses without short-circuiting the\ push. Pack-level unpack failures stay fatal.\ \ SyncPolicy.BestEffort wires that callback up at session construction.\ The session collects rejections in a map; after each strategy returns,\ applyRejections walks the plans and downgrades matching entries to a\ new ActionWarn with the server's reason in plan.Reason. Result.Warned\ counts the downgrades, complementing Pushed/Skipped/Blocked/Deleted,\ and the human-readable summary line surfaces it.\ \ The integration test wires a target receive-pack hook that returns\ "deny updating a hidden ref" for refs/notes/commits while accepting\ the branch ref, runs with AllRefs+BestEffort, and asserts the notes\ plan ends up as ActionWarn with the reason carried through.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: e5feb8b3d187+207/-21

07d05d5Wire --all-refs into the CLI surface\ \ Adds --all-refs to sync, plan, replicate, bootstrap, fetch, and probe.\ For sync/plan/replicate/bootstrap the flag bundles AllRefs scope with\ BestEffort policy: the broader ref discovery only earns its keep when\ hidden-ref refusals downgrade to warnings instead of tanking the run.\ Probe and fetch get AllRefs as scope-only (neither pushes), with a\ tailored flag description so the help text doesn't lie about\ "warnings".\ \ The unstable BootstrapRequest grows a BestEffort field so the CLI's\ RunE can mirror the sync-side coupling. Library callers keep the two\ halves decoupled via RefScope.AllRefs and SyncPolicy.BestEffort.\ \ docs/usage.md documents the --all-refs example, the strict/best-effort\ split, and adds "warned" to the JSON output keys list.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 011ba13b7e3d+48/-1

do we have integration tests?

2mo ago·20.1s

Three of them, all in internal/syncer/integration_test.go:

Gaps worth flagging:

Want me to add the incremental + materialized coverage before pushing?

yes, and can we think about a smoke test

2mo ago·2m·9 calls

Five commits now. Coverage status:

Path Test
Validator gate (strict default) TestRun_IntegrationAllRefsRejectsCustomMappingWithoutAllRefs
Bootstrap one-shot + AllRefs TestRun_IntegrationAllRefsBootstrapsCustomNamespace
Materialized fallback + AllRefs TestRun_IntegrationAllRefsMaterializedPathIntoExistingTarget
BestEffort downgrade on ng TestRun_IntegrationAllRefsBestEffortDowngradesNgToWarn
Full CLI pipeline TestRun_Sync_AllRefsSmokeTest

Honest gaps left:

Want me to add any of these, or call it good and push?

ebfdd77Cover materialized fallback and add a CLI smoke test for --all-refs\ \ The two existing AllRefs integration tests both hit bootstrap (empty\ target). For non-empty targets, RefKindOther refs fail CanIncrementalRelay\ by design and fall through to the materialized executor — a code path\ that's kind-agnostic on push but had no end-to-end coverage with\ AllRefs. The new test pre-populates the target with the branch, adds a\ refs/notes/commits ref on source, and asserts: result.Pushed=1 (notes\ create only, branch is a skip), result.Relay=false (materialized,\ not relay), and the notes ref lands on target at the right hash.\ \ The CLI smoke test goes through cmd/git-sync's cobra entry via run(...)\ with --all-refs --json, then walks the JSON for the notes ref entry to\ confirm kind=other and action=create. This pins the full pipeline\ (flag parsing → unstable client → bridge → syncer → receive-pack)\ against silent regressions where individual layers compile but the\ wiring breaks.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 63e589614284+139

yeah, let's close the gaps

2mo ago·2m·6 calls

Six commits, full coverage matrix:

Path Test
Validator gate (strict default) TestRun_IntegrationAllRefsRejectsCustomMappingWithoutAllRefs
Bootstrap one-shot + AllRefs TestRun_IntegrationAllRefsBootstrapsCustomNamespace
Bootstrap batched + AllRefs TestBootstrap_IntegrationAllRefsBatchedTailPhase
Incremental relay + AllRefs (branch-only push) TestRun_IntegrationAllRefsIncrementalRelayWithBranchOnlyPush
Materialized fallback + AllRefs TestRun_IntegrationAllRefsMaterializedPathIntoExistingTarget
BestEffort downgrade on ng TestRun_IntegrationAllRefsBestEffortDowngradesNgToWarn
Replicate + AllRefs Other-kind (rejected) TestRun_IntegrationAllRefsReplicateRejectsOtherKindIntoExistingTarget
Full CLI pipeline TestRun_Sync_AllRefsSmokeTest

The replicate-rejects test pins a deliberate v1 limitation (replicate doesn't fall back to materialized, so Other-kind refs into a non-empty target error out) — if you'd rather have replicate also support Other-kind, that's a follow-up that relaxes CanReplicateRelay. Ready to push?

b79fc08Pin remaining --all-refs paths: incremental relay, batched bootstrap, replicate\ \ Three tests cover the paths that were left implicit:\ - IncrementalRelayWithBranchOnlyPush: source has a notes ref already\ current on target, branch needs an update. AllRefs broadens the ref\ discovery, but pushPlans is branch-only so CanIncrementalRelay\ accepts. Asserts the relay path engages (RelayMode=incremental)\ rather than getting tripped up by the broader scope.\ - BatchedTailPhase: large-blob source forces TargetMaxPackBytes batching\ on bootstrap. The notes ref on source has to ride the renamed\ tail-phase (was tag-phase) of executeBatched. Asserts both that the\ batched mode engaged and the notes ref landed at the right hash.\ - ReplicateRejectsOtherKindIntoExistingTarget: replicate's relay-only\ push gates on CanReplicateRelay which rejects RefKindOther. The test\ pins this as a deliberate v1 limitation: replicate + AllRefs other\ refs into a non-empty target errors with "use sync instead" rather\ than silently doing the wrong thing. (Empty-target replicate works\ via the bootstrap path, which is already covered.)\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: a12ee0dac22b+150

Simplify: Code Review and Cleanup

Review all changed files for reuse, quality, and efficiency. Fix any issues found.

Phase 1: Identify Changes

Run git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.

Phase 2: Launch Three Review Agents in Parallel

Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.

Agent 1: Code Reuse Review

For each change:

  1. Search for existing utilities and helpers that could replace newly written code. Look for similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.
  2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.
  3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.

Agent 2: Code Quality Review

Review the same changes for hacky patterns:

  1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
  2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones
  3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
  4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
  5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
  6. Unnecessary JSX nesting: wrapper Boxes/elements that add no layout value — check if inner component props (flexShrink, alignItems, etc.) already provide the needed behavior
  7. Nested conditionals: ternary chains (a ? x : b ? y : ...), nested if/else, or nested switch 3+ levels deep — flatten with early returns, guard clauses, a lookup table, or an if/else-if cascade
  8. Unnecessary comments: comments explaining WHAT the code does (well-named identifiers already do that), narrating the change, or referencing the task/caller — delete; keep only non-obvious WHY (hidden constraints, subtle invariants, workarounds)

Agent 3: Efficiency Review

Review the same changes for efficiency:

  1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
  2. Missed concurrency: independent operations run sequentially when they could run in parallel
  3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths
  4. Recurring no-op updates: state/store updates inside polling loops, intervals, or event handlers that fire unconditionally — add a change-detection guard so downstream consumers aren't notified when nothing changed. Also: if a wrapper function takes an updater/reducer callback, verify it honors same-reference returns (or whatever the "no change" signal is) — otherwise callers' early-return no-ops are silently defeated
  5. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error
  6. Memory: unbounded data structures, missing cleanup, event listener leaks
  7. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one

Phase 3: Fix Issues

Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.

When done, briefly summarize what was fixed (or confirm the code was already clean).

2mo ago·16m·47 calls

Cleanup committed. Net -89 lines across 12 files, all 8 --all-refs tests still pass.

What got fixed:

Skipped (low value or structurally invasive):

Ready to push?

720b519Simplify after review: dedupe, trim comments, flatten conditionals\ - Extract addPruneCandidates helper; the prune-managed switch is no\ longer pasted into both BuildPlans and BuildReplicationPlans.\ - Extract syncSession.finalizeCounts; the applyRejections + counter\ loop block is no longer duplicated between runSync and runReplicate.\ - Use convert.DesiredRefsForPlans in the bootstrap tail phase instead\ of building gitproto.DesiredRef inline.\ - Drop the defensive pushed < 0 clamp in bootstrapWithInputs; if\ Pushed and Warned ever disagreed, masking it would hide the bug.\ - Fold the "AllRefs implies BestEffort" coupling into allRefsFlag via\ cobra PreRunE instead of pasting the if-statement into two RunE\ bodies.\ - Flatten the 3-level nested conditional in NormalizeMapping.\ - Trim verbose doc blocks: RefKind, ActionWarn, RefScope.AllRefs,\ SyncPolicy.BestEffort, Pusher.OnRejection, syncSession.rejections,\ the bootstrap tail-phase comment, and the test docstrings that\ narrated motivation rather than the assertion. Removes the two\ empty-branch comments in the count switches by collapsing\ ActionWarn/Skip/Block into one no-op branch.\ \ Net: -89 lines, all 8 AllRefs integration + smoke tests still pass.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: f8bbc26df8b3+99/-188

  1. High: replicate --all-refs still weakens replicate’s failure semantics and can return success after only a partial mirror. --all-refs is wired through allRefsFlag(..., &req.Policy.BestEffort) for every sync-like command, including replicate ( cmd/git-sync/syncplan.go, cmd/git-sync/flags.go). In the replicate path, per-ref receive-pack rejections are then downgraded to warn and counted via finalizeCounts, not returned as an error ( internal/syncer/syncer.go, internal/syncer/syncer.go). That means a host rejecting refs/pull/* or similar can leave the target incomplete while replicate still exits successfully, which contradicts the command’s “overwrite target refs to match source” behavior.

  2. High: replicate --prune --all-refs can still take the bootstrap shortcut even when stale managed “other” refs on the target should be pruned. replicateCanBootstrap only blocks bootstrap-on-prune for branches and opted-in tags; it never checks AllRefs namespaces ( internal/syncer/syncer.go). By contrast, normal prune planning does treat AllRefs namespaces as managed and deletable ( internal/planner/planner.go). So if the target only has stale refs/notes/* / refs/pull/* refs, replicate can incorrectly skip straight to bootstrap and leave them behind.

  3. Medium: the --all-refs surface still promises “every refs/*”, but tags are explicitly excluded unless --tags is also set. The docs say --all-refs broadens discovery “to every refs/* namespace” ( docs/usage.md) and the CLI help says “mirror every refs/*” ( cmd/git-sync/flags.go), but BuildDesiredRefs only adds tags under IncludeTags and the test suite locks that in ("tag should not appear without IncludeTags") ( internal/planner/planner.go, internal/planner/planner.go, internal/planner/planner_test.go). This is at least a user-facing contract mismatch.

I did not rerun the full Go test suite in this environment; the review is based on the updated diff and targeted code inspection.

2mo ago·12m·34 calls

Summary of what your review caught and what's now fixed:

#1 (replicate weakening failure semantics) — allRefsFlag now takes a per-command implies list and a usage string. Sync/bootstrap include BestEffort; replicate doesn't, and its help text says "per-ref rejections fail the run, since replicate's contract is target == source." Two new CLI smoke tests pin this: TestRun_Replicate_AllRefsKeepsStrictFailureOnNg (errors) and TestRun_Sync_AllRefsWarnsOnNg (succeeds with warned > 0).

#2 (replicateCanBootstrap missing Other-kind) — added the matching case mirroring addPruneCandidates. New test TestRun_IntegrationReplicateAllRefsPruneSkipsBootstrapForStaleOtherRef seeds a stale refs/notes/stale on target and asserts replicate --prune deletes it.

Bonus fix uncovered by #2's test: runReplicate gated executeReplicate on len(relayPlans) > 0, so delete-only scenarios silently skipped the prune even after the bootstrap shortcut was bypassed. The strategy itself handled delete-only fine; the gate was over-narrow. Now gates on len(pushPlans) > 0 and only validates CanReplicateRelay when there are actual relay plans.

#3 (help/docs lying about "every refs/*") — CLI --all-refs now also implies --tags for sync/bootstrap (the bundling matches the help text). Library users still get orthogonal AllRefs / IncludeTags / BestEffort flags. Docs updated to call out the per-command bundling.

Full suite still passes. Ready to push?

5265400Fix three review findings on --all-refs\ \ 1. replicate --all-refs no longer enables BestEffort. Bundling them\ contradicts replicate's "target == source" contract — a host rejecting\ refs/pull/* would leave the target incomplete while replicate exited\ successfully. allRefsFlag now takes a per-command implications list;\ sync/bootstrap include BestEffort, replicate doesn't. Added a CLI smoke\ test that hooks the target receive-pack to ng every ref and verifies:\ replicate --all-refs errors, sync --all-refs warns and exits 0. The\ help text also splits per command (best-effort vs strict).\ \ 2. replicateCanBootstrap missed AllRefs other-kind refs in its prune\ check, so replicate could shortcut to bootstrap and silently leave\ stale refs/notes/* on target. Added the matching case to mirror the\ prune-candidate logic in planner. Added an integration test that\ seeds a stale notes ref on target and asserts replicate --prune\ --all-refs deletes it.\ \ 3. The same test surfaced a pre-existing latent bug: runReplicate gated\ the executeReplicate call on len(relayPlans) > 0, so delete-only\ plans (no source-side updates) were silently skipped — even though\ replicate.Execute itself handles delete-only correctly. The gate is\ now len(pushPlans) > 0; the CanReplicateRelay check is only run when\ there are relay plans to validate. Existing replicate-prune test\ still passes.\ \ 4. CLI --all-refs now also implies --tags for sync and bootstrap so the\ help text "every refs/*" is honest. Library callers keep the three\ flags (AllRefs, IncludeTags, BestEffort) orthogonal. Docs updated to\ describe the per-command bundling and the deliberate decoupling for\ replicate.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 7674291aab5f+240/-24

  1. Medium: the public/API contract for AllRefs still overstates what the library and unstable fetch path actually do. RefScope says AllRefs broadens scope “to every refs/* on the source” ( types.go), and fetch --all-refs says it will “include every refs/*” ( cmd/git-sync/fetch.go). But the actual selection logic still excludes tags unless IncludeTags is set ( internal/planner/planner.go, unstable/client.go, internal/syncer/syncer.go), and the test suite explicitly locks that in ("tag should not appear without IncludeTags") ( internal/planner/planner_test.go). The CLI sync/bootstrap/replicate wrappers now paper over this by implying --tags, but direct library callers and the unstable fetch command still get a narrower scope than the wording promises.

The earlier replicate/best-effort and replicate/prune/bootstrap issues appear fixed in the current branch. I didn’t find additional correctness regressions beyond the remaining scope-contract mismatch above.

2mo ago·3m·17 calls

Fix landed. The contract is now honest at every layer:

Full suite green.

3acfb7bLibrary AllRefs implies tag inclusion; close the contract gap\ \ The library docs and the unstable fetch CLI promised "every refs/*", but\ BuildDesiredRefs and addPruneCandidates excluded tags unless IncludeTags\ was set. The CLI sync/bootstrap wrappers papered over this by implying\ --tags, but library callers and `fetch --all-refs` got a narrower scope\ than the wording promised.\ \ Move the tag-inclusion implication into the library so the contract is\ honest at every layer:\ - BuildDesiredRefs adds source tags when IncludeTags || AllRefs.\ - addPruneCandidates' tag-prune case fires under the same condition.\ - replicateCanBootstrap matches.\ \ Drop the now-redundant CLI bundling: sync/bootstrap no longer set\ IncludeTags from --all-refs because the library covers it. SyncPolicy.\ BestEffort stays orthogonal so callers can opt into per-ref warn\ semantics on a narrower scope.\ \ Updated the planner test that asserted the old (wrong) "tag should not\ appear without IncludeTags" behavior, and added a fetch CLI smoke test\ verifying `fetch --all-refs` puts tag and notes refs into the wants\ list without --tags being passed.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: a0c571b71c5f+97/-31

  1. Stale phase log when tail is non-tag (bootstrap.go:626) The on-phase reporter still emits "pushing tags" even when the tail batch contains only RefKindOther refs. After the rename to tailPlans, the user-visible phase string lies under --all-refs without --tags. Update to e.g. "pushing tail refs" or branch on whether tailPlans actually contains tags.

  2. Behavioral change in runReplicate beyond AllRefs (syncer.go:844) The check moved from len(relayPlans) > 0 to len(pushPlans) > 0, which means pure-delete replicate runs (executeReplicate) now where previously it was a silent no-op. This is what makes TestRun_IntegrationReplicateAllRefsPruneSkipsBootstrapForStaleOtherRef pass, but it also changes pre-existing replicate --prune scenarios that produce only deletions. The new test covers the AllRefs case; consider adding a non-AllRefs pure-prune replicate test, or at minimum confirm repstrat.Execute handles a delete-only pushPlans cleanly (it should skip the relay fetch).

  3. Pushed - warned arithmetic in bootstrapWithInputs (syncer.go:1001) This relies on bResult.Pushed having already counted the rejected refs (since the rejection callback fires after the pack is sent). It does today, but the relationship is implicit. A short comment on why warned is subtracted, or shifting to "count from plans after rewrite" symmetric with finalizeCounts, would prevent future drift.

  4. allRefsFlag PreRunE chain Composing cmd.PreRunE via captured-prev is fine for current callers but not idempotent — calling allRefsFlag twice on one command would silently install two hooks. Not a real bug given current usage; just worth a one-line guard or comment if you anticipate sharing this helper.

2mo ago·2m·10 calls

All four addressed:

  1. tailPhaseLabel picks "pushing tags" / "pushing other refs" / "pushing tags and other refs" based on what's actually in the tail batch.
  2. New TestRun_IntegrationReplicatePruneDeleteOnlyRunsExecutor pins the broader gate change for the non-AllRefs branch case.
  3. bootstrapWithInputs now recounts Pushed from the rewritten plan slice (mirroring finalizeCounts) rather than relying on the implicit bResult.Pushed - warned arithmetic.
  4. One-liner on allRefsFlag noting it's not idempotent.

Full suite still green.

13f69e3Address follow-up review on --all-refs\ \ 1. bootstrap.go's batched-mode phase log said "pushing tags" even when\ the tail batch contained only RefKindOther refs. tailPhaseLabel picks\ "pushing tags", "pushing other refs", or "pushing tags and other refs"\ based on the actual contents.\ \ 2. The runReplicate gate change (relayPlans → pushPlans) affects all\ replicate flows, not just AllRefs. Add a non-AllRefs delete-only\ replicate test that pre-fix would have silently no-op'd on prune,\ pinning the broader behavior.\ \ 3. bootstrapWithInputs now recounts Pushed by walking the rewritten\ plan slice (mirroring finalizeCounts) instead of subtracting warned\ from bResult.Pushed. The arithmetic relied on bResult.Pushed having\ already counted rejected refs, which was true but implicit. The new\ loop makes the relationship with applyRejections explicit and\ resists future drift.\ \ 4. Comment on allRefsFlag noting it's not idempotent — calling it twice\ on one command would stack PreRunE hooks. Not a current bug; a\ one-line guard for future readers.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 5e3f6446f73c+84/-2

Second pass — with corrections from the first review.

Corrections to first review

My #1 (stale "pushing tags" log) was wrong. I missed tailPhaseLabel at internal/strategy/bootstrap/bootstrap.go:661, which already picks between "pushing tags", "pushing other refs", and "pushing tags and other refs". Retracted.

My #3 (Pushed - warned arithmetic) was wrong. The actual code at syncer.go:1006 already recounts Pushed from the rewritten plan slice rather than subtracting from bResult.Pushed, with a comment explaining why. Retracted.

Findings that still stand

Replicate behavioral change beyond AllRefs (syncer.go:844). Original main: executeReplicate only ran when len(relayPlans) > 0. New code: runs when len(pushPlans) > 0. This silently fixes a long-standing issue where pure-delete replicate (e.g. replicate --prune against a target with stale refs but matching tips) was a no-op. replicate.Execute does handle delete-only correctly (skips the FetchPack section, just calls PushCommands). But the change is untested for the non-AllRefs case. Add one targeted regression: replicate --prune with a target that has one stale branch and otherwise-current tips, assert the deletion lands.

allRefsFlag PreRunE composition (flags.go:60). Capturing prev and reassigning cmd.PreRunE works for current callers but isn't idempotent. If anyone ever calls allRefsFlag twice on the same command, only the last implies set fires. Cheap to harden with a one-liner guard or comment.

New findings on the second pass

CanReplicateRelay rejects RefKindOther outright (internal/planner/relay.go:165). The default branch in the switch returns (false, "replicate-unsupported-ref-kind") regardless of action. Effects:

PlanRef treats RefKindOther like a branch (internal/planner/planner.go:288). The switch covers RefKindTag explicitly; everything else (including other-kind) falls through to fast-forward ancestry checks against the source store. For refs/notes/* updates this is wrong-shaped — notes commits don't form a linear chain in the same sense as branch tips, so a typical notes append will fail the FF check and require --force to update. Probably acceptable as v1 behavior, but the user-facing Reason ("is not an ancestor of") will be confusing for notes refs. Either special-case RefKindOther to behave like tags (block-without-force on differing hashes, with a clearer reason) or document the limitation.

fetch --all-refs flag string is misleading (cmd/git-sync/fetch.go:72). Reads "include every refs/* on the source (notes, pulls, custom namespaces) in the fetch request". AllRefs implies tags too at the library level, but the wording lists only the "extra" namespaces, so a user who reads it might think they need both --tags and --all-refs. The sync/bootstrap usage strings are clearer about this.

BestEffort only reachable from CLI via --all-refs implication. The library exposes SyncPolicy.BestEffort independently and the docs note this is intentional, but there's no --best-effort flag. If a CLI user wants warn-semantics on a narrower scope, they can't get it. Probably an intentional surface decision (avoid flag proliferation), but worth confirming — three lines in syncplan.go would expose it.

No unit tests for applyRejections / finalizeCounts. Both are pure functions over []BranchPlan and a map[ReferenceName]string; integration coverage is solid but a direct test would catch refactor regressions on the rejection-keying logic (especially the "warned downgrade preserves Reason format" assertion).

Non-issues I checked

Net

Smaller list than the first pass once I corrected my misses. The replicate-update-of-other-kind-fails-on-second-run UX and the RefKindOther ancestry behavior are the two things I'd want addressed (or explicitly documented as v1 limits) before merge. The pure-delete-replicate test is cheap insurance for a real behavioral change. Everything else is polish.

2mo ago·4m·21 calls

Addressed all four findings that still stood:

Replicate idempotent re-run (CanReplicateRelay rejecting RefKindOther) — relaxed to accept other-kind with branch-shaped checks. Replicate's overwrite contract sidesteps the FF concern that gates Other out of sync's incremental relay. The negative test that pinned the old behavior is now a positive test that creates the notes ref on run 1, updates it on run 2, and asserts the relay path engages both times.

PlanRef ancestry check on Other-kind — grouped RefKindOther with RefKindTag in the kind-aware branch. Non-ancestor updates now block with "use --force to update other ref <name>" instead of the misleading "is not an ancestor of" reason. New TestRun_IntegrationAllRefsSyncOtherKindUpdateRequiresForce pins both the block reason and the --force succeeding case.

fetch --all-refs flag string — updated to list branches and tags alongside notes/pulls/custom (probe got the same fix).

Unit tests for applyRejections / finalizeCounts — three pure-function tests covering the warn-downgrade keying, the empty-map fast path, the reason format, and the Pushed/Deleted/Warned tallies independent of any strategy.

Skipping --best-effort as a separate CLI flag (you flagged it as probably-intentional; library callers can compose freely).

Full suite green, 11 commits on the branch. Ready to push?

75deaf4Round out other-kind ref semantics and unit-test the rejection logic\ \ Two real semantic gaps in the AllRefs flow that the second-pass review\ caught:\ \ 1. CanReplicateRelay rejected RefKindOther outright, so a user running\ replicate --all-refs periodically as a mirror would succeed on first\ run (Action=Create), succeed on no-op runs (Action=Skip), and fail\ the moment a notes/pull ref updated ("replicate-unsupported-ref-\ kind"). Replicate's overwrite semantics make the FF concern that\ keeps other-kind out of the sync incremental relay irrelevant here:\ add the kind to CanReplicateRelay with the same shape as branch and\ tag. The TestRun_IntegrationAllRefsReplicateRejects... test that\ pinned the old behavior is converted to a positive idempotent re-run\ test that exercises both create and update.\ \ 2. PlanRef treated RefKindOther like a branch and ran a fast-forward\ ancestry check on it. A typical refs/notes/* append produces a new\ commit that isn't an ancestor of the previous notes tip, so the\ check would always fail and the user would see the cryptic "is not\ an ancestor of" message. Group RefKindOther with RefKindTag in\ PlanRef so a non-trivial update blocks with "use --force to update\ ref " — clear, kind-aware, and consistent with the\ tag-retarget pattern. Replicate is unaffected (it doesn't run the\ FF check).\ \ Plus polish:\ - fetch and probe --all-refs help text now lists branches and tags\ alongside notes/pulls/custom, matching the library contract.\ - Unit tests for applyRejections and finalizeCounts pin the keying\ logic, the empty-map fast path, the warned-Reason format, and the\ Pushed/Deleted/Warned tallies independent of strategy execution.\ - docs/usage.md notes the sync-vs-replicate force semantics for\ other-kind refs.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: fd6c72102033+211/-20

Third pass. New angles I hadn't checked.

New findings

1. --all-refs + --branch foo interaction is inconsistent (internal/planner/planner.go:71). With --branch foo --all-refs:

So --branch foo is honored for branches but silently ignored for tags and other-kind refs. The doc comment on PlanConfig.AllRefs says it broadens "in addition to whatever branches/tags the existing flags select" — but for tags the implementation overrides the existing tag scope, not adds to it. Either:

No test exists for this combination today.

2. Pusher value-receiver fragility (internal/gitproto/push.go:32).NewPusher returns Pusher by value; methods take value receivers. The wiring at syncer.go:603 works because:

  1. s.target.pusher = NewPusher(...) stores a value inside a *targetSession.
  2. s.target.pusher.OnRejection = ... mutates the stored value.
  3. Strategies receive s.target.pusher later (after step 2), so the captured copy includes the callback.

Order-of-operations dependent. If anyone moves the strategy capture before the OnRejection assignment, or stores the pusher in a non-pointer location, BestEffort silently breaks with no test signal. Cheap fix: switch NewPusher to return *Pusher and use pointer receivers, or expose WithOnRejection(fn) as a builder method.

3. BestEffort downgrades only target-side rejections, not source-side (internal/gitproto/push.go callback location). The OnRejection callback fires from sendReceivePack decoding receive-pack's report-status. If the sourceupload-pack rejects a want <hash> for an other-kind ref (e.g. Gerrit refs/changes/* where the hash isn't in the publicly-fetchable closure), the entire FetchPack errors out and nothing downgrades to a warning. Common case: --all-refs against a server that advertises hidden refs but doesn't allow direct fetch of them. The user gets a hard failure with no per-ref granularity.

docs/usage.md says BestEffort handles "per-ref receive-pack rejections" — that's accurate but the user-visible failure mode for hostile sources isn't called out. Worth adding a sentence: "Source-side upload-pack failures (the server refusing a want) remain fatal; BestEffort only covers target-side receive-pack."

4. --all-refs --prune for sync mode lacks a dedicated test. The replicate path is covered (TestRun_IntegrationReplicateAllRefsPruneSkipsBootstrapForStaleOtherRef), and addPruneCandidates works for both. But there's no sync-mode test asserting that a stale refs/notes/foo on the target is deleted under sync --all-refs --prune. Same code path as replicate-prune, but worth a one-test sanity check given how new this scope is.

5. JSON warned field has no omitempty.json:"warned" is always present, value defaults to 0. That's fine for forward compat but worth noting: a release-note line "stable JSON adds warned; field always present, value 0 when no warnings" lets downstream consumers know what to expect.

Things I checked and found clean

Carryover from second pass that I'd still prioritize

Net

Three passes haven't surfaced anything that should block merge. The branch is well-tested, the layering is clean, and the contract differences (replicate strict / sync best-effort, AllRefs implies tag inclusion, BestEffort independent of scope at library level) are explicit. Above are sharpening notes — pick what's worth the time, ship the rest.

2mo ago·4m·19 calls

Twelve commits total. Third-pass items addressed:

Finding Fix
--branch foo --all-refs inconsistency normalizeAllRefs(cfg) clears Branches filter at the entry of the three plan builders; replicateCanBootstrap mirrors via branchScopeCovers. New planner test pins it.
Pusher value-receiver fragility NewPusher returns *Pusher; methods take pointer receivers. Strategies capture the pointer, so order-of-operations on OnRejection doesn't matter.
BestEffort source-side fetch gap docs/usage.md calls out: "source-side upload-pack failures remain fatal; BestEffort only covers target-side receive-pack."
sync prune coverage New TestRun_IntegrationAllRefsSyncPruneDeletesStaleOtherRef mirrors the replicate-side test for the sync code path.

JSON warned field stays without omitempty — that's a release-note concern when the branch ships.

Carryovers from your prior pass that were already in the branch:

Full test suite green. Ready to push.

3cf7d2cSharpen --all-refs semantics: branch-filter override, Pusher pointer, prune coverage\ \ Third-pass review caught four sharpening items, all addressed:\ \ 1. --branch foo --all-refs was inconsistent: branches got filtered to\ foo, but tags and other-kind refs were unconditionally included. The\ doc claimed "in addition to whatever existing flags select," but for\ tags the implementation overrode the existing scope. New helper\ normalizeAllRefs(cfg) clears cfg.Branches at the entry of the three\ plan builders so AllRefs really means every branch. replicate-\ CanBootstrap mirrors the same logic via a branchScopeCovers\ predicate. Test pins the override.\ \ 2. NewPusher returned Pusher by value with value-receiver methods. The\ wiring at session construction (set OnRejection on the stored value,\ then later pass it to strategies) only worked because field\ assignment came before strategies captured copies. A future refactor\ that reordered those steps would break BestEffort silently. Switch\ to *Pusher with pointer receivers — strategies now hold a pointer so\ any later OnRejection mutation flows through. Strategy interfaces\ still satisfied since *Pusher.PushPack matches the same shape.\ \ 3. BestEffort only covers target-side receive-pack rejections, not\ source-side upload-pack failures. A server that advertises a hidden\ ref but refuses to serve a `want` for its tip (Gerrit refs/changes/*\ is the common case) errors out the whole fetch with no warn\ granularity. usage.md now spells this out so users don't expect\ per-ref warn semantics for source rejections.\ \ 4. sync --all-refs --prune lacked a dedicated regression. The replicate\ path was covered, and addPruneCandidates is shared, but the sync\ side wasn't pinned. New TestRun_IntegrationAllRefsSyncPrune-\ DeletesStaleOtherRef seeds a stale refs/notes/stale on target and\ asserts sync --all-refs --prune deletes it.\ \ (JSON warned field stays without omitempty — the field-always-present\ shape is a release-note concern for whenever this branch ships.)\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 830978bf4f5c+104/-8

Findings new to this pass

1. result.Pushed = len(plans) in batched bootstrap is internally inconsistent under BestEffort (internal/strategy/bootstrap/bootstrap.go:653). The batched executor sets Pushed to the plan count without consulting whether the tail-phase PushPack callback fired any rejections. It works in practice because bootstrapWithInputs (syncer.go:1006) recounts from the rewritten plan slice, but a direct caller of Bootstrap from internal/strategy/bootstrap would see inflated Pushed numbers. Currently only syncer wraps it, so not user-visible — but worth aligning the strategy's own bookkeeping (count from plan actions, same as the wrapping syncer does).

2. No test covers batched bootstrap + AllRefs + BestEffort with a hostile target.

The combination is the most complex code path: OnRejection callback flowing through the Pusher value-copy into bootstrap.Params.TargetPusher interface, hitting during the tail phase after checkpointed branch batches. A target rejecting the notes ref under that scenario would catch wiring breaks the existing tests don't. One additional test (extend TestBootstrap_IntegrationAllRefsBatchedTailPhase with the receivePack hook from the warn test) would close the gap.

3. applyRejections runs twice when there are rejections (syncer.go:393–395). Once on pushPlans, then again on result.Plans. Cosmetic — the second call iterates over the full plan slice (Skip/Block included) for no functional reason. A pointer-slice approach ([]*BranchPlan for pushPlans aliasing into result.Plans) would let one mutation suffice. Minor refactor opportunity, not a bug.

4. Tag-prune scope is broad in mapping mode (pre-existing, but worth noting alongside the --branch interaction from pass 3).addPruneCandidates for tags only checks IncludeTags || AllRefs — it doesn't gate on whether the user is in mapping mode. So git-sync sync --map main:stable --tags --prune will prune every stale tag on the target even though the user only mapped one branch. This is pre-existing main behavior, not introduced here, but the --all-refs --prune combination this PR adds inherits the same shape — so a user running --branch foo --all-refs --prune may be surprised by collateral tag/notes deletions on the target.

If the inconsistency from pass 3 (--branch + --all-refs) is addressed, this falls out naturally; otherwise the docs should call out that --prune operates over the full namespace scope, not just the filtered subset.

5. Phase reporter cardinality.tailPhaseLabel returns one of three strings: "pushing tags", "pushing other refs", "pushing tags and other refs". The third option is grammatically a little off ("pushing tags and other refs" reads ambiguously — "other refs" relative to what?). Pure nit, but "pushing tags and non-branch refs" or "pushing tail refs" would be clearer. Truly cosmetic.

Verified clean this pass

Overall after four passes

The branch is solid. The remaining items in priority order:

  1. replicate --all-refs updating an existing other-kind ref always errors (pass 2) — real UX foot-gun for periodic mirrors. Document or fix CanReplicateRelay.
  2. --branch foo --all-refs ignores branch filter for tags/other-kind (pass 3) — semantic surprise; either document precedence or reject the combination.
  3. Pure-delete replicate --prune regression test for non-AllRefs (pass 2) — covers the syncer.go:844 loop change.
  4. Batched bootstrap + AllRefs + BestEffort test (pass 4) — closes the most complex untested path.
  5. Doc: source-side want failures aren't downgraded by BestEffort (pass 3).

Everything else is polish (cosmetics, internal consistency, minor refactors). I don't think a fifth pass would surface much that's actionable — diminishing returns set in around here.

2mo ago·3m·11 calls

Thirteen commits. Fourth-pass items addressed:

Finding Fix
#1 Bootstrap Pushed bookkeeping under BestEffort Doc comment on bootstrap.Result clarifying that strategy-level Pushed is attempted-count and BestEffort downgrades happen at a higher layer. No direct strategy callers exist yet, so just documenting the layering.
#2 No batched + AllRefs + BestEffort coverage New TestBootstrap_IntegrationAllRefsBatchedBestEffortDowngradesNg: large source forces batched mode, the tail phase hits a hook that ng's the notes ref, asserts Warned=1 and the branch tip lands on target.
#5 "pushing tags and other refs" ambiguous Renamed the mixed-case label to "pushing tail refs".

Skipped:

Carryovers from your closing list (#1, #2, #3, #5) were already on the branch from earlier passes.

You said diminishing returns set in around here, so I'll stop chasing review passes. The branch is at 13 commits, full suite green, ready to push when you say go.

b2a248aCover batched + AllRefs + BestEffort, polish bootstrap polish-items\ \ The most complex --all-refs path had no end-to-end coverage: large source\ pack forces TargetMaxPackBytes batching, the tail phase pushes other-kind\ refs after checkpointed branch batches, and the target ng's the notes\ ref. The OnRejection callback has to flow through *Pusher into\ bootstrap.Params.TargetPusher's interface boundary and downgrade the\ rejected ref to a warning. New TestBootstrap_IntegrationAllRefsBatched-\ BestEffortDowngradesNg pins this — the receivePackHook only fires on the\ tail-phase push (so branch batches go through the real handler) and the\ test asserts both Warned=1 and the kept-on-target branch tip.\ \ Plus two polish items:\ - bootstrap.Result.Pushed doc: explicit that BestEffort downgrades happen\ at a higher layer, so direct strategy callers (currently none — only\ syncer wraps it) know they're seeing attempted-push count, not net.\ - tailPhaseLabel: "pushing tags and other refs" → "pushing tail refs"\ for the mixed case. The old wording read ambiguously.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 93ec937211e5+98/-2

Simplify: Code Review and Cleanup

Review all changed files for reuse, quality, and efficiency. Fix any issues found.

Phase 1: Identify Changes

Phase 2: Launch Three Review Agents in Parallel

Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.

Agent 1: Code Reuse Review

For each change:

Agent 2: Code Quality Review

Review the same changes for hacky patterns:

Agent 3: Efficiency Review

Review the same changes for efficiency:

Phase 3: Fix Issues

When done, briefly summarize what was fixed (or confirm the code was already clean).

2mo ago·24m·69 calls

Net -59 lines across 8 files, full suite green.

What got fixed:

Skipped per agent feedback or earlier-pass discussion:

Branch is now 14 commits, ready to push when you say go.

can you give me a short pr description

2mo ago·5.5s

Please run /login · API Error: 401 The socket connection was closed unexpectedly. For more information, pass verbose: true in the second argument to fetch()

can you give me a short pr description

2mo ago·22.8s

Add --all-refs for arbitrary refs/* mirroring + BestEffort failure mode

Mirrors any refs/* namespace (notes, pulls, custom) on a best-effort basis,
not just refs/heads/* and refs/tags/*. Per-ref receive-pack rejections
downgrade to ActionWarn instead of failing the whole sync — the mode that
makes mirroring into hosts with hidden refs (e.g. GitHub refs/pull/*)
actually work.

CLI:
  --all-refs              broadens scope to every refs/*; for sync and
                          bootstrap also enables best-effort failure
                          handling. Replicate keeps strict semantics
                          (target == source) so it doesn't bundle
                          BestEffort.

Library:
  RefScope.AllRefs        broadens scope; implies tag inclusion.
  SyncPolicy.BestEffort   downgrades per-ref rejections to warn; orthogonal
                          to AllRefs so callers can compose freely.
  RefKindOther            new kind for refs outside heads/tags.
  ActionWarn + Result.Warned  per-ref rejection result and counter.

Behavior:
  - Sync incremental relay rejects RefKindOther by design; falls through
    to materialized push.
  - Replicate's overwrite contract handles RefKindOther via relay.
  - Other-kind ref updates require --force (same as tags) — notes
    appends don't form FF chains.
  - --all-refs --prune extends to other-kind on both sync and replicate.
  - Source-side upload-pack failures stay fatal; BestEffort only covers
    target-side receive-pack.

Coverage:
  Bootstrap (one-shot + batched), incremental relay, materialized
  fallback, replicate (create/update/prune/delete-only), best-effort
  warn-downgrade, and CLI smoke tests for sync/replicate/fetch/bootstrap.

Docs:
  docs/usage.md walks through --all-refs, the per-command bundling, the
  sync/replicate split, and the source-side limitation.

can you give it to me without artifiical line breaks

2mo ago·6.7s

Add --all-refs for arbitrary refs/* mirroring + BestEffort failure mode

Library:
  RefScope.AllRefs broadens scope; implies tag inclusion.
  SyncPolicy.BestEffort downgrades per-ref rejections to warn; orthogonal to AllRefs so callers can compose freely.
  RefKindOther: new kind for refs outside heads/tags.
  ActionWarn + Result.Warned: per-ref rejection result and counter.

Docs:
  docs/usage.md walks through --all-refs, the per-command bundling, the sync/replicate split, and the source-side limitation.

39055c0Simplify after fifth review: dedupe, fold tag/other walk, reuse helpers\ \ Fifth review pass surfaced a few real wins after the obvious dedup\ opportunities had landed in earlier passes:\ - planner.RefPrefixes now takes PlanConfig instead of three positional\ bools. Five call sites all passed the same triple; the new shape\ removes the footgun (was-it-includeTags-or-allRefs?).\ - BuildDesiredRefs collapses the tag and other-kind walks into one\ iteration of sourceRefs under AllRefs+IncludeTags, instead of two\ full traversals. Saves a pass on Gerrit-style repos with many\ refs/changes/* or refs/notes/*.\ - syncer.tallyActions extracted from finalizeCounts; bootstrapWithInputs\ now uses it instead of an open-coded recount loop and three lines\ of self-justifying comment.\ - syncertest.SetRefAtBranch helper replaces 7+ copies of the\ resolve-head + SetReference + dual-fatal-check pattern across the\ AllRefs integration tests.\ - syncertest.DenyRefsReport helper replaces 4 copies of the ng-status\ hook synthesis (3 in syncer integration, 2 in cmd CLI tests).\ - The CLI test server's sideband-wrap now respects no-progress, mirroring\ the syncer test server's writeReceivePackReport behavior.\ - Comment trims: normalizeAllRefs, addPruneCandidates, tailPhaseLabel,\ syncSession.rejections, and four CLI test docstrings.\ \ Net: -59 lines, all tests still green.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 0c2a854b8908+121/-180

question: compared to git pull / push --mirror is there anything else we are missing?

2mo ago·48.8s

Three real gaps I'd flag:

1. HEAD / symbolic refs.git clone --mirror sets the target's HEAD to track whatever the source's HEAD points at (usually refs/heads/main). We discover the source HEAD target during ref advertisement (s.sourceService.HeadTarget is already in the session) but don't push it as a symref to the target. So a mirrored target's HEAD stays at whatever it was initialized to. Symbolic refs in general have the same gap — we treat every advertised ref as a direct hash ref.

2. Atomic update semantics.git push --mirror to an atomic-capable receive-pack is all-or-nothing within one push. Our pipeline is multi-phase: bootstrap branch batches first, then tail (tags + other), then prune deletes for --prune. A failure mid-pipeline leaves a partial mirror — which is exactly why we ended up adding BestEffort for the soft-failure case, but the hard-failure case still leaves the target in an intermediate state. The git-receive-pack atomic capability would let us send all commands in one update-request; we don't currently use it.

3. Force-with-lease.git push has --force-with-lease[=<refname>:<expect>] to refuse a force-update if the remote tip doesn't match the expected hash — protects against clobbering concurrent updates. We have --force (unconditional) only. Niche but real for periodic mirrors that race other writers.

Things I checked and that aren't gaps:

Of the three, HEAD propagation is the most user-visible — someone mirroring a repo and wondering why git clone <mirror> checks out the wrong branch. Atomic is the architecturally interesting one but probably not what users hit first. Force-with-lease is the least common.

Can we create issues for all 3 on the repo? One thing with --force-with-lease: this works with a local repo because you have a known remote state usually, but git-syncs next run is stateless as the one before, but the idea would be to get what target has and then when pushing new things (after loading them from source) to then check refs didn't move?

2mo ago·18m·6 calls

Three issues filed against entireio/git-sync:

linting fails can you fix and push

2mo ago·4h 31m·36 calls

Pushed. Lint summary:

All tests still pass. Commit 2bdef7a pushed to origin/soph/all-refs.

2bdef7aFix lint failures: exhaustive switches, errcheck, goconst\ \ Eleven golangci-lint issues from the all-refs branch additions:\ - exhaustive (8): every kind/action switch in planner, relay, replicate,\ bootstrap, and syncer needed explicit no-op cases for the new\ RefKindOther / ActionWarn enum members. Folded the dead default\ branches into explicit cases — exhaustive's intent is to flag any\ future enum addition that silently lands in default.\ - errcheck (2): the test code's `if v, _ := m["x"].(T); ...` pattern\ triggered errcheck for the discarded comma-ok bool. Switched to\ `if v, ok := m["x"].(T); ok && ...`.\ - goconst (1): "bootstrap" appeared three times across syncer test\ files. Added relayModeBootstrap alongside relayModeIncremental and\ relayModeBootstrapBatch.\ \ PlanReplicationRef: dropped the redundant default arm — the reason\ string was identical to the RefKindBranch case, so the new\ case RefKindBranch, RefKindOther covers it.\ \ CanIncrementalRelay: turned the `default:` arm into the explicit\ RefKindOther case it always was. Same outcome, exhaustive happy.\ \ replicate.Execute: dropped the unreachable `default` that errored on\ unknown actions. The new switch covers every Action explicitly; if\ a future Action gets added, exhaustive will yell at the call site.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: aacb424eaf37+18/-14

ok, one more thing: if we do --all-refs and include github pull refs, on an opensource repo this means pulling all the commits that are maybe not even merged so we are pulling commits in from forks... I wonder if we need to differentiate that

2mo ago·2.6s

Please run /login · API Error: 401 Invalid authentication credentials

2mo ago·37.2s

Yeah, this is a real concern and it bites hard on GitHub. Some numbers to ground it:

There's also refs/pull/N/merge (GitHub's test-merge commit, only on mergeable PRs) — even more synthetic objects.

For some users this is a feature ("archive every PR commit, even if the fork gets deleted"), for others it's a footgun ("why is my mirror 10x the size?"). So the right shape is letting users carve.

Two reasonable designs:

--exclude-ref-prefix refs/pull/ (repeatable). Subtracts from --all-refs scope. Composes well: --all-refs --exclude-ref-prefix refs/pull/ --exclude-ref-prefix refs/changes/ is "everything except the noisy hidden-ref namespaces." Pragmatic, matches the most common ask.

--ref-prefix refs/notes/ (repeatable). Adds to the branches-and-tags base. --ref-prefix refs/notes/ --ref-prefix refs/replace/ is "branches, tags, plus these specific namespaces." Cleaner conceptually — no need for --all-refs at all for users who know exactly what they want.

I'd lean toward shipping both eventually, but if I had to pick one for v1, --exclude-ref-prefix is the right starter because the dominant use case is "I tried --all-refs and got pull noise; let me opt out." The include-only path is for power users who know their namespaces.

Cheap interim: just document the GitHub refs/pull/* size blowup as a known sharp edge in usage.md, and ship the exclude flag in a follow-up. That keeps this PR scoped to the core --all-refs work.

Want me to add the doc note now and file an issue for the exclude flag, or implement exclude in this PR?

add the exclude flag and update the doc

2mo ago·36m·71 calls

Pushed. Summary:

--exclude-ref-prefix <prefix> (repeatable) subtracts namespaces from auto-discovery and prune scope. The semantic is "leave this namespace alone entirely" — not pulled, not pushed, not pruned. Explicit --map entries bypass the filter since they're explicit user intent.

Filter applies at four sites via planner.IsRefExcluded:

Wire-level: ls-refs still requests "refs/" under --all-refs (the protocol prefix arg is include-only), the exclude filter is purely client-side. That's fine because the cost saved is mostly on transfer of objects reachable from excluded refs, not the ls-refs response itself.

Coverage: planner unit tests (subtract under AllRefs + Mappings override), integration test (sync seeds refs/pull and refs/notes, asserts only notes lands), CLI smoke test through cobra entry. Docs in usage.md walk through the GitHub case.

Commit ee37983 pushed.

ee37983Add --exclude-ref-prefix to trim namespaces from --all-refs\ \ GitHub's refs/pull/* namespace is the load-bearing case: every PR ever\ opened against an open-source repo contributes a refs/pull/N/head that\ points at a commit in someone's fork. --all-refs against a busy repo\ (kubernetes ~120k PRs, react ~17k PRs) drags all of those into the\ target, even when the fork is long gone.\ \ --exclude-ref-prefix subtracts namespaces from auto-discovery and prune\ scope. Excluded refs are left alone entirely: not pulled from source,\ not pushed to target, not pruned from target. Explicit --map values\ override the filter, since they're explicit user intent.\ \ Common shape:\ \ git-sync sync --all-refs --exclude-ref-prefix refs/pull/ \ \ The flag is repeatable across all sync-like commands plus probe and\ fetch. ExcludeRefPrefixes is also exposed on RefScope (and on\ ProbeRequest for probe-only callers) at the library level.\ \ Implementation: a small IsRefExcluded helper in planner gates four\ sites — the branch walk, the tag/other walk, addPruneCandidates, and\ replicateCanBootstrap's prune-shortcut check. The wildcard filter\ applies in BuildDesiredRefs' three auto-discovery paths; the Mappings\ path bypasses it by construction. Wire-level ls-refs still requests\ "refs/" under AllRefs (the wire protocol's prefix arg is include-only),\ and the filter applies client-side.\ \ Tests:\ - planner unit: ExcludeRefPrefixes subtracts under AllRefs, doesn't\ override explicit Mappings.\ - integration: sync seeds refs/pull/1/head + refs/notes/commits, then\ syncs with --all-refs --exclude-ref-prefix refs/pull/ and asserts\ notes lands but pull doesn't.\ - CLI smoke: cobra entry through the full pipeline.\ \ Docs: usage.md walks through the GitHub use case and the\ subtract-from-auto-discovery semantics.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: dc14c9e7b56c+256/-54

Findings

  1. probe --exclude-ref-prefix is wired but ineffective in the returned ref list. cmd/git-sync/probe.go exposes the flag, and the docs say excluded refs are “left alone entirely” and “not pulled from source” via auto-discovery semantics docs/usage.md. But newProbeResult() still serializes every entry from s.sourceRefMap without applying planner.IsRefExcluded internal/syncer/syncer.go. The planner path does apply that filter for sync/fetch behavior internal/planner/planner.go, so probe is the odd one out. In practice, probe --all-refs --exclude-ref-prefix refs/pull/ will still report refs/pull/*, which makes the new public ProbeRequest.ExcludeRefPrefixes knob misleading and breaks probe as a way to preview the effective scoped ref set.

Assumption

Change summary

2mo ago·10m·9 calls

Fixed and pushed (commit eb2d4b0). newProbeResult now applies IsRefExcluded to the source ref map before serializing, so probe --all-refs --exclude-ref-prefix refs/pull/ returns the same filtered set that a sync would act on. Regression test TestRun_Probe_ExcludeRefPrefixFiltersReturnedRefs goes through the cobra entry — seeds both refs on source, asserts pull is absent and notes is present in the JSON output.

eb2d4b0Apply ExcludeRefPrefixes to probe output\ \ probe accepted --exclude-ref-prefix and ProbeRequest.ExcludeRefPrefixes\ since the flag landed, but newProbeResult serialized every ref from\ s.sourceRefMap verbatim — so `probe --all-refs --exclude-ref-prefix\ refs/pull/` still listed refs/pull/* in the JSON output, breaking\ probe as a way to preview the same effective ref set that sync/fetch\ would act on.\ \ Filter in newProbeResult so the contract holds across all command\ paths. Regression test goes through the CLI: seeds refs/pull/1/head\ and refs/notes/commits on source, runs `probe --all-refs\ --exclude-ref-prefix refs/pull/ --json`, asserts pull is absent and\ notes is present.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 7d2d1a10852b+60

ok, pr is merged, can you now work https://github.com/entireio/git-sync/issues/45 (create a new branch, start with tests, do meaningful commits)

2mo ago·2h 10m·136 calls

Branch soph/head-symref-info pushed with 5 commits. Worth flagging upfront: this PR is smaller than the issue asks for. Here's why and what's there.

What landed:

  1. Failing tests pin the public contract (commit 7d1c3b8).
  2. TargetFeatures.HeadTarget reads the symref capability from receive-pack advertisements (commit 29bf74c), with unit test.
  3. Result.SourceHEAD and ProbeResult.SourceHEAD surface the source's default branch via integration tests (164174f).
  4. Bridge + CLI render source-head: … in human output and execution.sourceHead in JSON (6a04be3).
  5. Docs explain the HEAD-propagation limit and the two manual workarounds (d5bc3f5).

What I scoped out and why:

The protocol limitation is genuine — git push --mirror doesn't propagate HEAD either. We're now at parity for what's observable. The docs make the limitation explicit so users aren't surprised, and the manual mitigation (git init --bare --initial-branch=<sourceHead> before first sync) is the same workaround mirror operators use today.

Want me to open a PR with this scope, file follow-up issues for the symref-update and target-HEAD-detection work, or push further on either of those in this branch?

7d1c3b8Add failing tests for HEAD propagation: source/target HEAD in Result + ProbeResult\ \ issue #45: git-sync mirrors refs but never propagates the source's HEAD\ symref target to the target. A repo mirrored from a default=main source\ into a default=master target ends up with a dangling target HEAD; a\ subsequent `git clone target` checks out master instead of main.\ \ Step 1 of the implementation: pin the public contract via three\ failing integration tests that drive the API shape.\ - TestRun_IntegrationBootstrapSurfacesHEADInResult: source default\ master, target init'd with --default-branch=main, asserts Result\ exposes both HEAD targets as plumbing.ReferenceName.\ - TestRun_IntegrationBootstrapHEADMatch: same default on both sides,\ asserts SourceHEAD == TargetHEAD so callers can detect mismatch via\ field equality.\ - TestProbe_IntegrationSurfacesBothHEADs: probe surfaces the same info\ without mutating target, useful for previewing the mismatch.\ \ The tests fail to build today (Result and ProbeResult don't have the\ SourceHEAD/TargetHEAD fields yet); subsequent commits add the fields\ and the capture logic.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: ce76aab97a1f+103

29bf74cParse target HEAD symref into TargetFeatures\ \ Read symref=HEAD: from the receive-pack advertisement the same way\ the source's RefService already does, and store it on TargetFeatures.\ The session can then surface it without needing to keep the raw\ *packp.AdvRefs around at the top level.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 9d1c1f5b25ea+17

164174fSurface source HEAD symref in Result and ProbeResult\ \ The source's symref HEAD target (e.g. refs/heads/main) is already\ captured during the source advertisement parse but never escapes the\ session. Surface it on syncer.Result and syncer.ProbeResult so library\ callers and the CLI can compare against the target's intended default\ branch.\ \ The test-server scope grew slightly: the v2 ls-refs handler now honors\ the `symrefs` argument and emits HEAD with a symref-target attribute,\ mirroring what real git advertises. Two new integration tests pin the\ field through both Sync and Probe.\ \ Target HEAD is deliberately not exposed here. The receive-pack\ advertisement we already perform doesn't include HEAD (per go-git's\ serve.go: addReferences passes addHead=false for forPush), and\ detecting it would need a separate upload-pack round-trip against the\ target. Tracked as a follow-up.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 5e441c7c5d73+98/-92

6a04be3Plumb SourceHEAD through bridge + render in human output\ \ internalbridge.ProbeResult adds a SourceHEAD field; bridge.SyncResult's\ ExecutionSummary gets one too. CLI JSON output picks them up\ automatically through the From* conversions.\ \ Result.Lines() and ProbeResult.Lines() emit a "source-head: " line\ when non-empty, matching the existing "source: " / "ref-prefixes"\ style. The line gives users a quick way to see the source's default\ branch alongside the other discovery metadata.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: e81d5fee3471+13/-2

d5bc3f5Document the HEAD propagation limitation and the workarounds\ \ usage.md gains a "HEAD / Default Branch" section explaining:\ - where to find the source's HEAD in JSON/human output\ - why the target's HEAD isn't reconciled (no portable receive-pack\ symref-update; go-git alpha doesn't ship the capability anyway)\ - two mitigations: match the default at init time (preferred), or set\ HEAD manually post-sync via `git symbolic-ref` or the host's API\ \ The JSON Output section lists the new `execution.sourceHead` /\ `sourceHead` fields.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: bd2959cd5c8a+29

we should rather push both on go-git, right?

2mo ago·27.7s

Partly, yeah — but they're two different problems.

Symref-update (active HEAD propagation): real go-git gap. The protocol exists (git 2.36+ receive-pack advertises symref-update and accepts a symref-update <ref> <target> command in the update-request), and go-git neither parses the advertisement nor encodes the command. Adding it benefits every go-git client, not just us. Worth filing upstream — either a packp.UpdateRequests extension or a new request type. Server-side support is the practical limit (GitHub/GitLab don't implement symref-update; they expose default-branch via their own APIs), but for self-hosted git ≥2.36 it'd work out of the box.

Target HEAD detection: not a go-git limitation. It's that receive-pack — every receive-pack, not just go-git's server — doesn't advertise HEAD by design. The fix is an extra upload-pack round-trip against the target URL during probe/session setup; go-git already supports that (it's how we read the source HEAD). We can do that work entirely on our side, no upstream needed.

So:

If you want, I can:

  1. File a go-git issue describing the symref-update gap with the relevant git spec link, then close issue #45 here pointing at it.
  2. Or land target HEAD detection as a follow-up in this repo (smaller scope, no upstream waiting).
  3. Or both.

Which way do you want to push?

can you explain the work for 2 more?

2mo ago·42.6s

Here's the shape of the work for target HEAD detection.

The protocol fact: every git server exposes two endpoints — upload-pack (read; used for git clone/git fetch) and receive-pack (write; used for git push). Same URL, different service. The info/refs advertisement from upload-pack includes HEAD as a symref. From receive-pack it doesn't. We currently query the target's receive-pack because we're pushing; that's why HEAD is invisible to us.

The fix: also do an info/refs?service=git-upload-pack against the target URL during session setup, parse HEAD from that response, and surface it. One extra GET; no pack transfer.

What changes in code:

  1. internal/syncer/syncer.go — newSession: after the existing target receive-pack advertisement, add an upload-pack advertisement against the same target URL. Stash the parsed HEAD on targetSession. Failures are non-fatal — if the target's upload-pack rejects our auth (rare; push-only mirrors), or the target is empty (HEAD's underlying ref doesn't exist), we just leave the field empty.

  2. internal/gitproto: reuse what's there. gitproto.AdvertisedRefsV1 + headTargetFromAdv already handle v1. For v2 source we use ListSourceRefs; that function is service-agnostic despite the name (it just does v2 ls-refs against the connection), so it works for the target too.

  3. targetSession struct: add a headTarget plumbing.ReferenceName field, parallel to sourceService.HeadTarget.

  4. Result.TargetHEAD / ProbeResult.TargetHEAD: re-add the field I stripped earlier; the bridge/CLI wiring is the same shape as SourceHEAD.

  5. Concurrency, optional but cheap: run the receive-pack and upload-pack advertisements against the target concurrently in goroutines (~10 LOC). Adds zero latency vs. doing receive-pack alone, since they're independent GETs.

Auth nuance: we'd use the existing target auth credentials for the upload-pack request. Most providers tie push and pull auth together (basic-auth covers both). The exceptions are push-only deploy tokens — in those cases the upload-pack 401s and we silently skip detection. Fail-open keeps it from being a regression.

Test surface: minimal. The test server's transport.AdvertiseRefs already serves HEAD correctly for upload-pack (we saw the addReferences(st, ar, !forPush) logic). I'd add a third integration test (TestRun_IntegrationSyncSurfacesTargetHEAD) that pre-seeds the target with a branch and HEAD pointing at it, runs sync, asserts Result.TargetHEAD matches.

Cost: one extra ~1-3 KB HTTP GET per session, parallelisable with the existing target request → realistically zero added latency. Negligible.

Effort: ~80-150 LOC including tests, mostly straight plumbing since the gitproto helpers exist. Maybe an hour if nothing's weird.

What it doesn't solve: the active propagation half — actually pushing the new HEAD value to target. That still needs symref-update capability from go-git. Detection-only is still useful (let users compare programmatically, surface mismatches in CLI output, optionally emit a real notice).

Want me to land this in the current branch, or as a separate follow-up PR?

yeah let's add that then to this PR, and then I'll see to get a stab at creating a go-git pr

2mo ago·1h 1m·56 calls

Branch pushed with both halves now. Nine commits total on soph/head-symref-info:

Source HEAD half:

Target HEAD half:

The shape:

Ready for PR review when you are.

03819d2Discover target HEAD via concurrent upload-pack info-refs\ \ receive-pack advertisements don't include HEAD (server-side go-git's\ addReferences passes addHead=false for forPush, matching real git\ servers). So the session now does a second info-refs against the\ target's upload-pack endpoint, runs it concurrently with the existing\ receive-pack info-refs, and parses HEAD's symref capability from the\ result.\ \ Cost: one extra HTTP GET per session, parallelised with the existing\ target request → effectively zero added latency. Failures are\ non-fatal: push-only auth that 401s on upload-pack just leaves\ targetSession.headTarget empty.\ \ gitproto.DiscoverHEAD wraps the v1 advertisement parse. The session\ goroutine swallows errors; future telemetry could surface them if\ useful.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: bf6948e65b2d+44/-7

e470b83Plumb TargetHEAD through Result/ProbeResult/bridge\ \ syncer.Result and syncer.ProbeResult re-gain the TargetHEAD field.\ internalbridge.ProbeResult.TargetHEAD and ExecutionSummary.TargetHEAD\ expose it to library callers / JSON output. Human Lines() emit a\ "target-head: " line when populated.\ \ All Result construction sites (sync, replicate, bootstrap wrapper)\ pick up s.target.headTarget. ProbeResult does the same in the\ s.target != nil branch.\ \ Together with the previous commit's upload-pack discovery, callers\ can now see both sides of the HEAD picture and decide what to do —\ git-sync still doesn't actively propagate HEAD over the wire (no\ go-git symref-update support yet), but the field-equality check is\ enough for a CLI script or downstream tool to detect a default-branch\ mismatch.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 0b4edbe8a55f+24/-9

f682d3eUpdate docs: target HEAD detection landed, mismatch is observable\ \ usage.md now describes both sourceHead and targetHead, the upload-pack\ round-trip behind the target side, and the three practical mitigations\ for a default-branch mismatch:\ - match defaults at init time (preferred)\ - set HEAD manually post-sync (host API or git symbolic-ref)\ - script around the probe output to detect mismatches before sync\ \ The receive-pack-omits-HEAD limitation note moves to a smaller side\ remark; we now work around it rather than calling it a follow-up.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 120ccae292d5+25/-17

Simplify: Code Review and Cleanup

Review all changed files for reuse, quality, and efficiency. Fix any issues found.

Phase 1: Identify Changes

Phase 2: Launch Three Review Agents in Parallel

Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.

Agent 1: Code Reuse Review

For each change:

Agent 2: Code Quality Review

Review the same changes for hacky patterns:

Agent 3: Efficiency Review

Review the same changes for efficiency:

Phase 3: Fix Issues

When done, briefly summarize what was fixed (or confirm the code was already clean).

2mo ago·14m·50 calls

Pushed. Net -43 lines, full suite + lint green.

Summary of what got fixed:

Real bugs / structural:

Quality:

Skipped per agent feedback:

Branch is at 10 commits, ready for PR.

c0dde3eSimplify after review: drop dead field, dedupe, serialize discovery\ - Drop TargetFeatures.HeadTarget. Receive-pack adverts omit HEAD by\ protocol design (server-side addReferences passes addHead=false for\ forPush), so the field was structurally always empty in real use.\ Real target HEAD lives on targetSession.headTarget from DiscoverHEAD.\ - Serialize the target upload-pack discovery instead of running it\ concurrently with receive-pack. Both calls share targetConn, and\ RequestInfoRefs mutates conn.Endpoint.Scheme/Host under\ FollowInfoRefsRedirect — concurrent use raced on a shared *url.URL.\ Cost: one extra round-trip wall time. Correctness > the 50-100ms save.\ - Add (s *syncSession) heads() helper; the six Result/ProbeResult\ construction sites previously repeated the same SourceHEAD/TargetHEAD\ field pair.\ - Extract seedTargetWithFeatureHEAD test helper; the new sync and probe\ HEAD tests had an identical 18-line target-seed dance.\ - Trim verbose docstrings on Result, DiscoverHEAD, the discovery block,\ the new test docstrings, and lsRefsCoversHead.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 535cfb3709fd+77/-120

2mo ago·48.3s·3 calls

Fixed and pushed (commit ac61873). usage.md now reflects the sequential ordering and the one-extra-RTT cost, with a short note on why (shared connection + redirect-mutation).

ac61873Fix docs: target HEAD discovery is sequential, not concurrent\ \ The previous commit serialized the upload-pack info-refs after the\ receive-pack call to avoid a conn.Endpoint race under\ FollowInfoRefsRedirect, but the docs still claimed the round-trip ran\ concurrently. Update usage.md to match the actual behavior (one extra\ RTT during target setup) and the reason for the ordering.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: e5ab3abb81d4+6/-4

let's open a PR

2mo ago·7m·6 calls

PR opened: https://github.com/entireio/git-sync/pull/50

11 commits, exposes source + target HEAD via the upload-pack discovery, sequential to avoid the conn.Endpoint race, with docs + integration tests covering both Sync and Probe paths. Active propagation is deliberately scoped out — that's the go-git symref-update upstream you're going to take a stab at.

can we also mention that GitHub and GitLab aren't supporting symref-update ?

2mo ago·19.6s·1 call

Updated. The "What this doesn't do" section now calls out that GitHub and GitLab don't implement symref-update server-side either — they expose default-branch changes through their own REST APIs — so even with a go-git fix, hosted providers still need manual reconciliation. Active wire-level propagation will mostly benefit self-hosted git ≥ 2.36 targets.

DiscoverHEAD may corrupt target endpoint for push operations

Medium Severity

When --target-follow-info-refs-redirect is enabled, DiscoverHEAD calls RequestInfoRefs for upload-pack on the same targetConn that was already used for receive-pack. RequestInfoRefs mutates conn.Endpoint.Host/Scheme to the final redirect destination. If the upload-pack endpoint redirects to a different host than receive-pack did, subsequent push operations (which use conn.Endpoint via the Pusher created on the same *Conn) will target the wrong server. The conn.Endpoint needs to be saved and restored around the DiscoverHEAD call, or a separate connection used.

2mo ago·10m·17 calls

Fixed, tested, and pushed (commit 5a9df918).

Summary of what changed:

Fix: newSession now toggles targetConn.FollowInfoRefsRedirect = false around the DiscoverHEAD call and restores it after. The http.Client still follows the 30x internally to read the advertisement; we just suppress the post-call mutation of conn.Endpoint, so the Pusher's captured endpoint stays put.

Regression test: TestRun_IntegrationSyncTargetFollowRedirectPreservesPushHost wraps the real receive-pack server with a handler that 307s GETs of /info/refs?service=git-upload-pack to a decoy host (which 404s POSTs). I verified the test fails cleanly without the fix — exactly the reviewer's symptom: push target refs: target receive-pack: http 404 ... decoy refuses POSTs.

PR #50 will pick this up automatically on the next CI run.

5a9df91Preserve target endpoint across HEAD discovery when redirects are followed\ \ DiscoverHEAD reuses targetConn, which Pusher captured after the\ receive-pack info-refs settled on the resolved push host under\ FollowInfoRefsRedirect. If the target's own upload-pack info-refs\ redirects to a different host (a common read-replica setup),\ RequestInfoRefs would clobber conn.Endpoint.Host/Scheme to the read\ host, and subsequent push POSTs would land on a host that 404s\ receive-pack.\ \ Fix: temporarily toggle conn.FollowInfoRefsRedirect off during the\ DiscoverHEAD call. http.Client still follows the 30x internally to read\ the advertisement; we just suppress the post-call conn.Endpoint\ mutation, leaving Pusher's endpoint intact.\ \ Regression test wraps the real receive-pack server with a handler that\ 307s GETs of /info/refs?service=git-upload-pack to a decoy host that\ 404s POSTs. Without the fix, sync fails with "decoy refuses POSTs"; with\ it, push lands on the real receive-pack host.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 7e51a96fa59a+70/-1

can you now give me a rough problem description for the work item on go-gi

2mo ago·28.4s

Here's a problem description suitable for filing on go-git (or for your own scoping notes):

Title: receive-pack: support the symref-update capability

Problem

go-git's receive-pack client (packp.UpdateRequests) can update direct hash refs but not symbolic ones. There is no way to change a remote symref's target — for example, to repoint HEAD from refs/heads/master to refs/heads/main on a mirror.

git itself added the symref-update capability to receive-pack in 2.36 (2022). The wire format, per Documentation/gitprotocol-pack.adoc, advertises symref-update in the capability list and accepts a command:

symref-update <refname> <new-target> [<old-target>]

mixed into the update-request body alongside the regular <old-oid> <new-oid> <refname> lines.

Use case

Mirror tools that want git push --mirror parity plus default-branch propagation. Concretely, a mirror of a default=main source into a target initialized with default=master ends up with a dangling HEAD; cloning the mirror checks out the wrong branch. The only portable wire-level fix is symref-update. Today, tools either skip propagation (git push --mirror does) or fall back to provider-specific REST APIs.

(For reference: git-sync runs into this — issue entireio/git-sync#45. GitHub and GitLab don't implement symref-update server-side, so this primarily benefits self-hosted git ≥2.36 targets, but it'd unblock the wire-level path for those that do.)

Scope

Read side (parse the advertised capability) is already partly there via capability.List. The asks:

  1. Advertise-side parsing: expose the symref-update capability in the parsed advertisement so clients can negotiate it conditionally.
  2. Encoder: add a way to attach symref-update commands to packp.UpdateRequests. Sketch:
1
2
3
4
5
6

req := packp.NewUpdateRequests()
req.SymrefUpdates = append(req.SymrefUpdates, &packp.SymrefUpdate{
       Name:       plumbing.HEAD,
       NewTarget:  plumbing.NewBranchReferenceName("main"),
       OldTarget:  "", // optional lease check
})

The encoder emits the symref-update HEAD refs/heads/main\n pkt-line in the update-request body when the capability is in req.Capabilities. 3. Report-status decoder: extend per-ref status to surface ok/ng for symref updates the same way regular ref updates work.

Out of scope

References

Want me to tighten any section, or add a concrete API/wire-format reference link?

The symref-update capability does not exist in git's receive-pack wire protocol.

Evidence (from git/git source):

No git server advertises or accepts symref-update on the wire. Any client-side implementation would be talking to a feature no server implements.

2mo ago·45.4s

You're right — I conflated git update-ref --stdin's symref-update (a local CLI feature for the ref backend) with a wire-protocol extension that doesn't exist. The protocol grammar in gitprotocol-pack.adoc only allows create/delete/update, and queue_command() rejects anything else. There's no symref-update capability advertised by any server because no server implements one. My earlier confidence on this was wrong.

That changes the picture pretty fundamentally:

I made the same wrong claim in several places that need cleanup:

  1. PR #50 description — "We're filing that upstream separately" and the whole symref-update discussion in "What this doesn't do".
  2. docs/usage.md — "The only portable wire-level mechanism is the newer symref-update receive-pack capability, which go-git's current alpha doesn't implement."
  3. Commit messages in this branch — 03819d2e, e470b83d, c0dde3e2, 5a9df918 all repeat variants of "no go-git symref-update support yet". History is what it is, but the PR text and docs are still mutable.

Want me to push a follow-up commit that fixes the docs + PR description to drop the symref-update framing and frame this as "active propagation isn't a wire-protocol primitive; use the host API or shell into the bare repo"? That's the accurate story.

yes, please clean up the PR :)

2mo ago·2m·6 calls

Cleaned up. PR description now says active propagation is fundamentally not a wire-protocol primitive (with the spec-grounded reasoning: receive-pack commands are create/delete/update only; symref-update is update-ref --stdin, not the wire). No more "filing with go-git" or "go-git's current alpha doesn't ship it" — both were chasing a ghost feature.

The "What this doesn't do — and why it can't" section frames it honestly: detection is the ceiling for any git-protocol client; default-branch changes are out-of-band (provider API or git symbolic-ref over SSH).

Pushed commit eb10e26 updates docs/usage.md with the same correction. Closes #45 added to the PR summary since active propagation isn't a follow-up anymore — there's nothing to follow up on.

eb10e26Docs: correct the "active HEAD propagation needs symref-update" claim\ \ Earlier docs framed active HEAD propagation as blocked on a future\ go-git change for the `symref-update` receive-pack capability. That\ capability doesn't exist in git's wire protocol — gitprotocol-pack.adoc\ defines receive-pack commands as create/delete/update only, and\ receive-pack's command parser rejects anything else. The symref-update\ that exists in git is in `update-ref --stdin`, a local CLI for the ref\ backend.\ \ There is no portable wire-level mechanism for updating a remote symref,\ which is why `git push --mirror` doesn't either. Reframe the docs to\ match: detection is the ceiling for any git-protocol client, and\ manual reconciliation (provider REST API or `git symbolic-ref` over\ SSH) is the supported path, not a v1 placeholder.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 3b713736e9d1+8/-6

so what of 45 did we actually address?

2mo ago·34.5s

Honest accounting:

Issue #45 asks for: propagate the source HEAD symref to the target. The sketch suggested two paths — pushing HEAD as a regular ref-update, or using a symref-update capability.

What this PR ships: detection. Source HEAD shows up in results (it was captured internally but never surfaced), target HEAD shows up via the new upload-pack round-trip (we didn't read it at all before), the mismatch is observable in JSON and human output, docs explain the manual reconciliation paths.

What we did NOT ship from #45's actual ask:

So #45 as written isn't achievable. The closest honest thing on git's wire protocol is what we shipped: make the mismatch programmatically observable so users (or wrappers) can reconcile out-of-band.

Suggested cleanup:

  1. Don't actually close #45 with this PR — it's misleading. Drop "Closes #45" from the description.
  2. Either reword #45 to scope it to detection (then it's closed cleanly), or close it as "wontfix: not possible over wire protocol" with a comment linking the PR and explaining what's observable now.

Which do you want?

I think one thing potentially to do: GitHub usually treats the first pushed branch as the default branch on a new empty repo. So maybe we should rather handle that properly?

2mo ago·52.3s

Good catch — that's actually a real thing we can leverage. GitHub (and several other hosts) sets the default branch to the first branch a fresh repo sees. Today our bootstrap pushes branches in alphabetical order, so on an empty GitHub repo where source HEAD = main, if there's also a master branch in scope, master gets created first and GitHub picks it as default. Wrong outcome.

Proposal: in the bootstrap path (one-shot and batched), reorder push commands so source-HEAD's branch is sent first. Hosts with "first-pushed-wins" semantics (GitHub, GitLab on empty projects) get the right default automatically. Hosts that don't care see no change — ordering within one receive-pack update-request doesn't break anything.

Scope sketch:

  1. One-shot bootstrap (bootstrap.go``Execute): right before convert.PlansToPushCommands(plans), hoist the plan whose TargetRef == p.SourceHeadTarget to the front. Source HEAD target is already on bootstrap.Params.
  2. Batched bootstrap: orderTrunkFirst already runs in planBatches for batched mode, so this is mostly free there. Worth a quick check that branch-batches are emitted in order, which I think they are (sequential PushPack per batch).
  3. Replicate empty-target bootstrap path goes through the same bootstrap.Execute, so it inherits the fix.
  4. No-op gracefully when SourceHeadTarget is empty (detached source HEAD) or its branch isn't among the plans.

Test setup: a fake target that records the order of commands in the receive-pack request and asserts source-HEAD's branch came first.

Docs: add a paragraph to the HEAD section noting that on GitHub-style hosts (default-branch-from-first-push), this Just Works on a fresh repo. On hosts without that behavior (raw bare repos), the manual mitigations still apply.

This actually addresses a meaningful slice of #45 — not "propagate symref over the wire" (which still isn't a wire feature), but "achieve the practical outcome on hosts that have UX for it." Want me to land this in a follow-up branch (smaller PR), or push it onto the current branch before merge?

what gains us the current PR? should we completely scratch it and only do this?

2mo ago·32.5s

Let me actually itemize what the current PR earns vs. what costs it carries:

Carries weight:

Carries cost:

If we do first-pushed-wins instead, what changes:

Honest recommendation: scratch the TargetHEAD piece, keep the SourceHEAD surfacing, and add first-pushed-wins.

Concretely: reduce the current PR to ~3 commits worth of small wins (source HEAD field, CLI rendering, docs that match) + add first-pushed-wins as the substantive change. Drop the DiscoverHEAD function, the goroutine-then-serialization saga, the redirect-mutation fix, the regression test, and the v2 ls-refs server change. The remaining diff would be ~150 lines total instead of ~700, with the actual user benefit (correct default on GitHub bootstraps) included.

Want me to:

  1. Rewrite this branch — squash to source-HEAD-only + first-pushed-wins, force-push.
  2. Close this PR and start fresh — soph/head-first-pushed from main, do both pieces clean.
  3. Keep both — current PR + a follow-up. Pays the RTT cost but ships sooner.

I'd pick (2) if you want history clean, (1) if you don't mind a force-push. (3) is the "ship and iterate" path but it locks in the unnecessary round-trip.

do 2

2mo ago·10h 26m·77 calls

PR opened: https://github.com/entireio/git-sync/pull/51

Four commits, ~280 lines total:

The substantive change is one ~25-line helper in bootstrap.go that reorders the push commands. The integration test asserts master beats alpha in the push order even though alpha sorts earlier. No per-session round-trip overhead, no extra target connection state, no concurrency dance — just an in-memory reorder right before PushPack.

Closes #45 for the GitHub/GitLab common case; for raw bare repo targets the docs spell out the two manual mitigations (init-time --initial-branch or post-sync git symbolic-ref / host API).

cc9e564test server: emit HEAD symref-target under v2 ls-refs symrefs\ \ The v2 ls-refs handler ignored the "symrefs" argument and never emitted\ HEAD with a symref-target attribute, so source-side HEAD info captured\ via v2 was always empty in tests. Real git emits\ \ HEAD symref-target:\n\ \ when the client requests "symrefs" and a ref-prefix would include HEAD\ (or no prefixes are given). Add that to the test server with two small\ helpers: coversHead checks prefix coverage, lsRefsHeadLine formats the\ pkt-line by resolving HEAD's symref target through the repo.\ \ Foundation for upcoming work on surfacing source HEAD.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 66bf19cf9e23+40

8ce7a4aSurface source HEAD symref in Result and ProbeResult\ \ The session already captures source HEAD's symref target during ref\ discovery (it's used by bootstrap-batch trunk ordering), but never\ escapes to callers. Expose it on syncer.Result.SourceHEAD,\ syncer.ProbeResult.SourceHEAD, and the matching internalbridge fields,\ so library callers and the CLI can compare against the target's\ intended default branch without out-of-band metadata.\ \ Human Lines() output picks up a "source-head: " line; JSON output\ gets `execution.sourceHead` for sync paths and `sourceHead` on\ probe. Field is `omitempty` so detached-HEAD sources omit it.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 1d796b864aa7+108/-29

c25015dDocs: HEAD / default branch section\ \ Adds a "HEAD / Default Branch" section to usage.md covering:\ - where the source HEAD shows up (JSON and human output)\ - bootstrap's first-pushed-wins ordering (the new behavior) and which\ hosts benefit from it (GitHub, GitLab on fresh repos)\ - the wire-protocol limit ("no command for updating a remote symref")\ and the manual workarounds for hosts that don't infer from first push:\ match the default at init time, or set HEAD post-sync via the host\ API or `git symbolic-ref`\ \ JSON Output section adds `sourceHead` to the documented keys.\ \ Refs #45.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: cd3eea2ac753+26

Simplify: Code Review and Cleanup

Review all changed files for reuse, quality, and efficiency. Fix any issues found.

Phase 1: Identify Changes

Phase 2: Launch Three Review Agents in Parallel

Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.

Agent 1: Code Reuse Review

For each change:

Agent 2: Code Quality Review

Review the same changes for hacky patterns:

Agent 3: Efficiency Review

Review the same changes for efficiency:

Phase 3: Fix Issues

When done, briefly summarize what was fixed (or confirm the code was already clean).

2mo ago·1h 43m·17 calls

Pushed. Net -12 lines in the cleanup commit:

Real fixes:

  1. hoistFirstMatch[T] generic replaces near-duplicate logic between hoistSourceHeadCommand (PushCommand) and orderTrunkFirst (DesiredRef). Same move-to-front algorithm, two slice types, one implementation.
  2. Dropped (s *syncSession) sourceHead() wrapper — every caller already dereferences s.sourceService directly nearby for other fields, so the nil guard was dead code. Inlined as s.sourceService.HeadTarget at six construction sites.
  3. Use syncertest.SetRefAtBranch instead of raw SetReference in the bootstrap-ordering test, matching how that helper is used 9+ other places in the file.
  4. Trimmed Result docstring — the second sentence was rationale that belongs in the changelog, not on the struct.

Skipped per agent guidance:

PR #51 picks up the cleanup on the next CI run.

b251f1fSimplify after review: dedupe hoist helpers, inline trivial wrapper\ - Extract hoistFirstMatch[T] generic and use it for both the new\ hoistSourceHeadCommand (PushCommand slice) and the existing\ orderTrunkFirst (DesiredRef slice). Same move-to-front-by-name\ algorithm in both; one implementation now.\ - Drop syncSession.sourceHead() wrapper. The nil guard was dead code:\ every caller already dereferences s.sourceService directly nearby\ (.Protocol, .Capabilities() in the same Result builders). Inline as\ s.sourceService.HeadTarget at the six construction sites.\ - Use syncertest.SetRefAtBranch instead of raw SetReference in the\ bootstrap-ordering integration test, matching the existing helper's\ use elsewhere in this file.\ - Trim Result struct docstring trailer — the field's own line is enough.\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: af47d0d2234c+38/-50

can you look at the cursor pr comment?

2mo ago·2m·11 calls

All four pass — including the new mapping regression and the original test. Full suite + lint.

833b123Match hoist on SourceRef so --map remappings push the right target first\ \ Cursor caught a real bug: hoistSourceHeadCommand compared cmd.Name\ (which PlansToPushCommands sets from p.TargetRef) against sourceHEAD\ (the source-side symref target). Under --map (e.g., master→stable),\ target ref name diverges from source — the match silently fails and\ the bootstrap pushes branches in alphabetical order, defeating the\ fix on the very setups that need it most (custom branch naming on\ mirror).\ \ Rework to operate on plans instead of commands, matching SourceRef\ against sourceHEAD. The mapped TargetRef inherits its position in the\ plans slice, so PlansToPushCommands emits commands in the right\ order. Aligns with executeBatched's orderTrunkFirst which already\ keys on DesiredRef.SourceRef.\ \ Regression test pins --map master:stable + a 1:1 alpha mapping: the\ target server's first command must be refs/heads/stable. Without the\ fix, the bootstrap pushes alpha first. Unit test gains a case\ asserting "matches on SourceRef, hoists mapped TargetRef".\ \ Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 81dd3a769cb7+91/-42