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

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    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(-)

diff --git a/Documentation/config/push.adoc b/Documentation/config/push.adoc
index 28132eedfe..9fd6a956a8 100644
--- a/Documentation/config/push.adoc
+++ b/Documentation/config/push.adoc
@@ -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
diff --git a/send-pack.c b/send-pack.c
index f20460fbf4..9a035d7403 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -14,6 +14,7 @@
 #include "transport.h"
 #include "version.h"
 #include "oid-array.h"
+#include "oidset.h"
 #include "gpg-interface.h"
 #include "shallow.h"
 #include "parse-options.h"
@@ -55,6 +56,86 @@ 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);
+
+/*
+ * 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.
+ */
+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,
+					    struct oid_array *haves)
+{
+	struct commit_list *pending = NULL;
+	struct oidset seen = OIDSET_INIT;
+	struct oidset known = OIDSET_INIT;
+	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);
+
+	for (ref = refs; ref; ref = ref->next) {
+		struct commit *commit;
+
+		if (is_null_oid(&ref->new_oid))
+			continue;
+		if (check_to_send_update(ref, args))
+			continue;
+		commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
+		if (commit)
+			commit_list_insert(commit, &pending);
+	}
+
+	while (pending) {
+		struct commit *commit = pop_commit(&pending);
+		const struct object_id *oid = &commit->object.oid;
+		struct commit_graft *graft;
+		struct commit_list *parent;
+
+		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;
+
+		graft = lookup_commit_graft(r, oid);
+		if (graft && graft->nr_parent == -1) {
+			append_negative_object(r, haves, oid);
+			continue;
+		}
+
+		if (repo_parse_commit(r, commit))
+			continue;
+		for (parent = commit->parents; parent; parent = parent->next)
+			commit_list_insert(parent->item, &pending);
+	}
+
+	oidset_clear(&seen);
+	oidset_clear(&known);
+}
+
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -88,6 +169,20 @@ 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);
+	}
+
 	while (refs) {
 		if (!is_null_oid(&refs->old_oid))
 			append_negative_object(r, &opts.haves, &refs->old_oid);
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index afab456b32..6b0425bdbc 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -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
 	) &&
 	(
@@ -164,4 +167,151 @@ test_expect_success 'push new commit from shallow clone has good deltas' '
 	test_region pack-objects path-walk config-push.txt
 '
 
+test_expect_success 'shallow push only pushes what is necessary' '
+	git init adv-origin &&
+	# The shallow grafts are intentionally untagged so that no
+	# advertised ref points at them.
+	test_commit --no-tag -C adv-origin a &&
+	test_commit --no-tag -C adv-origin b &&
+
+	git clone --depth=1 "file://$(pwd)/adv-origin" adv-client &&
+
+	# The remote branch advances past the history we have, so its
+	# advertised tip is something we cannot use as a negative tip;
+	# only the shallow graft lets us exclude the full tree.
+	test_commit --no-tag -C adv-origin c &&
+
+	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 &&
+
+	# Only the new commit, its tree, and the new blob are sent; sending
+	# the full tree is avoided by excluding the shallow graft.
+	test_grep "Enumerating objects: 4, done." err
+'
+
+test_expect_success 'push.shallowExcludeBoundary=false sends full tree' '
+	git init adv-origin2 &&
+	test_commit --no-tag -C adv-origin2 a &&
+	test_commit --no-tag -C adv-origin2 b &&
+
+	git clone --depth=1 "file://$(pwd)/adv-origin2" adv-client2 &&
+	test_commit --no-tag -C adv-origin2 c &&
+
+	git -C adv-client2 checkout -b topic &&
+	test_commit --no-tag -C adv-client2 new &&
+	GIT_PROGRESS_DELAY=0 git -C adv-client2 \
+		-c push.shallowExcludeBoundary=false \
+		push --progress origin topic 2>err &&
+
+	# With the optimization disabled and no advertised ref pointing at
+	# the shallow graft, the full snapshot down to the shallow graft is
+	# resent, including its full tree.
+	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 '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 &&
+	test_commit -C tworoot-origin --no-tag A1 &&
+	git -C tworoot-origin switch --orphan B &&
+	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 \
+		"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' '
+	git init seed-origin &&
+	test_commit -C seed-origin s1 &&
+	test_commit -C seed-origin s2 &&
+	test_commit -C seed-origin s3 &&
+
+	# depth-2: a shallow graft at s2, pushing s3 on top of it
+	git clone --depth=2 "file://$(pwd)/seed-origin" seed-client &&
+
+	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 \
+		"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 &&
+
+	# 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 \
+		"file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded &&
+	git --git-dir=seed-receiver.git rev-parse --verify seeded
+'
+
 test_done

base-commit: 96650039a0dff984f3575568aea85af68474f5c3
-- 
gitgitgadget

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

[Patrick Steinhardt]

On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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.

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.

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

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.

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

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

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

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?

> diff --git a/send-pack.c b/send-pack.c
> index f20460fbf4..9a035d7403 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -55,6 +56,86 @@ 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);
> +
> +/*
> + * 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.
> + */
> +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,
> +					    struct oid_array *haves)

