Re: [PATCH v4 14/28] format_trailer_info(): teach it about opts->trim_empty

[Linus Arver <linusa@xxxxxxxxxx>]

Christian Couder <christian.couder@xxxxxxxxx> writes:

>> diff --git a/trailer.c b/trailer.c
>> index f4defad3dae..c28b6c11cc5 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -162,20 +162,6 @@ static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
>>                 strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
>>  }
>>
>> -static void format_trailers(const struct process_trailer_options *opts,
>> -                           struct list_head *trailers,
>> -                           struct strbuf *out)
>> -{
>> -       struct list_head *pos;
>> -       struct trailer_item *item;
>> -       list_for_each(pos, trailers) {
>> -               item = list_entry(pos, struct trailer_item, list);
>> -               if ((!opts->trim_empty || strlen(item->value) > 0) &&
>> -                   (!opts->only_trailers || item->token))
>> -                       print_tok_val(out, item->token, item->value);
>> -       }
>> -}
>
> It seems to me that this function could and should have been removed
> in the previous patch. If there is a reason why it is better to do it
> in this patch, I think it should be explained more clearly in the
> commit message.

Ah yes, the decision to delay the deletion like this was deliberate.
Will update the commit message to add something like:

    Although we could have deleted format_trailers() in the previous patch,
    we perform the deletion here like this in order to isolate
    (and highlight) in this patch the salvaging of the logic in the deleted
    code

        if ((!opts->trim_empty || strlen(item->value) > 0) && ...)
            print_tok_val(...)

    as

        if (opts->trim_empty && !strlen(item->value))
            continue;

    in the new code, which has the same effect (because we are skipping the
    formatting in the new code).

>>  static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
>>  {
>>         struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
>> @@ -1101,6 +1087,15 @@ void format_trailer_info(const struct process_trailer_options *opts,
>>                         strbuf_addstr(&tok, item->token);
>>                         strbuf_addstr(&val, item->value);
>>
>> +                       /*
>> +                        * Skip key/value pairs where the value was empty. This
>> +                        * can happen from trailers specified without a
>> +                        * separator, like `--trailer "Reviewed-by"` (no
>> +                        * corresponding value).
>> +                        */
>> +                       if (opts->trim_empty && !strlen(item->value))
>> +                               continue;
>> +
>
> Wasn't it possible to make this change in format_trailer_info() before
> using format_trailer_info() to replace format_trailers()?

It was certainly possible, but the choice to purposely time the
addition/deletion of code like this was deliberate (see my comment
above).

>>                         if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>>                                 if (opts->separator && out->len != origlen)
>>                                         strbuf_addbuf(out, opts->separator);