[v3,1/8] t7519: rewrite sparse index test

Commit Message

Derrick Stolee Aug. 17, 2021, 1:23 p.m. UTC

From: Derrick Stolee <[e-mail]>

The sparse index is tested with the FS Monitor hook and extension since
f8fe49e (fsmonitor: integrate with sparse index, 2021-07-14). This test
was very fragile because it shared an index across sparse and non-sparse
behavior. Since that expansion and contraction could cause the index to
lose its FS Monitor bitmap and token, behavior is fragile to changes in
'git sparse-checkout set'.

Rewrite the test to use two clones of the original repo: full and
sparse. This allows us to also keep the test files (actual, expect,
trace2.txt) out of the repos we are testing with 'git status'.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 t/t7519-status-fsmonitor.sh | 38 ++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Comments

Johannes Schindelin Aug. 19, 2021, 7:45 a.m. UTC | #1

Hi Stolee,

On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <[e-mail]>
>
> The sparse index is tested with the FS Monitor hook and extension since
> f8fe49e (fsmonitor: integrate with sparse index, 2021-07-14). This test
> was very fragile because it shared an index across sparse and non-sparse
> behavior. Since that expansion and contraction could cause the index to
> lose its FS Monitor bitmap and token, behavior is fragile to changes in
> 'git sparse-checkout set'.
>
> Rewrite the test to use two clones of the original repo: full and
> sparse. This allows us to also keep the test files (actual, expect,
> trace2.txt) out of the repos we are testing with 'git status'.

Makes sense.

It also should accelerate the test, as it does not have to convert between
sparse and full index all the time.

>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  t/t7519-status-fsmonitor.sh | 38 ++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> index deea88d4431..6f2cf306f66 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -389,43 +389,47 @@ test_expect_success 'status succeeds after staging/unstaging' '
>  # If "!" is supplied, then we verify that we do not call ensure_full_index
>  # during a call to 'git status'. Otherwise, we verify that we _do_ call it.
>  check_sparse_index_behavior () {
> -	git status --porcelain=v2 >expect &&
> -	git sparse-checkout init --cone --sparse-index &&
> -	git sparse-checkout set dir1 dir2 &&
> +	git -C full status --porcelain=v2 >expect &&
>  	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
> -		git status --porcelain=v2 >actual &&
> +		git -C sparse status --porcelain=v2 >actual &&
>  	test_region $1 index ensure_full_index trace2.txt &&
>  	test_region fsm_hook query trace2.txt &&
>  	test_cmp expect actual &&
> -	rm trace2.txt &&
> -	git sparse-checkout disable
> +	rm trace2.txt
>  }
>
>  test_expect_success 'status succeeds with sparse index' '
> -	git reset --hard &&
> +	git clone . full &&
> +	git clone . sparse &&
> +	git -C sparse sparse-checkout init --cone --sparse-index &&

Would it make sense to call `git clone --sparse . sparse`? I see that
there is no support for `--sparse=cone`, which makes me wonder whether we
want that at some stage. In any case, cloning with `--sparse` and then
setting up the cone mode should result in a little less work, right?

> +	git -C sparse sparse-checkout set dir1 dir2 &&
>
> -	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
> -	check_sparse_index_behavior ! &&
> -
> -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	write_script .git/hooks/fsmonitor-test <<-\EOF &&
>  		printf "last_update_token\0"

Technically, the backslash needs to be escaped because it is within double
quotes and we do not want the shell to interpolate the `\0`, but `printf`.
Practically, all the shells I tried handle this as expected.

Also, I have a slight preference for:

		printf "%s\\0" last_update_token

and later

		printf "%s\\0" last_update_token dir1/modified

What do your taste buds say about this?

These are only minor nits, of course. I do like the overall patch and
would be fine with it as-is.

Ciao,
Dscho

>  	EOF
> -	git config core.fsmonitor .git/hooks/fsmonitor-test &&
> +	git -C full config core.fsmonitor ../.git/hooks/fsmonitor-test &&
> +	git -C sparse config core.fsmonitor ../.git/hooks/fsmonitor-test &&
>  	check_sparse_index_behavior ! &&
>
> -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	write_script .git/hooks/fsmonitor-test <<-\EOF &&
>  		printf "last_update_token\0"
>  		printf "dir1/modified\0"
>  	EOF
>  	check_sparse_index_behavior ! &&
>
> -	cp -r dir1 dir1a &&
> -	git add dir1a &&
> -	git commit -m "add dir1a" &&
> +	git -C sparse sparse-checkout add dir1a &&
> +
> +	for repo in full sparse
> +	do
> +		cp -r $repo/dir1 $repo/dir1a &&
> +		git -C $repo add dir1a &&
> +		git -C $repo commit -m "add dir1a" || return 1
> +	done &&
> +	git -C sparse sparse-checkout set dir1 dir2 &&
>
>  	# This one modifies outside the sparse-checkout definition
>  	# and hence we expect to expand the sparse-index.
> -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	write_script .git/hooks/fsmonitor-test <<-\EOF &&
>  		printf "last_update_token\0"
>  		printf "dir1a/modified\0"
>  	EOF
> --
> gitgitgadget
>
>

Derrick Stolee Aug. 20, 2021, 3:09 p.m. UTC | #2

