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

Commit Message

Hariom verma Feb. 6, 2021, 9:15 a.m. UTC

From: Hariom Verma <[e-mail]>

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 key and value in 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 <[e-mail]>
Mentored-by: Heba Waly <[e-mail]>
Signed-off-by: Hariom Verma <[e-mail]>
---
 Documentation/git-for-each-ref.txt |   8 +-
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 3 files changed, 129 insertions(+), 34 deletions(-)

Comments

Junio C Hamano Feb. 7, 2021, 5:45 a.m. UTC | #1

"Hariom Verma via GitGitGadget" <[e-mail]> writes:

> +test_trailer_option() {
> +	title="$1"
> +	option="$2"
> +	expect="$3"
> +	test_expect_success "$title" '
> +		printf "$expect\n" >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 <[e-mail]>\n'

This is *not* an issue about the test script and its helper
function, but I just noticed that --format="%(trailers:key=<key>)"
is expected to write the matching trailers *AND* an empty line, and
I wonder if that is a sensible thing to expect.

The "--pretty" side does not give such an extra blank line after the
output, though.

 $ git show -s --pretty=format:"%(trailers:key=Signed-off-by:)" \
   js/range-diff-wo-dotdot
 Signed-off-by: Johannes Schindelin <[e-mail]>
 Signed-off-by: Junio C Hamano <[e-mail]>
 $ git show -s --pretty=format:"%(trailers:key=None:)" \
   js/range-diff-wo-dotdot
 $ exit

Unlike the above, when there is no matching trailer lines, the
"for-each-ref" in this series shows zero lines, and when there is
one matching trailer line, it gives that single line plus an empty
line, two lines in total.  The inconsistency is a bit disturbing.

Is the extra blank line given on purpose?  I don't see why we would
want it.  Or is it a bug we did not catch during the previous two
rounds of reviews?

Thanks.

Hariom verma Feb. 7, 2021, 12:06 p.m. UTC | #2

Hi,

On Sun, Feb 7, 2021 at 11:15 AM Junio C Hamano <[e-mail]> wrote:
>
> "Hariom Verma via GitGitGadget" <[e-mail]> writes:
>
> > +test_trailer_option() {
> > +     title="$1"
> > +     option="$2"
> > +     expect="$3"
> > +     test_expect_success "$title" '
> > +             printf "$expect\n" >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 <[e-mail]>\n'
>
> This is *not* an issue about the test script and its helper
> function, but I just noticed that --format="%(trailers:key=<key>)"
> is expected to write the matching trailers *AND* an empty line, and
> I wonder if that is a sensible thing to expect.
>
> The "--pretty" side does not give such an extra blank line after the
> output, though.
>
>  $ git show -s --pretty=format:"%(trailers:key=Signed-off-by:)" \
>    js/range-diff-wo-dotdot
>  Signed-off-by: Johannes Schindelin <[e-mail]>
>  Signed-off-by: Junio C Hamano <[e-mail]>
>  $ git show -s --pretty=format:"%(trailers:key=None:)" \
>    js/range-diff-wo-dotdot
>  $ exit
>
> Unlike the above, when there is no matching trailer lines, the
> "for-each-ref" in this series shows zero lines, and when there is
> one matching trailer line, it gives that single line plus an empty
> line, two lines in total.  The inconsistency is a bit disturbing.
>
> Is the extra blank line given on purpose?  I don't see why we would
> want it.  Or is it a bug we did not catch during the previous two
> rounds of reviews?

I don't think that "extra blank line" is due to this patch series.
Wait. Let me see.

Since "for-each-ref"'s original code does not support
"trailers:key=<KEY>", Let's check original code for "trailers:only":
```
  $ git for-each-ref refs/heads/master --format="%(trailers:only)"
  Signed-off-by: Junio C Hamano <[e-mail]>

  $ exit
```
I see. The original code also gives an extra blank line.

Now, let's check for this patch series:
```
  $ ./bin-wrappers/git for-each-ref refs/heads/master
--format="%(trailers:key=Signed-off-by)"
  Signed-off-by: Junio C Hamano <[e-mail]>

  $ ./bin-wrappers/git for-each-ref refs/heads/master
--format="%(trailers:key=None)"

  $ exit
```
when there is no matching trailer lines, the "for-each-ref" in this
series shows one empty line, and when there is one matching trailer
line, it gives that single line plus an empty line, two lines in
total. Seems consistent to me.

So this isn't about the patch series. Question still remains the same.
Why extra blank line?
Let's dig a bit.
Ah. I guess I found the reason. It's due to `putchar('\n');` in
`show_ref_array_item() [1]`. It puts a new line after each ref item.

Do you want me to include a patch to get rid of this "extra blank
line" for trailers in "for-each-ref"?

Thanks,
Hariom.

[1]: https://github.com/git/git/blob/fb7fa4a1fd273f22efcafdd13c7f897814fd1eb9/ref-filter.c#L2435

Junio C Hamano Feb. 7, 2021, 6:19 p.m. UTC | #3

Hariom verma <[e-mail]> writes:

> So this isn't about the patch series. Question still remains the same.

Thanks for digging the history.

> Why extra blank line?
> Let's dig a bit.
> Ah. I guess I found the reason. It's due to `putchar('\n');` in
> `show_ref_array_item() [1]`. It puts a new line after each ref item.
>
> Do you want me to include a patch to get rid of this "extra blank
> line" for trailers in "for-each-ref"?

I do not know the answer to the last question, because we haven't
learned the original reason why we decided to add the extra blank
line after the trailer output.  Even though I find it unnecessary,
the code that adds it must have been written with a good reason to
do so, and I do not want to see us remove the "\n" without knowing
that reason.

Thanks.

Hariom verma Feb. 7, 2021, 7:38 p.m. UTC | #4

Hi,

On Sun, Feb 7, 2021 at 11:49 PM Junio C Hamano <[e-mail]> wrote:
>
> Hariom verma <[e-mail]> writes:
>
> > Do you want me to include a patch to get rid of this "extra blank
> > line" for trailers in "for-each-ref"?
>
> I do not know the answer to the last question, because we haven't
> learned the original reason why we decided to add the extra blank
> line after the trailer output.  Even though I find it unnecessary,
> the code that adds it must have been written with a good reason to
> do so, and I do not want to see us remove the "\n" without knowing
> that reason.

As per my understanding it works something like this:

print a ref item... put newline... print a ref item... put newline..
print a ref item... put newline... (so on)

But the catch is that trailer comes with a newline already included.
So it becomes:

print trailers with newline included... put newline... print trailers
with newline included... put newline.. (so on)

So we end up having 2 new lines in total.

we just can't directly remove the newline. but we introduce an option
to skip at will. Something like this?
https://github.com/harry-hov/git/commit/af75f5c9b0325af90831998f56d6f36b6baa928e

So we can turn off newline(extra) for trailers without disturbing
"for-each-ref"'s working.

Thanks,
Hariom.

Junio C Hamano Feb. 7, 2021, 8:09 p.m. UTC | #5

Hariom verma <[e-mail]> writes:

> As per my understanding it works something like this:
>
> print a ref item... put newline... print a ref item... put newline..
> print a ref item... put newline... (so on)
>
> But the catch is that trailer comes with a newline already included.
> So it becomes:
>
> print trailers with newline included... put newline... print trailers
> with newline included... put newline.. (so on)

We know how it happens.  The question is if that is a sensible
behaviour, and if the trailing blank line was _intended_, or a bug
that nobody has complained about so far.

> we just can't directly remove the newline.

If we agree that the current behaviour is *not* sensible, then we
can.  On the "log --pretty" side, we have "terminator semantics" and
"separator semantics" between "tformat" and "format", when showing
more than one commits in a row, the "terminator semantics" places
one blank line after each commit we emit, while the "separator
semantics" gives one blank line between each commit pair.  I think
we initially (incorrectly) used terminator semantics and our output
for two commits looked like "CommitA <blank> CommitB <blank>" before
we fixed it to use separator semantics to show "CommitA <blank> CommitB"
without the useless trailing blank line.  We can apply the same principle
when "fixing" this issue to show a block of trailer lines (that is, the
change in behaviour to remove the trailing blank line turns out to
be a "fix").

Hariom verma Feb. 8, 2021, 5:07 p.m. UTC | #6

Hi,

On Mon, Feb 8, 2021 at 1:39 AM Junio C Hamano <[e-mail]> wrote:
>
> If we agree that the current behaviour is *not* sensible, then we
> can.  On the "log --pretty" side, we have "terminator semantics" and
> "separator semantics" between "tformat" and "format", when showing
> more than one commits in a row, the "terminator semantics" places
> one blank line after each commit we emit, while the "separator
> semantics" gives one blank line between each commit pair.  I think
> we initially (incorrectly) used terminator semantics and our output
> for two commits looked like "CommitA <blank> CommitB <blank>" before
> we fixed it to use separator semantics to show "CommitA <blank> CommitB"
> without the useless trailing blank line.  We can apply the same principle
> when "fixing" this issue to show a block of trailer lines (that is, the
> change in behaviour to remove the trailing blank line turns out to
> be a "fix").

I suspect that "fix" for "log --pretty" isn't going to work here.

Even if we apply the same "log --pretty"'s fix here. I think we still
end up having an empty blank line between each ref item.

Thank,
Hariom

Junio C Hamano Feb. 8, 2021, 6:29 p.m. UTC | #7

Hariom verma <[e-mail]> writes:

> I suspect that "fix" for "log --pretty" isn't going to work here.
>
> Even if we apply the same "log --pretty"'s fix here. I think we still
> end up having an empty blank line between each ref item.

After sleeping on it and seeing a result of an experiment like
this one, I think that might be unavoidable.

    $ git for-each-ref \
	--format="One%0a%(trailers:key=Signed-off-by:)Two%0a" \
	refs/heads/js/range-diff-wo-dotdot
    One
    Signed-off-by: Johannes Schindelin <[e-mail]>
    Signed-off-by: Junio C Hamano <[e-mail]>
    Two
    $ exit

People who write such "Two" without prefixing it with a newline
"%0a" themselves may view such a "fix" a regression.

It is sad that this %(trailers) itself is relatively a new thing,
and I had thought that all the other ingredients are designed to
strip the trailing newline, e.g. try this:

    $ git for-each-ref \
	--format="%(subject)%0a%(trailers:key=Signed-off-by:)" \
	refs/heads/js/range-diff-wo-dotdot
    range-diff(docs): explain how to specify commit ranges
    Signed-off-by: Johannes Schindelin <[e-mail]>
    Signed-off-by: Junio C Hamano <[e-mail]>

Notice that %(subject) is followed explicitly by %0a.  I think
%(author:date), etc. would do the same.  But %(trailers) behave
differently, and that is because it expects to be multi-line and
perhaps to mimic %(body)?  In any case, it may be too late to change
its behaviour.  At least I do not think of a good waoy to do so.

By the way, when merged to 'seen' (you can try the above that shows
%(subject) followed by %(trailers) with the tip of 'seen'), it dies
like this:

    $ git for-each-ref \
	--format="%(subject)%0a%(trailers:key=Signed-off-by:)" \
	refs/heads/js/range-diff-wo-dotdot
    free(): double free detected in tcache 2
    Aborted

There must be some interaction with another topic but I didn't dig
deeper.

Thanks.

brian m. carlson Feb. 8, 2021, 11:43 p.m. UTC | #8

On 2021-02-08 at 19:54:18, Junio C Hamano wrote:
> Junio C Hamano <[e-mail]> writes:
> 
> > By the way, when merged to 'seen' (you can try the above that shows
> > %(subject) followed by %(trailers) with the tip of 'seen'), it dies
> > like this:
> >
> >     $ git for-each-ref \
> > 	--format="%(subject)%0a%(trailers:key=Signed-off-by:)" \
> > 	refs/heads/js/range-diff-wo-dotdot
> >     free(): double free detected in tcache 2
> >     Aborted
> >
> > There must be some interaction with another topic but I didn't dig
> > deeper.
> 
> It seems brian's bc/signed-objects-with-both-hashes topic alone has
> the double-free issue, without the "trailers" topic.
> 
>     $ git checkout --detach bc/signed-objects-with-both-hashes
>     $ make git
>     $ ./git for-each-ref --format='%(subject)%(body)' refs/heads/maint
>     free(): double free detected in tcache 2
>     Aborted
> 
> So for now, you do not have to worry about it in your topic.  Of
> course, you are very much welcome to help debugging and fixing it
> ;-)

I'll take a look.  Thanks for the heads up.

brian m. carlson Feb. 9, 2021, 3:04 a.m. UTC | #9

On 2021-02-08 at 19:54:18, Junio C Hamano wrote:
> Junio C Hamano <[e-mail]> writes:
> 
> > By the way, when merged to 'seen' (you can try the above that shows
> > %(subject) followed by %(trailers) with the tip of 'seen'), it dies
> > like this:
> >
> >     $ git for-each-ref \
> > 	--format="%(subject)%0a%(trailers:key=Signed-off-by:)" \
> > 	refs/heads/js/range-diff-wo-dotdot
> >     free(): double free detected in tcache 2
> >     Aborted
> >
> > There must be some interaction with another topic but I didn't dig
> > deeper.
> 
> It seems brian's bc/signed-objects-with-both-hashes topic alone has
> the double-free issue, without the "trailers" topic.
> 
>     $ git checkout --detach bc/signed-objects-with-both-hashes
>     $ make git
>     $ ./git for-each-ref --format='%(subject)%(body)' refs/heads/maint
>     free(): double free detected in tcache 2
>     Aborted
> 
> So for now, you do not have to worry about it in your topic.  Of
> course, you are very much welcome to help debugging and fixing it
> ;-)

