On Wed, Oct 26 2022, SZEDER Gábor wrote:
> 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.
>> 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.
>> ; /* 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.
