Re: [PATCH v5 1/3] builtin/grep.c: add --sparse option
- From
Victoria Dye <vdye@github.com>
- Date
- Sep 10, 2022, 01:07 UTC
- Message-ID
- <68d8c6f8-d2cd-185b-147d-dfa8d4190fc6@github.com>
- In-Reply-To
- <20220908001854.206789-2-shaoxuan.yuan02@gmail.com>
Shaoxuan Yuan wrote:
Show 25 quoted lines
> 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.