Re: BUG: config.c:129: kvi should not be set while parsing a config source

[Glen Choo <chooglen@google.com>]

Thanks Jesse for the bug report, and thanks Stolee for looping me in!

Derrick Stolee <derrickstolee@github.com> writes:

> On 6/21/2023 7:45 PM, Jesse Kasky wrote:
>> Thank you for filling out a Git bug report!
>> Please answer the following questions to help us understand your issue.
>> 
>> What did you do before the bug happened? (Steps to reproduce your issue)
>> 
>> git clone --no-checkout --sparse --filter=blob:none --depth=1 <repo> <dir>
>> cd <dir>
>> git sparse-checkout add <dir1> <dir2>
>> git fetch --depth=1 origin <commit>
>> Received the following error:
>> 
>> BUG: config.c:129: kvi should not be set while parsing a config source
>> [1]    5842 abort      /opt/homebrew/bin/git fetch --depth=1 origin

It took me some fiddling around to find a reproducing case, because, as
it turns out, the choice of repo is important. This bug occurs when you
have submodules + partial clone, _and_ .gitmodules has not been fetched
yet (e.g. because --no-checkout skipped the fetch).

E.g. you can see this for yourself with:

  rm -fr test-breakage &&
  git clone --filter=blob:none --no-checkout https://github.com/git/git test-breakage &&
  cd test-breakage &&
  git fetch

This BUG() assertion is meant to guard against reading config in an
unsafe nested way - the information about the config source we are
reading (i.e. the key_value_info/kvi) is global state in the config
machinery, and certain kinds of nesting will produce undefined
behavior if we try to read it.

What happens is that "git fetch" tries to read .gitmodules, which
triggers a partial clone fetch because .gitmodules is missing. But to do
this, the partial clone code reads config to figure out where to fetch
from, leading to a nested config-read-in-a-config-read.

For Git devs: the 'unsafe' thing is reading a config set while reading a
config file/params/blob/etc. In this case, we are trying to read
repo_config() (the config set in the_repository->config) while reading
.gitmodules from a blob. Reading from files while reading files is safe
(e.g. that's what we do when evaluating includes).

This should go away in my in-flight series:

  https://lore.kernel.org/git/pull.1497.v3.git.git.1687290231.gitgitgadget@gmail.com

because that removes all of the problematic global state. That's a very
big refactor, so I'm not sure we should wait until that goes in to fix
this bug. And while I'd like to fix this bug now, I don't see a point in
making the existing code 'safer' by making small changes in the
machinery that will just go away anyway.

So I think the best fix is for me to get rid of the BUG() check and go
back to the 'unsafe' behavior. It won't matter in this specific scenario
(since we don't actually need the config source information AFAICT), and
at least it doesn't make _other_ use cases worse off than before.

> I've come across this error while playing around with things in
> the config space, and the case I figured out was due to nested
> iterations over config.
>
> In my case, I was adding dynamic config loading when certain
> global variables were used, and some were used during config
> parsing causing this nesting.

Presumably this is how you were coming across this bug too: since your
experiments added config set lookups, maybe some of them were happening
in the middle of reading a config file.