Re: [PATCH v4 1/3] t1092: add tests for 'git check-attr'

From: Victoria Dye <vdye@github.com>
Date: Jul 20, 2023, 18:43 UTC
Message-ID: <c3ebe3b4-88b9-8ca2-2ee3-39a3e0d82201@github.com>
In-Reply-To: <20230718232916.31660-2-cheskaqiqi@gmail.com>

Shuqi Liang wrote:

Show 17 quoted lines

> Add tests for `git check-attr`, make sure attribute file does get read
> from index when path is either inside or outside of sparse-checkout
> definition.
> 
> Add a test named 'diff --check with pathspec outside sparse definition'.
> It starts by disabling the trailing whitespace and space-before-tab
> checks using the core.whitespace configuration option. Then, it
> specifically re-enables the trailing whitespace check for a file located
> in a sparse directory. This is accomplished by adding a
> whitespace=trailing-space rule to the .gitattributes file within that
> directory. To ensure that only the .gitattributes file in the index is
> being read, and not any .gitattributes files in the working tree, the
> test removes the .gitattributes file from the working tree after adding
> it to the index. The final part of the test uses 'git diff --check' to
> verify the correct application of the attribute rules. This ensures that
> the .gitattributes file is correctly read from index and applied, even
> when the file's path falls outside of the sparse-checkout definition.

Thanks for the thorough explanation! This presents a compelling case for why .gitattributes should be read from sparse directories (if it isn't, the behavior in sparse-index vs. full-checkout and sparse-checkout doesn't match).

Show 28 quoted lines

> 
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 8a95adf4b5..90633f383a 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2259,4 +2259,52 @@ test_expect_success 'worktree is not expanded' '
>  	ensure_not_expanded worktree remove .worktrees/hotfix
>  '
>  
> +test_expect_success 'check-attr with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	echo "a -crlf myAttr" >>.gitattributes &&
> +	run_on_all cp ../.gitattributes ./deep &&
> +
> +	test_all_match git check-attr -a -- deep/a &&
> +
> +	test_all_match git add deep/.gitattributes &&
> +	test_all_match git check-attr -a --cached -- deep/a
> +'
> +
> +test_expect_failure 'check-attr with pathspec outside sparse definition' '

Could you explain (either in a "NEEDSWORK" comment here or in the commit message) why this is 'test_expect_failure'?

Show 17 quoted lines

> +	init_repos &&
> +
> +	echo "a -crlf myAttr" >>.gitattributes &&
> +	run_on_sparse mkdir folder1 &&
> +	run_on_all cp ../.gitattributes ./folder1 &&
> +	run_on_all cp a folder1/a &&
> +
> +	test_all_match git check-attr -a -- folder1/a &&
> +
> +	git -C full-checkout add folder1/.gitattributes &&
> +	run_on_sparse git add --sparse folder1/.gitattributes &&
> +	run_on_all git commit -m "add .gitattributes" &&
> +	test_sparse_match git sparse-checkout reapply &&
> +	test_all_match git check-attr  -a --cached -- folder1/a
> +'
> +
> +test_expect_failure 'diff --check with pathspec outside sparse definition' '

Same here.

However, when I apply this patch locally and run this test, I get:

	ok 94 - diff --check with pathspec outside sparse definition # TODO known breakage vanished
	# 1 known breakage(s) vanished; please update test(s)

Looking at 'sparse-checkout-out', I see:

	folder1/a:1: trailing whitespace.
	+a

This test _should_ fail (as your 'test_expect_failure' indicates), but it passes because the outside-of-cone '.gitattributes' is somehow being applied to 'folder1/a'. After some debugging, I traced the issue to...

Show 7 quoted lines

> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo "a " >"$1"
> +	EOF
> +
> +	git config core.whitespace -trailing-space,-space-before-tab &&

...here. This 'git config' doesn't actually apply the configuration to any of the test repositories, it applies the config to the parent directory. To apply the config to the test repos, use:

	test_all_match git config core.whitespace -trailing-space,-space-before-tab &&

Show 6 quoted lines

> +
> +	echo "a whitespace=trailing-space,space-before-tab" >>.gitattributes &&
> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp ../.gitattributes ./folder1 &&
> +	git -C full-checkout add folder1/.gitattributes &&
> +	run_on_sparse git add --sparse folder1/.gitattributes &&

nit: 'git add --sparse' will work in 'full-checkout' - there's no need to have separate calls for 'full-checkout' and the sparse checkouts.

Also, please use 'test_(all|sparse)_match' when running Git commands in these tests, even if they're not the command explicitly being tested. It adds an extra level of verification to the test essentially "for free". Conversely, using 'run_on_(all|sparse)' could conceal subtle issues in the test or mask bugs in the implementation.

> +	run_on_all rm folder1/.gitattributes &&
> +	run_on_all  ../edit-contents folder1/a &&

nit: extra space between 'run_on_all' and '../edit-contents'.

> +	test_all_match test_must_fail git diff --check -- folder1/a

Because '.gitattributes' was added and 'folder1/a' exists on disk, the 'folder1/' sparse directory is "unsparsified" by the time of this check. As a result, there's essentially no difference in how 'sparse-index' is handled vs. 'sparse-checkout'. It would be nice to have this verify the attribute parsing in a sparse directory; one way to do that would be something like:

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

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 125b205b0d..183fce8531 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2300,11 +2300,12 @@ test_expect_success 'diff --check with pathspec outside sparse definition' '
        echo "a whitespace=trailing-space,space-before-tab" >>.gitattributes &&
        run_on_all mkdir -p folder1 &&
        run_on_all cp ../.gitattributes ./folder1 &&
-       git -C full-checkout add folder1/.gitattributes &&
-       run_on_sparse git add --sparse folder1/.gitattributes &&
-       run_on_all rm folder1/.gitattributes &&
-       run_on_all  ../edit-contents folder1/a &&
-       test_all_match test_must_fail git diff --check -- folder1/a
+       test_all_match git add --sparse folder1/.gitattributes &&
+       run_on_all ../edit-contents folder1/a &&
+       test_all_match git add --sparse folder1/a &&
+
+       test_sparse_match git sparse-checkout reapply &&
+       test_all_match test_must_fail git diff --check --cached -- folder1/a
 '
 
 test_expect_success 'sparse-index is not expanded: check-attr' '
----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 -----

The main differences from the current patch are:

1. adding 'folder1/a' to the index then "re-sparsifying" 'folder1/' with
   'git sparse-checkout reapply'.
2. using the '--cached' option to 'git diff' to compare "index vs. HEAD"
   rather than "working tree vs. index".

Finally, as a general point of feedback - all of the version of this series
so far have included some minor whitespace/stylistic issues (missing spaces
in expressions, double spaces, trailing whitespace, etc.). Please check your
patches carefully before submitting them to avoid excess re-rolls & get your
changes merged faster.

[1] https://lore.kernel.org/git/20230718232916.31660-3-cheskaqiqi@gmail.com/

> +'
> +
>  test_done