Re: [GSOC][PATCH v1] diff-index: enable diff-index

[Junio C Hamano <gitster@pobox.com>]

Victoria Dye <vdye@github.com> writes:

> Raghul Nanth A wrote:
>> 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.

Good point.  Comparing with HEAD~1 has a chance to compare _more_
paths (i.e. paths changed in the working tree plus paths changed
between the two commits), though it feels a bit too subtle if that
is what these two tests meant.

Testing with pathspec limited comparison, limiting within the cone
of interest or extending to outside the cone, does sound like a good
idea.  "diff-index --cached" to ignore working tree changes is also
an obvious thing we want to see working well.

> 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.

Thanks for a careful review and comment.