Re: [PATCH v8 2/2] diff-files: integrate with sparse index

From: Victoria Dye <vdye@github.com>
To: Shuqi Liang <cheskaqiqi@gmail.com>, git@vger.kernel.org
Cc: gitster@pobox.com, derrickstolee@github.com
Subject: Re: [PATCH v8 2/2] diff-files: integrate with sparse index
Date: Mon, 1 May 2023 15:26:51 -0700	[thread overview]
Message-ID: <427dff83-536d-46ee-6326-15e2f548082c@github.com> (raw)
In-Reply-To: <20230423010721.1402736-3-cheskaqiqi@gmail.com>

Shuqi Liang wrote:
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 3c140103c5..7ebcfe785e 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1377,7 +1377,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '
>  	! test_region index ensure_full_index trace2.txt
>  '
>  
> -ensure_not_expanded () {
> +ensure_index_state () {
> +	local expected_expansion="$1"
> +	shift
> +
>  	rm -f trace2.txt &&
>  	if test -z "$WITHOUT_UNTRACKED_TXT"
>  	then
> @@ -1398,7 +1401,21 @@ ensure_not_expanded () {
>  			>sparse-index-out \
>  			2>sparse-index-error || return 1
>  	fi &&
> -	test_region ! index ensure_full_index trace2.txt
> +
> +	if [ "$expected_expansion" = "expanded" ]
> +	then
> +		test_region index ensure_full_index trace2.txt
> +	else
> +		test_region ! index ensure_full_index trace2.txt
> +	fi
> +}
> +
> +ensure_expanded () {
> +	ensure_index_state "expanded" "$@"
> +}
> +
> +ensure_not_expanded () {
> +	ensure_index_state "not_expanded" "$@"
>  }

This still seems a bit more complicated than necessary (mainly due to the
new string comparison & local arg). What about something like this (applied
on top)?

-------- 8< -------- 8< -------- 8< --------

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 9d11d28891..333822f322 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,10 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_index_state () {
-	local expected_expansion="$1"
-	shift
-
+run_sparse_index_trace2 () {
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1400,22 +1397,17 @@ ensure_index_state () {
 			git -C sparse-index "$@" \
 			>sparse-index-out \
 			2>sparse-index-error || return 1
-	fi &&
-
-	if [ "$expected_expansion" = "expanded" ]
-	then
-		test_region index ensure_full_index trace2.txt
-	else
-		test_region ! index ensure_full_index trace2.txt
 	fi
 }
 
 ensure_expanded () {
-	ensure_index_state "expanded" "$@"
+	run_sparse_index_trace2 "$@" &&
+	test_region index ensure_full_index trace2.txt
 }
 
 ensure_not_expanded () {
-	ensure_index_state "not_expanded" "$@"
+	run_sparse_index_trace2 "$@" &&
+	test_region ! index ensure_full_index trace2.txt
 }
 
 test_expect_success 'sparse-index is not expanded' '
-------- >8 -------- >8 -------- >8 --------

That said, given that this is my only complaint with this iteration (and
it's pretty subjective), if others are happy with it then I'm not opposed to
merging to 'next'.