Nit: it might make sense to mark those parameters as `const` that are
only used as input.

> +{
> +	struct commit_list *pending = NULL;
> +	struct oidset seen = OIDSET_INIT;
> +	struct oidset known = OIDSET_INIT;
> +	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);

Okay, here we assemble the list of all objects that the remote is
supposed to know about.

> +	for (ref = refs; ref; ref = ref->next) {
> +		struct commit *commit;
> +
> +		if (is_null_oid(&ref->new_oid))
> +			continue;
> +		if (check_to_send_update(ref, args))
> +			continue;
> +		commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
> +		if (commit)
> +			commit_list_insert(commit, &pending);
> +	}

Hm. Why do we loop through the refs twice? Wouldn't it be possible to
combine both loops?

> +	while (pending) {
> +		struct commit *commit = pop_commit(&pending);
> +		const struct object_id *oid = &commit->object.oid;
> +		struct commit_graft *graft;
> +		struct commit_list *parent;
> +
> +		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;

We abort the walk whenever we hit any of the objects in our walk that
the remote supposedly already knows about.

> +		graft = lookup_commit_graft(r, oid);
> +		if (graft && graft->nr_parent == -1) {
> +			append_negative_object(r, haves, oid);
> +			continue;
> +		}

And when hitting a graft we explicitly add that graf to the negative
objects, too, so that we include the graft itself and its tree.
Logic-wise this make sense, pending the above questions around whether a
graft can be modified locally.

> +		if (repo_parse_commit(r, commit))
> +			continue;
> +		for (parent = commit->parents; parent; parent = parent->next)
> +			commit_list_insert(parent->item, &pending);
> +	}
> +
> +	oidset_clear(&seen);
> +	oidset_clear(&known);
> +}

Instead of doing a manual walk like this, shouldn't we use higher-level
interfaces like `repo_is_descendant_of()` that can make use of commit
graphs? That might be overkill though as we can assume that in most
shallow repositories we won't have deep commit history anyway.

I guess the answer is "no" though, as you don't only want to check
reachability, but also whether any commit in between is part of the
commits that either we or the server has advertised.

Thanks!

Patrick

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

[Elijah Newren]

On Fri, Aug 21, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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.
>
> 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.

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.

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

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.

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

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.

> [snip]
> > 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.
>
> 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.

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.

> [snip]
> >     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.
>
> 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?

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.

> > +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,
> > +                                         struct oid_array *haves)
>
> Nit: it might make sense to mark those parameters as `const` that are
> only used as input.

Good point; will fix.

> > +     for (ref = refs; ref; ref = ref->next)
> > +             if (!is_null_oid(&ref->old_oid))
> > +                     oidset_insert(&known, &ref->old_oid);
>
> Okay, here we assemble the list of all objects that the remote is
> supposed to know about.
>
> > +     for (ref = refs; ref; ref = ref->next) {
> > +             struct commit *commit;
> > +
> > +             if (is_null_oid(&ref->new_oid))
> > +                     continue;
> > +             if (check_to_send_update(ref, args))
> > +                     continue;
> > +             commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
> > +             if (commit)
> > +                     commit_list_insert(commit, &pending);
> > +     }
>
> Hm. Why do we loop through the refs twice? Wouldn't it be possible to
> combine both loops?

Oops, good catch.  Will fix.

> Instead of doing a manual walk like this, shouldn't we use higher-level
> interfaces like `repo_is_descendant_of()` that can make use of commit
> graphs? That might be overkill though as we can assume that in most
> shallow repositories we won't have deep commit history anyway.
>
> I guess the answer is "no" though, as you don't only want to check
> reachability, but also whether any commit in between is part of the
> commits that either we or the server has advertised.

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.

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

[Elijah Newren]

On Fri, Aug 21, 2026 at 10:36 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Fri, Aug 21, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> > > From: Elijah Newren <newren@gmail.com>
> > >
> > > 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.
> >
> > 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.
>
> 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.

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.

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

[Patrick Steinhardt]

