diff mbox series

[v2,3/3] ref-filter: use pretty.c logic for trailers

Message ID 7b8cfb2721c349f2bcebec98f84291b1cffd3b49.1611954543.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Unify trailers formatting logic for pretty.c and ref-filter.c | expand

Commit Message

Hariom verma Jan. 29, 2021, 9:09 p.m. UTC
From: Hariom Verma <hariom18599@gmail.com>

Now, ref-filter is using pretty.c logic for setting trailer options.

New to ref-filter:
  :key=<K> - only show trailers with specified key.
  :valueonly[=val] - only show the value part.
  :separator=<SEP> - inserted between trailer lines.
  :key_value_separator=<SEP> - inserted between trailer lines

Enhancement to existing options(now can take value and its optional):
  :only[=val]
  :unfold[=val]

'val' can be: true, on, yes or false, off, no.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 Documentation/git-for-each-ref.txt |  39 ++++++++--
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 3 files changed, 160 insertions(+), 34 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Jan. 30, 2021, 8:45 p.m. UTC | #1
On Fri, Jan 29 2021, Hariom Verma via GitGitGadget wrote:

> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 2962f85a502..ea1f8417176 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -260,11 +260,40 @@ contents:lines=N::
>  	The first `N` lines of the message.
>  
>  Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
> -are obtained as `trailers` (or by using the historical alias
> -`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
> -with `trailers:only`. Whitespace-continuations can be removed from trailers so
> -that each trailer appears on a line by itself with its full content with
> -`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
> +are obtained as `trailers[:options]` (or by using the historical alias
> +`contents:trailers[:options]`). Valid [:option] are:
> +** 'key=<K>': only show trailers with specified key. Matching is done
> +   case-insensitively and trailing colon is optional. If option is
> +   given multiple times trailer lines matching any of the keys are
> +   shown. This option automatically enables the `only` option so that
> +   non-trailer lines in the trailer block are hidden. If that is not
> +   desired it can be disabled with `only=false`.  E.g.,
> +   `%(trailers:key=Reviewed-by)` shows trailer lines with key
> +   `Reviewed-by`.
> +** 'only[=val]': select whether non-trailer lines from the trailer
> +   block should be included. The `only` keyword may optionally be
> +   followed by an equal sign and one of `true`, `on`, `yes` to omit or
> +   `false`, `off`, `no` to show the non-trailer lines. If option is
> +   given without value it is enabled. If given multiple times the last
> +   value is used.
> +** 'separator=<SEP>': specify a separator inserted between trailer
> +   lines. When this option is not given each trailer line is
> +   terminated with a line feed character. The string SEP may contain
> +   the literal formatting codes. To use comma as separator one must use
> +   `%x2C` as it would otherwise be parsed as next option. If separator
> +   option is given multiple times only the last one is used.
> +   E.g., `%(trailers:key=Ticket,separator=%x2C)` shows all trailer lines
> +   whose key is "Ticket" separated by a comma.
> +** 'key_value_separator=<SEP>': specify a separator inserted between
> +   key and value. The string SEP may contain the literal formatting codes.
> +   E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer
> +   lines whose key is "Ticket" with key and value separated by a comma.
> +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
> +   option was given. In same way as to for `only` it can be followed
> +   by an equal sign and explicit value. E.g.,
> +   `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
> +** 'valueonly[=val]': skip over the key part of the trailer line and only
> +   show the value part. Also this optionally allows explicit value.

Given that the goal of this series is to unify this parsing logic
between log/for-each-ref, why do we need to then copy/paste the exact
same docs we have in pretty-formats.txt?

At the very least we should move this to pretty-formats-trailers.txt or
something, and just include it in both places, or better yet just refer
to the relevan parts of "git log"'s man page, no?

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index ca62e764b58..a8835b13915 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -825,14 +825,32 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success '%(trailers:only) shows only "key: value" trailers' '
> +test_show_key_value_trailers () {
> +	option="$1"
> +	test_expect_success "%($option) shows only 'key: value' trailers" '
> +		{
> +			grep -v patch.description <trailers &&
> +			echo
> +		} >expect &&
> +		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_show_key_value_trailers 'trailers:only'
> +test_show_key_value_trailers 'trailers:only=no,only=true'
> +test_show_key_value_trailers 'trailers:only=yes'
> +
> +test_expect_success '%(trailers:only=no) shows all trailers' '
>  	{
> -		grep -v patch.description <trailers &&
> +		cat trailers &&
>  		echo
>  	} >expect &&
> -	git for-each-ref --format="%(trailers:only)" refs/heads/main >actual &&
> +	git for-each-ref --format="%(trailers:only=no)" refs/heads/main >actual &&
>  	test_cmp expect actual &&
> -	git for-each-ref --format="%(contents:trailers:only)" refs/heads/main >actual &&
> +	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/main >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -851,17 +869,92 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
>  	test_cmp actual actual
>  '
>  
> -test_expect_success '%(trailers) rejects unknown trailers arguments' '
> -	# error message cannot be checked under i18n
> -	cat >expect <<-EOF &&
> -	fatal: unknown %(trailers) argument: unsupported
> -	EOF
> -	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> -	test_i18ncmp expect actual &&
> -	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
> -	test_i18ncmp expect actual
> +test_trailer_option() {
> +	title="$1"
> +	option="$2"
> +	expect="$3"
> +	test_expect_success "$title" '
> +		echo $expect >expect &&
> +		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 '%(trailers:key=foo) shows that trailer' \
> +	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'
> +test_trailer_option '%(trailers:key=foo) is case insensitive' \
> +	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <author@example.com>\n'
> +test_trailer_option '%(trailers:key=foo:) trailing colon also works' \
> +	'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor <author@example.com>\n'
> +test_trailer_option '%(trailers:key=foo) multiple keys' \
> +	'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor <author@example.com>\nSigned-off-by: A U Thor <author@example.com>\n'
> +test_trailer_option '%(trailers:key=nonexistent) becomes empty' \
> +	'trailers:key=Shined-off-by:' ''
> +
> +test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
> +	{
> +		grep -v patch.description <trailers | grep -v Signed-off-by | grep -v Reviewed-by &&
> +		echo
> +	} >expect &&
> +	git for-each-ref --format="%(trailers:key=Acked-by)" refs/heads/main >actual &&
> +	test_cmp expect actual &&
> +	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/main >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
> +	{
> +		unfold <trailers | grep Signed-off-by &&
> +		echo
> +	} >expect &&
> +	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
> +	test_cmp expect actual &&
> +	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
> +	test_cmp expect actual
>  '
>  
> +test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
> +	{
> +		echo "Signed-off-by: A U Thor <author@example.com>" &&
> +		grep patch.description <trailers &&
> +		echo
> +	} >expect &&
> +	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
> +	test_cmp expect actual &&
> +	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_trailer_option '%(trailers:key=foo,valueonly) shows only value' \
> +	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>\n'
> +test_trailer_option '%(trailers:separator) changes separator' \
> +	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <author@example.com>,Signed-off-by: A U Thor <author@example.com>'7
> +test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \
> +	'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>\nSigned-off-by,A U Thor <author@example.com>\n'
> +test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \
> +	'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>'
> +
> +test_failing_trailer_option () {
> +	title="$1"
> +	option="$2"
> +	error="$3"
> +	test_expect_success "$title" '
> +		# error message cannot be checked under i18n
> +		echo $error >expect &&
> +		test_must_fail git for-each-ref --format="%($option)" refs/heads/main 2>actual &&
> +		test_i18ncmp expect actual &&
> +		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/main 2>actual &&
> +		test_i18ncmp expect actual
> +	'
> +}
> +
> +test_failing_trailer_option '%(trailers:key) without value is error' \
> +	'trailers:key' 'fatal: expected %(trailers:key=<value>)'
> +test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \
> +	'trailers:unsupported' 'fatal: unknown %(trailers) argument: unsupported'
> +
>  test_expect_success 'if arguments, %(contents:trailers) shows error if colon is missing' '
>  	cat >expect <<-EOF &&
>  	fatal: unrecognized %(contents) argument: trailersonly

And similarly, here we have now mostly duplicated tests for this between
here and t/t4205-log-pretty-formats.sh.

I think the right thing to do is to start by moving the tests that are
now in t/t4205-log-pretty-formats.sh relevant to this formatting into
its own file or something.

Then instead of duplicating the tests here, just prepare them to be
changed so that we can add both "git log" and a "git for-each-ref"
invocation to some for-loop, so we'll test both.
Hariom verma Feb. 4, 2021, 6:46 p.m. UTC | #2
Hi,

On Sun, Jan 31, 2021 at 2:15 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Given that the goal of this series is to unify this parsing logic
> between log/for-each-ref, why do we need to then copy/paste the exact
> same docs we have in pretty-formats.txt?
>
> At the very least we should move this to pretty-formats-trailers.txt or
> something, and just include it in both places, or better yet just refer
> to the relevan parts of "git log"'s man page, no?

Ok. I will refer to the trailers part of "pretty-formats"'s man page
in "git-for-each-ref"'s man page.

> And similarly, here we have now mostly duplicated tests for this between
> here and t/t4205-log-pretty-formats.sh.
>
> I think the right thing to do is to start by moving the tests that are
> now in t/t4205-log-pretty-formats.sh relevant to this formatting into
> its own file or something.
>
> Then instead of duplicating the tests here, just prepare them to be
> changed so that we can add both "git log" and a "git for-each-ref"
> invocation to some for-loop, so we'll test both.

With this unified trailer logic, "git log" and "git for-each-ref"
still behave differently.
For e.g.: "git log" does nothing for unknown/incorrect trailer option,
whereas "git for-each-ref" stops.

Even if we move trailer related tests for both into a new file, I
guess we still need to test trailers for both "git log" and "git
for-each-ref" separately?

Thanks,
Hariom.
Ævar Arnfjörð Bjarmason Feb. 4, 2021, 8:53 p.m. UTC | #3
On Thu, Feb 04 2021, Hariom verma wrote:

> Hi,
>
> On Sun, Jan 31, 2021 at 2:15 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> Given that the goal of this series is to unify this parsing logic
>> between log/for-each-ref, why do we need to then copy/paste the exact
>> same docs we have in pretty-formats.txt?
>>
>> At the very least we should move this to pretty-formats-trailers.txt or
>> something, and just include it in both places, or better yet just refer
>> to the relevan parts of "git log"'s man page, no?
>
> Ok. I will refer to the trailers part of "pretty-formats"'s man page
> in "git-for-each-ref"'s man page.

Sure, FWIW you can also (not saying it has to be this) include the same
section in both, maybe with some blurb on the top saying it's not
different between the two...

>> And similarly, here we have now mostly duplicated tests for this between
>> here and t/t4205-log-pretty-formats.sh.
>>
>> I think the right thing to do is to start by moving the tests that are
>> now in t/t4205-log-pretty-formats.sh relevant to this formatting into
>> its own file or something.
>>
>> Then instead of duplicating the tests here, just prepare them to be
>> changed so that we can add both "git log" and a "git for-each-ref"
>> invocation to some for-loop, so we'll test both.
>
> With this unified trailer logic, "git log" and "git for-each-ref"
> still behave differently.
> For e.g.: "git log" does nothing for unknown/incorrect trailer option,
> whereas "git for-each-ref" stops.
>
> Even if we move trailer related tests for both into a new file, I
> guess we still need to test trailers for both "git log" and "git
> for-each-ref" separately?

We have a few tests that define a test function to test these sorts of
cases, t/t3070-wildmatch.sh is one, t/t3800-mktag.sh another.

So you can just do:

    test_trailers A '%(trailers:keyonly)' 'Signed-off-by' 'ERR: error from for-each-ref' # (or whatever)

And make the "test_trailers" function do the common setup, have both
"log" and "for-each-ref" look at the "A" tag and assert what their
output is, respectively (or an error, or whatever).

I think that's especially valuable in cases where you have similar
codepaths, because it makes it easy for both the author and reviewers to
eyeball intended an unintended differences.
diff mbox series

Patch

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2962f85a502..ea1f8417176 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -260,11 +260,40 @@  contents:lines=N::
 	The first `N` lines of the message.
 
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
-are obtained as `trailers` (or by using the historical alias
-`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
-with `trailers:only`. Whitespace-continuations can be removed from trailers so
-that each trailer appears on a line by itself with its full content with
-`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
+are obtained as `trailers[:options]` (or by using the historical alias
+`contents:trailers[:options]`). Valid [:option] are:
+** 'key=<K>': only show trailers with specified key. Matching is done
+   case-insensitively and trailing colon is optional. If option is
+   given multiple times trailer lines matching any of the keys are
+   shown. This option automatically enables the `only` option so that
+   non-trailer lines in the trailer block are hidden. If that is not
+   desired it can be disabled with `only=false`.  E.g.,
+   `%(trailers:key=Reviewed-by)` shows trailer lines with key
+   `Reviewed-by`.
+** 'only[=val]': select whether non-trailer lines from the trailer
+   block should be included. The `only` keyword may optionally be
+   followed by an equal sign and one of `true`, `on`, `yes` to omit or
+   `false`, `off`, `no` to show the non-trailer lines. If option is
+   given without value it is enabled. If given multiple times the last
+   value is used.
+** 'separator=<SEP>': specify a separator inserted between trailer
+   lines. When this option is not given each trailer line is
+   terminated with a line feed character. The string SEP may contain
+   the literal formatting codes. To use comma as separator one must use
+   `%x2C` as it would otherwise be parsed as next option. If separator
+   option is given multiple times only the last one is used.
+   E.g., `%(trailers:key=Ticket,separator=%x2C)` shows all trailer lines
+   whose key is "Ticket" separated by a comma.
+** 'key_value_separator=<SEP>': specify a separator inserted between
+   key and value. The string SEP may contain the literal formatting codes.
+   E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer
+   lines whose key is "Ticket" with key and value separated by a comma.
+** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
+   option was given. In same way as to for `only` it can be followed
+   by an equal sign and explicit value. E.g.,
+   `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
+** 'valueonly[=val]': skip over the key part of the trailer line and only
+   show the value part. Also this optionally allows explicit value.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index ee337df232a..2b1c61eadf4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -67,6 +67,12 @@  struct refname_atom {
 	int lstrip, rstrip;
 };
 
+struct ref_trailer_buf {
+	struct string_list filter_list;
+	struct strbuf sepbuf;
+	struct strbuf kvsepbuf;
+} ref_trailer_buf;
+
 static struct expand_data {
 	struct object_id oid;
 	enum object_type type;
@@ -313,28 +319,26 @@  static int subject_atom_parser(const struct ref_format *format, struct used_atom
 static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {
-	struct string_list params = STRING_LIST_INIT_DUP;
-	int i;
-
 	atom->u.contents.trailer_opts.no_divider = 1;
 
 	if (arg) {
-		string_list_split(&params, arg, ',', -1);
-		for (i = 0; i < params.nr; i++) {
-			const char *s = params.items[i].string;
-			if (!strcmp(s, "unfold"))
-				atom->u.contents.trailer_opts.unfold = 1;
-			else if (!strcmp(s, "only"))
-				atom->u.contents.trailer_opts.only_trailers = 1;
-			else {
-				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s);
-				string_list_clear(&params, 0);
-				return -1;
-			}
+		const char *argbuf = xstrfmt("%s)", arg);
+		char *invalid_arg = NULL;
+
+		if (format_set_trailers_options(&atom->u.contents.trailer_opts,
+			&ref_trailer_buf.filter_list,
+			&ref_trailer_buf.sepbuf,
+			&ref_trailer_buf.kvsepbuf,
+			&argbuf, &invalid_arg)) {
+			if (!invalid_arg)
+				strbuf_addf(err, _("expected %%(trailers:key=<value>)"));
+			else
+				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), invalid_arg);
+			free((char *)invalid_arg);
+			return -1;
 		}
 	}
 	atom->u.contents.option = C_TRAILERS;
-	string_list_clear(&params, 0);
 	return 0;
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index ca62e764b58..a8835b13915 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -825,14 +825,32 @@  test_expect_success '%(trailers:unfold) unfolds trailers' '
 	test_cmp expect actual
 '
 
-test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+test_show_key_value_trailers () {
+	option="$1"
+	test_expect_success "%($option) shows only 'key: value' trailers" '
+		{
+			grep -v patch.description <trailers &&
+			echo
+		} >expect &&
+		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_show_key_value_trailers 'trailers:only'
+test_show_key_value_trailers 'trailers:only=no,only=true'
+test_show_key_value_trailers 'trailers:only=yes'
+
+test_expect_success '%(trailers:only=no) shows all trailers' '
 	{
-		grep -v patch.description <trailers &&
+		cat trailers &&
 		echo
 	} >expect &&
-	git for-each-ref --format="%(trailers:only)" refs/heads/main >actual &&
+	git for-each-ref --format="%(trailers:only=no)" refs/heads/main >actual &&
 	test_cmp expect actual &&
-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/main >actual &&
+	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/main >actual &&
 	test_cmp expect actual
 '
 
@@ -851,17 +869,92 @@  test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp actual actual
 '
 
-test_expect_success '%(trailers) rejects unknown trailers arguments' '
-	# error message cannot be checked under i18n
-	cat >expect <<-EOF &&
-	fatal: unknown %(trailers) argument: unsupported
-	EOF
-	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
-	test_i18ncmp expect actual &&
-	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
-	test_i18ncmp expect actual
+test_trailer_option() {
+	title="$1"
+	option="$2"
+	expect="$3"
+	test_expect_success "$title" '
+		echo $expect >expect &&
+		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 '%(trailers:key=foo) shows that trailer' \
+	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=foo) is case insensitive' \
+	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=foo:) trailing colon also works' \
+	'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=foo) multiple keys' \
+	'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor <author@example.com>\nSigned-off-by: A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:key=nonexistent) becomes empty' \
+	'trailers:key=Shined-off-by:' ''
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	{
+		grep -v patch.description <trailers | grep -v Signed-off-by | grep -v Reviewed-by &&
+		echo
+	} >expect &&
+	git for-each-ref --format="%(trailers:key=Acked-by)" refs/heads/main >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/main >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	{
+		unfold <trailers | grep Signed-off-by &&
+		echo
+	} >expect &&
+	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
+	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
+	{
+		echo "Signed-off-by: A U Thor <author@example.com>" &&
+		grep patch.description <trailers &&
+		echo
+	} >expect &&
+	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
+	test_cmp expect actual
+'
+
+test_trailer_option '%(trailers:key=foo,valueonly) shows only value' \
+	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:separator) changes separator' \
+	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <author@example.com>,Signed-off-by: A U Thor <author@example.com>'
+test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \
+	'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>\nSigned-off-by,A U Thor <author@example.com>\n'
+test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \
+	'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>'
+
+test_failing_trailer_option () {
+	title="$1"
+	option="$2"
+	error="$3"
+	test_expect_success "$title" '
+		# error message cannot be checked under i18n
+		echo $error >expect &&
+		test_must_fail git for-each-ref --format="%($option)" refs/heads/main 2>actual &&
+		test_i18ncmp expect actual &&
+		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/main 2>actual &&
+		test_i18ncmp expect actual
+	'
+}
+
+test_failing_trailer_option '%(trailers:key) without value is error' \
+	'trailers:key' 'fatal: expected %(trailers:key=<value>)'
+test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \
+	'trailers:unsupported' 'fatal: unknown %(trailers) argument: unsupported'
+
 test_expect_success 'if arguments, %(contents:trailers) shows error if colon is missing' '
 	cat >expect <<-EOF &&
 	fatal: unrecognized %(contents) argument: trailersonly