[PATCH 0/5] Unify trailer formatting functions

[PATCH 0/5] Unify trailer formatting functions

[Linus Arver via GitGitGadget]

This series is based on the initial series [1], notably the v4 version of
patches 10-16 as suggested by Christian [2]. This version addresses the
review comments for those patches, namely the avoidance of (temporary) test
breakages.

The central idea is to unify the functions format_trailer_info() and
format_trailers() together (preferring the name of the latter), so that both
the interpret-trailers built-in and format_trailers_from_commit() can call
the same format_trailers() function, while maintaining feature parity.

[1]
https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/
[2]
https://lore.kernel.org/git/CAP8UFD08F0V13X0+CJ1uhMPzPWVMs2okGVMJch0DkQg5M3BWLA@mail.gmail.com/

Linus Arver (5):
  format_trailer_info(): use trailer_item objects
  format_trailer_info(): drop redundant unfold_value()
  format_trailer_info(): append newline for non-trailer lines
  trailer: begin formatting unification
  trailer: finish formatting unification

 trailer.c | 81 ++++++++++++++++++++-----------------------------------
 trailer.h | 13 +++------
 2 files changed, 32 insertions(+), 62 deletions(-)


base-commit: 4f9b731bdeccffa1b13e5edf4bc0428b8d49704e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1694%2Flistx%2Ftrailer-api-part-2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1694/listx/trailer-api-part-2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1694
-- 
gitgitgadget

[PATCH 1/5] format_trailer_info(): use trailer_item objects

[Linus Arver via GitGitGadget]

From: Linus Arver <linusa@google.com>

This is another preparatory refactor to unify the trailer formatters.

Make format_trailer_info() operate on trailer_item objects, not the raw
string array.

We will continue to make improvements, culminating in the renaming of
format_trailer_info() to format_trailers(), at which point the
unification of these formatters will be complete.

In order to avoid breaking t4205 and t6300, flip *_success to *_failure
in the affected test cases. Add a temporary
"test_trailer_option_expect_failure" wrapper which we will use along
with "test_expect_failure" in the next commit to avoid breaking tests.
When the dust settles with the refactors a few more commits later, we
will drop the use of *_failure to make the tests truly pass again.

When the preparatory refactors are complete,
we'll be able to drop the use of *_failure that we introduce here.

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

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e3d655e6b8b..339e0c892ef 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_success '%(trailers:unfold) unfolds trailers' '
+test_expect_failure '%(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_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
+test_expect_failure '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_success '%(trailers:keyonly) shows only keys' '
+test_expect_failure '%(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_success '%(trailers:valueonly) shows only values' '
+test_expect_failure '%(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_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
+test_expect_failure '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_success 'pretty format %(trailers:key_value_separator) changes key-v
 	test_cmp expect actual
 '
 
-test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
+test_expect_failure '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 eb6c8204e8b..2688dcc7b9e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1446,7 +1446,19 @@ test_trailer_option () {
 	'
 }
 
-test_trailer_option '%(trailers:unfold) unfolds trailers' \
+# 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' \
 	'trailers:unfold' <<-EOF
 	$(unfold <trailers)
 
@@ -1530,7 +1542,7 @@ test_trailer_option '%(trailers:key=foo,unfold) properly unfolds' \
 
 	EOF
 