I'll send out a fixed patch tomorrow, but for the moment, here's the
gist of the change if you want to an immediate fix to squash in:

------- %< ---------
diff --git a/ref-filter.c b/ref-filter.c
index e6c8106377..5f8a443be5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1344,8 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
 
-		free((void *)sigpos);
 	}
+	free((void *)sigpos);
 }
 
 /*
------- %< ---------

Junio C Hamano Feb. 9, 2021, 8:54 p.m. UTC | #10

"brian m. carlson" <[e-mail]> writes:

> I'll send out a fixed patch tomorrow, but for the moment, here's the
> gist of the change if you want to an immediate fix to squash in:
>
> ------- %< ---------
> diff --git a/ref-filter.c b/ref-filter.c
> index e6c8106377..5f8a443be5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1344,8 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
>  		} else if (atom->u.contents.option == C_BARE)
>  			v->s = xstrdup(subpos);
>  
> -		free((void *)sigpos);
>  	}
> +	free((void *)sigpos);
>  }

Ah, I see.  find_subpos() will only called once to find the subject
and signature in the loop, and the finding will have to live even
the current iteration of the loop is done, only to be released after
everything is done.

Makes sense.

Patch

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2962f85a502a..2ae2478de706 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -260,11 +260,9 @@  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]`). For valid [:option] values see `trailers`
+section of linkgit:git-log[1].
 
 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 ee337df232a5..4dc4882cc768 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -67,6 +67,12 @@  struct refname_atom {
 	int lstrip, rstrip;
 };
 
+static struct ref_trailer_buf {
+	struct string_list filter_list;
+	struct strbuf sepbuf;
+	struct strbuf kvsepbuf;
+} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};
+
 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 ca62e764b586..4b3745839c86 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" '
+		printf "$expect\n" >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 <[e-mail]>\n'
+test_trailer_option '%(trailers:key=foo) is case insensitive' \
+	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <[e-mail]>\n'
+test_trailer_option '%(trailers:key=foo:) trailing colon also works' \
+	'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor <[e-mail]>\n'
+test_trailer_option '%(trailers:key=foo) multiple keys' \
+	'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor <[e-mail]>\nSigned-off-by: A U Thor <[e-mail]>\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 <[e-mail]>" &&
+		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 <[e-mail]>\n'
+test_trailer_option '%(trailers:separator) changes separator' \
+	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <[e-mail]>,Signed-off-by: A U Thor <[e-mail]>'
+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 <[e-mail]>\nSigned-off-by,A U Thor <[e-mail]>\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 <[e-mail]>,Signed-off-by,A U Thor <[e-mail]>'
+
+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