On Tue, Feb 13, 2024 at 6:05 PM Linus Arver <linusa@xxxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> writes:
> >> 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);
