Re: [PATCH] describe: enable sparse index for describe

[Derrick Stolee <derrickstolee@github.com>]

On 3/28/23 4:24 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>>>>  			setup_work_tree();
>>>> +			prepare_repo_settings(the_repository);
>>>> +			the_repository->settings.command_requires_full_index = 0;
>>>
>>> Offhand, the only case I know that "describe" even _needs_ a working
>>> tree or the index is when asked to do the "--dirty" thing.  To
>>> figure out if the working tree files are modified, the code calls
>>> into run_diff_index(), but has that codepath been made sparse-index
>>> aware already?
>>
>> It seems that this is a case where we can rely on the existing
>> changes around run_diff_index(), which is nice. We get a very
>> easy win for a narrow case.
> 
> Hmph, so "diff-index" is already sparse-index aware?  I looked for
> any old commit that flips .command_requires_full_index to false for
> the command and didn't find it, and if I recall correctly (I only
> checked when I wrote the comment you are responding to),
> cmd_diff_index() does not do that, either, so I assumed that nobody
> has converted run_diff_index() yet.  And that was where my question
> came from.

Since 'git diff --cached' is covered as of 51ba65b5c35 (diff:
enable and test the sparse index, 2021-12-06), that means that
the implementation run_diff_index() is covered.

In fact, it's likely that the reason it's covered is because
run_diff_index() calls diff_cache() which calls unpack_trees() and
unpack_trees() needs to work for _anything_ to work, especially
'git status' and 'git checkout'.

The only thing keeping us from enabling the sparse index on many
of these "diff" builtins is the necessary testing in t1092. I'm
sure that diff-index is very easy to convert. Hopefully, we will
have a GSoC student this summer who can tackle most of these easy
ones, as well as a tricky one or two.

Thanks,
-Stolee