On Fri, Aug 21, 2026 at 10:36:04AM -0700, Elijah Newren wrote:
> On Fri, Aug 21, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Aug 21, 2026 at 06:55:51AM +0000, Elijah Newren via GitGitGadget wrote:
> > > From: Elijah Newren <newren@gmail.com>
> > >
> > > 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.
> >
> > 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.
> 
> 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.

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.

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

That makes sense.

> > [snip]
> > >     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.
> >
> > 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?
> 
> 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.

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

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

[Elijah Newren]

On Sun, Aug 23, 2026 at 10:30 PM Patrick Steinhardt <ps@pks.im> wrote:
>
[...]
> TIL, thanks. I don't think I was even aware of "push.negotiate", and I
> mostly went by the folklore of "just clone with --depth=2" that I saw
> repeated on many sites.
>
> But this and all of your other answers make me lean strongly into the
> direction that the fix is at the wrong level, and the proper fix really
> is to enable "push.negotiate" by default.

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

  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.

[...]
> > 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.
>
> That makes sense.

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.

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

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.

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    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(-)

diff --git a/Documentation/config/push.adoc b/Documentation/config/push.adoc
index 28132eedfe..9fd6a956a8 100644
--- a/Documentation/config/push.adoc
+++ b/Documentation/config/push.adoc
@@ -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
diff --git a/send-pack.c b/send-pack.c
index f20460fbf4..5e1ac9dd89 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -14,6 +14,7 @@
 #include "transport.h"
 #include "version.h"
 #include "oid-array.h"
+#include "oidset.h"
 #include "gpg-interface.h"
 #include "shallow.h"
 #include "parse-options.h"
@@ -55,6 +56,91 @@ 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);
+
+/*
+ * 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.
+ */
+static void append_reachable_shallow_grafts(struct repository *r,
+					    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;
+	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]);
+
+	/*
+	 * 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))
+			continue;
+		commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
+		if (commit)
+			commit_list_insert(commit, &pending);
+	}
+
+	while (pending) {
+		struct commit *commit = pop_commit(&pending);
+		const struct object_id *oid = &commit->object.oid;
+		struct commit_graft *graft;
+		struct commit_list *parent;
+
+		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;
+
+		graft = lookup_commit_graft(r, oid);
+		if (graft && graft->nr_parent == -1) {
+			append_negative_object(r, haves, oid);
+			continue;
+		}
+
+		if (repo_parse_commit(r, commit))
+			continue;
+		for (parent = commit->parents; parent; parent = parent->next)
+			commit_list_insert(parent->item, &pending);
+	}
+
+	oidset_clear(&seen);
+	oidset_clear(&known);
+}
+
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -88,6 +174,20 @@ 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);
+	}
+
 	while (refs) {
 		if (!is_null_oid(&refs->old_oid))
 			append_negative_object(r, &opts.haves, &refs->old_oid);
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index afab456b32..6b0425bdbc 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -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
 	) &&
 	(
@@ -164,4 +167,151 @@ test_expect_success 'push new commit from shallow clone has good deltas' '
 	test_region pack-objects path-walk config-push.txt
 '
 
+test_expect_success 'shallow push only pushes what is necessary' '
+	git init adv-origin &&
+	# The shallow grafts are intentionally untagged so that no
+	# advertised ref points at them.
+	test_commit --no-tag -C adv-origin a &&
+	test_commit --no-tag -C adv-origin b &&
+
+	git clone --depth=1 "file://$(pwd)/adv-origin" adv-client &&
+
+	# The remote branch advances past the history we have, so its
+	# advertised tip is something we cannot use as a negative tip;
+	# only the shallow graft lets us exclude the full tree.
+	test_commit --no-tag -C adv-origin c &&
+
+	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 &&
+
+	# Only the new commit, its tree, and the new blob are sent; sending
+	# the full tree is avoided by excluding the shallow graft.
+	test_grep "Enumerating objects: 4, done." err
+'
+
+test_expect_success 'push.shallowExcludeBoundary=false sends full tree' '
+	git init adv-origin2 &&
+	test_commit --no-tag -C adv-origin2 a &&
+	test_commit --no-tag -C adv-origin2 b &&
+
+	git clone --depth=1 "file://$(pwd)/adv-origin2" adv-client2 &&
+	test_commit --no-tag -C adv-origin2 c &&
+
+	git -C adv-client2 checkout -b topic &&
+	test_commit --no-tag -C adv-client2 new &&
+	GIT_PROGRESS_DELAY=0 git -C adv-client2 \
+		-c push.shallowExcludeBoundary=false \
+		push --progress origin topic 2>err &&
+
+	# With the optimization disabled and no advertised ref pointing at
+	# the shallow graft, the full snapshot down to the shallow graft is
+	# resent, including its full tree.
+	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 '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 &&
+	test_commit -C tworoot-origin --no-tag A1 &&
+	git -C tworoot-origin switch --orphan B &&
+	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 \
+		"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' '
+	git init seed-origin &&
+	test_commit -C seed-origin s1 &&
+	test_commit -C seed-origin s2 &&
+	test_commit -C seed-origin s3 &&
+
+	# depth-2: a shallow graft at s2, pushing s3 on top of it
+	git clone --depth=2 "file://$(pwd)/seed-origin" seed-client &&
+
+	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 \
+		"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 &&
+
+	# 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 \
+		"file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded &&
+	git --git-dir=seed-receiver.git rev-parse --verify seeded
+'
+
 test_done

base-commit: 5176dd3d057ac5cae8321508febef61fa88537aa
-- 
gitgitgadget

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

[Derrick Stolee]

On 8/25/2026 3:06 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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 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.

> +static int check_to_send_update(const struct ref *ref, const struct send_pack_args *args);
> +
> +/*
> + * 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.

This "which are reachable from the refs being pushed" is the key problem,
I think. We need to verify that the shallow commits are reachable from
the refs advertised by the remote.

> + * 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.
> + */
> +static void append_reachable_shallow_grafts(struct repository *r,
> +					    const struct ref *refs,
> +					    const struct oid_array *advertised,
> +					    const struct oid_array *negotiated,
> +					    const struct send_pack_args *args,
> +					    struct oid_array *haves)

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

