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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2022, 21:14 UTC
Message-ID: <xmqqk0brz7tb.fsf@gitster.g>
In-Reply-To: <Ylhp+q96KOt2+OGZ@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 24 quoted lines

>> '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).

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?

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.