-test_trailer_option '%(trailers:key=foo,only=no) also includes nontrailer lines' \
+test_trailer_option_expect_failure '%(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 57b4aa7d5ac..a74f05db55c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1085,21 +1085,21 @@ void trailer_info_release(struct trailer_info *info)
 }
 
 static void format_trailer_info(const struct process_trailer_options *opts,
-				const struct trailer_info *info,
+				struct list_head *trailers,
 				struct strbuf *out)
 {
 	size_t origlen = out->len;
-	size_t i;
-
-	for (i = 0; i < info->trailer_nr; i++) {
-		char *trailer = info->trailers[i];
-		ssize_t separator_pos = find_separator(trailer, separators);
+	struct list_head *pos;
+	struct trailer_item *item;
 
-		if (separator_pos >= 1) {
+	list_for_each(pos, trailers) {
+		item = list_entry(pos, struct trailer_item, list);
+		if (item->token) {
 			struct strbuf tok = STRBUF_INIT;
 			struct strbuf val = STRBUF_INIT;
+			strbuf_addstr(&tok, item->token);
+			strbuf_addstr(&val, item->value);
 
-			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->unfold)
 					unfold_value(&val);
@@ -1126,13 +1126,12 @@ static void format_trailer_info(const struct process_trailer_options *opts,
 			if (opts->separator && out->len != origlen) {
 				strbuf_addbuf(out, opts->separator);
 			}
-			strbuf_addstr(out, trailer);
+			strbuf_addstr(out, item->value);
 			if (opts->separator) {
 				strbuf_rtrim(out);
 			}
 		}
 	}
-
 }
 
 void format_trailers_from_commit(const struct process_trailer_options *opts,
@@ -1151,7 +1150,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
 		strbuf_add(out, msg + info.trailer_block_start,
 			   info.trailer_block_end - info.trailer_block_start);
 	} else
-		format_trailer_info(opts, &info, out);
+		format_trailer_info(opts, &trailer_objects, out);
 
 	free_trailers(&trailer_objects);
 	trailer_info_release(&info);
-- 
gitgitgadget

[PATCH 2/5] format_trailer_info(): drop redundant unfold_value()

[Linus Arver via GitGitGadget]

From: Linus Arver <linusa@google.com>

This is another preparatory refactor to unify the trailer formatters.

In the last patch we made format_trailer_info() use trailer_item objects
instead of the "trailers" string array. This means that the call to
unfold_value() here is redundant because the trailer_item objects are
already unfolded in parse_trailers() which is a dependency of our
caller, format_trailers_from_commit().

Remove the redundant call.

Signed-off-by: Linus Arver <linusa@google.com>
---
 trailer.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/trailer.c b/trailer.c
index a74f05db55c..2c0dd8ac829 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1101,9 +1101,6 @@ static void format_trailer_info(const struct process_trailer_options *opts,
 			strbuf_addstr(&val, item->value);
 
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
-				if (opts->unfold)
-					unfold_value(&val);
-
 				if (opts->separator && out->len != origlen)
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
-- 
gitgitgadget

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

[Linus Arver via GitGitGadget]

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

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');
 		}
 	}
 }
-- 
gitgitgadget

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

[Junio C Hamano]

"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');
>  		}
>  	}
>  }

[PATCH 4/5] trailer: begin formatting unification

[Linus Arver via GitGitGadget]

From: Linus Arver <linusa@google.com>

Now that the preparatory refactors are over, we can replace the call to
format_trailers() in interpret-trailers with format_trailer_info(). This
unifies the trailer formatting machinery

In order to avoid breakages in t7502 and t7513, we have to steal the
features present in format_trailers(). Namely, we have to teach
format_trailer_info() as follows:

  (1) make it aware of opts->trim_empty, and

  (2) make it avoid hardcoding ": " as the separator and space (which
  can result in double-printing these characters).

For (2), make it only print the separator and space if we cannot find
any recognized separator somewhere in the key (yes, keys may have a
trailing separator in it --- we will eventually fix this design but not
now). Do so by copying the code out of print_tok_val(), and deleting the
same function.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c |  2 +-
 trailer.c                    | 54 ++++++++++++------------------------
 trailer.h                    |  2 +-
 3 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 11f4ce9e4a2..f57af0db37b 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	}
 
 	/* Print trailer block. */
-	format_trailers(opts, &head, &trailer_block);
+	format_trailer_info(opts, &head, &trailer_block);
 	free_trailers(&head);
 	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
 	strbuf_release(&trailer_block);
diff --git a/trailer.c b/trailer.c
index fe8b0819d55..43d5baef9ce 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,38 +144,6 @@ static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
-{
-	char c;
-
-	if (!tok) {
-		strbuf_addf(out, "%s\n", val);
-		return;
-	}
-
-	c = last_non_space_char(tok);
-	if (!c)
-		return;
-	if (strchr(separators, c))
-		strbuf_addf(out, "%s%s\n", tok, val);
-	else
-		strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
-}
-
-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));
@@ -1084,9 +1052,9 @@ void trailer_info_release(struct trailer_info *info)
 	free(info->trailers);
 }
 