These new tests are great:

> +test_expect_success 'shallow push only pushes what is necessary' '
> +test_expect_success 'push.shallowExcludeBoundary=false sends full tree' '
> +test_expect_success 'shallow push does not over-exclude for an accepted ref via a rejected one' '

This test that you are adding is hinting at some of this behavior of the
new test I added, except the multi-ref push causes unexpected behavior:

> +# 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' '
> +	git init seed-origin &&
> +	test_commit -C seed-origin s1 &&
> +	test_commit -C seed-origin s2 &&
> +	test_commit -C seed-origin s3 &&
> +
> +	# depth-2: a shallow graft at s2, pushing s3 on top of it
> +	git clone --depth=2 "file://$(pwd)/seed-origin" seed-client &&
> +
> +	git init --bare seed-receiver.git &&
> +	git --git-dir=seed-receiver.git config receive.shallowUpdate true &&
> +

Here is the chunk that doesn't work with the push.negotiate approach:

> +	# 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 \
> +		"file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded 2>err &&
> +	test_grep "remote rejected" err &&

but specifically it's because the remote doesn't reject it. The client
makes the appropriate adjustment.

> +	test_must_fail git --git-dir=seed-receiver.git rev-parse --verify seeded &&
> +
> +	# 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 \
> +		"file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded &&
> +	git --git-dir=seed-receiver.git rev-parse --verify seeded
> +'
So the diff on your test becomes

-       # 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 \
-               "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 &&
-
-       # 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
 '

Thanks,
-Stolee

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

[Derrick Stolee]

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

On 8/25/2026 1:00 AM, Elijah Newren wrote:
> On Sun, Aug 23, 2026 at 10:30 PM Patrick Steinhardt <ps@pks.im> wrote:
>>
> [...]
>> TIL, thanks. I don't think I was even aware of "push.negotiate", and I
>> mostly went by the folklore of "just clone with --depth=2" that I saw
>> repeated on many sites.
>>
>> But this and all of your other answers make me lean strongly into the
>> direction that the fix is at the wrong level, and the proper fix really
>> is to enable "push.negotiate" by default.
> 
> I don't think that fixes the problem, though:

You are right that the following cases are somewhat common.

>   a) Users can do a shallow clone of a specific branch for a specific
> pull-request/merge-request.  Then the pull-request/merge-request is
> rebased, and sensitive data removed due to a leaked secret.  The
> shallow graft is no longer common.  Pushing from the shallow clone
> should fail, but it shouldn't have to send several gigabytes of data
> in order to get the failure message.
>   b) (Very similar to a) Users can do a shallow clone of one repo (a
> local repository cache?) and then push to another; the shallow graft
> thus may not be common.  An error is expected, but sending gigabytes
> of data to get the error isn't.

