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

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 17, 2021, 20:43 UTC
Message-ID: <028fb838-f012-e8c7-eb8b-53c810802243@gmail.com>
In-Reply-To: <20210317132814.30175-4-avarab@gmail.com>

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:

Show 11 quoted lines

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

Show 22 quoted lines

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

Show 13 quoted 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.

Thanks, -Stolee