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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Derrick Stolee" <derrickstolee@github.com>,
	"Jeff King" <peff@peff.net>, "Taylor Blau" <me@ttaylorr.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH 09/10] config API: add "string" version of *_value_multi(), fix segfaults
Date: Mon, 31 Oct 2022 19:22:45 +0100	[thread overview]
Message-ID: <221031.868rkv7s70.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqmt9fiws7.fsf@gitster.g>

On Fri, Oct 28 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> 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.
>
> It is more like guiding a conceptually clean design using the need
> of the current users to rein in pursuit of theoretical "elegance".

I agree that the current code users don't care either way, they'll be
getting the same thing.

But being able to readily understand an API is valuable too. The config
API is bad enough with all repetition of:

	{git_configset,repo_config,git_config}_get_value()
	{git_configset,repo_config,git_config}_get_string()
	{git_configset,repo_config,git_config}_get_bool()
        [...]

I think it's worth it to be able to say that:

	{git_configset,repo_config,git_config}_get_value_multi()
	{git_configset,repo_config,git_config}_get_value_string()
        <ditto "bool">

Are "just like the scalar version, but multi". Actually when I summarize
it like that I realize I should really make it:

	{git_configset,repo_config,git_config}_get_string_multi()

I.e. "*_get_string_multi()", not "*_get_value_multi_string()". I don't
know what I was thinking.

But aside from that, the point is I think it's worth it not to have it
instead be:

        # "non-string" doesn't exist, but get it via some use of
        # (currently static) configset_find_element()

        # "get value", but really "get string, for multi"
	{git_configset,repo_config,git_config}_get_value_multi()

        # ???
	{git_configset,repo_config,git_config}_get_bool_multi()

We currently don't need/have a "*_get_bool_multi()", which I think is
besides the point. We might in the future, and should forsee that we're
picking a nonsensical naming convention.

We also have similar gaps in the current API (not all variants of all
functions exist, for the scalar variants), but at least those that we do
have behave consistently.

>> 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.
>
> I am not interested in _bool() variant that "stringifies" NULL to
> "true" at all.  What I was suggesting was:
>
>  * Reserve the current get and get_multi for those who should have
>    been calling config_error_nonbool() themselves (because your
>    _string() has not been available to them, they were lazy not to
>    bother, leading to NULL dereference given certain end-user data).
>    And do the config_error_nonbool() inside the updated get and
>    get_multi without introducing _string() variant at all.

I get what you're saying, I just think it suffers from the problem
outlined above, and that it's worth solving it.

>  * The above alone WILL break callers who are prepared to handle
>    "bool" and "bool plus some other string", because they are fully
>    expecting that the get API will give them NULL but the above
>    update will instead stop before they see the NULL they are
>    prepared to handle themselves.  Introduce _bool variants and make
>    them call them.

Even if it wasn't for the naming question, I think the arrangement in
this series is also better in that I need to go and change each caller
to the new variant, and explain for each one why it's OK.

Whereas if we just sneakconfig_error_nonbool() into the low-level API
we're going to have a smaller change, but also one that's basically
"trust me, I read the code of all the callers, this should be fine...".