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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 09, 2026 at 12:10:37AM +0200, Karthik Nayak wrote:
> 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.

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

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

> 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