Re: [PATCH v2 06/13] t5538: add tests to confirm deltas in shallow pushes

[Derrick Stolee <stolee@gmail.com>]

On 5/2/25 7:34 PM, Taylor Blau wrote:
> On Mon, Mar 24, 2025 at 03:22:42PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> It can be notoriously difficult to detect if delta bases are being
>> computed properly during 'git push'. Construct an example where it will
>> make a kilobyte worth of difference when a delta base is not found. We
>> can then use the progress indicators to distinguish between bytes and
>> KiB depending on whether the delta base is found and used.
>>
>> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>> ---
>>   t/t5538-push-shallow.sh | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
>> index e91fcc173e8..11b85cca9e8 100755
>> --- a/t/t5538-push-shallow.sh
>> +++ b/t/t5538-push-shallow.sh
>> @@ -123,4 +123,38 @@ EOF
>>   	git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null
>>   	)
>>   '
>> +
>> +test_expect_success 'push new commit from shallow clone has correct object count' '
>> +	git init origin &&
>> +	test_commit -C origin a &&
>> +	test_commit -C origin b &&
>> +
>> +	git clone --depth=1 "file://$(pwd)/origin" client &&
>> +	git -C client checkout -b topic &&
>> +	git -C client commit --allow-empty -m "empty" &&
>> +	GIT_PROGRESS_DELAY=0 git -C client push --progress origin topic 2>err &&
>> +	test_grep "Enumerating objects: 1, done." err
> 
> Why is a full 'git push' necessary here? Could you instead directly
> invoke pack-objects as git push / send-pack does? That test_grep to
> assert on the size of the pack seems very fragile to me.

Here, I think we _need_ to test at the 'git push' level, since that is the
user behavior we want to protect. Earlier testing using 'git pack-objects'
directly had not discovered this subtle issue in how it was used in the
shallow case.

It does make the test more fragile, but it's testing "the right thing"
instead of locking in an internal detail.

>> +
>> +test_expect_success 'push new commit from shallow clone has good deltas' '
>> +	git init base &&
>> +	test_seq 1 999 >base/a &&
>> +	test_commit -C base initial &&
>> +	git -C base add a &&
>> +	git -C base commit -m "big a" &&
> 
> I don't think it really matters, but you may want to write a test_tick
> here before committing.

I don't understand why. Is the point to get a different commit time?

>> +	test_grep "Enumerating objects: 5, done" err &&
>> +
>> +	# If the delta base is found, then this message uses "bytes".
>> +	# If the delta base is not found, then this message uses "KiB".
>> +	test_grep "Writing objects: .* bytes" err
> 
> If we had the raw pack that was generated, could you use verify-pack
> instead to check that the desired delta/base relationship was
> discovered?

True, we could do that with a lot of effort, but what we really care
about is "the push was small" not "this specific delta base was
picked" (though, the delta base is how we got a small packfile).

Thanks,
-Stolee