Fix three review findings on --all-refs · Entire
Fix three review findings on --all-refs
5265400→main· Soph·2mo ago·7 files·+240 added/-24 removed
replicate --all-refs no longer enables BestEffort. Bundling them contradicts replicate's "target == source" contract — a host rejecting refs/pull/* would leave the target incomplete while replicate exited successfully. allRefsFlag now takes a per-command implications list; sync/bootstrap include BestEffort, replicate doesn't. Added a CLI smoke test that hooks the target receive-pack to ng every ref and verifies: replicate --all-refs errors, sync --all-refs warns and exits 0. The help text also splits per command (best-effort vs strict).
replicateCanBootstrap missed AllRefs other-kind refs in its prune check, so replicate could shortcut to bootstrap and silently leave stale refs/notes/* on target. Added the matching case to mirror the prune-candidate logic in planner. Added an integration test that seeds a stale notes ref on target and asserts replicate --prune --all-refs deletes it.
The same test surfaced a pre-existing latent bug: runReplicate gated the executeReplicate call on len(relayPlans) > 0, so delete-only plans (no source-side updates) were silently skipped — even though replicate.Execute itself handles delete-only correctly. The gate is now len(pushPlans) > 0; the CanReplicateRelay check is only run when there are relay plans to validate. Existing replicate-prune test still passes.
CLI --all-refs now also implies --tags for sync and bootstrap so the help text "every refs/*" is honest. Library callers keep the three flags (AllRefs, IncludeTags, BestEffort) orthogonal. Docs updated to describe the per-command bundling and the deliberate decoupling for replicate.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Sessions
7674291aab5fView transcript
Changes
7
cmd/git-sync
Mbootstrap.go+1/-1
Mflags.go+17/-9
Mmain_test.go+138
Msyncplan.go+10/-1
docs
Musage.md+16/-9
internal/syncer
Mintegration_test.go+50
Msyncer.go+8/-4
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, &req.Scope.AllRefs, &req.BestEffort)
allRefsFlag(cmd, allRefsUsageBestEffort, &req.Scope.AllRefs, &req.BestEffort, &req.IncludeTags)
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
45 unmodified lines
46
47
48
49
50
51
52
53
54
55
56
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
62
66
67
68
69
70
71
72
73
45 unmodified lines
cmd.Flags().Var(mode, "protocol", "protocol mode: auto, v1, or v2")
}
// allRefsFlag registers --all-refs. The CLI semantic bundles AllRefs scope
// with best-effort failure handling: pass bestEffort to have it set whenever
// --all-refs is, via a PreRunE hook that fires after flag parsing.
func allRefsFlag(cmd *cobra.Command, allRefs, bestEffort *bool) {
cmd.Flags().BoolVar(allRefs, "all-refs", false,
"mirror every refs/* on the source (notes, pulls, custom namespaces) on a best-effort basis; "+
"per-ref server rejections become warnings rather than failing the sync")
if bestEffort == nil {
const (
allRefsUsageBestEffort = "mirror every refs/* on the source (branches, tags, notes, pulls, custom namespaces) on a best-effort basis; per-ref server rejections become warnings rather than failing the sync"
allRefsUsageStrict = "mirror every refs/* on the source (branches, tags, notes, pulls, custom namespaces); per-ref rejections fail the run, since replicate's contract is target == source"
allRefsUsageScopeOnly = "include every refs/* on the source (notes, pulls, custom namespaces) — scope only, no failure-handling effect"
}
// allRefsFlag registers --all-refs with the supplied usage string and
// bundles its implications. Each pointer in implies is set to true when
// --all-refs is set, via a PreRunE hook that fires after flag parsing.
func allRefsFlag(cmd *cobra.Command, usage string, allRefs *bool, implies ...*bool) {
cmd.Flags().BoolVar(allRefs, "all-refs", false, usage)
if len(implies) == 0 {
return
}
prev := cmd.PreRunE
cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
if *allRefs {
*bestEffort = true
for _, p := range implies {
if p != nil {
*p = true
}
}
}
if prev != nil {
return prev(cmd, args)
}
return nil
}
}
Mcmd/git-sync/flags.go+17/-9
19 unmodified lines
20
21
22
23
24
25
26
27
28
29
302 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
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
133 unmodified lines
562
563
564
565
566
567
568
569
570
571
572
125 unmodified lines
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
19 unmodified lines
git "github.com/go-git/go-git/v6"
"github.com/go-git/go-git/v6/plumbing"
"github.com/go-git/go-git/v6/plumbing/object"
"github.com/go-git/go-git/v6/plumbing/format/pktline"
"github.com/go-git/go-git/v6/plumbing/protocol/packp"
"github.com/go-git/go-git/v6/plumbing/protocol/packp/capability"
"github.com/go-git/go-git/v6/plumbing/protocol/packp/sideband"
"github.com/go-git/go-git/v6/plumbing/transport"
"github.com/go-git/go-git/v6/storage/memory"
)
302 unmodified lines
}
// 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.
func TestRun_Replicate_AllRefsKeepsStrictFailureOnNg(t *testing.T) {
sourceRepo, sourceFS := newSourceRepo(t)
makeCommits(t, sourceRepo, sourceFS, 1)
targetRepo, err := git.Init(memory.NewStorage())
if err != nil {
t.Fatalf("init target repo: %v", err)
}
sourceServer := newSmartHTTPRepoServer(t, sourceRepo)
targetServer := newSmartHTTPRepoServer(t, targetRepo)
defer sourceServer.Close()
defer targetServer.Close()
targetServer.receivePackHook = func(req *packp.UpdateRequests) *packp.ReportStatus {
treport := packp.NewReportStatus()
report.UnpackStatus = "ok"
for _, cmd := range req.Commands {
report.CommandStatuses = append(report.CommandStatuses, &packp.CommandStatus{
ReferenceName: cmd.Name,
Status: "deny updating a hidden ref",
})
}
return report
}
err = run(context.Background(), []string{
modeReplicate,
"--all-refs",
"--json",
sourceServer.RepoURL(),
targetServer.RepoURL(),
})
if err == nil {
t.Fatal("expected replicate --all-refs to error on per-ref ng")
}
}
// Mirror of the above for sync: the same target rejection must turn into a
// warning and a successful exit, so the CLI binding really differs by mode.
func TestRun_Sync_AllRefsWarnsOnNg(t *testing.T) {
sourceRepo, sourceFS := newSourceRepo(t)
makeCommits(t, sourceRepo, sourceFS, 1)
targetRepo, err := git.Init(memory.NewStorage())
if err != nil {
t.Fatalf("init target repo: %v", err)
}
targetServer.receivePackHook = func(req *packp.UpdateRequests) *packp.ReportStatus {
make_report := packp.NewReportStatus()
make_report.UnpackStatus = "ok"
for _, cmd := range req.Commands {
make_report.CommandStatuses = append(make_report.CommandStatuses, &packp.CommandStatus{
ReferenceName: cmd.Name,
Status: "deny updating a hidden ref",
})
}
return make_report
}
output, err := captureStdout(func() error {
return run(context.Background(), []string{
"sync",
"--all-refs",
"--json",
sourceServer.RepoURL(),
targetServer.RepoURL(),
})
})
if err != nil {
t.Fatalf("expected sync --all-refs to succeed with warning, got: %v\noutput=%s", err, output)
}
var result map[string]any
if err := json.Unmarshal([]byte(output), &result); err != nil {
t.Fatalf("decode sync json: %v\noutput=%s", err, output)
}
if got, _ := result["warned"].(float64); got == 0 {
t.Fatalf("expected warned > 0 in result, got %#v", result["warned"])
}
}
func TestRun_Replicate_SubcommandRejectsForce(t *testing.T) {
err := run(context.Background(), []string{
modeReplicate,
"--force",
})
if err != nil {
if err.Error() != "replicate with force is not allowed" {
t.Fatalf("unexpected error: %v", err)
}
}
}
repo *git.Repository
repoPath string
// receivePackHook synthesizes the receive-pack response when set,
// bypassing the embedded ReceivePack handler. Used to simulate
// per-ref ng statuses from hostile targets.
receivePackHook func(*packp.UpdateRequests) *packp.ReportStatus
mu sync.Mutex
receivePacks int
thinCapable bool
125 unmodified lines
}
}
s.receivePacks++
s.mu.Unlock()
if s.receivePackHook != nil {
body, err := io.ReadAll(r.Body)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
req := packp.NewUpdateRequests()
if err := req.Decode(bytes.NewReader(body)); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
report := s.receivePackHook(req)
// Wrap the report in sideband framing when negotiated, mirroring
// what transport.ReceivePack writes; the client's demuxer otherwise
// fails on raw report-status pkt-lines.
var buf bytes.Buffer
var writer io.Writer = &buf
useSideband := false
switch {
case req.Capabilities.Supports(capability.Sideband64k):
writer = sideband.NewMuxer(sideband.Sideband64k, &buf)
useSideband = true
case req.Capabilities.Supports(capability.Sideband):
writer = sideband.NewMuxer(sideband.Sideband, &buf)
useSideband = true
}
if err := report.Encode(nopWriteCloser{writer}); err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
if useSideband {
_ = pktline.WriteFlush(&buf)
}
w.Header().Set("Content-Type", "application/x-git-receive-pack-result")
if _, err := w.Write(buf.Bytes()); err != nil {
s.t.Fatalf("write receive-pack hook response: %v", err)
}
return
}
var buf bytes.Buffer
wc := nopWriteCloser{&buf}
Mcmd/git-sync/main_test.go+138
110 unmodified lines
111
112
113
114
114
115
116
117
118
119
120
121
122
123
124
125
126
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")
allRefsFlag(cmd, &req.Scope.AllRefs, &req.Policy.BestEffort)
// Replicate keeps strict failure semantics — its contract is "target
// refs match source." BestEffort would let partial mirrors exit success.
implies := []*bool{&req.Policy.IncludeTags}
usage := allRefsUsageBestEffort
if defaultMode == gitsync.ModeReplicate {
usage = allRefsUsageStrict
} else {
implies = append(implies, &req.Policy.BestEffort)
}
allRefsFlag(cmd, usage, &req.Scope.AllRefs, implies...)
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/syncplan.go+10/-1
165 unmodified lines
166
167
168
169
170
171
172
173
174
169
170
171
172
173
174
175
176
176
177
178
177
178
179
180
181
182
183
184
185
186
187
188
165 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). It 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.
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.
Library callers can decouple the two halves: `RefScope.AllRefs` controls scope alone, and `SyncPolicy.BestEffort` controls failure handling. The CLI bundles them under `--all-refs` for convenience.
`replicate --all-refs` broadens the same scope but does NOT enable best-effort. Replicate's contract is "target refs match source"; downgrading rejected refs to warnings would let partial mirrors exit successfully, 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.
Force source-side protocol v2:
Mdocs/usage.md+16/-9
3049 unmodified lines
3050
3051
3052
3053
3054
3055
3056
3057
3058
3059
3060
3061
3062
3063
3064
3065
3066
3067
3068
3069
3070
3071
3072
3073
3074
3075
3076
3077
3078
3079
3080
3081
3082
3083
3084
3085
3086
3087
3088
3089
3090
3091
3092
3093
3094
3095
3096
3097
3098
3099
3100
3101
3102
3103
3104
3105
3049 unmodified lines
assertHeadsMatch(t, sourceRepo, targetRepo, testBranch)
}
// Replicate's bootstrap shortcut must not fire when --prune --all-refs has
// stale other-kind refs to delete on target; otherwise replicate would
// claim "target matches source" while leaving orphaned refs/notes/* behind.
func TestRun_IntegrationReplicateAllRefsPruneSkipsBootstrapForStaleOtherRef(t *testing.T) {
sourceRepo, sourceFS := newSourceRepo(t)
makeCommits(t, sourceRepo, sourceFS, 1)
targetRepo, err := git.Init(memory.NewStorage())
if err != nil {
t.Fatalf("init target repo: %v", err)
}
// Target has an orphaned notes ref that doesn't exist on source.
staleHead, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true)
if err != nil {
t.Fatalf("resolve source head: %v", err)
}
if err := copyRefsAndObjects(sourceRepo.Storer, targetRepo.Storer, []plumbing.ReferenceName{plumbing.NewBranchReferenceName(testBranch)}); err != nil {
t.Fatalf("copy target baseline: %v", err)
}
staleNotes := plumbing.ReferenceName("refs/notes/stale")
if err := targetRepo.Storer.SetReference(plumbing.NewHashReference(staleNotes, staleHead.Hash())); err != nil {
t.Fatalf("set stale notes ref on target: %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: protocolModeAuto,
Mode: modeReplicate,
AllRefs: true,
Prune: true,
})
if err != nil {
t.Fatalf("replicate --all-refs --prune failed: %v", err)
}
if result.RelayMode == "bootstrap" {
t.Fatalf("expected replicate to take prune path, not bootstrap; got RelayMode=%q", result.RelayMode)
}
if _, err := targetRepo.Reference(staleNotes, true); err == nil {
t.Fatalf("expected stale %s to be pruned from target", staleNotes)
} else if !errors.Is(err, plumbing.ErrReferenceNotFound) {
t.Fatalf("unexpected error resolving stale ref: %v", err)
}
}
// Pins a v1 limitation: replicate is relay-only, so other-kind refs into a
// non-empty target error out (sync handles it via materialized fallback).
func TestRun_IntegrationAllRefsReplicateRejectsOtherKindIntoExistingTarget(t *testing.T) {
Minternal/syncer/integration_test.go+50
840 unmodified lines
841
842
843
844
845
846
847
844
845
846
847
848
849
850
851
852
31 unmodified lines
884
885
886
887
888
889
890
891
840 unmodified lines
}
}
if !s.cfg.DryRun && len(relayPlans) > 0 {
ok, reason := planner.CanReplicateRelay(relayPlans)
if !ok {
return result, fmt.Errorf("replicate requires relay-capable target: %s; use sync instead", reason)
}
}
repResult, err := s.executeReplicate(ctx, desiredRefs, pushPlans)
if err != nil {
return false
// Additional check for branch and mappings
}
// Adding to allRefs flag check
case targetRef.IsBranch() && len(s.cfg.Mappings) == 0 && len(s.cfg.Branches) == 0:
return false
case s.cfg.AllRefs && planner.RefKindFromName(targetRef) == planner.RefKindOther && len(s.cfg.Mappings) == 0:
return false
}
}
return true
Minternal/syncer/syncer.go+8/-4
// Additional helper functions for reference handling
func (s *syncer) handleRefs() error {
}