On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Linus Arver <linusa@xxxxxxxxxx>
>
> 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.
> 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.
