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

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 13, 2024, 17:21 UTC
Message-ID: <CAP8UFD3Mx8TT0vZgNNAVBw1L3QBbRtbRL_mR1KGe6FNK77oMeg@mail.gmail.com>
In-Reply-To: <owlyttmcb8ge.fsf@fine.c.googlers.com>

On Tue, Feb 13, 2024 at 6:05 PM Linus Arver <linusa@google.com> wrote:

> Christian Couder <christian.couder@gmail.com> writes:

Show 23 quoted lines

> >>  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).

I would have thought that it would be better to make this change earlier to avoid breaking tests.

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