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

On Wed, Sep 2, 2026 at 12:05 PM Derrick Stolee [off-list ref] wrote:

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:

quoted

On Sun, Aug 23, 2026 at 10:30 PM Patrick Steinhardt [off-list ref] wrote:

quoted

[...]

quoted

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.

quoted

  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/ (local)

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.

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

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.