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 10:36:04AM -0700, Elijah Newren 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.

TIL, thanks. I don't think I was even aware of "push.negotiate", and I
mostly went by the folklore of "just clone with --depth=2" that I saw
repeated on many sites.

But this and all of your other answers make me lean strongly into the
direction that the fix is at the wrong level, and the proper fix really
is to enable "push.negotiate" by default.

> > 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.

That makes sense.

> > [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.

That's all fair, but it does dramatically help in the case of shallow
clones. And the number of times I've seen this question come up hints
that this is a very common scenario.

We could be clever about it: if "push.negotiate" is very likely to help
in shallow clones but mostly just adds latency in full clones, then why
don't we introduce a new "push.negotiate=shallow" option that enables
this feature automatically for shallow clones and make it the default?
That to me sounds like a low-hanging fruit, and I would prefer such a
fix compared to introducing new logic.

Patrick