Re: [PATCH 15/27] [RFC-VERSION] *: ensure full index

On 2/1/2021 3:22 PM, Elijah Newren wrote:

On Mon, Jan 25, 2021 at 9:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

This giant patch is not intended for actual review. I have a branch that
has these changes split out in a sane way with some commentary in each
file that is modified.

The idea here is to guard certain portions of the codebase that do not
know how to handle sparse indexes by ensuring that the index is expanded
to a full index before proceeding with the logic.

This also provides a good mechanism for testing which code needs
updating to enable the sparse index in a Git builtin. The builtin can
set the_repository->settings.command_requires_full_index to zero and
then we can debug the command with a breakpoint on ensure_full_index().
That identifies the portion of code that needs adjusting before enabling
sparse indexes for that command.

Some index operations must be changed to operate on a non-const pointer,
since ensuring a full index will modify the index itself.

There are likely some gaps to these protections, which is why it will be
important to carefully test each scenario as we relax the requirements.
I expect that to be a long effort.

I think the idea makes sense; it provides a way for us to
incrementally build support for this new feature.

I skimmed over the code and noticed various interesting places that
had the ensure_full_index() call (e.g.
read_skip_worktree_file_from_index() -- whose existence comes from
sparsity; what irony...).  Better breakouts would be great, so I'll
defer commenting much until then.  But, just to verify I'm
understanding: the primary defence is the command_requires_full_index
setting, and you have added several ensure_full_index() calls
throughout the code in places you believe would need to be fixed up in
case someone switches the command_requires_full_index setting.  Is
that correct?  And your comment on the gaps is just that there may be
other places that are missing the secondary protection (as opposed to
my first reading of that paragraph as suggesting we aren't sure if we
have enough protections yet and need to add more before this moves out
of RFC); is that right?

Yes, the idea is that we can incrementally enable
command_requires_full_index for some builtins and be confident that
corner cases will be protected by ensure_full_index(). Further, we
can test whether ensure_full_index() was called using test_region
in test scripts to demonstrate that a command is truly "sparse aware"
or if it is converting to full and back to sparse.

There is also the case that when we write the index into a sparse
format, the in-memory structure is modified. If the index is re-used
afterwards, then we must expand to full again for these code paths.

unpack_trees() already has one of these calls because it was necessary
for the sparse-index write to work.

The ensure_full_index() pattern also works when updating a builtin to
work with the sparse-index because of the breakpoint trick.

When I submit this as a full series, this patch will be one full
patch series submission with careful comments about why each of these
is added on a file-by-file basis.

Thanks,
-Stolee