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

From: Elijah Newren <newren@gmail.com>
Date: Sep 2, 2026, 20:57 UTC
Message-ID: <CABPp-BFekYtzXo7BscEw=6CvPve-shA5ZvXkQaj-jcALE7Sx+w@mail.gmail.com>
In-Reply-To: <1d6a4047-fa41-45cc-8097-88680e8ea67d@gmail.com>

On Wed, Sep 2, 2026 at 12:05 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 30 quoted lines

>
> Sorry that I missed this portion of the discussion talking about
> push.negotiate. Coming back to correct that.
>
> On 8/25/2026 1:00 AM, Elijah Newren wrote:
> > On Sun, Aug 23, 2026 at 10:30 PM Patrick Steinhardt <ps@pks.im> wrote:
> >>
> > [...]
> >> 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.
> >
> > I don't think that fixes the problem, though:
>
> You are right that the following cases are somewhat common.
>
> >   a) Users can do a shallow clone of a specific branch for a specific
> > pull-request/merge-request.  Then the pull-request/merge-request is
> > rebased, and sensitive data removed due to a leaked secret.  The
> > shallow graft is no longer common.  Pushing from the shallow clone
> > should fail, but it shouldn't have to send several gigabytes of data
> > in order to get the failure message.
> >   b) (Very similar to a) Users can do a shallow clone of one repo (a
> > local repository cache?) and then push to another; the shallow graft
> > thus may not be common.  An error is expected, but sending gigabytes
> > of data to get the error isn't.

I personally think (d) which you snipped out, namely push.negotiate=true doesn't work for some users/servers, may be more common. I know you, Patrick, and I were all hoping that push.negotiate=true would be a panacea for the common case, but the conditions behind (d) that prevent that option from working for some users would seem to be more common to me than these two conditions.

Further, my previous list for (d) was incomplete...

push.negotiate=true can fail in another case both under http and ssh:
  - repack replaces packfiles on the server with a new packfile.
  - The client points to the shallow-graft as something it has.
  - The server looks up that commit ID with QUICK, losing the race
with repacking, and reports it doesn't have it.
  - The client doesn't have any more history further back so it can't
find any more shared history.
  - Under current versions of git, the client believes it has to send
_everything_ it has.

In the concurrent-repack discussion, upload-pack's QUICK "have" check was deemed working-as-intended, on the grounds that a dropped "have" just means "the client is sent more than it needs." For a shallow clone that "bit more" is the whole history the client has, which is exactly the problem this patch fixes. I'm not trying to reopen that other discussion, and I admit this race is rare, but when it triggers, it'll defeat push.negotiate=true. I think we need a backstop. (And even if we do revisit that QUICK race, there's still the other conditions in my previous email under which push.negotiate=true fails.)

[1] https://lore.kernel.org/git/20260827055743.GB189659@coredump.intra.peff.net/

> For this case (b) I can think of it as doing a shallow clone of a
> base repo (https://github.com/git/git) and then needing to push to
> a user-owned fork (https://github.com/derrickstolee/git) and the
> fork not advertising reachability to the shallow commit.

Yep, that's probably a better way to put it.

Show 8 quoted lines

> I think the difficulties here is that your approach is assuming
> something about how "non-advertised" objects may exist due to either
>
>  a) delayed garbage collection, or
>  b) shared object databases across a fork network.
>
> I don't think these are reasonable assumptions to have by default,
> so we need to be really clear about the reason to use this setting.

I don't follow.

A shallow push already assumes something about how "non-advertised" objects may exist -- it assumes the *parents* of the shallow graft exist on the server. Why is it such a big leap to move from assuming the server has the parents of the shallow graft to assuming it has the shallow graft itself? Further, what are the consequences of assuming or not assuming the shallow graft exists?

Here's the matrix:

Assume the shallow graft exists:
  (A) and it does -> push succeeds, and does so orders of magnitude
faster in large repos
  (B) but it doesn't, nor does its parents -> push fails with error
message we would have gotten anyway, and does so dramatically faster
  (C) but it doesn't, but its parents (magically) do -> sends an error
message quickly, where the push would have (eventually) previously
succeeded

Assume the shallow graft doesn't exist:
  (D) but it does -> push succeeds, AFTER pushing hundreds of
megabytes of almost certainly unnecessary data
  (E) and it doesn't, nor does its parents -> get back an error
message, AFTER pushing hundreds of megabytes of unnecessary data
  (F) and it doesn't, but its parents (magically) do -> push succeeds,
AFTER pushing hundreds of megabytes of mostly unnecessary data since
we can't determine which parts are necessary

Clearly, (A) and (B) are vastly superior to (D) and (E). The only case in question then is (C) vs (F). My opinions there:

(1) We already generally require folks to push from shallow clones back to repositories that have the parents of the shallow graft and extending that requirement to the shallow graft itself does not seem unreasonable to me. I would much rather be told I'm pushing to the wrong remote than wait forever. (2) case C/F is incredibly unlikely (people tend to push back to the same server, and even if they don't, the server likely either has the shallow graft and its history or is missing the parents of the graft as well).

Show 7 quoted lines

> As your test demonstrates, some amount of "our assumption was wrong"
> is built in, so we should have a way for users to respond quickly
> or automatically (retry without the setting?).
>
> The multi-push case that I brought up is tricky, though. It may
> be very narrow, and HTTP servers would be protected, but we should
> avoid allowing corruption over file:// protocol.

Ah! I see where the disconnect may have been. Yeah, corruption needs to be prevented, and if corruption was a risk then it'd override other concerns. But that isn't relevant here: receive-pack checks for connectivity (regardless of protocol -- http, ssh, or file) and fails the push if objects are missing. (See commit 52fed6e1ce07 (receive-pack: check connectivity before concluding "git push", 2011-09-02)). The multi-push case then ends up being a case of us failing more refs than necessary, not a way to induce corruption.

Also, I didn't state this earlier, but this bug can actually be more
comical.  If someone clones with e.g. `git clone --depth ${N}
--filter=blob:none --sparse ...`, then after making their changes and
deciding to push and the server no longer has references to one of the
commits in our shallow clone:
  (i) Push notes that it knows no objects the server has -> it needs
to send ALL trees and blobs from the shallow graft
  (ii) It doesn't have ALL blobs from the shallow graft -> promisor
remote handling kicks in
  (iii) promisor remote downloads ALL blobs from the shallow graft
from our origin
  (iv) push can now push all objects to our origin

Above, you'll note that although the user started with a tiny clone, step (iii) downloads huge amounts of data from origin so that step (iv) can upload that "necessary" data back to the server it just downloaded it from. My patch avoids both the unnecessary huge download and upload.