For this case (b) I can think of it as doing a shallow clone of a
base repo (https://github.com/git/git) and then needing to push to
a user-owned fork (https://github.com/derrickstolee/git) and the
fork not advertising reachability to the shallow commit.

I think the difficulties here is that your approach is assuming
something about how "non-advertised" objects may exist due to either

 a) delayed garbage collection, or
 b) shared object databases across a fork network.

I don't think these are reasonable assumptions to have by default,
so we need to be really clear about the reason to use this setting.

As your test demonstrates, some amount of "our assumption was wrong"
is built in, so we should have a way for users to respond quickly
or automatically (retry without the setting?).

The multi-push case that I brought up is tricky, though. It may
be very narrow, and HTTP servers would be protected, but we should
avoid allowing corruption over file:// protocol.

Thanks,
-Stolee

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

[Elijah Newren]

On Wed, Sep 2, 2026 at 12:05 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> Sorry that I missed this portion of the discussion talking about
> push.negotiate. Coming back to correct that.
>
> On 8/25/2026 1:00 AM, Elijah Newren wrote:
> > On Sun, Aug 23, 2026 at 10:30 PM Patrick Steinhardt <ps@pks.im> wrote:
> >>
> > [...]
> >> TIL, thanks. I don't think I was even aware of "push.negotiate", and I
> >> mostly went by the folklore of "just clone with --depth=2" that I saw
> >> repeated on many sites.
> >>
> >> But this and all of your other answers make me lean strongly into the
> >> direction that the fix is at the wrong level, and the proper fix really
> >> is to enable "push.negotiate" by default.
> >
> > I don't think that fixes the problem, though:
>
> You are right that the following cases are somewhat common.
>
> >   a) Users can do a shallow clone of a specific branch for a specific
> > pull-request/merge-request.  Then the pull-request/merge-request is
> > rebased, and sensitive data removed due to a leaked secret.  The
> > shallow graft is no longer common.  Pushing from the shallow clone
> > should fail, but it shouldn't have to send several gigabytes of data
> > in order to get the failure message.
> >   b) (Very similar to a) Users can do a shallow clone of one repo (a
> > local repository cache?) and then push to another; the shallow graft
> > thus may not be common.  An error is expected, but sending gigabytes
> > of data to get the error isn't.

I personally think (d) which you snipped out, namely
push.negotiate=true doesn't work for some users/servers, may be more
common.  I know you, Patrick, and I were all hoping that
push.negotiate=true would be a panacea for the common case, but the
conditions behind (d) that prevent that option from working for some
users would seem to be more common to me than these two conditions.

Further, my previous list for (d) was incomplete...

push.negotiate=true can fail in another case both under http and ssh:
  - repack replaces packfiles on the server with a new packfile.
  - The client points to the shallow-graft as something it has.
  - The server looks up that commit ID with QUICK, losing the race
with repacking, and reports it doesn't have it.
  - The client doesn't have any more history further back so it can't
find any more shared history.
  - Under current versions of git, the client believes it has to send
_everything_ it has.

In the concurrent-repack discussion, upload-pack's QUICK "have" check
was deemed working-as-intended, on the grounds that a dropped "have"
just means "the client is sent more than it needs." For a shallow
clone that "bit more" is the whole history the client has, which is
exactly the problem this patch fixes.  I'm not trying to reopen that
other discussion, and I admit this race is rare, but when it triggers,
it'll defeat push.negotiate=true.  I think we need a backstop.  (And
even if we do revisit that QUICK race, there's still the other
conditions in my previous email under which push.negotiate=true
fails.)

[1] https://lore.kernel.org/git/20260827055743.GB189659@coredump.intra.peff.net/

> For this case (b) I can think of it as doing a shallow clone of a
> base repo (https://github.com/git/git) and then needing to push to
> a user-owned fork (https://github.com/derrickstolee/git) and the
> fork not advertising reachability to the shallow commit.

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

> I think the difficulties here is that your approach is assuming
> something about how "non-advertised" objects may exist due to either
>
>  a) delayed garbage collection, or
>  b) shared object databases across a fork network.
>
> I don't think these are reasonable assumptions to have by default,
> so we need to be really clear about the reason to use this setting.

I don't follow.

A shallow push already assumes something about how "non-advertised"
objects may exist -- it assumes the *parents* of the shallow graft
exist on the server.  Why is it such a big leap to move from assuming
the server has the parents of the shallow graft to assuming it has the
shallow graft itself?  Further, what are the consequences of assuming
or not assuming the shallow graft exists?

Here's the matrix:

Assume the shallow graft exists:
  (A) and it does -> push succeeds, and does so orders of magnitude
faster in large repos
  (B) but it doesn't, nor does its parents -> push fails with error
