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:

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.

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.

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:

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:

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

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

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