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

From: Junio C Hamano <gitster@pobox.com>
To: Elijah Newren <newren@gmail.com>
Cc: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <stolee@gmail.com>, Taylor Blau <me@ttaylorr.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Victoria Dye <vdye@github.com>
Subject: Re: [PATCH v2 4/7] reset: integrate with sparse index
Date: Wed, 06 Oct 2021 10:48:51 -0700	[thread overview]
Message-ID: <xmqqo882rqm4.fsf@gitster.g> (raw)
In-Reply-To: <CABPp-BHKjLVpL_U5viVA++hCZmGKBHxnubGCk=1YhTkpFc-E-Q@mail.gmail.com> (Elijah Newren's message of "Tue, 5 Oct 2021 19:15:55 -0700")

Elijah Newren <newren@gmail.com> writes:

> On Tue, Oct 5, 2021 at 6:21 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Victoria Dye <vdye@github.com>
>>
>> `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.

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