On Thu, Feb 09 2023, Glen Choo wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>> [...]
>> This still leaves various inconsistencies and clobbering or ignoring
>> of the return value in place. E.g here we're modifying
>> configset_add_value(), but ever since it was added in [2] we've been
>> ignoring its "int" return value, but as we're changing the
>> configset_find_element() it uses, let's have it faithfully ferry that
>> "ret" along.
>>
>> Let's also use the "RESULT_MUST_BE_USED" macro introduced in [3] to
>> assert that we're checking the return value of
>> configset_find_element().
>>
>> We're leaving the same change to configset_add_value() for some future
>> series. Once we start paying attention to its return value we'd need
>> to ferry it up as deep as do_config_from(), and would need to make
>> least read_{,very_}early_config() and git_protected_config() return an
>> "int" instead of "void". Let's leave that for now, and focus on
>> the *_get_*() functions.
>>
>> In a subsequent commit we'll fix the other *_get_*() functions to so
>> that they'll ferry our underlying "ret" along, rather than normalizing
>> it to a "return 1". But as an intermediate step to that we'll need to
>> fix git_configset_get_value_multi() to return "int", and that change
>> itself is smaller because of this change to migrate some callers away
>> from the *_value_multi() API.
>
> I haven't read ahead, but on first impression this sounds like it might
> be too intrusive for a series whose goal is to clean up
> *_get_value_multi().
Yeah, that was my inclination too :) But Glen seemed to have a strong
opinion on the end-state of the topic being inconsistent in its API
(which he's right about, some stuff returning -1 or 1, some only 1).
I wanted to just leave it for a follow-up topic I've got to fix various
warts in the API, but cherry-picked & included the new 06/10 here to
address that concern.
I'm also confident that we can expose this to current API users, so
partly I'm playing reviewer flip-flop here and seeing what sticks. If
you feel it should be ejected I'm also happy to do that, and re-roll...
