Simplify after review: dedupe, trim comments, flatten conditionals · Entire
Simplify after review: dedupe, trim comments, flatten conditionals
720b519→main·
Soph·2mo ago·12 files·+99 added/-188 removed
- Extract addPruneCandidates helper; the prune-managed switch is no longer pasted into both BuildPlans and BuildReplicationPlans.
- Extract syncSession.finalizeCounts; the applyRejections + counter loop block is no longer duplicated between runSync and runReplicate.
- Use convert.DesiredRefsForPlans in the bootstrap tail phase instead of building gitproto.DesiredRef inline.
- Drop the defensive pushed < 0 clamp in bootstrapWithInputs; if Pushed and Warned ever disagreed, masking it would hide the bug.
- Fold the "AllRefs implies BestEffort" coupling into allRefsFlag via cobra PreRunE instead of pasting the if-statement into two RunE bodies.
- Flatten the 3-level nested conditional in NormalizeMapping.
- Trim verbose doc blocks: RefKind, ActionWarn, RefScope.AllRefs, SyncPolicy.BestEffort, Pusher.OnRejection, syncSession.rejections, the bootstrap tail-phase comment, and the test docstrings that narrated motivation rather than the assertion. Removes the two empty-branch comments in the count switches by collapsing ActionWarn/Skip/Block into one no-op branch.
Net: -89 lines, all 8 AllRefs integration + smoke tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Sessions
f8bbc26df8b3View transcript
Changes
12
cmd/git-sync
Mbootstrap.go+1/-4
Mflags.go+17/-5
Mmain_test.go+2/-6
Msyncplan.go+1/-4
internal
gitproto
Mpush.go+3/-13
planner
Mplanner.go+23/-26
Mtypes.go+3/-10
strategy/bootstrap
Mbootstrap.go+3/-11
syncer
Mintegration_test.go+8/-31
Msyncer.go+27/-50
validation
Mvalidation.go+7/-13
Mtypes.go+4/-15
28 unmodified lines
29
30
31
32
33
34
32
33
34
39 unmodified lines
74
75
76
80
77
78
79
80
28 unmodified lines
SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { req.Protocol = gitsync.ProtocolMode(protocolVal) if req.Scope.AllRefs { req.BestEffort = true }
if req.Source.URL == "" && len(args) > 0 { req.Source.URL = args[0] 39 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) allRefsFlag(cmd, &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/-4
45 unmodified lines
46
47
48
49
50
51
52
53
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
45 unmodified lines
cmd.Flags().Var(mode, "protocol", "protocol mode: auto, v1, or v2")
}
// allRefsFlag registers --all-refs. The CLI semantic is "mirror everything"
// from source on a best-effort basis — callers must propagate AllRefs to
// BestEffort inside RunE after flag parsing (cobra has no on-set hook), so
// per-ref rejections become warnings on top of the broader scope.
func allRefsFlag(cmd *cobra.Command, allRefs *bool) {
// 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 {
return
}
prev := cmd.PreRunE
cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
if *allRefs {
*bestEffort = true
}
if prev != nil {
return prev(cmd, args)
}
return nil
}
}
func newProtocolFlag() protocolModeFlag {
Mcmd/git-sync/flags.go+17/-5
253 unmodified lines
254
255
256
257
258
259
260
261
262
257
258
259
260
261
253 unmodified lines
}
// TestRun_Sync_AllRefsSmokeTest exercises the full CLI pipeline with
// --all-refs: cobra flag parsing → unstable client → bridge → syncer →
// receive-pack. Confirms a custom-namespace ref (refs/notes/commits) on
// the source ends up on an empty target and shows up in the JSON output
// with the right shape, so future refactors that break the wiring don't
// pass tests until they reach the integration suite.
// CLI smoke test for --all-refs: covers cobra flag parsing through the full
// sync pipeline so wiring breaks fail at the cmd layer, not just integration.
func TestRun_Sync_AllRefsSmokeTest(t *testing.T) {
sourceRepo, sourceFS := newSourceRepo(t)
makeCommits(t, sourceRepo, sourceFS, 2)
Mcmd/git-sync/main_test.go+2/-6
42 unmodified lines
43
44
45
46
47
48
46
47
48
62 unmodified lines
111
112
113
117
114
115
116
117
42 unmodified lines
RunE: func(cmd *cobra.Command, args []string) error {
req.Policy.Mode = gitsync.OperationMode(modeValue)
req.Policy.Protocol = gitsync.ProtocolMode(protocolVal)
if req.Scope.AllRefs {
req.Policy.BestEffort = true
}
if req.Source.URL == "" && len(args) > 0 {
req.Source.URL = args[0]
62 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)
allRefsFlag(cmd, &req.Scope.AllRefs, &req.Policy.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/syncplan.go+1/-4
25 unmodified lines
26
27
28
29
30
31
32
33
34
29
30
31
32
33
66 unmodified lines
100
101
102
107
108
109
110
103
104
105
106
41 unmodified lines
148
149
150
158
159
160
151
152
153
25 unmodified lines
// Pusher wraps target-side receive-pack state behind a smaller execution API.
// OnRejection, when non-nil, is invoked with each per-ref rejection reported // by the target's receive-pack instead of returning a fatal error. The pack // itself unpacking is still fatal; only individual ref ng statuses are // downgraded to callbacks. This is how best-effort all-refs mode keeps a // sync going past hidden-ref refusals (refs/pull/* on GitHub, etc.). // When OnRejection is non-nil, per-ref ng statuses invoke it instead of erroring; // pack-level unpack failure remains fatal.
type Pusher struct { Conn *Conn Adv *packp.AdvRefs 66 unmodified lines
return req, hasDelete, hasUpdates, nil }
// sendReceivePack encodes and POSTs a receive-pack request, then decodes the // report. When onRejection is non-nil, per-ref ng statuses are reported via // the callback instead of erroring; the entire push only fails on transport // errors or unpack failure. // sendReceivePack encodes and POSTs a receive-pack request, then decodes the report. func sendReceivePack( ctx context.Context, conn *Conn, 41 unmodified lines
} return nil } // Best-effort: unpack failure is still fatal (the whole pack went // nowhere, but per-ref ng statuses go to the callback so the // caller can downgrade them to warnings. if report.UnpackStatus != "" && report.UnpackStatus != "ok" { return fmt.Errorf("report-status: unpack error: %s", report.UnpackStatus) }
Minternal/gitproto/push.go+3/-13
115 unmodified lines
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
119
120
121
122
60 unmodified lines
183
184
185
198
199
200
201
202
203
204
205
206
207
208
209
210
186
187
188
189
31 unmodified lines
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
115 unmodified lines
cfg PlanConfig, ) ([]BranchPlan, error) { if cfg.Prune { for targetRef := range targetRefs { if _, ok := managed[targetRef]; ok { continue } switch { case targetRef.IsTag() && cfg.IncludeTags: 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()} } } addPruneCandidates(managed, targetRefs, cfg) }
targetNames := make([]plumbing.ReferenceName, 0, len(managed)) 60 unmodified lines
) ([]BranchPlan, error) { managed = copyManagedTargets(managed) if cfg.Prune { for targetRef := range targetRefs { if _, ok := managed[targetRef]; ok { continue } switch { case targetRef.IsTag() && cfg.IncludeTags: 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()} } } addPruneCandidates(managed, targetRefs, cfg) }
targetNames := make([]plumbing.ReferenceName, 0, len(managed)) 31 unmodified lines
return plans, nil }
// addPruneCandidates registers unmanaged target refs as deletion candidates
// when they fall in a namespace the user is currently mirroring. The branch
// guard len(cfg.Mappings) == 0 && len(cfg.Branches) == 0 keeps a narrow
// branch-filter or mapping run from pruning branches outside its scope; tags
// and other-kind only enter scope when their respective opt-in flag is set.
func addPruneCandidates(managed map[plumbing.ReferenceName]ManagedTarget, targetRefs map[plumbing.ReferenceName]plumbing.Hash, cfg PlanConfig) {
for targetRef := range targetRefs {
if _, ok := managed[targetRef]; ok {
continue
}
switch {
case targetRef.IsTag() && cfg.IncludeTags:
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()}
}
}
}
func copyManagedTargets(input map[plumbing.ReferenceName]ManagedTarget) map[plumbing.ReferenceName]ManagedTarget { out := make(map[plumbing.ReferenceName]ManagedTarget, len(input)) for k, v := range input {
Minternal/planner/planner.go+23/-26
9 unmodified lines
10
11
12
13
14
15
16
17
13
14
15
16
11 unmodified lines
28
29
30
35
36
37
38
39
31
32
33
34
35
9 unmodified lines
"github.com/go-git/go-git/v6/plumbing"
// RefKind distinguishes ref namespaces. Branch and tag refs have specific // semantics (fast-forward checks, retarget rules); RefKindOther covers any // other refs/* namespace (notes, pulls, replace, custom) the user opts into // via the AllRefs scope. Other refs follow the same fast-forward / force // semantics as branches but can live in arbitrary ref namespaces. // RefKind distinguishes ref namespaces: branch, tag, or other (notes/pulls/custom). type RefKind string
const (
ActionDelete Action = "delete" ActionSkip Action = "skip" ActionBlock Action = "block" // ActionWarn is set after a push when the target rejected an // individual ref update under best-effort policy. The push itself // succeeded for other refs; this ref carries the server's reason in // BranchPlan.Reason. Used so AllRefs syncs into hostile targets // (e.g. GitHub refs/pull/* hidden refs) don't fail the whole run. // ActionWarn is set when the target rejected an individual ref under // best-effort policy; the server's reason is carried in BranchPlan.Reason. ActionWarn Action = "warn" )
Minternal/planner/types.go+3/-10
224 unmodified lines
225
226
227
228
229
230
229
230
231
232
231
234
232
233
234
238
239
240
241
242
243
235
236
237
1 unmodified line
239
240
241
242
243
244
245
224 unmodified lines
\treturn result, errors.New("bootstrap batching requires protocol v2 source fetch filter support") }\n // Tags and other-kind refs are create-only and ride a single tail phase // after the checkpointed branch batches; they reuse branch-tip haves. planRefs := make([]planner.DesiredRef, 0, len(plans)) // Tags and other-kind refs (notes, pulls, custom namespaces) are pushed // in a single create-only phase after the branch batches finish — they // don't need checkpointing because they're create-only and reuse the // branch-tip haves already on the target. tailPlans := make([]planner.BranchPlan, 0, len(plans)) tailDesired := make(map[plumbing.ReferenceName]gitproto.DesiredRef) for _, plan := range plans { if plan.Kind == planner.RefKindTag || plan.Kind == planner.RefKindOther { tailPlans = append(tailPlans, plan) if d, ok := p.DesiredRefs[plan.TargetRef]; ok { tailDesired[plan.TargetRef] = gitproto.DesiredRef{ SourceRef: d.SourceRef, TargetRef: d.TargetRef, SourceHash: d.SourceHash, IsTag: plan.Kind == planner.RefKindTag, } continue } if !plan.SourceRef.IsBranch() || !plan.TargetRef.IsBranch() {