Re: [PATCH] fetch: commit references fetched before backfilling tags

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 9, 2026, 11:26 UTC
Message-ID: <asjPWXAO3Cwpkerk@pks.im>
In-Reply-To: <20261009-799-shallow-fetch-with-tags-v1-1-379d61504af5@gmail.com>

On Fri, Oct 09, 2026 at 12:10:37AM +0200, Karthik Nayak wrote:

Show 11 quoted lines

> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
> fetch code was modified to use batched updates to provide a good
> performance improvement. Wherein batched updates were used to fetch both
> references and backfill tags.
> 
> When using batched updates, the references aren't yet committed to disk
> when we start backfilling tags. This means in situations such as shallow
> fetching the negotiation during backfilling tags, the client doesn't
> have any references to report in the 'have' section. Since backfilling
> doesn't use a depth limit, this can cause the server to send all the
> objects present in the repository.

So in my own words: the server sends the reference, we queue them in a transaction, but don't commit it yet. We then try to backfill tags, and because we don't have the refs committed yet the backfill will think we don't have any of the relevant commits that those tags point to. Consequently, the packfile negotiation will result in way more objects being fetched than necessary.

This makes me wonder why we even do a proper fetch. In theory, we could basically just ask the server for the individual tagged objects without performing any negotiation, right?

Or... well, would that work with nested annotated tags? No idea.

Show 17 quoted lines

> Fix this by committing the previous batched update and initiating a new
> one for backfilling tags. Also add a test which captures this regression.
> 
> While this does make it a little slower than master, due to creation of
> two transactions, It is still faster than not using batched updates:
> 
> Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
>   Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
>   Range (min … max):    1.427 s …  1.558 s    10 runs
> 
> Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
>   Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
>   Range (min … max):    81.4 ms …  88.6 ms    29 runs
> 
> Summary
>   fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
>    17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)

I was expecting to also see HEAD~ here to back up your claim that this is a bit slower than master.

Show 28 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2decc6cfd..68b04d0f8a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>  
>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
> +
> +		/*
> +		 * Backfilling tags has no depth limit. If we don't commit
> +		 * the fetched references, the backfill will report no refs
> +		 * in the 'have' section of the negotiation. This can cause
> +		 * the server to send all objects.
> +		 */
> +		if (tags_ref_map && !atomic_fetch) {
> +			retcode |= commit_ref_transaction(&transaction, false,
> +							  transport->remote->name,
> +							  &rejected_refs, &err);
> +
> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +			if (!transaction) {
> +				free_refs(tags_ref_map);
> +				retcode = -1;
> +				goto cleanup;
> +			}
> +		}

Okay, makes sense. `commit_ref_transaction()` knows to already handle the failures for us and print them. And `rejected_refs` is basically being treated additive, so if both transactions have some failures then the map will contain the combined set of rejected refs.

Show 24 quoted lines

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 300bd5396d..81865c1ecc 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1942,6 +1942,23 @@ test_expect_success "backfill tags when providing a refspec" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'shallow fetch does not fetch objects again for tags' '
> +	test_when_finished rm -rf source target trace &&
> +
> +	git init source &&
> +	test_commit_bulk -C source 10 &&
> +	git -C source tag -a tag -m tag HEAD~2 &&
> +	HEAD_OID=$(git -C source rev-parse HEAD) &&
> +
> +	git init target &&
> +	git -C target remote add origin ../source &&
> +	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
> +		git -C target fetch --depth 5 origin &&
> +
> +	test $(grep -c "fetch> command=fetch" trace) -gt 2 &&
> +	test $(grep -c "fetch> have $HEAD_OID" trace) -eq 2
> +'

You don't really verify that we don't re-fetch objects, you only verify that we provide "have" lines to the remote side. Which is ultimately the same, but in a bit more of a roundabout way.

Thanks!

Patrick