Re: [PATCH 0/4] Sparse index integration with 'git show'

[Derrick Stolee <derrickstolee@github.com>]

On 4/14/2022 5:14 PM, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
>>> 'git show' is relatively simple to get working in a way that doesn't fail
>>> when it would previously succeed, but there are some sutbleties when the
>>> user passes a directory path. If that path happens to be a sparse directory
>>> entry, we suddenly start succeeding and printing the tree information!
>>>
>>> Since this behavior can change depending on the sparse checkout definition
>>> and the state of index entries within that directory, this new behavior
>>> would be more likely to confuse users than help them.
>>
>> The two paragraphs above did not really convey to me on first
>> read-through what the problem was. IIUC the main points are:
>>
>> * git-show does not currently work with the sparse index.
>> * A simple change fixes the above, but causes us to sometimes print
>>   unexpected information about trees.
>> * We can use the simple change in our implementation, but must do
>>   additional work to make sure we only print the expected information.
>>
>> I think this could be clarified by:
>> * Including examples of the unhelpful output from the simple
>>   implementation.
>> * Describing in more detail the situations that trigger this.
>> * Describing why those situations don't currently happen without support
>>   for sparse indexes.
> 
> I think the issues patches 2-4 addresses are not limited to any
> specific command, but how ":<path-in-the-index>" syntax is resolved
> to an object name (including the way how error messages are issued
> when the given path is not found in the index).

Yes, this is the critical bit. It's the only part of "git show"
that cares about the index.
 
> But the title of the series and presentation place undue stress on
> "git show", which I suspect may be confusing to some readers.
> 
> For example, with these patches, wouldn't "git rev-parse" start
> working better, even though the proposed log message for no commit
> in the series talks about "rev-parse" and no tests are done with the
> "rev-parse" command?

Probably, assuming we unset the command_requires_full_index
protection on 'git rev-parse'. This might be a good case for
Shaoxuan to try.

> I am not suggesting that commands other than "show" should be also
> discussed in detail or tested.  It would help readers if we said
> that we are using 'show' merely as an example to demonstrate how
> ":<path-in-the-index>" syntax interacts with the sparse index
> entries (1) before the series, and (2) how well the improved
> object-name parsing logic works after the series.
 
I can see that.
Some background: as we shipped our sparse index integrations in
the microsoft/git fork and measured performance of real users, we
noticed that 'git show' was a frequently-used command that went
from ~0.5 seconds to ~4 seconds in monorepo situations. This was
unexpected, because we didn't know about the ":<path>" syntax.
Further, it seemed that some third-party tools were triggering
this behavior as a frequent way to check on staged content. That
motivated quickly shipping this integration. Performance improved
to ~0.1 seconds because of the reduced index size.

Thanks,
-Stolee