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

On Fri, Aug 21, 2026 at 6:17 AM Patrick Steinhardt <[email protected]> wrote:
>
> On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[email protected]>
> >
> > 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.