Re: [PATCH v4 3/9] config API: add and use a "git_config_get()" family of functions

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Feb 06 2023, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> We already have the basic "git_config_get_value()" function and its
>> "repo_*" and "configset" siblings to get a given "key" and assign the
>> last key found to a provided "value".
>>
>> But some callers don't care about that value, but just want to use the
>> return value of the "get_value()" function to check whether the key
>> exist (or another non-zero return value).
>
> [...]
>
>> We could have changed git_configset_get_value_multi() to accept a
>> "NULL" as a "dest" for all callers, but let's avoid changing the
>> behavior of existing API users. Having an "unused" value that we throw
>> away internal to config.c is cheap.
>
> There is yet another option, which is to teach "git_config_get_value()"
> (mentioned earlier) to accept NULL to mean "I just want to know if there
> is a value, I don't care what it is". That's what the *_get_<type>()
> functions use under the hood (i.e. the ones that return either 0 or 1 or
> exit).

I've clarified the commit message, but that's the same as what this is
describing. I.e. I meant "git_config_get_value_multi() and the functions
that are wrapping it", which is "git_config_get_value()" etc.

> This amounts to implementing the "*_config_key_exists()" API you
> mentioned, but I think this is better fit for the current set of
> semantics. At the very least, that would be an easy 1-1 replacement for
> the *_get_string[_tmp]() replacements we make here. There's also the
> small benefit of saving one function implementation.

I think this is the wrong approach, and have updated the commit message
further to advocate for this one.

>> Another name for this function could have been
>> "*_config_key_exists()", as suggested in [1]. That would work for all
>> of these callers, and would currently be equivalent to this function,
>> as the git_configset_get_value() API normalizes all non-zero return
>> values to a "1".
>>
>> But adding that API would set us up to lose information, as e.g. if
>> git_config_parse_key() in the underlying configset_find_element()
>> fails we'd like to return -1, not 1.
>
> We were already 'losing' (or rather, not caring about) this information
> with the *_get_<type>() functions. The only reason we'd care about this
> is if we using git_configset_get_value_multi() or similar.
>
> We replace two callers of git_configset_get_value_multi() in this patch,
> but they didn't care about the -1 case anyway...
> [...]
> As Junio pointed out, git_configset_get() can now return -1, so this
> isn't so accurate any more. git_configset_get() is really the exception
> here, since all the other functions in this section are the
> git_configset_get_*() functions that use git_configset_get_value(). I'd
> prefer returning only 0 or 1 for consistency.

I really prefer not clobbering these return values, but I take your
point that the end-state here is inconsistent.

I figured that I could fix it for the APIs added here, and follow-up
(with a patch I already had mostly ready) after this series to fix the
remaining config API warts.

I won't fix all of those in the incoming re-roll of this, but I'll fix
this issue, i.e. we'll consistently ferry up "ret", and stop normalizing
non-zero-non-1 to "return 1".