From: Karthik Nayak <karthik.188@gmail.com>
Date: Fri, 09 Oct 2026 22:31:29 GMT
Subject: Re: [PATCH] fetch: commit references fetched before backfilling tags
Message-ID: <CAOLa=ZTBFox1K2rxzYT1x+rvJpiYiXLnjqkvummh=J-CFWY0vA@mail.gmail.com>
In-Reply-To: <asjPWXAO3Cwpkerk@pks.im>

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.