Library AllRefs implies tag inclusion; close the contract gap

3acfb7b→main·

Soph·2mo ago·8 files·+97 added/-31 removed

The library docs and the unstable fetch CLI promised "every refs/*", but BuildDesiredRefs and addPruneCandidates excluded tags unless IncludeTags was set. The CLI sync/bootstrap wrappers papered over this by implying --tags, but library callers and fetch --all-refs got a narrower scope than the wording promised.

Move the tag-inclusion implication into the library so the contract is honest at every layer:

• BuildDesiredRefs adds source tags when IncludeTags || AllRefs.
• addPruneCandidates' tag-prune case fires under the same condition.
• replicateCanBootstrap matches.

Drop the now-redundant CLI bundling: sync/bootstrap no longer set IncludeTags from --all-refs because the library covers it. SyncPolicy. BestEffort stays orthogonal so callers can opt into per-ref warn semantics on a narrower scope.

Updated the planner test that asserted the old (wrong) "tag should not appear without IncludeTags" behavior, and added a fetch CLI smoke test verifying fetch --all-refs puts tag and notes refs into the wants list without --tags being passed.

Sessions

a0c571b71c5fView transcript

Changes

8

• cmd/git-sync

• Mbootstrap.go+1/-1

  • Mmain_test.go+59
  • Msyncplan.go+5/-3

• docs

• Musage.md+14/-11

• internal

• planner

• Mplanner.go+4/-2

  • Mplanner_test.go+11/-12

• syncer

• Msyncer.go+1/-1

• Mtypes.go+2/-1

73 unmodified lines

74
75
76
77
77
78
79
80

73 unmodified lines

cmd.Flags().StringVar(&branches, "branch", "", "comma-separated branch list; default is all source branches")
    cmd.Flags().StringArrayVar(&mappings, "map", nil, "ref mapping in src:dst form; short names map branches, full refs map exact refs")
    cmd.Flags().BoolVar(&req.IncludeTags, "tags", false, "mirror tags")
    allRefsFlag(cmd, allRefsUsageBestEffort, &req.Scope.AllRefs, &req.BestEffort, &req.IncludeTags)
    allRefsFlag(cmd, allRefsUsageBestEffort, &req.Scope.AllRefs, &req.BestEffort)
    cmd.Flags().BoolVar(&req.Options.CollectStats, "stats", false, "print transfer statistics")
    cmd.Flags().BoolVar(&req.Options.MeasureMemory, "measure-memory", false, "sample elapsed time and Go heap usage")
    cmd.Flags().BoolVar(&req.Options.Progress, "progress", false, "show live per-side throughput on stderr (TTY only)")

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

331 unmodified lines

332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396

331 unmodified lines

}
}

// fetch --all-refs must broaden discovery to tags and other-kind refs
// without needing --tags, matching the AllRefs contract at the library level.
func TestRun_Fetch_AllRefsCoversTagsAndOtherKind(t *testing.T) {
    sourceRepo, sourceFS := newSourceRepo(t)
    makeCommits(t, sourceRepo, sourceFS, 1)

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

sourceServer := newSmartHTTPRepoServer(t, sourceRepo)
    defer sourceServer.Close()

output, err := captureStdout(func() error {
        return run(context.Background(), []string{
            "fetch",
            "--all-refs",
            "--json",
            sourceServer.RepoURL(),
        })
    })
    if err != nil {
        t.Fatalf("run fetch --all-refs: %v\noutput=%s", err, output)
    }

var result map[string]any
    if err := json.Unmarshal([]byte(output), &result); err != nil {
        t.Fatalf("decode fetch json: %v\noutput=%s", err, output)
    }
    wants, ok := result["wants"].([]any)
    if !ok {
        t.Fatalf("expected wants in result, got %#v", result)
    }
    seen := make(map[string]bool)
    for _, raw := range wants {
        entry, ok := raw.(map[string]any)
        if !ok {
            continue
        }
        if name, _ := entry["name"].(string); name != "" {
            seen[name] = true
        }
    }
    for _, want := range []string{string(tagRef), string(notesRef)} {
        if !seen[want] {
            t.Errorf("expected %s in fetch wants, got %v", want, seen)
        }
    }
}

// replicate must keep strict failure semantics — its contract is "target
// matches source" — so --all-refs must NOT bundle BestEffort the way it does
// for sync. A target ng on any ref must surface as a non-nil error.

Mcmd/git-sync/main_test.go+59

110 unmodified lines

111
112
113
114
115
116
114
115
116
117
118
119
120
121

110 unmodified lines

cmd.Flags().BoolVar(&req.Policy.IncludeTags, "tags", false, "mirror tags")
    cmd.Flags().BoolVar(&req.Policy.Force, "force", false, "allow non-fast-forward branch updates and retarget tags")
    cmd.Flags().BoolVar(&req.Policy.Prune, "prune", false, "delete managed target refs that no longer exist on source")
    // Replicate keeps strict failure semantics — its contract is "target
    // refs match source." BestEffort would let partial mirrors exit success.
    implies := []*bool{&req.Policy.IncludeTags}
    // Tag inclusion is now handled at the library level (AllRefs implies
    // it in BuildDesiredRefs). Replicate keeps strict failure semantics —
    // its contract is "target refs match source," so BestEffort is not
    // bundled there; sync/plan get it for the best-effort UX.
    var implies []*bool
    usage := allRefsUsageBestEffort
    if defaultMode == gitsync.ModeReplicate {
        usage = allRefsUsageStrict
    }

Mcmd/git-sync/syncplan.go+5/-3

164 unmodified lines

