plumbing: transport, Align flush size with upstream git#2000
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns go-git pack negotiation “flush” behavior with upstream git to reduce inefficiency on large negotiations, and updates tests to validate multi-round stateless RPC behavior.
Changes:
- Introduces a
nextFlushgrowth strategy and replaces the fixed flush size of 32 with an adaptiveflushAtstarting at 16. - Adds unit tests for
nextFlushbehavior in stateless vs stateful modes. - Replaces the prior HTTP negotiator regression test with a higher-level smart fetch test that asserts multiple stateless RPC POST rounds occur.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plumbing/transport/negotiate.go | Adds adaptive flush sizing (nextFlush) and uses it in the negotiation loop. |
| plumbing/transport/negotiate_test.go | Adds unit coverage for nextFlush growth rules. |
| plumbing/transport/http/handshake_test.go | Adds an integration-style regression test to confirm multi-round stateless RPC fetch behavior and records POST bodies via a reverse proxy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pjbgf
force-pushed
the
align-fetch
branch
3 times, most recently
from
52bbf4b to
950ddd7
Compare
| base, backend := setupSmartServer(t) | ||
| remoteFS := prepareRepo(t, fixture, base, "packfile.git") | ||
| remotePath := remoteFS.Root() | ||
| remoteStorage := filesystem.NewStorage(osfs.New(remotePath), cache.NewObjectLRUDefault()) |
Contributor
There was a problem hiding this comment.
Suggested change
| remoteStorage := filesystem.NewStorage(osfs.New(remotePath), cache.NewObjectLRUDefault()) | |
| remoteStorage := filesystem.NewStorage(osfs.New(remotePath), cache.NewObjectLRUDefault()) | |
| defer func() { _ = remoteStorage.Close() }() |
| seedRef := plumbing.ReferenceName("refs/heads/seed-old") | ||
| require.NoError(t, remoteStorage.SetReference(plumbing.NewHashReference(seedRef, oldCommit))) | ||
| seedPath := filepath.Join(t.TempDir(), "seed.git") | ||
| seedStorage := initBareStorage(t, seedPath) |
Contributor
There was a problem hiding this comment.
Suggested change
| seedStorage := initBareStorage(t, seedPath) | |
| seedStorage := initBareStorage(t, seedPath) | |
| defer func() { _ = seedStorage.Close() }() |
| neg := &httpNegotiator{session: session, ctx: context.Background()} | ||
| defer neg.closeResponse() | ||
| clientPath := filepath.Join(t.TempDir(), "client.git") | ||
| clientStorage := initBareStorage(t, clientPath) |
Contributor
There was a problem hiding this comment.
Suggested change
| clientStorage := initBareStorage(t, clientPath) | |
| clientStorage := initBareStorage(t, clientPath) | |
| defer func() { _ = clientStorage.Close() }() |
Member
Author
There was a problem hiding this comment.
Thanks for that @AriehSchneier, added the three instances to the final change. 🙇
Signed-off-by: Paulo Gomes <[email protected]>
Previously go-git flush size was hard-coded to 32, which could be rather inefficient for very large negotiations. This change aligns the behaviour with upstream, growing exponentially that number when it is safe to do so, as per next_flush() (fetch-pack.c:277-291). The initial flush size is now 16 - which is also aligned with upstream. ref: https://github.com/git/git/blob/9f223ef1c026d91c7ac68cc0211bde255dda6199/fetch-pack.c Signed-off-by: Paulo Gomes <[email protected]>
… git Add applyServerACKs to properly handle multi-ack-detailed responses: - Reset in_vain on ACK_continue and ACK_ready (fetch-pack.c:550-552) - Re-queue ACK_common haves for stateless RPC rounds (fetch-pack.c:533-542) - Suppress duplicate ACK_common to avoid re-queuing already-known objects - Track ACK_ready and stop negotiation early (fetch-pack.c:555-556, 569-570) - Send a terminal done round after ACK_ready (fetch-pack.c:577-579) - Clamp batch size to remaining in-vein budget after got_continue Assisted-by: Claude Opus 4.6 <[email protected]> Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
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.
Previously go-git flush size was hard-coded to
32, which could be rather inefficient for verylarge negotiations. This change aligns the behaviour with upstream, growing exponentially that
number when it is safe to do so, as per next_flush().
The initial flush size is now
16- which is also aligned with upstream.