[v4,12/28] format_trailer_info(): append newline for non-trailer lines

Commit Message

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

From: Linus Arver <[e-mail]>

This wraps up the preparatory refactors to unify the trailer formatters.

Two patches ago we made format_trailer_info() use trailer_item objects
instead of the "trailers" string array. The strings in the array
include trailing newlines, because the string array is split up with

    trailer_lines = strbuf_split_buf(str + trailer_block_start,
                                     end_of_log_message - trailer_block_start,
                                     '\n',
                                     0);

in trailer_info_get() and strbuf_split_buf() includes the terminator (in
this case the newline character '\n') for each split-up substring.

And before we made the transition to use trailer_item objects for it,
format_trailer_info() called parse_trailer() (which trims newlines) for
trailer lines but did _not_ call parse_trailer() for non-trailer lines.
So for trailer lines it had to add back the trimmed newline like this

    if (!opts->separator)
        strbuf_addch(out, '\n');

But for non-trailer lines it didn't have to add back the newline because
it could just reuse same string in the "trailers" string array (which
again, already included the trailing newline).

Now that format_trailer_info() uses trailer_item objects for all cases,
it can't rely on "trailers" string array anymore.  And so it must be
taught to add a newline back when printing non-trailer lines, just like
it already does for trailer lines. Do so now.

The test suite passes again, so format_trailer_info() is in better shape
supersede format_trailers(), which we'll do in the next patch.

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

Comments

Junio C Hamano Feb. 9, 2024, 9:53 p.m. UTC | #1

"Linus Arver via GitGitGadget" <[e-mail]> writes:

> And before we made the transition to use trailer_item objects for it,
> format_trailer_info() called parse_trailer() (which trims newlines) for
> trailer lines but did _not_ call parse_trailer() for non-trailer lines.
> So for trailer lines it had to add back the trimmed newline like this
>
>     if (!opts->separator)
>         strbuf_addch(out, '\n');
>
> But for non-trailer lines it didn't have to add back the newline because
> it could just reuse same string in the "trailers" string array (which
> again, already included the trailing newline).
>
> Now that format_trailer_info() uses trailer_item objects for all cases,
> it can't rely on "trailers" string array anymore.  And so it must be
> taught to add a newline back when printing non-trailer lines, just like
> it already does for trailer lines. Do so now.

Very nicely explained.

> The test suite passes again, so format_trailer_info() is in better shape
> supersede format_trailers(), which we'll do in the next patch.
>
> Signed-off-by: Linus Arver <[e-mail]>
> ---
>  trailer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 12cae5b73d2..0774a544c4f 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1125,9 +1125,10 @@ static void format_trailer_info(const struct process_trailer_options *opts,
>  				strbuf_addbuf(out, opts->separator);
>  			}
>  			strbuf_addstr(out, item->value);
> -			if (opts->separator) {
> +			if (opts->separator)
>  				strbuf_rtrim(out);
> -			}
> +			else
> +				strbuf_addch(out, '\n');
>  		}
>  	}
>  }

Christian Couder Feb. 12, 2024, 11:37 p.m. UTC | #2

On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<[e-mail]> wrote:

[...]

> The test suite passes again, so format_trailer_info() is in better shape
> supersede format_trailers(), which we'll do in the next patch.

s/supersede/to supersede/

Linus Arver Feb. 13, 2024, 4:49 p.m. UTC | #3

Christian Couder <[e-mail]> writes:

> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>
> [...]
>
>> The test suite passes again, so format_trailer_info() is in better shape
>> supersede format_trailers(), which we'll do in the next patch.
>
> s/supersede/to supersede/

Nice catch! Will update for next reroll, thanks.

Patch

diff --git a/trailer.c b/trailer.c
index 12cae5b73d2..0774a544c4f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1125,9 +1125,10 @@  static void format_trailer_info(const struct process_trailer_options *opts,
 				strbuf_addbuf(out, opts->separator);
 			}
 			strbuf_addstr(out, item->value);
-			if (opts->separator) {
+			if (opts->separator)
 				strbuf_rtrim(out);
-			}
+			else
+				strbuf_addch(out, '\n');
 		}
 	}
 }