Re: [PATCH v5 3/9] trailer: prepare to expose functions as part of API

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 19, 2024, 21:31 UTC
Message-ID: <CAP8UFD1dE2EiSxohose6U9SGn+zeHPyVB=KZ2xdQi-v-a8d1XQ@mail.gmail.com>
In-Reply-To: <4372af244f02b71cc70f3a8e1b5591b3b9fec93a.1708124951.git.gitgitgadget@gmail.com>

On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 7 quoted lines

>
> From: Linus Arver <linusa@google.com>
>
> In the next patch, we will move "process_trailers" from trailer.c to
> builtin/interpret-trailers.c. That move will necessitate the growth of
> the trailer.h API, forcing us to expose some additional functions in
> trailer.h.

Nit: actually this patch renames process_trailers() to
interpret_trailers() so the function that will be moved will be
interpret_trailers().

Nit: this patch and the next one will become commits, so perhaps:

s/In the next patch/In a following commit/

> Rename relevant functions so that they include the term "trailer" in
> their name, so that clients of the API will be able to easily identify
> them by their "trailer" moniker, just like all the other functions
> already exposed by trailer.h.

Except that "process_trailers()" already contains "trailer" but will still be renamed by this patch to "interpret_trailers()". So I think it might be nice to explain a bit why renaming process_trailers() to interpret_trailers() makes sense too.

Also I think the subject, "trailer: prepare to expose functions as part of API" could be more explicit about what the patch is actually doing, like perhaps "trailer: rename functions to use 'trailer'".

In general, when there is a patch called "prepare to do X", then we might expect a following patch called something like "actually do X". But there isn't any patch in the series named like "trailer: expose functions as part of API".

> Take the opportunity to start putting trailer processing options (opts)
> as the first parameter. This will be the pattern going forward in this
> series.

It's interesting to know that this will be the pattern going forward in the series, but that doesn't quite tell why it's a good idea to do it.

So I think it might be nice to repeat an explanation similar to the one you give in "trailer: start preparing for formatting unification" for format_trailers_from_commit():

"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 maybe also say that parameters like `FILE *outfile` should be last because they are some kind of 'out' parameters.

Show 12 quoted lines

> diff --git a/trailer.c b/trailer.c
> index f74915bd8cd..916175707d8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
>                 fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
>  }
>
> -static void print_all(FILE *outfile, struct list_head *head,
> -                     const struct process_trailer_options *opts)
> +static void format_trailers(const struct process_trailer_options *opts,
> +                           struct list_head *trailers, FILE *outfile)

This also renames `struct list_head *head` to `struct list_head *trailers`. I think it would be nice if the commit message could talk a bit about these renames too.