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

Hi Stolee,

On Wed, Sep 2, 2026 at 11:23 AM Derrick Stolee [off-list ref] wrote:

On 8/25/2026 3:06 PM, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

The reason for this is that the push likely lacks the commits the
receiver has advertised, so it walks back to its shallow grafts.  Since
it doesn't know that the server has anything, it sends the entire tree
for the graft.  It would also send the parents of the shallow graft,
except the shallow clone doesn't have those by construction.  We thus
are forced to assume that the server has the parents of the shallow
graft -- if it doesn't, the server's receive-pack will reject the push.

I was ready to assume this patch was fully correct, but then I asked
an AI agent to review it and it found an interesting subtlety that
puts the entire approach in question. It also presents an alternative
approach that is much simpler and helps improve things immediately.

The bug you found here is a really good discovery; thanks for sending
it along.  I think there are still some misunderstandings, though,
which I think may significantly affect the resulting conclusion.

The gist is that we can attempt to push a shallow object to a remote
that _doesn't have that commit or its parent_. This gets rejected by
the remote as not allowing a shallow update.

The problem occurs when this shallow update is attempted alongside
another non-shallow branch being pushed that also has some "new"
objects reachable, so the "assume the remote has the shallow
commit" condition leads to novel failures due to that other ref
update not having full connectivity.

Ah, I already had a similar test ("does not over-exclude for an
accepted ref via a rejected one"), but this is a different variant I
overlooked.  Good catch.

Here's a test for t5538 that the AI agent generated, and I
massaged into something more understandable/readable:

[...]

This test passes before this patch, but fails after.

As I was working on this test case, the key step that will fail with the
current patch is the test_grep here:

        test_must_fail git push --force receiver A topic 2>err &&
        test_grep "remote rejected.*shallow update not allowed" err

because the error that will be returned instead is more of a hard failure.
This failure "at grep time" is something I added. If this line doesn't
exist, then the 'git rev-parse --verify topic' fails which shows that we
are able to break the receiver repo with this push, as the second ref
update is accepted even though the packfile isn't complete.

Isn't this self-contradictory?  Saying "git rev-parse --verify topic
fails" means that `topic` was not created on the server.  Saying "the
second ref update is accepted" claims it was created on the server.

Also, I'm not sure where you got "break the receiver repo" from.  When
I re-run your exact testcase against the v2 patch, it is not broken:
  - git fsck passes
  - `A` remains unmodified
  - `topic` was also rejected
which seems to be guaranteed by 52fed6e1ce07 (receive-pack: check
connectivity before concluding "git push", 2011-09-02).

In particular, `git rev-parse --verify topic` failing here is the
*safe* outcome which means the push was denied.  So, the case you
provided has no corruption.  In fact, all that has happened is that
this shallow push caused the pushes to fail.  A simple re-push of
individual refs by the user seems like the natural next step.

However, the error message returned for this testcase is inscrutable;
by my count the potential error messages here are about half a dozen
depending on the exact codepath that is triggered based on a few
tweaks of config settings, and the unpack-objects ones are
particularly bad.  So we really ought to make those error messages
better, and perhaps provide a hint to the user to just retry pushing
individual refs as a simple workaround; that'd point out to the user
that does hit your usecase that there's a really simple "recovery"
path for them.  I've got some patches to fix that up.

When I asked the agent to implement something that instead cared about
whether the remote refs could reach the shallow commits, it deleted this
method in favor of having your push.shallowexcludeboundary setting enable
push.negotiate when the local repo is shallow:

        repo_config_get_bool(r, "push.shallowexcludeboundary",
                             &shallow_exclude_boundary);
        if (is_repository_shallow(r) && shallow_exclude_boundary)
                push_negotiate = 1;

That was sufficient to pass the new test, as well as all other tests you
added, except one. I'm not sure if we need a new option or if we should
recommend push.negotiate in more places (plus these new tests).

Yeah, as noted elsewhere in this thread, there is a flowchart of
reasons why push.negotiate=true will fail to solve the problem.  You
have since commented in that thread, so we can leave that discussion
over there.