Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index

[Derrick Stolee <derrickstolee@github.com>]

On 2/27/2023 12:18 PM, Victoria Dye wrote:
> Shuqi Liang wrote:

>> +	prepare_repo_settings(the_repository);
>> +	the_repository->settings.command_requires_full_index = 0;
> 
> The test below doesn't do anything special related to the sparse index, so
> this change is unnecessary (and, as far as I can tell, will break in some
> usage of 'git check-attr'). If you're only looking for feedback on testing,
> it'd better to leave this out.

This change is part of the performance improvements given by sparse index,
but the correctness test you've added only ensures that the end result is
correct, not fast.

It's possible that even with this change we hit an ensure_full_index() call
somewhere in the call stack. To test that the sparse-index stays sparse
throughout the process lifetime (when possible) create a test that uses the
ensure_not_expanded helper. There are several examples in t1092 to use as
a starting point.

Victoria is right that it is helpful to first establish test coverage of
the builtin for correctness reasons before making this change. It helps to
add tests for cases that would require expanding the sparse index, such as
checking the attributes for paths outside of the sparse-checkout cone.

Once the correctness tests are in place, you can then make this change to
the builtin and add the tests that check ensure_not_expanded, since the
change at that point is _only_ that we are allowing the builtin to
operate upon the sparse index without expanding it immediately after read.

A good example of this "final step" is [1], which updates the builtin for
'git diff' as well as adding _only_ the ensure_not_expanded tests.

[1] https://lore.kernel.org/git/897611682af64ba6bd0d2dfcfeae56cfe953c45e.1638806161.git.gitgitgadget@gmail.com/
    [PATCH v6 6/7] diff: enable and test the sparse index

You can look at the surrounding patches for other ideas, but it should be
noted that both 'git diff' and 'git blame' in that series had previous
correctness tests in t1092 that only needed _edits_, instead of being
created from scratch.

Thanks,
-Stolee