threads / patch / 61124

patch, 5 partsUnify trailer formatting functions

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

## tl;dr

16 messages between Mar 15, 2024 and Jul 2, 2024. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Linus Arver via GitGitGadget· Mar 15, 2024, 06:55 UTC · lore

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
Linus Arver via GitGitGadget· Mar 15, 2024, 06:55 UTC · re: Linus Arver via GitGitGadget · lore

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

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(-)
Show changes to 3 files +30 −19

t/t4205-log-pretty-formats.sh, t/t6300-for-each-ref.sh, trailer.c

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
Linus Arver via GitGitGadget· Mar 15, 2024, 06:55 UTC · re: Linus Arver via GitGitGadget · lore

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

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(-)
Show changes to trailer.c +0 −3
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
Linus Arver via GitGitGadget· Mar 15, 2024, 06:55 UTC · re: Linus Arver via GitGitGadget · lore

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

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(-)
Show changes to 3 files +11 −22

t/t4205-log-pretty-formats.sh, t/t6300-for-each-ref.sh, trailer.c

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
Junio C Hamano· Mar 15, 2024, 17:22 UTC · re: Linus Arver via GitGitGadget · lore

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

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 117 quoted lines
>
> 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');
>  		}
>  	}
>  }
Linus Arver via GitGitGadget· Mar 15, 2024, 06:55 UTC · re: Linus Arver via GitGitGadget · lore

[PATCH 4/5] trailer: begin formatting unification

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(-)
Show changes to 3 files +19 −39

builtin/interpret-trailers.c, trailer.c, trailer.h

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
Linus Arver via GitGitGadget· Mar 15, 2024, 06:55 UTC · re: Linus Arver via GitGitGadget · lore

[PATCH 5/5] trailer: finish formatting unification

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(-)
Show changes to 3 files +9 −16

builtin/interpret-trailers.c, trailer.c, trailer.h

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
Junio C Hamano· Mar 15, 2024, 17:20 UTC · re: Linus Arver via GitGitGadget · lore

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

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.
Kristoffer Haugsbakk· Mar 15, 2024, 18:26 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 15, 2024, at 18:20, Junio C Hamano wrote:
Show 19 quoted lines
> 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
Junio C Hamano· Mar 15, 2024, 19:10 UTC · re: Kristoffer Haugsbakk · lore

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

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

Linus Arver· Mar 15, 2024, 21:36 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> 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.

Junio C Hamano· Mar 15, 2024, 21:43 UTC · re: Linus Arver · lore

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

Linus Arver <linusa@google.com> writes:
Show 21 quoted lines
> 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.
Johannes Schindelin· Jul 2, 2024, 09:37 UTC · re: Linus Arver · lore

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

Hi,
On Fri, 15 Mar 2024, Linus Arver wrote:
Show 41 quoted lines
> 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

Junio C Hamano· Mar 26, 2024, 21:57 UTC · re: Linus Arver via GitGitGadget · lore

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

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

Linus Arver· Apr 2, 2024, 00:27 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> "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!

Junio C Hamano· Apr 15, 2024, 21:02 UTC · re: Linus Arver · lore

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

Linus Arver <linusa@google.com> writes:
Show 16 quoted lines
> 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.

← back to recent threads