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

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

From: Elijah Newren <redacted>

When pushing from a shallow clone, even if we only have made a small
one-line change to a tiny file, we often push the entire toplevel tree
of files.  For large repositories, this could be gigabytes instead of
kilobytes.

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

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

The 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