Re: [PATCH v11 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 v11 1/2] t1092: add tests for `git diff-files`
Date: Mon, 8 May 2023 15:25:45 -0700	[thread overview]
Message-ID: <8489e272-ccb9-62b1-992e-d305bb27c895@github.com> (raw)
In-Reply-To: <20230508184652.4283-2-cheskaqiqi@gmail.com>

Shuqi Liang wrote:
> +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 &&
> +
> +	# test wildcard
> +	test_all_match git diff-files "deep/*"

You added the '--' separator below, but not here. Was that intentional, or
should these have it as well? It doesn't make much of a practical difference
in this case, but it would be nice to remain consistent across all tests of
'diff-files' that you're adding. 

The same goes for some of the tests in patch 2 [1] ('sparse index is not
expanded: diff-files' and the perf tests).

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

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	test_sparse_match git diff-files -- folder2/a &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# The directory "folder1" is outside the cone of interest
> +	# and may not exist in the sparse checkout repositories.
> +	# Create it as needed, add file "folder1/a" there with
> +	# contents that is different from the staged version.

nit: 'folder1/' *definitely* won't be present in the sparse-checkout
repositories, so "will not" would be more accurate than "may not".
Otherwise, this comment is clearer than before & better explains what's
going on here.

> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp a folder1/a &&
> +
> +	run_on_all ../edit-contents folder1/a &&
> +	test_all_match git diff-files &&
> +	test_all_match git diff-files -- folder1/a &&
> +	test_all_match git diff-files -- "folder*/a"
> +'
> +
>  test_done