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

Commit Message

Linus Arver Feb. 6, 2024, 5:12 a.m. UTC

From: Linus Arver <[e-mail]>

This fixes 4 tests in t7513 to go from

    t7513-interpret-trailers.sh  (Wstat: 256 (exited 1) Tests: 94 Failed: 55)
      Failed tests:  2-5, 8, 14, 24-28, 31-37, 43-62, 66-74
                    77-80, 82-85

to

    t7513-interpret-trailers.sh  (Wstat: 256 (exited 1) Tests: 94 Failed: 51)
      Failed tests:  2-5, 14, 24-28, 31-32, 36-37, 43-62, 66-74
                    77-80, 82-85

. The next patch will fix the remaining broken test cases in t7513 and
t7502.

Even though the next patch fixes the vast majority of these test cases,
we have to position that patch after this one to avoid breaking the
build because of the way these patches delete relevant (and obsolete)
code.

Signed-off-by: Linus Arver <[e-mail]>
---
 trailer.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

Comments

Christian Couder Feb. 12, 2024, 11:38 p.m. UTC | #1

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

>  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()?


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

Linus Arver Feb. 13, 2024, 5:05 p.m. UTC | #2

Christian Couder <[e-mail]> 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);

Christian Couder Feb. 13, 2024, 5:21 p.m. UTC | #3

On Tue, Feb 13, 2024 at 6:05 PM Linus Arver <[e-mail]> wrote:
> Christian Couder <[e-mail]> 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);

Patch

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);
-	}
-}
-
 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;
+
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->separator && out->len != origlen)
 					strbuf_addbuf(out, opts->separator);