message we would have gotten anyway, and does so dramatically faster
  (C) but it doesn't, but its parents (magically) do -> sends an error
message quickly, where the push would have (eventually) previously
succeeded

Assume the shallow graft doesn't exist:
  (D) but it does -> push succeeds, AFTER pushing hundreds of
megabytes of almost certainly unnecessary data
  (E) and it doesn't, nor does its parents -> get back an error
message, AFTER pushing hundreds of megabytes of unnecessary data
  (F) and it doesn't, but its parents (magically) do -> push succeeds,
AFTER pushing hundreds of megabytes of mostly unnecessary data since
we can't determine which parts are necessary

Clearly, (A) and (B) are vastly superior to (D) and (E).  The only
case in question then is (C) vs (F).  My opinions there:

(1) We already generally require folks to push from shallow clones
back to repositories that have the parents of the shallow graft and
extending that requirement to the shallow graft itself does not seem
unreasonable to me.  I would much rather be told I'm pushing to the
wrong remote than wait forever.
(2) case C/F is incredibly unlikely (people tend to push back to the
same server, and even if they don't, the server likely either has the
shallow graft and its history or is missing the parents of the graft
as well).

> As your test demonstrates, some amount of "our assumption was wrong"
> is built in, so we should have a way for users to respond quickly
> or automatically (retry without the setting?).
>
> The multi-push case that I brought up is tricky, though. It may
> be very narrow, and HTTP servers would be protected, but we should
> avoid allowing corruption over file:// protocol.

Ah!  I see where the disconnect may have been.  Yeah, corruption needs
to be prevented, and if corruption was a risk then it'd override other
concerns.  But that isn't relevant here: receive-pack checks for
connectivity (regardless of protocol -- http, ssh, or file) and fails
the push if objects are missing.  (See commit 52fed6e1ce07
(receive-pack: check connectivity before concluding "git push",
2011-09-02)).  The multi-push case then ends up being a case of us
failing more refs than necessary, not a way to induce corruption.


Also, I didn't state this earlier, but this bug can actually be more
comical.  If someone clones with e.g. `git clone --depth ${N}
--filter=blob:none --sparse ...`, then after making their changes and
deciding to push and the server no longer has references to one of the
commits in our shallow clone:
  (i) Push notes that it knows no objects the server has -> it needs
to send ALL trees and blobs from the shallow graft
  (ii) It doesn't have ALL blobs from the shallow graft -> promisor
remote handling kicks in
  (iii) promisor remote downloads ALL blobs from the shallow graft
from our origin
  (iv) push can now push all objects to our origin

Above, you'll note that although the user started with a tiny clone,
step (iii) downloads huge amounts of data from origin so that step
(iv) can upload that "necessary" data back to the server it just
downloaded it from.  My patch avoids both the unnecessary huge
download and upload.

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

[Elijah Newren]

Hi Stolee,

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

[PATCH v3 0/6] send-pack: avoid sending the whole tree when pushing from a shallow clone

[Elijah Newren via GitGitGadget]

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 <newren@gmail.com>
      
       ## 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.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ 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

[PATCH v3 1/6] unpack-objects: distinguish missing objects from type mismatches

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/unpack-objects.c        | 9 +++++++--
 t/t5504-fetch-receive-strict.sh | 7 +++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 351948724a..ceefeb5a49 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -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;
 	}
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 75b2b87999..0848e2da4a 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -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

[PATCH v3 2/6] receive-pack: avoid repeating connectivity errors

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/receive-pack.c  | 7 +++++++
 t/t5410-receive-pack.sh | 6 ++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e6e54ba55f..8079901bb6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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;
 
diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
index 09d6bfd2a1..20d221044f 100755
--- a/t/t5410-receive-pack.sh
+++ b/t/t5410-receive-pack.sh
@@ -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

[PATCH v3 3/6] shallow: reject missing boundaries without disconnecting

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 shallow.c               | 16 +++++++++++---
 t/t5538-push-shallow.sh | 46 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/shallow.c b/shallow.c
index 8e244a5669..c6f7437022 100644
--- a/shallow.c
+++ b/shallow.c
@@ -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)
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index afab456b32..10ca7833d8 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -164,4 +164,50 @@ test_expect_success 'push new commit from shallow clone has good deltas' '
 	test_region pack-objects path-walk config-push.txt
 '
 
