Re: [PATCH] config: don't BUG when both kvi and source are set

From: Glen Choo <chooglen@google.com>
Date: Jun 26, 2023, 22:56 UTC
Message-ID: <kl6l7crpsuhs.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq352e59h9.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

>> Therefore, fix the bug by removing the BUG() check. We're reverting to
>> an older, less safe state, but that's generally okay since
>> key_value_info is always preferentially read, so we'd always read the
>> correct values when we iterate a config set in the middle of a config
>> parse (like we are here).
>
> I wonder if the source being pushed and config_kvi value at this
> point have some particular relationship (like "if kvi exists, the
> source must match kvi's source" or something) that we can cheaply
> use to avoid "reverting to an older less safe state"?

Not at all. In this case, the source should reflect .gitmodules, but the config_kvi should reflect the promisor config (aka the full repo config). config_source implements stack semantics, so we could co-opt it by e.g. converting config_kvi into a fake config_source and pushing it onto the stack (at which point, we could just get rid of config_kvi altogether too), but that's really way too much work for something that will _hopefully_ go away soon.

Show 5 quoted lines

>> The reverse would be wrong, but extremely
>> unlikely to happen since very few callers parse config without going
>> through a config set.
>
> Sorry, but I do not quite get this comment.

Ah, I meant that this bug occurred because most users of config use git_config()/repo_config() (a wrapper around config sets), so it's very easy to accidentally read repo config, e.g. in the middle of parsing config (config file -> config set). I'd imagine it might also be quite easy to read repo config while reading repo config (config set -> config set), which would make current_config_* return the wrong thing, but at least it doesn't BUG().

The "reverse" case (config set -> config file) is very _unlikely_ because very few places need to know about config files, so it's unlikely that we'd have an explicit call to parse a config file, especially in the middle of reading repo config.