send-pack: avoid sending the whole tree when pushing from a shallow clone

From: Elijah Newren <[email protected]>

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.

But that raises the obvious question: if we're going to assume the server has the parents of the shallow graft, why not just assume the server has the shallow graft itself -- which this clone almost certainly received from the server when the shallow clone was created? As noted above, receive-pack already has a builtin connectivity check that predates pushing from a shallow clone by years[*], so even if a client is pushing to a different server than it cloned from, the worst that happens is a rejected push. And by assuming the server has the shallow graft commits, then for large repositories (those most likely to use shallow clone) we can avoid transferring (and perhaps re-compressing) gigabytes of file contents that the server already has.

[*] Compare 5dbd76760181 (receive/send-pack: support pushing from a

    shallow clone, 2013-12-05) and 52fed6e1ce07 (receive-pack: check
    connectivity before concluding "git push", 2011-09-02)

Fix this by finding the shallow grafts behind the history we're pushing and adding them to the pack boundary as uninteresting (negative) tips, so the generated pack leaves out everything underneath them. We only use grafts that the pushed commits can actually reach; excluding every graft in the repository would be simpler, but it could drop an object we really do need to send -- for example, a new blob we're pushing that also happens to sit under some unrelated shallow root pulled from a different remote.

We can also stop early at any commit we and the server both have -- one the server advertised, or that push negotiation found in common. Such a commit already marks the edge of what we need to send, so there's no reason to keep walking down to a graft below it. For deeper clones the server usually has a commit close by, which keeps this walk short; we only reach a graft when we and the server share no history that we know about.

One very rare (and non-default) workflow genuinely needs the larger
push: seeding a receiver willing to adopt new shallow roots (receive.shallowUpdate; see 5dbd76760181 (receive/send-pack: support pushing from a shallow clone, 2013-12-05) and 0a1bc12b6e40 (receive-pack: allow pushes that update .git/shallow, 2013-12-05)). When the server sets receive.shallowUpdate, it is willing to accept pushes despite lacking ancestors of the pushed commits. But it expects us to send all tree objects so it can graft a new shallow root. For that case, add a sender-side config, push.shallowExcludeBoundary, defaulting to true (the optimization), while allowing users to set it to false to restore the previous behavior needed for that rare case.

Update the existing shallow-seeding tests in t5538 to set push.shallowExcludeBoundary=false, since they exercise that receive.shallowUpdate path. Add tests for the optimized default and the opt-out, that a rejected ref does not cause an accepted ref to be over-excluded, and that a shallowUpdate receiver still rejects a rootless snapshot by default.

---
send-pack: avoid sending the whole tree when pushing from a shallow clone

Maintainer note: The base for this series is ps/odb-pluggable-pack-generation; that series' removal of feed_object() conflicted with my original version of this patch, so I rebased on that series and fixed up the conflict.

Users can work around the problem described in this patch with push.negotiate=true, but while we can educate some users to set that, trying to get them all to do so is quite unlikely. Let's help users by providing sane default behavior.

One alternative I considered here is making the new push.shallowExcludeBoundary config a tri-state: true, false, or abort, and default to abort. If abort, then when shallow grafts are reached by send-pack, simply abort the push on the client side and tell the user to set push.shallowExcludeBoundary to either true or false. That'd be the more traditional backward compatibility approach of introducing an error period before changing the default. But since the "traditional" case seems extraordinarily rare to me and already requires additional special configuration (receive.shallowUpdate=true on any relevant server), I thought the transition period wasn't warranted in this case. Let me know if you disagree.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2208%2Fnewren%2Favoid-expensive-shallow-pushes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2208/newren/avoid-expensive-shallow-pushes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2208

 Documentation/config/push.adoc |  12 +++
 send-pack.c                    |  95 ++++++++++++++++++++
 t/t5538-push-shallow.sh        | 156 ++++++++++++++++++++++++++++++++-
 3 files changed, 260 insertions(+), 3 deletions(-)

index 28132eedfe..9fd6a956a8 100644
@@ -134,6 +134,18 @@ This will result in only b (a and c are cleared).
 	rely solely on the server's ref advertisement to find commits
 	in common.
 
+`push.shallowExcludeBoundary`::
+	When pushing from a shallow repository (see linkgit:git-clone[1]
+	`--depth`), Git normally assumes that the receiving end already
+	has the pushing repository's shallow grafts, and omits those
+	objects from the generated pack rather than resending the full
+	toplevel tree of those grafts. This is safe because the
+	receiving end rejects a push that references objects it does not
+	have. Set this to `false` to send those objects anyway; this is
+	only needed for the highly unusual case of using a push to seed
+	a receiver that adopts new shallow roots (i.e. a receiver that
+	has explicitly set `receive.shallowUpdate`). Default is `true`.
+
 `push.useBitmaps`::
 	If set to `false`, disable use of bitmaps for `git push` even if
 	`pack.useBitmaps` is `true`, without preventing other git operations

base-commit: 96650039a0dff984f3575568aea85af68474f5c3

gitgitgadget

Oh yeah, that issue. It's a common foot gun indeed, and the common advice here is to never clone with "--depth=1", but always with "--depth=2" so that there is at least one non-grafted commit available on the client so that they can indeed perform proper negotiation with a server. But over the years I had to explain this again and again, so it is clear that this common knowledge might only be commonly known to people who have spent way too much time in the Git codebase.

It's a good question to ask. In theory though, can't it happen that the client changes the commit in question locally, e.g. via git commit --amend, and then pushes? If we now assume that the local commit exists on the remote side then we'd be insufficient information to the server.

Right, the server would catch that case and abort the push. But it highlights the need for an escape hatch, and it makes me wonder what the current behaviour is when the grafted commit got modified. I guess nothing good comes out of it.

There's another question though: can we properly determine whether the tree of the grafted commit matches a tree that the remote side has, for example example by including the tree in the reference negotiation? I have no idea whether that would break git-recieve-pack(1) or any other clients out there, as I don't think we ever negotiated down to trees until now. But in theory, there isn't really much of a reason why we cannot do so.

[snip]

Do we have tests that modify the grafted commit? It would be good to learn how such pushes behave right now, and how the proposed change modifies it.

[snip]

Makes me wonder whether the default is something that we should adjust so that this defaults to enabled. Are there any downsides to doing so?

I don't think --depth=2 actually helps here. What enables real negotiation is push.negotiate, not the extra commit, and push.negotiate works just as well at --depth=1.

Without push.negotiate, send-pack's only negatives come from the refs the server advertised filtered by what we actually have. In the foot-gun scenario -- clone shallow, server advances, then push, using depth of 2 just walks one commit further to the graft and then re-sends the whole tree anyway. Running the four combinations (server advanced after clone, optimization disabled) in a small test repo:

    depth=1, push.negotiate=false:  Enumerating objects: 205
    depth=2, push.negotiate=false:  Enumerating objects: 208
    depth=1, push.negotiate=true:   Enumerating objects: 4
    depth=2, push.negotiate=true:   Enumerating objects: 4

--depth=2 without negotiation is if anything a hair worse, while negotiation fixes it regardless of depth (the negotiator offers the shallow graft commit itself as a "have", and the server ACKs it).

--depth=2 can in rare cases help, but only in the lucky/accidental case where some advertised ref happens to point at the extra commit you now have.

Oh, wow, I had never thought to amend a shallow graft. As soon as you asked, I assumed it'd create a corrupt repo -- a commit that wasn't itself a shallow graft but had parents we didn't know about. I got surprised in a different way, though: commit --amend treats a shallow graft as a parent-less commit, and thus creates a new root commit. That does avoid corruption, but only by providing a different kind of foot-gun. (If users really wanted a new root commit, git {switch,checkout} --orphan is the tool to do that.)

Since we've got another place where commit --amend can serve as a foot-gun that I've long meant to fix up, I'll submit a separate series that'll make it throw errors for both cases.

Interesting idea...but doesn't this happen too late to help? Without push.negotiate=true, I _think_ (double check me) that the flow is:

* server blindly speaks first, advertising the refs it has
* client responds, including its shallow <oid> lines and then sending the pack
* server reports status

If I'm right about that, the server doesn't know about the client's shallow grafts until too late, so it'd have to advertise the toplevel tree of every commit it has if it wanted the client to be able to take advantage of them.

Alternatively, we could change the protocol, but we already have push.negotiate=true that is implemented and is more thorough than sharing the common tree (the common commit contains the shared toplevel tree). The only place I think a tree negotiation could win over commit negotiation is when you keep a tree the server already has but under a commit it doesn't -- e.g. you rewrite the grafted commit but leave its tree (or part of it) unchanged. And if you changed the top-level tree, you'd have to recurse and share each unchanged subtree to avoid re-sending common history. That's a lot of machinery for a narrow, contrived case, so I'm not sure it leads to a helpful path.

