Re: [PATCH v2 4/7] reset: integrate with sparse index

Elijah Newren [off-list ref] writes:

On Tue, Oct 5, 2021 at 6:21 AM Victoria Dye via GitGitGadget
[off-list ref] wrote:

quoted

From: Victoria Dye <redacted>

`reset --soft` does not modify the index, so no compatibility changes are
needed for it to function without expanding the index. For all other reset
modes (`--mixed`, `--hard`, `--keep`, `--merge`), the full index is
explicitly expanded with `ensure_full_index` to maintain current behavior.

"to maintain current behavior"?  You are changing code here, which
suggests some kind of behavior is changing, but that description seems
to be claiming the opposite.  Is it some kind of preventative change
to add ensure_full_index calls in an additional place, with a later
patch in the series intending to remove the other one(s), so you're
making sure that later changes won't cause unwanted behavioral
changes?  Or was something else meant here?

If the above wasn't what you meant, but you're adding
ensure_full_index calls, does that suggest that we had some important
code paths that were not protected by such calls?

The original called read_cache() before we know which mode we
operate in, near the end of parse_args(), which resulted in an
unconditional call to ensure_full_index() in repo_read_index().

This patch delays the call to read_cache().  If parse_pathspec()
and everything the original called after the point where it called
read_cache() needed to have a populated in-core index, the change
can break things---I didn't check thoroughly, but I am guessing
it is OK.

quoted

Additionally, the `read_cache()` check verifying an uncorrupted index is
moved after argument parsing and preparing the repo settings. The index is
not used by the preceding argument handling, but `read_cache()` does need to
be run after enabling sparse index for the command and before resetting.

This seems to be discussing what code changes are being made, but not
why.  I'm guessing at the reasoning, but is it something along the
lines of:

"""
Also, make sure to read_cache() after setting
command_requires_full_index = 0, so that we don't unnecessarily expand
the index as part of our early index-corruption check.
"""

I think it is more like "we used to expand very early for all modes,
but with this change we move the read_cache() call to much later,
and force it not to expand.  The modes that call read_from_tree()
needs in-core index fully expanded, so we do so there, but the soft
reset does not call it and would stop expanding."