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

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 27, 2022, 13:47 UTC
Message-ID: <bb28d9d3-8152-b6cf-6717-df8534e2871b@github.com>
In-Reply-To: <xmqqmtg7pntg.fsf@gitster.g>

On 4/26/2022 4:55 PM, Junio C Hamano wrote:

Show 21 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> This ":" syntax is shared by other commands like "git rev-parse", but we are
>> not adding those integrations at this point.
> 
> This has been the most curious thing since the initial round.  The
> changes in the series are mostly about the code that parses the
> ":<path>" syntax and yield an object name (when exists) or give an
> error messages (otherwise) and die, before the computed object name
> gets used by "git show", or any other command that takes an object
> name from the command line.
> 
> I guess what has been confusing me was the phrase "integration",
> that you seem to be using to refer only to the final step of setting
> require_full_index to 0, while that is the least interesting part of
> a series like this one.  The work done in patches 3/ and 4/ that
> paves the way to allow us to set the require_full_index to 0 in any
> command that needs to work with the ":<path>" syntax is much more
> interesting part of the series, and when viewed from that angle, the
> series is not about preparing "show" but about ":<path>" syntax to
> work better with the sparse index.

In general, yes, we do need to be teaching different parts of the codebase about the sparse index. The way I've been tracking that progress is by which builtins can stop expanding a sparse index to a full one upon parse. This progress indicator also matches the testing strategy, which focuses on preserving behavior for top-level Git commands (and checking that they don't expand the sparse index when they don't need to).

I understand your thought that it would be better to sell the series to reviewers by the interesting pieces under the hood that are changing. I think this is one of the first times where only one of these systems is sufficient to make an entire builtin (or two) work with the sparse index.

I'll keep this in mind for pitching future updates.

Thanks, -Stolee