Re: [PATCH 01/10] config API: have *_multi() return an "int" and take a "dest"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 26, 2022, 19:33 UTC
Message-ID: <221026.86pmeebcj9.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221026184915.GA1828@szeder.dev>

On Wed, Oct 26 2022, SZEDER Gábor wrote:

Show 12 quoted lines

> On Wed, Oct 26, 2022 at 05:35:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> The git_configset_get_value_multi() function added in 3c8687a73ee (add
>> `config_set` API for caching config-like files, 2014-07-28) is a
>> fundamental part of of the config API, and
>> e.g. "git_config_get_value()" and others are implemented in terms of
>> it.
>> 
>> But it has had the limitation that configset_find_element() calls
>> git_config_parse_key(), but then throws away the distinction between a
>> "ret < 1" return value from it, and return values that indicate a key
>
> Shouldn't that be "ret < 0"?

Yes, sorry, that's just a typo. It's <0 for API errors (e.g. unable to parse your key bad key), 0 for OK, 1 for key doesn't exist.

Show 24 quoted lines

>> doesn't exist. As a result the git_config_get_value_multi() function
>> would either return a "const struct string_list *", or NULL.
>> 
>> By changing the *_multi() function to return an "int" for the status
>> and to write to a "const struct string_list **dest" parameter we can
>> avoid losing this information. API callers can now do:
>> 
>> 	const struct string_list *dest;
>> 	int ret;
>> 
>> 	ret = git_config_get_value_multi(key, &dest);
>> 	if (ret < 1)
>
> This catches all negative values and zero.
>
>> 		die("bad key: %s", key);
>> 	else if (ret)
>
> This catches all non-zero values.
>
>> 		; /* key does not exist */
>> 	else
>
> So how could this ever be executed?!

Yes, sorry. It's the same typo/thinko.

Show 11 quoted lines

>> 		; /* got key, can use "dest" */
>> 
>> A "get_knownkey_value_multi" variant is also provided, which will
>> BUG() out in the "ret < 1" case. This is useful in the cases where we
>
> Shouldn't that be "ret < 0" as well?  The condition in that "knownkey"
> variant added in this patch is:
>
>   +	ret = configset_find_element(cs, key, &e);
>   +	if (ret < 0 && knownkey)
>   +		BUG("*_get_knownkey_*() only accepts known-good (hardcoded) keys, but '%s' is bad!", key);

Yes, FWIW the code isn't incorrect in this regard, I just screwed up the commit message, sorry.

The canonical example that isn't tricky is in builtin/for-each-repo.c, i.e.:

        err = repo_config_get_value_multi_string(the_repository, config_key, &values);
        if (err < 0)
                usage_msg_optf(_("got bad config --config=%s"),
                               for_each_repo_usage, options, config_key);
        else if (err)
                return 0;

I.e. it wants to ignore non-existing config ("else if"), but now we distinguish that from errors. The *_multi() API on master doesn't allow for that.