diff mbox series

[3/5] format_trailer_info(): append newline for non-trailer lines

Message ID 259a4edcfa5138053255dbade411ef151617bb1b.1710485706.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 9f0c9702de9c87aa30697d88c4dc9ad0610f4201
Headers show
Series Unify trailer formatting functions | expand

Commit Message

Linus Arver March 15, 2024, 6:55 a.m. UTC 
From: Linus Arver <linusa@google.com>

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 can pass again without the need to hide failures
with *_failure, so flip the affected test cases back to *_success. Now,
format_trailer_info() is in better shape to supersede format_trailers(),
which we'll do in the next commit.

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t4205-log-pretty-formats.sh | 12 ++++++------
 t/t6300-for-each-ref.sh       | 16 ++--------------
 trailer.c                     |  5 +++--
 3 files changed, 11 insertions(+), 22 deletions(-)

Comments

Junio C Hamano March 15, 2024, 5:22 p.m. UTC | #1 
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> This wraps up the preparatory refactors to unify the trailer formatters.
> ...
> The test suite can pass again without the need to hide failures
> with *_failure, so flip the affected test cases back to *_success. Now,
> format_trailer_info() is in better shape to supersede format_trailers(),
> which we'll do in the next commit.

Nicely done.  Queued.

>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  t/t4205-log-pretty-formats.sh | 12 ++++++------
>  t/t6300-for-each-ref.sh       | 16 ++--------------
>  trailer.c                     |  5 +++--
>  3 files changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 339e0c892ef..e3d655e6b8b 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -675,7 +675,7 @@ test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trail
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure '%(trailers:unfold) unfolds trailers' '
> +test_expect_success '%(trailers:unfold) unfolds trailers' '
>  	git log --no-walk --pretty="%(trailers:unfold)" >actual &&
>  	{
>  		unfold <trailers &&
> @@ -737,7 +737,7 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
> +test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
>  	git log --no-walk --pretty="format:%(trailers:key=Acked-by,only=no)" >actual &&
>  	{
>  		echo "Acked-by: A U Thor <author@example.com>" &&
> @@ -752,7 +752,7 @@ test_expect_success '%(trailers:key) without value is error' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure '%(trailers:keyonly) shows only keys' '
> +test_expect_success '%(trailers:keyonly) shows only keys' '
>  	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
>  	test_write_lines \
>  		"Signed-off-by" \
> @@ -774,7 +774,7 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure '%(trailers:valueonly) shows only values' '
> +test_expect_success '%(trailers:valueonly) shows only values' '
>  	git log --no-walk --pretty="format:%(trailers:valueonly)" >actual &&
>  	test_write_lines \
>  		"A U Thor <author@example.com>" \
> @@ -813,7 +813,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-value separator' '
> +test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
>  	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual &&
>  	(
>  		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
> @@ -824,7 +824,7 @@ test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-v
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
> +test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
>  	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
>  	(
>  		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2688dcc7b9e..eb6c8204e8b 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1446,19 +1446,7 @@ test_trailer_option () {
>  	'
>  }
>  
> -# Just like test_trailer_option, but expect failure instead of success.
> -test_trailer_option_expect_failure () {
> -	title=$1 option=$2
> -	cat >expect
> -	test_expect_failure "$title" '
> -		git for-each-ref --format="%($option)" refs/heads/main >actual &&
> -		test_cmp expect actual &&
> -		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
> -		test_cmp expect actual
> -	'
> -}
> -
> -test_trailer_option_expect_failure '%(trailers:unfold) unfolds trailers' \
> +test_trailer_option '%(trailers:unfold) unfolds trailers' \
>  	'trailers:unfold' <<-EOF
>  	$(unfold <trailers)
>  
> @@ -1542,7 +1530,7 @@ test_trailer_option '%(trailers:key=foo,unfold) properly unfolds' \
>  
>  	EOF
>  
> -test_trailer_option_expect_failure '%(trailers:key=foo,only=no) also includes nontrailer lines' \
> +test_trailer_option '%(trailers:key=foo,only=no) also includes nontrailer lines' \
>  	'trailers:key=Signed-off-by,only=no' <<-EOF
>  	Signed-off-by: A U Thor <author@example.com>
>  	$(grep patch.description <trailers)
> diff --git a/trailer.c b/trailer.c
> index 2c0dd8ac829..fe8b0819d55 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1124,9 +1124,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');
>  		}
>  	}
>  }
diff mbox series

Patch

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 339e0c892ef..e3d655e6b8b 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -675,7 +675,7 @@  test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trail
 	test_cmp expect actual
 '
 
-test_expect_failure '%(trailers:unfold) unfolds trailers' '
+test_expect_success '%(trailers:unfold) unfolds trailers' '
 	git log --no-walk --pretty="%(trailers:unfold)" >actual &&
 	{
 		unfold <trailers &&
@@ -737,7 +737,7 @@  test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,only=no)" >actual &&
 	{
 		echo "Acked-by: A U Thor <author@example.com>" &&
@@ -752,7 +752,7 @@  test_expect_success '%(trailers:key) without value is error' '
 	test_cmp expect actual
 '
 
-test_expect_failure '%(trailers:keyonly) shows only keys' '
+test_expect_success '%(trailers:keyonly) shows only keys' '
 	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
 	test_write_lines \
 		"Signed-off-by" \
@@ -774,7 +774,7 @@  test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
-test_expect_failure '%(trailers:valueonly) shows only values' '
+test_expect_success '%(trailers:valueonly) shows only values' '
 	git log --no-walk --pretty="format:%(trailers:valueonly)" >actual &&
 	test_write_lines \
 		"A U Thor <author@example.com>" \
@@ -813,7 +813,7 @@  test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
 	test_cmp expect actual
 '
 
-test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-value separator' '
+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
 	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual &&
 	(
 		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
@@ -824,7 +824,7 @@  test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-v
 	test_cmp expect actual
 '
 
-test_expect_failure 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
+test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
 	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
 	(
 		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2688dcc7b9e..eb6c8204e8b 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1446,19 +1446,7 @@  test_trailer_option () {
 	'
 }
 
-# Just like test_trailer_option, but expect failure instead of success.
-test_trailer_option_expect_failure () {
-	title=$1 option=$2
-	cat >expect
-	test_expect_failure "$title" '
-		git for-each-ref --format="%($option)" refs/heads/main >actual &&
-		test_cmp expect actual &&
-		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
-		test_cmp expect actual
-	'
-}
-
-test_trailer_option_expect_failure '%(trailers:unfold) unfolds trailers' \
+test_trailer_option '%(trailers:unfold) unfolds trailers' \
 	'trailers:unfold' <<-EOF
 	$(unfold <trailers)
 
@@ -1542,7 +1530,7 @@  test_trailer_option '%(trailers:key=foo,unfold) properly unfolds' \
 
 	EOF
 
-test_trailer_option_expect_failure '%(trailers:key=foo,only=no) also includes nontrailer lines' \
+test_trailer_option '%(trailers:key=foo,only=no) also includes nontrailer lines' \
 	'trailers:key=Signed-off-by,only=no' <<-EOF
 	Signed-off-by: A U Thor <author@example.com>
 	$(grep patch.description <trailers)
diff --git a/trailer.c b/trailer.c
index 2c0dd8ac829..fe8b0819d55 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1124,9 +1124,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');
 		}
 	}
 }