Round out other-kind ref semantics and unit-test the rejection logic

75deaf4→main·

Soph·2mo ago·7 files·+211 added/-20 removed

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

Sessions

Changes

7

• cmd/git-sync

• Mfetch.go+1/-1

  • Mprobe.go+1/-1

• docs

  • Musage.md+6

• internal

• planner

  • Mplanner.go+8/-3
  • Mrelay.go+10

• syncer

  • Mintegration_test.go+116/-15
  • Msyncer_test.go+69

68 unmodified lines

68 unmodified lines

cmd.Flags().StringVar(&branches, "branch", "", "comma-separated branch list; default is all source branches")
    cmd.Flags().BoolVar(&req.IncludeTags, "tags", false, "include tags in the fetch request")
    cmd.Flags().BoolVar(&req.Scope.AllRefs, "all-refs", false, "include every refs/* on the source (notes, pulls, custom namespaces) in the fetch request")
    cmd.Flags().BoolVar(&req.Scope.AllRefs, "all-refs", false, "include every refs/* on the source (branches, tags, notes, pulls, custom namespaces) in the fetch request")
    addProtocolFlag(cmd, &protocolVal)
    cmd.Flags().BoolVar(&req.Options.CollectStats, "stats", false, "print transfer statistics")
    cmd.Flags().BoolVar(&req.Options.MeasureMemory, "measure-memory", false, "sample elapsed time and Go heap usage")

Mcmd/git-sync/fetch.go+1/-1

64 unmodified lines

64 unmodified lines
    addTargetAuth(cmd, &targetAuth)
    cmd.Flags().BoolVar(&req.IncludeTags, "tags", false, "include tag ref prefixes in probe")
    cmd.Flags().BoolVar(&req.AllRefs, "all-refs", false, "advertise all refs/* prefixes (notes, pulls, custom namespaces) in the probe")
    cmd.Flags().BoolVar(&req.AllRefs, "all-refs", false, "advertise all refs/* prefixes (branches, tags, notes, pulls, custom namespaces) in the probe")
    addProtocolFlag(cmd, &protocolVal)
    cmd.Flags().BoolVar(&req.Options.CollectStats, "stats", false, "print transfer statistics")
    cmd.Flags().BoolVar(&req.Options.MeasureMemory, "measure-memory", false, "sample elapsed time and Go heap usage")

Mcmd/git-sync/probe.go+1/-1

182 unmodified lines

182 unmodified lines

which contradicts the command. Use `sync --all-refs` if you want
best-effort completeness against hostile targets.

`sync --all-refs` blocks updates to non-branch refs (notes, pulls, custom
namespaces) by default — those refs don't generally form fast-forward
chains, so the same `--force` opt-in that retargets tags is required to
update them. `replicate` doesn't run that check; its overwrite contract
covers other-kind refs without `--force`.

`SyncPolicy.BestEffort` is independent of scope and can be set without
`AllRefs` if a library caller wants per-ref warn semantics on a narrower
scope.

Mdocs/usage.md+6

300 unmodified lines

300 unmodified lines return plan, nil }

if want.Kind == RefKindTag { // Tags and other-kind refs (notes, pulls, custom namespaces) don't // generally form fast-forward chains — a notes append creates a new // commit that isn't an ancestor of the previous notes tip. Treat // them the same way: require --force to retarget rather than // running an ancestry check that would always fail. if want.Kind == RefKindTag || want.Kind == RefKindOther { if force { plan.Action = ActionUpdate plan.Reason = ShortHash(targetHash) + " -> " + ShortHash(want.SourceHash) + " (force tag update)" plan.Reason = ShortHash(targetHash) + " -> " + ShortHash(want.SourceHash) + " (force " + string(want.Kind) + " update)" return plan, nil } plan.Action = ActionBlock plan.Reason = ShortHash(targetHash) + " differs from " + ShortHash(want.SourceHash) + "; use --force to retarget tag" plan.Reason = ShortHash(targetHash) + " differs from " + ShortHash(want.SourceHash) + "; use --force to update " + string(want.Kind) + " ref " + want.TargetRef.String() return plan, nil }

Minternal/planner/planner.go+8/-3

161 unmodified lines

161 unmodified lines if plan.Action != ActionCreate && plan.Action != ActionUpdate { return false, "replicate-tag-action-not-create-or-update" } case RefKindOther: // Replicate's contract is overwrite, so the FF concern that keeps // other-kind refs out of the sync incremental relay doesn't apply // here — a notes/pull ref update is just another ref-update relay. if RefKindFromName(plan.SourceRef) != RefKindOther || RefKindFromName(plan.TargetRef) != RefKindOther { return false, "replicate-non-other-mapping" } if plan.Action != ActionCreate && plan.Action != ActionUpdate { return false, "replicate-other-action-not-create-or-update" } default: return false, "replicate-unsupported-ref-kind" }`

Minternal/planner/relay.go+10

3049 unmodified lines

3049 unmodified lines

// Other-kind refs don't have FF semantics (a notes append is rarely an // ancestor of the previous notes tip), so PlanRef requires --force to // retarget them — same as tags. This pins the block reason and the // successful update under --force. func TestRun_IntegrationAllRefsSyncOtherKindUpdateRequiresForce(t *testing.T) { sourceRepo, sourceFS := newSourceRepo(t) makeCommits(t, sourceRepo, sourceFS, 1) firstHead, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true) if err != nil { t.Fatalf("resolve first head: %v", err) } notesRef := plumbing.ReferenceName("refs/notes/commits") if err := sourceRepo.Storer.SetReference(plumbing.NewHashReference(notesRef, firstHead.Hash())); err != nil { t.Fatalf("set source notes ref: %v", err) }

targetRepo, err := git.Init(memory.NewStorage()) if err != nil { t.Fatalf("init target repo: %v", err) } sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo) targetServer := newSmartHTTPRepoServer(t, targetRepo) def defer sourceServer.Close() def defer targetServer.Close()

