Re: [PATCH v5 5/9] trailer: start preparing for formatting unification

> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> Currently there are two functions for formatting trailers in
>> <trailer.h>:
>>
>>     void format_trailers(const struct process_trailer_options *,
>>                          struct list_head *trailers, FILE *outfile);
>>
>>     void format_trailers_from_commit(struct strbuf *out, const char *msg,
>>                                      const struct process_trailer_options *opts);
>>
>> and although they are similar enough (even taking the same
>> process_trailer_options struct pointer) they are used quite differently.
>> One might intuitively think that format_trailers_from_commit() builds on
>> top of format_trailers(), but this is not the case. Instead
>> format_trailers_from_commit() calls format_trailer_info() and
>> format_trailers() is never called in that codepath.
>>
>> This is a preparatory refactor to help us deprecate format_trailers() in
>> favor of format_trailer_info() (at which point we can rename the latter
>> to the former). When the deprecation is complete, both
>> format_trailers_from_commit(), and the interpret-trailers builtin will
>> be able to call into the same helper function (instead of
>> format_trailers() and format_trailer_info(), respectively). Unifying the
>> formatters is desirable because it simplifies the API.
>>
>> Reorder parameters for format_trailers_from_commit() to prefer
>>
>>     const struct process_trailer_options *opts
>>
>> as the first parameter, because these options are intimately tied to
>> formatting trailers. And take
>>
>>     struct strbuf *out
>>
>> last, because it's an "out parameter" (something that the caller wants
>> to use as the output of this function).
>
> Here also I think the subject could be more specific like for example:
>
> "trailer: reorder format_trailers_from_commit() parameters"

>> diff --git a/trailer.c b/trailer.c
>> index d23afa0a65c..5025be97899 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info)
>>         free(info->trailers);
>>  }
>>
>> -static void format_trailer_info(struct strbuf *out,
>> +static void format_trailer_info(const struct process_trailer_options *opts,
>>                                 const struct trailer_info *info,
>>                                 const char *msg,
>> -                               const struct process_trailer_options *opts)
>> +                               struct strbuf *out)
>
> Ok, so it's not just format_trailers_from_commit() parameters that are
> reordered, but also format_trailer_info() parameters. It would be nice
> if the commit message mentioned it.