On 8/19/2021 3:45 AM, Johannes Schindelin wrote:> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
>>  test_expect_success 'status succeeds with sparse index' '
>> -	git reset --hard &&
>> +	git clone . full &&
>> +	git clone . sparse &&
>> +	git -C sparse sparse-checkout init --cone --sparse-index &&
> 
> Would it make sense to call `git clone --sparse . sparse`? I see that
> there is no support for `--sparse=cone`, which makes me wonder whether we
> want that at some stage. In any case, cloning with `--sparse` and then
> setting up the cone mode should result in a little less work, right?

The amount of work saved is miniscule, but I can understand wanting to
shave what we can from the cost of tests.

>> +	git -C sparse sparse-checkout set dir1 dir2 &&
>>
>> -	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
>> -	check_sparse_index_behavior ! &&
>> -
>> -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
>> +	write_script .git/hooks/fsmonitor-test <<-\EOF &&
>>  		printf "last_update_token\0"
> 
> Technically, the backslash needs to be escaped because it is within double
> quotes and we do not want the shell to interpolate the `\0`, but `printf`.
> Practically, all the shells I tried handle this as expected.
> 
> Also, I have a slight preference for:
> 
> 		printf "%s\\0" last_update_token
> 
> and later
> 
> 		printf "%s\\0" last_update_token dir1/modified
> 
> What do your taste buds say about this?

I have no opinions on this one way or another. I will just point out that
the pattern used in this test is also used throughout the test script, so
any change to that format should be applied universally. The test has been
operating without complaint since it was introduced in 5c8cdcf (fsmonitor:
add test cases for fsmonitor extension, 2017-09-22), so compatibility is
likely not a problem.

Thanks,
-Stolee

Eric Sunshine Aug. 20, 2021, 4:40 p.m. UTC | #3

On Fri, Aug 20, 2021 at 11:09 AM Derrick Stolee <[e-mail]> wrote:
> On 8/19/2021 3:45 AM, Johannes Schindelin wrote:> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
> >> +    write_script .git/hooks/fsmonitor-test <<-\EOF &&
> >>              printf "last_update_token\0"
> >
> > Technically, the backslash needs to be escaped because it is within double
> > quotes and we do not want the shell to interpolate the `\0`, but `printf`.
> > Practically, all the shells I tried handle this as expected.

Technically, for a POSIX-conforming shell, `\0` is correct, and the
backslash does not need to be escaped. Of course, it doesn't hurt to
write `\\0` since it resolves to the same thing, but it isn't
necessary. See POSIX section "2.2.3 Double-Quotes":
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_03

> I have no opinions on this one way or another. I will just point out that
> the pattern used in this test is also used throughout the test script, so
> any change to that format should be applied universally. The test has been
> operating without complaint since it was introduced in 5c8cdcf (fsmonitor:
> add test cases for fsmonitor extension, 2017-09-22), so compatibility is
> likely not a problem.

Since POSIX says that `\0` is correct and since we haven't encountered
any shells which handle this incorrectly, there seems little reason to
change it.

Patch

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index deea88d4431..6f2cf306f66 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -389,43 +389,47 @@  test_expect_success 'status succeeds after staging/unstaging' '
 # If "!" is supplied, then we verify that we do not call ensure_full_index
 # during a call to 'git status'. Otherwise, we verify that we _do_ call it.
 check_sparse_index_behavior () {
-	git status --porcelain=v2 >expect &&
-	git sparse-checkout init --cone --sparse-index &&
-	git sparse-checkout set dir1 dir2 &&
+	git -C full status --porcelain=v2 >expect &&
 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
-		git status --porcelain=v2 >actual &&
+		git -C sparse status --porcelain=v2 >actual &&
 	test_region $1 index ensure_full_index trace2.txt &&
 	test_region fsm_hook query trace2.txt &&
 	test_cmp expect actual &&
-	rm trace2.txt &&
-	git sparse-checkout disable
+	rm trace2.txt
 }
 
 test_expect_success 'status succeeds with sparse index' '
-	git reset --hard &&
+	git clone . full &&
+	git clone . sparse &&
+	git -C sparse sparse-checkout init --cone --sparse-index &&
+	git -C sparse sparse-checkout set dir1 dir2 &&
 
-	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
-	check_sparse_index_behavior ! &&
-
-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	write_script .git/hooks/fsmonitor-test <<-\EOF &&
 		printf "last_update_token\0"
 	EOF
-	git config core.fsmonitor .git/hooks/fsmonitor-test &&
+	git -C full config core.fsmonitor ../.git/hooks/fsmonitor-test &&
+	git -C sparse config core.fsmonitor ../.git/hooks/fsmonitor-test &&
 	check_sparse_index_behavior ! &&
 
-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	write_script .git/hooks/fsmonitor-test <<-\EOF &&
 		printf "last_update_token\0"
 		printf "dir1/modified\0"
 	EOF
 	check_sparse_index_behavior ! &&
 
-	cp -r dir1 dir1a &&
-	git add dir1a &&
-	git commit -m "add dir1a" &&
+	git -C sparse sparse-checkout add dir1a &&
+
+	for repo in full sparse
+	do
+		cp -r $repo/dir1 $repo/dir1a &&
+		git -C $repo add dir1a &&
+		git -C $repo commit -m "add dir1a" || return 1
+	done &&
+	git -C sparse sparse-checkout set dir1 dir2 &&
 
 	# This one modifies outside the sparse-checkout definition
 	# and hence we expect to expand the sparse-index.
-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	write_script .git/hooks/fsmonitor-test <<-\EOF &&
 		printf "last_update_token\0"
 		printf "dir1a/modified\0"
 	EOF