Re: [RFC/PATCH 3/5] ls-files: add and use a new --sparse option

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Mar 17 2021, Derrick Stolee wrote:

> On 3/17/2021 9:28 AM, Ævar Arnfjörð Bjarmason wrote:
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>
> I want to learn from your suggested changes to the test, here,
> so forgive my questions here:
>   
>> +test_index_entry_like () {
>> +	dir=$1
>> +	shift
>> +	fmt=$1
>> +	shift
>> +	rev=$1
>> +	shift
>> +	entry=$1
>> +	shift
>> +	file=$1
>> +	shift
>
> Why all the shifts? Why not just use $1, $2, $3,...? My
> guess is that you want to be able to insert a new parameter
> in the middle in the future without changing the later
> numbers, but that seems unlikely, and we could just add
> the parameter at the end.

It's just crappy RFC-quality code. I probably copied some other function
and went with it. No good reason. Yeah it's ugly.

>> +	hash=$(git -C "$dir" rev-parse "$rev") &&
>> +	printf "$fmt\n" "$hash" "$entry" >expected &&
>> +	if grep "$entry" "$file" >line
>> +	then
>> +		test_cmp expected line
>> +	else
>> +		cat cache &&
>> +		false
>> +	fi
>> +}
>> +
>>  test_expect_success 'sparse-index contents' '
>>  	init_repos &&
>>  
>> -	test-tool -C sparse-index read-cache --table >cache &&
>> +	git -C sparse-index ls-files --sparse >cache &&
>>  	for dir in folder1 folder2 x
>>  	do
>> -		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
>> -		grep "040000 tree $TREE	$dir/" cache \
>> -			|| return 1
>> +		test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>
> I see how this uses only one line, but it seems like the
> test_index_entry_like is too generic to make it not a
> complicated mess of format strings that need to copy
> over and over again.
>
> Perhaps instead it could be a "test_entry_is_tree"
> and it only passes "$dir" and "cache"? Then we could drop the loop and
> just have
>
> 	test_entry_is_tree cache folder1 &&
> 	test_entry_is_tree cache folder2 &&
> 	test_entry_is_tree cache x &&
>
> or we could still use the loop, especially when we test for four trees.

Yeah that sounds good. Personally I don't mind 4x similar lines
copy/pasted over a for-loop in the tests. You don't need to worry about
the || return doing the right thing, and just setting up the for-loop is
already 3 lines...

>> -	test-tool -C sparse-index read-cache --table >cache &&
>> +	git -C sparse-index ls-files --sparse >cache &&
>>  	for dir in deep/deeper2 folder1 folder2 x
>>  	do
>> -		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
>> -		grep "040000 tree $TREE	$dir/" cache \
>> -			|| return 1
>> +		test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>>  	done &&
>>  
>> +	grep 040000 cache >lines &&
>> +	test_line_count = 4 lines &&
>> +
>
> The point here is to check that no other entries are trees? We know
> that this number will be _at least_ 4 based on the loop above.

It's exactly 4 because we have 4 folders we're checking. But you tell
me. I was just trying to refactor this dependence on the ls-tree format
while moving it over to ls-files without spending too much time on
understanding all the specifics.