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

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 2, 2026, 19:05 UTC
Message-ID: <1d6a4047-fa41-45cc-8097-88680e8ea67d@gmail.com>
In-Reply-To: <CABPp-BHwa7QM=XDuO=9xqm-OL8dn8uGf1=rv+sgBRQ9hHKMFuQ@mail.gmail.com>

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:

Show 12 quoted lines

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

Show 10 quoted lines

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

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.

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.

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.

Thanks, -Stolee