From d71f3061bfd527a8baadbe84d7dedf6ff433111a Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 16:26:22 +0200 Subject: [PATCH 01/10] Separate an empty source from an unmatched scope, and let a verified-empty source converge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replicate failed every run whose planning produced no desired refs, with one message covering two unrelated conditions: the source has no refs, and the source has refs that the requested scope excluded. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date, yet it read as an error forever. SyncPolicy.AllowEmptySource (off by default) opts into the distinction. With it set, Replicate reports ErrNoRefsSelected when the source does advertise refs, ErrSourceEmptyUnverified when it advertised none but never confirmed it is empty, ErrSourceEmptyTargetPopulated when it is confirmed empty while the target still holds refs, and a zero-plan success carrying ExecutionSummary.SourceEmpty when source and target are both empty and therefore already agree. Emptiness is established from what the source asserts, never inferred from a response that merely carried no refs. git-sync now requests protocol v2's ls-refs=unborn where the server advertises it, so a repository with no commits answers with an explicit "unborn HEAD" line; only that assertion, under an all-refs scope, qualifies. The distinction is the point: a blank body behind a valid header, a server-side ref-listing or hide-pattern regression, or a narrowed ref-prefix all produce the same silence as an empty repository, and a caller acting on silence would act on every affected repository at once. The unborn line's symref-target is deliberately not surfaced as SourceHEAD, which consumers read as a branch that exists. The divergent case refuses rather than converging. Converging means deleting every ref on the target, and the states that produce that signature — a source restored from backup, a wiped data plane, an out-of-band emptying — are the ones where the target may hold the only surviving copy. The opt-in gate is checked first, so "off" is structurally identical to the behavior that predates this and not merely identical in the cases someone thought to test: a caller that has not opted in cannot receive a sentinel it has never heard of. The sentinels' messages deliberately avoid the historical "no source refs matched" text, so a caller that substring-matches that phrase cannot read one as the other and the order the checks run in is not load-bearing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01714HJZAqpgwuwp6fcMWEhG Entire-Checkpoint: 01M0JBEEG33N57NPRZ1MAT4DY6 --- CHANGELOG.md | 6 ++ client.go | 1 + errors.go | 37 ++++++- internal/gitproto/capability.go | 19 +++- internal/gitproto/capability_test.go | 33 ++++++ internal/gitproto/fetch_test.go | 48 ++++++++- internal/gitproto/refs.go | 67 +++++++++--- internal/syncer/empty_source.go | 112 ++++++++++++++++++++ internal/syncer/empty_source_test.go | 146 +++++++++++++++++++++++++++ internal/syncer/syncer.go | 13 ++- results.go | 7 ++ types.go | 13 +++ 12 files changed, 480 insertions(+), 22 deletions(-) create mode 100644 internal/syncer/empty_source.go create mode 100644 internal/syncer/empty_source_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b00e9d6a..fae2d169 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Added +- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when the source says so itself.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering two unrelated conditions: the source has no refs, and the source has refs that the requested scope excluded. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports the three cases separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` when it advertised none but never confirmed it is empty, `ErrSourceEmptyTargetPopulated` when it is confirmed empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.SourceEmpty` when source and target are both empty and therefore already agree. + + Emptiness is established from what the source asserts, never inferred from a response that merely carried no refs. git-sync now requests protocol v2's `ls-refs=unborn` feature where the server advertises it, so a repository with no commits answers with an explicit `unborn HEAD` line; only that assertion, under an all-refs scope, qualifies. The distinction is the point: a blank body behind a valid header, a server-side ref-listing or hide-pattern regression, or a narrowed ref-prefix all produce the same silence as an empty repository, and a caller acting on silence would act on every affected repository at once. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. + + Off by default, so nothing changes for existing callers: without the opt-in — or under a narrower scope, where an empty desired set says nothing about the repository as a whole — an empty source still fails with the historical message, and the new sentinels deliberately do not carry that text so a caller still matching on it cannot mistake a divergence for the old benign no-op. + - A `Vulnerability Scan` workflow running `govulncheck ./...` on pull requests, pushes to main, and a weekly schedule. The existing lint suite cannot see this class of issue, and the weekly run matters because advisories are published against versions already in go.mod — without it, a newly disclosed vulnerability goes unreported until someone happens to open a PR. ## [0.8.0] - 2026-07-09 diff --git a/client.go b/client.go index c05ac652..a1b99634 100644 --- a/client.go +++ b/client.go @@ -131,6 +131,7 @@ func (c *Client) buildSyncConfig(ctx context.Context, req SyncRequest, dryRun bo ForceBlind: req.Policy.ForceBlind, Prune: req.Policy.Prune, BestEffort: req.Policy.BestEffort, + AllowEmptySource: req.Policy.AllowEmptySource, ProtocolMode: string(req.Policy.Protocol), MaterializedMaxObjects: syncer.DefaultMaterializedMaxObjects, }, nil diff --git a/errors.go b/errors.go index 6b5686aa..13c69e85 100644 --- a/errors.go +++ b/errors.go @@ -1,6 +1,9 @@ package gitsync -import "entire.io/entire/git-sync/internal/gitproto" +import ( + "entire.io/entire/git-sync/internal/gitproto" + "entire.io/entire/git-sync/internal/syncer" +) // ErrTargetRefMoved is returned (wrapped) by Sync and Replicate when a push was // rejected because the target ref changed concurrently between this run's plan @@ -24,3 +27,35 @@ var ErrTargetRefMoved = gitproto.ErrTargetRefMoved // and Reason is the raw server reason text. Rejections that are concurrent // target-ref moves also satisfy errors.Is(err, ErrTargetRefMoved). type RefRejectedError = gitproto.RefRejectedError + +// ErrNoRefsSelected is returned (wrapped) by Replicate when the source +// advertises refs but the requested scope — branch selection, ref mappings, +// exclude prefixes — matched none of them. The source is healthy; the request +// asked for refs it does not have. Benign for some sources by design: a GitHub +// repository whose only refs are under refs/pull/* selects nothing once that +// namespace is excluded. Test for it with errors.Is. +// +// It is deliberately distinct from the empty-source errors below, which mean +// the source has no refs AT ALL. Before these existed both cases shared one +// message and callers could not tell "nothing to mirror" from "nothing +// matched". +var ErrNoRefsSelected = syncer.ErrNoRefsSelected + +// ErrSourceEmptyUnverified is returned (wrapped) by Replicate under +// SyncPolicy.AllowEmptySource when the source advertised no refs but never +// confirmed that it is empty — no protocol v2 unborn-HEAD assertion. The +// response's silence has several possible causes besides an empty repository +// (a blank body behind a valid header, a server-side ref-listing or +// hide-pattern regression), so the state is reported as unknown rather than +// converged. Test for it with errors.Is. +var ErrSourceEmptyUnverified = syncer.ErrSourceEmptyUnverified + +// ErrSourceEmptyTargetPopulated is returned (wrapped) by Replicate under +// SyncPolicy.AllowEmptySource when the source is confirmed empty while the +// target still holds refs — a real divergence, since nothing the target serves +// exists on the source. Replicate refuses instead of converging: converging +// means deleting every ref on the target, and the states that produce this +// signature (a source restored from backup, a wipe, an out-of-band emptying) +// are the ones where the target may hold the only surviving copy. Test for it +// with errors.Is, and surface it as divergence rather than as "nothing to do". +var ErrSourceEmptyTargetPopulated = syncer.ErrSourceEmptyTargetPopulated diff --git a/internal/gitproto/capability.go b/internal/gitproto/capability.go index a203d3b0..d5f6851c 100644 --- a/internal/gitproto/capability.go +++ b/internal/gitproto/capability.go @@ -35,10 +35,27 @@ func (c *V2Capabilities) Value(name string) string { // FetchSupports checks whether a specific feature is listed in the // "fetch" capability value (space-separated feature list). func (c *V2Capabilities) FetchSupports(feature string) bool { + return c.commandSupports("fetch", feature) +} + +// LSRefsSupports checks whether a specific feature is listed in the "ls-refs" +// capability value (space-separated feature list). The only feature defined +// today is "unborn": a server advertising it will report an unborn HEAD as an +// explicit "unborn HEAD symref-target:" line, which is the difference +// between a repository asserting it has no commits and a response that merely +// carries no ref lines. Protocol v2 forbids sending a command argument the +// server did not advertise, so callers MUST gate the request on this. +func (c *V2Capabilities) LSRefsSupports(feature string) bool { + return c.commandSupports("ls-refs", feature) +} + +// commandSupports reports whether feature appears in command's advertised +// space-separated feature list. +func (c *V2Capabilities) commandSupports(command, feature string) bool { if c == nil { return false } - for _, f := range strings.Fields(c.Value("fetch")) { + for _, f := range strings.Fields(c.Value(command)) { if f == feature { return true } diff --git a/internal/gitproto/capability_test.go b/internal/gitproto/capability_test.go index 62dc3eb1..5ce81fa3 100644 --- a/internal/gitproto/capability_test.go +++ b/internal/gitproto/capability_test.go @@ -42,6 +42,39 @@ func TestV2CapabilitiesFetchSupports(t *testing.T) { } } +func TestV2CapabilitiesLSRefsSupports(t *testing.T) { + caps := &V2Capabilities{ + Caps: map[string]string{ + "ls-refs": "unborn", + "fetch": "thin-pack", + }, + } + if !caps.LSRefsSupports("unborn") { + t.Error(`LSRefsSupports("unborn") = false, want true`) + } + // Features are per-command: ls-refs must not answer for fetch's list, or + // we would send an argument the server never advertised for this command. + if caps.LSRefsSupports("thin-pack") { + t.Error(`LSRefsSupports("thin-pack") = true; fetch features must not leak into ls-refs`) + } + if caps.FetchSupports("unborn") { + t.Error(`FetchSupports("unborn") = true; ls-refs features must not leak into fetch`) + } + + // A server advertising ls-refs with no feature list supports no features + // — the common case for older servers, and the one that must keep the + // unborn argument off the wire. + bare := &V2Capabilities{Caps: map[string]string{"ls-refs": ""}} + if bare.LSRefsSupports("unborn") { + t.Error("bare ls-refs advertisement reported unborn support") + } + + var nilCaps *V2Capabilities + if nilCaps.LSRefsSupports("unborn") { + t.Error("nil V2Capabilities.LSRefsSupports should return false") + } +} + func TestV2CapabilitiesSortedKeys(t *testing.T) { caps := &V2Capabilities{ Caps: map[string]string{ diff --git a/internal/gitproto/fetch_test.go b/internal/gitproto/fetch_test.go index c6f345e9..804443ca 100644 --- a/internal/gitproto/fetch_test.go +++ b/internal/gitproto/fetch_test.go @@ -194,7 +194,7 @@ func TestDecodeV2LSRefs(t *testing.T) { FormatPktLine("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb refs/heads/dev\n") + "0000" // flush - refs, head, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + refs, head, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) if err != nil { t.Fatalf("decodeV2LSRefs: %v", err) } @@ -220,7 +220,7 @@ func TestDecodeV2LSRefsHeadSymref(t *testing.T) { FormatPktLine("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa HEAD symref-target:refs/heads/main\n") + FormatPktLine("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\n") + "0000" - refs, head, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + refs, head, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) if err != nil { t.Fatalf("decodeV2LSRefs: %v", err) } @@ -241,7 +241,10 @@ func TestDecodeV2LSRefsSkipsUnbornLines(t *testing.T) { FormatPktLine("unborn HEAD symref-target:refs/heads/main\n") + FormatPktLine("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\n") + "0000" - refs, head, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + refs, head, unborn, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + if !unborn { + t.Error("unborn = false, want true: the response carried an unborn HEAD line") + } if err != nil { t.Fatalf("decodeV2LSRefs: %v", err) } @@ -262,7 +265,7 @@ func TestDecodeV2LSRefsSkipsUnbornLines(t *testing.T) { func TestDecodeV2LSRefsMalformed(t *testing.T) { // Line with only one field (no refname). wire := FormatPktLine("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n") + "0000" - _, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + _, _, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) if err == nil { t.Fatal("expected error for malformed ls-refs line, got nil") } @@ -271,7 +274,7 @@ func TestDecodeV2LSRefsMalformed(t *testing.T) { func TestDecodeV2LSRefsEmpty(t *testing.T) { // Empty response (just flush). wire := "0000" - refs, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + refs, _, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) if err != nil { t.Fatalf("decodeV2LSRefs: %v", err) } @@ -280,6 +283,41 @@ func TestDecodeV2LSRefsEmpty(t *testing.T) { } } +// An unborn HEAD is the whole point of requesting the capability: the +// repository asserts it has no commits, which a caller may act on, while the +// symref target stays out of HeadTarget because that ref does not exist. +func TestDecodeV2LSRefsUnbornOnly(t *testing.T) { + wire := FormatPktLine("unborn HEAD symref-target:refs/heads/main\n") + "0000" + refs, head, unborn, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + if err != nil { + t.Fatalf("decodeV2LSRefs: %v", err) + } + if len(refs) != 0 { + t.Fatalf("expected 0 refs, got %d", len(refs)) + } + if !unborn { + t.Error("unborn = false, want true") + } + if head != "" { + t.Errorf("head target = %q, want empty: an unborn target is not a ref that exists", head) + } +} + +// A response carrying no lines at all must NOT read as "the repository is +// empty" — that is the ambiguity the unborn capability exists to remove. +func TestDecodeV2LSRefsEmptyIsNotUnborn(t *testing.T) { + refs, _, unborn, _, err := decodeV2LSRefs(bytes.NewReader([]byte("0000"))) + if err != nil { + t.Fatalf("decodeV2LSRefs: %v", err) + } + if len(refs) != 0 { + t.Fatalf("expected 0 refs, got %d", len(refs)) + } + if unborn { + t.Error("unborn = true for a response with no lines; silence must not assert emptiness") + } +} + func TestBufReader(t *testing.T) { input := bytes.NewBufferString("test data") pr := NewPacketReader(input) diff --git a/internal/gitproto/refs.go b/internal/gitproto/refs.go index d70846c9..115961fc 100644 --- a/internal/gitproto/refs.go +++ b/internal/gitproto/refs.go @@ -34,6 +34,20 @@ type RefService struct { // objects", "Compressing objects", ...) to stderr and asks the source // upload-pack to emit progress by not sending the no-progress option. Verbose bool + // SourceUnborn is true when the source ASSERTED that its HEAD is unborn + // — i.e. the repository exists but has no commits yet. It is positive + // evidence of emptiness, not an inference: only a v2 source advertising + // ls-refs=unborn (which we then request) can set it, and it arrives as + // an explicit "unborn HEAD" line rather than as the absence of ref + // lines. Callers that must not confuse "this repo is empty" with "this + // response carried no refs" (a blank proxy body, a ref-listing or + // hide-pattern regression, a narrowed prefix) gate on this rather than + // on len(refs) == 0. + // + // False therefore means "not asserted", NOT "the source has commits": + // a v1 source, or a v2 source that does not advertise the feature, + // leaves it false however empty it is. + SourceUnborn bool } // ListSourceRefs discovers refs from the source using the configured protocol mode. @@ -63,11 +77,11 @@ func ListSourceRefs(ctx context.Context, conn Conn, protocolMode string, refPref if !caps.Supports("ls-refs") || !caps.Supports("fetch") { return nil, nil, errors.New("source does not advertise required protocol v2 commands") } - refs, headTarget, err := listSourceRefsV2(ctx, conn, caps, refPrefixes) + refs, headTarget, unborn, err := listSourceRefsV2(ctx, conn, caps, refPrefixes) if err != nil { return nil, nil, err } - return refs, &RefService{Protocol: "v2", V2Caps: caps, HeadTarget: headTarget}, nil + return refs, &RefService{Protocol: "v2", V2Caps: caps, HeadTarget: headTarget, SourceUnborn: unborn}, nil } if protocolMode == "v2" { return nil, nil, errors.New("source did not negotiate protocol v2") @@ -177,54 +191,79 @@ func listSourceRefsV1(ctx context.Context, conn Conn) (*packp.AdvRefs, []*plumbi return adv, refs, nil } -func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, prefixes []string) ([]*plumbing.Reference, plumbing.ReferenceName, error) { +func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, prefixes []string) ([]*plumbing.Reference, plumbing.ReferenceName, bool, error) { // Always include "HEAD" so the server returns the symref-target attribute // for HEAD. Without this, callers that pass only "refs/heads/" or // "refs/tags/" prefixes filter HEAD out of the response and lose the // default-branch hint that bootstrap planning uses as a trunk cutoff. args := []string{"peel", "symrefs", "ref-prefix HEAD"} + // Ask the server to report an unborn HEAD explicitly, so a repository + // with no commits says so instead of answering with no ref lines at all. + // Gated on the advertisement: protocol v2 forbids sending an argument the + // server did not advertise, and a stricter server may fail the command. + // Costs nothing — no extra round trip, and a source WITH commits answers + // exactly as before. + if caps.LSRefsSupports("unborn") { + args = append(args, "unborn") + } for _, prefix := range prefixes { args = append(args, "ref-prefix "+prefix) } body, err := EncodeCommand("ls-refs", caps.RequestCapabilities(), args) if err != nil { - return nil, "", err + return nil, "", false, err } data, err := PostRPC(ctx, conn, transport.UploadPackService, body, true, "upload-pack ls-refs") if err != nil { - return nil, "", err + return nil, "", false, err } - refs, headTarget, skipped, err := decodeV2LSRefs(bytes.NewReader(data)) + refs, headTarget, unborn, skipped, err := decodeV2LSRefs(bytes.NewReader(data)) if err != nil { - return nil, "", err + return nil, "", false, err } WarnSkippedRefNames(conn.ProgressWriter(), "source", skipped) - return refs, headTarget, nil + return refs, headTarget, unborn, nil } -func decodeV2LSRefs(r *bytes.Reader) ([]*plumbing.Reference, plumbing.ReferenceName, []string, error) { +// decodeV2LSRefs decodes an ls-refs response into its refs, the branch HEAD +// points at, whether the server reported HEAD as unborn, and the ref names +// dropped as invalid for the caller to report. +func decodeV2LSRefs(r *bytes.Reader) ([]*plumbing.Reference, plumbing.ReferenceName, bool, []string, error) { reader := NewPacketReader(r) var refs []*plumbing.Reference var headTarget plumbing.ReferenceName + var unborn bool for { kind, payload, err := reader.ReadPacket() if err != nil { - return nil, "", nil, err + return nil, "", false, nil, err } if kind == PacketFlush { // Names come from the remote, so drop anything git would reject - // and hand the list back for the caller to report. + // and hand the list back for the caller to report. unborn rides + // alongside rather than through this filter: it is a property of + // the repository, not a ref, and the line that carries it is + // consumed below without ever entering refs. valid, skipped := PartitionRefNames(refs) - return valid, headTarget, skipped, nil + return valid, headTarget, unborn, skipped, nil } if kind != PacketData { - return nil, "", nil, fmt.Errorf("unexpected packet type %v in ls-refs response", kind) + return nil, "", false, nil, fmt.Errorf("unexpected packet type %v in ls-refs response", kind) } fields := strings.Fields(strings.TrimSpace(string(payload))) if len(fields) < 2 { - return nil, "", nil, fmt.Errorf("malformed ls-refs response line %q", payload) + return nil, "", false, nil, fmt.Errorf("malformed ls-refs response line %q", payload) } if fields[0] == "unborn" { + // "unborn HEAD symref-target:refs/heads/main": the repository has + // no commits. Recorded as a flag only — headTarget deliberately + // stays empty, because consumers treat a non-empty HeadTarget as + // a branch that exists on the source (mirror-pipeline's + // default-branch reconcile drives SetDefaultBranch off it), and + // an unborn target does not. + if fields[1] == string(plumbing.HEAD) { + unborn = true + } continue } hash := plumbing.NewHash(fields[0]) diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go new file mode 100644 index 00000000..aec2bdba --- /dev/null +++ b/internal/syncer/empty_source.go @@ -0,0 +1,112 @@ +package syncer + +import ( + "errors" + "fmt" +) + +// The empty-desired-set outcomes. Replicate reaches this family whenever +// planning produces no desired refs, which happens for two unrelated reasons +// that the wire signal alone cannot tell apart: the source has no refs, or the +// source has refs and the requested scope excluded all of them. Collapsing +// them into one error (as this package did before) forces every caller to +// guess, and the two demand opposite handling — one may be a converged state, +// the other never is. +var ( + // ErrNoRefsSelected means the source DOES advertise refs, but the + // requested scope (branch selection, mappings, exclude prefixes) matched + // none of them. Nothing is wrong with the source; the request asked for + // refs it does not have. Benign and expected for some sources — e.g. a + // GitHub repository whose only refs live under refs/pull/*, which mirror + // callers deliberately exclude. + // + // Its message deliberately avoids the historical "no source refs matched" + // text. A caller that substring-matches that phrase (mirror-pipeline does, + // to stay correct across a vendor bump) must not be able to read one of + // these as the other, and keeping the texts disjoint means the order the + // checks run in is not load-bearing. + ErrNoRefsSelected = errors.New("source has refs but none matched the requested scope") + + // ErrSourceEmptyUnverified means planning found no desired refs AND the + // source advertised no refs, but nothing asserted that the source is + // actually empty, so the emptiness cannot be trusted. Absence of refs in + // a response is not the same claim as a repository reporting it has no + // commits: a blank body behind a valid header, a server-side ref-listing + // or hide-pattern regression, or a narrowed ref-prefix all produce the + // same silence. So does ref-name validation dropping every advertised + // name (gitproto.PartitionRefNames) — a repository full of refs whose + // names git would reject arrives here indistinguishable from an empty + // one, which is exactly why the unborn assertion and not the ref count + // is what decides. Callers must treat this as "unknown", never as + // "converged". + ErrSourceEmptyUnverified = errors.New("source advertised no refs but did not confirm it is empty") + + // ErrSourceEmptyTargetPopulated means the source is VERIFIED empty while + // the target still holds refs. The two have genuinely diverged: whatever + // the target serves does not exist on the source. Replicate refuses + // rather than converging, because converging means deleting every ref on + // the target and the states that produce this signature — a primary + // restored from backup, a data-plane wipe, an out-of-band emptying — + // are exactly the ones where the target may hold the only surviving copy. + // The refusal is deliberately its own error so a caller can surface this + // divergence instead of filing it under "nothing to do". + ErrSourceEmptyTargetPopulated = errors.New("source is empty but the target still has refs") +) + +// resolveEmptyDesiredSet decides what an empty desired set means, and is the +// only place that may conclude "the source is empty and we are converged". +// +// Emptiness is established from positive evidence, never inferred from a +// response carrying no refs (see ErrSourceEmptyUnverified). Three conditions +// must all hold before this returns success: +// +// 1. The caller opted in (AllowEmptySource). Off by default so no existing +// caller's contract changes. +// 2. The request was unscoped (AllRefs). Under a narrower scope an empty +// desired set says nothing about the repository as a whole, and the +// target's refs — which are never scope-filtered — are not ours to judge +// against a partial view of the source. +// 3. The source asserted an unborn HEAD (RefService.SourceUnborn), i.e. the +// server itself reported that the repository has no commits. +// +// With all three satisfied, the source is known to hold nothing; whether that +// is a converged state then depends on the target, which the session has +// already listed. An empty target means the two match with nothing to apply, +// and the zero-plan success carries SourceEmpty so the caller can tell this +// apart from an ordinary no-op sync. A populated target means real divergence +// and returns ErrSourceEmptyTargetPopulated. +// +// A caller that has not opted in (or asked for a narrower scope) falls back to +// the historical error text and sees byte-identical behavior to before this +// existed. An opted-in caller talking to a source that cannot make the +// assertion gets ErrSourceEmptyUnverified instead — same refusal to conclude +// anything, but named, because for such a caller the missing assertion is +// itself worth knowing about. +func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { + // The opt-in gate comes FIRST so that "off" is structurally identical to + // the behavior that predates this function — one error, one message — and + // not merely identical in the cases anyone thought to check. A caller that + // has not opted in cannot receive a sentinel it has never heard of, which + // is what makes the new errors safe to introduce: only code that asked for + // the distinction has to know how to classify it. + if !s.cfg.AllowEmptySource || !s.cfg.AllRefs { + return Result{}, errors.New("no source refs matched") + } + if len(s.sourceRefMap) > 0 { + return Result{}, ErrNoRefsSelected + } + if s.sourceService == nil || !s.sourceService.SourceUnborn { + return Result{}, ErrSourceEmptyUnverified + } + if len(s.target.refMap) > 0 { + return Result{}, fmt.Errorf("%w (%d)", ErrSourceEmptyTargetPopulated, len(s.target.refMap)) + } + return Result{ + Plans: []BranchPlan{}, + OperationMode: modeReplicate, + Protocol: s.sourceService.Protocol, + SourceEmpty: true, + Stats: s.stats.snapshot(), + Measurement: s.measurementDone(), + }, nil +} diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go new file mode 100644 index 00000000..6bbe7f69 --- /dev/null +++ b/internal/syncer/empty_source_test.go @@ -0,0 +1,146 @@ +package syncer + +import ( + "errors" + "strings" + "testing" + + "github.com/go-git/go-git/v6/plumbing" + + "entire.io/entire/git-sync/internal/gitproto" +) + +// emptySourceSession builds the session state resolveEmptyDesiredSet reads, +// with the same non-nil stats/measurement fields newSession always installs. +func emptySourceSession(cfg Config, sourceRefs, targetRefs map[plumbing.ReferenceName]plumbing.Hash, unborn bool) *syncSession { + return &syncSession{ + cfg: cfg, + stats: newStats(false), + measurementDone: startMeasurement(false), + sourceService: &gitproto.RefService{Protocol: "v2", SourceUnborn: unborn}, + sourceRefMap: sourceRefs, + target: &targetSession{refMap: targetRefs}, + } +} + +func oneRef() map[plumbing.ReferenceName]plumbing.Hash { + return map[plumbing.ReferenceName]plumbing.Hash{ + plumbing.ReferenceName("refs/heads/main"): plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + } +} + +// The converged case: the source confirmed it is empty and the target holds +// nothing either. This is the only input that may succeed, and it must be +// distinguishable from an ordinary no-work sync via SourceEmpty. +func TestResolveEmptyDesiredSetConverged(t *testing.T) { + s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, nil, nil, true) + result, err := s.resolveEmptyDesiredSet() + if err != nil { + t.Fatalf("expected success for empty source + empty target, got %v", err) + } + if !result.SourceEmpty { + t.Error("SourceEmpty = false; the caller cannot tell this from a no-op sync") + } + if len(result.Plans) != 0 { + t.Errorf("expected 0 plans, got %d", len(result.Plans)) + } + if result.Pushed != 0 || result.Deleted != 0 { + t.Errorf("expected nothing applied, got pushed=%d deleted=%d", result.Pushed, result.Deleted) + } + if result.OperationMode != modeReplicate { + t.Errorf("OperationMode = %q, want %q", result.OperationMode, modeReplicate) + } +} + +// Real divergence: refuse rather than converge, because converging here means +// deleting the target's refs. +func TestResolveEmptyDesiredSetTargetPopulated(t *testing.T) { + s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, nil, oneRef(), true) + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected ErrSourceEmptyTargetPopulated, got %v", err) + } +} + +// Silence is not an assertion of emptiness. Without the source's unborn-HEAD +// confirmation the state is unknown, even with an empty target — this is the +// case a blank proxy body or a server-side ref-listing regression lands in, +// and treating it as converged would advance a caller's watermark over a +// source that may well have refs. +func TestResolveEmptyDesiredSetUnverified(t *testing.T) { + for _, target := range []map[plumbing.ReferenceName]plumbing.Hash{nil, oneRef()} { + s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, nil, target, false) + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrSourceEmptyUnverified) { + t.Fatalf("target=%v: expected ErrSourceEmptyUnverified, got %v", target, err) + } + } +} + +// A source that HAS refs whose scope selected none of them is a different +// condition entirely, and must not be reported as an empty source however the +// policy is set — a caller acting on emptiness here would be acting on a repo +// full of refs. +func TestResolveEmptyDesiredSetSelectionEmpty(t *testing.T) { + s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, oneRef(), nil, true) + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrNoRefsSelected) { + t.Fatalf("expected ErrNoRefsSelected, got %v", err) + } + // An empty target does not soften it: the source has refs, so there is + // nothing here that could be called converged. + if errors.Is(err, ErrSourceEmptyUnverified) || errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Errorf("selection-empty misreported as an empty-source case: %v", err) + } + + // Without the opt-in it stays the historical error, like every other case + // in this family — a caller that never asked for the distinction cannot + // receive a sentinel it does not know how to classify. + s = emptySourceSession(Config{}, oneRef(), nil, true) + if _, err := s.resolveEmptyDesiredSet(); err.Error() != "no source refs matched" { + t.Errorf("un-opted-in error = %q, want the historical message", err) + } +} + +// Without the opt-in — or under a narrowed scope, where an empty desired set +// says nothing about the repository as a whole — behavior is byte-identical to +// before this logic existed, including the message callers match on. +func TestResolveEmptyDesiredSetFallsBackToHistoricalError(t *testing.T) { + cases := map[string]Config{ + "no opt-in": {AllRefs: true}, + "scoped request": {AllowEmptySource: true}, + "neither": {}, + } + for name, cfg := range cases { + t.Run(name, func(t *testing.T) { + s := emptySourceSession(cfg, nil, nil, true) + _, err := s.resolveEmptyDesiredSet() + if err == nil { + t.Fatal("expected an error, got nil") + } + if err.Error() != "no source refs matched" { + t.Errorf("error = %q, want the historical %q", err, "no source refs matched") + } + // The new sentinels must not leak into the fallback: callers + // matching the old message must not also match these. + for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrSourceEmptyTargetPopulated} { + if errors.Is(err, sentinel) { + t.Errorf("fallback error satisfies errors.Is(%v)", sentinel) + } + } + }) + } +} + +// The new sentinels must not carry the historical message, or a caller still +// substring-matching it (mirror-pipeline does, across a vendor bump) would +// classify a divergence or an unknown state as the old benign no-op. +func TestEmptySourceSentinelsDoNotCarryHistoricalMessage(t *testing.T) { + for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrSourceEmptyTargetPopulated} { + // Substring, not equality: a sentinel that merely CONTAINS the phrase + // is matched by such a caller just as surely as one that equals it. + if strings.Contains(sentinel.Error(), "no source refs matched") { + t.Errorf("%v contains the historical error message %q", sentinel, "no source refs matched") + } + } +} diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 9783a186..238cb050 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -92,6 +92,12 @@ type Config struct { MaterializedMaxObjects int ProtocolMode string BootstrapStrategy string // "" | "first-parent" | "topo" + // AllowEmptySource opts into treating a VERIFIED-empty source as an + // outcome rather than an error, in replicate mode only. Off by default: + // with it unset, an empty source fails exactly as it always has, so no + // existing caller changes behavior. See resolveEmptyDesiredSet for what + // "verified" requires and which outcome each case produces. + AllowEmptySource bool // progressOut overrides the writer used by the live progress ticker. // Defaults to os.Stderr when nil. Exposed for tests. @@ -156,6 +162,11 @@ type Result struct { Stats Stats `json:"stats"` Measurement Measurement `json:"measurement"` Protocol string `json:"protocol"` + // SourceEmpty is true when the source was verified to have no refs and + // the target had none either, so the two are converged with nothing to + // apply. Only ever set on a successful zero-plan replicate; see + // resolveEmptyDesiredSet. + SourceEmpty bool `json:"sourceEmpty,omitempty"` } func (r Result) Lines() []string { @@ -981,7 +992,7 @@ func (s *syncSession) runReplicate(ctx context.Context) (Result, error) { return Result{}, fmt.Errorf("build desired refs: %w", err) } if len(desiredRefs) == 0 { - return Result{}, errors.New("no source refs matched") + return s.resolveEmptyDesiredSet() } if ok, reason := planner.SupportsReplicateRelay(s.target.policy); !ok { diff --git a/results.go b/results.go index b1a3e863..8cb97817 100644 --- a/results.go +++ b/results.go @@ -129,6 +129,12 @@ type ExecutionSummary struct { BootstrapSuggested bool `json:"bootstrapSuggested"` SourceHEAD string `json:"sourceHead,omitempty"` Batch BatchSummary `json:"batch"` + // SourceEmpty reports that this run applied nothing because the source + // was confirmed to have no refs and the target had none either — the two + // are converged. Only ever set under SyncPolicy.AllowEmptySource, and it + // is what distinguishes that converged state from an ordinary sync that + // happened to have no work to do. + SourceEmpty bool `json:"sourceEmpty,omitempty"` } // SyncResult is the outcome of a Sync or Replicate. @@ -182,6 +188,7 @@ func fromSyncResult(result syncer.Result) SyncResult { Reason: result.RelayReason, BootstrapSuggested: result.BootstrapSuggested, SourceHEAD: result.SourceHEAD.String(), + SourceEmpty: result.SourceEmpty, Batch: BatchSummary{ Enabled: result.Batching, Planned: result.PlannedBatchCount, diff --git a/types.go b/types.go index fee0e14c..b14ee911 100644 --- a/types.go +++ b/types.go @@ -114,6 +114,19 @@ type SyncPolicy struct { Prune bool `json:"prune"` BestEffort bool `json:"bestEffort,omitempty"` Protocol ProtocolMode `json:"protocol"` + // AllowEmptySource opts into treating a verified-empty source as an + // outcome instead of an error. Replicate only, and only when the source + // itself confirms it has no commits (protocol v2 ls-refs=unborn) under + // an all-refs scope; a source that merely advertises no refs does not + // qualify. When the source is confirmed empty AND the target has no + // refs either, Replicate succeeds with zero plans and + // ExecutionSummary.SourceEmpty set. When the target still has refs the + // two have diverged and Replicate fails with + // ErrSourceEmptyTargetPopulated rather than deleting them. + // + // Off by default: leave it unset and an empty source errors exactly as + // it always has. + AllowEmptySource bool `json:"allowEmptySource,omitempty"` } // Validate enforces SyncPolicy invariants at the request edge. From 0c6bb4c912456e64c39c24740e63027ca87611fb Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 17:01:01 +0200 Subject: [PATCH 02/10] Address review: unborn HEAD is a cross-check, not evidence of emptiness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The converged path rested on a false reading of the protocol. `unborn HEAD` means only that HEAD's symref target does not exist; it says nothing about whether other refs exist. Verified against git 2.53: a repository holding refs/heads/other with HEAD pointed at a never-created refs/heads/main reports unborn, and hiding that branch with uploadpack.hideRefs reduces its entire advertisement to the unborn line alone — exactly the input the previous commit treated as proof of an empty repository. No client-side fix exists. Ref hiding is designed to be invisible to the client, so a hidden ref and an absent one are the same observation, and no combination of ls-refs arguments distinguishes them. "This repository has no refs" is therefore not a client-observable fact, and git-sync must stop claiming to establish it. So the assertion becomes an input. SyncPolicy.SourceAssertedEmpty carries the caller's authoritative answer, from a repository-state query that sees past hiding, and git-sync's role is reduced to refusing to act on it unless everything git CAN observe agrees: nothing advertised, HEAD reported unborn, and no advertised ref name dropped as invalid. Every one of those can only refuse — none can promote an absent assertion into a success — so a caller that supplies nothing gets ErrSourceEmptyUnverified however the wire reads. That closes a second instance of the same hole the review did not mention: ref-name validation dropping every advertised name (the new PartitionRefNames path) also leaves the ref set empty while unborn still fires, so RefService.SkippedRefNames is now surfaced and a non-empty one refuses. SourceUnborn is renamed HeadUnborn, because the old name asserted the conclusion rather than the observation, and its doc now says what the line does and does not prove. Also fixes the dry-run flag being dropped from the zero-plan success, so a replicate-mode plan of two empty repositories no longer reports execution.dryRun=false, with a regression test. Tests cover the review's cases: unborn alongside another branch decodes as both facts and never lets one imply the other; an asserted-empty source whose HEAD is born, or that cannot report unborn at all, or whose names were dropped as invalid, all fail closed to unverified against both an empty and a populated target. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01714HJZAqpgwuwp6fcMWEhG Entire-Checkpoint: 01M0JDDWHVPD92X8NY6EH6YNKR --- CHANGELOG.md | 6 +- client.go | 1 + internal/gitproto/fetch_test.go | 25 ++++++ internal/gitproto/refs.go | 52 ++++++----- internal/syncer/empty_source.go | 114 ++++++++++++++---------- internal/syncer/empty_source_test.go | 126 +++++++++++++++++++-------- internal/syncer/syncer.go | 9 ++ types.go | 15 ++++ 8 files changed, 242 insertions(+), 106 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fae2d169..60430d59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,9 +39,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Added -- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when the source says so itself.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering two unrelated conditions: the source has no refs, and the source has refs that the requested scope excluded. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports the three cases separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` when it advertised none but never confirmed it is empty, `ErrSourceEmptyTargetPopulated` when it is confirmed empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.SourceEmpty` when source and target are both empty and therefore already agree. +- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when a source of truth says so.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering unrelated conditions: the source has no refs, the source has refs the requested scope excluded, and the source has refs this reader was never shown. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports them separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` when emptiness could not be established, `ErrSourceEmptyTargetPopulated` when the source is empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.SourceEmpty` when source and target are both empty and therefore already agree. - Emptiness is established from what the source asserts, never inferred from a response that merely carried no refs. git-sync now requests protocol v2's `ls-refs=unborn` feature where the server advertises it, so a repository with no commits answers with an explicit `unborn HEAD` line; only that assertion, under an all-refs scope, qualifies. The distinction is the point: a blank body behind a valid header, a server-side ref-listing or hide-pattern regression, or a narrowed ref-prefix all produce the same silence as an empty repository, and a caller acting on silence would act on every affected repository at once. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. + **git-sync does not decide that a repository is empty, and will not guess.** It cannot: ref hiding is designed to be invisible to the client, so a hidden ref and an absent one are the same observation. An unborn HEAD does not close the gap either — git emits that line for any dangling HEAD, so a repository holding `refs/heads/other` with HEAD pointed at a never-created `refs/heads/main` reports unborn, and hiding that branch reduces its entire advertisement to the unborn line alone (verified against git 2.53). The assertion is therefore an input: `SyncPolicy.SourceAssertedEmpty`, which the caller supplies from a repository-state query that sees past hiding. + + What git-sync contributes is a consistency check on that claim, and it only ever refuses. Before reporting convergence it independently requires an empty advertisement, an unborn HEAD (now requested via protocol v2's `ls-refs=unborn` where the server advertises it, and surfaced as `RefService.HeadUnborn`), no advertised ref name dropped as invalid, and an empty target. None of those can promote an absent assertion into a success, so a caller cannot get a false converge out of a compliant server, and a caller that supplies no assertion gets `ErrSourceEmptyUnverified` no matter what the wire says. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. Off by default, so nothing changes for existing callers: without the opt-in — or under a narrower scope, where an empty desired set says nothing about the repository as a whole — an empty source still fails with the historical message, and the new sentinels deliberately do not carry that text so a caller still matching on it cannot mistake a divergence for the old benign no-op. diff --git a/client.go b/client.go index a1b99634..f9f18ac3 100644 --- a/client.go +++ b/client.go @@ -132,6 +132,7 @@ func (c *Client) buildSyncConfig(ctx context.Context, req SyncRequest, dryRun bo Prune: req.Policy.Prune, BestEffort: req.Policy.BestEffort, AllowEmptySource: req.Policy.AllowEmptySource, + SourceAssertedEmpty: req.Policy.SourceAssertedEmpty, ProtocolMode: string(req.Policy.Protocol), MaterializedMaxObjects: syncer.DefaultMaterializedMaxObjects, }, nil diff --git a/internal/gitproto/fetch_test.go b/internal/gitproto/fetch_test.go index 804443ca..6e4bc659 100644 --- a/internal/gitproto/fetch_test.go +++ b/internal/gitproto/fetch_test.go @@ -318,6 +318,31 @@ func TestDecodeV2LSRefsEmptyIsNotUnborn(t *testing.T) { } } +// The counterexample that makes unborn insufficient as evidence of emptiness: +// git emits the unborn line for ANY dangling HEAD, so a repository holding a +// branch reports it too. Reproduced against git 2.53 with a repo containing +// refs/heads/other and HEAD pointed at a never-created refs/heads/main. +// Decoding must report both facts and never let one imply the other. +func TestDecodeV2LSRefsUnbornCoexistsWithRefs(t *testing.T) { + wire := "" + + FormatPktLine("unborn HEAD symref-target:refs/heads/main\n") + + FormatPktLine("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/other\n") + + "0000" + refs, head, unborn, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) + if err != nil { + t.Fatalf("decodeV2LSRefs: %v", err) + } + if !unborn { + t.Error("unborn = false, want true") + } + if len(refs) != 1 || refs[0].Name().String() != "refs/heads/other" { + t.Fatalf("expected refs/heads/other to survive alongside the unborn line, got %v", refs) + } + if head != "" { + t.Errorf("head target = %q, want empty: refs/heads/main does not exist", head) + } +} + func TestBufReader(t *testing.T) { input := bytes.NewBufferString("test data") pr := NewPacketReader(input) diff --git a/internal/gitproto/refs.go b/internal/gitproto/refs.go index 115961fc..973147be 100644 --- a/internal/gitproto/refs.go +++ b/internal/gitproto/refs.go @@ -34,20 +34,32 @@ type RefService struct { // objects", "Compressing objects", ...) to stderr and asks the source // upload-pack to emit progress by not sending the no-progress option. Verbose bool - // SourceUnborn is true when the source ASSERTED that its HEAD is unborn - // — i.e. the repository exists but has no commits yet. It is positive - // evidence of emptiness, not an inference: only a v2 source advertising - // ls-refs=unborn (which we then request) can set it, and it arrives as - // an explicit "unborn HEAD" line rather than as the absence of ref - // lines. Callers that must not confuse "this repo is empty" with "this - // response carried no refs" (a blank proxy body, a ref-listing or - // hide-pattern regression, a narrowed prefix) gate on this rather than - // on len(refs) == 0. + // HeadUnborn is true when the source reported HEAD as unborn — its + // symref target does not exist. That is ALL it means. It is emphatically + // NOT a statement that the repository is empty: git emits the unborn line + // for any dangling HEAD, so a repository holding refs/heads/other while + // HEAD points at a never-created refs/heads/main reports unborn too, and + // hiding (uploadpack.hideRefs) can reduce that repository's whole + // advertisement to the unborn line alone. Verified against git 2.53. // - // False therefore means "not asserted", NOT "the source has commits": - // a v1 source, or a v2 source that does not advertise the feature, - // leaves it false however empty it is. - SourceUnborn bool + // It is therefore a NECESSARY condition for emptiness and never a + // sufficient one: a truly empty repository always reports unborn, so its + // absence is a reliable disqualifier, but its presence proves nothing + // about refs the reader cannot see. A caller that needs to act on + // emptiness must obtain that from the server's own repository state; use + // this only to cross-check such an assertion. + // + // False means "not reported", NOT "the source has commits": a v1 source, + // or a v2 source that does not advertise ls-refs=unborn, leaves it false + // however empty it is. + HeadUnborn bool + + // SkippedRefNames are advertised ref names dropped as invalid (see + // PartitionRefNames). Surfaced because their absence is load-bearing for + // any caller reasoning about an empty ref set: names dropped here leave + // refs empty while the repository plainly has some, which is + // indistinguishable from emptiness by ref count alone. + SkippedRefNames []string } // ListSourceRefs discovers refs from the source using the configured protocol mode. @@ -77,11 +89,11 @@ func ListSourceRefs(ctx context.Context, conn Conn, protocolMode string, refPref if !caps.Supports("ls-refs") || !caps.Supports("fetch") { return nil, nil, errors.New("source does not advertise required protocol v2 commands") } - refs, headTarget, unborn, err := listSourceRefsV2(ctx, conn, caps, refPrefixes) + refs, headTarget, unborn, skipped, err := listSourceRefsV2(ctx, conn, caps, refPrefixes) if err != nil { return nil, nil, err } - return refs, &RefService{Protocol: "v2", V2Caps: caps, HeadTarget: headTarget, SourceUnborn: unborn}, nil + return refs, &RefService{Protocol: "v2", V2Caps: caps, HeadTarget: headTarget, HeadUnborn: unborn, SkippedRefNames: skipped}, nil } if protocolMode == "v2" { return nil, nil, errors.New("source did not negotiate protocol v2") @@ -191,7 +203,7 @@ func listSourceRefsV1(ctx context.Context, conn Conn) (*packp.AdvRefs, []*plumbi return adv, refs, nil } -func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, prefixes []string) ([]*plumbing.Reference, plumbing.ReferenceName, bool, error) { +func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, prefixes []string) ([]*plumbing.Reference, plumbing.ReferenceName, bool, []string, error) { // Always include "HEAD" so the server returns the symref-target attribute // for HEAD. Without this, callers that pass only "refs/heads/" or // "refs/tags/" prefixes filter HEAD out of the response and lose the @@ -211,18 +223,18 @@ func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, pref } body, err := EncodeCommand("ls-refs", caps.RequestCapabilities(), args) if err != nil { - return nil, "", false, err + return nil, "", false, nil, err } data, err := PostRPC(ctx, conn, transport.UploadPackService, body, true, "upload-pack ls-refs") if err != nil { - return nil, "", false, err + return nil, "", false, nil, err } refs, headTarget, unborn, skipped, err := decodeV2LSRefs(bytes.NewReader(data)) if err != nil { - return nil, "", false, err + return nil, "", false, nil, err } WarnSkippedRefNames(conn.ProgressWriter(), "source", skipped) - return refs, headTarget, unborn, nil + return refs, headTarget, unborn, skipped, nil } // decodeV2LSRefs decodes an ls-refs response into its refs, the branch HEAD diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go index aec2bdba..3360536e 100644 --- a/internal/syncer/empty_source.go +++ b/internal/syncer/empty_source.go @@ -6,12 +6,12 @@ import ( ) // The empty-desired-set outcomes. Replicate reaches this family whenever -// planning produces no desired refs, which happens for two unrelated reasons -// that the wire signal alone cannot tell apart: the source has no refs, or the -// source has refs and the requested scope excluded all of them. Collapsing -// them into one error (as this package did before) forces every caller to -// guess, and the two demand opposite handling — one may be a converged state, -// the other never is. +// planning produces no desired refs, which happens for unrelated reasons the +// wire signal alone cannot tell apart: the source has no refs, the source has +// refs the requested scope excluded, or the source has refs this reader was +// never shown. Collapsing them into one error (as this package did before) +// forces every caller to guess, and they demand opposite handling — one may be +// a converged state, the others never are. var ( // ErrNoRefsSelected means the source DOES advertise refs, but the // requested scope (branch selection, mappings, exclude prefixes) matched @@ -27,19 +27,26 @@ var ( // checks run in is not load-bearing. ErrNoRefsSelected = errors.New("source has refs but none matched the requested scope") - // ErrSourceEmptyUnverified means planning found no desired refs AND the - // source advertised no refs, but nothing asserted that the source is - // actually empty, so the emptiness cannot be trusted. Absence of refs in - // a response is not the same claim as a repository reporting it has no - // commits: a blank body behind a valid header, a server-side ref-listing - // or hide-pattern regression, or a narrowed ref-prefix all produce the - // same silence. So does ref-name validation dropping every advertised - // name (gitproto.PartitionRefNames) — a repository full of refs whose - // names git would reject arrives here indistinguishable from an empty - // one, which is exactly why the unborn assertion and not the ref count - // is what decides. Callers must treat this as "unknown", never as - // "converged". - ErrSourceEmptyUnverified = errors.New("source advertised no refs but did not confirm it is empty") + // ErrSourceEmptyUnverified means planning found no desired refs and + // nothing established that the source is actually empty, so emptiness + // cannot be concluded. This is the default outcome, and deliberately the + // wide one: an advertisement carrying no refs is not a claim that a + // repository has none. + // + // The cases that land here are all real and all indistinguishable from an + // empty repository by ref count alone: + // + // - the caller supplied no authoritative assertion (SourceAssertedEmpty); + // - the source did not report an unborn HEAD, so HEAD's target exists + // and some ref is therefore being withheld; + // - ref-name validation dropped every advertised name + // (gitproto.PartitionRefNames), so a repository full of refs git would + // reject arrives looking empty; + // - a blank body behind a valid header, a server-side ref-listing or + // hide-pattern regression, or a narrowed ref-prefix. + // + // Callers must treat this as "unknown", never as "converged". + ErrSourceEmptyUnverified = errors.New("source advertised no refs but its emptiness could not be verified") // ErrSourceEmptyTargetPopulated means the source is VERIFIED empty while // the target still holds refs. The two have genuinely diverged: whatever @@ -56,53 +63,66 @@ var ( // resolveEmptyDesiredSet decides what an empty desired set means, and is the // only place that may conclude "the source is empty and we are converged". // -// Emptiness is established from positive evidence, never inferred from a -// response carrying no refs (see ErrSourceEmptyUnverified). Three conditions -// must all hold before this returns success: +// Emptiness is NOT established here. Git offers no way to prove it: ref hiding +// is designed to be invisible to the client, so a hidden ref and an absent one +// are the same observation, and an unborn HEAD says only that HEAD's target +// does not exist (a repository holding refs/heads/other with HEAD pointed at a +// never-created refs/heads/main reports unborn, and hiding that branch reduces +// its whole advertisement to the unborn line — verified against git 2.53). The +// assertion therefore has to come from the server's own repository state, +// which the caller supplies via Config.SourceAssertedEmpty; what this function +// does is refuse to act on that assertion unless everything git CAN observe +// agrees with it. +// +// Five conditions must all hold before this returns success: // -// 1. The caller opted in (AllowEmptySource). Off by default so no existing -// caller's contract changes. +// 1. The caller opted in (AllowEmptySource). Checked FIRST so that "off" is +// structurally identical to the behavior that predates this function — one +// error, one message — and not merely identical in the cases someone +// thought to test. A caller that has not opted in cannot receive a +// sentinel it has never heard of, which is what makes these errors safe to +// introduce. // 2. The request was unscoped (AllRefs). Under a narrower scope an empty // desired set says nothing about the repository as a whole, and the // target's refs — which are never scope-filtered — are not ours to judge // against a partial view of the source. -// 3. The source asserted an unborn HEAD (RefService.SourceUnborn), i.e. the -// server itself reported that the repository has no commits. +// 3. The caller asserted emptiness (SourceAssertedEmpty) from a source of +// truth that sees past hiding. +// 4. Every git-side observation corroborates it: nothing advertised, HEAD +// reported unborn, and no advertised name dropped as invalid. Each of +// these can only ever REFUSE — none can promote an absent assertion into a +// success — so the git signal is a consistency check on the caller's +// claim, never a substitute for it. +// 5. The target has no refs either, which is what makes the state converged +// rather than divergent. // -// With all three satisfied, the source is known to hold nothing; whether that -// is a converged state then depends on the target, which the session has -// already listed. An empty target means the two match with nothing to apply, -// and the zero-plan success carries SourceEmpty so the caller can tell this -// apart from an ordinary no-op sync. A populated target means real divergence -// and returns ErrSourceEmptyTargetPopulated. -// -// A caller that has not opted in (or asked for a narrower scope) falls back to -// the historical error text and sees byte-identical behavior to before this -// existed. An opted-in caller talking to a source that cannot make the -// assertion gets ErrSourceEmptyUnverified instead — same refusal to conclude -// anything, but named, because for such a caller the missing assertion is -// itself worth knowing about. +// Anything unmet fails closed, to ErrSourceEmptyUnverified or (for a populated +// target) ErrSourceEmptyTargetPopulated. Only condition 5 distinguishes +// "converged" from "diverged"; every other failure is "unknown". func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { - // The opt-in gate comes FIRST so that "off" is structurally identical to - // the behavior that predates this function — one error, one message — and - // not merely identical in the cases anyone thought to check. A caller that - // has not opted in cannot receive a sentinel it has never heard of, which - // is what makes the new errors safe to introduce: only code that asked for - // the distinction has to know how to classify it. if !s.cfg.AllowEmptySource || !s.cfg.AllRefs { return Result{}, errors.New("no source refs matched") } if len(s.sourceRefMap) > 0 { return Result{}, ErrNoRefsSelected } - if s.sourceService == nil || !s.sourceService.SourceUnborn { - return Result{}, ErrSourceEmptyUnverified + if !s.cfg.SourceAssertedEmpty { + return Result{}, fmt.Errorf("%w: no authoritative assertion from the source", ErrSourceEmptyUnverified) + } + if s.sourceService == nil || !s.sourceService.HeadUnborn { + // HEAD's target exists while nothing was advertised, so a ref is being + // withheld — the caller's assertion disagrees with the wire and loses. + return Result{}, fmt.Errorf("%w: source asserted empty but did not report an unborn HEAD", ErrSourceEmptyUnverified) + } + if n := len(s.sourceService.SkippedRefNames); n > 0 { + return Result{}, fmt.Errorf("%w: source asserted empty but %d advertised ref name(s) were dropped as invalid", ErrSourceEmptyUnverified, n) } if len(s.target.refMap) > 0 { return Result{}, fmt.Errorf("%w (%d)", ErrSourceEmptyTargetPopulated, len(s.target.refMap)) } return Result{ Plans: []BranchPlan{}, + DryRun: s.cfg.DryRun, OperationMode: modeReplicate, Protocol: s.sourceService.Protocol, SourceEmpty: true, diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index 6bbe7f69..fd38f0e2 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -10,70 +10,125 @@ import ( "entire.io/entire/git-sync/internal/gitproto" ) -// emptySourceSession builds the session state resolveEmptyDesiredSet reads, -// with the same non-nil stats/measurement fields newSession always installs. -func emptySourceSession(cfg Config, sourceRefs, targetRefs map[plumbing.ReferenceName]plumbing.Hash, unborn bool) *syncSession { +// converged is the one input that may succeed: opted in, unscoped, the caller +// asserted emptiness, and every git-side observation agrees. +func converged() Config { + return Config{AllowEmptySource: true, AllRefs: true, SourceAssertedEmpty: true} +} + +func emptySourceSession(cfg Config, sourceRefs, targetRefs map[plumbing.ReferenceName]plumbing.Hash, svc *gitproto.RefService) *syncSession { return &syncSession{ cfg: cfg, stats: newStats(false), measurementDone: startMeasurement(false), - sourceService: &gitproto.RefService{Protocol: "v2", SourceUnborn: unborn}, + sourceService: svc, sourceRefMap: sourceRefs, target: &targetSession{refMap: targetRefs}, } } +func unbornSource() *gitproto.RefService { + return &gitproto.RefService{Protocol: "v2", HeadUnborn: true} +} + func oneRef() map[plumbing.ReferenceName]plumbing.Hash { return map[plumbing.ReferenceName]plumbing.Hash{ plumbing.ReferenceName("refs/heads/main"): plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), } } -// The converged case: the source confirmed it is empty and the target holds -// nothing either. This is the only input that may succeed, and it must be -// distinguishable from an ordinary no-work sync via SourceEmpty. func TestResolveEmptyDesiredSetConverged(t *testing.T) { - s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, nil, nil, true) + s := emptySourceSession(converged(), nil, nil, unbornSource()) result, err := s.resolveEmptyDesiredSet() if err != nil { - t.Fatalf("expected success for empty source + empty target, got %v", err) + t.Fatalf("expected success for an asserted-empty source and empty target, got %v", err) } if !result.SourceEmpty { t.Error("SourceEmpty = false; the caller cannot tell this from a no-op sync") } - if len(result.Plans) != 0 { - t.Errorf("expected 0 plans, got %d", len(result.Plans)) - } - if result.Pushed != 0 || result.Deleted != 0 { - t.Errorf("expected nothing applied, got pushed=%d deleted=%d", result.Pushed, result.Deleted) + if len(result.Plans) != 0 || result.Pushed != 0 || result.Deleted != 0 { + t.Errorf("expected nothing applied, got plans=%d pushed=%d deleted=%d", len(result.Plans), result.Pushed, result.Deleted) } if result.OperationMode != modeReplicate { t.Errorf("OperationMode = %q, want %q", result.OperationMode, modeReplicate) } } +// A plan must report itself as one. Client.Plan runs replicate with DryRun set, +// and the result is what the caller renders, so dropping the flag makes a plan +// of two empty repos read as a sync that really ran. +func TestResolveEmptyDesiredSetKeepsDryRun(t *testing.T) { + cfg := converged() + cfg.DryRun = true + s := emptySourceSession(cfg, nil, nil, unbornSource()) + result, err := s.resolveEmptyDesiredSet() + if err != nil { + t.Fatalf("expected success, got %v", err) + } + if !result.DryRun { + t.Error("DryRun = false on a dry-run session") + } + if !result.SourceEmpty { + t.Error("SourceEmpty = false; a plan should still report what it found") + } +} + // Real divergence: refuse rather than converge, because converging here means // deleting the target's refs. func TestResolveEmptyDesiredSetTargetPopulated(t *testing.T) { - s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, nil, oneRef(), true) + s := emptySourceSession(converged(), nil, oneRef(), unbornSource()) _, err := s.resolveEmptyDesiredSet() if !errors.Is(err, ErrSourceEmptyTargetPopulated) { t.Fatalf("expected ErrSourceEmptyTargetPopulated, got %v", err) } } -// Silence is not an assertion of emptiness. Without the source's unborn-HEAD -// confirmation the state is unknown, even with an empty target — this is the -// case a blank proxy body or a server-side ref-listing regression lands in, -// and treating it as converged would advance a caller's watermark over a -// source that may well have refs. +// Every way the evidence can fall short lands on "unknown", never "converged". +// These are the cases where acting on emptiness would advance a watermark over +// a repository that may well hold refs. func TestResolveEmptyDesiredSetUnverified(t *testing.T) { - for _, target := range []map[plumbing.ReferenceName]plumbing.Hash{nil, oneRef()} { - s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, nil, target, false) - _, err := s.resolveEmptyDesiredSet() - if !errors.Is(err, ErrSourceEmptyUnverified) { - t.Fatalf("target=%v: expected ErrSourceEmptyUnverified, got %v", target, err) - } + noAssertion := converged() + noAssertion.SourceAssertedEmpty = false + + cases := map[string]struct { + cfg Config + svc *gitproto.RefService + }{ + // The caller never asserted emptiness, so there is nothing to + // corroborate — an unborn HEAD on its own proves only that HEAD's + // target does not exist. + "no authoritative assertion": {noAssertion, unbornSource()}, + + // The assertion disagrees with the wire: HEAD's target exists, so a + // ref is being withheld. git 2.53 emits no unborn line here. + "asserted empty but HEAD is born": {converged(), &gitproto.RefService{Protocol: "v2"}}, + + // A v1 source, or a v2 source not advertising ls-refs=unborn, cannot + // report unborn at all — so it can never corroborate, however empty. + "source cannot report unborn": {converged(), &gitproto.RefService{Protocol: "v1"}}, + + // Ref-name validation ate the whole advertisement: the repository + // plainly has refs, and by ref count alone it looks empty. + "advertised names dropped as invalid": {converged(), &gitproto.RefService{ + Protocol: "v2", HeadUnborn: true, SkippedRefNames: []string{"refs/heads/bad name"}, + }}, + + // No ref service at all (a listing that failed upstream of here). + "no source service": {converged(), nil}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // Asserted against BOTH target states: an empty target must not + // rescue missing evidence, and a populated one must not be + // reported as divergence when emptiness was never established. + for _, target := range []map[plumbing.ReferenceName]plumbing.Hash{nil, oneRef()} { + s := emptySourceSession(tc.cfg, nil, target, tc.svc) + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrSourceEmptyUnverified) { + t.Fatalf("target=%v: expected ErrSourceEmptyUnverified, got %v", target, err) + } + } + }) } } @@ -82,13 +137,11 @@ func TestResolveEmptyDesiredSetUnverified(t *testing.T) { // policy is set — a caller acting on emptiness here would be acting on a repo // full of refs. func TestResolveEmptyDesiredSetSelectionEmpty(t *testing.T) { - s := emptySourceSession(Config{AllowEmptySource: true, AllRefs: true}, oneRef(), nil, true) + s := emptySourceSession(converged(), oneRef(), nil, unbornSource()) _, err := s.resolveEmptyDesiredSet() if !errors.Is(err, ErrNoRefsSelected) { t.Fatalf("expected ErrNoRefsSelected, got %v", err) } - // An empty target does not soften it: the source has refs, so there is - // nothing here that could be called converged. if errors.Is(err, ErrSourceEmptyUnverified) || errors.Is(err, ErrSourceEmptyTargetPopulated) { t.Errorf("selection-empty misreported as an empty-source case: %v", err) } @@ -96,7 +149,7 @@ func TestResolveEmptyDesiredSetSelectionEmpty(t *testing.T) { // Without the opt-in it stays the historical error, like every other case // in this family — a caller that never asked for the distinction cannot // receive a sentinel it does not know how to classify. - s = emptySourceSession(Config{}, oneRef(), nil, true) + s = emptySourceSession(Config{}, oneRef(), nil, unbornSource()) if _, err := s.resolveEmptyDesiredSet(); err.Error() != "no source refs matched" { t.Errorf("un-opted-in error = %q, want the historical message", err) } @@ -104,16 +157,17 @@ func TestResolveEmptyDesiredSetSelectionEmpty(t *testing.T) { // Without the opt-in — or under a narrowed scope, where an empty desired set // says nothing about the repository as a whole — behavior is byte-identical to -// before this logic existed, including the message callers match on. +// before this logic existed, including the message callers match on. Note the +// assertion is set in every case: opting out must win over it. func TestResolveEmptyDesiredSetFallsBackToHistoricalError(t *testing.T) { cases := map[string]Config{ - "no opt-in": {AllRefs: true}, - "scoped request": {AllowEmptySource: true}, - "neither": {}, + "no opt-in": {AllRefs: true, SourceAssertedEmpty: true}, + "scoped request": {AllowEmptySource: true, SourceAssertedEmpty: true}, + "neither": {SourceAssertedEmpty: true}, } for name, cfg := range cases { t.Run(name, func(t *testing.T) { - s := emptySourceSession(cfg, nil, nil, true) + s := emptySourceSession(cfg, nil, nil, unbornSource()) _, err := s.resolveEmptyDesiredSet() if err == nil { t.Fatal("expected an error, got nil") @@ -121,8 +175,6 @@ func TestResolveEmptyDesiredSetFallsBackToHistoricalError(t *testing.T) { if err.Error() != "no source refs matched" { t.Errorf("error = %q, want the historical %q", err, "no source refs matched") } - // The new sentinels must not leak into the fallback: callers - // matching the old message must not also match these. for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrSourceEmptyTargetPopulated} { if errors.Is(err, sentinel) { t.Errorf("fallback error satisfies errors.Is(%v)", sentinel) diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 238cb050..8a5c0d64 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -92,6 +92,15 @@ type Config struct { MaterializedMaxObjects int ProtocolMode string BootstrapStrategy string // "" | "first-parent" | "topo" + // SourceAssertedEmpty is the caller's authoritative statement that the + // source repository holds no refs at all, obtained from something that + // sees the repository's real state rather than its advertisement — git + // cannot supply this, because ref hiding is invisible to the client by + // design. Only consulted under AllowEmptySource, and never sufficient on + // its own: resolveEmptyDesiredSet still requires every git-side + // observation to corroborate it. + SourceAssertedEmpty bool + // AllowEmptySource opts into treating a VERIFIED-empty source as an // outcome rather than an error, in replicate mode only. Off by default: // with it unset, an empty source fails exactly as it always has, so no diff --git a/types.go b/types.go index b14ee911..988ce967 100644 --- a/types.go +++ b/types.go @@ -114,6 +114,21 @@ type SyncPolicy struct { Prune bool `json:"prune"` BestEffort bool `json:"bestEffort,omitempty"` Protocol ProtocolMode `json:"protocol"` + // SourceAssertedEmpty is your authoritative statement that the source + // repository holds no refs at all — from a repository-state query, not + // from a ref listing. git-sync cannot determine this and will not guess: + // ref hiding is invisible to the client, so an unborn HEAD and an empty + // advertisement are consistent with a repository that holds refs you were + // not shown. Supply this only from a source of truth that sees past + // hiding; without it Replicate reports ErrSourceEmptyUnverified. + // + // It is a necessary input, never a sufficient one: Replicate independently + // requires an empty advertisement, an unborn HEAD, no ref names dropped as + // invalid, and an empty target before it will report convergence. Those + // checks can only refuse — they never turn an absent assertion into a + // success. + SourceAssertedEmpty bool `json:"sourceAssertedEmpty,omitempty"` + // AllowEmptySource opts into treating a verified-empty source as an // outcome instead of an error. Replicate only, and only when the source // itself confirms it has no commits (protocol v2 ls-refs=unborn) under From 502b1e0ea4773f797f7b97e3602eb96c1e1748b3 Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 17:12:39 +0200 Subject: [PATCH 03/10] Address review: the target's emptiness needs asserting too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit fixed this asymmetry on the source leg and left it standing on the target: len(target.refMap) == 0 was still read as proof that the target holds no refs. It is the same mistake. receive.hideRefs omits matching refs from receive-pack's advertisement, so a populated target advertises nothing but the bare capabilities^{} sentinel — verified against git 2.53, where a repo holding refs/heads/other with receive.hideRefs=refs/heads/other advertises exactly that. The target case is the sharper of the two, because receive.hideRefs and uploadpack.hideRefs are separate settings: the same probe confirms upload-pack still serves refs/heads/other to fetchers while receive-pack conceals it. A target wrongly judged empty is therefore one whose READERS see refs the source does not have — live divergence, reported as convergence, which is the one direction a watermark claim must never fail in. So TargetAssertedEmpty joins SourceAssertedEmpty, corroborated the same way and failing closed to a distinct ErrTargetEmptyUnverified. A VISIBLE target ref still reports ErrSourceEmptyTargetPopulated rather than an unknown: hiding can conceal refs but never invent them, so anything advertised is real and that is divergence, not uncertainty. Target ref names dropped by validation are now retained rather than only warned about, closing the same secondary hole the source side already covers: they leave refMap empty while the target plainly holds refs. Exported documentation is corrected where it still described the superseded contract. SyncPolicy said AllowEmptySource relied on the source confirming emptiness through ls-refs=unborn, which stopped being true when the assertion became an input; ErrSourceEmptyUnverified said it meant a missing unborn assertion, when it covers a missing caller assertion and dropped ref names as well. Both now describe what the implementation actually requires, so an embedder cannot omit an assertion or misread the error. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01714HJZAqpgwuwp6fcMWEhG Entire-Checkpoint: 01M0JE3619BM1KPE2YXS7Y5XVP --- CHANGELOG.md | 6 ++- client.go | 1 + errors.go | 34 ++++++++++++++--- internal/syncer/empty_source.go | 48 +++++++++++++++++++++--- internal/syncer/empty_source_test.go | 56 ++++++++++++++++++++++++++-- internal/syncer/syncer.go | 17 +++++++++ types.go | 55 ++++++++++++++++----------- 7 files changed, 178 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60430d59..c1dba677 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,11 +39,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Added -- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when a source of truth says so.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering unrelated conditions: the source has no refs, the source has refs the requested scope excluded, and the source has refs this reader was never shown. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports them separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` when emptiness could not be established, `ErrSourceEmptyTargetPopulated` when the source is empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.SourceEmpty` when source and target are both empty and therefore already agree. +- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when a source of truth says so.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering unrelated conditions: the source has no refs, the source has refs the requested scope excluded, and the source has refs this reader was never shown. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports them separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` / `ErrTargetEmptyUnverified` when either side's emptiness could not be established, `ErrSourceEmptyTargetPopulated` when the source is empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.SourceEmpty` when source and target are both empty and therefore already agree. **git-sync does not decide that a repository is empty, and will not guess.** It cannot: ref hiding is designed to be invisible to the client, so a hidden ref and an absent one are the same observation. An unborn HEAD does not close the gap either — git emits that line for any dangling HEAD, so a repository holding `refs/heads/other` with HEAD pointed at a never-created `refs/heads/main` reports unborn, and hiding that branch reduces its entire advertisement to the unborn line alone (verified against git 2.53). The assertion is therefore an input: `SyncPolicy.SourceAssertedEmpty`, which the caller supplies from a repository-state query that sees past hiding. - What git-sync contributes is a consistency check on that claim, and it only ever refuses. Before reporting convergence it independently requires an empty advertisement, an unborn HEAD (now requested via protocol v2's `ls-refs=unborn` where the server advertises it, and surfaced as `RefService.HeadUnborn`), no advertised ref name dropped as invalid, and an empty target. None of those can promote an absent assertion into a success, so a caller cannot get a false converge out of a compliant server, and a caller that supplies no assertion gets `ErrSourceEmptyUnverified` no matter what the wire says. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. + The target needs the same treatment, via `SourceAssertedEmpty`'s counterpart `TargetAssertedEmpty`, and for a sharper reason: `receive.hideRefs` omits matching refs from receive-pack's advertisement, so a populated target can advertise nothing but the bare `capabilities^{}` sentinel — and because `receive.hideRefs` and `uploadpack.hideRefs` are separate settings, a ref hidden from the push side is still served to fetchers, so a target wrongly judged empty is one whose readers see refs the source does not have. + + What git-sync contributes is a consistency check on those claims, and it only ever refuses. Before reporting convergence it independently requires an empty advertisement on each side, an unborn HEAD on the source (now requested via protocol v2's `ls-refs=unborn` where the server advertises it, and surfaced as `RefService.HeadUnborn`), no advertised ref name dropped as invalid on either side, and no visible target ref. None of those can promote an absent assertion into a success, so a caller cannot get a false converge out of a compliant server, and a caller that supplies no assertion gets `ErrSourceEmptyUnverified` or `ErrTargetEmptyUnverified` no matter what the wire says. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. Off by default, so nothing changes for existing callers: without the opt-in — or under a narrower scope, where an empty desired set says nothing about the repository as a whole — an empty source still fails with the historical message, and the new sentinels deliberately do not carry that text so a caller still matching on it cannot mistake a divergence for the old benign no-op. diff --git a/client.go b/client.go index f9f18ac3..3179748c 100644 --- a/client.go +++ b/client.go @@ -133,6 +133,7 @@ func (c *Client) buildSyncConfig(ctx context.Context, req SyncRequest, dryRun bo BestEffort: req.Policy.BestEffort, AllowEmptySource: req.Policy.AllowEmptySource, SourceAssertedEmpty: req.Policy.SourceAssertedEmpty, + TargetAssertedEmpty: req.Policy.TargetAssertedEmpty, ProtocolMode: string(req.Policy.Protocol), MaterializedMaxObjects: syncer.DefaultMaterializedMaxObjects, }, nil diff --git a/errors.go b/errors.go index 13c69e85..574f7483 100644 --- a/errors.go +++ b/errors.go @@ -42,14 +42,36 @@ type RefRejectedError = gitproto.RefRejectedError var ErrNoRefsSelected = syncer.ErrNoRefsSelected // ErrSourceEmptyUnverified is returned (wrapped) by Replicate under -// SyncPolicy.AllowEmptySource when the source advertised no refs but never -// confirmed that it is empty — no protocol v2 unborn-HEAD assertion. The -// response's silence has several possible causes besides an empty repository -// (a blank body behind a valid header, a server-side ref-listing or -// hide-pattern regression), so the state is reported as unknown rather than -// converged. Test for it with errors.Is. +// SyncPolicy.AllowEmptySource when the source advertised no refs but its +// emptiness could not be established. It covers every way the evidence can +// fall short, not one of them: +// +// - SyncPolicy.SourceAssertedEmpty was not supplied, so there is no +// authoritative claim to act on; +// - the source did not report an unborn HEAD, meaning HEAD's target exists +// and a ref is therefore being withheld; +// - ref-name validation dropped every advertised name, so a repository full +// of refs git would reject arrives looking empty; +// - a blank body behind a valid header, a server-side ref-listing or +// hide-pattern regression, or a narrowed ref-prefix. +// +// Treat it as "unknown", never as "converged". Test for it with errors.Is. var ErrSourceEmptyUnverified = syncer.ErrSourceEmptyUnverified +// ErrTargetEmptyUnverified is returned (wrapped) by Replicate under +// SyncPolicy.AllowEmptySource when the source was verified empty but the +// TARGET's emptiness could not be established — no SyncPolicy.TargetAssertedEmpty, +// or a target ref name dropped as invalid. Distinct from +// ErrSourceEmptyTargetPopulated, which is a target KNOWN to hold refs. +// +// An empty receive-pack advertisement proves no more than an empty ls-refs +// one: receive.hideRefs omits matching refs from it. And because +// receive.hideRefs and uploadpack.hideRefs are separate settings, a ref hidden +// from the push side is still served to fetchers — so a target wrongly judged +// empty is one whose readers see refs the source does not have. Test for it +// with errors.Is. +var ErrTargetEmptyUnverified = syncer.ErrTargetEmptyUnverified + // ErrSourceEmptyTargetPopulated is returned (wrapped) by Replicate under // SyncPolicy.AllowEmptySource when the source is confirmed empty while the // target still holds refs — a real divergence, since nothing the target serves diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go index 3360536e..32030ca2 100644 --- a/internal/syncer/empty_source.go +++ b/internal/syncer/empty_source.go @@ -48,6 +48,22 @@ var ( // Callers must treat this as "unknown", never as "converged". ErrSourceEmptyUnverified = errors.New("source advertised no refs but its emptiness could not be verified") + // ErrTargetEmptyUnverified means the source was verified empty but the + // TARGET's emptiness could not be established, so whether the two are + // converged is unknown. Distinct from ErrSourceEmptyTargetPopulated, + // which is a target KNOWN to hold refs: this is a target that advertised + // none and could not confirm it. + // + // The same asymmetry applies as on the source side, and it bites harder: + // receive.hideRefs omits matching refs from receive-pack's advertisement, + // so a populated target can advertise nothing but the capabilities^{} + // sentinel. And because receive.hideRefs and uploadpack.hideRefs are + // separate settings, such a ref is still served to fetchers — a target + // wrongly judged empty is one whose readers see refs the source does not + // have, which is precisely the divergence a convergence claim must never + // paper over. + ErrTargetEmptyUnverified = errors.New("target advertised no refs but its emptiness could not be verified") + // ErrSourceEmptyTargetPopulated means the source is VERIFIED empty while // the target still holds refs. The two have genuinely diverged: whatever // the target serves does not exist on the source. Replicate refuses @@ -74,7 +90,7 @@ var ( // does is refuse to act on that assertion unless everything git CAN observe // agrees with it. // -// Five conditions must all hold before this returns success: +// Six conditions must all hold before this returns success: // // 1. The caller opted in (AllowEmptySource). Checked FIRST so that "off" is // structurally identical to the behavior that predates this function — one @@ -93,12 +109,20 @@ var ( // these can only ever REFUSE — none can promote an absent assertion into a // success — so the git signal is a consistency check on the caller's // claim, never a substitute for it. -// 5. The target has no refs either, which is what makes the state converged -// rather than divergent. +// 5. The target advertises no refs — anything visible there is real, since +// hiding can conceal refs but never invent them, so a visible ref means +// divergence. +// 6. The target's emptiness is asserted too (TargetAssertedEmpty) and +// corroborated the same way. An empty receive-pack advertisement proves no +// more than an empty ls-refs one: receive.hideRefs omits matching refs +// from it, and since that is a separate setting from uploadpack.hideRefs, +// a target wrongly judged empty may still be serving those refs to its +// readers. // -// Anything unmet fails closed, to ErrSourceEmptyUnverified or (for a populated -// target) ErrSourceEmptyTargetPopulated. Only condition 5 distinguishes -// "converged" from "diverged"; every other failure is "unknown". +// Anything unmet fails closed: ErrSourceEmptyUnverified for the source half, +// ErrTargetEmptyUnverified for the target half, ErrSourceEmptyTargetPopulated +// for a target known to hold refs. Only condition 5 reports divergence; every +// other failure is "unknown". func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { if !s.cfg.AllowEmptySource || !s.cfg.AllRefs { return Result{}, errors.New("no source refs matched") @@ -117,9 +141,21 @@ func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { if n := len(s.sourceService.SkippedRefNames); n > 0 { return Result{}, fmt.Errorf("%w: source asserted empty but %d advertised ref name(s) were dropped as invalid", ErrSourceEmptyUnverified, n) } + // A target that advertises refs is populated, full stop — hiding can only + // ever conceal refs, never invent them, so anything visible here is real + // and this is divergence. if len(s.target.refMap) > 0 { return Result{}, fmt.Errorf("%w (%d)", ErrSourceEmptyTargetPopulated, len(s.target.refMap)) } + // An EMPTY target advertisement proves nothing on its own, for the same + // reason the source's did not, so it needs the same authoritative + // assertion and the same corroboration. + if !s.cfg.TargetAssertedEmpty { + return Result{}, fmt.Errorf("%w: no authoritative assertion from the target", ErrTargetEmptyUnverified) + } + if n := len(s.target.skippedRefNames); n > 0 { + return Result{}, fmt.Errorf("%w: target asserted empty but %d advertised ref name(s) were dropped as invalid", ErrTargetEmptyUnverified, n) + } return Result{ Plans: []BranchPlan{}, DryRun: s.cfg.DryRun, diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index fd38f0e2..7027a4b5 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -13,7 +13,7 @@ import ( // converged is the one input that may succeed: opted in, unscoped, the caller // asserted emptiness, and every git-side observation agrees. func converged() Config { - return Config{AllowEmptySource: true, AllRefs: true, SourceAssertedEmpty: true} + return Config{AllowEmptySource: true, AllRefs: true, SourceAssertedEmpty: true, TargetAssertedEmpty: true} } func emptySourceSession(cfg Config, sourceRefs, targetRefs map[plumbing.ReferenceName]plumbing.Hash, svc *gitproto.RefService) *syncSession { @@ -27,6 +27,14 @@ func emptySourceSession(cfg Config, sourceRefs, targetRefs map[plumbing.Referenc } } +// withTargetSkipped marks the target as having advertised a ref name that +// validation dropped, which leaves its refMap empty while it plainly holds a +// ref. +func withTargetSkipped(s *syncSession, names ...string) *syncSession { + s.target.skippedRefNames = names + return s +} + func unbornSource() *gitproto.RefService { return &gitproto.RefService{Protocol: "v2", HeadUnborn: true} } @@ -132,6 +140,48 @@ func TestResolveEmptyDesiredSetUnverified(t *testing.T) { } } +// The target half needs the same evidence as the source, and for a sharper +// reason: receive.hideRefs omits matching refs from receive-pack's +// advertisement, so a populated target can advertise nothing but the +// capabilities^{} sentinel (verified against git 2.53) — and since that is a +// separate setting from uploadpack.hideRefs, such a ref is still served to the +// mirror's readers. An unverifiable target must therefore never read as +// converged. +func TestResolveEmptyDesiredSetTargetUnverified(t *testing.T) { + noTargetAssertion := converged() + noTargetAssertion.TargetAssertedEmpty = false + + t.Run("no authoritative assertion", func(t *testing.T) { + s := emptySourceSession(noTargetAssertion, nil, nil, unbornSource()) + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrTargetEmptyUnverified) { + t.Fatalf("expected ErrTargetEmptyUnverified, got %v", err) + } + // Must not be mistaken for either neighbouring outcome. + if errors.Is(err, ErrSourceEmptyUnverified) || errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Errorf("target-unverified conflated with another outcome: %v", err) + } + }) + + t.Run("advertised target names dropped as invalid", func(t *testing.T) { + s := withTargetSkipped(emptySourceSession(converged(), nil, nil, unbornSource()), "refs/heads/bad name") + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrTargetEmptyUnverified) { + t.Fatalf("expected ErrTargetEmptyUnverified, got %v", err) + } + }) + + // A VISIBLE target ref is still divergence rather than an unknown: hiding + // can conceal refs but never invent them, so anything advertised is real. + t.Run("visible target refs stay divergence", func(t *testing.T) { + s := emptySourceSession(noTargetAssertion, nil, oneRef(), unbornSource()) + _, err := s.resolveEmptyDesiredSet() + if !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected ErrSourceEmptyTargetPopulated, got %v", err) + } + }) +} + // A source that HAS refs whose scope selected none of them is a different // condition entirely, and must not be reported as an empty source however the // policy is set — a caller acting on emptiness here would be acting on a repo @@ -175,7 +225,7 @@ func TestResolveEmptyDesiredSetFallsBackToHistoricalError(t *testing.T) { if err.Error() != "no source refs matched" { t.Errorf("error = %q, want the historical %q", err, "no source refs matched") } - for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrSourceEmptyTargetPopulated} { + for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrTargetEmptyUnverified, ErrSourceEmptyTargetPopulated} { if errors.Is(err, sentinel) { t.Errorf("fallback error satisfies errors.Is(%v)", sentinel) } @@ -188,7 +238,7 @@ func TestResolveEmptyDesiredSetFallsBackToHistoricalError(t *testing.T) { // substring-matching it (mirror-pipeline does, across a vendor bump) would // classify a divergence or an unknown state as the old benign no-op. func TestEmptySourceSentinelsDoNotCarryHistoricalMessage(t *testing.T) { - for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrSourceEmptyTargetPopulated} { + for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrTargetEmptyUnverified, ErrSourceEmptyTargetPopulated} { // Substring, not equality: a sentinel that merely CONTAINS the phrase // is matched by such a caller just as surely as one that equals it. if strings.Contains(sentinel.Error(), "no source refs matched") { diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 8a5c0d64..cd817675 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -101,6 +101,17 @@ type Config struct { // observation to corroborate it. SourceAssertedEmpty bool + // TargetAssertedEmpty is the same statement about the TARGET, required for + // the same reason rather than as belt-and-braces: receive.hideRefs omits + // matching refs from receive-pack's advertisement, so a populated target + // can advertise nothing but the capabilities^{} sentinel and read as + // empty. Worse than the source case, because receive.hideRefs and + // uploadpack.hideRefs are separate settings — a ref hidden from the push + // side is still served to fetchers, so a target wrongly judged empty is + // one whose readers see refs the source does not have. Verified against + // git 2.53. + TargetAssertedEmpty bool + // AllowEmptySource opts into treating a VERIFIED-empty source as an // outcome rather than an error, in replicate mode only. Off by default: // with it unset, an empty source fails exactly as it always has, so no @@ -704,6 +715,11 @@ type targetSession struct { features gitproto.TargetFeatures policy planner.RelayTargetPolicy pusher *gitproto.Pusher + // skippedRefNames are advertised target ref names dropped as invalid. + // Retained, not just warned about, because their absence is load-bearing + // for any caller reasoning about an EMPTY target: names dropped here + // leave refMap empty while the target plainly holds refs. + skippedRefNames []string } // newSession performs the shared setup: protocol validation, mapping validation, @@ -811,6 +827,7 @@ func newSession(ctx context.Context, cfg Config, needTarget bool) (*syncSession, // never picked as a prune candidate — the safe direction, but the // operator should know the target holds a name git would reject. gitproto.WarnSkippedRefNames(targetConn.ProgressWriter(), "target", skippedTargetRefs) + s.target.skippedRefNames = skippedTargetRefs targetRefMap := gitproto.RefHashMap(targetRefSlice) targetFeatures := gitproto.TargetFeaturesFromAdvRefs(targetAdv) s.target.adv = targetAdv diff --git a/types.go b/types.go index 988ce967..e9ec73bf 100644 --- a/types.go +++ b/types.go @@ -114,33 +114,44 @@ type SyncPolicy struct { Prune bool `json:"prune"` BestEffort bool `json:"bestEffort,omitempty"` Protocol ProtocolMode `json:"protocol"` - // SourceAssertedEmpty is your authoritative statement that the source - // repository holds no refs at all — from a repository-state query, not - // from a ref listing. git-sync cannot determine this and will not guess: - // ref hiding is invisible to the client, so an unborn HEAD and an empty - // advertisement are consistent with a repository that holds refs you were - // not shown. Supply this only from a source of truth that sees past - // hiding; without it Replicate reports ErrSourceEmptyUnverified. + // SourceAssertedEmpty and TargetAssertedEmpty are your authoritative + // statements that the source and target repositories hold no refs at all + // — from a repository-state query on each side, not from a ref listing. // - // It is a necessary input, never a sufficient one: Replicate independently - // requires an empty advertisement, an unborn HEAD, no ref names dropped as - // invalid, and an empty target before it will report convergence. Those - // checks can only refuse — they never turn an absent assertion into a - // success. + // git-sync cannot determine either and will not guess. Ref hiding is + // invisible to the client by design, on both legs and independently: + // uploadpack.hideRefs can reduce a populated source's ls-refs response to + // an unborn-HEAD line alone, and receive.hideRefs can reduce a populated + // target's receive-pack advertisement to the bare capabilities^{} + // sentinel. Neither is distinguishable from a genuinely empty repository. + // The target case is the sharper one: receive.hideRefs and + // uploadpack.hideRefs are separate settings, so a ref hidden from the push + // side is still served to fetchers — a target wrongly judged empty is one + // whose readers see refs the source does not have. + // + // Supply these only from a source of truth that sees past hiding. Without + // them Replicate reports ErrSourceEmptyUnverified or + // ErrTargetEmptyUnverified and never claims convergence. + // + // They are necessary inputs, never sufficient ones: Replicate + // independently requires an empty advertisement on each side, an unborn + // HEAD on the source, no ref names dropped as invalid on either, and no + // visible target ref, before it will report convergence. Those checks can + // only refuse — they never turn an absent assertion into a success. SourceAssertedEmpty bool `json:"sourceAssertedEmpty,omitempty"` + TargetAssertedEmpty bool `json:"targetAssertedEmpty,omitempty"` // AllowEmptySource opts into treating a verified-empty source as an - // outcome instead of an error. Replicate only, and only when the source - // itself confirms it has no commits (protocol v2 ls-refs=unborn) under - // an all-refs scope; a source that merely advertises no refs does not - // qualify. When the source is confirmed empty AND the target has no - // refs either, Replicate succeeds with zero plans and - // ExecutionSummary.SourceEmpty set. When the target still has refs the - // two have diverged and Replicate fails with - // ErrSourceEmptyTargetPopulated rather than deleting them. + // outcome instead of an error. Replicate only. When source and target are + // both verified empty (see the asserted-empty fields above) Replicate + // succeeds with zero plans and ExecutionSummary.SourceEmpty set; when the + // target holds refs the two have diverged and it fails with + // ErrSourceEmptyTargetPopulated rather than deleting them; when either + // side's emptiness cannot be established it fails with the matching + // unverified error. // - // Off by default: leave it unset and an empty source errors exactly as - // it always has. + // Off by default: leave it unset and an empty source errors exactly as it + // always has. AllowEmptySource bool `json:"allowEmptySource,omitempty"` } From 028a84b0f3dc3efdea0920db3e24f8c4fef8d13b Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 17:30:49 +0200 Subject: [PATCH 04/10] Address review: thread the empty-source policy through unstable, and guard the class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit unstable.Client accepted gitsync.SyncPolicy and dropped AllowEmptySource, SourceAssertedEmpty and TargetAssertedEmpty on the floor, so Plan/Sync/Replicate there could not use the feature at all — it was accepted by the API and then ignored. The interesting part is why no test failed. unstable already had a test asserting that "advanced options" propagate, and it enumerates the fields it checks by hand, so it covered exactly what someone had remembered to add to it. A newly declared policy field is therefore invisible to it by construction. That is the same shape as the two protocol findings on this branch: the check existed, and the check's own blind spot was the bug. So both config builders now get a reflection guard: for every bool on SyncPolicy, set it alone and require the same-named bool on syncer.Config to be set. A new policy bool is covered the moment it is declared, and the test fails until it is threaded — verified by removing one assignment and watching it go red, rather than trusting that it would. A field whose config counterpart is deliberately named differently, or deliberately absent, is meant to be listed in the skip map with a reason instead of quietly renamed to pass. Also corrects the ErrNoRefsSelected doc, which described the empty-source errors below it as meaning the source has "no refs AT ALL". That contradicts the fail-closed contract those errors exist to express: they cover a source that ADVERTISED no refs, which is deliberately the weaker statement, because whether the repository really holds none is not something a client can determine. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01714HJZAqpgwuwp6fcMWEhG Entire-Checkpoint: 01M0JF4F1CK0PDKS2AG0JF5DYN --- client_test.go | 44 +++++++++++++++++++++++++++++++++ errors.go | 9 ++++--- unstable/client.go | 3 +++ unstable/client_test.go | 55 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 4 deletions(-) diff --git a/client_test.go b/client_test.go index 44ea21da..24fd636c 100644 --- a/client_test.go +++ b/client_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "os" + "reflect" "testing" git "github.com/go-git/go-git/v6" @@ -294,3 +295,46 @@ func (s *smartHTTPRepoServer) writeReceivePackReport(w http.ResponseWriter, repo type nopWriteCloser struct{ io.Writer } func (nopWriteCloser) Close() error { return nil } + +// The stable Client's config builder gets the same reflection guard as +// unstable's, for the same reason: an enumerated list of fields only covers +// what someone remembered to add, so a newly declared policy bool can be +// accepted by the API and silently ignored with every test still green. See +// unstable's TestBuildSyncConfigThreadsEveryPolicyBool — that is where this +// class of omission was actually found. +func TestBuildSyncConfigThreadsEveryPolicyBool(t *testing.T) { + skip := map[string]string{} + + policyType := reflect.TypeOf(SyncPolicy{}) + for i := range policyType.NumField() { + field := policyType.Field(i) + if field.Type.Kind() != reflect.Bool { + continue + } + if reason, ok := skip[field.Name]; ok { + t.Logf("skipping %s: %s", field.Name, reason) + continue + } + t.Run(field.Name, func(t *testing.T) { + policy := SyncPolicy{} + reflect.ValueOf(&policy).Elem().FieldByName(field.Name).SetBool(true) + + cfg, err := New(Options{}).buildSyncConfig(context.Background(), SyncRequest{ + Source: Endpoint{URL: "https://source.example/repo.git"}, + Target: Endpoint{URL: "https://target.example/repo.git"}, + Policy: policy, + }, false) + if err != nil { + t.Fatalf("buildSyncConfig: %v", err) + } + + got := reflect.ValueOf(cfg).FieldByName(field.Name) + if !got.IsValid() { + t.Fatalf("syncer.Config has no %s field; thread it, or add it to skip with a reason", field.Name) + } + if !got.Bool() { + t.Errorf("SyncPolicy.%s = true was dropped by buildSyncConfig", field.Name) + } + }) + } +} diff --git a/errors.go b/errors.go index 574f7483..4eb548f5 100644 --- a/errors.go +++ b/errors.go @@ -35,10 +35,11 @@ type RefRejectedError = gitproto.RefRejectedError // repository whose only refs are under refs/pull/* selects nothing once that // namespace is excluded. Test for it with errors.Is. // -// It is deliberately distinct from the empty-source errors below, which mean -// the source has no refs AT ALL. Before these existed both cases shared one -// message and callers could not tell "nothing to mirror" from "nothing -// matched". +// It is deliberately distinct from the empty-source errors below, which cover +// a source that ADVERTISED no refs — a weaker statement on purpose, since +// whether such a repository really holds none is not something a client can +// determine. Before these existed both cases shared one message and callers +// could not tell "nothing was advertised" from "nothing matched". var ErrNoRefsSelected = syncer.ErrNoRefsSelected // ErrSourceEmptyUnverified is returned (wrapped) by Replicate under diff --git a/unstable/client.go b/unstable/client.go index 3913d7b4..babd319d 100644 --- a/unstable/client.go +++ b/unstable/client.go @@ -277,6 +277,9 @@ func (c *Client) buildSyncConfig(ctx context.Context, req SyncRequest) (syncer.C ForceBlind: req.Policy.ForceBlind, Prune: req.Policy.Prune, BestEffort: req.Policy.BestEffort, + AllowEmptySource: req.Policy.AllowEmptySource, + SourceAssertedEmpty: req.Policy.SourceAssertedEmpty, + TargetAssertedEmpty: req.Policy.TargetAssertedEmpty, MaxPackBytes: req.Options.MaxPackBytes, TargetMaxPackBytes: req.Options.TargetMaxPackBytes, TargetMaxRefUpdates: req.Options.TargetMaxRefUpdates, diff --git a/unstable/client_test.go b/unstable/client_test.go index 09b421f3..a3638f25 100644 --- a/unstable/client_test.go +++ b/unstable/client_test.go @@ -3,6 +3,7 @@ package unstable import ( "context" "net/http" + "reflect" "testing" "github.com/go-git/go-git/v6/plumbing" @@ -150,3 +151,57 @@ func TestBuildFetchConfigThreadsMaterializedMaxObjects(t *testing.T) { t.Errorf("MaterializedMaxObjects = %d, want 1234", cfg.MaterializedMaxObjects) } } + +// Every bool on gitsync.SyncPolicy must reach the syncer config, checked by +// reflection rather than by an enumerated list. +// +// This exists because the hand-written test above did not catch three policy +// fields that buildSyncConfig silently dropped: a list of fields only covers +// what someone remembered to add to it, so the failure mode is a new policy +// field that is accepted by the API and then ignored, with no test going red. +// Reflection inverts that — a new bool is covered the moment it is declared, +// and this fails until it is threaded. +// +// Matching is by identical field name in syncer.Config, which is the +// convention every policy bool follows today. A future policy field whose +// config counterpart is deliberately named differently (or deliberately +// absent) will fail here and should be added to skip with a reason, not +// renamed to satisfy the test. +func TestBuildSyncConfigThreadsEveryPolicyBool(t *testing.T) { + skip := map[string]string{} + + policyType := reflect.TypeOf(gitsync.SyncPolicy{}) + for i := range policyType.NumField() { + field := policyType.Field(i) + if field.Type.Kind() != reflect.Bool { + continue + } + if reason, ok := skip[field.Name]; ok { + t.Logf("skipping %s: %s", field.Name, reason) + continue + } + t.Run(field.Name, func(t *testing.T) { + // One field at a time, so a failure names the culprit and no + // mutually-exclusive pair is ever set together. + policy := gitsync.SyncPolicy{} + reflect.ValueOf(&policy).Elem().FieldByName(field.Name).SetBool(true) + + cfg, err := New(Options{HTTPClient: &http.Client{}}).buildSyncConfig(context.Background(), SyncRequest{ + Source: gitsync.Endpoint{URL: "https://source.example/repo.git"}, + Target: gitsync.Endpoint{URL: "https://target.example/repo.git"}, + Policy: policy, + }) + if err != nil { + t.Fatalf("buildSyncConfig: %v", err) + } + + got := reflect.ValueOf(cfg).FieldByName(field.Name) + if !got.IsValid() { + t.Fatalf("syncer.Config has no %s field; thread it, or add it to skip with a reason", field.Name) + } + if !got.Bool() { + t.Errorf("SyncPolicy.%s = true was dropped by buildSyncConfig", field.Name) + } + }) + } +} From 3487798df0689da2318a204d95ce7247e70dc8b4 Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 19:46:49 +0200 Subject: [PATCH 05/10] Thread Scope.ExcludeRefs through unstable's config builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit unstable's buildSyncConfig, buildBootstrapConfig and buildFetchConfig each forwarded Scope.ExcludeRefPrefixes but dropped Scope.ExcludeRefs; the stable client threads both. Under Policy{Prune:true} planner.IsRefExcluded therefore never matched, so a ref the caller had explicitly reserved — a directory-anchor name like refs/heads/entire — became a prune candidate and was deleted from the target, and overwritten from the source when present. The reflection guard added to catch exactly this class walked only bool fields on SyncPolicy, so it could not see a dropped RefScope slice. It now covers both structs, and the two near-identical copies of it (which had already begun to drift) are replaced by one implementation in internal/syncertest: - exported-field filter: an unexported bool on SyncPolicy previously made both copies panic inside SetBool instead of naming the field; - kind check on the config side: a same-named field of a different type panicked at Bool() rather than reaching the Fatalf that tells the author to thread it; - the always-empty `skip` map is now a parameter, so its lookup branch is reachable rather than dead. Bootstrap and Fetch get the same guard as Sync, since they take the same RefScope and dropped the same field. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JvpGRBapBppY4xh2x38kDL Entire-Checkpoint: 01M0JPXF237GDW2AMXECSEYYWV --- client_test.go | 59 +++++++--------- internal/syncertest/configguard.go | 91 +++++++++++++++++++++++++ unstable/client.go | 3 + unstable/client_test.go | 104 ++++++++++++++++------------- 4 files changed, 177 insertions(+), 80 deletions(-) create mode 100644 internal/syncertest/configguard.go diff --git a/client_test.go b/client_test.go index 24fd636c..c1832550 100644 --- a/client_test.go +++ b/client_test.go @@ -9,7 +9,6 @@ import ( "net/http" "net/http/httptest" "os" - "reflect" "testing" git "github.com/go-git/go-git/v6" @@ -301,40 +300,32 @@ func (nopWriteCloser) Close() error { return nil } // what someone remembered to add, so a newly declared policy bool can be // accepted by the API and silently ignored with every test still green. See // unstable's TestBuildSyncConfigThreadsEveryPolicyBool — that is where this -// class of omission was actually found. +// class of omission was actually found. Both call one shared implementation so +// the two copies cannot drift. func TestBuildSyncConfigThreadsEveryPolicyBool(t *testing.T) { - skip := map[string]string{} - - policyType := reflect.TypeOf(SyncPolicy{}) - for i := range policyType.NumField() { - field := policyType.Field(i) - if field.Type.Kind() != reflect.Bool { - continue - } - if reason, ok := skip[field.Name]; ok { - t.Logf("skipping %s: %s", field.Name, reason) - continue + syncertest.AssertFieldsThreaded(t, nil, func(t *testing.T, policy SyncPolicy) any { + cfg, err := New(Options{}).buildSyncConfig(context.Background(), SyncRequest{ + Source: Endpoint{URL: "https://source.example/repo.git"}, + Target: Endpoint{URL: "https://target.example/repo.git"}, + Policy: policy, + }, false) + if err != nil { + t.Fatalf("buildSyncConfig: %v", err) } - t.Run(field.Name, func(t *testing.T) { - policy := SyncPolicy{} - reflect.ValueOf(&policy).Elem().FieldByName(field.Name).SetBool(true) - - cfg, err := New(Options{}).buildSyncConfig(context.Background(), SyncRequest{ - Source: Endpoint{URL: "https://source.example/repo.git"}, - Target: Endpoint{URL: "https://target.example/repo.git"}, - Policy: policy, - }, false) - if err != nil { - t.Fatalf("buildSyncConfig: %v", err) - } + return cfg + }) +} - got := reflect.ValueOf(cfg).FieldByName(field.Name) - if !got.IsValid() { - t.Fatalf("syncer.Config has no %s field; thread it, or add it to skip with a reason", field.Name) - } - if !got.Bool() { - t.Errorf("SyncPolicy.%s = true was dropped by buildSyncConfig", field.Name) - } - }) - } +func TestBuildSyncConfigThreadsEveryScopeField(t *testing.T) { + syncertest.AssertFieldsThreaded(t, nil, func(t *testing.T, scope RefScope) any { + cfg, err := New(Options{}).buildSyncConfig(context.Background(), SyncRequest{ + Source: Endpoint{URL: "https://source.example/repo.git"}, + Target: Endpoint{URL: "https://target.example/repo.git"}, + Scope: scope, + }, false) + if err != nil { + t.Fatalf("buildSyncConfig: %v", err) + } + return cfg + }) } diff --git a/internal/syncertest/configguard.go b/internal/syncertest/configguard.go new file mode 100644 index 00000000..5e969746 --- /dev/null +++ b/internal/syncertest/configguard.go @@ -0,0 +1,91 @@ +package syncertest + +import ( + "reflect" + "testing" +) + +// probeValue returns a distinctive non-zero value for a field the guard can +// drive, and reports whether the kind is one it knows how to drive at all. +// +// Bools and string slices are the two shapes every request-edge scope and +// policy field uses today. Kinds outside that set are skipped rather than +// guessed at: a false pass is worse than an uncovered field, and an unsupported +// kind shows up as a missing subtest rather than as a green assertion. +func probeValue(ft reflect.Type) (reflect.Value, bool) { + if ft.Kind() == reflect.Bool { + return reflect.ValueOf(true), true + } + if ft.Kind() == reflect.Slice && ft.Elem().Kind() == reflect.String { + probe := reflect.MakeSlice(ft, 1, 1) + probe.Index(0).SetString("refs/heads/threading-probe") + return probe, true + } + return reflect.Value{}, false +} + +// AssertFieldsThreaded drives a reflection-based "no field silently dropped" +// check over a request-edge struct. +// +// An enumerated list of assertions only ever covers what someone remembered to +// add to it, so the failure mode is a newly declared scope or policy field that +// the API accepts and then ignores, with no test going red. Reflection inverts +// that: a new field is covered the moment it is declared, and this fails until +// it is threaded. Scope.ExcludeRefs was dropped by three of unstable's config +// builders for exactly as long as the guard only walked policy bools. +// +// T is the request-side struct (gitsync.SyncPolicy, gitsync.RefScope), inferred +// from build. For each exported field of a supported kind, build is called with +// that one field set and must return the syncer.Config the request produced; +// the config is then required to carry a same-named field holding the same +// value. +// +// Matching is by identical field name, which is the convention every scope and +// policy field follows today. A future field whose config counterpart is +// deliberately named differently — or deliberately absent — fails here and +// belongs in skip with a reason, rather than being renamed to satisfy a test. +func AssertFieldsThreaded[T any](t *testing.T, skip map[string]string, build func(*testing.T, T) any) { + t.Helper() + var zero T + inType := reflect.TypeOf(zero) + for i := range inType.NumField() { + field := inType.Field(i) + // Unexported fields cannot be set through reflection: enumerating one + // panics inside Set rather than reporting anything useful, and no field + // a caller can populate at the request edge is unexported anyway. + if !field.IsExported() { + continue + } + probe, drivable := probeValue(field.Type) + if !drivable { + continue + } + if reason, skipped := skip[field.Name]; skipped { + t.Logf("skipping %s: %s", field.Name, reason) + continue + } + t.Run(field.Name, func(t *testing.T) { + // One field at a time, so a failure names the culprit and no + // mutually-exclusive pair is ever set together. + var populated T + reflect.ValueOf(&populated).Elem().FieldByName(field.Name).Set(probe) + + cfg := reflect.ValueOf(build(t, populated)) + got := cfg.FieldByName(field.Name) + if !got.IsValid() { + t.Fatalf("syncer.Config has no %s field; thread it, or add it to skip with a reason", field.Name) + } + // A same-named field of a different kind is reported rather than + // panicked through: that is itself the silent-drop case this guard + // exists to name, and a reflect panic names nothing. + if got.Kind() != probe.Kind() { + t.Fatalf("%s.%s is %s but syncer.Config.%s is %s; thread it explicitly, or add it to skip with a reason", + inType.Name(), field.Name, probe.Kind(), field.Name, got.Kind()) + } + if !reflect.DeepEqual(got.Interface(), probe.Interface()) { + t.Errorf("%s.%s = %v was dropped by the config builder (got %v)", + inType.Name(), field.Name, probe.Interface(), got.Interface()) + } + }) + } +} diff --git a/unstable/client.go b/unstable/client.go index babd319d..50ddaefa 100644 --- a/unstable/client.go +++ b/unstable/client.go @@ -267,6 +267,7 @@ func (c *Client) buildSyncConfig(ctx context.Context, req SyncRequest) (syncer.C Mappings: validationMappings(req.Scope.Mappings), AllRefs: req.Scope.AllRefs, ExcludeRefPrefixes: append([]string(nil), req.Scope.ExcludeRefPrefixes...), + ExcludeRefs: append([]string(nil), req.Scope.ExcludeRefs...), IncludeTags: req.Policy.IncludeTags, DryRun: req.DryRun, ShowStats: req.Options.CollectStats, @@ -307,6 +308,7 @@ func (c *Client) buildBootstrapConfig(ctx context.Context, req BootstrapRequest) Mappings: validationMappings(req.Scope.Mappings), AllRefs: req.Scope.AllRefs, ExcludeRefPrefixes: append([]string(nil), req.Scope.ExcludeRefPrefixes...), + ExcludeRefs: append([]string(nil), req.Scope.ExcludeRefs...), IncludeTags: req.IncludeTags, BestEffort: req.BestEffort, ShowStats: req.Options.CollectStats, @@ -333,6 +335,7 @@ func (c *Client) buildFetchConfig(ctx context.Context, req FetchRequest) (syncer Mappings: validationMappings(req.Scope.Mappings), AllRefs: req.Scope.AllRefs, ExcludeRefPrefixes: append([]string(nil), req.Scope.ExcludeRefPrefixes...), + ExcludeRefs: append([]string(nil), req.Scope.ExcludeRefs...), IncludeTags: req.IncludeTags, ShowStats: req.Options.CollectStats, MeasureMemory: req.Options.MeasureMemory, diff --git a/unstable/client_test.go b/unstable/client_test.go index a3638f25..9bb93fc4 100644 --- a/unstable/client_test.go +++ b/unstable/client_test.go @@ -3,12 +3,12 @@ package unstable import ( "context" "net/http" - "reflect" "testing" "github.com/go-git/go-git/v6/plumbing" "entire.io/entire/git-sync" + "entire.io/entire/git-sync/internal/syncertest" ) func TestBuildSyncConfigCarriesAdvancedOptions(t *testing.T) { @@ -152,56 +152,68 @@ func TestBuildFetchConfigThreadsMaterializedMaxObjects(t *testing.T) { } } -// Every bool on gitsync.SyncPolicy must reach the syncer config, checked by -// reflection rather than by an enumerated list. +// Every policy bool and every scope ref list must reach the syncer config, +// checked by reflection rather than by an enumerated list. The guard itself +// lives in internal/syncertest so this and the stable client's copy cannot +// drift apart; see syncertest.AssertFieldsThreaded for why it is shaped this +// way. // -// This exists because the hand-written test above did not catch three policy -// fields that buildSyncConfig silently dropped: a list of fields only covers -// what someone remembered to add to it, so the failure mode is a new policy -// field that is accepted by the API and then ignored, with no test going red. -// Reflection inverts that — a new bool is covered the moment it is declared, -// and this fails until it is threaded. -// -// Matching is by identical field name in syncer.Config, which is the -// convention every policy bool follows today. A future policy field whose -// config counterpart is deliberately named differently (or deliberately -// absent) will fail here and should be added to skip with a reason, not -// renamed to satisfy the test. +// Scope is covered alongside Policy because a policy-only guard is what let +// Scope.ExcludeRefs stay dropped here (and in buildBootstrapConfig and +// buildFetchConfig) while the stable client threaded it. func TestBuildSyncConfigThreadsEveryPolicyBool(t *testing.T) { - skip := map[string]string{} - - policyType := reflect.TypeOf(gitsync.SyncPolicy{}) - for i := range policyType.NumField() { - field := policyType.Field(i) - if field.Type.Kind() != reflect.Bool { - continue + syncertest.AssertFieldsThreaded(t, nil, func(t *testing.T, policy gitsync.SyncPolicy) any { + cfg, err := New(Options{HTTPClient: &http.Client{}}).buildSyncConfig(context.Background(), SyncRequest{ + Source: gitsync.Endpoint{URL: "https://source.example/repo.git"}, + Target: gitsync.Endpoint{URL: "https://target.example/repo.git"}, + Policy: policy, + }) + if err != nil { + t.Fatalf("buildSyncConfig: %v", err) } - if reason, ok := skip[field.Name]; ok { - t.Logf("skipping %s: %s", field.Name, reason) - continue + return cfg + }) +} + +func TestBuildSyncConfigThreadsEveryScopeField(t *testing.T) { + syncertest.AssertFieldsThreaded(t, nil, func(t *testing.T, scope gitsync.RefScope) any { + cfg, err := New(Options{HTTPClient: &http.Client{}}).buildSyncConfig(context.Background(), SyncRequest{ + Source: gitsync.Endpoint{URL: "https://source.example/repo.git"}, + Target: gitsync.Endpoint{URL: "https://target.example/repo.git"}, + Scope: scope, + }) + if err != nil { + t.Fatalf("buildSyncConfig: %v", err) } - t.Run(field.Name, func(t *testing.T) { - // One field at a time, so a failure names the culprit and no - // mutually-exclusive pair is ever set together. - policy := gitsync.SyncPolicy{} - reflect.ValueOf(&policy).Elem().FieldByName(field.Name).SetBool(true) + return cfg + }) +} - cfg, err := New(Options{HTTPClient: &http.Client{}}).buildSyncConfig(context.Background(), SyncRequest{ - Source: gitsync.Endpoint{URL: "https://source.example/repo.git"}, - Target: gitsync.Endpoint{URL: "https://target.example/repo.git"}, - Policy: policy, - }) - if err != nil { - t.Fatalf("buildSyncConfig: %v", err) - } +// Bootstrap and Fetch take the same RefScope and dropped the same field, so +// they get the same guard rather than a comment promising someone will remember. +func TestBuildBootstrapConfigThreadsEveryScopeField(t *testing.T) { + syncertest.AssertFieldsThreaded(t, nil, func(t *testing.T, scope gitsync.RefScope) any { + cfg, err := New(Options{HTTPClient: &http.Client{}}).buildBootstrapConfig(context.Background(), BootstrapRequest{ + Source: gitsync.Endpoint{URL: "https://source.example/repo.git"}, + Target: gitsync.Endpoint{URL: "https://target.example/repo.git"}, + Scope: scope, + }) + if err != nil { + t.Fatalf("buildBootstrapConfig: %v", err) + } + return cfg + }) +} - got := reflect.ValueOf(cfg).FieldByName(field.Name) - if !got.IsValid() { - t.Fatalf("syncer.Config has no %s field; thread it, or add it to skip with a reason", field.Name) - } - if !got.Bool() { - t.Errorf("SyncPolicy.%s = true was dropped by buildSyncConfig", field.Name) - } +func TestBuildFetchConfigThreadsEveryScopeField(t *testing.T) { + syncertest.AssertFieldsThreaded(t, nil, func(t *testing.T, scope gitsync.RefScope) any { + cfg, err := New(Options{HTTPClient: &http.Client{}}).buildFetchConfig(context.Background(), FetchRequest{ + Source: gitsync.Endpoint{URL: "https://source.example/repo.git"}, + Scope: scope, }) - } + if err != nil { + t.Fatalf("buildFetchConfig: %v", err) + } + return cfg + }) } From b7dea6e46b73eb30817ff52f5b88409e4882c55e Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 19:47:20 +0200 Subject: [PATCH 06/10] Make the empty-source policy reachable, scoped, and honestly reported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review of the empty-source work found the policy inert or wrong on several of its own headline paths. Reachability. The emptiness decision hung off len(desiredRefs) == 0, after planning. For any request pinning refs by mapping, planner.BuildDesiredRefs errors on the absent mapped source ref first, so a mapping-scoped mirror of a genuinely empty repository got "source ref X not found" — matching none of the new sentinels — on exactly the state the policy exists to make succeed. An empty advertisement is now resolved before planning, where under AllRefs it is already a complete observation. Gated on the opt-in, so a caller that never asked for this still gets the planner's error verbatim. Scope. The target-populated check counted every advertised target ref, ignoring exclusions and zero hashes, unlike every other consumer of the target ref map (replicateCanBootstrap, addPruneCandidates). A mirror that trims refs/pull/* whose target held only refs/pull/1/head was reported as permanently diverged over a ref the run would neither push nor prune — and refs/pull/* is the namespace this package's own docs cite as the benign case. Contract. ExecutionSummary.SourceEmpty is renamed Converged: it requires both sides verified, so it was false in every other outcome including the diverged one where the source WAS verified empty. It also loses `omitempty` — for the field whose whole purpose is separating a converged run from a no-op, "false" and "this binary has no such field" must not be the same JSON — and is now rendered by Result.Lines(), so the text output cmd/git-sync actually prints is no longer byte-identical to an ordinary zero-work sync. The converged result carries the Relay fields every other successful replicate return sets. Validation. AllowEmptySource silently required AllRefs, was silently discarded outside replicate mode, and could never succeed over protocol v1, whose ls-refs has no unborn signal. All three are now rejected at the request edge instead of threaded in and dropped. The v1 case validation cannot see — an "auto" SSH source that falls back mid-run — reports that the protocol cannot carry the signal, rather than "did not report an unborn HEAD", which reads as the server withholding refs and points an operator at a hideRefs misconfiguration or a compromised source. A v2 source not advertising ls-refs=unborn was misreported the same way and is now distinguished too. Corroboration. RefService.SkippedRefNames was populated at one of four construction sites, leaving the invalid-name cross-check vacuous on v1; it fails closed today only because the !HeadUnborn check happens to run first. It is now a count set on every path through newV1RefService. A count also stops the slice being pinned to a struct that outlives the pack transfer — megabytes retained on a source advertising many invalid names, to answer a boolean. Coverage. Every test hand-built a syncSession, so the chain the design rests on (ls-refs "unborn" -> decodeV2LSRefs -> RefService.HeadUnborn -> Config -> converged Result) had none: deleting the request argument left the whole suite green. The in-package fake v2 server advertised ls-refs=unborn but never emitted an unborn line; it does now, and Run is exercised end to end. Reverting any fix here turns a test red. The argument staying ungated is deliberate — it costs no round trip and lets Probe report an unborn HEAD without a convergence policy — and is pinned by tests, along with the advertisement gate that does matter. Also: guard s.target, which is legitimately nil on Fetch and target-less Probe sessions; correct ErrNoRefsSelected's doc, which named two causes unreachable by construction; label the divergence count; document the unborn argument in docs/protocol.md and correct the CHANGELOG's "nothing changes for existing callers", which the ungated request falsifies for every v2 caller. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JvpGRBapBppY4xh2x38kDL Entire-Checkpoint: 01M0JPYDW1P7C7KB0TS9JM24P7 --- CHANGELOG.md | 8 +- client.go | 7 + client_test.go | 41 +++ docs/protocol.md | 15 +- errors.go | 19 +- internal/gitproto/fetch_test.go | 12 - internal/gitproto/refs.go | 71 +++-- internal/syncer/empty_source.go | 192 ++++++++++-- internal/syncer/empty_source_test.go | 426 ++++++++++++++++++++++++++- internal/syncer/integration_test.go | 46 ++- internal/syncer/syncer.go | 60 +++- results.go | 16 +- types.go | 50 +++- 13 files changed, 852 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1dba677..a71b4be8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,15 +39,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Added -- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when a source of truth says so.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering unrelated conditions: the source has no refs, the source has refs the requested scope excluded, and the source has refs this reader was never shown. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports them separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` / `ErrTargetEmptyUnverified` when either side's emptiness could not be established, `ErrSourceEmptyTargetPopulated` when the source is empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.SourceEmpty` when source and target are both empty and therefore already agree. +- **`SyncPolicy.AllowEmptySource` — an empty source can now be an outcome instead of an error, when a source of truth says so.** Replicate previously failed any run whose planning produced no desired refs, with one message (`no source refs matched`) covering unrelated conditions: the source has no refs, the source has refs the requested scope excluded, and the source has refs this reader was never shown. A caller could not tell them apart, and the first is not always a failure — a mirror of a repository that has never been pushed to is trivially up to date. With the policy set, Replicate reports them separately: `ErrNoRefsSelected` when the source does advertise refs, `ErrSourceEmptyUnverified` / `ErrTargetEmptyUnverified` when either side's emptiness could not be established, `ErrSourceEmptyTargetPopulated` when the source is empty while the target still holds refs (a real divergence — refused rather than converged, since converging means deleting them), and a zero-plan success with `ExecutionSummary.Converged` when source and target are both empty and therefore already agree. **git-sync does not decide that a repository is empty, and will not guess.** It cannot: ref hiding is designed to be invisible to the client, so a hidden ref and an absent one are the same observation. An unborn HEAD does not close the gap either — git emits that line for any dangling HEAD, so a repository holding `refs/heads/other` with HEAD pointed at a never-created `refs/heads/main` reports unborn, and hiding that branch reduces its entire advertisement to the unborn line alone (verified against git 2.53). The assertion is therefore an input: `SyncPolicy.SourceAssertedEmpty`, which the caller supplies from a repository-state query that sees past hiding. The target needs the same treatment, via `SourceAssertedEmpty`'s counterpart `TargetAssertedEmpty`, and for a sharper reason: `receive.hideRefs` omits matching refs from receive-pack's advertisement, so a populated target can advertise nothing but the bare `capabilities^{}` sentinel — and because `receive.hideRefs` and `uploadpack.hideRefs` are separate settings, a ref hidden from the push side is still served to fetchers, so a target wrongly judged empty is one whose readers see refs the source does not have. - What git-sync contributes is a consistency check on those claims, and it only ever refuses. Before reporting convergence it independently requires an empty advertisement on each side, an unborn HEAD on the source (now requested via protocol v2's `ls-refs=unborn` where the server advertises it, and surfaced as `RefService.HeadUnborn`), no advertised ref name dropped as invalid on either side, and no visible target ref. None of those can promote an absent assertion into a success, so a caller cannot get a false converge out of a compliant server, and a caller that supplies no assertion gets `ErrSourceEmptyUnverified` or `ErrTargetEmptyUnverified` no matter what the wire says. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. + What git-sync contributes is a consistency check on those claims, and it only ever refuses. Before reporting convergence it independently requires an empty advertisement on each side, an unborn HEAD on the source (now requested via protocol v2's `ls-refs=unborn` where the server advertises it, and surfaced as `RefService.HeadUnborn`), no advertised ref name dropped as invalid on either side, and no visible target ref within the request's scope (an excluded namespace the run would neither push nor prune is not divergence). None of those can promote an absent assertion into a success, so a caller cannot get a false converge out of a compliant server, and a caller that supplies no assertion gets `ErrSourceEmptyUnverified` or `ErrTargetEmptyUnverified` no matter what the wire says. The unborn line's `symref-target` is deliberately not surfaced as `SourceHEAD`, which consumers read as a branch that exists. - Off by default, so nothing changes for existing callers: without the opt-in — or under a narrower scope, where an empty desired set says nothing about the repository as a whole — an empty source still fails with the historical message, and the new sentinels deliberately do not carry that text so a caller still matching on it cannot mistake a divergence for the old benign no-op. + Off by default: without the opt-in an empty source still fails with the historical message, and the new sentinels deliberately do not carry that text so a caller still matching on it cannot mistake a divergence for the old benign no-op. The policy is replicate-only and requires `RefScope.AllRefs` — under a narrower scope the source ref listing is itself narrowed, so an empty result says nothing about the repository as a whole — and both requirements are now rejected at the request edge rather than accepted and silently discarded. Convergence also requires protocol v2 on the source leg, since the unborn cross-check has no v1 equivalent: `ProtocolV1` is rejected at the request edge, and an `auto` source that falls back to v1 mid-run reports that the protocol cannot carry the signal rather than implying the server withheld refs. + + One thing does change for every v2 caller, opt-in or not: `unborn` is appended to each `ls-refs` request whose server advertises support for it (see `docs/protocol.md`). It adds no round trip and a source with commits answers exactly as before, but the request bytes differ, so a test asserting on the exact ls-refs body will need updating. Only the *reader* of the resulting flag is gated on the policy. - A `Vulnerability Scan` workflow running `govulncheck ./...` on pull requests, pushes to main, and a weekly schedule. The existing lint suite cannot see this class of issue, and the weekly run matters because advisories are published against versions already in go.mod — without it, a newly disclosed vulnerability goes unreported until someone happens to open a PR. diff --git a/client.go b/client.go index 3179748c..99ad309d 100644 --- a/client.go +++ b/client.go @@ -179,6 +179,13 @@ func validateSyncFields(source, target Endpoint, scope RefScope, policy SyncPoli if _, err := validation.ValidateMappings(validationMappings(scope.Mappings), scope.AllRefs); err != nil { return fmt.Errorf("validate mappings: %w", err) } + // The half of the AllowEmptySource contract that needs the scope as well as + // the policy, so it cannot live on SyncPolicy.Validate. Silently accepting + // it left the caller with the historical "no source refs matched" and no + // hint that their policy had been discarded. + if policy.AllowEmptySource && !scope.AllRefs { + return errors.New("AllowEmptySource requires Scope.AllRefs; a narrowed scope cannot establish that a repository is empty") + } return nil } diff --git a/client_test.go b/client_test.go index c1832550..5ac2f534 100644 --- a/client_test.go +++ b/client_test.go @@ -329,3 +329,44 @@ func TestBuildSyncConfigThreadsEveryScopeField(t *testing.T) { return cfg }) } + +// AllowEmptySource has two requirements that no path would otherwise report: +// the policy is replicate-only, and it needs an unscoped request. Both were +// accepted at the edge, threaded into the syncer, and then discarded — the +// caller got the historical "no source refs matched" with no hint that their +// safety policy had been ignored. +func TestValidateRejectsUnusableAllowEmptySource(t *testing.T) { + base := SyncRequest{ + Source: Endpoint{URL: "https://source.example/repo.git"}, + Target: Endpoint{URL: "https://target.example/repo.git"}, + } + + replicateUnscoped := base + replicateUnscoped.Policy = SyncPolicy{Mode: ModeReplicate, AllowEmptySource: true} + if err := replicateUnscoped.Validate(); err == nil { + t.Error("expected a scoped AllowEmptySource replicate to be rejected") + } + + syncMode := base + syncMode.Scope = RefScope{AllRefs: true} + syncMode.Policy = SyncPolicy{Mode: ModeSync, AllowEmptySource: true} + if err := syncMode.Validate(); err == nil { + t.Error("expected AllowEmptySource outside replicate to be rejected") + } + + // Mode unset defaults to sync, so it must be rejected the same way rather + // than slipping through on the zero value. + modeUnset := base + modeUnset.Scope = RefScope{AllRefs: true} + modeUnset.Policy = SyncPolicy{AllowEmptySource: true} + if err := modeUnset.Validate(); err == nil { + t.Error("expected AllowEmptySource with an unset mode to be rejected") + } + + ok := base + ok.Scope = RefScope{AllRefs: true} + ok.Policy = SyncPolicy{Mode: ModeReplicate, AllowEmptySource: true, SourceAssertedEmpty: true, TargetAssertedEmpty: true} + if err := ok.Validate(); err != nil { + t.Errorf("an unscoped replicate with the policy set must validate, got %v", err) + } +} diff --git a/docs/protocol.md b/docs/protocol.md index cce7ab17..bbc3dfa6 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -151,6 +151,7 @@ agent=git-sync/... 0001 peel symrefs +unborn ref-prefix HEAD ref-prefix refs/heads/ ref-prefix refs/tags/ @@ -159,13 +160,25 @@ ref-prefix refs/tags/ The `peel` and `symrefs` arguments are ls-refs request features that ask the server to include peeled object IDs (for tags) and symref-target attributes. `ref-prefix HEAD` is unconditionally included so HEAD shows up in the response even when the caller only asked for `refs/heads/` or `refs/tags/`. +`unborn` asks the server to report a HEAD whose target does not exist, instead of answering with no ref lines at all. It is appended only when the server advertised `ls-refs=unborn` (protocol v2 forbids sending an unadvertised argument, and a strict server may fail the command), and it is **not** gated on any caller policy: every v2 source listing sends it — probe, plan, sync, replicate, bootstrap, fetch and convert-sha256 alike. It costs no extra round trip, and a source with commits answers exactly as it did before. Only the reader of the resulting flag is policy-gated (see `SyncPolicy.AllowEmptySource`). + The server's HEAD line then looks like: ``` HEAD symref-target:refs/heads/main ``` -`decodeV2LSRefs` parses the line, extracts the `symref-target:` attribute, and returns it as `headTarget` alongside the ref slice. HEAD itself is filtered out of the returned refs because it is a symbolic ref, not a real one — matching v1 behavior where symrefs are filtered out by the downstream `RefHashMap`. +For an unborn HEAD the server instead emits: + +``` +unborn HEAD symref-target:refs/heads/main +``` + +`decodeV2LSRefs` parses the born line, extracts the `symref-target:` attribute, and returns it as `headTarget` alongside the ref slice. HEAD itself is filtered out of the returned refs because it is a symbolic ref, not a real one — matching v1 behavior where symrefs are filtered out by the downstream `RefHashMap`. + +The unborn line is recorded as `RefService.HeadUnborn` and deliberately does **not** populate `headTarget`: consumers read a non-empty `HeadTarget` as a branch that exists on the source, and an unborn target does not. `HeadUnborn` is only ever a *disqualifier* for a caller's emptiness assertion — a repository holding `refs/heads/other` with HEAD pointed at a never-created `refs/heads/main` reports unborn too, so its presence proves nothing on its own. There is no v1 equivalent, so a v1 source (or an SSH source that falls back to v1) always leaves it false. + +Because convergence cannot be corroborated without it, `SyncPolicy.AllowEmptySource` rejects `ProtocolV1` at the request edge. `ProtocolAuto` is accepted — it negotiates v2 wherever the server supports it — but an SSH source whose v2 probe fails and falls back to v1 can only be caught mid-run, where it reports that the *protocol* cannot carry the signal rather than implying the server withheld refs. A v2 source that does not advertise `ls-refs=unborn` is reported the same way. ### Consumers diff --git a/errors.go b/errors.go index 4eb548f5..bfa893bb 100644 --- a/errors.go +++ b/errors.go @@ -28,12 +28,19 @@ var ErrTargetRefMoved = gitproto.ErrTargetRefMoved // target-ref moves also satisfy errors.Is(err, ErrTargetRefMoved). type RefRejectedError = gitproto.RefRejectedError -// ErrNoRefsSelected is returned (wrapped) by Replicate when the source -// advertises refs but the requested scope — branch selection, ref mappings, -// exclude prefixes — matched none of them. The source is healthy; the request -// asked for refs it does not have. Benign for some sources by design: a GitHub -// repository whose only refs are under refs/pull/* selects nothing once that -// namespace is excluded. Test for it with errors.Is. +// ErrNoRefsSelected is returned (wrapped) by Replicate under +// SyncPolicy.AllowEmptySource when the source advertises refs but +// RefScope.ExcludeRefPrefixes / ExcludeRefs subtracted all of them. The source +// is healthy; the request asked for refs it does not have. Benign for some +// sources by design: a GitHub repository whose only refs are under refs/pull/* +// selects nothing once that namespace is excluded. Test for it with errors.Is. +// +// Exclusions are the only scoping mechanism that reaches it, and the opt-in is +// required — like every sentinel in this family, a caller that has not asked +// for the distinction keeps receiving the historical error. Branch selection +// cannot produce it, because AllowEmptySource requires RefScope.AllRefs and +// that clears any branch filter; nor can a ref mapping, because a mapping whose +// source ref is absent fails earlier, while the source is being planned. // // It is deliberately distinct from the empty-source errors below, which cover // a source that ADVERTISED no refs — a weaker statement on purpose, since diff --git a/internal/gitproto/fetch_test.go b/internal/gitproto/fetch_test.go index 6e4bc659..f5a850a6 100644 --- a/internal/gitproto/fetch_test.go +++ b/internal/gitproto/fetch_test.go @@ -271,18 +271,6 @@ func TestDecodeV2LSRefsMalformed(t *testing.T) { } } -func TestDecodeV2LSRefsEmpty(t *testing.T) { - // Empty response (just flush). - wire := "0000" - refs, _, _, _, err := decodeV2LSRefs(bytes.NewReader([]byte(wire))) - if err != nil { - t.Fatalf("decodeV2LSRefs: %v", err) - } - if len(refs) != 0 { - t.Fatalf("expected 0 refs, got %d", len(refs)) - } -} - // An unborn HEAD is the whole point of requesting the capability: the // repository asserts it has no commits, which a caller may act on, while the // symref target stays out of HeadTarget because that ref does not exist. diff --git a/internal/gitproto/refs.go b/internal/gitproto/refs.go index 973147be..cfbf58f9 100644 --- a/internal/gitproto/refs.go +++ b/internal/gitproto/refs.go @@ -54,12 +54,36 @@ type RefService struct { // however empty it is. HeadUnborn bool - // SkippedRefNames are advertised ref names dropped as invalid (see - // PartitionRefNames). Surfaced because their absence is load-bearing for - // any caller reasoning about an empty ref set: names dropped here leave - // refs empty while the repository plainly has some, which is - // indistinguishable from emptiness by ref count alone. - SkippedRefNames []string + // SkippedRefCount is how many advertised ref names were dropped as invalid + // (see PartitionRefNames). Surfaced because a non-zero count is + // load-bearing for any caller reasoning about an empty ref set: names + // dropped here leave refs empty while the repository plainly has some, + // which is indistinguishable from emptiness by ref count alone. + // + // A count, not the names: every consumer asks only whether any were + // dropped, and the names are already reported to the operator by + // WarnSkippedRefNames at the decode boundary. Retaining the slice pinned + // it to this struct — which outlives the pack fetch and push — for the + // whole run, megabytes of it on a source advertising many invalid names, + // purely to answer a boolean. + // + // Set on every construction path, v1 and v2 alike. It was populated at + // only one of four before, leaving the corroboration silently vacuous on + // v1; newV1RefService exists so a new path cannot forget. + SkippedRefCount int +} + +// newV1RefService builds the v1 RefService. All three v1 construction sites go +// through it: hand-built literals are how SkippedRefCount came to be set on one +// path and silently left zero on the others, which reads to a caller as +// "nothing was dropped". +func newV1RefService(adv *packp.AdvRefs, skippedRefCount int) *RefService { + return &RefService{ + Protocol: "v1", + V1Adv: adv, + HeadTarget: headTargetFromAdv(adv), + SkippedRefCount: skippedRefCount, + } } // ListSourceRefs discovers refs from the source using the configured protocol mode. @@ -67,11 +91,11 @@ type RefService struct { func ListSourceRefs(ctx context.Context, conn Conn, protocolMode string, refPrefixes []string) ([]*plumbing.Reference, *RefService, error) { switch protocolMode { case "v1": - adv, refs, err := listSourceRefsV1(ctx, conn) + adv, refs, skippedCount, err := listSourceRefsV1(ctx, conn) if err != nil { return nil, nil, err } - return refs, &RefService{Protocol: "v1", V1Adv: adv, HeadTarget: headTargetFromAdv(adv)}, nil + return refs, newV1RefService(adv, skippedCount), nil case "auto", "v2": data, err := RequestInfoRefs(ctx, conn, transport.UploadPackService, GitProtocolV2) @@ -89,11 +113,11 @@ func ListSourceRefs(ctx context.Context, conn Conn, protocolMode string, refPref if !caps.Supports("ls-refs") || !caps.Supports("fetch") { return nil, nil, errors.New("source does not advertise required protocol v2 commands") } - refs, headTarget, unborn, skipped, err := listSourceRefsV2(ctx, conn, caps, refPrefixes) + refs, headTarget, unborn, skippedCount, err := listSourceRefsV2(ctx, conn, caps, refPrefixes) if err != nil { return nil, nil, err } - return refs, &RefService{Protocol: "v2", V2Caps: caps, HeadTarget: headTarget, HeadUnborn: unborn, SkippedRefNames: skipped}, nil + return refs, &RefService{Protocol: "v2", V2Caps: caps, HeadTarget: headTarget, HeadUnborn: unborn, SkippedRefCount: skippedCount}, nil } if protocolMode == "v2" { return nil, nil, errors.New("source did not negotiate protocol v2") @@ -108,7 +132,7 @@ func ListSourceRefs(ctx context.Context, conn Conn, protocolMode string, refPref return nil, nil, err } WarnSkippedRefNames(conn.ProgressWriter(), "source", skipped) - return refs, &RefService{Protocol: "v1", V1Adv: adv, HeadTarget: headTargetFromAdv(adv)}, nil + return refs, newV1RefService(adv, len(skipped)), nil default: return nil, nil, fmt.Errorf("unsupported protocol mode %q", protocolMode) @@ -116,11 +140,11 @@ func ListSourceRefs(ctx context.Context, conn Conn, protocolMode string, refPref } func listSourceRefsAutoV1(ctx context.Context, conn Conn) ([]*plumbing.Reference, *RefService, error) { - adv, refs, err := listSourceRefsV1(ctx, conn) + adv, refs, skippedCount, err := listSourceRefsV1(ctx, conn) if err != nil { return nil, nil, err } - return refs, &RefService{Protocol: "v1", V1Adv: adv, HeadTarget: headTargetFromAdv(adv)}, nil + return refs, newV1RefService(adv, skippedCount), nil } func isSSHScheme(conn Conn) bool { @@ -190,20 +214,23 @@ func AdvRefsCaps(adv *packp.AdvRefs) []string { return items } -func listSourceRefsV1(ctx context.Context, conn Conn) (*packp.AdvRefs, []*plumbing.Reference, error) { +// listSourceRefsV1 returns the advertisement, its valid refs, and how many +// names were dropped as invalid — the count travels back so the caller can put +// it on the RefService, rather than being warned about and then discarded. +func listSourceRefsV1(ctx context.Context, conn Conn) (*packp.AdvRefs, []*plumbing.Reference, int, error) { adv, err := AdvertisedRefsV1(ctx, conn, transport.UploadPackService) if err != nil { - return nil, nil, err + return nil, nil, 0, err } refs, skipped, err := AdvRefsToSlice(adv) if err != nil { - return nil, nil, err + return nil, nil, 0, err } WarnSkippedRefNames(conn.ProgressWriter(), "source", skipped) - return adv, refs, nil + return adv, refs, len(skipped), nil } -func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, prefixes []string) ([]*plumbing.Reference, plumbing.ReferenceName, bool, []string, error) { +func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, prefixes []string) ([]*plumbing.Reference, plumbing.ReferenceName, bool, int, error) { // Always include "HEAD" so the server returns the symref-target attribute // for HEAD. Without this, callers that pass only "refs/heads/" or // "refs/tags/" prefixes filter HEAD out of the response and lose the @@ -223,18 +250,18 @@ func listSourceRefsV2(ctx context.Context, conn Conn, caps *V2Capabilities, pref } body, err := EncodeCommand("ls-refs", caps.RequestCapabilities(), args) if err != nil { - return nil, "", false, nil, err + return nil, "", false, 0, err } data, err := PostRPC(ctx, conn, transport.UploadPackService, body, true, "upload-pack ls-refs") if err != nil { - return nil, "", false, nil, err + return nil, "", false, 0, err } refs, headTarget, unborn, skipped, err := decodeV2LSRefs(bytes.NewReader(data)) if err != nil { - return nil, "", false, nil, err + return nil, "", false, 0, err } WarnSkippedRefNames(conn.ProgressWriter(), "source", skipped) - return refs, headTarget, unborn, skipped, nil + return refs, headTarget, unborn, len(skipped), nil } // decodeV2LSRefs decodes an ls-refs response into its refs, the branch HEAD diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go index 32030ca2..61e92546 100644 --- a/internal/syncer/empty_source.go +++ b/internal/syncer/empty_source.go @@ -3,10 +3,13 @@ package syncer import ( "errors" "fmt" + + "entire.io/entire/git-sync/internal/planner" ) -// The empty-desired-set outcomes. Replicate reaches this family whenever -// planning produces no desired refs, which happens for unrelated reasons the +// The empty-desired-set outcomes. Replicate reaches this family whenever it +// finds nothing to act on — either the source advertised no refs at all, or +// planning produced no desired refs — which happens for unrelated reasons the // wire signal alone cannot tell apart: the source has no refs, the source has // refs the requested scope excluded, or the source has refs this reader was // never shown. Collapsing them into one error (as this package did before) @@ -14,11 +17,16 @@ import ( // a converged state, the others never are. var ( // ErrNoRefsSelected means the source DOES advertise refs, but the - // requested scope (branch selection, mappings, exclude prefixes) matched - // none of them. Nothing is wrong with the source; the request asked for - // refs it does not have. Benign and expected for some sources — e.g. a - // GitHub repository whose only refs live under refs/pull/*, which mirror - // callers deliberately exclude. + // requested exclusions (ExcludeRefPrefixes, ExcludeRefs) subtracted all of + // them. Nothing is wrong with the source; the request asked for refs it + // does not have. Benign and expected for some sources — e.g. a GitHub + // repository whose only refs live under refs/pull/*, which mirror callers + // deliberately exclude. + // + // Exclusions are the only scoping mechanism that can reach it. Branch + // selection cannot: this family requires AllRefs, and normalizeAllRefs + // clears cfg.Branches whenever AllRefs is set. Nor can a mapping, whose + // absent source ref fails inside BuildDesiredRefs first. // // Its message deliberately avoids the historical "no source refs matched" // text. A caller that substring-matches that phrase (mirror-pipeline does, @@ -76,8 +84,63 @@ var ( ErrSourceEmptyTargetPopulated = errors.New("source is empty but the target still has refs") ) -// resolveEmptyDesiredSet decides what an empty desired set means, and is the -// only place that may conclude "the source is empty and we are converged". +// validateEmptySourcePolicy rejects an empty-source policy that no path would +// consult, at the one point every entry (Run, Bootstrap, Fetch, Probe) shares. +// +// Both conditions were previously accepted, threaded into this package, and +// then silently discarded: the caller set the policy, got the historical +// "no source refs matched", and had no way to learn the policy did nothing. +// Failing at the edge is the same treatment SyncPolicy.Validate already gives +// mode-specific force flags. +func validateEmptySourcePolicy(cfg Config) error { + if !cfg.AllowEmptySource { + // The assertions are inputs to this policy alone. Set without it they + // are inert by design — the opt-in is what makes the outcome + // reachable — so they are not an error on their own. + return nil + } + if cfg.Mode != modeReplicate { + return fmt.Errorf("AllowEmptySource applies to replicate only, got mode %q", cfg.Mode) + } + // Protocol v1 has no unborn-HEAD signal at all, so the corroboration this + // policy requires can never be satisfied over it: every run would fail, + // and with a message that reads as the source withholding refs. Refusing + // the combination up front names the real cause — the caller's own + // protocol selection. "auto" is accepted: it negotiates v2 wherever the + // server supports it, and the SSH fallback to v1 is only discoverable + // mid-run (see resolveEmptySource). + if cfg.ProtocolMode == protocolModeV1 { + return errors.New("AllowEmptySource requires protocol v2 on the source; v1 cannot report an unborn HEAD, so emptiness can never be corroborated") + } + // Under a narrower scope the source advertisement is itself narrowed (see + // planner.RefPrefixes), so an empty one says nothing about the repository + // as a whole — and the target's refs, which are never scope-filtered, are + // not ours to judge against a partial view of the source. + if !cfg.AllRefs { + return errors.New("AllowEmptySource requires an unscoped request (AllRefs); a narrowed scope cannot establish that a repository is empty") + } + return nil +} + +// resolveEmptyDesiredSet decides what an empty desired set means when planning +// produced no refs to act on. +// +// It is the post-planning entry point only. An empty source ADVERTISEMENT is +// intercepted before planning (see runReplicate), because a mapping whose +// source ref is absent errors inside BuildDesiredRefs and would never let this +// run; both entry points share resolveEmptySource so they cannot disagree. +func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { + if !s.cfg.AllowEmptySource || !s.cfg.AllRefs { + return Result{}, errors.New("no source refs matched") + } + if len(s.sourceRefMap) > 0 { + return Result{}, ErrNoRefsSelected + } + return s.resolveEmptySource() +} + +// resolveEmptySource is the only place that may conclude "the source is empty +// and we are converged". // // Emptiness is NOT established here. Git offers no way to prove it: ref hiding // is designed to be invisible to the client, so a hidden ref and an absent one @@ -109,9 +172,13 @@ var ( // these can only ever REFUSE — none can promote an absent assertion into a // success — so the git signal is a consistency check on the caller's // claim, never a substitute for it. -// 5. The target advertises no refs — anything visible there is real, since -// hiding can conceal refs but never invent them, so a visible ref means -// divergence. +// 5. The target advertises no refs THIS REQUEST WOULD MANAGE — anything +// visible there is real, since hiding can conceal refs but never invent +// them, so a visible ref means divergence. Refs the request excludes are +// not ours to judge: the run would neither push nor prune them, and a +// mirror that excludes refs/pull/* while its target holds only refs/pull/* +// is converged over everything it manages. Counting them would make such a +// mirror permanently unconvergeable over a ref it never touches. // 6. The target's emptiness is asserted too (TargetAssertedEmpty) and // corroborated the same way. An empty receive-pack advertisement proves no // more than an empty ls-refs one: receive.hideRefs omits matching refs @@ -123,29 +190,49 @@ var ( // ErrTargetEmptyUnverified for the target half, ErrSourceEmptyTargetPopulated // for a target known to hold refs. Only condition 5 reports divergence; every // other failure is "unknown". -func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { +func (s *syncSession) resolveEmptySource() (Result, error) { if !s.cfg.AllowEmptySource || !s.cfg.AllRefs { return Result{}, errors.New("no source refs matched") } - if len(s.sourceRefMap) > 0 { - return Result{}, ErrNoRefsSelected - } if !s.cfg.SourceAssertedEmpty { return Result{}, fmt.Errorf("%w: no authoritative assertion from the source", ErrSourceEmptyUnverified) } - if s.sourceService == nil || !s.sourceService.HeadUnborn { + if s.sourceService == nil { + return Result{}, fmt.Errorf("%w: the source ref listing is unavailable", ErrSourceEmptyUnverified) + } + // Distinguish "the source COULD NOT report unborn" from "the source DID NOT + // report it". Only the second says anything about the repository; the first + // is a property of the protocol, and reporting it as the second tells the + // operator their server is withholding refs — implying a hideRefs + // misconfiguration or a compromised source — when nothing is wrong with it. + // + // A v1 request is refused before any I/O (see validateEmptySourcePolicy); + // this catches what validation cannot see: an "auto" SSH source whose v2 + // probe failed and fell back to v1 mid-run. + if reason, cannot := s.sourceCannotReportUnborn(); cannot { + return Result{}, fmt.Errorf("%w: %s, so emptiness cannot be corroborated over this connection", ErrSourceEmptyUnverified, reason) + } + if !s.sourceService.HeadUnborn { // HEAD's target exists while nothing was advertised, so a ref is being // withheld — the caller's assertion disagrees with the wire and loses. return Result{}, fmt.Errorf("%w: source asserted empty but did not report an unborn HEAD", ErrSourceEmptyUnverified) } - if n := len(s.sourceService.SkippedRefNames); n > 0 { + if n := s.sourceService.SkippedRefCount; n > 0 { return Result{}, fmt.Errorf("%w: source asserted empty but %d advertised ref name(s) were dropped as invalid", ErrSourceEmptyUnverified, n) } - // A target that advertises refs is populated, full stop — hiding can only - // ever conceal refs, never invent them, so anything visible here is real - // and this is divergence. - if len(s.target.refMap) > 0 { - return Result{}, fmt.Errorf("%w (%d)", ErrSourceEmptyTargetPopulated, len(s.target.refMap)) + // Every remaining condition is about the target, and this function is + // billed as the one place that may conclude convergence — so a session + // built without a target (Fetch, a target-less Probe) must be refused + // rather than dereferenced. runReplicate always has one; a future reuse + // might not. + if s.target == nil { + return Result{}, fmt.Errorf("%w: no target was listed to compare against", ErrTargetEmptyUnverified) + } + // A target that advertises refs this request manages is populated, full + // stop — hiding can only ever conceal refs, never invent them, so anything + // visible here is real and this is divergence. + if n := s.targetRefsInScope(); n > 0 { + return Result{}, fmt.Errorf("%w (%d in scope)", ErrSourceEmptyTargetPopulated, n) } // An EMPTY target advertisement proves nothing on its own, for the same // reason the source's did not, so it needs the same authoritative @@ -153,7 +240,7 @@ func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { if !s.cfg.TargetAssertedEmpty { return Result{}, fmt.Errorf("%w: no authoritative assertion from the target", ErrTargetEmptyUnverified) } - if n := len(s.target.skippedRefNames); n > 0 { + if n := s.target.skippedRefCount; n > 0 { return Result{}, fmt.Errorf("%w: target asserted empty but %d advertised ref name(s) were dropped as invalid", ErrTargetEmptyUnverified, n) } return Result{ @@ -161,8 +248,59 @@ func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { DryRun: s.cfg.DryRun, OperationMode: modeReplicate, Protocol: s.sourceService.Protocol, - SourceEmpty: true, - Stats: s.stats.snapshot(), - Measurement: s.measurementDone(), + Converged: true, + // Replicate refuses a non-relay target outright, so every successful + // replicate reports a relay; a consumer reading Relay=false with an + // empty RelayMode would file this under "materialized fallback" or + // "malformed". The reason names why nothing moved. SourceHEAD stays + // empty on purpose: an unborn HEAD has no existing target branch, and + // consumers read a non-empty SourceHEAD as one that exists. + Relay: true, + RelayMode: modeReplicate, + RelayReason: "source-empty-converged", + Stats: s.stats.snapshot(), + Measurement: s.measurementDone(), }, nil } + +// sourceCannotReportUnborn reports whether this connection is structurally +// incapable of carrying an unborn-HEAD signal, and why. +// +// HeadUnborn being false means only "not reported", which covers two very +// different situations. Over a connection that CAN report it, false is evidence +// against the caller's assertion — HEAD's target exists, so a ref is hidden. +// Over one that cannot, false is no evidence at all, and treating it as the +// former blames the server for the client's protocol. +func (s *syncSession) sourceCannotReportUnborn() (string, bool) { + if s.sourceService.Protocol == protocolModeV1 { + return "the source negotiated protocol v1, which has no unborn-HEAD signal", true + } + // Nil caps means a caller assembled this session without an advertisement + // (tests do); only an advertisement that is present and lacks the feature + // is evidence the server cannot report it. + if caps := s.sourceService.V2Caps; caps != nil && !caps.LSRefsSupports("unborn") { + return "the source does not advertise ls-refs=unborn", true + } + return "", false +} + +// targetRefsInScope counts the target refs this request would actually manage. +// +// Every other consumer of the target ref map filters the same two ways before +// acting (syncer.replicateCanBootstrap, planner.addPruneCandidates): a zero +// hash is a deletion sentinel rather than a ref, and an excluded name is one +// the run would neither push nor prune. A divergence check that skipped those +// filters would report refs the request has explicitly disclaimed. +func (s *syncSession) targetRefsInScope() int { + n := 0 + for name, hash := range s.target.refMap { + if hash.IsZero() { + continue + } + if planner.IsRefExcluded(name, s.cfg.ExcludeRefPrefixes, s.cfg.ExcludeRefs) { + continue + } + n++ + } + return n +} diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index 7027a4b5..7bef3615 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -1,13 +1,18 @@ package syncer import ( + "context" "errors" + "slices" "strings" "testing" + git "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/storage/memory" "entire.io/entire/git-sync/internal/gitproto" + "entire.io/entire/git-sync/internal/validation" ) // converged is the one input that may succeed: opted in, unscoped, the caller @@ -27,11 +32,11 @@ func emptySourceSession(cfg Config, sourceRefs, targetRefs map[plumbing.Referenc } } -// withTargetSkipped marks the target as having advertised a ref name that -// validation dropped, which leaves its refMap empty while it plainly holds a -// ref. -func withTargetSkipped(s *syncSession, names ...string) *syncSession { - s.target.skippedRefNames = names +// withTargetSkipped marks the target as having advertised ref names that +// validation dropped, which leaves its refMap empty while it plainly holds +// refs. +func withTargetSkipped(s *syncSession, count int) *syncSession { + s.target.skippedRefCount = count return s } @@ -51,8 +56,8 @@ func TestResolveEmptyDesiredSetConverged(t *testing.T) { if err != nil { t.Fatalf("expected success for an asserted-empty source and empty target, got %v", err) } - if !result.SourceEmpty { - t.Error("SourceEmpty = false; the caller cannot tell this from a no-op sync") + if !result.Converged { + t.Error("Converged = false; the caller cannot tell this from a no-op sync") } if len(result.Plans) != 0 || result.Pushed != 0 || result.Deleted != 0 { t.Errorf("expected nothing applied, got plans=%d pushed=%d deleted=%d", len(result.Plans), result.Pushed, result.Deleted) @@ -76,8 +81,8 @@ func TestResolveEmptyDesiredSetKeepsDryRun(t *testing.T) { if !result.DryRun { t.Error("DryRun = false on a dry-run session") } - if !result.SourceEmpty { - t.Error("SourceEmpty = false; a plan should still report what it found") + if !result.Converged { + t.Error("Converged = false; a plan should still report what it found") } } @@ -118,7 +123,7 @@ func TestResolveEmptyDesiredSetUnverified(t *testing.T) { // Ref-name validation ate the whole advertisement: the repository // plainly has refs, and by ref count alone it looks empty. "advertised names dropped as invalid": {converged(), &gitproto.RefService{ - Protocol: "v2", HeadUnborn: true, SkippedRefNames: []string{"refs/heads/bad name"}, + Protocol: "v2", HeadUnborn: true, SkippedRefCount: 1, }}, // No ref service at all (a listing that failed upstream of here). @@ -164,7 +169,7 @@ func TestResolveEmptyDesiredSetTargetUnverified(t *testing.T) { }) t.Run("advertised target names dropped as invalid", func(t *testing.T) { - s := withTargetSkipped(emptySourceSession(converged(), nil, nil, unbornSource()), "refs/heads/bad name") + s := withTargetSkipped(emptySourceSession(converged(), nil, nil, unbornSource()), 1) _, err := s.resolveEmptyDesiredSet() if !errors.Is(err, ErrTargetEmptyUnverified) { t.Fatalf("expected ErrTargetEmptyUnverified, got %v", err) @@ -200,7 +205,14 @@ func TestResolveEmptyDesiredSetSelectionEmpty(t *testing.T) { // in this family — a caller that never asked for the distinction cannot // receive a sentinel it does not know how to classify. s = emptySourceSession(Config{}, oneRef(), nil, unbornSource()) - if _, err := s.resolveEmptyDesiredSet(); err.Error() != "no source refs matched" { + // err is checked for nil first: calling Error() straight away panics in + // exactly the regression this assertion exists to catch (a nil error where + // the historical one is required), turning a clear failure into a crash. + _, err = s.resolveEmptyDesiredSet() + if err == nil { + t.Fatal("expected the historical error, got nil") + } + if err.Error() != "no source refs matched" { t.Errorf("un-opted-in error = %q, want the historical message", err) } } @@ -246,3 +258,393 @@ func TestEmptySourceSentinelsDoNotCarryHistoricalMessage(t *testing.T) { } } } + +// A target whose only refs are ones this request excludes is converged over +// everything it manages. Counting them made a mirror that trims refs/pull/* +// permanently unconvergeable over refs it would never push or prune — and +// refs/pull/* is the namespace ErrNoRefsSelected's own doc cites as the benign +// case. Every other consumer of the target ref map filters the same way. +func TestResolveEmptySourceIgnoresOutOfScopeTargetRefs(t *testing.T) { + hash := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + + cases := map[string]struct { + cfg Config + target map[plumbing.ReferenceName]plumbing.Hash + }{ + "excluded by prefix": { + func() Config { c := converged(); c.ExcludeRefPrefixes = []string{"refs/pull/"}; return c }(), + map[plumbing.ReferenceName]plumbing.Hash{"refs/pull/1/head": hash}, + }, + "excluded by exact name": { + func() Config { c := converged(); c.ExcludeRefs = []string{"refs/heads/entire"}; return c }(), + map[plumbing.ReferenceName]plumbing.Hash{"refs/heads/entire": hash}, + }, + // A zero hash is a deletion sentinel, not a ref that exists. + "zero hash": { + converged(), + map[plumbing.ReferenceName]plumbing.Hash{"refs/heads/main": plumbing.ZeroHash}, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + s := emptySourceSession(tc.cfg, nil, tc.target, unbornSource()) + result, err := s.resolveEmptySource() + if err != nil { + t.Fatalf("expected convergence over out-of-scope target refs, got %v", err) + } + if !result.Converged { + t.Error("Converged = false") + } + }) + } +} + +// An in-scope target ref is still divergence, so the scope filter must not have +// turned the check off altogether. +func TestResolveEmptySourceStillSeesInScopeTargetRefs(t *testing.T) { + cfg := converged() + cfg.ExcludeRefPrefixes = []string{"refs/pull/"} + target := oneRef() + target["refs/pull/1/head"] = plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") + + s := emptySourceSession(cfg, nil, target, unbornSource()) + _, err := s.resolveEmptySource() + if !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected ErrSourceEmptyTargetPopulated, got %v", err) + } + // The count must report only what is in scope, or an operator reading it + // goes looking for refs the run had disclaimed. + if !strings.Contains(err.Error(), "(1 in scope)") { + t.Errorf("error = %q, want the in-scope count", err) + } +} + +// This function is billed as the one place that may conclude convergence, and +// Fetch and a target-less Probe both build sessions with no target at all. A +// reuse from either must be refused, not panic on a nil dereference — and only +// on this branch, so no test that syncs a non-empty source would catch it. +func TestResolveEmptySourceWithoutTargetSession(t *testing.T) { + s := emptySourceSession(converged(), nil, nil, unbornSource()) + s.target = nil + _, err := s.resolveEmptySource() + if !errors.Is(err, ErrTargetEmptyUnverified) { + t.Fatalf("expected ErrTargetEmptyUnverified, got %v", err) + } +} + +// Replicate refuses a non-relay target outright, so every successful replicate +// reports a relay. A converged run that left these zero would be read as a +// materialized fallback or as a malformed result. +func TestResolveEmptySourceReportsRelayFields(t *testing.T) { + s := emptySourceSession(converged(), nil, nil, unbornSource()) + result, err := s.resolveEmptySource() + if err != nil { + t.Fatalf("expected success, got %v", err) + } + if !result.Relay || result.RelayMode != modeReplicate || result.RelayReason == "" { + t.Errorf("relay fields unset on a converged replicate: relay=%t mode=%q reason=%q", + result.Relay, result.RelayMode, result.RelayReason) + } + // SourceHEAD stays empty on purpose: an unborn HEAD has no target branch + // that exists, and consumers read a non-empty SourceHEAD as one that does. + if result.SourceHEAD != "" { + t.Errorf("SourceHEAD = %q, want empty for an unborn HEAD", result.SourceHEAD) + } +} + +// The text output is what cmd/git-sync and every non-JSON consumer render, so a +// converged run that prints the same summary as a no-op sync drops the one +// distinction the field exists to draw. +func TestConvergedResultIsVisibleInTextOutput(t *testing.T) { + s := emptySourceSession(converged(), nil, nil, unbornSource()) + result, err := s.resolveEmptySource() + if err != nil { + t.Fatalf("expected success, got %v", err) + } + converged := strings.Join(result.Lines(), "\n") + if !strings.Contains(converged, "converged:") { + t.Errorf("converged replicate renders no converged line:\n%s", converged) + } + if plain := strings.Join(Result{OperationMode: modeReplicate}.Lines(), "\n"); strings.Contains(plain, "converged:") { + t.Errorf("an ordinary no-op run claims convergence:\n%s", plain) + } +} + +// The wiring the whole policy rests on, exercised end to end: the ls-refs +// "unborn" argument goes on the wire, the response's unborn line becomes +// RefService.HeadUnborn, and Run turns that into a converged zero-plan result. +// +// Every other test in this file hand-builds a syncSession, so none of that +// chain was covered: deleting the "unborn" argument from listSourceRefsV2 left +// the entire suite green while every real empty-source run degraded to +// ErrSourceEmptyUnverified forever. +func TestRun_EmptySourceConvergesEndToEnd(t *testing.T) { + sourceRepo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init source repo: %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) + defer sourceServer.Close() + defer targetServer.Close() + + cfg := Config{ + Source: Endpoint{URL: sourceServer.RepoURL()}, + Target: Endpoint{URL: targetServer.RepoURL()}, + ProtocolMode: protocolModeV2, + Mode: modeReplicate, + AllRefs: true, + AllowEmptySource: true, + SourceAssertedEmpty: true, + TargetAssertedEmpty: true, + } + + result, err := Run(context.Background(), cfg) + if err != nil { + t.Fatalf("expected two empty repos to converge, got %v", err) + } + if !result.Converged { + t.Error("Converged = false on two verified-empty repositories") + } + if len(result.Plans) != 0 || result.Pushed != 0 || result.Deleted != 0 { + t.Errorf("expected nothing applied, got plans=%d pushed=%d deleted=%d", + len(result.Plans), result.Pushed, result.Deleted) + } + + // Without the opt-in the same pair of repositories must still fail, and + // with the historical message: the policy is what changes the outcome, not + // the wire. + optedOut := cfg + optedOut.AllowEmptySource = false + optedOut.SourceAssertedEmpty = false + optedOut.TargetAssertedEmpty = false + if _, err := Run(context.Background(), optedOut); err == nil { + t.Error("an empty source without the opt-in must still be an error") + } else if !strings.Contains(err.Error(), "no source refs matched") { + t.Errorf("un-opted-in error = %q, want the historical message", err) + } +} + +// A caller that pins refs by mapping is the headline use case for a mirror, and +// the policy was entirely inert for it: BuildDesiredRefs errors on the absent +// mapped source ref before the empty-set branch can run, so the caller got a +// hard failure matching none of the sentinels on exactly the state the policy +// exists to make succeed. +func TestRun_EmptySourceConvergesWithMappings(t *testing.T) { + sourceRepo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init source repo: %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) + defer sourceServer.Close() + defer targetServer.Close() + + result, err := Run(context.Background(), Config{ + Source: Endpoint{URL: sourceServer.RepoURL()}, + Target: Endpoint{URL: targetServer.RepoURL()}, + ProtocolMode: protocolModeV2, + Mode: modeReplicate, + AllRefs: true, + Mappings: []validation.RefMapping{{Source: "refs/heads/main", Target: "refs/heads/main"}}, + AllowEmptySource: true, + SourceAssertedEmpty: true, + TargetAssertedEmpty: true, + }) + if err != nil { + t.Fatalf("expected a mapping-pinned mirror of two empty repos to converge, got %v", err) + } + if !result.Converged { + t.Error("Converged = false on two verified-empty repositories with a mapping") + } +} + +// A policy no path would consult must fail at the edge rather than be threaded +// in and discarded. Both conditions previously produced the historical +// "no source refs matched", which tells the caller nothing about their policy +// having been ignored. +func TestValidateEmptySourcePolicy(t *testing.T) { + cases := map[string]struct { + cfg Config + wantErr bool + }{ + "replicate, unscoped": {Config{AllowEmptySource: true, AllRefs: true, Mode: modeReplicate}, false}, + "sync mode": {Config{AllowEmptySource: true, AllRefs: true, Mode: modeSync}, true}, + "scoped replicate": {Config{AllowEmptySource: true, Mode: modeReplicate}, true}, + // The assertions are inputs to the policy, inert without it by design. + "assertions without the opt-in": {Config{SourceAssertedEmpty: true, TargetAssertedEmpty: true, Mode: modeSync}, false}, + "policy unset": {Config{Mode: modeSync}, false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := validateEmptySourcePolicy(tc.cfg) + if (err != nil) != tc.wantErr { + t.Errorf("validateEmptySourcePolicy() err = %v, wantErr = %t", err, tc.wantErr) + } + }) + } +} + +// Reached through a real entry point, not just the predicate: a scoped request +// must be refused before any I/O rather than reaching the planner. +func TestRun_EmptySourcePolicyRejectedAtTheEdge(t *testing.T) { + cfg := Config{ + Source: Endpoint{URL: "https://source.invalid/repo.git"}, + Target: Endpoint{URL: "https://target.invalid/repo.git"}, + Mode: modeReplicate, + AllowEmptySource: true, + SourceAssertedEmpty: true, + TargetAssertedEmpty: true, + } + if _, err := Run(context.Background(), cfg); err == nil { + t.Fatal("expected a scoped AllowEmptySource request to be rejected") + } else if !strings.Contains(err.Error(), "AllRefs") { + t.Errorf("error = %q, want it to name the AllRefs requirement", err) + } +} + +// Convergence needs an unborn HEAD, which only v2's ls-refs can carry. Pinning +// v1 therefore fails every run — and with a message that reads as the source +// withholding refs, when the real cause is the caller's protocol selection. It +// is refused before any I/O instead. +func TestValidateEmptySourceRejectsProtocolV1(t *testing.T) { + base := Config{AllowEmptySource: true, AllRefs: true, Mode: modeReplicate} + + v1 := base + v1.ProtocolMode = protocolModeV1 + if err := validateEmptySourcePolicy(v1); err == nil { + t.Error("expected AllowEmptySource + protocol v1 to be rejected") + } else if !strings.Contains(err.Error(), "v2") { + t.Errorf("error = %q, want it to name the v2 requirement", err) + } + + // auto is fine: it negotiates v2 wherever the server supports it, and the + // SSH fallback to v1 is only observable mid-run. + for _, mode := range []string{protocolModeAuto, protocolModeV2, ""} { + cfg := base + cfg.ProtocolMode = mode + if err := validateEmptySourcePolicy(cfg); err != nil { + t.Errorf("protocol %q rejected: %v", mode, err) + } + } +} + +// What validation cannot catch: an "auto" source whose v2 probe failed and fell +// back to v1 mid-run. The error must name the protocol rather than implying the +// server hid refs, because the two call for opposite responses — one is a +// client configuration note, the other a reason to suspect the source. +func TestResolveEmptySourceNamesTheProtocolNotTheServer(t *testing.T) { + cases := map[string]struct { + svc *gitproto.RefService + want string + }{ + "fell back to v1": { + &gitproto.RefService{Protocol: protocolModeV1}, + "protocol v1", + }, + "v2 without the capability": { + &gitproto.RefService{Protocol: protocolModeV2, V2Caps: &gitproto.V2Capabilities{}}, + "ls-refs=unborn", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + s := emptySourceSession(converged(), nil, nil, tc.svc) + _, err := s.resolveEmptySource() + if !errors.Is(err, ErrSourceEmptyUnverified) { + t.Fatalf("expected ErrSourceEmptyUnverified, got %v", err) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to mention %q", err, tc.want) + } + // The message that blames the server must not be the one shown. + if strings.Contains(err.Error(), "did not report an unborn HEAD") { + t.Errorf("a protocol limitation was reported as a withheld ref: %q", err) + } + }) + } +} + +// A source that CAN report unborn and does not is the case where false really is +// evidence against the caller's assertion, so that message must survive. +func TestResolveEmptySourceStillBlamesAWithheldRef(t *testing.T) { + // No V2Caps: nothing establishes that the server lacks the feature, so + // false is read as evidence rather than as a protocol limitation. + s := emptySourceSession(converged(), nil, nil, &gitproto.RefService{Protocol: protocolModeV2}) + _, err := s.resolveEmptySource() + if !errors.Is(err, ErrSourceEmptyUnverified) { + t.Fatalf("expected ErrSourceEmptyUnverified, got %v", err) + } + if !strings.Contains(err.Error(), "did not report an unborn HEAD") { + t.Errorf("error = %q, want the withheld-ref message", err) + } +} + +// The "unborn" ls-refs argument is deliberately NOT gated on +// SyncPolicy.AllowEmptySource: it is appended to every v2 listing whose server +// advertised support for it, opt-in or not. +// +// That is a decision, not an oversight, so it is pinned here. Gating it would +// mean threading the policy through gitproto.ListSourceRefs for no benefit — +// the argument adds no round trip, a source with commits answers exactly as it +// did before, and the value is that Probe and Plan can report an unborn HEAD +// without the caller having opted into a convergence policy first. What IS +// gated is the reader: only resolveEmptySource acts on HeadUnborn. +// +// The gate that does matter is the advertisement: protocol v2 forbids sending +// an argument the server did not advertise, and a strict server may fail the +// command. +func TestLSRefsUnbornIsSentRegardlessOfPolicy(t *testing.T) { + repo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init repo: %v", err) + } + server := newSmartHTTPRepoServerV2(t, repo) + defer server.Close() + + // A probe: no policy at all, not even a target. + if _, err := Probe(context.Background(), Config{ + Source: Endpoint{URL: server.RepoURL()}, + ProtocolMode: protocolModeV2, + }); err != nil { + t.Fatalf("probe: %v", err) + } + args := server.LastLSRefsArgs() + if !slices.Contains(args, "unborn") { + t.Errorf("ls-refs args = %v, want the unborn argument on an un-opted-in probe", args) + } +} + +// The one gate that is load-bearing: protocol v2 forbids sending an argument +// the server did not advertise, and a strict server may fail the command +// outright. A source advertising a bare "ls-refs" must therefore see no unborn +// argument — and must still list refs normally. +func TestLSRefsUnbornWithheldWhenUnadvertised(t *testing.T) { + repo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init repo: %v", err) + } + server := newSmartHTTPRepoServerV2(t, repo) + server.lsRefsNoUnborn = true + defer server.Close() + + if _, err := Probe(context.Background(), Config{ + Source: Endpoint{URL: server.RepoURL()}, + ProtocolMode: protocolModeV2, + }); err != nil { + t.Fatalf("probe: %v", err) + } + if args := server.LastLSRefsArgs(); slices.Contains(args, "unborn") { + t.Errorf("ls-refs args = %v; sent an argument the server did not advertise", args) + } +} diff --git a/internal/syncer/integration_test.go b/internal/syncer/integration_test.go index 524ed779..afa6b9dd 100644 --- a/internal/syncer/integration_test.go +++ b/internal/syncer/integration_test.go @@ -3644,6 +3644,20 @@ type smartHTTPRepoServer struct { mu sync.Mutex metrics []exchangeMetric + // lsRefsArgs is the argument list of the most recent ls-refs request, so a + // test can assert on what actually went on the wire. + lsRefsArgs []string + + // lsRefsNoUnborn drops "unborn" from the advertised ls-refs features, for + // testing that the client does not send an unadvertised argument. + lsRefsNoUnborn bool +} + +// LastLSRefsArgs returns the arguments of the most recent ls-refs request. +func (s *smartHTTPRepoServer) LastLSRefsArgs() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.lsRefsArgs...) } func newSmartHTTPRepoServer(tb testing.TB, repo *git.Repository) *smartHTTPRepoServer { @@ -3844,9 +3858,13 @@ func rewriteReceivePackAdvertisement(data []byte, mutate func(*capability.List)) func (s *smartHTTPRepoServer) handleInfoRefsV2(w http.ResponseWriter, _ *http.Request) { var buf bytes.Buffer + lsRefs := "ls-refs=unborn\n" + if s.lsRefsNoUnborn { + lsRefs = "ls-refs\n" + } lines := []string{ "version 2\n", - "ls-refs=unborn\n", + lsRefs, "fetch=thin-pack filter\n", "agent=test-server\n", } @@ -3923,13 +3941,21 @@ func (s *smartHTTPRepoServer) handleUploadPackV2(w http.ResponseWriter, _ *http. } func (s *smartHTTPRepoServer) handleUploadPackV2LSRefs(w http.ResponseWriter, req v2TestCommandRequest, body []byte) { + s.mu.Lock() + s.lsRefsArgs = append([]string(nil), req.Args...) + s.mu.Unlock() + prefixes := make([]string, 0, len(req.Args)) wantSymrefs := false + wantUnborn := false for _, arg := range req.Args { - if strings.HasPrefix(arg, "ref-prefix ") { + switch { + case strings.HasPrefix(arg, "ref-prefix "): prefixes = append(prefixes, strings.TrimPrefix(arg, "ref-prefix ")) - } else if arg == "symrefs" { + case arg == "symrefs": wantSymrefs = true + case arg == "unborn": + wantUnborn = true } } @@ -3943,7 +3969,7 @@ func (s *smartHTTPRepoServer) handleUploadPackV2LSRefs(w http.ResponseWriter, re // Real git emits HEAD with symref-target attribute under "symrefs", as // long as a ref-prefix covers HEAD (or no prefixes are given). if wantSymrefs && coversHead(prefixes) { - if line, ok := s.lsRefsHeadLine(); ok { + if line, ok := s.lsRefsHeadLine(wantUnborn); ok { if _, err := pktline.WriteString(&buf, line); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -4211,13 +4237,23 @@ func coversHead(prefixes []string) bool { // lsRefsHeadLine formats a v2 ls-refs HEAD line with the symref-target // attribute, matching what real git advertises under "symrefs". -func (s *smartHTTPRepoServer) lsRefsHeadLine() (string, bool) { +// +// When HEAD's target does not exist, real git emits an "unborn HEAD" line if +// the client asked for the unborn argument and says nothing at all if it did +// not — which is precisely the ambiguity that argument exists to remove. The +// fake reproduced only the silent half, which is why nothing here exercised +// RefService.HeadUnborn end to end: deleting the request argument left the +// whole suite green. +func (s *smartHTTPRepoServer) lsRefsHeadLine(wantUnborn bool) (string, bool) { head, err := s.repo.Storer.Reference(plumbing.HEAD) if err != nil || head.Type() != plumbing.SymbolicReference { return "", false } resolved, err := s.repo.Reference(head.Target(), true) if err != nil { + if wantUnborn { + return fmt.Sprintf("unborn %s symref-target:%s\n", plumbing.HEAD, head.Target()), true + } return "", false } return fmt.Sprintf("%s HEAD symref-target:%s\n", resolved.Hash(), head.Target()), true diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index cd817675..284aad65 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -182,11 +182,22 @@ type Result struct { Stats Stats `json:"stats"` Measurement Measurement `json:"measurement"` Protocol string `json:"protocol"` - // SourceEmpty is true when the source was verified to have no refs and - // the target had none either, so the two are converged with nothing to - // apply. Only ever set on a successful zero-plan replicate; see - // resolveEmptyDesiredSet. - SourceEmpty bool `json:"sourceEmpty,omitempty"` + // Converged is true when the source was verified to have no refs and the + // target had none either, so the two already agree with nothing to apply. + // Only ever set on a successful zero-plan replicate; see + // resolveEmptySource. + // + // Named for what it reports rather than for the source alone: it requires + // BOTH sides verified, so it is false in every other empty-source outcome + // — including the diverged one, where the source WAS verified empty. A + // "SourceEmpty" that is false on a verified-empty source is a field that + // invites the wrong read. + // + // Emitted unconditionally (no omitempty) like every other discriminating + // bool here: this is the field that separates a converged run from an + // ordinary no-op, so "false" and "this binary has no such field" must not + // be the same JSON. + Converged bool `json:"converged"` } func (r Result) Lines() []string { @@ -213,6 +224,13 @@ func (r Result) Lines() []string { if r.SourceHEAD != "" { lines = append(lines, "source-head: "+r.SourceHEAD.String()) } + // Without this the text output of a converged empty replicate is + // byte-identical to an ordinary sync that had no work to do — the one + // distinction this field exists to draw, dropped on the path that + // cmd/git-sync and every non-JSON consumer actually render. + if r.Converged { + lines = append(lines, "converged: source and target both verified empty; nothing to apply") + } return lines } @@ -715,11 +733,14 @@ type targetSession struct { features gitproto.TargetFeatures policy planner.RelayTargetPolicy pusher *gitproto.Pusher - // skippedRefNames are advertised target ref names dropped as invalid. - // Retained, not just warned about, because their absence is load-bearing - // for any caller reasoning about an EMPTY target: names dropped here - // leave refMap empty while the target plainly holds refs. - skippedRefNames []string + // skippedRefCount is how many advertised target ref names were dropped as + // invalid. Retained, not just warned about, because a non-zero count is + // load-bearing for any caller reasoning about an EMPTY target: names + // dropped here leave refMap empty while the target plainly holds refs. + // A count rather than the names, for the reason on + // gitproto.RefService.SkippedRefCount — this struct outlives the pack + // transfer and only a boolean is ever read from it. + skippedRefCount int } // newSession performs the shared setup: protocol validation, mapping validation, @@ -737,6 +758,9 @@ func newSession(ctx context.Context, cfg Config, needTarget bool) (*syncSession, default: return nil, fmt.Errorf("unsupported operation mode %q", cfg.Mode) } + if err := validateEmptySourcePolicy(cfg); err != nil { + return nil, err + } if _, err := validation.ValidateMappings(cfg.Mappings, cfg.AllRefs); err != nil { return nil, fmt.Errorf("validate mappings: %w", err) } @@ -827,7 +851,7 @@ func newSession(ctx context.Context, cfg Config, needTarget bool) (*syncSession, // never picked as a prune candidate — the safe direction, but the // operator should know the target holds a name git would reject. gitproto.WarnSkippedRefNames(targetConn.ProgressWriter(), "target", skippedTargetRefs) - s.target.skippedRefNames = skippedTargetRefs + s.target.skippedRefCount = len(skippedTargetRefs) targetRefMap := gitproto.RefHashMap(targetRefSlice) targetFeatures := gitproto.TargetFeaturesFromAdvRefs(targetAdv) s.target.adv = targetAdv @@ -1013,6 +1037,20 @@ func (s *syncSession) runSync(ctx context.Context) (Result, error) { } func (s *syncSession) runReplicate(ctx context.Context) (Result, error) { + // An empty source advertisement is resolved BEFORE planning. Under AllRefs + // the advertisement covers refs/, so "nothing advertised" is already a + // complete observation and planning cannot add to it — while + // BuildDesiredRefs errors on a mapping whose source ref is absent, which + // on a genuinely empty source is every mapping. Deciding after planning + // left the whole policy inert for any request that pinned refs by mapping: + // the caller got "source ref X not found" matching no sentinel, on exactly + // the state the policy exists to make succeed. + // + // Gated on the opt-in so a caller that never asked for this still gets the + // planner's error verbatim. + if s.cfg.AllowEmptySource && s.cfg.AllRefs && len(s.sourceRefMap) == 0 { + return s.resolveEmptySource() + } desiredRefs, managedTargets, err := planner.BuildDesiredRefs(s.sourceRefMap, planConfig(s.cfg)) if err != nil { return Result{}, fmt.Errorf("build desired refs: %w", err) diff --git a/results.go b/results.go index 8cb97817..6dd23e97 100644 --- a/results.go +++ b/results.go @@ -129,12 +129,18 @@ type ExecutionSummary struct { BootstrapSuggested bool `json:"bootstrapSuggested"` SourceHEAD string `json:"sourceHead,omitempty"` Batch BatchSummary `json:"batch"` - // SourceEmpty reports that this run applied nothing because the source - // was confirmed to have no refs and the target had none either — the two - // are converged. Only ever set under SyncPolicy.AllowEmptySource, and it + // Converged reports that this run applied nothing because the source was + // confirmed to have no refs and the target had none either — the two + // already agree. Only ever set under SyncPolicy.AllowEmptySource, and it // is what distinguishes that converged state from an ordinary sync that // happened to have no work to do. - SourceEmpty bool `json:"sourceEmpty,omitempty"` + // + // It requires BOTH sides verified, so it is false in every other + // empty-source outcome, including ErrSourceEmptyTargetPopulated where the + // source was verified empty but the two have diverged. Emitted + // unconditionally so "false" is distinguishable from a caller running a + // build that predates the field. + Converged bool `json:"converged"` } // SyncResult is the outcome of a Sync or Replicate. @@ -188,7 +194,7 @@ func fromSyncResult(result syncer.Result) SyncResult { Reason: result.RelayReason, BootstrapSuggested: result.BootstrapSuggested, SourceHEAD: result.SourceHEAD.String(), - SourceEmpty: result.SourceEmpty, + Converged: result.Converged, Batch: BatchSummary{ Enabled: result.Batching, Planned: result.PlannedBatchCount, diff --git a/types.go b/types.go index e9ec73bf..c6ca7060 100644 --- a/types.go +++ b/types.go @@ -8,6 +8,13 @@ import ( // ProtocolMode controls source-side protocol negotiation. type ProtocolMode string +// ProtocolAuto negotiates v2 and falls back to v1; ProtocolV1 and ProtocolV2 +// pin the choice. +// +// ProtocolV1 is incompatible with SyncPolicy.AllowEmptySource: the unborn-HEAD +// cross-check that policy requires exists only in v2's ls-refs, so pinning v1 +// is rejected at the request edge rather than failing every run with an error +// that looks like the source is withholding refs. const ( ProtocolAuto ProtocolMode = "auto" ProtocolV1 ProtocolMode = "v1" @@ -142,13 +149,26 @@ type SyncPolicy struct { TargetAssertedEmpty bool `json:"targetAssertedEmpty,omitempty"` // AllowEmptySource opts into treating a verified-empty source as an - // outcome instead of an error. Replicate only. When source and target are - // both verified empty (see the asserted-empty fields above) Replicate - // succeeds with zero plans and ExecutionSummary.SourceEmpty set; when the - // target holds refs the two have diverged and it fails with - // ErrSourceEmptyTargetPopulated rather than deleting them; when either - // side's emptiness cannot be established it fails with the matching - // unverified error. + // outcome instead of an error. When source and target are both verified + // empty (see the asserted-empty fields above) Replicate succeeds with zero + // plans and ExecutionSummary.Converged set; when the target holds refs the + // two have diverged and it fails with ErrSourceEmptyTargetPopulated rather + // than deleting them; when either side's emptiness cannot be established it + // fails with the matching unverified error. + // + // Two requirements, both rejected at the request edge rather than silently + // ignored: + // + // - Mode must be ModeReplicate. Sync, Bootstrap and Fetch have no + // convergence outcome to report. + // - RefScope.AllRefs must be set. Under a narrower scope the source ref + // listing is itself narrowed, so an empty result says nothing about the + // repository as a whole, and the target's refs — which are never + // scope-filtered — cannot be judged against a partial view of the + // source. + // + // Convergence additionally requires protocol v2 on the source leg, because + // the unborn-HEAD cross-check has no v1 equivalent. See ProtocolV1. // // Off by default: leave it unset and an empty source errors exactly as it // always has. @@ -163,6 +183,22 @@ func (p SyncPolicy) Validate() error { if p.Mode == ModeReplicate && (p.ForceWithLease || p.ForceBlind) { return errors.New("replicate does not support force flags; use sync instead") } + // Sync, Bootstrap and Fetch have no convergence outcome, so they would + // accept the policy, carry it all the way into the syncer, and then fail + // with the historical "no source refs matched" — leaving the caller no way + // to learn their safety policy was a no-op. The AllRefs half of the same + // requirement needs the scope and is enforced where both are in view. + if p.AllowEmptySource && p.Mode != ModeReplicate { + return errors.New("AllowEmptySource applies to replicate only; set Mode to ModeReplicate or use Replicate") + } + // v1 has no unborn-HEAD signal, so the cross-check this policy requires can + // never be satisfied over it: every run would fail, and the failure would + // read as the source withholding refs rather than as the caller's own + // protocol choice. ProtocolAuto is fine — it negotiates v2 wherever the + // server supports it. + if p.AllowEmptySource && p.Protocol == ProtocolV1 { + return errors.New("AllowEmptySource requires protocol v2 on the source; v1 cannot report an unborn HEAD, so emptiness can never be corroborated") + } return nil } From 11d8679e56fc3f511d8640fd3d987995c7d9539b Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 23:34:27 +0200 Subject: [PATCH 07/10] Ask the planner what the target ref map means, instead of re-deriving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot caught that the divergence check's idea of scope was exclusions-only, while the planner's is wider: with Mappings set, addPruneCandidates declines to manage unmapped branches and other namespaces too. A mapping-pinned mirror whose target held any unmapped branch was therefore reported as permanently diverged over a ref the run would neither push nor prune — the same false divergence the exclusion filter fixed, defeating the very case the pre-planning path exists to serve. The cause is that "does this request manage this target ref" had been written out three times, so a fourth copy would repeat the mistake. It is now planner.PruneTarget, which addPruneCandidates and the divergence check share. replicateCanBootstrap deliberately keeps its own broader branch rule (under AllRefs a stale branch matters even with a Branches filter set), which is identical to this one wherever AllowEmptySource applies, since that policy requires AllRefs. PruneTarget normalizes its config rather than assuming a normalized one: syncer.planConfig does not normalize, and reading the raw config is silently wrong in the dangerous direction — an AllRefs request still carrying a Branches filter reports a branch as unmanaged when the request would in fact prune it, so the run converges over a populated target instead of refusing. Both behaviors are pinned by tests that fail if the fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JvpGRBapBppY4xh2x38kDL Entire-Checkpoint: 01M0K3Y9622187AV0J1TCK78X1 --- internal/planner/planner.go | 49 +++++++++++++++++----- internal/syncer/empty_source.go | 17 +++++--- internal/syncer/empty_source_test.go | 61 ++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 16 deletions(-) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index efa7ce44..1281e62d 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -250,6 +250,43 @@ func BuildReplicationPlans( return plans, nil } +// PruneTarget reports whether a request would manage an unmanaged target ref — +// equivalently, whether prune could select it for deletion — and with what +// classification. +// +// It normalizes cfg itself rather than assuming a normalized one. Callers +// inside planning have already normalized (this is then a no-op), but the +// planner's own entry points normalize on the way in, so an outside caller +// holding a raw config has no obvious cue that it must. Getting it wrong is +// silent and one-directional: an un-normalized AllRefs config still carrying a +// Branches filter reports a branch as unmanaged when the request would in fact +// prune it. +// +// This is the single answer to "is this target ref ours to act on", and callers +// outside planning need it too: anything reasoning about what the target holds +// must ask the same question the planner will, or it reports refs the request +// has disclaimed. Exclusions are part of the answer, not a separate pre-filter, +// for the same reason. +// +// Note what mappings do here: a mapping-scoped request manages the refs it +// mapped and, for prune purposes, tags — but not unmapped branches and not +// other namespaces, which it neither pushes nor prunes. +func PruneTarget(targetRef plumbing.ReferenceName, cfg PlanConfig) (ManagedTarget, bool) { + cfg = normalizeAllRefs(cfg) + if IsRefExcluded(targetRef, cfg.ExcludeRefPrefixes, cfg.ExcludeRefs) { + return ManagedTarget{}, false + } + switch { + case targetRef.IsTag() && (cfg.IncludeTags || cfg.AllRefs): + return ManagedTarget{Kind: RefKindTag, Label: targetRef.Short()}, true + case targetRef.IsBranch() && len(cfg.Mappings) == 0 && len(cfg.Branches) == 0: + return ManagedTarget{Kind: RefKindBranch, Label: targetRef.Short()}, true + case cfg.AllRefs && RefKindFromName(targetRef) == RefKindOther && len(cfg.Mappings) == 0: + return ManagedTarget{Kind: RefKindOther, Label: targetRef.Short()}, true + } + return ManagedTarget{}, false +} + // addPruneCandidates registers unmanaged target refs as deletion candidates // within the user's current scope. cfg is assumed normalized. func addPruneCandidates(managed map[plumbing.ReferenceName]ManagedTarget, targetRefs map[plumbing.ReferenceName]plumbing.Hash, cfg PlanConfig) { @@ -257,16 +294,8 @@ func addPruneCandidates(managed map[plumbing.ReferenceName]ManagedTarget, target if _, ok := managed[targetRef]; ok { continue } - if IsRefExcluded(targetRef, cfg.ExcludeRefPrefixes, cfg.ExcludeRefs) { - continue - } - switch { - case targetRef.IsTag() && (cfg.IncludeTags || cfg.AllRefs): - managed[targetRef] = ManagedTarget{Kind: RefKindTag, Label: targetRef.Short()} - case targetRef.IsBranch() && len(cfg.Mappings) == 0 && len(cfg.Branches) == 0: - managed[targetRef] = ManagedTarget{Kind: RefKindBranch, Label: targetRef.Short()} - case cfg.AllRefs && RefKindFromName(targetRef) == RefKindOther && len(cfg.Mappings) == 0: - managed[targetRef] = ManagedTarget{Kind: RefKindOther, Label: targetRef.Short()} + if target, prunable := PruneTarget(targetRef, cfg); prunable { + managed[targetRef] = target } } } diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go index 61e92546..b2d4b858 100644 --- a/internal/syncer/empty_source.go +++ b/internal/syncer/empty_source.go @@ -286,18 +286,23 @@ func (s *syncSession) sourceCannotReportUnborn() (string, bool) { // targetRefsInScope counts the target refs this request would actually manage. // -// Every other consumer of the target ref map filters the same two ways before -// acting (syncer.replicateCanBootstrap, planner.addPruneCandidates): a zero -// hash is a deletion sentinel rather than a ref, and an excluded name is one -// the run would neither push nor prune. A divergence check that skipped those -// filters would report refs the request has explicitly disclaimed. +// It must ask exactly the question the planner asks, so it delegates to +// planner.PruneTarget rather than re-deriving the answer — a divergence check +// with its own idea of scope reports refs the request has disclaimed, and +// "excluded names only" was such an idea: with Mappings set, an unmapped branch +// is equally untouchable, and counting it left a mapping-pinned mirror +// permanently diverged over a ref it would neither push nor prune. +// +// The zero-hash skip stays here because it is about the value rather than the +// name: a zero hash is a deletion sentinel, not a ref that exists. func (s *syncSession) targetRefsInScope() int { + cfg := planConfig(s.cfg) n := 0 for name, hash := range s.target.refMap { if hash.IsZero() { continue } - if planner.IsRefExcluded(name, s.cfg.ExcludeRefPrefixes, s.cfg.ExcludeRefs) { + if _, managed := planner.PruneTarget(name, cfg); !managed { continue } n++ diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index 7bef3615..b556d298 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -648,3 +648,64 @@ func TestLSRefsUnbornWithheldWhenUnadvertised(t *testing.T) { t.Errorf("ls-refs args = %v; sent an argument the server did not advertise", args) } } + +// Scope is not only exclusions. A mapping-scoped request manages the refs it +// mapped and, for prune, tags — but not unmapped branches and not other +// namespaces. Counting those left a mapping-pinned mirror permanently diverged +// over a ref it would neither push nor prune, which is the same false +// divergence exclusions produced and defeats the very case the pre-planning +// path was added to serve. +func TestResolveEmptySourceRespectsMappingScope(t *testing.T) { + hash := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + mapped := func() Config { + c := converged() + c.Mappings = []validation.RefMapping{{Source: "refs/heads/main", Target: "refs/heads/main"}} + return c + } + + t.Run("unmapped branch is not this request's business", func(t *testing.T) { + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/heads/other": hash} + s := emptySourceSession(mapped(), nil, target, unbornSource()) + result, err := s.resolveEmptySource() + if err != nil { + t.Fatalf("expected convergence over an unmapped target branch, got %v", err) + } + if !result.Converged { + t.Error("Converged = false") + } + }) + + t.Run("other namespaces likewise", func(t *testing.T) { + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/notes/commits": hash} + s := emptySourceSession(mapped(), nil, target, unbornSource()) + if _, err := s.resolveEmptySource(); err != nil { + t.Fatalf("expected convergence over an unmapped namespace, got %v", err) + } + }) + + // Tags stay in scope under a mapping, because prune still selects them — + // the check must track what the planner does, not a simpler story. + t.Run("tags remain divergence", func(t *testing.T) { + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/tags/v1": hash} + s := emptySourceSession(mapped(), nil, target, unbornSource()) + if _, err := s.resolveEmptySource(); !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected ErrSourceEmptyTargetPopulated for a target tag, got %v", err) + } + }) +} + +// planConfig does not normalize, and un-normalized AllRefs configs still carry +// the caller's Branches filter. Reading that raw would report a branch as +// unmanaged when the request would in fact prune it — an undercount, so it +// converges over a populated target rather than refusing. +func TestResolveEmptySourceNormalizesScopeBeforeJudging(t *testing.T) { + cfg := converged() + cfg.Branches = []string{"main"} // AllRefs is set, so this is cleared by normalization + target := map[plumbing.ReferenceName]plumbing.Hash{ + "refs/heads/other": plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + } + s := emptySourceSession(cfg, nil, target, unbornSource()) + if _, err := s.resolveEmptySource(); !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("a populated target must refuse regardless of a stale Branches filter, got %v", err) + } +} From be483b3a180f19b40b7a1f6378d2465dfb52cfeb Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 23:40:39 +0200 Subject: [PATCH 08/10] Count mapping targets as in scope, not just prune candidates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit read planner.PruneTarget as "refs this request manages". It is not: it answers whether prune could select an ALREADY-UNMANAGED ref, and addPruneCandidates only consults it after skipping the managed set. With Mappings set it therefore reports false for every branch — including the mapping targets themselves. So the divergence check stopped seeing the one ref a mapping-pinned request most obviously owns. An empty source whose target still held the mapped ref counted zero refs in scope and converged, or reported ErrTargetEmptyUnverified instead of divergence. Converging there means deleting that ref, which is the outcome this whole path exists to refuse, and it is the same mapping case the two preceding commits were meant to fix. Scope is now planner.TargetScope: a target ref is the request's responsibility if it is a declared mapping target, or if prune could select it. Mapping targets are resolved through validation.ValidateMappings, the same call BuildDesiredRefs uses, so the two cannot disagree about what a mapping names, and short-form mappings match the full ref the target advertises. They are also in scope regardless of exclusions, matching the mapping pass in BuildDesiredRefs, which applies exclusions only to auto-discovery. PruneTarget's doc now says what it does not answer, since reading it as whole-scope is what went wrong. The gap was in the tests as much as the code: the mapping cases covered only refs the request does not own, so nothing exercised a target holding the mapped ref. That case, the excluded-but-mapped case, and short-form resolution are all covered now, and all three fail against the previous commit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JvpGRBapBppY4xh2x38kDL Entire-Checkpoint: 01M0K49M97M6VEWX1VPC0ADWX4 --- internal/planner/planner.go | 55 ++++++++++++++++++++++++++-- internal/syncer/empty_source.go | 34 +++++++++++------ internal/syncer/empty_source_test.go | 37 +++++++++++++++++++ 3 files changed, 111 insertions(+), 15 deletions(-) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index 1281e62d..05e8bc83 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -250,9 +250,58 @@ func BuildReplicationPlans( return plans, nil } -// PruneTarget reports whether a request would manage an unmanaged target ref — -// equivalently, whether prune could select it for deletion — and with what -// classification. +// TargetScope answers whether a target ref is a request's responsibility at +// all. Build it once per request with NewTargetScope; Manages is then O(1). +// +// Two ways a ref qualifies, and both are needed. It may be a ref the request +// explicitly MANAGES — a mapping target, which BuildDesiredRefs adds to the +// managed set — or one prune could select. PruneTarget alone answers only the +// second, and deliberately reports false for every branch once Mappings is set, +// because addPruneCandidates consults it only for refs already known to be +// unmanaged. Reading it as the whole answer silently drops the mapping targets, +// which is the worst direction for anything deciding divergence: a target still +// holding the mapped ref looks like nothing to worry about. +type TargetScope struct { + cfg PlanConfig + mapped map[plumbing.ReferenceName]struct{} +} + +// NewTargetScope precomputes the mapping targets a request declares. It +// normalizes cfg and resolves mapping names the same way BuildDesiredRefs +// does, so the two cannot disagree about what a mapping names. +func NewTargetScope(cfg PlanConfig) (TargetScope, error) { + cfg = normalizeAllRefs(cfg) + scope := TargetScope{cfg: cfg, mapped: map[plumbing.ReferenceName]struct{}{}} + if len(cfg.Mappings) == 0 { + return scope, nil + } + normalized, err := validation.ValidateMappings(cfg.Mappings, cfg.AllRefs) + if err != nil { + return TargetScope{}, fmt.Errorf("validate ref mappings: %w", err) + } + for _, nm := range normalized { + scope.mapped[nm.TargetRef] = struct{}{} + } + return scope, nil +} + +// Manages reports whether the request would push to, or prune, this target ref. +func (s TargetScope) Manages(targetRef plumbing.ReferenceName) bool { + // Mapping targets are not subject to exclusions, matching the mapping pass + // in BuildDesiredRefs, which applies exclusions only to auto-discovery. + if _, ok := s.mapped[targetRef]; ok { + return true + } + _, prunable := PruneTarget(targetRef, s.cfg) + return prunable +} + +// PruneTarget reports whether prune could select an ALREADY-UNMANAGED target +// ref for deletion, and with what classification. +// +// It is not the answer to "is this ref in scope" — with Mappings set it reports +// false for every branch, because its only caller has already excluded the +// managed ones. Use TargetScope.Manages for that question. // // It normalizes cfg itself rather than assuming a normalized one. Callers // inside planning have already normalized (this is then a no-op), but the diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go index b2d4b858..3b9d67c6 100644 --- a/internal/syncer/empty_source.go +++ b/internal/syncer/empty_source.go @@ -231,8 +231,12 @@ func (s *syncSession) resolveEmptySource() (Result, error) { // A target that advertises refs this request manages is populated, full // stop — hiding can only ever conceal refs, never invent them, so anything // visible here is real and this is divergence. - if n := s.targetRefsInScope(); n > 0 { - return Result{}, fmt.Errorf("%w (%d in scope)", ErrSourceEmptyTargetPopulated, n) + inScope, err := s.targetRefsInScope() + if err != nil { + return Result{}, err + } + if inScope > 0 { + return Result{}, fmt.Errorf("%w (%d in scope)", ErrSourceEmptyTargetPopulated, inScope) } // An EMPTY target advertisement proves nothing on its own, for the same // reason the source's did not, so it needs the same authoritative @@ -284,28 +288,34 @@ func (s *syncSession) sourceCannotReportUnborn() (string, bool) { return "", false } -// targetRefsInScope counts the target refs this request would actually manage. +// targetRefsInScope counts the target refs this request is responsible for. // // It must ask exactly the question the planner asks, so it delegates to -// planner.PruneTarget rather than re-deriving the answer — a divergence check -// with its own idea of scope reports refs the request has disclaimed, and -// "excluded names only" was such an idea: with Mappings set, an unmapped branch -// is equally untouchable, and counting it left a mapping-pinned mirror -// permanently diverged over a ref it would neither push nor prune. +// planner.TargetScope rather than re-deriving the answer. Both directions of +// getting this wrong are real, and they fail oppositely: too narrow a scope +// leaves a mirror permanently diverged over a ref it would never touch, while +// too wide a scope — or, worse, a predicate that silently omits the mapping +// targets — converges over a target that still holds refs the source does not. // // The zero-hash skip stays here because it is about the value rather than the // name: a zero hash is a deletion sentinel, not a ref that exists. -func (s *syncSession) targetRefsInScope() int { - cfg := planConfig(s.cfg) +func (s *syncSession) targetRefsInScope() (int, error) { + scope, err := planner.NewTargetScope(planConfig(s.cfg)) + if err != nil { + // Unreachable in practice: newSession validates mappings before any + // session exists. Surfaced rather than swallowed, because guessing a + // scope here would mean guessing at divergence. + return 0, fmt.Errorf("resolve target scope: %w", err) + } n := 0 for name, hash := range s.target.refMap { if hash.IsZero() { continue } - if _, managed := planner.PruneTarget(name, cfg); !managed { + if !scope.Manages(name) { continue } n++ } - return n + return n, nil } diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index b556d298..f4d3b2b5 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -683,6 +683,43 @@ func TestResolveEmptySourceRespectsMappingScope(t *testing.T) { } }) + // The case that matters most, and the one an "unmapped refs are out of + // scope" reading silently drops: the target still holds the very ref the + // request maps. Converging here would mean deleting it. + t.Run("the mapped ref itself is divergence", func(t *testing.T) { + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/heads/main": hash} + s := emptySourceSession(mapped(), nil, target, unbornSource()) + _, err := s.resolveEmptySource() + if !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected ErrSourceEmptyTargetPopulated for a populated mapping target, got %v", err) + } + }) + + // A mapping target is managed regardless of exclusions, matching the + // mapping pass in BuildDesiredRefs, which applies exclusions only to + // auto-discovery. + t.Run("a mapped ref stays in scope even when excluded", func(t *testing.T) { + cfg := mapped() + cfg.ExcludeRefPrefixes = []string{"refs/heads/"} + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/heads/main": hash} + s := emptySourceSession(cfg, nil, target, unbornSource()) + if _, err := s.resolveEmptySource(); !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected an excluded-but-mapped target ref to still be divergence, got %v", err) + } + }) + + // Mapping targets are compared by resolved name, so a short-form mapping + // must match the full ref the target advertises. + t.Run("short-form mapping names resolve", func(t *testing.T) { + cfg := converged() + cfg.Mappings = []validation.RefMapping{{Source: "main", Target: "trunk"}} + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/heads/trunk": hash} + s := emptySourceSession(cfg, nil, target, unbornSource()) + if _, err := s.resolveEmptySource(); !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected a short-form mapping target to resolve to refs/heads/trunk, got %v", err) + } + }) + // Tags stay in scope under a mapping, because prune still selects them — // the check must track what the planner does, not a simpler story. t.Run("tags remain divergence", func(t *testing.T) { From 6d1eb86fa6117a19e58454bca68a95be87ca35c5 Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Fri, 21 Aug 2026 23:46:41 +0200 Subject: [PATCH 09/10] Scope is push OR prune, not prune alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BuildDesiredRefs' auto-discovery pass — tags, and other-kind names under AllRefs — sits outside the mapping/branch branch, so a mapping-scoped AllRefs request still mirrors refs/notes/* and tags. Prune is the narrower set: it skips both once Mappings is set. TargetScope.Manages delegated wholly to PruneTarget and so reported those refs out of scope, letting an empty source converge against a target holding refs the config actively mirrors. Manages is now the union of the two halves, which is what its own doc always claimed ("would push to, or prune"). Exclusions still apply to the auto-discovery half, matching BuildDesiredRefs, and mapping targets still bypass them. PruneTarget is deliberately left alone. Widening it would change what prune deletes, which is a live behaviour change well outside this branch — the asymmetry between push and prune scope under mappings is the planner's existing contract, not a bug this PR should quietly alter. The previous revision of the mapping test asserted the wrong thing here: it expected an unmapped namespace to converge, by analogy with unmapped branches. Branches really are out of scope under mappings (the branch pass is in the else); other-kind refs are not. Both cases are now covered, along with the excluded-namespace counterpart that keeps the exclusion behaviour honest. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JvpGRBapBppY4xh2x38kDL Entire-Checkpoint: 01M0K4MP27NST43HX4HC4HP7KC --- internal/planner/planner.go | 20 ++++++++++++++++++++ internal/syncer/empty_source_test.go | 22 ++++++++++++++++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index 05e8bc83..655cba2d 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -286,12 +286,32 @@ func NewTargetScope(cfg PlanConfig) (TargetScope, error) { } // Manages reports whether the request would push to, or prune, this target ref. +// +// Push scope and prune scope are not the same set, and this is their union. +// Delegating wholly to PruneTarget was wrong twice over, in the direction that +// under-reports: it omits mapping targets, and it omits the refs +// BuildDesiredRefs auto-discovers under Mappings. That pass — tags, and +// other-kind names under AllRefs — sits OUTSIDE the mapping/branch branch, so +// a mapping-scoped AllRefs request still mirrors refs/notes/* and tags even +// though prune leaves them alone. func (s TargetScope) Manages(targetRef plumbing.ReferenceName) bool { // Mapping targets are not subject to exclusions, matching the mapping pass // in BuildDesiredRefs, which applies exclusions only to auto-discovery. if _, ok := s.mapped[targetRef]; ok { return true } + if IsRefExcluded(targetRef, s.cfg.ExcludeRefPrefixes, s.cfg.ExcludeRefs) { + return false + } + // The auto-discovery half: what BuildDesiredRefs would push, regardless of + // whether prune would also take it. + switch kind := RefKindFromName(targetRef); { + case kind == RefKindTag && (s.cfg.IncludeTags || s.cfg.AllRefs): + return true + case kind == RefKindOther && s.cfg.AllRefs: + return true + } + // The prune half, which is what additionally covers unmapped branches. _, prunable := PruneTarget(targetRef, s.cfg) return prunable } diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index f4d3b2b5..3ef43294 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -675,11 +675,29 @@ func TestResolveEmptySourceRespectsMappingScope(t *testing.T) { } }) - t.Run("other namespaces likewise", func(t *testing.T) { + // Other namespaces are NOT like unmapped branches, which is what an + // earlier revision of this test got wrong. BuildDesiredRefs' tag and + // other-kind pass sits outside the mapping branch, so an AllRefs request + // mirrors refs/notes/* even with Mappings set — prune is what skips them, + // and push scope is the wider of the two. + t.Run("other namespaces are mirrored under AllRefs, so they diverge", func(t *testing.T) { target := map[plumbing.ReferenceName]plumbing.Hash{"refs/notes/commits": hash} s := emptySourceSession(mapped(), nil, target, unbornSource()) + if _, err := s.resolveEmptySource(); !errors.Is(err, ErrSourceEmptyTargetPopulated) { + t.Fatalf("expected ErrSourceEmptyTargetPopulated for a mirrored namespace, got %v", err) + } + }) + + // Same reasoning, and it is what keeps the exclusion behaviour honest: an + // EXCLUDED other-kind ref is genuinely out of scope, since auto-discovery + // applies exclusions. + t.Run("an excluded namespace stays out of scope", func(t *testing.T) { + cfg := mapped() + cfg.ExcludeRefPrefixes = []string{"refs/notes/"} + target := map[plumbing.ReferenceName]plumbing.Hash{"refs/notes/commits": hash} + s := emptySourceSession(cfg, nil, target, unbornSource()) if _, err := s.resolveEmptySource(); err != nil { - t.Fatalf("expected convergence over an unmapped namespace, got %v", err) + t.Fatalf("expected convergence over an excluded namespace, got %v", err) } }) From 3184daccd191d7d76aaca51b520c05fbfb774137 Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Mon, 24 Aug 2026 11:33:05 +0200 Subject: [PATCH 10/10] Address review: check relay first, and pin the scope primitives where they live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from review, none a correctness bug. All five stand. Relay capability is now checked before the empty-source intercept rather than after planning. The converged result claims Relay: true on the grounds that "replicate refuses a non-relay target outright" — but the intercept returned above that check, so a target whose receive-pack advertisement carries no capabilities got a success asserting a relay the ordinary path would have refused. Nothing moves either way, so convergence is arguably still the right answer, but the field was fabricated in a change whose subject is honest reporting. s.target.policy is populated in newSession, so the check simply moves up. ProbeResult.SourceHeadUnborn makes a claim in the PR body true rather than restating it. The body defended sending `unborn` on every v2 ls-refs partly because it lets Probe report an unborn HEAD without a convergence policy — which was not implemented: HeadUnborn had exactly one reader, behind AllowEmptySource. Probe is the diagnostic surface and the argument is already on the wire, so an operator asking why a mirror will not converge can now see whether the source reported unborn at all. Diagnostic only; it carries none of the policy's weight, and its doc says so. TargetScope and PruneTarget get planner-local tests. Their semantics were wrong in three consecutive commits and PruneTarget now sits on the live prune path, yet every assertion about them lived in internal/syncer. The table covers mapping target, excluded-but-mapped, short-form mapping names, tags under both AllRefs and IncludeTags, other-kind under AllRefs, unmapped branches with and without mappings, both exclusion forms, prune disabled, and an un-normalized Branches filter — plus the PruneTarget-is-narrower property whose conflation caused two of the three regressions. Manages' doc no longer promises to track cfg.Prune. It deliberately does not: the question is whose ref this is, not what this run would do to it, and "source empty, target holds refs" is divergence whether or not this run would have pruned. Behaviour unchanged; the doc was overpromising. buildProbeConfig joins the reflection guard. It is the one request-edge builder outside it, and while ProbeRequest has no ExcludeRefs today, it is exactly where that bug class could recur unseen. CollectStats reaches syncer.Config as ShowStats, so it is skipped with a reason and covered by its own assertion — which also makes the previously-dead skip branch live. Finally, resolveEmptyDesiredSet's doc no longer implies both entry points are live. The pre-planning intercept means its delegation is currently unreachable; it is kept so the paths cannot drift if that gate is loosened, and now says so. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JvpGRBapBppY4xh2x38kDL Entire-Checkpoint: 01M0SHVJW7ZX6S8F925MKM07TM --- client_test.go | 32 +++++++++ internal/planner/planner.go | 10 ++- internal/planner/targetscope_test.go | 104 +++++++++++++++++++++++++++ internal/syncer/empty_source.go | 11 ++- internal/syncer/empty_source_test.go | 42 +++++++++++ internal/syncer/syncer.go | 41 ++++++++--- results.go | 52 ++++++++------ 7 files changed, 259 insertions(+), 33 deletions(-) create mode 100644 internal/planner/targetscope_test.go diff --git a/client_test.go b/client_test.go index 5ac2f534..65858fb8 100644 --- a/client_test.go +++ b/client_test.go @@ -370,3 +370,35 @@ func TestValidateRejectsUnusableAllowEmptySource(t *testing.T) { t.Errorf("an unscoped replicate with the policy set must validate, got %v", err) } } + +// buildProbeConfig is the one request-edge builder the guard above does not +// cover, because ProbeRequest carries flat fields rather than a RefScope. It +// drops nothing today — ProbeRequest has no ExcludeRefs — but it is exactly +// where the bug class the guard exists for could recur unseen, so it gets the +// same treatment. +func TestBuildProbeConfigThreadsEveryField(t *testing.T) { + syncertest.AssertFieldsThreaded(t, map[string]string{ + "CollectStats": "deliberately renamed: reaches syncer.Config as ShowStats", + }, func(t *testing.T, req ProbeRequest) any { + req.Source = Endpoint{URL: "https://source.example/repo.git"} + cfg, err := New(Options{}).buildProbeConfig(context.Background(), req) + if err != nil { + t.Fatalf("buildProbeConfig: %v", err) + } + return cfg + }) +} + +// The renamed field still has to arrive, it just cannot be checked by name. +func TestBuildProbeConfigThreadsCollectStats(t *testing.T) { + cfg, err := New(Options{}).buildProbeConfig(context.Background(), ProbeRequest{ + Source: Endpoint{URL: "https://source.example/repo.git"}, + CollectStats: true, + }) + if err != nil { + t.Fatalf("buildProbeConfig: %v", err) + } + if !cfg.ShowStats { + t.Error("ProbeRequest.CollectStats = true was dropped by buildProbeConfig") + } +} diff --git a/internal/planner/planner.go b/internal/planner/planner.go index 655cba2d..d63cee2f 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -285,7 +285,15 @@ func NewTargetScope(cfg PlanConfig) (TargetScope, error) { return scope, nil } -// Manages reports whether the request would push to, or prune, this target ref. +// Manages reports whether this target ref is the request's responsibility — +// that is, whether the request would push to it, or whether prune could select +// it were prune enabled. +// +// Deliberately independent of cfg.Prune. A caller asking "is this ref mine" +// while prune is off still needs the same answer: the divergence check reads +// it to decide whether a target ref contradicts a claim of convergence, and +// "the source is empty but the target holds refs" is a genuine disagreement +// whether or not this particular run would have deleted them. // // Push scope and prune scope are not the same set, and this is their union. // Delegating wholly to PruneTarget was wrong twice over, in the direction that diff --git a/internal/planner/targetscope_test.go b/internal/planner/targetscope_test.go new file mode 100644 index 00000000..a76b1b51 --- /dev/null +++ b/internal/planner/targetscope_test.go @@ -0,0 +1,104 @@ +package planner + +import ( + "testing" + + "github.com/go-git/go-git/v6/plumbing" +) + +// TargetScope decides whether a target ref is the request's responsibility, and +// the divergence check in internal/syncer reads it to decide whether a target +// ref contradicts a claim that two repositories agree. Its semantics were wrong +// in three consecutive commits, each time because the answer was derived from +// one half of the question — so the contract is pinned here, next to the code, +// rather than only indirectly through the syncer. +func TestTargetScopeManages(t *testing.T) { + t.Parallel() + + mapping := []RefMapping{{Source: "refs/heads/main", Target: "refs/heads/main"}} + + cases := map[string]struct { + cfg PlanConfig + ref plumbing.ReferenceName + want bool + }{ + // A mapping target is pushed by BuildDesiredRefs' mapping pass, so it + // is in scope even though prune declines every branch under mappings. + "mapping target": {PlanConfig{AllRefs: true, Mappings: mapping}, "refs/heads/main", true}, + "mapping target, short form": {PlanConfig{AllRefs: true, Mappings: []RefMapping{{Source: "main", Target: "trunk"}}}, "refs/heads/trunk", true}, + // Exclusions apply to auto-discovery, not to explicit mappings — + // matching the mapping pass in BuildDesiredRefs. + "mapping target, excluded": {PlanConfig{AllRefs: true, Mappings: mapping, ExcludeRefPrefixes: []string{"refs/heads/"}}, "refs/heads/main", true}, + // An unmapped branch is neither pushed (that pass is in the else) nor + // pruned once mappings are set. + "unmapped branch with mappings": {PlanConfig{AllRefs: true, Mappings: mapping}, "refs/heads/other", false}, + "branch without mappings": {PlanConfig{AllRefs: true}, "refs/heads/other", true}, + // The tag and other-kind pass sits OUTSIDE the mapping/branch branch, + // so both are still mirrored under mappings even though prune skips them. + "tag under AllRefs": {PlanConfig{AllRefs: true, Mappings: mapping}, "refs/tags/v1", true}, + "tag under IncludeTags": {PlanConfig{IncludeTags: true}, "refs/tags/v1", true}, + "tag without either": {PlanConfig{}, "refs/tags/v1", false}, + "other kind under AllRefs": {PlanConfig{AllRefs: true, Mappings: mapping}, "refs/notes/commits", true}, + "other kind without AllRefs": {PlanConfig{IncludeTags: true}, "refs/notes/commits", false}, + "excluded by prefix": {PlanConfig{AllRefs: true, ExcludeRefPrefixes: []string{"refs/pull/"}}, "refs/pull/1/head", false}, + "excluded by exact name": {PlanConfig{AllRefs: true, ExcludeRefs: []string{"refs/heads/entire"}}, "refs/heads/entire", false}, + "exact exclusion spares children": {PlanConfig{AllRefs: true, ExcludeRefs: []string{"refs/heads/entire"}}, "refs/heads/entire/foo", true}, + // Prune being off must not narrow the answer: the question is whose ref + // this is, not what this run would do to it. + "prune off, still in scope": {PlanConfig{AllRefs: true, Prune: false}, "refs/heads/other", true}, + // AllRefs clears a Branches filter during normalization; a scope built + // from an un-normalized config must not read the stale filter. + "unnormalized branches filter": {PlanConfig{AllRefs: true, Branches: []string{"main"}}, "refs/heads/other", true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + scope, err := NewTargetScope(tc.cfg) + if err != nil { + t.Fatalf("NewTargetScope: %v", err) + } + if got := scope.Manages(tc.ref); got != tc.want { + t.Errorf("Manages(%s) = %t, want %t", tc.ref, got, tc.want) + } + }) + } +} + +// PruneTarget answers the narrower question — whether prune could select an +// ALREADY-UNMANAGED ref — and reading it as whole-scope is what broke twice. +// The difference is pinned so the distinction cannot quietly erode. +func TestPruneTargetIsNarrowerThanScope(t *testing.T) { + t.Parallel() + + cfg := PlanConfig{AllRefs: true, Mappings: []RefMapping{{Source: "refs/heads/main", Target: "refs/heads/main"}}} + scope, err := NewTargetScope(cfg) + if err != nil { + t.Fatalf("NewTargetScope: %v", err) + } + + for _, ref := range []plumbing.ReferenceName{"refs/heads/main", "refs/notes/commits"} { + if _, prunable := PruneTarget(ref, cfg); prunable { + t.Errorf("PruneTarget(%s) = true; prune declines these under mappings", ref) + } + if !scope.Manages(ref) { + t.Errorf("Manages(%s) = false; the request still pushes it", ref) + } + } +} + +// NewTargetScope resolves mapping names the same way BuildDesiredRefs does, so +// an invalid mapping must fail here rather than silently yielding a scope that +// disagrees with the planner. +func TestNewTargetScopeRejectsInvalidMappings(t *testing.T) { + t.Parallel() + + _, err := NewTargetScope(PlanConfig{ + Mappings: []RefMapping{ + {Source: "main", Target: "stable"}, + {Source: "release", Target: "stable"}, + }, + }) + if err == nil { + t.Fatal("expected duplicate target refs to be rejected") + } +} diff --git a/internal/syncer/empty_source.go b/internal/syncer/empty_source.go index 3b9d67c6..2f05af72 100644 --- a/internal/syncer/empty_source.go +++ b/internal/syncer/empty_source.go @@ -125,10 +125,17 @@ func validateEmptySourcePolicy(cfg Config) error { // resolveEmptyDesiredSet decides what an empty desired set means when planning // produced no refs to act on. // -// It is the post-planning entry point only. An empty source ADVERTISEMENT is +// It is the post-planning entry point. An empty source ADVERTISEMENT is // intercepted before planning (see runReplicate), because a mapping whose // source ref is absent errors inside BuildDesiredRefs and would never let this -// run; both entry points share resolveEmptySource so they cannot disagree. +// run. +// +// Given that intercept, the delegation below is currently unreachable: getting +// here with an empty advertisement requires the opt-in to be off or the scope +// narrowed, and either returns the historical error one line earlier. It is +// kept rather than replaced with an assertion so the two entry points cannot +// drift apart if that gate is ever loosened — but do not read it as evidence +// that both are live today. Only the pre-planning path reaches convergence. func (s *syncSession) resolveEmptyDesiredSet() (Result, error) { if !s.cfg.AllowEmptySource || !s.cfg.AllRefs { return Result{}, errors.New("no source refs matched") diff --git a/internal/syncer/empty_source_test.go b/internal/syncer/empty_source_test.go index 3ef43294..8861e056 100644 --- a/internal/syncer/empty_source_test.go +++ b/internal/syncer/empty_source_test.go @@ -12,6 +12,7 @@ import ( "github.com/go-git/go-git/v6/storage/memory" "entire.io/entire/git-sync/internal/gitproto" + "entire.io/entire/git-sync/internal/planner" "entire.io/entire/git-sync/internal/validation" ) @@ -764,3 +765,44 @@ func TestResolveEmptySourceNormalizesScopeBeforeJudging(t *testing.T) { t.Fatalf("a populated target must refuse regardless of a stale Branches filter, got %v", err) } } + +// The converged result claims Relay: true on the strength of replicate +// refusing non-relay targets outright. That justification only holds if the +// relay check actually runs on this path — and it did not: the empty-source +// intercept returned first, so a target whose receive-pack advertisement +// carries no capabilities got a success claiming a relay that the ordinary +// path would have refused. Nothing moves either way, but the field was +// fabricated, in a change whose whole subject is honest reporting. +func TestRunReplicateChecksRelayBeforeResolvingEmptySource(t *testing.T) { + s := emptySourceSession(converged(), nil, nil, unbornSource()) + // CapabilitiesKnown false is what an unreadable target advertisement + // produces; SupportsReplicateRelay rejects it. + s.target.policy = planner.RelayTargetPolicy{CapabilitiesKnown: false} + + _, err := s.runReplicate(context.Background()) + if err == nil { + t.Fatal("expected a non-relay-capable target to be refused, got convergence") + } + if !strings.Contains(err.Error(), "replicate requires relay-capable target") { + t.Errorf("error = %q, want the relay-capability refusal", err) + } + // It must not be reported as an empty-source outcome: the run never got + // far enough to judge emptiness. + for _, sentinel := range []error{ErrNoRefsSelected, ErrSourceEmptyUnverified, ErrTargetEmptyUnverified, ErrSourceEmptyTargetPopulated} { + if errors.Is(err, sentinel) { + t.Errorf("relay refusal misreported as %v", sentinel) + } + } + + // With a capable target the same session converges, so the new check is + // a gate on capability rather than a blanket refusal. + s = emptySourceSession(converged(), nil, nil, unbornSource()) + s.target.policy = planner.RelayTargetPolicy{CapabilitiesKnown: true} + result, err := s.runReplicate(context.Background()) + if err != nil { + t.Fatalf("expected convergence against a relay-capable target, got %v", err) + } + if !result.Converged || !result.Relay { + t.Errorf("converged=%t relay=%t; want both true", result.Converged, result.Relay) + } +} diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 284aad65..3579ea86 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -245,8 +245,21 @@ type ProbeResult struct { TargetCaps []string `json:"targetCapabilities,omitempty"` Refs []RefInfo `json:"refs"` SourceHEAD plumbing.ReferenceName `json:"sourceHead,omitempty"` - Stats Stats `json:"stats"` - Measurement Measurement `json:"measurement"` + // SourceHeadUnborn is true when the source reported HEAD as unborn: its + // symref target does not exist. Diagnostic only — it is emphatically NOT a + // statement that the repository is empty (git emits the line for any + // dangling HEAD), and acting on emptiness needs the far stricter path + // behind SyncPolicy.AllowEmptySource. + // + // Surfaced here because probe is the diagnostic surface and the ls-refs + // request already carries the unborn argument on every v2 listing: an + // operator asking "why will this mirror not converge" can see whether the + // source reported unborn at all, which is otherwise invisible. False for a + // v1 source, or a v2 source that does not advertise ls-refs=unborn, + // however empty it is. + SourceHeadUnborn bool `json:"sourceHeadUnborn"` + Stats Stats `json:"stats"` + Measurement Measurement `json:"measurement"` } func (r ProbeResult) Lines() []string { @@ -261,6 +274,9 @@ func (r ProbeResult) Lines() []string { if r.SourceHEAD != "" { lines = append(lines, "source-head: "+r.SourceHEAD.String()) } + if r.SourceHeadUnborn { + lines = append(lines, "source-head-unborn: true") + } if len(r.Capabilities) > 0 { lines = append(lines, "source-capabilities: "+strings.Join(r.Capabilities, ", ")) } @@ -1048,6 +1064,15 @@ func (s *syncSession) runReplicate(ctx context.Context) (Result, error) { // // Gated on the opt-in so a caller that never asked for this still gets the // planner's error verbatim. + // + // The relay check runs FIRST, above both this intercept and planning. It is + // a property of the target, not of the work: replicate refuses a non-relay + // target outright, and a converged result claims Relay: true on the + // strength of that refusal. Deciding emptiness ahead of it would let the + // one path that makes the claim be the one path that never checked it. + if ok, reason := planner.SupportsReplicateRelay(s.target.policy); !ok { + return Result{OperationMode: modeReplicate}, fmt.Errorf("replicate requires relay-capable target: %s; use sync instead", reason) + } if s.cfg.AllowEmptySource && s.cfg.AllRefs && len(s.sourceRefMap) == 0 { return s.resolveEmptySource() } @@ -1059,10 +1084,6 @@ func (s *syncSession) runReplicate(ctx context.Context) (Result, error) { return s.resolveEmptyDesiredSet() } - if ok, reason := planner.SupportsReplicateRelay(s.target.policy); !ok { - return Result{OperationMode: modeReplicate}, fmt.Errorf("replicate requires relay-capable target: %s; use sync instead", reason) - } - allAbsent := s.replicateCanBootstrap(desiredRefs) if allAbsent { if s.cfg.DryRun { @@ -1395,8 +1416,12 @@ func (s *syncSession) newProbeResult() ProbeResult { Capabilities: s.sourceService.Capabilities(), Refs: refInfos, SourceHEAD: s.sourceService.HeadTarget, - Stats: s.stats.snapshot(), - Measurement: s.measurementDone(), + // Reported even though nothing in Probe acts on it: it is the one + // wire fact that explains an unconvergeable empty mirror, and it is + // otherwise unobservable from outside. + SourceHeadUnborn: s.sourceService.HeadUnborn, + Stats: s.stats.snapshot(), + Measurement: s.measurementDone(), } if s.target != nil { result.TargetURL = redact.URL(s.cfg.Target.URL) diff --git a/results.go b/results.go index 6dd23e97..b204eb29 100644 --- a/results.go +++ b/results.go @@ -89,17 +89,24 @@ type Measurement struct { // ProbeResult is the outcome of a Probe. type ProbeResult struct { - SourceURL string `json:"sourceUrl"` - TargetURL string `json:"targetUrl,omitempty"` - RequestedMode string `json:"requestedMode"` - Protocol string `json:"protocol"` - RefPrefixes []string `json:"refPrefixes"` - Capabilities []string `json:"sourceCapabilities"` - TargetCaps []string `json:"targetCapabilities,omitempty"` - Refs []RefInfo `json:"refs"` - SourceHEAD string `json:"sourceHead,omitempty"` - Stats Stats `json:"stats"` - Measurement Measurement `json:"measurement"` + SourceURL string `json:"sourceUrl"` + TargetURL string `json:"targetUrl,omitempty"` + RequestedMode string `json:"requestedMode"` + Protocol string `json:"protocol"` + RefPrefixes []string `json:"refPrefixes"` + Capabilities []string `json:"sourceCapabilities"` + TargetCaps []string `json:"targetCapabilities,omitempty"` + Refs []RefInfo `json:"refs"` + SourceHEAD string `json:"sourceHead,omitempty"` + // SourceHeadUnborn reports that the source described HEAD as unborn — its + // target branch does not exist. Diagnostic only: git emits that line for + // any dangling HEAD, so it is never on its own evidence that a repository + // is empty. It answers "did the source say anything about an unborn HEAD", + // which is what distinguishes a source that cannot report one (protocol + // v1, or a v2 server not advertising ls-refs=unborn) from one that did not. + SourceHeadUnborn bool `json:"sourceHeadUnborn"` + Stats Stats `json:"stats"` + Measurement Measurement `json:"measurement"` } // SyncCounts tallies per-ref outcomes of a sync. @@ -157,17 +164,18 @@ type PlanResult = SyncResult func fromProbeResult(result syncer.ProbeResult) ProbeResult { out := ProbeResult{ - SourceURL: result.SourceURL, - TargetURL: result.TargetURL, - RequestedMode: result.RequestedMode, - Protocol: result.Protocol, - RefPrefixes: append([]string(nil), result.RefPrefixes...), - Capabilities: append([]string(nil), result.Capabilities...), - TargetCaps: append([]string(nil), result.TargetCaps...), - Refs: make([]RefInfo, 0, len(result.Refs)), - SourceHEAD: result.SourceHEAD.String(), - Stats: fromStats(result.Stats), - Measurement: fromMeasurement(result.Measurement), + SourceURL: result.SourceURL, + TargetURL: result.TargetURL, + RequestedMode: result.RequestedMode, + Protocol: result.Protocol, + RefPrefixes: append([]string(nil), result.RefPrefixes...), + Capabilities: append([]string(nil), result.Capabilities...), + TargetCaps: append([]string(nil), result.TargetCaps...), + Refs: make([]RefInfo, 0, len(result.Refs)), + SourceHEAD: result.SourceHEAD.String(), + SourceHeadUnborn: result.SourceHeadUnborn, + Stats: fromStats(result.Stats), + Measurement: fromMeasurement(result.Measurement), } for _, ref := range result.Refs { out.Refs = append(out.Refs, RefInfo{Name: ref.Name, Hash: ref.Hash.String()})