As noted above, modified commits are actually root commits and do not have a shallow history, and thus aren't really part of shallow push testing. I think it's a bug that modified commits become root commits, but one that really is tangential to this patch. I'll submit a separate series with a fix.

The only one I can think of is that it adds a round-trip to every push, which increases latency in order to sometimes reduce bandwidth and cpu.

It can dramatically reduce bandwidth and cpu, but not always (single person projects would probably never see a benefit, for example, nor would anyone interacting with a fetch v0 server), and it always increases latency.

Good point; will fix.

Oops, good catch. Will fix.

Right, that's the reason: I need to stop at commits the peer already has and pick out graft boundaries along the way, which a descendant check doesn't give me. Shallow histories tend to be short, so the explicit walk is likely cheap.

I guess I should add that --depth=2 is not really "luck" for some users, but may be guaranteed by their workflow:
- customers of forges
- assuming those forges (make refs for merge/pull requests AND
advertise those refs from receive-pack) OR (keep a branch pointing at the tip of the {pull,merge} request)
- assuming those users never push directly to their main branch
(instead only updating it via merge requests or pull requests)
- assuming those users don't use squash merges or rebases on their
merge request/pull requests, but do actual merges

If all the conditions above are met, forges should have a ref pointing to the tip of the now-merged {merge,pull} request, which will never change since it was merged, and thus a --depth=2 clone will pick up such a commit and have some common history it discovers.

GitHub includes refs/pull/ in receive.hiderefs, and users often delete branches upon merge, so neither half of that second condition holds for us and this wouldn't help our customers. Further, even if we did change the ref advertisement, we have a number of big repositories who don't satisfy the other conditions (e.g. some customers make heavy use of squash merges), so it still wouldn't help them.

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.

That makes sense.

That's all fair, but it does dramatically help in the case of shallow clones. And the number of times I've seen this question come up hints that this is a very common scenario.

We could be clever about it: if "push.negotiate" is very likely to help in shallow clones but mostly just adds latency in full clones, then why don't we introduce a new "push.negotiate=shallow" option that enables this feature automatically for shallow clones and make it the default? That to me sounds like a low-hanging fruit, and I would prefer such a fix compared to introducing new logic.

Patrick

[...]

I don't think that fixes the problem, though:

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.
c) Users set push.negotiate=false explicitly. In my opinion, they
shouldn't get this bug just for opting out of that kind-of-related feature.
d) push.negotiate=true silently fails for some setups

I think case (d) is particularly interesting: For push.negotiate to work, fetch v2 must be working. For http, that's not a big deal. When using ssh, it requires the client to send GIT_PROTOCOL=version=2 environment variable and for the server to accept it:
* Server side:
* Some self-hosting forges may not automatically support receiving
the environment variable. My searches suggest BitBucket always uses v0 for ssh, and GitLab depends on the installation method -- either the Linux package or self-compile installs requiring manual action (only Helm and the all-in-one Docker image are preconfigured)
* Some corporate setups may specifically want to disallow sending
any environment variables over ssh (perhaps through an "upstream stock configuration only" policy?)
* Client side:
* git only requests v2 when it decides the client is OpenSSH (I
think that maps to the command being named ssh/ssh.exe, or an auto-probe succeeds)
* plink / putty / tortoiseplink appear to not allow sending this
environment variable, so many Windows users may be cut out
* Some corporate setups might restrict sending environment
variables over ssh on the client side as well

When it's not supported, it falls back to v0 with a simple warning, does no negotiation, and runs into the old bug.

So, while I support the idea of moving towards push.negotiate=true or even adding push.negotiate=shallow, because they would provide other benefits, I don't think they fix the problem at hand and thus believe that this patch is still important.

[...]

Turns out there's a bunch of additional stuff on the shallow side, so I think I'm going to split it into two series; a single patch for rebase/revert/am, and five or so patches for shallow graft handling across a variety of commands.

I kind of like the idea of somehow making push.negotiate default on for big repos in general (not just shallow), but while push.negotiate has lots of other benefits and has the side effect of solving most cases of this problem for some users, it falls short of actually fully solving the problem. This patch, or something like it, is still needed.

Sorry that I missed this portion of the discussion talking about push.negotiate. Coming back to correct that.

You are right that the following cases are somewhat common.

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

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/[email protected]/

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

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

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.

From: Elijah Newren <[email protected]>

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.

But that raises the obvious question: if we're going to assume the server has the parents of the shallow graft, why not just assume the server has the shallow graft itself -- which this clone almost certainly received from the server when the shallow clone was created? As noted above, receive-pack already has a builtin connectivity check that predates pushing from a shallow clone by years[*], so even if a client is pushing to a different server than it cloned from, the worst that happens is a rejected push. And by assuming the server has the shallow graft commits, then for large repositories (those most likely to use shallow clone) we can avoid transferring (and perhaps re-compressing) gigabytes of file contents that the server already has.

[*] Compare 5dbd76760181 (receive/send-pack: support pushing from a

    shallow clone, 2013-12-05) and 52fed6e1ce07 (receive-pack: check
    connectivity before concluding "git push", 2011-09-02)

Fix this by finding the shallow grafts behind the history we're pushing and adding them to the pack boundary as uninteresting (negative) tips, so the generated pack leaves out everything underneath them. We only use grafts that the pushed commits can actually reach; excluding every graft in the repository would be simpler, but it could drop an object we really do need to send -- for example, a new blob we're pushing that also happens to sit under some unrelated shallow root pulled from a different remote.

We can also stop early at any commit we and the server both have -- one the server advertised, or that push negotiation found in common. Such a commit already marks the edge of what we need to send, so there's no reason to keep walking down to a graft below it. For deeper clones the server usually has a commit close by, which keeps this walk short; we only reach a graft when we and the server share no history that we know about.

One very rare (and non-default) workflow genuinely needs the larger
push: seeding a receiver willing to adopt new shallow roots (receive.shallowUpdate; see 5dbd76760181 (receive/send-pack: support pushing from a shallow clone, 2013-12-05) and 0a1bc12b6e40 (receive-pack: allow pushes that update .git/shallow, 2013-12-05)). When the server sets receive.shallowUpdate, it is willing to accept pushes despite lacking ancestors of the pushed commits. But it expects us to send all tree objects so it can graft a new shallow root. For that case, add a sender-side config, push.shallowExcludeBoundary, defaulting to true (the optimization), while allowing users to set it to false to restore the previous behavior needed for that rare case.

Update the existing shallow-seeding tests in t5538 to set push.shallowExcludeBoundary=false, since they exercise that receive.shallowUpdate path. Add tests for the optimized default and the opt-out, that a rejected ref does not cause an accepted ref to be over-excluded, and that a shallowUpdate receiver still rejects a rootless snapshot by default.

---
send-pack: avoid sending the whole tree when pushing from a shallow clone

Changes since v1:

* Fixed two small code style issues
* Updated the cover letter below to point out that push.negotiate=true doesn't work for everyone, and even when negotiation does work it doesn't solve all cases.
* Updated to latest ps/odb-pluggable-pack-generation branch.

Maintainer note: The base for this series is ps/odb-pluggable-pack-generation; that series' removal of feed_object() conflicted with my original version of this patch, so I rebased on that series and fixed up the conflict.

Some users can work around the problem described in this patch with push.negotiate=true. Even if we were to make that the default, though,
(a) negotiation doesn't work for some people (depending on other server and client settings and programs), and (b) even for those for whom negotiation does happen, that doesn't solve all cases. Let's help users by providing sane default behavior.

One alternative I considered here is making the new push.shallowExcludeBoundary config a tri-state: true, false, or abort, and default to abort. If abort, then when shallow grafts are reached by send-pack, simply abort the push on the client side and tell the user to set push.shallowExcludeBoundary to either true or false. That'd be the more traditional backward compatibility approach of introducing an error period before changing the default. But since the "traditional" case seems extraordinarily rare to me and already requires additional special configuration (receive.shallowUpdate=true on any relevant server), I thought the transition period wasn't warranted in this case. Let me know if you disagree.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2208%2Fnewren%2Favoid-expensive-shallow-pushes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2208/newren/avoid-expensive-shallow-pushes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2208

Range-diff vs v1:

