Subject: Re: [PATCH 09/10] config API: add "string" version of *_value_multi(), fix segfaults

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 28 Oct 2022 01:44:19 +0200

On Thu, Oct 27 2022, Junio C Hamano wrote:

> Junio C Hamano <[email-masked]> writes:
>
>> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>>
>>> Fix numerous and mostly long-standing segfaults in consumers of
>>> the *_config_*value_multi() API. As discussed in the preceding commit
>>> an empty key in the config syntax yields a "NULL" string, which these
>>> users would give to strcmp() (or similar), resulting in segfaults.
>>
>> Sounds like a good idea.
>>
>> I would have called them _nonbool(), not _string(), especially
>> because we are not going to have other variants like _int(), though.
>
> Actually, I take it back.  Instead of introducing _string(), how
> about introducing _bool() and convert those minority callers that do
> want to see boolean values to use the new one, while rejecting NULLs
> for everybody else that calls the traditional "get_value" family of
> functions?  That would "optimize" for the majority of simpler users,
> wouldn't it?

I don't think the goal should be just to optimize for those current
users, but to leave the config API in a state where it makes sense
conceptually.

For the scalar (single) values we have a low-level "low-level" function,
and then variants to get it as a bool, path, string, int etc.

I think a "multi" function should just be the logical result of applying
one of those "types" to list. I.e. (pseudocode):

	a_raw = get_config_raw("a.key");
	a_string = stringify(a_raw);

And, as a list:

	list_raw = get_config_raw_multi("a.key");
	list_strings = map { stringify(item) } list_raw;

Now, if we don't supply the equivalent of the "raw, but multi-value"
function we'll make it hard to use the API, because now you can't think
about it as the "multi" just being a list version of what you get with
the scalar version.

E.g. what should we do about "[a]key" in a list API that stringifies by
default? If you then want "stringified bool" we're only left with bad choices:

 - If you die that's bed, because that's a legit true value
 - If you coerce it to "" to help the string use case you get the wrong
   answer, because "[a]key=" (empty string) is false, but "[a]key"
   (value-less) is true.
 - Ditto if you prune it out, as then it won't be seen in the bool list.

Which is why I went for the end-state here. I.e. it's now easy to add
other "multi" variants (we'd need to add coercion, but that's easy
enough).