Re: [PATCH v7 5/9] config API: have *_multi() return an "int" and take a "dest"

From: Glen Choo <chooglen@google.com>
Date: Mar 9, 2023, 19:01 UTC
Message-ID: <kl6lbkl1hhqo.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-v7-5.9-dced12a40d2-20230308T090513Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 8 quoted lines

> Have the "git_configset_get_value_multi()" function and its siblings
> return an "int" and populate a "**dest" parameter like every other
> git_configset_get_*()" in the API.
>
> As we'll take advantage of in subsequent commits, this fixes a blind
> spot in the API where it wasn't possible to tell whether a list was
> empty from whether a config key existed. For now we don't make use of
> those new return values, but faithfully convert existing API users.

I think the commit message is fine as-is, but perhaps you intended to include this paragraph from v4 [1]?

    A logical follow-up to this would be to change the various "*_get_*()"
    functions to ferry the git_configset_get_value() return value to their
    own callers, e.g. git_configset_get_int() returns "1" rather than
    ferrying up the "-1" that "git_configset_get_value()" might return,
    but that's not being done in this series

Which is nice, but the commit message reads fine without it too.

1. https://lore.kernel.org/git/patch-v4-5.9-23449ff2c4e-20230202T131155Z-avarab@gmail.com/

Show 21 quoted lines

>
> Most of this is straightforward, commentary on cases that stand out:
>
> - To ensure that we'll properly use the return values of this function
>   in the future we're using the "RESULT_MUST_BE_USED" macro introduced
>   in [1].
>
>   As git_die_config() now has to handle this return value let's have
>   it BUG() if it can't find the config entry. As tested for in a
>   preceding commit we can rely on getting the config list in
>   git_die_config().
>
> - The loops after getting the "list" value in "builtin/gc.c" could
>   also make use of "unsorted_string_list_has_string()" instead of using
>   that loop, but let's leave that for now.
>
> - In "versioncmp.c" we now use the return value of the functions,
>   instead of checking if the lists are still non-NULL.
>
> 1. 1e8697b5c4e (submodule--helper: check repo{_submodule,}_init()
>    return values, 2022-09-01),