1: 649efa1c5f ! 1: d4501a5c23 send-pack: avoid sending the whole tree when pushing from a shallow clone

     @@ send-pack.c: static void append_negative_object(struct repository *r,
      + * the peer can use and the graft beneath it would be redundant.
      + */
      +static void append_reachable_shallow_grafts(struct repository *r,
     -+					    struct ref *refs,
     -+					    struct oid_array *advertised,
     -+					    struct oid_array *negotiated,
     -+					    struct send_pack_args *args,
     ++					    const struct ref *refs,
     ++					    const struct oid_array *advertised,
     ++					    const struct oid_array *negotiated,
     ++					    const struct send_pack_args *args,
      +					    struct oid_array *haves)
      +{
      +	struct commit_list *pending = NULL;
      +	struct oidset seen = OIDSET_INIT;
      +	struct oidset known = OIDSET_INIT;
     -+	struct ref *ref;
     ++	const struct ref *ref;
      +	size_t i;
      +
      +	for (i = 0; i < advertised->nr; i++)
      +		oidset_insert(&known, &advertised->oid[i]);
      +	for (i = 0; i < negotiated->nr; i++)
      +		oidset_insert(&known, &negotiated->oid[i]);
     -+	for (ref = refs; ref; ref = ref->next)
     -+		if (!is_null_oid(&ref->old_oid))
     -+			oidset_insert(&known, &ref->old_oid);
      +
     ++	/*
     ++	 * Record every commit the peer is known to have as a boundary for
     ++	 * the walk, and seed the walk from the tips we are actually sending.
     ++	 * The walk below does not begin until "known" is fully populated.
     ++	 */
      +	for (ref = refs; ref; ref = ref->next) {
      +		struct commit *commit;
      +
     ++		if (!is_null_oid(&ref->old_oid))
     ++			oidset_insert(&known, &ref->old_oid);
     ++
      +		if (is_null_oid(&ref->new_oid))
      +			continue;
      +		if (check_to_send_update(ref, args))

 Documentation/config/push.adoc |  12 +++
 send-pack.c                    | 100 +++++++++++++++++++++
 t/t5538-push-shallow.sh        | 156 ++++++++++++++++++++++++++++++++-
 3 files changed, 265 insertions(+), 3 deletions(-)

index 28132eedfe..9fd6a956a8 100644
@@ -134,6 +134,18 @@ This will result in only b (a and c are cleared).
 	rely solely on the server's ref advertisement to find commits
 	in common.
 
+`push.shallowExcludeBoundary`::
+	When pushing from a shallow repository (see linkgit:git-clone[1]
+	`--depth`), Git normally assumes that the receiving end already
+	has the pushing repository's shallow grafts, and omits those
+	objects from the generated pack rather than resending the full
+	toplevel tree of those grafts. This is safe because the
+	receiving end rejects a push that references objects it does not
+	have. Set this to `false` to send those objects anyway; this is
+	only needed for the highly unusual case of using a push to seed
+	a receiver that adopts new shallow roots (i.e. a receiver that
+	has explicitly set `receive.shallowUpdate`). Default is `true`.
+
 `push.useBitmaps`::
 	If set to `false`, disable use of bitmaps for `git push` even if
 	`pack.useBitmaps` is `true`, without preventing other git operations

base-commit: 5176dd3d057ac5cae8321508febef61fa88537aa

gitgitgadget

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

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

# A ref that passes the client's checks can still be rejected by the receiver. # Its shallow graft must not trim objects needed by another ref in the shared # pack, since a non-atomic push should still allow that other ref to succeed. #
# The client has two unrelated shallow histories ("x" marks a shallow graft). # Blob O is present in A1 and is reintroduced by cY on topic:
#
# contains O
# |
# A0----------A1(x)---cX refs/heads/A
#
# B0----------B1(x)---cY refs/heads/topic
# \
# contains O
#
# The receiver has only the B history. Both of its refs A and B point to

# the same B1 commit as full history. It has neither A1 nor blob O in its
# object database.
#
# The '--force' option lets the force-push of A from client to receiver
# pass the client's checks, but the receiver rejects A because it will not
# adopt A1 as a new shallow root.
test_expect_success 'shallow push does not over-exclude via a remotely rejected ref' '
	# origin: two unrelated histories; only branch A has blob "shared"
	git init remote-reject-origin &&
	(
		cd remote-reject-origin &&
		git checkout -b A &&
		test_commit --no-tag has-shared sh shared &&
		test_commit --no-tag A1 &&
		git switch --orphan B &&
		test_commit --no-tag B0 &&
		test_commit --no-tag B1
	) &&

	# receiver: commit B1 is exposed as both B and A and lacks A1
	git init --bare remote-reject-receiver.git &&
	(
		cd remote-reject-origin &&
		git remote add receiver ../remote-reject-receiver.git &&
		git push receiver B:refs/heads/B B:refs/heads/A
	) &&

	# client: each remote branch tip is a shallow graft
	git clone --depth=1 --no-single-branch \
		"file://$(pwd)/remote-reject-origin" remote-reject-client &&

	old_a=$(cd remote-reject-receiver.git && git rev-parse A) &&
	(
		cd remote-reject-client &&
		git remote add receiver ../remote-reject-receiver.git &&

		# Force makes A pass the client-side non-fast-forward check. The
		# receiver will reject it because A1 is a new shallow root and
		# receive.shallowUpdate is disabled.
		git checkout A &&
		test_commit --no-tag cX &&

		# topic is independently valid but needs the shared blob from A1.
		git checkout -b topic B &&
		test_commit --no-tag reintroduce sh shared &&

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

	# The non-atomic push should reject A without affecting topic.
	(
		cd remote-reject-receiver.git &&
		test "$old_a" = "$(git rev-parse A)" &&
		git rev-parse --verify topic
	)

'

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.

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

-       # Default (optimization on): the s2 boundary snapshot is withheld, so
-       # the receiver cannot graft the new root and rejects the push, leaving
-       # the ref uncreated.
-       test_must_fail git -C seed-client push \

-       # Opt-out: the full snapshot is sent, so the same push now succeeds and
-       # the new shallow root is grafted.
-       git -C seed-client -c push.shallowExcludeBoundary=false push \
+       git -C seed-client rev-parse HEAD^ >expect &&
+       git -C seed-client push \
                "file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded &&
-       git --git-dir=seed-receiver.git rev-parse --verify seeded
+       git --git-dir=seed-receiver.git rev-parse --verify seeded &&
+       test_cmp expect seed-receiver.git/shallow &&
+       git --git-dir=seed-receiver.git fsck

Hi Stolee,

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.

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.

[...]

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.

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.

Changes since v2:

* Rebased on master (ps/odb-pluggable-pack-generation has now been merged)
* Completely rewrote the cover letter below
* Fixed several suboptimal error messages related to Stolee's suggested testcase (Patches [1-3]/6)
* Made push.shallowExcludeBoundary a tri-state setting, initially keeping the same default (Patch 4/6)
* Argued for the change of default in a separate patch (Patch 5/6)
* Added advice for retrying problematic multi-ref pushes separately. (Patch 6/6)

Changes since v1:

* Fixed two small code style issues
* Updated the cover letter below to point out that push.negotiate=true doesn't work for everyone, and even when negotiation does work it doesn't solve all cases.
* Updated to latest ps/odb-pluggable-pack-generation branch.

[Note: push.negotiate=true is NOT a general solution to this problem. There's too many holes it leaves open.]

When pushing from a shallow clone, send-pack may know none of the commits advertised by the receiver. Pack generation then walks back to the client's shallow boundary and sends the boundary commit's entire tree. A tiny change can consequently result in transferring gigabytes of objects that the receiver almost certainly already has.

The behavior can be even worse with a partial, sparse clone. For example, a repository created with:

git clone --depth=2 --filter=blob:none --sparse ...

may not have the blobs from its shallow boundary locally. Before sending the unnecessarily large pack, Git first downloads those blobs from its promisor remote, only to upload them back to what is often the same server.

A shallow client already omits the boundary commit's parents, which it does not have, and relies on receive-pack's connectivity check to reject the push if the receiver lacks them. This series allows the client to make the same assumption about the boundary commit itself. Omitting that commit prevents its tree from becoming part of the generated pack.

If the receiver has the shallow graft commit, the push avoids transferring and recompressing its tree. If the receiver lacks both the shallow graft commit and its history, the push was going to fail anyway, but now fails without first sending the large pack.

The special case to consider is the rare use of push to seed a receiver that accepts new shallow roots. Such a receiver already requires receive.shallowUpdate=true; it must now be paired with push.shallowExcludeBoundary=false on the client so that the boundary snapshot is sent.

The series introduces push.shallowExcludeBoundary with three values:

* true omits reachable shallow boundaries from the pack;
* false retains the historical behavior; and
* abort refuses to choose either behavior, telling the user to specify.

The series first introduces the option while not changing the default, and then argues for the change of default in a separate patch.

As highlighted by Stolee's testcase, omitting a boundary can also cause a shared pack for multiple refs to lack an object needed by one of those refs. However, there is no corruption -- the receiver safely rejects the affected updates, and the final patch advises retrying the refs separately, allowing the user to easily recover.

Before changing send-pack, the first three patches improve how receive-pack handles and reports incomplete pushes. Missing objects are distinguished from type mismatches, repeated connectivity diagnostics are suppressed, and a missing shallow boundary results in per-ref "missing necessary objects" errors instead of receive-pack disconnecting.

Elijah Newren (6):
unpack-objects: distinguish missing objects from type mismatches
receive-pack: avoid repeating connectivity errors
shallow: reject missing boundaries without disconnecting
send-pack: optionally omit shallow boundaries
send-pack: default to excluding shallow boundaries
send-pack: advise splitting incomplete shallow pushes

 Documentation/config/advice.adoc |   5 +
 Documentation/config/push.adoc   |  23 ++++
 advice.c                         |   1 +
 advice.h                         |   1 +
 builtin/receive-pack.c           |   7 +
 builtin/unpack-objects.c         |   9 +-
 send-pack.c                      | 146 ++++++++++++++++++++-
 shallow.c                        |  16 ++-
 t/t5410-receive-pack.sh          |   6 +-
 t/t5504-fetch-receive-strict.sh  |   7 +-
 t/t5538-push-shallow.sh          | 216 ++++++++++++++++++++++++++++++-
 11 files changed, 422 insertions(+), 15 deletions(-)

base-commit: 1630431f326e15fcde608827b5ff38422528eb59
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2208%2Fnewren%2Favoid-expensive-shallow-pushes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2208/newren/avoid-expensive-shallow-pushes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2208

Range-diff vs v2:

 -:  ---------- > 1:  6056689be0 unpack-objects: distinguish missing objects from type mismatches
 -:  ---------- > 2:  74a52a632e receive-pack: avoid repeating connectivity errors
 -:  ---------- > 3:  fc21ecf832 shallow: reject missing boundaries without disconnecting
 1:  d4501a5c23 ! 4:  7a4fb38450 send-pack: avoid sending the whole tree when pushing from a shallow clone

@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- send-pack: avoid sending the whole tree when pushing from a shallow clone
+ send-pack: optionally omit shallow boundaries

- 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.
+ When the receiver advertises no commit the shallow client has, pack
+ generation walks to a shallow boundary and sends its entire tree. A tiny
+ push can consequently transfer gigabytes of objects the receiver likely
+ already has.

- 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.
+ The client already assumes the receiver has the boundary's parents,
+ which are absent from the shallow clone. Extend that option to the
+ boundary itself: push.shallowExcludeBoundary=true adds reachable shallow
+ grafts as negative tips, letting receive-pack's connectivity check
+ reject the push if the assumption is wrong.

- But that raises the obvious question: if we're going to assume the
- server has the parents of the shallow graft, why not just assume the
- server has the shallow graft itself -- which this clone almost certainly
- received from the server when the shallow clone was created? As noted
- above, receive-pack already has a builtin connectivity check that
- predates pushing from a shallow clone by years[*], so even if a client
- is pushing to a different server than it cloned from, the worst that
- happens is a rejected push. And by assuming the server has the shallow
- graft commits, then for large repositories (those most likely to use
- shallow clone) we can avoid transferring (and perhaps re-compressing)
- gigabytes of file contents that the server already has.
+ Only use grafts reached from refs contributing to the pack. An unrelated
+ graft could otherwise exclude an object another ref needs. Stop at
+ commits known to both sides, since they already bound the pack.

- [*] Compare 5dbd76760181 (receive/send-pack: support pushing from a
- shallow clone, 2013-12-05) and 52fed6e1ce07 (receive-pack: check
- connectivity before concluding "git push", 2011-09-02)
-
- Fix this by finding the shallow grafts behind the history we're pushing
- and adding them to the pack boundary as uninteresting (negative) tips,
- so the generated pack leaves out everything underneath them. We only
- use grafts that the pushed commits can actually reach; excluding every
- graft in the repository would be simpler, but it could drop an object we
- really do need to send -- for example, a new blob we're pushing that
- also happens to sit under some unrelated shallow root pulled from a
- different remote.
-
- We can also stop early at any commit we and the server both have --
- one the server advertised, or that push negotiation found in common.
- Such a commit already marks the edge of what we need to send, so
- there's no reason to keep walking down to a graft below it. For
- deeper clones the server usually has a commit close by, which keeps
- this walk short; we only reach a graft when we and the server share no
- history that we know about.
-
- One very rare (and non-default) workflow genuinely needs the larger
- push: seeding a receiver willing to adopt new shallow roots
- (receive.shallowUpdate; see 5dbd76760181 (receive/send-pack: support
- pushing from a shallow clone, 2013-12-05) and 0a1bc12b6e40
- (receive-pack: allow pushes that update .git/shallow, 2013-12-05)).
- When the server sets receive.shallowUpdate, it is willing to accept
- pushes despite lacking ancestors of the pushed commits. But it expects
- us to send all tree objects so it can graft a new shallow root. For
- that case, add a sender-side config, push.shallowExcludeBoundary,
- defaulting to true (the optimization), while allowing users to set it to
- false to restore the previous behavior needed for that rare case.
-
- Update the existing shallow-seeding tests in t5538 to set
- push.shallowExcludeBoundary=false, since they exercise that
- receive.shallowUpdate path. Add tests for the optimized default and the
- opt-out, that a rejected ref does not cause an accepted ref to be
- over-excluded, and that a shallowUpdate receiver still rejects a
- rootless snapshot by default.
+ Also accept "abort" to make no assumption, and "false" to retain the
+ historical behavior required when seeding a receive.shallowUpdate
+ receiver. Keep false as the default for now, so introducing the
+ mechanism does not change existing pushes.

@@ Documentation/config/push.adoc: This will result in only b (a and c are cleared)
in common.

+push.shallowExcludeBoundary::
-+ When pushing from a shallow repository (see linkgit:git-clone[1]
-+ --depth), Git normally assumes that the receiving end already
-+ has the pushing repository's shallow grafts, and omits those
-+ objects from the generated pack rather than resending the full
-+ toplevel tree of those grafts. This is safe because the
-+ receiving end rejects a push that references objects it does not
-+ have. Set this to false to send those objects anyway; this is
-+ only needed for the highly unusual case of using a push to seed
-+ a receiver that adopts new shallow roots (i.e. a receiver that
-+ has explicitly set receive.shallowUpdate). Default is true.
++ When pushing from a shallow repository, Git can omit the shallow
++ grafts' objects from the generated pack rather than resending the
++ full toplevel tree of those grafts. This assumes the receiver
++ already has those objects. If it does not, the receiver rejects
++ the push rather than accepting incomplete history. This setting
++ controls that behavior and accepts three values:
+++
++--
++abort;;
++ If the push reaches such a boundary, refuse it rather than
++ choosing whether to send or omit it.
++true;;
++ Omit the boundary objects (fast). If the receiver does not have
++ them, the push is rejected.
++false;;
++ (the default) Send the boundary objects, retaining the historical
++ behavior. This can send the boundary's entire tree, which may be
++ very large. This is only needed when pushing to a receiver that
++ accepts new shallow roots (i.e. one with receive.shallowUpdate
++ enabled), which is very rare.
++--
+
push.useBitmaps::
If set to false, disable use of bitmaps for git push even if
@@ send-pack.c: static void append_negative_object(struct repository *r,
oid_array_append(haves, oid);
}

-+static int check_to_send_update(const struct ref *ref, const struct send_pack_args *args); ++static int check_to_send_update(const struct ref *ref,
++ const struct send_pack_args *args);
++
++enum exclude_boundary_mode {
++ EXCLUDE_BOUNDARY_NONE = 0,
++ EXCLUDE_BOUNDARY_YES,
++ EXCLUDE_BOUNDARY_ABORT
++};
++
++static enum exclude_boundary_mode get_exclude_boundary_mode(struct repository *r) ++{
++ const char *value;
++
++ if (repo_config_get_string_tmp(r, "push.shallowexcludeboundary", &value))
++ return EXCLUDE_BOUNDARY_NONE;
++
++ switch (git_parse_maybe_bool(value)) {
++ case 1:
++ return EXCLUDE_BOUNDARY_YES;
++ case 0:
++ return EXCLUDE_BOUNDARY_NONE;
++ default:
++ if (!strcasecmp(value, "abort"))
++ return EXCLUDE_BOUNDARY_ABORT;
++ die(_("bad push.shallowExcludeBoundary value: %s"), value);
++ }
++}
+
+/*
-+ * Add the shallow grafts (nr_parent == -1), which are reachable from the -+ * refs being pushed, to the pack boundary ("haves") as uninteresting
-+ * (negative) tips so the generated pack leaves out everything beneath them. -+ *
-+ * Walk only from the pushed tips, and only until a graft: using a graft -+ * that does not bound the pushed history could exclude an object we are -+ * genuinely sending (if it is also reachable from that unrelated graft). -+ * Stop early at any commit the peer already has, since it is a negative -+ * the peer can use and the graft beneath it would be redundant.
++ * Append shallow grafts bounding contributing refs. Grafts from unrelated ++ * history could exclude objects this push needs, while commits both sides ++ * have make any graft below them irrelevant.
+ */
-+static void append_reachable_shallow_grafts(struct repository *r,
++static int append_reachable_shallow_grafts(struct repository *r,
+ const struct ref *refs,
+ const struct oid_array *advertised,
+ const struct oid_array *negotiated,
@@ send-pack.c: static void append_negative_object(struct repository *r,
+ struct oidset seen = OIDSET_INIT;
+ struct oidset known = OIDSET_INIT;
+ const struct ref *ref;
++ int found = 0;
+ size_t i;
+
+ for (i = 0; i < advertised->nr; i++)
@@ send-pack.c: static void append_negative_object(struct repository *r,
+ for (i = 0; i < negotiated->nr; i++)
+ oidset_insert(&known, &negotiated->oid[i]);
+
-+ /*
-+ * Record every commit the peer is known to have as a boundary for
-+ * the walk, and seed the walk from the tips we are actually sending.
-+ * The walk below does not begin until "known" is fully populated.
-+ */
++ /* Populate "known" fully before starting the walk. */
+ for (ref = refs; ref; ref = ref->next) {
+ struct commit *commit;
+
@@ send-pack.c: static void append_negative_object(struct repository *r,
+ if (oidset_insert(&seen, oid))
+ continue;
+
-+ /*
-+ * A commit the peer already has bounds the pushed history
-+ * with a negative it can use, so stop here rather than
-+ * descend to a graft that would only be redundant.
-+ */
+ if (oidset_contains(&known, oid) &&
+ odb_has_object(r->objects, oid, 0))
+ continue;
@@ send-pack.c: static void append_negative_object(struct repository *r,
+ graft = lookup_commit_graft(r, oid);
+ if (graft && graft->nr_parent == -1) {
+ append_negative_object(r, haves, oid);
++ found++;
+ continue;
+ }
+
@@ send-pack.c: static void append_negative_object(struct repository *r,
+
+ oidset_clear(&seen);
+ oidset_clear(&known);
++ return found;
+}
+
/*
@@ send-pack.c: static int pack_objects(struct repository *r,
for (size_t i = 0; i < negotiated->nr; i++)
append_negative_object(r, &opts.haves, &negotiated->oid[i]);

-+ /*
-+ * When pushing from a shallow repository, avoid re-pushing the
-+ * entire toplevel tree.
-+ */
-+ if (is_repository_shallow(r)) {
-+ int exclude_boundary = 1;
-+ repo_config_get_bool(r, "push.shallowexcludeboundary",
-+ &exclude_boundary);
-+ if (exclude_boundary)
-+ append_reachable_shallow_grafts(r, refs, advertised,
-+ negotiated, args,
-+ &opts.haves);
-+ }
++ /* Exclude reachable shallow boundaries from the pack. */
++ if (is_repository_shallow(r) &&
++ get_exclude_boundary_mode(r) == EXCLUDE_BOUNDARY_YES)
++ append_reachable_shallow_grafts(r, refs, advertised,
++ negotiated, args,
++ &opts.haves);
+
while (refs) {
if (!is_null_oid(&refs->old_oid))
append_negative_object(r, &opts.haves, &refs->old_oid);
+@@ send-pack.c: int send_pack(struct repository *r,
+ ref->status = REF_STATUS_EXPECTING_REPORT;
+ }
+
++ /* Honor ABORT before sending any ref-update commands. */
++ if (!args->dry_run && need_pack_data && is_repository_shallow(r) &&
++ get_exclude_boundary_mode(r) == EXCLUDE_BOUNDARY_ABORT) {
++ struct oid_array probe = OID_ARRAY_INIT;
++ int reachable = append_reachable_shallow_grafts(r, remote_refs,
++ extra_have,
++ &commons, args,
++ &probe);
++ oid_array_clear(&probe);
++ if (reachable)
++ die(_("refusing to push a shallow boundary commit\n"
++ "Set push.shallowExcludeBoundary to true to omit it (fast),\n"
++ "or false to send it (needed for receive.shallowUpdate)."));
++ }
++
+ if (!args->dry_run)
+ advertise_shallow_grafts_buf(r, &req_buf);
+

## t/t5538-push-shallow.sh ##
-@@ t/t5538-push-shallow.sh: EOF
- test_expect_success 'push from shallow clone, with grafted roots' '
- (
- cd shallow2 &&
-- test_must_fail git push ../.git +main:refs/remotes/shallow2/main 2>err &&
-+ test_must_fail git -c push.shallowExcludeBoundary=false \
-+ push ../.git +main:refs/remotes/shallow2/main 2>err &&
- test_grep "shallow2/main.*shallow update not allowed" err
- ) &&
- test_must_fail git rev-parse shallow2/main &&
-@@ t/t5538-push-shallow.sh: test_expect_success 'add new shallow root with receive.updateshallow on' '
- test_config receive.shallowupdate true &&
- (
- cd shallow2 &&
-- git push ../.git +main:refs/remotes/shallow2/main
-+ git -c push.shallowExcludeBoundary=false \
-+ push ../.git +main:refs/remotes/shallow2/main
- ) &&
- git log --format=%s shallow2/main >actual &&
- git fsck &&
-@@ t/t5538-push-shallow.sh: test_expect_success 'push from shallow to shallow' '
- (
- cd shallow &&
- git --git-dir=../shallow2/.git config receive.shallowupdate true &&
-- git push ../shallow2/.git +main:refs/remotes/shallow/main &&
-+ git -c push.shallowExcludeBoundary=false \
-+ push ../shallow2/.git +main:refs/remotes/shallow/main &&
- git --git-dir=../shallow2/.git config receive.shallowupdate false
- ) &&
- (
-@@ t/t5538-push-shallow.sh: test_expect_success 'push new commit from shallow clone has good deltas' '
- test_region pack-objects path-walk config-push.txt
+@@ t/t5538-push-shallow.sh: test_expect_success 'incomplete shallow push rejects without disconnecting' '
+ test_grep ! "unable to parse commit" err
'

-+test_expect_success 'shallow push only pushes what is necessary' ' ++test_expect_success 'shallow boundary exclusion avoids sending the full tree' '
+ git init adv-origin &&
+ # The shallow grafts are intentionally untagged so that no
+ # advertised ref points at them.
@@ t/t5538-push-shallow.sh: test_expect_success 'push new commit from shallow clone
+
+ git -C adv-client checkout -b topic &&
+ test_commit --no-tag -C adv-client new &&
-+ GIT_PROGRESS_DELAY=0 git -C adv-client push --progress origin topic 2>err &&
++ GIT_PROGRESS_DELAY=0 git -C adv-client \
++ -c push.shallowExcludeBoundary=true \
++ push --progress origin topic 2>err &&
+
+ # Only the new commit, its tree, and the new blob are sent; sending
+ # the full tree is avoided by excluding the shallow graft.
@@ t/t5538-push-shallow.sh: test_expect_success 'push new commit from shallow clone
+ test_grep "Enumerating objects: 7, done." err
+'
+
-+# A rejected ref must not over-exclude objects that another, accepted ref
-+# legitimately needs in the pack. Set up a testcase using two independent
-+# shallow roots.
-+#
-+# origin: two unrelated histories; only branch A carries blob O (sh=shared)
-+# A: A0---A1 (A0, A1 trees contain sh=O)
-+# B: B0---B1 (no "shared" blob)
-+#
-+# receiver: seeded from branch B only, under both ref names; lacks blob O
-+# refs/heads/B -> B1
-+# refs/heads/A -> B1 (makes our A push a non-fast-forward)
-+#
-+# client: "clone --depth=1 --no-single-branch" gives a graft at each tip
-+# and a copy of blob O under A1 (x = cut parents = shallow graft)
-+# x x
-+# | |
-+# A1 B1
-+# | |
-+# cX topic=cY (cY re-adds sh=O, which the receiver lacks)
-+#
-+# push "A topic" (non-atomic):
-+# A -> a non-fast-forward vs receiver A=B1, so its ref update is
-+# rejected locally and never applied. It still takes part in
-+# the shared pack computation, and the buggy code also walked
-+# back from it to graft A1 (which owns O).
-+# topic -> accepted; cY grafts onto B1 and needs blob O.
-+#
-+# Using the shallow graft A1 (an ancestor of A) to trim the pack, even
-+# though our push of A is rejected locally, would omit blob O from topic's
-+# pack -- yet topic needs O. We want to ensure that when topic is pushed,
-+# O is sent along with it despite A being rejected.
++test_expect_success 'push.shallowExcludeBoundary=abort refuses when a graft is reached' '
++ git init adv-origin3 &&
++ test_commit --no-tag -C adv-origin3 a &&
++ test_commit --no-tag -C adv-origin3 b &&
++
++ git clone --depth=1 "file://$(pwd)/adv-origin3" adv-client3 &&
++
++ # The remote branch advances past the history we have, so its
++ # advertised tip cannot bound the walk; only the shallow graft could,
++ # which is exactly what "abort" refuses to rely on.
++ test_commit --no-tag -C adv-origin3 c &&
++
++ git -C adv-client3 checkout -b topic &&
++ test_commit --no-tag -C adv-client3 new &&
++
++ test_must_fail git -C adv-client3 \
++ -c push.shallowExcludeBoundary=abort push origin topic 2>err &&
++ test_grep "push.shallowExcludeBoundary" err &&
++
++ # The receiver must be left untouched: no ref was created.
++ test_must_fail git -C adv-origin3 rev-parse --verify refs/heads/topic
++'
++
++# A and B are unrelated shallow histories. The receiver has B1 under both
++# names, but lacks the "shared" blob from A1. The client adds cX atop A1 and
++# reintroduces "shared" on a topic atop B1. Pushing A and topic together
++# rejects A as a non-fast-forward, but A still participates in pack selection. ++# Its A1 boundary must not exclude the blob needed by topic.
+test_expect_success 'shallow push does not over-exclude for an accepted ref via a rejected one' '
-+ # origin
+ git init tworoot-origin &&
+ git -C tworoot-origin checkout -b A &&
+ test_commit -C tworoot-origin --no-tag has-shared sh shared &&
@@ t/t5538-push-shallow.sh: test_expect_success 'push new commit from shallow clone
+ test_commit -C tworoot-origin --no-tag B0 &&
+ test_commit -C tworoot-origin --no-tag B1 &&
+
-+ # receiver: branch B only, exposed as both B and A
+ git init --bare tworoot-receiver.git &&
+ git -C tworoot-origin push "file://$(pwd)/tworoot-receiver.git" \
+ B:refs/heads/B B:refs/heads/A &&
+
-+ # client: a shallow graft at each branch tip
+ git clone --depth=1 --no-single-branch \
+ "file://$(pwd)/tworoot-origin" tworoot-client &&
+
-+ # branch A gets commit cX; including A in the push gives us a
-+ # locally-rejected ref whose graft A1 the buggy code walked to. The A
-+ # ref update is a non-fast-forward, so it is rejected and never applied.
+ git -C tworoot-client checkout A &&
+ test_commit -C tworoot-client --no-tag cX &&
+
-+ # branch topic is what we actually send, reintroducing blob O on B1
+ git -C tworoot-client checkout -b topic B &&
+ test_commit -C tworoot-client --no-tag reintroduce sh shared &&
+
-+ # push both in one command: they share a single pack computation, so a
-+ # graft reached from the rejected A can strip objects that topic needs.
-+ # The A ref update is rejected locally (non-fast-forward); the shared
-+ # pack must still contain blob O for topic to land on the receiver.
-+ test_must_fail git -C tworoot-client push \
++ test_must_fail git -C tworoot-client \
++ -c push.shallowExcludeBoundary=true push \
+ "file://$(pwd)/tworoot-receiver.git" A topic &&
+ git --git-dir=tworoot-receiver.git rev-parse --verify topic
+'
+
-+# push.shallowExcludeBoundary (default true) omits the shallow boundary
-+# snapshot from the pack, since an ordinary receiver already has it. The
-+# exception is a receiver willing to adopt a *new* shallow root
-+# (receive.shallowUpdate): it genuinely needs that snapshot, so the default
-+# optimization leaves it unable to graft the new root. Verify the receiver
-+# rejects such a push (rather than corrupting itself), and that setting the
-+# config to false restores the full snapshot and lets the push succeed. This -+# is the tradeoff that motivates the config knob.
-+test_expect_success 'default push to a shallowUpdate receiver rejects a rootless snapshot' ' ++# A receive.shallowUpdate receiver needs the boundary snapshot to adopt a new ++# shallow root, so omission must reject rather than create a broken ref. ++test_expect_success 'push to a shallowUpdate receiver rejects a rootless snapshot' '
+ git init seed-origin &&
+ test_commit -C seed-origin s1 &&
+ test_commit -C seed-origin s2 &&
@@ t/t5538-push-shallow.sh: test_expect_success 'push new commit from shallow clone
+ git init --bare seed-receiver.git &&
+ git --git-dir=seed-receiver.git config receive.shallowUpdate true &&
+
-+ # Default (optimization on): the s2 boundary snapshot is withheld, so
-+ # the receiver cannot graft the new root and rejects the push, leaving
-+ # the ref uncreated.
-+ test_must_fail git -C seed-client push \
++ # Optimization on: the s2 boundary snapshot is withheld, so the
++ # receiver cannot graft the new root and rejects the push, leaving the
++ # ref uncreated.
++ test_must_fail git -C seed-client \
++ -c push.shallowExcludeBoundary=true push \
+ "file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded 2>err &&
+ test_grep "remote rejected" err &&
+ test_must_fail git --git-dir=seed-receiver.git rev-parse --verify seeded &&

 -:  ---------- > 5:  afa44c6d22 send-pack: default to excluding shallow boundaries
 -:  ---------- > 6:  ae821ce078 send-pack: advise splitting incomplete shallow pushes

gitgitgadget

From: Elijah Newren <[email protected]>

With receive.fsckObjects enabled, an incomplete pushed pack reports "object of unexpected type" when the expected object is simply absent. That suggests corruption rather than identifying the missing object.

Use the same diagnostics as index-pack: report "did not receive expected object" when lookup fails, and reserve the type-mismatch message for an object that exists with the wrong type.

---
 builtin/unpack-objects.c        | 9 +++++++--
 t/t5504-fetch-receive-strict.sh | 7 +++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

index 351948724a..ceefeb5a49 100644
@@ -233,8 +233,13 @@ static int check_object(struct object *obj, enum object_type type,
 	if (!(obj->flags & FLAG_OPEN)) {
 		size_t size;
 		int type = odb_read_object_info(the_repository->objects, &obj->oid, &size);
-		if (type != obj->type || type <= 0)
-			die("object of unexpected type");
+		if (type <= 0)
+			die(_("did not receive expected object %s"),
+			    oid_to_hex(&obj->oid));
+		if (type != obj->type)
+			die(_("object %s: expected type %s, found %s"),
+			    oid_to_hex(&obj->oid),
+			    type_name(obj->type), type_name(type));
 		obj->flags |= FLAG_WRITTEN;
 		return 0;
 	}

index 75b2b87999..0848e2da4a 100755
@@ -105,8 +105,11 @@ test_expect_success 'push with receive.fsckobjects' '
 	To dst
 	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
 	EOF
-	test_must_fail git push --porcelain dst main:refs/heads/test >act &&
-	test_cmp exp act
+	test_must_fail git push --porcelain dst main:refs/heads/test >act 2>err &&
+	test_cmp exp act &&
+	missing_oid=$(sed -e s%/%% S) &&
+	test_grep "did not receive expected object $missing_oid" err &&
+	test_grep ! "object of unexpected type" err
 '
 
 test_expect_success 'push with transfer.fsckobjects' '

gitgitgadget

From: Elijah Newren <[email protected]>

receive-pack first checks all proposed ref tips together. If that bulk connectivity check fails, it checks each tip separately to identify which ref updates need "missing necessary objects".

The bulk check already reports rev-list's diagnostic. The per-ref checks repeat it merely as a side effect of attributing the failure, potentially once for every broken ref. Silence their stderr while retaining their exit status and the per-ref rejection.

---
 builtin/receive-pack.c  | 7 +++++++
 t/t5410-receive-pack.sh | 6 ++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

index e6e54ba55f..8079901bb6 100644
@@ -1785,6 +1785,13 @@ static void set_connectivity_errors(struct command *commands,
 			/* to be checked in update_shallow_ref() */
 			continue;
 
+		/*
+		 * The bulk check already reported rev-list's diagnostics;
+		 * this per-ref pass only attributes the failure, so keep it
+		 * quiet rather than repeat those errors for every ref.
+		 */
+		opt.quiet = 1;
+
 		odb_transaction_env(transaction, &env);
 		opt.env = env.v;
 

index 09d6bfd2a1..20d221044f 100755
@@ -68,9 +68,11 @@ test_expect_success TEE_DOES_NOT_HANG \
 	# Replay captured git-send-pack(1) output on new empty repository.
 	git init --bare remote.git &&
 	git receive-pack remote.git <out >actual 2>err &&
+	depacketize <actual >actual.raw &&
 
-	test_grep "missing necessary objects" actual &&
-	test_grep "fatal: Failed to traverse parents" err &&
+	test_grep "missing necessary objects" actual.raw &&
+	test_grep "fatal: Failed to traverse parents" actual.raw &&
+	test_must_be_empty err &&
 	test_must_fail git -C remote.git cat-file -e $(git -C repo rev-parse HEAD)
 '
 

gitgitgadget

From: Elijah Newren <[email protected]>

An incomplete shallow push can refer to a boundary commit the receiver does not have. remove_nonexistent_theirs_shallow() drops that graft, so paint_down() does not recognize it as a boundary and dies when parsing the missing commit. The client then sees only that the remote hung up.

Treat an absent commit as the end of that traversal path rather than aborting receive-pack. This lets paint_down() process the remaining commits, after which the connectivity check rejects each affected ref with "missing necessary objects". A present commit that cannot be parsed still indicates corruption and remains fatal.

---
 shallow.c               | 16 +++++++++++---
 t/t5538-push-shallow.sh | 46 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)

index 8e244a5669..c6f7437022 100644
@@ -659,9 +659,19 @@ static void paint_down(struct paint_info *info, const struct object_id *oid,
 		if (c->object.flags & BOTTOM)
 			continue;
 
-		if (repo_parse_commit(the_repository, c))
-			die("unable to parse commit %s",
-			    oid_to_hex(&c->object.oid));
+		if (repo_parse_commit_gently(the_repository, c, 1)) {
+			/*
+			 * remove_nonexistent_theirs_shallow() may have
+			 * dropped a missing boundary, leaving it unmarked
+			 * as BOTTOM. Let the connectivity check reject a
+			 * missing commit, but still die on a corrupt one.
+			 */
+			if (odb_has_object(the_repository->objects,
+					   &c->object.oid, 0))
+				die("unable to parse commit %s",
+				    oid_to_hex(&c->object.oid));
+			continue;
+		}
 
 		for (p = c->parents; p; p = p->next) {
 			if (p->item->object.flags & SEEN)

gitgitgadget

From: Elijah Newren <[email protected]>

When the receiver advertises no commit the shallow client has, pack generation walks to a shallow boundary and sends its entire tree. A tiny push can consequently transfer gigabytes of objects the receiver likely already has.

The client already assumes the receiver has the boundary's parents, which are absent from the shallow clone. Extend that option to the boundary itself: push.shallowExcludeBoundary=true adds reachable shallow grafts as negative tips, letting receive-pack's connectivity check reject the push if the assumption is wrong.

Only use grafts reached from refs contributing to the pack. An unrelated graft could otherwise exclude an object another ref needs. Stop at commits known to both sides, since they already bound the pack.

Also accept "abort" to make no assumption, and "false" to retain the historical behavior required when seeding a receive.shallowUpdate receiver. Keep false as the default for now, so introducing the mechanism does not change existing pushes.

---
 Documentation/config/push.adoc |  23 ++++++
 send-pack.c                    | 122 ++++++++++++++++++++++++++++++
 t/t5538-push-shallow.sh        | 131 +++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+)

index 28132eedfe..0ad55965e8 100644
@@ -134,6 +134,29 @@ This will result in only b (a and c are cleared).
 	rely solely on the server's ref advertisement to find commits
 	in common.
 
+`push.shallowExcludeBoundary`::
+	When pushing from a shallow repository, Git can omit the shallow
+	grafts' objects from the generated pack rather than resending the
+	full toplevel tree of those grafts.  This assumes the receiver
+	already has those objects.  If it does not, the receiver rejects
+	the push rather than accepting incomplete history. This setting
+	controls that behavior and accepts three values:
++
+--
+`abort`;;
+	If the push reaches such a boundary, refuse it rather than
+	choosing whether to send or omit it.
+`true`;;
+	Omit the boundary objects (fast). If the receiver does not have
+	them, the push is rejected.
+`false`;;
+	(the default) Send the boundary objects, retaining the historical
+	behavior.  This can send the boundary's entire tree, which may be
+	very large.  This is only needed when pushing to a receiver that
+	accepts new shallow roots (i.e. one with `receive.shallowUpdate`
+	enabled), which is very rare.
+--
+
 `push.useBitmaps`::
 	If set to `false`, disable use of bitmaps for `git push` even if
 	`pack.useBitmaps` is `true`, without preventing other git operations

gitgitgadget

From: Elijah Newren <[email protected]>

Sending a shallow boundary is almost always wasted work. We got the shallow boundary from somewhere, and most likely that is the server we are pushing to. If the receiver has the boundary, omitting it avoids transferring and recompressing its entire tree. If the receiver lacks both it and its history, the push is rejected either way, but omission reaches that answer without first sending the tree.

Make push.shallowExcludeBoundary default to true. This also covers cases where push negotiation is disabled, unavailable, or fails to find the boundary, so users do not need special configuration to avoid unexpectedly huge pushes.

The practical compatibility cost is the rare use of push to seed a new shallow root. That already requires receive.shallowUpdate on the server; it now also requires push.shallowExcludeBoundary=false on the client so the receiver gets the boundary snapshot.

Two other edge cases instead fail faster with the new default:

(A) A receiver has the boundary's parents but not the boundary itself. This likely means the user is pushing to the wrong receiver, where a quick rejection is preferable to a slow accidental success.

(B) In a multi-ref push, one ref's shallow boundary can exclude objects needed by another ref. This may reject more refs than necessary, but retrying the refs separately avoids the problem; the next patch advises users to do so.

Neither case justifies making every ordinary shallow push send the boundary's potentially enormous tree.

---
 Documentation/config/push.adoc | 10 +++++-----
 send-pack.c                    |  2 +-
 t/t5538-push-shallow.sh        | 10 ++++++----
 3 files changed, 12 insertions(+), 10 deletions(-)

index 0ad55965e8..a08ec04c21 100644
@@ -147,12 +147,12 @@ This will result in only b (a and c are cleared).
 	If the push reaches such a boundary, refuse it rather than
 	choosing whether to send or omit it.
 `true`;;
-	Omit the boundary objects (fast). If the receiver does not have
-	them, the push is rejected.
+	(the default) Omit the boundary objects (fast). If the receiver
+	does not have them, the push is rejected.
 `false`;;
-	(the default) Send the boundary objects, retaining the historical
-	behavior.  This can send the boundary's entire tree, which may be
-	very large.  This is only needed when pushing to a receiver that
+	Send the boundary objects, retaining the historical behavior.
+	This can send the boundary's entire tree, which may be very
+	large.  This is only needed when pushing to a receiver that
 	accepts new shallow roots (i.e. one with `receive.shallowUpdate`
 	enabled), which is very rare.
 --

index 386ea8b9a2..8a7cedf65a 100644
@@ -70,7 +70,7 @@ static enum exclude_boundary_mode get_exclude_boundary_mode(struct repository *r
 	const char *value;
 
 	if (repo_config_get_string_tmp(r, "push.shallowexcludeboundary", &value))
-		return EXCLUDE_BOUNDARY_NONE;
+		return EXCLUDE_BOUNDARY_YES;
 
 	switch (git_parse_maybe_bool(value)) {
 	case 1:

index 67db51e60e..e52f3e50e2 100755
@@ -64,7 +64,8 @@ EOF
 test_expect_success 'push from shallow clone, with grafted roots' '
 	(
 	cd shallow2 &&
-	test_must_fail git push ../.git +main:refs/remotes/shallow2/main 2>err &&
+	test_must_fail git -c push.shallowExcludeBoundary=false \
+		push ../.git +main:refs/remotes/shallow2/main 2>err &&
 	test_grep "shallow2/main.*shallow update not allowed" err
 	) &&
 	test_must_fail git rev-parse shallow2/main &&

@@ -75,7 +76,8 @@ test_expect_success 'add new shallow root with receive.updateshallow on' '
 	test_config receive.shallowupdate true &&
 	(
 	cd shallow2 &&
-	git push ../.git +main:refs/remotes/shallow2/main
+	git -c push.shallowExcludeBoundary=false \
+		push ../.git +main:refs/remotes/shallow2/main
 	) &&
 	git log --format=%s shallow2/main >actual &&
 	git fsck &&

@@ -90,7 +92,8 @@ test_expect_success 'push from shallow to shallow' '
 	(
 	cd shallow &&
 	git --git-dir=../shallow2/.git config receive.shallowupdate true &&
-	git push ../shallow2/.git +main:refs/remotes/shallow/main &&
+	git -c push.shallowExcludeBoundary=false \
+		push ../shallow2/.git +main:refs/remotes/shallow/main &&
 	git --git-dir=../shallow2/.git config receive.shallowupdate false
 	) &&
 	(

@@ -227,7 +230,6 @@ test_expect_success 'shallow boundary exclusion avoids sending the full tree' '
 	git -C adv-client checkout -b topic &&
 	test_commit --no-tag -C adv-client new &&
 	GIT_PROGRESS_DELAY=0 git -C adv-client \
-		-c push.shallowExcludeBoundary=true \
 		push --progress origin topic 2>err &&
 
 	# Only the new commit, its tree, and the new blob are sent; sending

gitgitgadget

From: Elijah Newren <[email protected]>

When several refs share a pack, an omitted shallow boundary reached from one ref can exclude an object needed by another. Pushing each ref separately recomputes the pack and avoids that interaction.

When such a multi-ref push fails after excluding a boundary, suggest separate pushes. Gate the message on advice.pushShallowBoundary.

---
 Documentation/config/advice.adoc |  5 +++++
 advice.c                         |  1 +
 advice.h                         |  1 +
 send-pack.c                      | 26 ++++++++++++++++++++++----
 t/t5538-push-shallow.sh          | 31 +++++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 4 deletions(-)

index 81f80a9274..6bb6955246 100644
@@ -99,6 +99,11 @@ all advice messages.
 		a configured remote but looks like a `<remote>/<branch>` ref,
 		suggesting that the remote and branch be given as separate
 		arguments.
+	pushShallowBoundary::
+		Shown when a push from a shallow clone is rejected because
+		the remote could not unpack the pack, hinting that a shallow
+		boundary may have omitted objects and suggesting the refs be
+		pushed one at a time.
 	pushUnqualifiedRefname::
 		Shown when linkgit:git-push[1] gives up trying to
 		guess based on the source and destination refs what

index 63bf8b0c5f..3701672048 100644
@@ -70,6 +70,7 @@ static struct {
 	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching" },
 	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate" },
 	[ADVICE_PUSH_REPO_LOOKS_LIKE_REF]		= { "pushRepoLooksLikeRef" },
+	[ADVICE_PUSH_SHALLOW_BOUNDARY]			= { "pushShallowBoundary" },
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
 	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */

index 66f6cd6a77..b2e281baa5 100644
@@ -37,6 +37,7 @@ enum advice_type {
 	ADVICE_PUSH_NON_FF_MATCHING,
 	ADVICE_PUSH_REF_NEEDS_UPDATE,
 	ADVICE_PUSH_REPO_LOOKS_LIKE_REF,
+	ADVICE_PUSH_SHALLOW_BOUNDARY,
 	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,

index 8a7cedf65a..4fa17810a7 100644
@@ -1,4 +1,5 @@
 #include "git-compat-util.h"
+#include "advice.h"
 #include "config.h"
 #include "commit.h"
 #include "date.h"

@@ -161,7 +162,8 @@ static int append_reachable_shallow_grafts(struct repository *r,
 static int pack_objects(struct repository *r,
 			int fd, struct ref *refs, struct oid_array *advertised,
 			struct oid_array *negotiated,
-			struct send_pack_args *args)
+			struct send_pack_args *args,
+			int *excluded_boundary)
 {
 	struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
 	struct odb_pack_generator *generator;

@@ -191,7 +193,8 @@ static int pack_objects(struct repository *r,
 	/* Exclude reachable shallow boundaries from the pack. */
 	if (is_repository_shallow(r) &&
 	    get_exclude_boundary_mode(r) == EXCLUDE_BOUNDARY_YES)
-		append_reachable_shallow_grafts(r, refs, advertised,
+		*excluded_boundary = append_reachable_shallow_grafts(
+						r, refs, advertised,
 						negotiated, args,
 						&opts.haves);
 

@@ -607,6 +610,8 @@ int send_pack(struct repository *r,
 	int push_options_supported = 0;
 	int object_format_supported = 0;
 	unsigned cmds_sent = 0;
+	int excluded_boundary = 0;
+	int pack_contributing_refs = 0;
 	int ret;
 	struct async demux;
 	char *push_cert_nonce = NULL;

@@ -742,8 +747,10 @@ int send_pack(struct repository *r,
 		default:
 			continue;
 		}
-		if (!ref->deletion)
+		if (!ref->deletion) {
 			need_pack_data = 1;
+			pack_contributing_refs++;
+		}
 
 		if (args->dry_run || !status_report)
 			ref->status = REF_STATUS_OK;

@@ -832,7 +839,8 @@ int send_pack(struct repository *r,
 			   PACKET_READ_DIE_ON_ERR_PACKET);
 
 	if (need_pack_data && cmds_sent) {
-		if (pack_objects(r, out, remote_refs, extra_have, &commons, args) < 0) {
+		if (pack_objects(r, out, remote_refs, extra_have, &commons, args,
+				 &excluded_boundary) < 0) {
 			if (args->stateless_rpc)
 				close(out);
 			if (git_connection_is_socket(conn))

@@ -878,6 +886,16 @@ int send_pack(struct repository *r,
 		}
 	}
 
+	/*
+	 * Per-ref pushes prevent one ref's boundary from excluding objects
+	 * needed by another.
+	 */
+	if (ret < 0 && excluded_boundary && pack_contributing_refs > 1)
+		advise_if_enabled(ADVICE_PUSH_SHALLOW_BOUNDARY,
+			_("A shallow boundary may have excluded objects needed by another ref.\n"
+			  "Try pushing the refs one at a time, e.g.:\n"
+			  "  git push <remote> <ref>"));
+
 	if (ret < 0)
 		goto out;
 

index e52f3e50e2..f2a84eb227 100755
@@ -343,4 +343,35 @@ test_expect_success 'push to a shallowUpdate receiver rejects a rootless snapsho
 	git --git-dir=seed-receiver.git rev-parse --verify seeded
 '
 
+# Splitting a multi-ref push recomputes the pack and avoids exclusions from
+# one ref stripping objects needed by another.
+test_expect_success 'incomplete multi-ref shallow push advises pushing refs separately' '
+	git init hint-origin &&
+	git -C hint-origin checkout -b A &&
+	test_commit -C hint-origin --no-tag has-shared sh shared &&
+	test_commit -C hint-origin --no-tag A1 &&
+	git -C hint-origin switch --orphan B &&
+	test_commit -C hint-origin --no-tag B0 &&
+	test_commit -C hint-origin --no-tag B1 &&
+
+	# Strict checking rejects the incomplete pack before connectivity.
+	git init --bare hint-receiver.git &&
+	git --git-dir=hint-receiver.git config receive.fsckObjects true &&
+	git -C hint-origin push "file://$(pwd)/hint-receiver.git" \
+		B:refs/heads/B B:refs/heads/A &&
+
+	git clone --depth=1 --no-single-branch \
+		"file://$(pwd)/hint-origin" hint-client &&
+
+	git -C hint-client checkout A &&
+	test_commit -C hint-client --no-tag cX &&
+	git -C hint-client checkout -b topic B &&
+	test_commit -C hint-client --no-tag reintroduce sh shared &&
+
+	test_must_fail git -C hint-client \
+		-c push.shallowExcludeBoundary=true \
+		push --force "file://$(pwd)/hint-receiver.git" A topic 2>err &&
+	test_grep "shallow boundary may have excluded objects" err
+'
+
 test_done

gitgitgadget