165
166
167
168
169
170
171
172
173
174
175
168
169
170
171
172
173
174
175
176
177
178
179
180
181
1 unmodified line

183
184
185
183
184
185
186
187
188
189
190
191

164 unmodified lines

```

`--all-refs` broadens the source ref discovery from `refs/heads/`+`refs/tags/`
to every `refs/*` namespace and lets ref mappings target arbitrary
namespaces (`refs/notes/*`, `refs/pull/*`, custom refs). For `sync` and
`bootstrap` the flag also implies `--tags` (so the broader scope really is
"every refs/*") and turns on best-effort failure handling: when the
target's `receive-pack` rejects an individual ref (e.g. GitHub refusing
writes to `refs/pull/*` hidden refs), the rejected ref appears in the
result with `action=warn` and the server's reason instead of failing the
whole sync. Pack-level transport or unpack failures remain fatal.

to every `refs/*` namespace (branches, tags, `refs/notes/*`, `refs/pull/*`,
custom refs) and lets ref mappings target arbitrary namespaces. Tag
inclusion is implied — `RefScope.AllRefs` covers tags at the library level,
so `--tags` does not need to be combined with `--all-refs`.

For `sync` and `bootstrap` the flag also turns on best-effort failure
handling: when the target's `receive-pack` rejects an individual ref (e.g.
GitHub refusing writes to `refs/pull/*` hidden refs), the rejected ref
appears in the result with `action=warn` and the server's reason instead
of failing the whole sync. Pack-level transport or unpack failures remain
fatal.

`replicate --all-refs` broadens the same scope but does NOT enable
best-effort. Replicate's contract is "target refs match source"; downgrading
1 unmodified line

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

Library callers can decouple the halves: `RefScope.AllRefs`,
`SyncPolicy.IncludeTags`, and `SyncPolicy.BestEffort` are independent.
The CLI bundles them under `--all-refs` for convenience.
`SyncPolicy.BestEffort` is independent of scope and can be set without
`AllRefs` if a library caller wants per-ref warn semantics on a narrower
scope.

Force source-side protocol v2:

Mdocs/usage.md+14/-11

78 unmodified lines

79
80
81
82
82
83
84
85
86

146 unmodified lines

234
235
236
235
237
238
239
240

78 unmodified lines

}
    
    if cfg.IncludeTags {
    // AllRefs implies tag inclusion: the contract is "every refs/* on the
    // source," so tags are part of the broadened scope by definition.
    if cfg.IncludeTags || cfg.AllRefs {
        for refName, hash := range sourceRefs {
            if !refName.IsTag() {
                continue
146 unmodified lines

continue
    }
        switch {
        case targetRef.IsTag() && cfg.IncludeTags:
        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()}

Minternal/planner/planner.go+4/-2

389 unmodified lines

390
391
392
393
393
394
395
396
397
398
399
400
398
399
400
401
402
403
404
402
403
404
405
406
407
408
409
405
406
407
408
409
410
411

389 unmodified lines

plumbing.ReferenceName("refs/pull/1/head"):   hashPull,
    }

t.Run("AllRefs adds non-branch non-tag refs", func(t *testing.T) {
        t.Run("AllRefs covers branches, tags, and other-kind refs", func(t *testing.T) {
            desired, _, err := BuildDesiredRefs(sourceRefs, PlanConfig{AllRefs: true})
            if err != nil {
                t.Fatalf("unexpected error: %v", err)
            }
            // Branches still come in; tags need IncludeTags; "other" comes from AllRefs.
            if _, ok := desired[plumbing.ReferenceName("refs/notes/commits")]; !ok {
                t.Error("expected refs/notes/commits in desired set")
            // AllRefs implies tag inclusion: the contract is "every refs/*".
            want := []plumbing.ReferenceName{
                plumbing.NewBranchReferenceName("main"),
                plumbing.NewTagReferenceName("v1.0"),
                plumbing.ReferenceName("refs/notes/commits"),
                plumbing.ReferenceName("refs/pull/1/head"),
            }
            if _, ok := desired[plumbing.ReferenceName("refs/pull/1/head")]; !ok {
                t.Error("expected refs/pull/1/head in desired set")
            }
            if _, ok := desired[plumbing.NewTagReferenceName("v1.0")]; ok {
                t.Error("tag should not appear without IncludeTags")
            }
            if _, ok := desired[plumbing.NewBranchReferenceName("main")]; !ok {
                t.Error("branch should still appear with default selection")
            for _, ref := range want {
                if _, ok := desired[ref]; !ok {
                    t.Errorf("expected %s in desired set", ref)
                }
            }
        })
}

Minternal/planner/planner_test.go+11/-12

879 unmodified lines

880
881
882
883
883
884
885
886

879 unmodified lines

continue
    }
    switch {
    case targetRef.IsTag() && s.cfg.IncludeTags:
    case targetRef.IsTag() && (s.cfg.IncludeTags || s.cfg.AllRefs):
        return false
    case targetRef.IsBranch() && len(s.cfg.Mappings) == 0 && len(s.cfg.Branches) == 0:
        return false

Minternal/syncer/syncer.go+1/-1

78 unmodified lines

79
80
81
82
82
83
84
85
86

78 unmodified lines

// RefScope constrains which refs a request manages. AllRefs broadens scope
// to every refs/* on the source (notes, pulls, custom namespaces).
// to every refs/* on the source (branches, tags, notes, pulls, custom
// namespaces) and implies SyncPolicy.IncludeTags.
type RefScope struct {
    Branches []string     `json:"branches"`
    Mappings []RefMapping `json:"mappings"`
}

Mtypes.go+2/-1