Re: [PATCH] send-pack: avoid sending the whole tree when pushing from a shallow clone

[Patrick Steinhardt <ps@pks.im>]

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