patchconfig: don't BUG when both kvi and source are set

From: Glen Choo <chooglen@google.com>

When iterating through config, we read config source metadata from global values - either a "struct config_source + enum config_scope" or a "struct key_value_info", using the current_config* functions. Prior to the series starting from 0c60285147 (config.c: create config_reader and the_reader, 2023-03-28), we weren't very picky about which values we should read in which situation; we did note that both groups of values generally shouldn't be set together, but if both were set, current_config* preferentially reads key_value_info. When that series added more structure, we enforced that either the former (when parsing a config source) can be set, or the latter (when iterating a config set), but *never* both at the same time. See 9828453ff0 (config.c: remove current_config_kvi, 2023-03-28) and 5cdf18e7cd (config.c: remove current_parsing_scope, 2023-03-28).

That was a good simplifying constraint that helped us reason about the global state, but it turns out that there is at least one situation where we need both to be set at the same time: in a blobless partial clone where .gitmodules is missing. "git fetch" in such a repo will start a config parse over .gitmodules (setting the config_source), and Git will attempt to lazy-fetch it from the promisor remote. However, when we try to read the promisor configuration, we start iterating a config set (setting the key_value_info), and we BUG() out because that's not allowed any more.

Teaching config_reader to gracefully handle this is somewhat complicated, but fortunately, there are proposed changes to the config.c machinery to get rid of this global state, and make the BUG() obsolete [1]. We should rely on that as the eventual solution, and avoid doing yet another refactor in the meantime.

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). The reverse would be wrong, but extremely unlikely to happen since very few callers parse config without going through a config set.

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
    config: don't BUG when both kvi and source are set
    
    Here's a quick fix for the bug reported at [1]. As noted in the commit
    message and that thread, I think the real fix to take [2], which
    simplifies the config.c state and makes this a non-issue, so this is
    just a band-aid while we wait for that.
    
    [1]
    https://lore.kernel.org/git/CAJSLrw6qhHj8Kxrqhp7xN=imTHgg79QB9Fxa9XpdZYFnBKhkvA@mail.gmail.com/
    [2]
    https://lore.kernel.org/git/pull.1497.v3.git.git.1687290231.gitgitgadget@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1535%2Fchooglen%2Fpush-ppuusrxwqpkt-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1535/chooglen/push-ppuusrxwqpkt-v1
Pull-Request: https://github.com/git/git/pull/1535

 config.c                 |  6 ------
 t/t5616-partial-clone.sh | 10 ++++++++--
 2 files changed, 8 insertions(+), 8 deletions(-)

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

I would agree that, as long as we know by the end of this summer a real fix would come to rescue us ;-), it is sensible not to add too much code to work it around for the short-term.

Sorry, but I do not quite get this comment.

Thanks for a quick fix.  Will queue.

We only tentatively configured uploadpack in src_with_sub in the original because this single test piece was the only place where src_with_sub repository was used, but now we use a more permanent configuration because ...

... we run another "git clone" from the repository now.

OK.

Thanks.

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

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.

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.

Glen Choo <chooglen@google.com> writes:

I think BUG() is better than silently computing a wrong result, but it would probably be much rare than the problem at hand, and with the getting rid of global dependencies, it won't be an issue anymore, hopefull? So it is good.

As long as existing codepaths do not do that, it would be OK ;-)

Thanks.