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

From: Derrick Stolee <stolee@gmail.com>
Date: May 16, 2025, 15:32 UTC
Message-ID: <f0f1ebed-06af-4d73-9068-a08956f01e48@gmail.com>
In-Reply-To: <aBVWfJUFNdfwS0PO@nand.local>

On 5/2/25 7:34 PM, Taylor Blau wrote:

Show 37 quoted lines

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

Show 10 quoted lines

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

Show 9 quoted lines

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