Re: [PATCH v7 00/16] Sparse-index: integrate with status

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 12, 2021, 14:46 UTC
Message-ID: <6cdb4ba8-fe85-2a65-8e6c-74d3b5c3ef96@gmail.com>
In-Reply-To: <CABPp-BF2BLQm6864m+esquJ7og3fWHvTZeX_R-y4pnx9ApZ6XA@mail.gmail.com>

On 7/8/2021 9:16 PM, Elijah Newren wrote:

Show 25 quoted lines

> On Wed, Jun 30, 2021 at 7:32 AM Elijah Newren <newren@gmail.com> wrote:
>>
>> On Mon, Jun 28, 2021 at 7:04 PM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>>
>>> This is the first "payoff" series in the sparse-index work. It makes 'git
>>> status' very fast when a sparse-index is enabled on a repository with
>>> cone-mode sparse-checkout (and a small populated set).
>>>
> ...
>>> Because the range-diff is a big difficult to read this time, I'll break the
>>> changes down on a patch-by-patch basis.
> 
> Thanks for doing this; it was helpful.
> 
>> This is SUPER exciting.  I've only read the cover letter, but it
>> strongly suggests you've not only handled all my feedback in previous
>> rounds, but got things pretty solidly nailed away.  I'll try to make
>> some time to go over it all soon.
> 
> You have indeed addressed nearly all my feedback in previous rounds,
> and I found few problems with all the new code in this round.
> Overall, this round is looking really good, though there are a couple
> things I called out in comments on individual patches that I'll
> summarize here:

This summary is nice. I will try to do a similar thing myself in the future.

> Patch 9: a few minor suggestions for improving comments

Done.

Show 5 quoted lines

> Patch 10: the new code is never triggered and probably should either
> be dropped or made part of a later series if the later series needs
> it.  Also, although it doesn't necessarily need to hold up this
> series, I found a bug affecting sparse-checkouts with or without
> sparse-index while trying to understand this patch.

I checked and this patch is not necessary until the next series, so I will move it to that one.

> Patch 12: the code was slightly confusing to me, but I found that
> there seems to be an invariant it is based upon.  Adding an assert
> with a comment or just a comment about this invariant might help make
> the code more readable.

The assert() plus a better comment makes this patch cleaner.

> Patch 15: since the new tests in t1092 are written to compare
> sparse-checkout and sparse-index, it seems we should investigate a bug
> where the testsuite commands we invoke are giving incorrect behavior
> in both sparse-checkout and sparse-index.

I have made a note in an internal tracker to follow-up on this along with some other behavior things that currently exist in sparse-checkout as they should be pursued in a separate series.

I'm not ignoring your comments, but I would like to keep these patches that change the underlying data structure from being conflated with behavior changes. The fact that I am discovering (and documenting in tests) the changes is only highlighting that they already exist.

Thanks, -Stolee