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

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

On Wed, Jun 30, 2021 at 7:32 AM Elijah Newren [off-list ref] wrote:

quoted

On Mon, Jun 28, 2021 at 7:04 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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

...

quoted

quoted

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.

quoted

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.

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