Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`

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 v5 1/2] t1092: add tests for `git diff-files`
Date: Fri, 10 Mar 2023 10:23:51 -0800	[thread overview]
Message-ID: <b537d855-edb7-4f67-de08-d651868247a5@github.com> (raw)
In-Reply-To: <20230310050021.123769-2-cheskaqiqi@gmail.com>

Shuqi Liang wrote:

Hi Shuqi! 

Apologies for taking so long to review; thanks for working on this.

> 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 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.Test 'newdirectory/testfile'

nit: missing space after "definition."

> being present on-disk without modifications, then change content inside
> 'newdirectory/testfile' in order to test 'newdirectory/testfile'
> being present on-disk with modifications.

Generally, you don't need to create a new file to get to this state, and I'd
personally advise against it. Personally, I find that adding a new file can
make the test more cumbersome than is necessary. I'll see if I can suggest
an alternative later on.

> 
> 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 801919009e..9b71d7f5f9 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,52 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	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 
> +

I'd be interested in seeing an additional test case for a pathspec with
wildcards or other "magic" [1], e.g. 'git diff-files "deep/*"'. In past
sparse index integrations, there has occasionally been a need for special
handling of those types of pathspecs [2][3], so it would be good for the
test to cover cases like that.

Otherwise, this test looks good.

[1] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec
[2] https://lore.kernel.org/git/822d7344587f698e73abba1ca726c3a905f7b403.1638201164.git.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/20220807041335.1790658-3-shaoxuan.yuan02@gmail.com/

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&

Before messing with modified files on disk, it'd be nice to show a
"baseline" of correct behavior when a pathspec points to out-of-cone files,
e.g. 'test_all_match git diff-files folder2/a'.

> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# add file to the index but outside of cone
> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git add --sparse newdirectory/testfile &&
> +
> +	# file present on-disk without modifications

From the comment you've added here, it looks like the state you want to test
is "file outside sparse checkout definition exists on disk". However, since
one of the goals of this test is to verify sparse index behavior once its
integrated with the 'diff-files' command, whether the "outside of sparse-
checkout definition" file belongs to a sparse directory entry is an
important (and fairly nuanced) factor to consider.

The main difference between a "regular" sparse-checkout and a sparse
index-enabled sparse-checkout is the existence of "sparse directory"
entries: index entries with 'SKIP_WORKTREE' that represent directories and
their contents. In the context of these tests, the thing we really want to
verify is that the sparse index-enabled case produces the same results as
the full index when an operation needs to get some information out of a
sparse directory. 

Coming back to this test, the 'newdirectory/testfile' you create, while
outside the sparse-checkout definition, never actually belongs to a sparse
directory because 'newdirectory' is never collapsed - in fact, I don't
think it even gets the SKIP_WORKTREE bit in the index. To ensure you have a
sparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout
reapply' after removing 'newdirectory/testfile' from disk - which doesn't
help if you want to test what happens when the file exists on disk!

In fact, because of built-in safeguards around on-disk files and
sparse-checkout, there isn't really a way in Git to materialize a file
without also expanding its sparse directory and removing SKIP_WORKTREE. If
you want to preserve a sparse directory, you should write the contents of an
existing file inside that sparse directory to disk manually:

	run_on_sparse mkdir folder1 &&
	run_on_sparse cp a folder1/a &&  # `folder1/a` is identical to `a` in the base commit

Git's index will not be touched by doing this, meaning the next command you
invoke (in this case, 'diff-files') will need to reconcile the file on disk
with what it sees in the index.

> +	test_sparse_match git diff-files &&
> +	test_must_be_empty sparse-checkout-out &&
> +	test_must_be_empty sparse-checkout-err &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_must_be_empty sparse-checkout-out &&
> +	test_must_be_empty sparse-checkout-err &&

These checks should be 'test_all_match' rather than 'test_sparse_match'.
Since (through other tests) we're confident that 'git diff-files' on an
unmodified, non-sparse-checkout repository will give us an empty result, you
wouldn't need the additional 'test_must_be_empty' checks.

A bit of a "spoiler": when I tested this out locally, I found that the diff
was *not* empty for the sparse-checkout cases, until I ran 'git status'
(which strips the 'SKIP_WORKTREE' bit from the file and writes it to the
index). That's not the desired behavior, so there's a bug in the
sparse-checkout logic used in 'diff-files' that needs to be fixed (my first
guess would be that 'clear_skip_worktree_from_present_files()' is not being
applied when the index is read).

If you'd like any help investigating this or get stuck, please let me know -
I'd be happy to assist!

> +
> +	# file present on-disk with modifications
> +	FN=newdirectory/testfile &&
> +	OID=$(git -C sparse-checkout hash-object $FN) &&
> +	ZERO=$(test_oid zero) &&
> +	echo ":100644 100644 $OID $ZERO M	$FN" >expect &&
> +
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_cmp expect sparse-checkout-out &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_cmp expect sparse-checkout-out

Similarly, you should use 'run_on_all' to modify the file & 'test_all_match'
to verify that they all match here. It would demonstrate that we don't
expect any "special" behavior from sparse-checkout, meaning you can probably
avoid checking the result verbatim. 

Finally, as with the earlier test, it'd be nice to show that the result is
the same with a wildcard pathspec, e.g. 'git diff-files "folder*/a"'.

> +'
> +
>  test_done