Skip to content

plumbing/transport API Refactoring#1983

Merged
pjbgf merged 34 commits into
mainfrom
transport-refactoring
Apr 13, 2026
Merged

plumbing/transport API Refactoring#1983
pjbgf merged 34 commits into
mainfrom
transport-refactoring

Conversation

@pjbgf

@pjbgf pjbgf commented Apr 12, 2026

Copy link
Copy Markdown
Member

Refactoring of the plumbing/transport API with a new design and add a plumbing/client package for scheme-based transport resolution.

More info on the original PR #1972.

aymanbagabas and others added 30 commits April 11, 2026 19:18
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]>
Copilot AI review requested due to automatic review settings April 12, 2026 20:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Conn interfaces and helpers.
  • Add plumbing/client to 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.log can be nil (it is set from Backend.ErrorLog), so calling f.log.Printf(...) can panic during error handling. Either guard these calls (e.g., if f.log != nil { ... }) or route logging through Backend.logf consistently.
    backend/writer.go:40
  • When nr != nw, err is nil here, so ReadFrom would return a nil error on a short write. Return io.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.

Comment on lines +141 to +143
func (s *ReceivePackServeSuite) TestReceivePackAdvertiseV2() {
testAdvertise(s.T(), UploadPack, "version=2", false)
}

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +81 to +87
for _, tc := range []struct {
name string
command string
}{
{"UploadPack", "git-upload-pack"},
{"ReceivePack", "git-upload-pack"},
} {

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +126
func (r *testProxyRule) Allow(_ context.Context, _ *socks5.Request) (context.Context, bool) {
*r.proxiedRequests++
return context.Background(), true

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread plumbing/transport/ssh/ssh.go
Comment thread plumbing/client/client.go
func WithCABundle(pem []byte) Option {
return func(o *options) {
pool := x509.NewCertPool()
pool.AppendCertsFromPEM(pem)

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
pool.AppendCertsFromPEM(pem)
if !pool.AppendCertsFromPEM(pem) {
return
}

Copilot uses AI. Check for mistakes.
@pjbgf pjbgf merged commit e0f7a92 into main Apr 13, 2026
20 checks passed
@pjbgf pjbgf deleted the transport-refactoring branch April 13, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants