internal: servers, add git server implementation#2014
Merged
Conversation
80016a4 to
2e441dd
Compare
This adds a new Git TCP server implementation based on the existing backend package. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
2e441dd to
a99d4be
Compare
Reject repeated Start calls for the git and HTTP test servers while preserving one-shot server lifecycle state. Assisted-By: Codex GPT 5.5 <noreply@openai.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
Move common server coverage into an internal/server suite that runs against both git and HTTP implementations. Assisted-By: Codex GPT 5.5 <noreply@openai.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
pjbgf
previously approved these changes
May 1, 2026
pjbgf
left a comment
Member
There was a problem hiding this comment.
@aymanbagabas thanks for working on this. 🙇
I added two additional commits:
- Enforce safe flows (e.g. can't call
Start()on a started server) for both git and http implementation. - Moved some of the tests into a suite to exercise across all implementations.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an in-process git:// (TCP) server implementation under internal/server/git, integrates it into the server helper set, and expands test coverage so suites using internal/server.All() can exercise both HTTP and Git transports.
Changes:
- Introduce a new
internal/server/gitTCP server built on the existingbackendpackage. - Update
internal/server.All()to include the new git server alongside HTTP. - Adjust
packp.GitProtoRequestvalidation/tests and add/extend server tests for both transports.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| plumbing/protocol/packp/gitproto_test.go | Updates encoding validation test case coverage for GitProtoRequest. |
| plumbing/protocol/packp/gitproto.go | Modifies GitProtoRequest validation rules (pathname no longer validated). |
| internal/server/servers_test.go | Adds a suite to run common server behavior tests across HTTP and git:// implementations. |
| internal/server/servers.go | Extends All() to include the new git:// server implementation. |
| internal/server/http/http.go | Adds start-state tracking and locking around HTTP server start/endpoint/close behavior. |
| internal/server/git/git_test.go | Adds integration tests for the git:// server (archive, timeouts, max connections). |
| internal/server/git/git.go | Implements the git:// TCP server, connection tracking, and deadline management. |
Comments suppressed due to low confidence (1)
plumbing/protocol/packp/gitproto_test.go:45
- This test now duplicates TestEncodeEmptyGitProtoRequest (both encode a zero-value GitProtoRequest and only assert a non-nil error). It no longer covers the previously distinct invalid case (non-empty RequestCommand with missing/empty Pathname), so the suite loses meaningful coverage. Consider restoring a separate assertion for the missing-pathname case (especially if pathname validation is reinstated).
func TestEncodeInvalidGitProtoRequest(t *testing.T) {
t.Parallel()
var buf bytes.Buffer
p := GitProtoRequest{}
err := p.Encode(&buf)
if err == nil {
t.Fatal("expected error")
}
}
Comment on lines
31
to
37
| // validate validates the request. | ||
| func (g *GitProtoRequest) validate() error { | ||
| if g.RequestCommand == "" { | ||
| return fmt.Errorf("%w: empty request command", ErrInvalidGitProtoRequest) | ||
| } | ||
|
|
||
| if g.Pathname == "" { | ||
| return fmt.Errorf("%w: empty pathname", ErrInvalidGitProtoRequest) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
GitProtoRequest.Encode always writes a " \x00" line, but validate() no longer rejects an empty Pathname. That allows encoding a syntactically invalid git:// request ("command \x00"), which can mask caller bugs (e.g., URL with empty path) and diverge from the documented pack-protocol transport format. Consider restoring the empty-pathname validation (and/or tightening Decode to reject an empty pathname as well).
The cancel func stored in s.conns was tied to a discarded context, so nothing it gated actually cancelled backend.Serve. Wire a single per-connection context through addConn/removeConn/cancelConn so that timeouts, Close, and connection removal all abort the backend handler. InitTimeout was recomputed from time.Now() on every updateDeadline call, letting a slow client trickling bytes keep extending the handshake deadline indefinitely (slowloris). Compute and store an absolute initDeadline once at accept time. Tighten the related tests: assert that the rejected MaxConnections client sees EOF rather than returning early on any Connect error, and make the Timeout test deterministic by inducing an idle period instead of racing handshake+GetRemoteRefs against a 100ms budget. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: ee43feb8a5f3
pjbgf
approved these changes
May 6, 2026
pjbgf
left a comment
Member
There was a problem hiding this comment.
@aymanbagabas thanks for working on this. 🙇
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.
This adds a new Git TCP server implementation based on the existing backend package. All the tests that use the
internal/server.All()for testing now will also use a git daemongit://TCP server in addition to the existing HTTP server.I assume this can eventually grow to offer a full
git-daemonimplementation that can replace go-git/cli/server/git