Unify trailer formatting functions

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/[email protected]/
[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

From: Linus Arver <[email protected]>

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.

---
 t/t4205-log-pretty-formats.sh | 12 ++++++------
 t/t6300-for-each-ref.sh       | 16 ++++++++++++++--
 trailer.c                     | 21 ++++++++++-----------
 3 files changed, 30 insertions(+), 19 deletions(-)

index e3d655e6b8b..339e0c892ef 100755
@@ -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 <[email protected]>" &&

@@ -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 <[email protected]>" \

@@ -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 <[email protected]>\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 <[email protected]>\n" &&

index eb6c8204e8b..2688dcc7b9e 100755
@@ -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 <[email protected]>
 	$(grep patch.description <trailers)

index 57b4aa7d5ac..a74f05db55c 100644
@@ -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

From: Linus Arver <[email protected]>

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.

---
 trailer.c | 3 ---
 1 file changed, 3 deletions(-)

index a74f05db55c..2c0dd8ac829 100644
@@ -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

From: Linus Arver <[email protected]>

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.

---
 t/t4205-log-pretty-formats.sh | 12 ++++++------
 t/t6300-for-each-ref.sh       | 16 ++--------------
 trailer.c                     |  5 +++--
 3 files changed, 11 insertions(+), 22 deletions(-)

index 339e0c892ef..e3d655e6b8b 100755
@@ -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 <[email protected]>" &&

@@ -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 <[email protected]>" \

@@ -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 <[email protected]>\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 <[email protected]>\n" &&

index 2688dcc7b9e..eb6c8204e8b 100755
@@ -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 <[email protected]>
 	$(grep patch.description <trailers)

index 2c0dd8ac829..fe8b0819d55 100644
@@ -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

From: Linus Arver <[email protected]>

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.

---
 builtin/interpret-trailers.c |  2 +-
 trailer.c                    | 54 ++++++++++++------------------------
 trailer.h                    |  2 +-
 3 files changed, 19 insertions(+), 39 deletions(-)

index 11f4ce9e4a2..f57af0db37b 100644
@@ -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);

index 1d106b6dd40..3c13006a4c1 100644
@@ -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

From: Linus Arver <[email protected]>

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.

---
 builtin/interpret-trailers.c |  2 +-
 trailer.c                    |  8 ++++----
 trailer.h                    | 15 ++++-----------
 3 files changed, 9 insertions(+), 16 deletions(-)

index f57af0db37b..11f4ce9e4a2 100644
@@ -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);

index 43d5baef9ce..3e4dab9c065 100644
@@ -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);

index 3c13006a4c1..9f42aa75994 100644
@@ -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

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/[email protected]/

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.

Nicely done. Queued.

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

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.

UGH, I'm so sorry about that.

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

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.

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.

Hi,

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