Christian Couder <christian.couder@xxxxxxxxx> writes:
> 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().
Oops, fixed locally.
> Nit: this patch and the next one will become commits, so perhaps:
>
> s/In the next patch/In a following commit/
TBH I've always wondered whether "patch" or "commit" matters --- I've
seen examples of patch series that referred to "commits" instead of
"patches", and vice versa. I was hoping to hear an opinion on this, so
I'm happy to see (and apply) your suggestion. Thanks.
>> 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.
I will add something like:
Rename process_trailers() to interpret_trailers(), because it
matches the name for the builtin command of the same name
(git-interpret-trailers), which is the sole user of
process_trailers().
> 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'".
Applied.
> 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".
Sounds like a very sensible rule. IOW, leave the detailed explanation to
the commit message and use the subject line only for the most obvious
explanation of what's going on in the patch. +1
>> 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.
SGTM, will do.
>> 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.
Ah nice catch. Will do.
Really appreciate the quality of your reviews, thanks so much! :)
