Re: [PATCH 1/3] t7700: add failing --path-walk test

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 21, 2025, 12:42 UTC
Message-ID: <d9f69281-6aa3-4ef7-b52f-9660bc60a46d@gmail.com>
In-Reply-To: <aKbSObIzXwUtjAdE@pks.im>

On 8/21/2025 4:00 AM, Patrick Steinhardt wrote:

Show 12 quoted lines

> On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>> index 611755cc139b..1998d9bf291c 100755
>> --- a/t/t7700-repack.sh
>> +++ b/t/t7700-repack.sh
>> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
> 
> Tiny nit: I would've probably squashed this patch into the second patch,
> as we usually don't use the add-failing-test-and-then-fix-it-later
> dance. On the other hand though it gives some nice context, so I
> ultimately don't mind it all that much. So please feel free to ignore
> this nit.

I'm probably the person who is always asking folks to create a test that either fails or demonstrates the "before" behavior before making the actual change that updates the case. This allows the ability to "test the test" by checking it in place to confirm that it is indeed failing.

Using test_expect_failure allows us to avoid breaking bisect.

Show 8 quoted lines

>>  	test_server_info_missing
>>  '
>>  
>> +test_expect_failure 'pending objects are repacked appropriately' '
>> +	git init pending &&
> 
> We probably also want `test_when_finished "rm -rf pending"` before
> calling git-init(1).

Good idea.

Show 31 quoted lines

>> +
>> +	(
>> +		cd pending &&
>> +
>> +		mkdir -p a/b &&
>> +		echo singleton >file &&
>> +		echo stuff >a/b/c &&
>> +		echo more >a/d &&
>> +		git add file a &&
>> +		git commit -m "single blobs" &&
>> +
>> +		echo d >a/d &&
>> +		echo e >a/e &&
>> +		git add a &&
>> +		git commit -m "more blobs" &&
>> +
>> +		# This use of a sparse index helps to force
>> +		# test that the cache-tree is walked, too.
>> +		git sparse-checkout set --sparse-index a x &&
>> +
>> +		# Just _stage_ the changes.
>> +		echo f >a/d &&
>> +		echo h >a/e &&
>> +		echo i >a/i &&
>> +		mkdir x &&
>> +		echo y >x/y &&
>> +		git add a x &&
> 
> Nit: I think I would've moved the explanations you have in the commit
> message into these hunks so that the test becomes a bit more
> self-explanatory.

Hm. That seems to go against our typical pattern of leaving comments sparse and having the longer explanation available in commit messages but maybe I'm out of date or tests are a different beast. I'll see what I can do to make the test more self-documented.

Thanks, -Stolee