Re: [RFC][PATCH] t1092: add tests for `git diff-files`

From: Derrick Stolee <derrickstolee@github.com>
To: Shuqi Liang <cheskaqiqi@gmail.com>, git@vger.kernel.org
Cc: vdye@github.com
Subject: Re: [RFC][PATCH] t1092: add tests for `git diff-files`
Date: Mon, 6 Mar 2023 09:14:35 -0500	[thread overview]
Message-ID: <99252618-28a9-6aa7-880a-f8ab0714bbb9@github.com> (raw)
In-Reply-To: <20230304025740.107830-1-cheskaqiqi@gmail.com>

On 3/3/2023 9:57 PM, Shuqi Liang wrote:
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2054,5 +2054,37 @@ test_expect_success 'grep sparse directory within submodules' '
>  	git grep --cached --recurse-submodules a -- "*/folder1/*" >actual &&
>  	test_cmp actual expect
>  '
> +test_expect_success 'diff-files with pathspec inside sparse definition' '

nit: you need an empty line between the previous test's closing quote
and the start of your new test.

> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>$1
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files  &&
> +	test_all_match git diff-files deep/a &&
> +	test_all_match git diff-files --find-object=HEAD:a
> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>$1
> +	EOF
> +
> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git sparse-checkout set newdirectory &&
> +	test_sparse_match git add newdirectory/testfile &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git sparse-checkout set &&

These uses of 'git sparse-checkout set' are probably not necessary
if you use "git add --sparse newdirectory/testfile". It does present
an interesting modification of your test case: what if the file
exists on-disk but outside of the sparse-checkout definition? What
happens in each case? What if the file is different from the staged
version?

> +
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile
> +'

These tests look like a good start here. I was first confused as
to why you were doing such steps to modify the sparse-checkout
definition, but I see it is critical that you have staged changes
outside of the sparse-checkout cone. These kinds of details, the
"why" you are doing subtle things, are great to add to the commit
message.

> To make sure git diff-files behaves as expected when
> inside or outside of sparse-checkout definition.
> 
> Add test for git diff-files:
> Path is within sparse-checkout cone
> Path is outside sparse-checkout cone

With that in mind, here is a way you could edit your commit
message to be more informative:

  Before integrating the 'git diff-files' builtin with the sparse
  index feature, add tests to t1092-sparse-checkout-compatibility.sh
  to ensure it currently works with sparse-checkout and will still
  work with sparse index after that integration.

  When adding tests against a sparse-checkout definition, we must
  test two modes: all changes are within the sparse-checkout cone
  and some changes are outside the sparse-checkout cone. In order to
  have staged changes outside of the sparse-checkout cone, create a
  'newdirectory/testfile' and add it to the index, while leaving it
  outside of the sparse-checkout definition.

(If you decide to add tests for the case of 'newdirectory/testfile'
being present on-disk with or without modifications, then you can
expand your commit message to include details about those tests,
too.)

Thanks,
-Stolee