Simplify: remove pass-through layers, dead code, and copy-pasted helpers#99
Merged
Conversation
internal/internalbridge was a pure pass-through layer: it mirrored the public request types field-for-field, wrapped syncer.Config in an opaque struct, and forwarded Probe/Run calls verbatim. The root client had to convert every request through bridge mirror types, and unstable/client.go round-tripped through them just to build a syncer.Endpoint literal. Define the public result types (RefResult, Stats, ProbeResult, SyncResult, ...) directly in the root package instead of aliasing internal types -- this also makes their godoc visible on pkg.go.dev and exports SideBytes, which was previously reachable through Stats.Sides but not nameable by callers. Build syncer.Config directly in the client, matching what unstable/client.go already does. Field sets and JSON tags are unchanged, so the published API surface is identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: f27edbb5033d
Remove code with no production callers, verified by repo-wide search: - planner: DesiredSubset, SingleDesired, SingleHaveMap, and FirstParentChainFromMap (superseded by FirstParentChainFromParents) had no references at all. - gitproto: PushPlan/ToPushCommands and convert.PlansToPushPlans formed a parallel plan-conversion path; every strategy uses convert.PlansToPushCommands directly. The TestPlansToPushPlans copies in incremental and materialized were identical and tested only this dead path. - gitproto: FetchCommitGraph was superseded by FetchCommitParents (same wire protocol, lower memory); its capability-guard tests now cover FetchCommitParents, which had no direct tests. - gitproto: progressWriter (superseded by progressSink) and HashHex were referenced only from tests. - gitproto: FetchFeatures existed to carry one bit consumed by SupportsBootstrapBatch plus a write-only IncludeTag field; inline the filter check. - sha256convert: normalizeAuth/authAdapter converted between two structurally identical interfaces (auth.Method and gitproto.AuthMethod); the value is directly assignable, and the adapter branch was unreachable since auth.Resolve only returns the two concrete types the switch already passed through. - syncer: newProbeResult assigned Stats/Measurement twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 9c1282a34a36
- closeOnceReadCloser existed verbatim in the bootstrap, incremental, and replicate strategies; move it to gitproto as CloseOnce next to LimitPackReader (all strategies already depend on gitproto) and add the unit test it never had. - The byte formatter existed three times: syncer.formatBytes and bootstrap.humanBytes were identical, and gitproto.humanizeBytes was a slightly cruder variant (fixed one-decimal output, capped at GB). Keep the precision-tiered implementation as gitproto.HumanBytes and use it everywhere; pack-encode progress now formats like the rest of the progress output. - runSync and runReplicate carried the same plan-classification switch; extract classifyPlans. Replicate derives its relay subset from the returned push plans. - auth.Method duplicated gitproto.AuthMethod (identical single-method interfaces); alias it so the contract is declared once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 96d8a7027ab3
- convert-sha256 re-registered the four source-auth flags by hand; reuse addSourceAuth, which also carries the secret-leak protection for token flags. - sync/plan/replicate and bootstrap had identical --map parse loops; extract parseMappings in flags.go. - Collapse the protocolMode/operationMode intermediate types: the flag types now derive directly from the public gitsync types, removing triple conversions like protocolModeFlag(protocolMode(gitsync.ProtocolMode(mode))). - The version line format existed in root.go and version.go; move it to versioninfo.String(). - bench: uniqueStrings hand-rolled slices.Compact. - sha256convert: the resolveCacheEntry type had been inserted between resolveMessageRef and its doc comment, silently reattaching the comment to the wrong declaration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 6a45cb3266a7
syncer.newSession already normalizes an empty protocol mode to auto and an empty operation mode to sync (and rejects unknown values), so the protocolString/operationModeString helpers in the stable and unstable clients were a third copy of the same defaulting. Pass the request values through unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 2c0d78a8a805
- parseMappings returned a non-nil empty slice when no --map was given, where the old inline loops left Scope.Mappings nil; preserve nil so the request shape is unchanged. - gitproto/convert.go no longer contained conversions after the dead PushPlan path was removed; rename it to readers.go to match its content (CloseOnce, LimitPackReader). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 1dd678f9643b
nodo
approved these changes
Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A de-slop pass over the whole repo: −840 net lines with no feature or public-API change. Each commit is standalone and could be split into its own PR for review.
internal/internalbridgewas a pure pass-through layer: it mirrored the public request types field-for-field, wrappedsyncer.Configin an opaque struct, and forwardedProbe/Runverbatim. The public result types are now defined directly in the root package (same fields, same JSON tags), which also makes their godoc visible on pkg.go.dev and exportsSideBytes(previously reachable throughStats.Sidesbut not nameable by callers).PushPlan/ToPushCommandsconversion path,FetchCommitGraph(superseded byFetchCommitParents, whose capability guards are now tested directly),progressWriter,HashHex, theFetchFeaturesstruct, and the unreachableauthAdapterin convert-sha256.closeOnceReadCloserexisted verbatim in three strategy packages (nowgitproto.CloseOnce, with the unit test it never had); the byte formatter existed three times (nowgitproto.HumanBytes); the plan-classification switch existed twice in the syncer (nowclassifyPlans);auth.Methodduplicatedgitproto.AuthMethod.addSourceAuth(including its secret-leak protection), sharedparseMappings, collapsed theprotocolModeFlagtype ladder,versioninfo.String(),slices.Compact.syncer.newSessionalready normalizes empty protocol → auto and empty mode → sync; both clients repeated that defaulting.Review
Multi-angle review (line-by-line, removed-behavior audit, cross-file trace, reuse/altitude) ran on the full diff; correctness angles came back clean and the two actionable notes were fixed in the last commit. Two trade-offs were considered and accepted:
syncerEndpoint/validationMappingsare now duplicated between the stable and unstable clients (~24 lines) — the only shared home would be a bridge-style package, i.e. the indirection this PR removes.HumanByteslives in gitproto (the one package all consumers, including gitproto itself, already import); the pack-encode ticker now formats like the rest of the progress output.Testing
go test -race ./...green (CI parity)golangci-lint run— 0 issues;gofmt -scleanversion,--version,--helpfor sync/convert-sha256,--modevalidation error path🤖 Generated with Claude Code
Note
Medium Risk
Large refactor across sync orchestration, gitproto fetch/push, and strategy relay paths; behavior should be unchanged but the blast radius warrants careful review and existing test coverage.
Overview
This PR is a de-slop refactor (~840 net lines removed) with no intended feature or public API behavior change.
Public client path:
internal/internalbridgeis deleted. The rootgitsyncclient now buildssyncer.Configdirectly and mapssyncerresults viafromProbeResult/fromSyncResultinresults.go. Result types (RefResult,ProbeResult,SyncResult, etc.) live in the root package instead of being re-exported through the bridge. Theunstableclient follows the same pattern (directsyncer.Endpointconstruction, no bridge).Shared internals: Pack reader helpers move to
gitproto(CloseOnce,LimitPackReader, exportedHumanBytes). Relay strategies and sync progress/batching use those instead of local copies.auth.Methodis aliased togitproto.AuthMethodso convert-sha256 no longer needs anauthAdapter. Sync/replicate planning share newclassifyPlansin the syncer.Removed dead paths: Unused planner/gitproto/convert helpers (
PushPlan/ToPushCommands,FetchCommitGraph,FetchFeatures,HashHex,progressWriter, etc.) and duplicate tests.CLI: Shared
parseMappings,addSourceAuthon convert-sha256,versioninfo.String()for version output, simpler protocol/mode flag types,slices.Compactin git-sync-bench.Reviewed by Cursor Bugbot for commit d715649. Configure here.