From: Victoria Dye <vdye@github.com>
Date: Sat, 10 Sep 2022 01:07:22 GMT
Subject: Re: [PATCH v5 1/3] builtin/grep.c: add --sparse option
Message-ID: <68d8c6f8-d2cd-185b-147d-dfa8d4190fc6@github.com>
In-Reply-To: <20220908001854.206789-2-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan wrote:
> diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh
> index eb59564565..a9879cc980 100755
> --- a/t/t7817-grep-sparse-checkout.sh
> +++ b/t/t7817-grep-sparse-checkout.sh
> @@ -118,13 +118,19 @@ test_expect_success 'grep searches unmerged file despite not matching sparsity p
>  	test_cmp expect actual
>  '
>  
> -test_expect_success 'grep --cached searches entries with the SKIP_WORKTREE bit' '
> +test_expect_success 'grep --cached and --sparse searches entries with the SKIP_WORKTREE bit' '
> +	cat >expect <<-EOF &&
> +	a:text
> +	EOF
> +	git grep --cached "text" >actual &&
> +	test_cmp expect actual &&
> +
>  	cat >expect <<-EOF &&
>  	a:text
>  	b:text
>  	dir/c:text
>  	EOF
> -	git grep --cached "text" >actual &&
> +	git grep --cached --sparse "text" >actual &&
>  	test_cmp expect actual
>  '

At first, seeing that all the test titles were changed from "grep --cached
<does something>" to "grep --cached and --sparse <does something>", I was
going to suggest that 'git grep --cached' (without '--sparse') should
receive some new tests in addition to updating existing ones (which now
require '--sparse' to work as before).

However, looking at the actual content of the tests like the one above, I
can see that you've added cases demonstrating the expected difference in
behavior between 'grep --cached' and 'grep --cached --sparse'. I can't think
of a clearer way to name the tests, though, so this looks okay to me.

The rest of the patch (namely, the implementation of '--sparse' and
corresponding documentation) looked good as well - I didn't have anything
specific to note on that.