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

From: Elijah Newren <newren@gmail.com>
Date: Mar 17, 2021, 18:03 UTC
Message-ID: <CABPp-BGfkPscpro=W5vcRHD5cV6pddYpvHLLzMWjL9WLaxyu3w@mail.gmail.com>
In-Reply-To: <pull.906.git.1615929435.gitgitgadget@gmail.com>

On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 42 quoted lines

>
> Here is the second patch series submission coming out of the sparse-index
> RFC [1].
>
> [1]
> https://lore.kernel.org/git/pull.847.git.1611596533.gitgitgadget@gmail.com/
>
> This is based on v3 of the format series [2].
>
> [2]
> https://lore.kernel.org/git/pull.883.v3.git.1615912983.gitgitgadget@gmail.com/
>
> 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.
>
> Many callers use index_name_pos() to find a path by name. In these cases, we
> can check if that position resolves to a sparse directory instance. In those
> cases, we just expand to a full index and run the search again.
>
> The last few patches deal with the name-hash hashtable for doing O(1)
> lookups.
>
> These protections don't do much right now, since the previous series created
> the_repository->settings.command_requires_full_index to guard all index
> reads and writes to ensure the in-memory copy is full for commands that have
> not been tested with the sparse index yet.
>
> 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.

I started reading the series, then noticed I didn't like the first few additions of ensure_full_index(). The first was because I thought it just wasn't needed as per a few lines of code later, but the more important one that stuck out to me was another where if the ensure_full_index() call is needed to avoid the code blowing up, then the code has a good chance that it is doing something inherently wrong in a sparse-checkout/sparse-index anyway.

So I guess that brings me to the question I asked in 07/27 -- is the presence of ensure_full_index() a note that the code needs to be later audited and tweaked to work with sparse-indexes? If so, then good, this series may make sense. However, will that always be the case? If we think some of these will be left in the code, is there a plan to annotate each one that has been audited and determined that it needs to stay? If not, then each ensure_full_index() might or might not have been audited for correctness and it becomes a pain to know what's left to fix. If the plan is that these are to be audited, and to be marked if they are truly deemed necessary, then the series makes sense to me. If not, then I'm confused about something with the series and need some help with the goals and plans.

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.