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

From: Josh Steadmon <steadmon@google.com>
Date: Apr 14, 2022, 18:37 UTC
Message-ID: <Ylhp+q96KOt2+OGZ@google.com>
In-Reply-To: <pull.1207.git.1649349442.gitgitgadget@gmail.com>

Hi Stolee,

Thanks for the series! All my comments are from the perspective of someone without much knowledge of the index, much less sparse-index, or sparse-checkout. I am not sure whether the project should cater to reviewers in my position, but I did have trouble understanding most of this series. I didn't have any concerns with the implementation, but the cover letter, commit messages, and tests were fairly confusing and I think can be explained better with a few changes:

On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:

Show 10 quoted lines

> This continues our sequence of integrating builtins with the sparse index.
> 
> '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.

Show 22 quoted lines

> Here is an outline of this series:
> 
>  * Patch 1: Add more tests around 'git show :' in t1092.
> 
>  * Patch 2: Make 'git show' stop expanding the index by default. Make note
>    of this behavior change in the tests.
> 
>  * Patches 3-4: Make the subtle changes to object-name.c that help us reject
>    sparse directories (patch 3) and print the correct error message (patch
>    4).
> 
> Patches 2-4 could realistically be squashed into a single commit, but I
> thought it might be instructive to show these individual steps, especially
> as an example for our GSoC project.
> 
> This series is based on the current 'master'. I know that Victoria intends
> to submit her 'git stash' integration soon, and this provides a way to test
> if our split of test changes in t1092 are easy to merge without conflict. If
> that is successful, then I will likely submit my integration with the
> 'sparse-checkout' builtin after this series is complete.
> 
> Thanks, -Stolee

Thanks, Josh