Re: [PATCH v4 26/28] trailer: unify "--trailer ..." arg handling

From: Linus Arver <linusa@google.com>
Date: Feb 13, 2024, 18:12 UTC
Message-ID: <owly7cj8b5d6.fsf@fine.c.googlers.com>
In-Reply-To: <CAP8UFD3u8qNpxObdOJDfBq+zVfxNwAG56bBcPeSC4i2=ZuhWjw@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 11 quoted lines

> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> Move the logic of parse_trailer_from_command_line_arg() into
>> option_parse_trailer(), because that is the only caller and there's no
>> benefit in keeping these two separate.
>
> Well one benefit could be that 2 small functions might be easier to
> understand than a big one.

True.

> So perhaps
> parse_trailer_from_command_line_arg() could just have been made
> static?

In this case I don't think keeping these two functions separate would make sense because parse_trailer_from_command_line_arg() is much more heavyweight than option_parse_trailer() (one is just a thin wrapper around the other). And I didn't like the thought of having 2 function names that look very different:

    parse_trailer_from_command_line_arg()
    option_parse_trailer()

be so closely related in behavior.

And I already have some more patches (not in this series) that refactors this area a bit also, so I wanted to wait until the dust settled down a bit before deciding (esp. when unit tests are added) whether keep this function separate. It's not clear to me yet whether we do want to add unit tests for parse_trailer_from_command_line_arg() (if we do end up "resurrecting it" in this patch or later), so IDK. The main reason is because I think the first set of unit tests should be for the exposed functions in <trailer.h>, not so much the helper functions that only the builtin uses.