plumbing/transport API Refactoring#1983
Conversation
Replace the alpha plumbing/transport API with the new x/transport API. Copy all core files from x/transport/ to plumbing/transport/ with import path rewrites. Delete old files that have no equivalent in the new API (registry.go, mocks.go, pack.go). See rfcs/0002-replace-plumbing-transport.md for design rationale.
Replace the old upload_pack.go and receive_pack.go test suites in internal/transport/test/ with the new versions from x/transport/test/. These new suites test against the new plumbing/transport API.
Replace the old plumbing/transport/ssh package with the new version from x/transport/ssh. This includes the new auth, handshake, knownhosts, and sshagent implementations.
Replace the old plumbing/transport/http package with the new version from x/transport/http. This includes new auth, handshake, dumb HTTP, TLS, redirect, and proxy implementations.
Replace the old plumbing/transport/git package with the new version from x/transport/git. Includes new handshake implementation.
Replace the old plumbing/transport/file package with the new version from x/transport/file. Includes new handshake implementation.
Add plumbing/client from x/client. This package provides scheme-based transport resolution, replacing the old registry.go functionality.
Replace old transport API usage in remote.go, repository.go, submodule.go, and options.go with the new plumbing/transport and plumbing/client APIs. Key changes: - Replace Auth, InsecureSkipTLS, CABundle, ProxyOptions fields with ClientOptions []client.Option on all option structs - Replace newClient() to use client.New() + transport.ParseURL() - Collapse Transport.NewSession + Session.Handshake into client.Handshake - Pass storer to Session.Fetch/Push (new signature) - Replace transport.NewEndpoint with transport.ParseURL (*url.URL)
Update server and backend packages for the new plumbing/transport API: - Replace transport.Service type with plain string - Replace transport.NewEndpoint with transport.ParseURL (*url.URL) - Replace svc.Name()/service.Name() with transport.ServiceName() - Update Loader.Load signature from *Endpoint to *url.URL
Update all examples that use authentication or custom HTTP clients to use the new client.Option pattern instead of the old Auth field and transport.Register.
Fix all remaining build, test, and lint errors after the transport API migration so the full stack compiles, passes go vet, and all tests pass. Highlights: - Remove NewConn helper; each transport now implements transport.Conn directly (sshConn, gitConn, fileConn) - Surface remote stderr at Push/Fetch via Stderr() io.Reader interface instead of at Close() time — the idiomatic Go pattern for subprocess error reporting - Replace UploadPackOptions/ReceivePackOptions with UploadPackRequest/ ReceivePackRequest in backend packages - Fix missing closing paren and stale variable references in remote.go - Update worktree.go to use ClientOptions instead of old fields - Consolidate duplicate internal/transport/test imports across all sub-transport test files - Refactor duplicated test code into table-driven tests and shared setup helpers (file, git, ssh) - Fix TestHostKeyCallbackHelper_NilFallback to account for /etc/ssh/ssh_known_hosts - Fix gci import ordering across all modified files Verified: go build, go vet, go test ./..., and example tests all pass.
The go.mod requires v6 but test files still imported v5.
Merge backend/git and backend/http into a single backend package with a unified API: - Backend.Serve(ctx, r, w, *Request) — core dispatch for all transports - Backend.ServeConn(ctx, rwc, *Request) — convenience for full-duplex connections (TCP, SSH, pipes) - Backend.ServeHTTP(w, r) — HTTP adapter (smart + dumb protocols) - RequestFromProto(*packp.GitProtoRequest) — helper for TCP git daemon The shared logic (loader resolution, service dispatch, UploadPack/ ReceivePack invocation) lives in Serve. HTTP-specific concerns (routing, gzip, content-type, dumb file serving) stay in ServeHTTP. Error handling is the caller's responsibility — Serve returns errors instead of writing them to the wire. Delete backend/git/ and backend/http/ subpackages. Update internal/server/http to import backend directly.
Co-authored-by: Paulo Gomes <[email protected]> Co-authored-by: Ayman Bagabas <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
plumbing: transport, replace transport API with new design
There was a problem hiding this comment.
Pull request overview
This PR completes a breaking refactor of the plumbing/transport API, introducing a scheme-resolving plumbing/client and consolidating server backends into a unified backend package, while rewiring repository operations and examples to the new client/transport option model.
Changes:
- Replace the old transport registry/endpoint/auth model with
*url.URL+transport.Request+Transport/Session/Conninterfaces and helpers. - Add
plumbing/clientto resolve URL schemes and apply transport-specific options (auth/TLS/proxy/dialer). - Update fetch/push/clone/pull/submodule flows, transports (ssh/http/git/file), backends, and tests/examples to use
ClientOptions.
Reviewed changes
Copilot reviewed 103 out of 103 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| worktree.go | Propagate ClientOptions through pull/submodule update paths |
| submodule.go | Switch to transport.ParseURL and pass ClientOptions into fetches |
| repository.go | Use ClientOptions during clone/fetch and submodule recursion |
| remote.go | Replace old registry-based client/session handshake with client.Client + transport.Request |
| plumbing/transport/x_test.go | Add basic unit tests for Conn, Request, and GitProtocolEnv |
| plumbing/transport/url.go | Add ParseURL helper that normalizes SCP/file/standard URLs |
| plumbing/transport/url_test.go | Rename endpoint tests to ParseURL and update fuzz target |
| plumbing/transport/upload_pack.go | Rename options struct to request struct; update advertise helper call |
| plumbing/transport/upload_pack_test.go | Rename/extend serve suites; add receive-pack serve suite |
| plumbing/transport/transport.go | Replace legacy transport API with Conn + Connector definitions |
| plumbing/transport/ssh/upload_pack_test.go | Remove old SSH upload-pack test suite tied to deprecated API |
| plumbing/transport/ssh/ssh.go | New SSH transport implementation for the redesigned API |
| plumbing/transport/ssh/ssh_test.go | Add SSH Connect tests for new request model |
| plumbing/transport/ssh/proxy_test.go | Rewrite SOCKS5 proxy tests for new dial-proxy design |
| plumbing/transport/ssh/pack_test.go | Reintroduce SSH pack protocol suites using new transport/session API |
| plumbing/transport/ssh/handshake.go | Implement Transport.Handshake via Connect + NewStreamSession |
| plumbing/transport/ssh/common.go | Remove legacy SSH transport implementation (globals/registry-based) |
| plumbing/transport/ssh/common_test.go | Remove legacy SSH tests tied to deprecated globals/registry |
| plumbing/transport/ssh/auth_test.go | Add unit tests for SSH auth implementations under new API |
| plumbing/transport/ssh/auth_extended_test.go | Add extended SSH auth/config/known_hosts coverage |
| plumbing/transport/service.go | Replace Service type with string constants + ServiceName helper |
| plumbing/transport/serverinfo_test.go | Expand coverage and tidy file handling in server-info tests |
| plumbing/transport/serve.go | Rename advertise helper and switch service param to string |
| plumbing/transport/serve_test.go | Update generic serve/advertise helpers to request struct types |
| plumbing/transport/request.go | Introduce transport.Request for transport-neutral command opening |
| plumbing/transport/registry.go | Remove global transport registry |
| plumbing/transport/registry_test.go | Remove registry tests |
| plumbing/transport/receive_pack.go | Rename options struct to request struct; update advertise helper call |
| plumbing/transport/receive_pack_test.go | Remove old receive-pack suite (moved/merged into serve tests) |
| plumbing/transport/push.go | Refactor SendPack to take capabilities directly and keep helper local |
| plumbing/transport/protocol.go | Add GitProtocolEnv helper for GIT_PROTOCOL formatting |
| plumbing/transport/pack.go | Remove old pack session/connection implementation |
| plumbing/transport/pack_test.go | Remove old pack session tests tied to removed types |
| plumbing/transport/pack_stream.go | Add StreamSession implementation over Conn |
| plumbing/transport/pack_session.go | Define new Transport/Session interfaces for pack protocol |
| plumbing/transport/negotiate.go | Update negotiation to operate on caps + stateless flag (no Connection) |
| plumbing/transport/negotiate_test.go | Update negotiation tests and add a simplified write-closer mock |
| plumbing/transport/mocks.go | Remove legacy mock commander/command tied to removed API |
| plumbing/transport/loader.go | Update loader interface to accept *url.URL and adjust MapLoader keying |
| plumbing/transport/loader_test.go | Replace suite with direct tests for new Loader signatures |
| plumbing/transport/http/upload_pack_test.go | Rewire HTTP upload-pack suite to new transport/session API |
| plumbing/transport/http/transport.go | Remove legacy HTTP transport caching/options layer |
| plumbing/transport/http/tls_test.go | Add tests for TLS/client resolution behavior in new HTTP transport |
| plumbing/transport/http/redirect_test.go | Add redirect regression tests, including fetch-after-redirect |
| plumbing/transport/http/receive_pack_test.go | Rewire HTTP receive-pack suite to new transport/session API |
| plumbing/transport/http/http.go | New HTTP transport options + client resolution |
| plumbing/transport/http/http_test.go | Add smart HTTP server test helper + endpoint construction |
| plumbing/transport/http/dumb_test.go | Rework dumb HTTP tests and server scaffolding for new API |
| plumbing/transport/http/auth.go | Add HTTP auth helpers (basic + bearer token) for new option model |
| plumbing/transport/http/auth_test.go | Add unit tests for HTTP auth helpers |
| plumbing/transport/git/upload_pack_test.go | Remove old git:// upload-pack suite tied to deprecated transport API |
| plumbing/transport/git/receive_pack_test.go | Remove old git:// receive-pack suite tied to deprecated transport API |
| plumbing/transport/git/pack_test.go | Add new git:// pack protocol suites using new transport/session API |
| plumbing/transport/git/handshake.go | Implement Transport.Handshake via Connect + NewStreamSession |
| plumbing/transport/git/git.go | New git:// TCP transport implementing Connector over net.Conn |
| plumbing/transport/git/git_test.go | Add git:// Connect tests and daemon harness for new API |
| plumbing/transport/git/common.go | Remove legacy git:// transport (registry-based) |
| plumbing/transport/git/common_test.go | Remove legacy git:// tests tied to deprecated implementation |
| plumbing/transport/file/upload_pack_test.go | Remove legacy file transport suite tied to deprecated API |
| plumbing/transport/file/receive_pack_test.go | Remove legacy file transport suite tied to deprecated API |
| plumbing/transport/file/pack_test.go | Add new file:// pack protocol suites using new transport/session API |
| plumbing/transport/file/handshake.go | Implement Transport.Handshake via Connect + NewStreamSession |
| plumbing/transport/file/file.go | New in-process file transport using loader + Upload/ReceivePack handlers |
| plumbing/transport/file/file_test.go | Add unit tests for file transport connect/unsupported command behavior |
| plumbing/transport/file/file_integration_test.go | Add integration tests for file transport over in-process pipes |
| plumbing/transport/file/client.go | Remove legacy file transport implementation (registry-based) |
| plumbing/transport/file/client_test.go | Remove legacy file transport tests |
| plumbing/transport/fetch.go | Refactor FetchPack to take capabilities directly (no Connection) |
| plumbing/transport/errors.go | Centralize transport/negotiation errors in a dedicated file |
| plumbing/transport/doc.go | Document redesigned transport API concepts and roles |
| plumbing/transport/common.go | Add DialContextFunc and remove legacy transport/session/command types |
| plumbing/transport/common_test.go | Remove legacy tests tied to removed pack transport implementation |
| plumbing/transport/build_update_requests_test.go | Update tests to match refactored push capability handling |
| plumbing/client/client.go | Add scheme-resolving client with transport-specific option helpers |
| options.go | Replace Auth/TLS/proxy fields with unified ClientOptions []client.Option |
| internal/url/url.go | Introduce canonical URL parsing helpers used by the new transport API |
| internal/server/loader.go | Update test loader to satisfy new transport.Loader signature |
| internal/server/http/http.go | Switch internal server to unified backend package |
| example_test.go | Update examples to use ClientOptions for HTTP auth configuration |
| backend/writer.go | Move writer helper into backend package and adjust logging usage |
| backend/http_test.go | Update backend tests for new loader signature and package layout |
| backend/git/git.go | Remove legacy git backend package (superseded by unified backend) |
| backend/backend.go | Add unified backend entrypoint and proto-to-request conversion helper |
| _examples/tag-create-push/main.go | Update example to use ClientOptions with SSH auth |
| _examples/custom_http/main.go | Update example to use WithHTTPClient instead of transport registry |
| _examples/clone/auth/ssh/ssh_agent/main.go | Update example to use ClientOptions with SSH agent auth |
| _examples/clone/auth/ssh/private_key/main.go | Update example to use ClientOptions with SSH key auth |
| _examples/clone/auth/basic/username_password/main.go | Update example to use ClientOptions with HTTP basic auth |
| _examples/clone/auth/basic/access_token/main.go | Update example to use ClientOptions with HTTP auth for tokens |
Comments suppressed due to low confidence (2)
backend/writer.go:36
f.logcan be nil (it is set fromBackend.ErrorLog), so callingf.log.Printf(...)can panic during error handling. Either guard these calls (e.g.,if f.log != nil { ... }) or route logging throughBackend.logfconsistently.
backend/writer.go:40- When
nr != nw,erris nil here, soReadFromwould return a nil error on a short write. Returnio.ErrShortWrite(or a wrapped error) in this branch so callers can detect partial writes reliably.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *ReceivePackServeSuite) TestReceivePackAdvertiseV2() { | ||
| testAdvertise(s.T(), UploadPack, "version=2", false) | ||
| } |
There was a problem hiding this comment.
TestReceivePackAdvertiseV2 is calling testAdvertise with UploadPack instead of ReceivePack, so it isn't exercising the receive-pack advertisement path (and could mask regressions specific to receive-pack). Update the test to pass ReceivePack here.
| for _, tc := range []struct { | ||
| name string | ||
| command string | ||
| }{ | ||
| {"UploadPack", "git-upload-pack"}, | ||
| {"ReceivePack", "git-upload-pack"}, | ||
| } { |
There was a problem hiding this comment.
In TestGitTransport_Connect, the "ReceivePack" test case is using git-upload-pack as the command, so the receive-pack connect path is never tested. Change the command for that case to git-receive-pack.
| func (r *testProxyRule) Allow(_ context.Context, _ *socks5.Request) (context.Context, bool) { | ||
| *r.proxiedRequests++ | ||
| return context.Background(), true |
There was a problem hiding this comment.
This increment is a compile error in Go (++ is a statement, not an expression). Use an increment statement on the dereferenced pointer (e.g., (*r.proxiedRequests)++) or store the counter as an atomic.Int32/int64 and increment it safely.
| func WithCABundle(pem []byte) Option { | ||
| return func(o *options) { | ||
| pool := x509.NewCertPool() | ||
| pool.AppendCertsFromPEM(pem) |
There was a problem hiding this comment.
WithCABundle ignores the boolean return from CertPool.AppendCertsFromPEM. If the input PEM is invalid/empty, this silently installs an empty RootCAs pool and will make all HTTPS handshakes fail in a non-obvious way. Consider checking the return value and either leaving RootCAs unchanged when parsing fails or providing a way to surface an error to the caller.
| pool.AppendCertsFromPEM(pem) | |
| if !pool.AppendCertsFromPEM(pem) { | |
| return | |
| } |
Refactoring of the
plumbing/transportAPI with a new design and add aplumbing/clientpackage for scheme-based transport resolution.More info on the original PR #1972.