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? > 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@xxxxxxxxxxxxxxxxxxx/ > 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. > 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. > 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@xxxxxxxxxx/ > 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
