Re: [GSOC][PATCH v1] diff-index: enable diff-index
- From
Victoria Dye <vdye@github.com>
- Date
- Apr 5, 2023, 17:53 UTC
- Message-ID
- <91d3fd23-8120-db65-481a-e9f56017bb04@github.com>
- In-Reply-To
- <20230403190538.361840-1-nanth.raghul@gmail.com>
Raghul Nanth A wrote:
Show 10 quoted lines
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh > index 3242cfe91a..9e74cb22b9 100755 > --- a/t/perf/p2000-sparse-operations.sh > +++ b/t/perf/p2000-sparse-operations.sh > @@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all > test_perf_on_all git update-index --add --remove $SPARSE_CONE/a > test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a" > test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*" > +test_perf_on_all git diff-index HEAD > +test_perf_on_all git diff-index HEAD~1
What is the benefit of testing 'diff-index' with 'HEAD' *and* 'HEAD~1'? I wouldn't expect internal behavior in the command to change based on the revision, so the performance should be nearly identical. I'd much rather see 'diff-index --cached' and/or other options & pathspecs exercised.
Show 17 quoted lines
> > test_done > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 801919009e..13801f327d 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -1996,6 +1996,24 @@ test_expect_success 'sparse index is not expanded: rm' ' > ensure_not_expanded rm -r deep > ' > > +test_expect_success 'sparse index is not expanded: diff-index' ' > + init_repos && > + > + echo "new" >>sparse-index/g && > + git -C sparse-index add g && > + git -C sparse-index commit -m "dummy" && > + ensure_not_expanded diff-index HEAD~1
As with the other tests, please exercise different options and pathspecs with 'diff-index' to improve coverage.
Show 10 quoted lines
> +' > + > +test_expect_success 'match all: diff-index' ' > + init_repos && > + > + test_all_match git diff-index HEAD && > + run_on_all rm g && > + test_all_match git diff-index HEAD && > + test_all_match git diff-index HEAD --cached > +'
In addition to the '--cached' option, please test different pathspecs (especially different wildcard variations; see the 'git grep' [1] and 'git diff-files' [2] integrations for examples you could build off of).
Seeing that 'diff-files' needed 'pathspec_needs_expanded_index', it's possible that this command needs similar treatment. I'm curious as to whether 'diff' needs it as well - the tests in 't1092' don't cover 'diff' with pathspecs, so it might be behaving incorrectly. If that's the case, it would be nice to see pathspecs handled all in one place ('run_diff_index()'?), if possible.
[1] https://lore.kernel.org/git/20220923041842.27817-1-shaoxuan.yuan02@gmail.com/ [2] https://lore.kernel.org/git/20230322161820.3609-1-cheskaqiqi@gmail.com/
> + > test_expect_success 'grep with and --cached' ' > init_repos && >