+test_expect_success 'incomplete shallow push rejects without disconnecting' '
+	git init raw-origin &&
+	git -C raw-origin checkout -b A &&
+	test_commit -C raw-origin --no-tag has-shared sh shared &&
+	test_commit -C raw-origin --no-tag A1 &&
+	A1=$(git -C raw-origin rev-parse HEAD) &&
+	git -C raw-origin switch --orphan B &&
+	test_commit -C raw-origin --no-tag B0 &&
+	test_commit -C raw-origin --no-tag B1 &&
+	B1=$(git -C raw-origin rev-parse HEAD) &&
+
+	git init --bare raw-receiver.git &&
+	git -C raw-receiver.git config receive.fsckObjects false &&
+	git -C raw-origin push ../raw-receiver.git \
+		B:refs/heads/B B:refs/heads/A &&
+
+	git -C raw-origin checkout A &&
+	test_commit -C raw-origin --no-tag cX &&
+	cX=$(git -C raw-origin rev-parse HEAD) &&
+	git -C raw-origin checkout -b topic B &&
+	test_commit -C raw-origin --no-tag reintroduce sh shared &&
+	topic=$(git -C raw-origin rev-parse HEAD) &&
+
+	# Declare A1 and B1 as shallow, but omit them and their objects from
+	# the pack. This mimics an incomplete shallow push without relying on
+	# send-pack to create one.
+	{
+		printf "shallow %s\nshallow %s\n" "$A1" "$B1" |
+		packetize &&
+		printf "%s %s refs/heads/A\0report-status object-format=%s\n" \
+			"$B1" "$cX" "$(test_oid algo)" |
+		packetize_raw &&
+		printf "%s %s refs/heads/topic\n" "$ZERO_OID" "$topic" |
+		packetize &&
+		printf 0000 &&
+		printf "%s\n%s\n^%s\n^%s\n" "$cX" "$topic" "$A1" "$B1" |
+		git -C raw-origin pack-objects --stdout --revs
+	} >input &&
+
+	git receive-pack raw-receiver.git <input >out 2>err &&
+	depacketize <out >out.raw &&
+	test_grep "ng refs/heads/A missing necessary objects" out.raw &&
+	test_grep "ng refs/heads/topic missing necessary objects" out.raw &&
+	test_grep ! "unable to parse commit" err
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 4/6] send-pack: optionally omit shallow boundaries

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/config/push.adoc |  23 ++++++
 send-pack.c                    | 122 ++++++++++++++++++++++++++++++
 t/t5538-push-shallow.sh        | 131 +++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+)

diff --git a/Documentation/config/push.adoc b/Documentation/config/push.adoc
index 28132eedfe..0ad55965e8 100644
--- a/Documentation/config/push.adoc
+++ b/Documentation/config/push.adoc
@@ -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
diff --git a/send-pack.c b/send-pack.c
index f20460fbf4..386ea8b9a2 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -14,6 +14,7 @@
 #include "transport.h"
 #include "version.h"
 #include "oid-array.h"
+#include "oidset.h"
 #include "gpg-interface.h"
 #include "shallow.h"
 #include "parse-options.h"
@@ -55,6 +56,105 @@ 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);
+
+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);
+	}
+}
+
+/*
+ * 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 int append_reachable_shallow_grafts(struct repository *r,
+					    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;
+	const struct ref *ref;
+	int found = 0;
+	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]);
+
+	/* Populate "known" fully before starting the walk. */
+	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))
+			continue;
+		commit = lookup_commit_reference_gently(r, &ref->new_oid, 1);
+		if (commit)
+			commit_list_insert(commit, &pending);
+	}
+
+	while (pending) {
+		struct commit *commit = pop_commit(&pending);
+		const struct object_id *oid = &commit->object.oid;
+		struct commit_graft *graft;
+		struct commit_list *parent;
+
+		if (oidset_insert(&seen, oid))
+			continue;
+
+		if (oidset_contains(&known, oid) &&
+		    odb_has_object(r->objects, oid, 0))
+			continue;
+
+		graft = lookup_commit_graft(r, oid);
+		if (graft && graft->nr_parent == -1) {
+			append_negative_object(r, haves, oid);
+			found++;
+			continue;
+		}
+
+		if (repo_parse_commit(r, commit))
+			continue;
+		for (parent = commit->parents; parent; parent = parent->next)
+			commit_list_insert(parent->item, &pending);
+	}
+
+	oidset_clear(&seen);
+	oidset_clear(&known);
+	return found;
+}
+
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -88,6 +188,13 @@ 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]);
 
