Re: [PATCH v7 2/2] diff-files: integrate with sparse index

From: Victoria Dye <vdye@github.com>
Date: Apr 21, 2023, 21:26 UTC
Message-ID: <111153b4-dba0-b533-fe49-57a6d5d3ba22@github.com>
In-Reply-To: <CAMO4yUESBZw2Jr8y4NW_2N7640o2o2mpq58+nnC+3qffG3Y8=Q@mail.gmail.com>

Shuqi Liang wrote:

Show 19 quoted lines

> Hi Victoria,
> 
>> If a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be
>> removed from the file and the index expanded automatically [1]. If that
>> mechanism is working properly, there would be no need to manually check the
>> pathspec and expand the index.
>>
>> Have you tried removing the 'pathspec_needs_expanded_index()' and running
>> the tests? If so, is 'diff-files' producing incorrect results?
>>
>> [1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/
> 
> As per your suggestion, I tried removing pathspec_needs_expanded_index()
> from the code, and 'diff-files pathspec expands index when necessary'
> test failed.
> 
> So, I'm thinking about keeping it to ensure everything works properly.
> I'd like to know your thoughts on this. Should we keep it or explore
> other options?

Did the test fail because the index wasn't expanded in a case where you previously expended it to be expanded? Or because of the returned results of 'diff-files' are invalid?

Only the latter represents incorrect behavior. If we're aren't expanding the index for a case that was causing index expansion before *and* the user-facing behavior is as-expected, that's the best-case scenario for a sparse index integration!

Taking a step back, it's important to remember that the overarching goal of the project is not just to switch 'command_requires_full_index' to '0' everywhere, but to find all of the places where Git is working with the index and make sure that work can be done on a sparse directory.

In most cases, it's possible to adapt an index-related operation to work with sparse directories (albeit with varying levels of complexity). The use of 'ensure_full_index()' is reserved for cases where it is _impossible_ to make Git perform a given action on a sparse directory - expanding the index completely eliminates the performance gains had by using a sparse index, so it should be avoided at all costs.

I hope that helps!

> 
> Thanks,
> Shuqi