[PATCH] fetch: commit references fetched before backfilling tags

[PATCH] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

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.

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)

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799
---
 builtin/fetch.c  | 21 +++++++++++++++++++++
 t/t5510-fetch.sh | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

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;
+			}
+		}
+
 		if (tags_ref_map) {
 			/*
 			 * If backfilling of tags fails then we want to tell
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
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

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

[Patrick Steinhardt]

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

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

[Justin Tobler]

On 26/10/09 01:26PM, Patrick Steinhardt wrote:
> 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.

IIUC, when we backfill tags, we only fetch tags that reference objects
that we have locally. The server advertises the tag reference OID and
its recursively peeled non-tag OID so the client can figure this out:

  efe1aaafb77990c4f023cec81b198e0af55bbfb5	refs/tags/foo
  c8dd1e3bb1152844983558802a52c9e4c17652b4	refs/tags/foo^{}

So because there could be nested annontated tags, I think we would need
to fetch to get the intermediate objects.

-Justin

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> On 26/10/09 01:26PM, Patrick Steinhardt wrote:
>> 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.
>
> IIUC, when we backfill tags, we only fetch tags that reference objects
> that we have locally. The server advertises the tag reference OID and
> its recursively peeled non-tag OID so the client can figure this out:
>
>   efe1aaafb77990c4f023cec81b198e0af55bbfb5	refs/tags/foo
>   c8dd1e3bb1152844983558802a52c9e4c17652b4	refs/tags/foo^{}
>
> So because there could be nested annontated tags, I think we would need
> to fetch to get the intermediate objects.
>
> -Justin

Yup exactly this. Afaik the protocol also works by transferring the set
of objects that form the closure of between have <> want.

So for:

   foo -> [tag A] -> [tag B] -> [commit C]

We cannot simply request for A without saying we have C. We have C and
we know it, but the haves are obtained from the committed refs, so
until refs are written to disk we never advertise C.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

[snip]

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

I had it and removed it thinking it was too much information. Will add
it back in.

[snip]

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

Ah, that's what I wanted and realized the trace only says 'packfile'
without listing whats in it.

Now I realize I could just check the repo with `cat-file`. Let me do
that and clean this up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]