On Thu, Oct 27 2022, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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).
