Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Linus Arver <linusa@xxxxxxxxxx>
>>
>> Currently have two functions for formatting trailers exposed in
>> trailer.h:
>>
>> void format_trailers(FILE *outfile, struct list_head *head,
>> const struct process_trailer_options *opts);
>>
>> void format_trailers_from_commit(struct strbuf *out, const char *msg,
>> const struct process_trailer_options *opts);
>>
>> and previously these functions, although similar enough (even taking the
>> same process_trailer_options struct pointer), did not build on each
>> other.
>>
>> Make format_trailers_from_commit() rely on format_trailers(). Teach
>> format_trailers() to process trailers with the additional
>> process_trailer_options fields like opts->key_only which is only used by
>> format_trailers_from_commit() and not builtin/interpret-trailers.c.
>
> Yay. It feels a bit disappointing to see the diffstat and learn
> that we are not deleting substantial number of lines.
Indeed ;)
>> ---
>> builtin/interpret-trailers.c | 5 +-
>> pretty.c | 2 +-
>> ref-filter.c | 2 +-
>> trailer.c | 105 +++++++++++++++++++++++++++++------
>> trailer.h | 21 +++----
>> 5 files changed, 102 insertions(+), 33 deletions(-)
>
>> diff --git a/pretty.c b/pretty.c
>> index cf964b060cd..f0721a5214f 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>> goto trailer_out;
>> }
>> if (*arg == ')') {
>> - format_trailers_from_commit(sb, msg + c->subject_off, &opts);
>> + format_trailers_from_commit(msg + c->subject_off, &opts, sb);
>
> I am curious (read: no objection---merely wondering if there is a
> guiding principle behind the choice of the new order) why this new
> parameter ordering.
Glen Choo told me a while ago [1] that we usually put "out parameters"
at the end, and somehow that stuck with me.
> I suspect it was originally written with a
> strbuf-centric worldview and having sb at the beginning may have
> made sense,
Having gotten more familiar with the strbuf.h functions since this patch
was originally written, I agree.
> but if we are moving them around, wouldn't it make more
> sense to have &opts as the first parameter, as this is primarily a
> "trailers" function? Unsure until I read through to the end, but
> that is my gut reaction.
I simply (mechanically) moved "sb" from the beginning of the parameters
to the end, and didn't think much beyond that adjustment.
Your ordering seems fine to me. Noted for the next reroll, but I will
wait until you're done reviewing the other patches before going with the
new ordering (in case you change your mind).
>> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
>> {
>> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
>> @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val)
>> strbuf_release(&out);
>> }
>>
>> +void format_trailers(struct list_head *head,
>> + const struct process_trailer_options *opts,
>> + struct strbuf *out)
>> +{
>> + struct list_head *pos;
>> + struct trailer_item *item;
>> + int need_separator = 0;
>> +
>> + list_for_each(pos, head) {
>> + item = list_entry(pos, struct trailer_item, list);
>> + if (item->token) {
>> + char c;
>> + ...
>> + if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>> + if (opts->unfold)
>> + unfold_value(&val);
>> +
>> + if (opts->separator && need_separator)
>> + strbuf_addbuf(out, opts->separator);
>> + if (!opts->value_only)
>> + strbuf_addbuf(out, &tok);
>> + if (!opts->key_only && !opts->value_only) {
>> + if (opts->key_value_separator)
>> + strbuf_addbuf(out, opts->key_value_separator);
>> + else {
>> + c = last_non_space_char(tok.buf);
>> + if (c) {
>> + if (!strchr(separators, c))
>> + strbuf_addf(out, "%c ", separators[0]);
>> + }
>> + }
>
> That's an overly deep nesting. I wonder if a small file-scope
> helper function is in order?
>
> static add_separator(struct process_trailer_options *opts,
> const char *token
> struct strbuf *out)
> {
> if (opts->key_value_separator)
> strbuf_addbuf(out, opts->key_value_separator);
> else
> strbuf_addstr(out, ": ");
> }
>
> Or perhaps inside the context of the loop to go over the list of
> trailer items, one iteration of the list_for_each() loop can become
> one call to a small helper function format_one_trailer() and that
> may make the result easier to follow?
This is actually what I've already done (introducing helper functions to
make this more readable and reduce nesting), but in the larger series
(not in this patch series). I think in this patch I tried to avoid
introducing new functions in order to keep the original "shape" of the
existing refactored functions, including all of the nesting that they
originally had. I wanted to keep the original shape because I thought
that would make review simpler.
> In any case, I didn't see anything glaringly wrong so far in this
> series. Let me keep reading.
>
> Thanks.
Thank you for reviewing!
[1] https://lore.kernel.org/git/kl6l5y5qa34v.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
