Re: [PATCH 3/5] config: add BUG() statement instead of possible segfault

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Sep 27 2022, Derrick Stolee wrote:

> On 9/27/2022 12:17 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:
>> 
>>> From: Derrick Stolee <derrickstolee@github.com>
>>>
>>> The git_die_config() method calls git_config_get_value_multi() but
>>> immediately navigates to its first value without checking if the result
>>> is NULL or empty. Callers should only call git_die_config() if there is
>>> at least one value for the given 'key', but such a mistaken use might
>>> slip through. It would be better to show a BUG() statement than a
>>> possible segfault.
>  
>> AFAIKT the intent of the current code on "master" is that this will only
>> get called if the likes of git_configset_get_string() returns < 0, not
>> if it returns > 0.
>> 
>> So isn't the combination of your 1/5 and this 3/5 now conflating these
>> two conditions? See e.g. repo_config_get_string_tmp() and when it would
>> call git_die_config().
>> 
>> I.e. isn't the whole point of git_die_config() to print an error message
>> about a configuration *value* that we've parsed out of the config?
>> 
>> If e.g. the key itself is bad we'll get a -1, but in this case it seems
>> we would have a BUG(), but it's not that we "expected a non-empty list
>> of values", but that the state of the world changed between our previous
>> configset invocation, no?
>
> If git_die_config() was static to config.c, then I would agree with you
> that its use is controlled enough to avoid that possibility. However, it
> is available in config.h and its doc comment does not say anything about
> "make sure 'key' properly parses at least one value".

It does, it's part of the configset API, which any non-trivial user
understands adhares to the semantics of the configset cache.

E.g. the fast-import.c caller is doing, basically:

	if (!git_config_get_int("pack.indexversion", &indexversion_value))
		/* ...if we're not happy with indexversion_value ...*/
		git_die_config("pack.indexversion", ...);

Anyway, I'm all for adding some extra paranoia with a BUG(), it was just
unclear to me if this was some segfault we were expecting to perhaps
have now, or with API use after this series.

AFAICT it's really neither of those, but just some side-paranoia added
while-at-it for a *different* use-case than the primary goal of this
series. I.e. one where you're looking up a key you don't know exists,
whereas git_die_config() is for a key we *know* exists, but don't like
its value.

I think it's better if we just clarify that it's supposed to be used by
that in the API docs, and if you insist on the BUG then the:

	BUG("expected a non-empty list of values");

Should really get to the point directly, and say something like:

	BUG("called with non-existing key '%s'? "
            "Call git_die_config(...) only with keys whose value you don't like", key);