-static void format_trailer_info(const struct process_trailer_options *opts,
-				struct list_head *trailers,
-				struct strbuf *out)
+void format_trailer_info(const struct process_trailer_options *opts,
+			 struct list_head *trailers,
+			 struct strbuf *out)
 {
 	size_t origlen = out->len;
 	struct list_head *pos;
@@ -1100,6 +1068,15 @@ static 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);
@@ -1108,8 +1085,11 @@ static void format_trailer_info(const struct process_trailer_options *opts,
 				if (!opts->key_only && !opts->value_only) {
 					if (opts->key_value_separator)
 						strbuf_addbuf(out, opts->key_value_separator);
-					else
-						strbuf_addstr(out, ": ");
+					else {
+						char c = last_non_space_char(tok.buf);
+						if (c && !strchr(separators, c))
+							strbuf_addf(out, "%c ", separators[0]);
+					}
 				}
 				if (!opts->key_only)
 					strbuf_addbuf(out, &val);
diff --git a/trailer.h b/trailer.h
index 1d106b6dd40..3c13006a4c1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -101,7 +101,7 @@ void trailer_info_get(const struct process_trailer_options *,
 void trailer_info_release(struct trailer_info *info);
 
 void trailer_config_init(void);
-void format_trailers(const struct process_trailer_options *,
+void format_trailer_info(const struct process_trailer_options *,
 		     struct list_head *trailers,
 		     struct strbuf *out);
 void free_trailers(struct list_head *);
-- 
gitgitgadget

[PATCH 5/5] trailer: finish formatting unification

[Linus Arver via GitGitGadget]

From: Linus Arver <linusa@google.com>

Rename format_trailer_info() to format_trailers(). Finally, both
interpret-trailers and format_trailers_from_commit() can call
"format_trailers()"!

Update the comment in <trailer.h> to remove the (now obsolete) caveats
about format_trailers_from_commit(). Those caveats come from
a388b10fc1 (pretty: move trailer formatting to trailer.c, 2017-08-15)
where it says:

    pretty: move trailer formatting to trailer.c

    The next commit will add many features to the %(trailer)
    placeholder in pretty.c. We'll need to access some internal
    functions of trailer.c for that, so our options are either:

      1. expose those functions publicly

    or

      2. make an entry point into trailer.c to do the formatting

    Doing (2) ends up exposing less surface area, though do note
    that caveats in the docstring of the new function.

which suggests format_trailers_from_commit() started out from pretty.c
and did not have access to all of the trailer implementation internals,
and was never intended to replace (unify) the formatting machinery in
trailer.c. The refactors leading up to this commit (as well as
additional refactors that will follow) expose additional functions
publicly, and is therefore choosing option (1) as described in
a388b10fc1.

Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c |  2 +-
 trailer.c                    |  8 ++++----
 trailer.h                    | 15 ++++-----------
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f57af0db37b..11f4ce9e4a2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	}
 
 	/* Print trailer block. */
-	format_trailer_info(opts, &head, &trailer_block);
+	format_trailers(opts, &head, &trailer_block);
 	free_trailers(&head);
 	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
 	strbuf_release(&trailer_block);
diff --git a/trailer.c b/trailer.c
index 43d5baef9ce..3e4dab9c065 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1052,9 +1052,9 @@ void trailer_info_release(struct trailer_info *info)
 	free(info->trailers);
 }
 
-void format_trailer_info(const struct process_trailer_options *opts,
-			 struct list_head *trailers,
-			 struct strbuf *out)
+void format_trailers(const struct process_trailer_options *opts,
+		     struct list_head *trailers,
+		     struct strbuf *out)
 {
 	size_t origlen = out->len;
 	struct list_head *pos;
@@ -1128,7 +1128,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
 		strbuf_add(out, msg + info.trailer_block_start,
 			   info.trailer_block_end - info.trailer_block_start);
 	} else
-		format_trailer_info(opts, &trailer_objects, out);
+		format_trailers(opts, &trailer_objects, out);
 
 	free_trailers(&trailer_objects);
 	trailer_info_release(&info);
diff --git a/trailer.h b/trailer.h
index 3c13006a4c1..9f42aa75994 100644
--- a/trailer.h
+++ b/trailer.h
@@ -101,23 +101,16 @@ void trailer_info_get(const struct process_trailer_options *,
 void trailer_info_release(struct trailer_info *info);
 
 void trailer_config_init(void);
-void format_trailer_info(const struct process_trailer_options *,
+void format_trailers(const struct process_trailer_options *,
 		     struct list_head *trailers,
 		     struct strbuf *out);
 void free_trailers(struct list_head *);
 
 /*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
- *
- *   - this is primarily a helper for pretty.c, and not
- *     all of the flags are supported.
- *
- *   - this differs from process_trailers slightly in that we always format
- *     only the trailer block itself, even if the "only_trailers" option is not
- *     set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers() internally.
  */
-void format_trailers_from_commit(const struct process_trailer_options *opts,
+void format_trailers_from_commit(const struct process_trailer_options *,
 				 const char *msg,
 				 struct strbuf *out);
 
-- 
gitgitgadget

Re: [PATCH 0/5] Unify trailer formatting functions

[Junio C Hamano]

Not about the series, but about the way it was sent.

The messages in this series have exactly the same kind of breakages
in the recipient names/addresses we recently saw:

    https://lore.kernel.org/git/xmqqh6hkxox6.fsf@gitster.g/

Human-readable names with a SP inside [square bracket] pair
appended, and one of the addresses had that square bracket applied
inside <angle bracket> pair and breaking MTAs (I manually fixed
khaugsbakk's address before sending this response, so replying to
this messages should be OK).

What are you and Aryan's pull.1675.v3 did differently from other
series sent via GGG to trigger this, I have to wonder?  Without
knowing it, it would be hard to avoid seeing these broken addresses
again.

Thanks.

Re: [PATCH 0/5] Unify trailer formatting functions

[Kristoffer Haugsbakk]

On Fri, Mar 15, 2024, at 18:20, Junio C Hamano wrote:
> Not about the series, but about the way it was sent.
>
> The messages in this series have exactly the same kind of breakages
> in the recipient names/addresses we recently saw:
>
>     https://lore.kernel.org/git/xmqqh6hkxox6.fsf@gitster.g/
>
> Human-readable names with a SP inside [square bracket] pair
> appended, and one of the addresses had that square bracket applied
> inside <angle bracket> pair and breaking MTAs (I manually fixed
> khaugsbakk's address before sending this response, so replying to
> this messages should be OK).
>
> What are you and Aryan's pull.1675.v3 did differently from other
> series sent via GGG to trigger this, I have to wonder?  Without
> knowing it, it would be hard to avoid seeing these broken addresses
> again.
>
> Thanks.

I’m the only common CC on these two. Maybe dropping me for the possible
next round would fix it. Would be interesting to see.

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH 0/5] Unify trailer formatting functions

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> I’m the only common CC on these two. Maybe dropping me for the possible
> next round would fix it. Would be interesting to see.

But there is nothing strange in your name or address.  There may be
"something" that confuses GGG about how they spelled your name and
address, and if the next user does that same "something" for some
other recipient, it wouldn't be you but that other recipient with
the same issue, I am afraid.

Re: [PATCH 0/5] Unify trailer formatting functions

[Linus Arver]

Junio C Hamano <gitster@pobox.com> writes:

> Not about the series, but about the way it was sent.
>
> The messages in this series have exactly the same kind of breakages
> in the recipient names/addresses we recently saw:
>
>     https://lore.kernel.org/git/xmqqh6hkxox6.fsf@gitster.g/
>
> Human-readable names with a SP inside [square bracket] pair
> appended, and one of the addresses had that square bracket applied
> inside <angle bracket> pair and breaking MTAs (I manually fixed
> khaugsbakk's address before sending this response, so replying to
> this messages should be OK).

UGH, I'm so sorry about that.

> What are you and Aryan's pull.1675.v3 did differently from other
> series sent via GGG to trigger this, I have to wonder?

I realize now that it's because I copy/pasted the "Cc: ..." lines in the PR
description from
https://github.com/gitgitgadget/git/pull/1632#issue-2068188239, such
that when I pasted those in for the PR description for this series at
https://github.com/gitgitgadget/git/pull/1694#issue-2187804953, it
carried over the email addresses as Markdown-formatted hyperlinks.
Currently it reads

    Cc: Christian Couder [chriscool@tuxfamily.org](mailto:chriscool@tuxfamily.org)
    Cc: Junio C Hamano [gitster@pobox.com](mailto:gitster@pobox.com)
    Cc: Emily Shaffer [nasamuffin@google.com](mailto:nasamuffin@google.com)
    cc: Josh Steadmon [steadmon@google.com](mailto:steadmon@google.com)
    cc: Randall S. Becker [rsbecker@nexbridge.com](mailto:rsbecker@nexbridge.com)
    cc: Christian Couder [christian.couder@gmail.com](mailto:christian.couder@gmail.com)
    cc: "Kristoffer Haugsbakk" [code@khaugsbakk.name](mailto:code@khaugsbakk.name)
    cc: "Kristoffer Haugsbakk" <code@khaugsbakk.name>

when I click on "edit", where the last line must be from your manual fix
which GGG picked up. I've cleaned up the PR description manually now,
and for this message I'm also attempting to clean up those square
brackets.

Re: [PATCH 0/5] Unify trailer formatting functions

[Junio C Hamano]

Linus Arver <linusa@google.com> writes:

> I realize now that it's because I copy/pasted the "Cc: ..." lines in the PR
> description from
> https://github.com/gitgitgadget/git/pull/1632#issue-2068188239, such
> that when I pasted those in for the PR description for this series at
> https://github.com/gitgitgadget/git/pull/1694#issue-2187804953, it
> carried over the email addresses as Markdown-formatted hyperlinks.
> Currently it reads
>
>     Cc: Christian Couder [chriscool@tuxfamily.org](mailto:chriscool@tuxfamily.org)
>     Cc: Junio C Hamano [gitster@pobox.com](mailto:gitster@pobox.com)
>     Cc: Emily Shaffer [nasamuffin@google.com](mailto:nasamuffin@google.com)
>     cc: Josh Steadmon [steadmon@google.com](mailto:steadmon@google.com)
>     cc: Randall S. Becker [rsbecker@nexbridge.com](mailto:rsbecker@nexbridge.com)
>     cc: Christian Couder [christian.couder@gmail.com](mailto:christian.couder@gmail.com)
>     cc: "Kristoffer Haugsbakk" [code@khaugsbakk.name](mailto:code@khaugsbakk.name)
>     cc: "Kristoffer Haugsbakk" <code@khaugsbakk.name>
>
> when I click on "edit", where the last line must be from your manual fix
> which GGG picked up. I've cleaned up the PR description manually now,
> and for this message I'm also attempting to clean up those square
> brackets.

The last time I asked the author with the same question, we
unfortunately did not get a clear answer on what should be avoided
(probably the author was happy enough with the message resulted from
the updated pull request and forgot that it is more important to
help others from having the same issue).  Now we have an example we
can point at when a similar issue arises.

Thanks.

Re: [PATCH 0/5] Unify trailer formatting functions

[Johannes Schindelin]

Hi,

On Fri, 15 Mar 2024, Linus Arver wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Not about the series, but about the way it was sent.
> >
> > The messages in this series have exactly the same kind of breakages
> > in the recipient names/addresses we recently saw:
> >
> >     https://lore.kernel.org/git/xmqqh6hkxox6.fsf@gitster.g/
> >
> > Human-readable names with a SP inside [square bracket] pair
> > appended, and one of the addresses had that square bracket applied
> > inside <angle bracket> pair and breaking MTAs (I manually fixed
> > khaugsbakk's address before sending this response, so replying to
> > this messages should be OK).
>
> UGH, I'm so sorry about that.
>
> > What are you and Aryan's pull.1675.v3 did differently from other
> > series sent via GGG to trigger this, I have to wonder?
>
> I realize now that it's because I copy/pasted the "Cc: ..." lines in the PR
> description from
> https://github.com/gitgitgadget/git/pull/1632#issue-2068188239, such
> that when I pasted those in for the PR description for this series at
> https://github.com/gitgitgadget/git/pull/1694#issue-2187804953, it
> carried over the email addresses as Markdown-formatted hyperlinks.
> Currently it reads
>
>     Cc: Christian Couder [chriscool@tuxfamily.org](mailto:chriscool@tuxfamily.org)
>     Cc: Junio C Hamano [gitster@pobox.com](mailto:gitster@pobox.com)
>     Cc: Emily Shaffer [nasamuffin@google.com](mailto:nasamuffin@google.com)
>     cc: Josh Steadmon [steadmon@google.com](mailto:steadmon@google.com)
>     cc: Randall S. Becker [rsbecker@nexbridge.com](mailto:rsbecker@nexbridge.com)
>     cc: Christian Couder [christian.couder@gmail.com](mailto:christian.couder@gmail.com)
>     cc: "Kristoffer Haugsbakk" [code@khaugsbakk.name](mailto:code@khaugsbakk.name)
>     cc: "Kristoffer Haugsbakk" <code@khaugsbakk.name>
>
> when I click on "edit", where the last line must be from your manual fix
> which GGG picked up. I've cleaned up the PR description manually now,
> and for this message I'm also attempting to clean up those square
> brackets.

I would love to let myself be nerdsniped into working on this, alas,
I cannot afford that before I learn the trick to stretch time.

So I did the next-best thing and jotted down pointers for any volunteer
who wants to work on this:
https://github.com/gitgitgadget/gitgitgadget/issues/1645#issuecomment-2202545542

Ciao,
Johannes

Re: [PATCH 0/5] Unify trailer formatting functions

[Junio C Hamano]

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This series is based on the initial series [1], notably the v4 version of
> patches 10-16 as suggested by Christian [2]. This version addresses the
> review comments for those patches, namely the avoidance of (temporary) test
> breakages.

It has been 10 days but we haven't seen any reviews on this one.  It
could be that it did not get to intended mailboxes due to the header
corruption (I manually fixed in this message), but without reviews
we cannot move forward.

Re: [PATCH 0/5] Unify trailer formatting functions

[Linus Arver]

Junio C Hamano <gitster@pobox.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This series is based on the initial series [1], notably the v4 version of
>> patches 10-16 as suggested by Christian [2]. This version addresses the
>> review comments for those patches, namely the avoidance of (temporary) test
>> breakages.
>
> It has been 10 days but we haven't seen any reviews on this one.  It
> could be that it did not get to intended mailboxes due to the header
> corruption (I manually fixed in this message), but without reviews
> we cannot move forward.

Adding a couple more folks to the CC list according to the
contrib/contacts/git-contacts script. Thanks!

Re: [PATCH 0/5] Unify trailer formatting functions

[Junio C Hamano]

Linus Arver <linusa@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> This series is based on the initial series [1], notably the v4 version of
>>> patches 10-16 as suggested by Christian [2]. This version addresses the
>>> review comments for those patches, namely the avoidance of (temporary) test
>>> breakages.
>>
>> It has been 10 days but we haven't seen any reviews on this one.  It
>> could be that it did not get to intended mailboxes due to the header
>> corruption (I manually fixed in this message), but without reviews
>> we cannot move forward.
>
> Adding a couple more folks to the CC list according to the
> contrib/contacts/git-contacts script. Thanks!

As we saw no responses after two weeks, I took some time to scan
these patches myself for the final time.  I didn't see anything
gravely wrong in it and am tempted to merge it down to 'next'.

Unless I hear objections in the next few days, that is.

Thanks.