Re: [PATCH v4 0/4] Optionally skip hashing index on write

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 6, 2023, 15:33 UTC
Message-ID: <b2164be2-72e9-cee5-26db-3e4fbfec3051@github.com>
In-Reply-To: <221216.86sfhf1gbc.gmgdl@evledraar.gmail.com>

On 12/16/2022 10:43 AM, Ævar Arnfjörð Bjarmason wrote:

> But as it is split up the individual steps should make sense. The 2/4
> here should really just use "bool", not "maybe_bool" to begin with, no?

Show 26 quoted lines

> And part of this in 4/4 is inheriting a non-stricture in
> repo-settings.c, but for this new config variable that we're introducing
> as only a boolean from day one can't we just die() on anything that's
> not a boolean?
> 
> On other implied feedback, in
> https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/
> I noted the switching between istate->repo & the_repository, and that
> you could hit a BUG() (when uncommenting a line in my testing patch on
> top) if we didn't have istate->repo:
> 
> There was a commit message update for that:
> 
>>  2:  00738c81a12 ! 2:  aae047cbc9f read-cache: add index.skipHash config option
>>      @@ Commit message
>>       
>>           [1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201
>>       
>>      +    We load this config from the repository config given by istate->repo,
>>      +    with a fallback to the_repository if it is not set.
> 
> But I think that really sweeps a potential issue under the rug. What are
> these cases where we don't get istate->repo, are those expected? Is
> preferring istate->repo always known to be redundant now, and just done
> for good measure, or are there subtle cases (e.g. when reading
> submodules) where we pick the wrong repo's config?

After investigating some of the failures from creating a BUG() statement when istate->repo is NULL I see several problems, and they are not related to submodules for the most part.

The first issues I've found are due to code paths that operate on the_index without actually initializing it with a do_read_index(), or otherwise initialize an index using a memset() instead of a common initializer. This looks to be a frequent enough problem that solving it would require a significant effort. It's not a quick fix.

Show 5 quoted lines

> In that context I'd really like to see some testing of submodules in the
> added tests, i.e. does this act as we'd like it to when you set the
> config as "true" in the parent, but "false" in the submodule, & the
> other way around? That's a case that should stress this "the_repository"
> v.s. "istate->repo".

I can add a test for this, though we do not do that for every new config option. Further, we can only rely on commands like 'git add' that correctly initialize the index from a do_read_index(). It should be sufficient to use the index-local repository (when available) and trust that the repo_config_...() method is doing the right thing.

Show 12 quoted lines

> I really don't care per-se where we read the config, as long as it's
> doing what we expect from the UX point of view.
> 
> But in your
> https://lore.kernel.org/git/9e754af8-ecd3-6aed-60e8-2fc09a6a8759@github.com/
> you noted "There's no reason to load the config inside repo-settings.c
> unless it's part of something like feature.manyFiles.".
> 
> I think that's true, but on the flip side of that: Is there a reason to
> move the reading of such localized config (only needed in read-cache.c,
> as 2/4 shows) to repo-settings.c, just because it's now relying on the
> "manyfiles"?

Yes, because it makes it clear what options are part of these meta-config keys. It is better to centralize the parsing instead of having several different ad-hoc parsing strategies.

Thanks, -Stolee