Hi Stolee,
On Wed, Sep 2, 2026 at 11:23 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> On 8/25/2026 3:06 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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.