On Thu, Feb 09 2023, Ævar Arnfjörð Bjarmason wrote:
> 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).
Hrm, so clearly I lost track of who I was replying to there, sorry :)
I thought this was a reply from Junio at the time.
But the rest of this stands, i.e. I thought I'd integrate this based on
your feedback on the previous version.
> 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...