+	/* 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);
@@ -644,6 +751,21 @@ 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);
 
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index 10ca7833d8..67db51e60e 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -210,4 +210,135 @@ test_expect_success 'incomplete shallow push rejects without disconnecting' '
 	test_grep ! "unable to parse commit" err
 '
 
+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.
+	test_commit --no-tag -C adv-origin a &&
+	test_commit --no-tag -C adv-origin b &&
+
+	git clone --depth=1 "file://$(pwd)/adv-origin" adv-client &&
+
+	# The remote branch advances past the history we have, so its
+	# advertised tip is something we cannot use as a negative tip;
+	# only the shallow graft lets us exclude the full tree.
+	test_commit --no-tag -C adv-origin c &&
+
+	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
+	# the full tree is avoided by excluding the shallow graft.
+	test_grep "Enumerating objects: 4, done." err
+'
+
+test_expect_success 'push.shallowExcludeBoundary=false sends full tree' '
+	git init adv-origin2 &&
+	test_commit --no-tag -C adv-origin2 a &&
+	test_commit --no-tag -C adv-origin2 b &&
+
+	git clone --depth=1 "file://$(pwd)/adv-origin2" adv-client2 &&
+	test_commit --no-tag -C adv-origin2 c &&
+
+	git -C adv-client2 checkout -b topic &&
+	test_commit --no-tag -C adv-client2 new &&
+	GIT_PROGRESS_DELAY=0 git -C adv-client2 \
+		-c push.shallowExcludeBoundary=false \
+		push --progress origin topic 2>err &&
+
+	# With the optimization disabled and no advertised ref pointing at
+	# the shallow graft, the full snapshot down to the shallow graft is
+	# resent, including its full tree.
+	test_grep "Enumerating objects: 7, done." err
+'
+
+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' '
+	git init tworoot-origin &&
+	git -C tworoot-origin checkout -b A &&
+	test_commit -C tworoot-origin --no-tag has-shared sh shared &&
+	test_commit -C tworoot-origin --no-tag A1 &&
+	git -C tworoot-origin switch --orphan B &&
+	test_commit -C tworoot-origin --no-tag B0 &&
+	test_commit -C tworoot-origin --no-tag B1 &&
+
+	git init --bare tworoot-receiver.git &&
+	git -C tworoot-origin push "file://$(pwd)/tworoot-receiver.git" \
+		B:refs/heads/B B:refs/heads/A &&
+
+	git clone --depth=1 --no-single-branch \
+		"file://$(pwd)/tworoot-origin" tworoot-client &&
+
+	git -C tworoot-client checkout A &&
+	test_commit -C tworoot-client --no-tag cX &&
+
+	git -C tworoot-client checkout -b topic B &&
+	test_commit -C tworoot-client --no-tag reintroduce sh shared &&
+
+	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
+'
+
+# 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 &&
+	test_commit -C seed-origin s3 &&
+
+	# depth-2: a shallow graft at s2, pushing s3 on top of it
+	git clone --depth=2 "file://$(pwd)/seed-origin" seed-client &&
+
+	git init --bare seed-receiver.git &&
+	git --git-dir=seed-receiver.git config receive.shallowUpdate true &&
+
+	# 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 &&
+
+	# 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 \
+		"file://$(pwd)/seed-receiver.git" HEAD:refs/heads/seeded &&
+	git --git-dir=seed-receiver.git rev-parse --verify seeded
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 5/6] send-pack: default to excluding shallow boundaries

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/config/push.adoc | 10 +++++-----
 send-pack.c                    |  2 +-
 t/t5538-push-shallow.sh        | 10 ++++++----
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/push.adoc b/Documentation/config/push.adoc
index 0ad55965e8..a08ec04c21 100644
--- a/Documentation/config/push.adoc
+++ b/Documentation/config/push.adoc
@@ -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.
 --
diff --git a/send-pack.c b/send-pack.c
index 386ea8b9a2..8a7cedf65a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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:
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index 67db51e60e..e52f3e50e2 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -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

[PATCH v3 6/6] send-pack: advise splitting incomplete shallow pushes

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Assisted-by: Claude Opus 4.8
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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(-)

diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
index 81f80a9274..6bb6955246 100644
--- a/Documentation/config/advice.adoc
+++ b/Documentation/config/advice.adoc
@@ -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
diff --git a/advice.c b/advice.c
index 63bf8b0c5f..3701672048 100644
--- a/advice.c
+++ b/advice.c
@@ -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 */
diff --git a/advice.h b/advice.h
index 66f6cd6a77..b2e281baa5 100644
--- a/advice.h
+++ b/advice.h
@@ -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,
diff --git a/send-pack.c b/send-pack.c
index 8a7cedf65a..4fa17810a7 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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;
 
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index e52f3e50e2..f2a84eb227 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -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