send-pack: avoid sending the whole tree when pushing from a shallow clone - #2208
Open
newren wants to merge 1 commit into
Open
Conversation
newren
force-pushed
the
avoid-expensive-shallow-pushes
branch
from
August 20, 2026 05:38
0669a4b to
19d8607
Compare
newren
changed the base branch from
master
to
ps/odb-pluggable-pack-generation
August 21, 2026 03:25
…lone When pushing from a shallow clone, even if we only have made a small one-line change to a tiny file, we often push the entire toplevel tree of files. For large repositories, this could be gigabytes instead of kilobytes. The reason for this is that the push likely lacks the commits the receiver has advertised, so it walks back to its shallow grafts. Since it doesn't know that the server has anything, it sends the entire tree for the graft. It would also send the parents of the shallow graft, except the shallow clone doesn't have those by construction. We thus are forced to assume that the server has the parents of the shallow graft -- if it doesn't, the server's receive-pack will reject the push. But that raises the obvious question: if we're going to assume the server has the parents of the shallow graft, why not just assume the server has the shallow graft itself -- which this clone almost certainly received from the server when the shallow clone was created? As noted above, receive-pack already has a builtin connectivity check that predates pushing from a shallow clone by years[*], so even if a client is pushing to a different server than it cloned from, the worst that happens is a rejected push. And by assuming the server has the shallow graft commits, then for large repositories (those most likely to use shallow clone) we can avoid transferring (and perhaps re-compressing) gigabytes of file contents that the server already has. [*] Compare 5dbd767 (receive/send-pack: support pushing from a shallow clone, 2013-12-05) and 52fed6e (receive-pack: check connectivity before concluding "git push", 2011-09-02) Fix this by finding the shallow grafts behind the history we're pushing and adding them to the pack boundary as uninteresting (negative) tips, so the generated pack leaves out everything underneath them. We only use grafts that the pushed commits can actually reach; excluding every graft in the repository would be simpler, but it could drop an object we really do need to send -- for example, a new blob we're pushing that also happens to sit under some unrelated shallow root pulled from a different remote. We can also stop early at any commit we and the server both have -- one the server advertised, or that push negotiation found in common. Such a commit already marks the edge of what we need to send, so there's no reason to keep walking down to a graft below it. For deeper clones the server usually has a commit close by, which keeps this walk short; we only reach a graft when we and the server share no history that we know about. One very rare (and non-default) workflow genuinely needs the larger push: seeding a receiver willing to adopt new shallow roots (receive.shallowUpdate; see 5dbd767 (receive/send-pack: support pushing from a shallow clone, 2013-12-05) and 0a1bc12 (receive-pack: allow pushes that update .git/shallow, 2013-12-05)). When the server sets receive.shallowUpdate, it is willing to accept pushes despite lacking ancestors of the pushed commits. But it expects us to send all tree objects so it can graft a new shallow root. For that case, add a sender-side config, push.shallowExcludeBoundary, defaulting to true (the optimization), while allowing users to set it to false to restore the previous behavior needed for that rare case. Update the existing shallow-seeding tests in t5538 to set push.shallowExcludeBoundary=false, since they exercise that receive.shallowUpdate path. Add tests for the optimized default, the opt-out, and a deeper clone where a possessed common commit bounds the push. Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
avoid-expensive-shallow-pushes
branch
from
August 21, 2026 03:34
19d8607 to
fc54700
Compare
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
1 participant
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.
The base for this series is ps/odb-pluggable-pack-generation (which I rebased on top of and resolved conflicts with before submitting).
Users can work around the problem described in this patch with push.negotiate=true, but while we can educate some users to set that, trying to get them all to do so is quite unlikely. Let's help users by providing sane default behavior.
One alternative I considered here is making the new push.shallowExcludeBoundary config a tri-state: true, false, or abort, and default to abort. If abort, then when shallow grafts are reached by send-pack, simply abort the push on the client side and tell the user to set push.shallowExcludeBoundary to either true or false. That'd be the more traditional backward compatibility approach of introducing an error period before changing the default. But since the "traditional" case seems extraordinarily rare to me and already requires additional special configuration (receive.shallowUpdate=true on any relevant server), I thought the transition period wasn't warranted in this case. Let me know if you disagree.