Re: [PATCH 00/27] Sparse Index: API protections

From: Elijah Newren <newren@gmail.com>
Date: Mar 18, 2021, 06:32 UTC
Message-ID: <CABPp-BE4qc1-Y+gdZCxB=o+DEF0F_dH-=-YaGWJfO_T6LC6x2Q@mail.gmail.com>
In-Reply-To: <CABPp-BGfkPscpro=W5vcRHD5cV6pddYpvHLLzMWjL9WLaxyu3w@mail.gmail.com>

On Wed, Mar 17, 2021 at 11:03 AM Elijah Newren <newren@gmail.com> wrote:

Show 10 quoted lines

>
> On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > The point of this series is to insert protections for the consumers of the
> > in-memory index. We mark certain regions of code as needing a full index, so
> > we call ensure_full_index() to expand a sparse index to a full one, if
> > necessary. These protections are inserted file-by-file in every loop over
> > all cache entries. Well, "most" loops, because some are going to be handled
> > in the very next series so I leave them out.

...

Show 12 quoted lines

> > However, after this series is complete, we now have a straight-forward plan
> > for making commands "sparse aware" one-by-one:
> >
> >  1. Disable settings.command_requires_full_index to allow an in-memory
> >     sparse-index.
> >  2. Run versions of that command under a debugger, breaking on
> >     ensure_full_index().
> >  3. Examine the call stack to determine the context of that expansion, then
> >     implement the proper behavior in those locations.
> >  4. Add tests to ensure we are checking this logic in the presence of sparse
> >     directory entries.
>

...

> If I'm confused about the goals and the plans, then my reviews will
> probably be less than helpful, so I'll suspend reading the series
> until I understand the plan a little better.

Thanks for patiently responding to all my other queries. I read through more of the series today. I inserted comments on a couple specific patches, but most of patches 6-25 are all along the same theme.

I have some overall comments on those patches in 6-25 (none of which need to be addressed in this series, but are meant as hopefully helpful guides about future work):

add/rm: both of these were just above loops that had a skip-the-SKIP_WORKTREE entries, at least once Matheus' series is merged (https://lore.kernel.org/git/cover.1615588108.git.matheus.bernardino@usp.br/). I think the upshot is just that these become easier to convert.

checkout/commit -- I think these follow the add/rm model and likewise become easy to convert.

ls-files - I commented elsewhere in this thread about how I think it'd make sense to have it list the entries in the index, as always. Obviously, that'd give different output than a full index, because there are different entries present in the index when using a sparse-index vs. a full one.

However, all of these, as you say, can wait until later.

I also noted that patches 26 and 27 were based on ones from the RFC series that I reviewed before, and I noticed you fixed an issue or two I flagged there, but you also made some other changes and it's too late at night for my brain to continue thinking and try to compare them; I may try again tomorrow.

Other than patch 26 which I'm too tired to think through right now, and the patches I specifically commented on, the rest of the series looks good to me.