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
newren
force-pushed
the
avoid-expensive-shallow-pushes
branch
3 times, most recently
from
August 21, 2026 05:38
69ea93b to
649efa1
Compare
Author
|
/submit |
|
Submitted as pull.2208.git.1787295352016.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> 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.
Oh yeah, that issue. It's a common foot gun indeed, and the common
advice here is to never clone with "--depth=1", but always with
"--depth=2" so that there is at least one non-grafted commit available
on the client so that they can indeed perform proper negotiation with a
server. But over the years I had to explain this again and again, so it
is clear that this common knowledge might only be commonly known to
people who have spent way too much time in the Git codebase.
> 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?
It's a good question to ask. In theory though, can't it happen that the
client changes the commit in question locally, e.g. via `git commit
--amend`, and then pushes? If we now assume that the local commit exists
on the remote side then we'd be insufficient information to the server.
> 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.
Right, the server would catch that case and abort the push. But it
highlights the need for an escape hatch, and it makes me wonder what the
current behaviour is when the grafted commit got modified. I guess
nothing good comes out of it.
There's another question though: can we properly determine whether the
tree of the grafted commit matches a tree that the remote side has, for
example example by including the tree in the reference negotiation? I
have no idea whether that would break git-recieve-pack(1) or any other
clients out there, as I don't think we ever negotiated down to trees
until now. But in theory, there isn't really much of a reason why we
cannot do so.
[snip]
> 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 and the
> opt-out, that a rejected ref does not cause an accepted ref to be
> over-excluded, and that a shallowUpdate receiver still rejects a
> rootless snapshot by default.
Do we have tests that modify the grafted commit? It would be good to
learn how such pushes behave right now, and how the proposed change
modifies it.
[snip]
> 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.
Makes me wonder whether the default is something that we should adjust
so that this defaults to enabled. Are there any downsides to doing so?
> diff --git a/send-pack.c b/send-pack.c
> index f20460fbf4..9a035d7403 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -55,6 +56,86 @@ static void append_negative_object(struct repository *r,
> oid_array_append(haves, oid);
> }
>
> +static int check_to_send_update(const struct ref *ref, const struct send_pack_args *args);
> +
> +/*
> + * Add the shallow grafts (nr_parent == -1), which are reachable from the
> + * refs being pushed, to the pack boundary ("haves") as uninteresting
> + * (negative) tips so the generated pack leaves out everything beneath them.
> + *
> + * Walk only from the pushed tips, and only until a graft: using a graft
> + * that does not bound the pushed history could exclude an object we are
> + * genuinely sending (if it is also reachable from that unrelated graft).
> + * Stop early at any commit the peer already has, since it is a negative
> + * the peer can use and the graft beneath it would be redundant.
> + */
> +static void append_reachable_shallow_grafts(struct repository *r,
> + struct ref *refs,
> + struct oid_array *advertised,
> + struct oid_array *negotiated,
> + struct send_pack_args *args,
> + struct oid_array *haves)
Nit: it might make sense to mark those parameters as `const` that are
only used as input.
> +{
> + struct commit_list *pending = NULL;
> + struct oidset seen = OIDSET_INIT;
> + struct oidset known = OIDSET_INIT;
> + struct ref *ref;
> + size_t i;
> +
> + for (i = 0; i < advertised->nr; i++)
> + oidset_insert(&known, &advertised->oid[i]);
> + for (i = 0; i < negotiated->nr; i++)
> + oidset_insert(&known, &negotiated->oid[i]);
> + for (ref = refs; ref; ref = ref->next)
> + if (!is_null_oid(&ref->old_oid))
> + oidset_insert(&known, &ref->old_oid);
Okay, here we assemble the list of all objects that the remote is
supposed to know about.
> + for (ref = refs; ref; ref = ref->next) {
> + struct commit *commit;
> +
> + if (is_null_oid(&ref->new_oid))
> + continue;
> + if (check_to_send_update(ref, args))
> + continue;
> + commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
> + if (commit)
> + commit_list_insert(commit, &pending);
> + }
Hm. Why do we loop through the refs twice? Wouldn't it be possible to
combine both loops?
> + while (pending) {
> + struct commit *commit = pop_commit(&pending);
> + const struct object_id *oid = &commit->object.oid;
> + struct commit_graft *graft;
> + struct commit_list *parent;
> +
> + if (oidset_insert(&seen, oid))
> + continue;
> +
> + /*
> + * A commit the peer already has bounds the pushed history
> + * with a negative it can use, so stop here rather than
> + * descend to a graft that would only be redundant.
> + */
> + if (oidset_contains(&known, oid) &&
> + odb_has_object(r->objects, oid, 0))
> + continue;
We abort the walk whenever we hit any of the objects in our walk that
the remote supposedly already knows about.
> + graft = lookup_commit_graft(r, oid);
> + if (graft && graft->nr_parent == -1) {
> + append_negative_object(r, haves, oid);
> + continue;
> + }
And when hitting a graft we explicitly add that graf to the negative
objects, too, so that we include the graft itself and its tree.
Logic-wise this make sense, pending the above questions around whether a
graft can be modified locally.
> + if (repo_parse_commit(r, commit))
> + continue;
> + for (parent = commit->parents; parent; parent = parent->next)
> + commit_list_insert(parent->item, &pending);
> + }
> +
> + oidset_clear(&seen);
> + oidset_clear(&known);
> +}
Instead of doing a manual walk like this, shouldn't we use higher-level
interfaces like `repo_is_descendant_of()` that can make use of commit
graphs? That might be overkill though as we can assume that in most
shallow repositories we won't have deep commit history anyway.
I guess the answer is "no" though, as you don't only want to check
reachability, but also whether any commit in between is part of the
commits that either we or the server has advertised.
Thanks!
Patrick |
…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 and the opt-out, that a rejected ref does not cause an accepted ref to be over-excluded, and that a shallowUpdate receiver still rejects a rootless snapshot by default. Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
avoid-expensive-shallow-pushes
branch
from
August 21, 2026 17:34
649efa1 to
4e36fcc
Compare
|
Elijah Newren wrote on the Git mailing list (how to reply to this email): On Fri, Aug 21, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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.
>
> Oh yeah, that issue. It's a common foot gun indeed, and the common
> advice here is to never clone with "--depth=1", but always with
> "--depth=2" so that there is at least one non-grafted commit available
> on the client so that they can indeed perform proper negotiation with a
> server. But over the years I had to explain this again and again, so it
> is clear that this common knowledge might only be commonly known to
> people who have spent way too much time in the Git codebase.
I don't think --depth=2 actually helps here. What enables real
negotiation is push.negotiate, not the extra commit, and
push.negotiate works just as well at --depth=1.
Without push.negotiate, send-pack's only negatives come from the refs
the server advertised filtered by what we actually have. In the
foot-gun scenario -- clone shallow, server advances, then push, using
depth of 2 just walks one commit further to the graft and then
re-sends the whole tree anyway. Running the four combinations (server
advanced after clone, optimization disabled) in a small test repo:
depth=1, push.negotiate=false: Enumerating objects: 205
depth=2, push.negotiate=false: Enumerating objects: 208
depth=1, push.negotiate=true: Enumerating objects: 4
depth=2, push.negotiate=true: Enumerating objects: 4
--depth=2 without negotiation is if anything a hair worse, while
negotiation fixes it regardless of depth (the negotiator offers the
shallow graft commit itself as a "have", and the server ACKs it).
--depth=2 can in rare cases help, but only in the lucky/accidental
case where some advertised ref happens to point at the extra commit
you now have.
> It's a good question to ask. In theory though, can't it happen that the
> client changes the commit in question locally, e.g. via `git commit
> --amend`, and then pushes? If we now assume that the local commit exists
> on the remote side then we'd be insufficient information to the server.
Oh, wow, I had never thought to amend a shallow graft. As soon as you
asked, I assumed it'd create a corrupt repo -- a commit that wasn't
itself a shallow graft but had parents we didn't know about. I got
surprised in a different way, though: commit --amend treats a shallow
graft as a parent-less commit, and thus creates a new root commit.
That does avoid corruption, but only by providing a different kind of
foot-gun. (If users really wanted a new root commit, `git
{switch,checkout} --orphan` is the tool to do that.)
Since we've got another place where commit --amend can serve as a
foot-gun that I've long meant to fix up, I'll submit a separate series
that'll make it throw errors for both cases.
> There's another question though: can we properly determine whether the
> tree of the grafted commit matches a tree that the remote side has, for
> example example by including the tree in the reference negotiation? I
> have no idea whether that would break git-recieve-pack(1) or any other
> clients out there, as I don't think we ever negotiated down to trees
> until now. But in theory, there isn't really much of a reason why we
> cannot do so.
Interesting idea...but doesn't this happen too late to help? Without
push.negotiate=true, I _think_ (double check me) that the flow is:
* server blindly speaks first, advertising the refs it has
* client responds, including its shallow <oid> lines and then sending the pack
* server reports status
If I'm right about that, the server doesn't know about the client's
shallow grafts until too late, so it'd have to advertise the toplevel
tree of every commit it has if it wanted the client to be able to take
advantage of them.
Alternatively, we could change the protocol, but we already have
push.negotiate=true that is implemented and is more thorough than
sharing the common tree (the common commit contains the shared
toplevel tree). The only place I think a tree negotiation could win
over commit negotiation is when you keep a tree the server already has
but under a commit it doesn't -- e.g. you rewrite the grafted commit
but leave its tree (or part of it) unchanged. And if you changed the
top-level tree, you'd have to recurse and share each unchanged subtree
to avoid re-sending common history. That's a lot of machinery for a
narrow, contrived case, so I'm not sure it leads to a helpful path.
> [snip]
> > 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 and the
> > opt-out, that a rejected ref does not cause an accepted ref to be
> > over-excluded, and that a shallowUpdate receiver still rejects a
> > rootless snapshot by default.
>
> Do we have tests that modify the grafted commit? It would be good to
> learn how such pushes behave right now, and how the proposed change
> modifies it.
As noted above, modified commits are actually root commits and do not
have a shallow history, and thus aren't really part of shallow push
testing. I think it's a bug that modified commits become root
commits, but one that really is tangential to this patch. I'll submit
a separate series with a fix.
> [snip]
> > 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.
>
> Makes me wonder whether the default is something that we should adjust
> so that this defaults to enabled. Are there any downsides to doing so?
The only one I can think of is that it adds a round-trip to every
push, which increases latency in order to sometimes reduce bandwidth
and cpu.
It can dramatically reduce bandwidth and cpu, but not always (single
person projects would probably never see a benefit, for example, nor
would anyone interacting with a fetch v0 server), and it always
increases latency.
> > +static void append_reachable_shallow_grafts(struct repository *r,
> > + struct ref *refs,
> > + struct oid_array *advertised,
> > + struct oid_array *negotiated,
> > + struct send_pack_args *args,
> > + struct oid_array *haves)
>
> Nit: it might make sense to mark those parameters as `const` that are
> only used as input.
Good point; will fix.
> > + for (ref = refs; ref; ref = ref->next)
> > + if (!is_null_oid(&ref->old_oid))
> > + oidset_insert(&known, &ref->old_oid);
>
> Okay, here we assemble the list of all objects that the remote is
> supposed to know about.
>
> > + for (ref = refs; ref; ref = ref->next) {
> > + struct commit *commit;
> > +
> > + if (is_null_oid(&ref->new_oid))
> > + continue;
> > + if (check_to_send_update(ref, args))
> > + continue;
> > + commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
> > + if (commit)
> > + commit_list_insert(commit, &pending);
> > + }
>
> Hm. Why do we loop through the refs twice? Wouldn't it be possible to
> combine both loops?
Oops, good catch. Will fix.
> Instead of doing a manual walk like this, shouldn't we use higher-level
> interfaces like `repo_is_descendant_of()` that can make use of commit
> graphs? That might be overkill though as we can assume that in most
> shallow repositories we won't have deep commit history anyway.
>
> I guess the answer is "no" though, as you don't only want to check
> reachability, but also whether any commit in between is part of the
> commits that either we or the server has advertised.
Right, that's the reason: I need to stop at commits the peer already
has and pick out graft boundaries along the way, which a descendant
check doesn't give me. Shallow histories tend to be short, so the
explicit walk is likely cheap. |
|
User |
|
Elijah Newren wrote on the Git mailing list (how to reply to this email): On Fri, Aug 21, 2026 at 10:36 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Fri, Aug 21, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> > > From: Elijah Newren <newren@gmail.com>
> > >
> > > 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.
> >
> > Oh yeah, that issue. It's a common foot gun indeed, and the common
> > advice here is to never clone with "--depth=1", but always with
> > "--depth=2" so that there is at least one non-grafted commit available
> > on the client so that they can indeed perform proper negotiation with a
> > server. But over the years I had to explain this again and again, so it
> > is clear that this common knowledge might only be commonly known to
> > people who have spent way too much time in the Git codebase.
>
> I don't think --depth=2 actually helps here. What enables real
> negotiation is push.negotiate, not the extra commit, and
> push.negotiate works just as well at --depth=1.
>
> Without push.negotiate, send-pack's only negatives come from the refs
> the server advertised filtered by what we actually have. In the
> foot-gun scenario -- clone shallow, server advances, then push, using
> depth of 2 just walks one commit further to the graft and then
> re-sends the whole tree anyway. Running the four combinations (server
> advanced after clone, optimization disabled) in a small test repo:
>
> depth=1, push.negotiate=false: Enumerating objects: 205
> depth=2, push.negotiate=false: Enumerating objects: 208
> depth=1, push.negotiate=true: Enumerating objects: 4
> depth=2, push.negotiate=true: Enumerating objects: 4
>
> --depth=2 without negotiation is if anything a hair worse, while
> negotiation fixes it regardless of depth (the negotiator offers the
> shallow graft commit itself as a "have", and the server ACKs it).
>
> --depth=2 can in rare cases help, but only in the lucky/accidental
> case where some advertised ref happens to point at the extra commit
> you now have.
I guess I should add that --depth=2 is not really "luck" for some
users, but may be guaranteed by their workflow:
- customers of forges
- assuming those forges (make refs for merge/pull requests AND
advertise those refs from receive-pack) OR (keep a branch pointing at
the tip of the {pull,merge} request)
- assuming those users never push directly to their main branch
(instead only updating it via merge requests or pull requests)
- assuming those users don't use squash merges or rebases on their
merge request/pull requests, but do actual merges
If all the conditions above are met, forges should have a ref pointing
to the tip of the now-merged {merge,pull} request, which will never
change since it was merged, and thus a --depth=2 clone will pick up
such a commit and have some common history it discovers.
GitHub includes refs/pull/ in receive.hiderefs, and users often delete
branches upon merge, so neither half of that second condition holds
for us and this wouldn't help our customers. Further, even if we did
change the ref advertisement, we have a number of big repositories who
don't satisfy the other conditions (e.g. some customers make heavy use
of squash merges), so it still wouldn't help them. |
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.
Changes since v1:
Maintainer note: The base for this series is ps/odb-pluggable-pack-generation; that series' removal of feed_object() conflicted with my original version of this patch, so I rebased on that series and fixed up the conflict.
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.
cc: Patrick Steinhardt ps@pks.im
cc: Elijah Newren newren@gmail.com