cfg := Config{ Source: Endpoint{URL: sourceServer.RepoURL()}, Target: Endpoint{URL: targetServer.RepoURL()}, ProtocolMode: protocolModeAuto, AllRefs: true, } if _, err := Run(context.Background(), cfg); err != nil { t.Fatalf("initial all-refs sync failed: %v", err) }

// Move the notes ref to a new, non-ancestor commit and try a plain sync. makeCommits(t, sourceRepo, sourceFS, 1) newHead, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true) if err != nil { t.Fatalf("resolve new head: %v", err) } if err := sourceRepo.Storer.SetReference(plumbing.NewHashReference(notesRef, newHead.Hash())); err != nil { t.Fatalf("update source notes ref: %v", err) }

result, err := Run(context.Background(), cfg) if err == nil { t.Fatal("expected sync to block on non-ancestor other-kind update") } var notesPlan *BranchPlan for i := range result.Plans { if result.Plans[i].TargetRef == notesRef { notesPlan = &result.Plans[i] } } if notesPlan == nil { t.Fatalf("expected notes ref plan in result, got %+v", result.Plans) } if notesPlan.Action != ActionBlock { t.Errorf("expected notes ref Action=%s, got %s", ActionBlock, notesPlan.Action) } if !strings.Contains(notesPlan.Reason, "use --force to update other ref") { t.Errorf("expected clear --force-required reason for other-kind ref, got %q", notesPlan.Reason) }

// Same scenario with --force succeeds. cfg.Force = true if _, err := Run(context.Background(), cfg); err != nil { t.Fatalf("force-update of other-kind ref failed: %v", err) } gotNotes, err := targetRepo.Reference(notesRef, true) if err != nil { t.Fatalf("expected refs/notes/commits on target: %v", err) } if gotNotes.Hash() != newHead.Hash() { t.Fatalf("target notes hash = %s, want %s", gotNotes.Hash(), newHead.Hash()) } }

// Pure-prune replicate runs (no source-side updates) must actually delete // the orphaned ref. The runReplicate gate previously required at least one // relay plan, so delete-only scenarios silently no-op'd; this pins the 92 unmodified lines

}

// Pins a v1 limitation: replicate is relay-only, so other-kind refs into a // non-empty target error out (sync handles it via materialized fallback). func TestRun_IntegrationAllRefsReplicateRejectsOtherKindIntoExistingTarget(t *testing.T) { // Replicate's relay covers other-kind refs (notes, pulls, custom namespaces) // just like branches and tags — the overwrite semantics make the // fast-forward concern that keeps them out of incremental sync relay // irrelevant here. This pins idempotent re-runs: replicate --all-refs // must keep working when a notes ref updates between runs. func TestRun_IntegrationAllRefsReplicateUpdatesOtherKindOnSecondRun(t *testing.T) { sourceRepo, sourceFS := newSourceRepo(t) makeCommits(t, sourceRepo, sourceFS, 2)

head, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true) firstHead, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true) if err != nil { t.Fatalf("resolve source head: %v", err) t.Fatalf("resolve first source head: %v", err) } notesRef := plumbing.ReferenceName("refs/notes/commits") if err := sourceRepo.Storer.SetReference(plumbing.NewHashReference(notesRef, head.Hash())); err != nil { if err := sourceRepo.Storer.SetReference(plumbing.NewHashReference(notesRef, firstHead.Hash())); err != nil { t.Fatalf("set source notes ref: %v", err) }

1 unmodified line

if err != nil { t.Fatalf("init target repo: %v", err) } if err := copyRefsAndObjects(sourceRepo.Storer, targetRepo.Storer, []plumbing.ReferenceName{plumbing.NewBranchReferenceName(testBranch)}); err != nil { t.Fatalf("copy target baseline: %v", err) }

sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo) targetServer := newSmartHTTPRepoServer(t, targetRepo) def defer sourceServer.Close() def defer targetServer.Close()

_, err = Run(context.Background(), Config{ cfg := Config{ Source: Endpoint{URL: sourceServer.RepoURL()}, Target: Endpoint{URL: targetServer.RepoURL()}, ProtocolMode: protocolModeAuto, Mode: modeReplicate, AllRefs: true, }) if err == nil { t.Fatal("expected replicate to reject other-kind refs in non-empty target") } if !strings.Contains(err.Error(), "use sync instead") { t.Fatalf("expected error to direct user to sync, got %v", err) if _, err := Run(context.Background(), cfg); err != nil { t.Fatalf("first replicate --all-refs failed: %v", err) }

// Move the notes ref forward on the source and run replicate again — // this used to fail with "replicate-unsupported-ref-kind". makeCommits(t, sourceRepo, sourceFS, 1) updatedHead, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true) if err != nil { t.Fatalf("resolve updated source head: %v", err) } if err := sourceRepo.Storer.SetReference(plumbing.NewHashReference(notesRef, updatedHead.Hash())); err != nil { t.Fatalf("update source notes ref: %v", err) } result, err := Run(context.Background(), cfg) if err != nil { t.Fatalf("second replicate --all-refs failed: %v", err) } if !result.Relay { t.Errorf("expected relay path on second replicate, got Relay=false RelayMode=%q", result.RelayMode) } gotNotes, err := targetRepo.Reference(notesRef, true) if err != nil { t.Fatalf("expected refs/notes/commits on target: %v", err) } if gotNotes.Hash() != updatedHead.Hash() { t.Fatalf("target notes hash = %s, want %s", gotNotes.Hash(), updatedHead.Hash()) } } }