Hi Victoria, Hi Derrick Thanks a lot for your valuable feedback! They are really helpful! These two days are very busy for me for preparing the midterm. After 1st March, I'll start to address them in the next roll or start to Investigation around 'git diff-files‘ as soon as possible . -- Thanks & Regards, Shuqi On Mon, Feb 27, 2023 at 12:57 PM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: > > 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@xxxxxxxxx/ > [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
