pretty: Add %(trailer:X) to display single trailer
diff mbox series

Message ID 20181028125025.30952-1-anders@0x63.nu
State New
Headers show
Series
  • pretty: Add %(trailer:X) to display single trailer
Related show

Commit Message

Anders Waldenborg Oct. 28, 2018, 12:50 p.m. UTC
This new format placeholder allows displaying only a single
trailer. The formatting done is similar to what is done for
--decorate/%d using parentheses and comma separation.

It's intended use is for things like ticket references in trailers.

So with a commit with a message like:

 > Some good commit
 >
 > Ticket: XYZ-123

running:

 $ git log --pretty="%H %s% (trailer:Ticket)"

will give:

 > 123456789a Some good commit (Ticket: XYZ-123)

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         | 16 +++++++++++++
 t/t4205-log-pretty-formats.sh    | 40 ++++++++++++++++++++++++++++++++
 trailer.c                        | 18 ++++++++++++--
 trailer.h                        |  1 +
 5 files changed, 77 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Oct. 29, 2018, 4:49 a.m. UTC | #1
Anders Waldenborg <anders@0x63.nu> writes:

> This new format placeholder allows displaying only a single
> trailer. The formatting done is similar to what is done for
> --decorate/%d using parentheses and comma separation.
>
> It's intended use is for things like ticket references in trailers.
>
> So with a commit with a message like:
>
>  > Some good commit
>  >
>  > Ticket: XYZ-123
>
> running:
>
>  $ git log --pretty="%H %s% (trailer:Ticket)"
>
> will give:
>
>  > 123456789a Some good commit (Ticket: XYZ-123)

Sounds useful, but a few questions off the top of my head are:

 - How would this work together with existing %(trailers:...)?

 - Can't this be made to a new option, in addition to existing
   'only' and 'unfold', to existing %(trailer:...)?  If not, what
   are the missing pieces that we need to add in order to make that
   possible?

The latter is especially true as from the surface, it smell like
that the whole reason why this patch introduces a new placeholder
with confusingly simliar name is because the patch did not bother to
think of a way to make it fit there as an enhancement of it.

> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 6109ef09aa..a46d0c0717 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -211,6 +211,10 @@ endif::git-rev-list[]
>    If the `unfold` option is given, behave as if interpret-trailer's
>    `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
>    both.
> +- %(trailer:<t>): display the specified trailer in parentheses (like
> +  %d does for refnames). If there are multiple entries of that trailer
> +  they are shown comma separated. If there are no matching trailers
> +  nothing is displayed.


As this list is sorted roughly alphabetically for short ones, I
think it is better to keep that order for the longer ones that begin
with "%(".  This should be instead inserted before the description
for the existing "%(trailers[:options])".

Assuming that we want this %(trailer) separate from %(trailers),
that is, of course.

> diff --git a/pretty.c b/pretty.c
> index 8ca29e9281..61ae34ced4 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1324,6 +1324,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  		}
>  	}
>  
> +	if (skip_prefix(placeholder, "(trailer:", &arg)) {
> +		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> +		opts.no_divider = 1;
> +		opts.only_trailers = 1;
> +		opts.unfold = 1;

This makes me suspect that it would be very nice if this is
implemented as a new "option" to the existing "%(trailers[:option])"
thing.  It does mostly identical thing as the existing code.

> +		const char *end = strchr(arg, ')');

Avoid decl-after-statement.

> +		if (!end)
> +			return 0;
> +
> +		opts.filter_trailer = xstrndup(arg, end - arg);
> +		format_trailers_from_commit(sb, msg + c->subject_off, &opts);
> +		free(opts.filter_trailer);
> +		return end - placeholder + 1;
> +	}
> +
>  	return 0;	/* unknown placeholder */
>  }
>  
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 978a8a66ff..e929f820e7 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -598,6 +598,46 @@ test_expect_success ':only and :unfold work together' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'pretty format %(trailer:foo) shows that trailer' '
> +	git log --no-walk --pretty="%(trailer:Acked-By)" >actual &&
> +	{
> +		echo "(Acked-By: A U Thor <author@example.com>)"
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '%(trailer:nonexistant) becomes empty' '
> +	git log --no-walk --pretty="x%(trailer:Nacked-By)x" >actual &&
> +	{
> +		echo "xx"
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '% (trailer:nonexistant) with space becomes empty' '
> +	git log --no-walk --pretty="x% (trailer:Nacked-By)x" >actual &&
> +	{
> +		echo "xx"
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '% (trailer:foo) with space adds space before' '
> +	git log --no-walk --pretty="x% (trailer:Acked-By)x" >actual &&
> +	{
> +		echo "x (Acked-By: A U Thor <author@example.com>)x"
> +	} >expect &&
> +	test_cmp expect actual
> +'

These are both good positive-negative pairs of tests.

> +test_expect_success '%(trailer:foo) with multiple lines becomes comma separated and unwrapped' '
> +	git log --no-walk --pretty="%(trailer:Signed-Off-By)" >actual &&
> +	{
> +		echo "(Signed-Off-By: A U Thor <author@example.com>, A U Thor <author@example.com>)"
> +	} >expect &&
> +	test_cmp expect actual
> +'

This also tells me that it is a bad design to add this as a separate
new feature that takes the trailer key as an end-user suppied value.
There is no way to extend this to other needs, such as "do similar
thing as %(trailer:foo) does by default, but do not unwrap; give two
or more 'Signed-off-by:' separately)".

I wonder why something like %(trailers:comma,token=foo) were not
considered.  %(trailers:only,token=foo,token=bar) might even be a good
way to grab only Foo: and Bar: trailers in the order they appear in
the original commit, filtering out all the other trailers and non-trailer
text in the log message.

> diff --git a/trailer.c b/trailer.c
> index 0796f326b3..d337bca8dd 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1138,6 +1138,7 @@ static void format_trailer_info(struct strbuf *out,
>  		return;
>  	}
>  
> +	int printed_first = 0;

decl-afer-stmt.

>  	for (i = 0; i < info->trailer_nr; i++) {
>  		char *trailer = info->trailers[i];
>  		ssize_t separator_pos = find_separator(trailer, separators);
> @@ -1150,7 +1151,19 @@ static void format_trailer_info(struct strbuf *out,
>  			if (opts->unfold)
>  				unfold_value(&val);
>  
> -			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
> +			if (opts->filter_trailer) {
> +				if (!strcasecmp (tok.buf, opts->filter_trailer)) {
> +					if (!printed_first) {
> +						strbuf_addf(out, "(%s: ", opts->filter_trailer);
> +						printed_first = 1;
> +					} else {
> +						strbuf_addstr(out, ", ");
> +					}
> +					strbuf_addstr(out, val.buf);
> +				}
> +			} else {
> +				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
> +			}
>  			strbuf_release(&tok);
>  			strbuf_release(&val);
>  
> @@ -1158,7 +1171,8 @@ static void format_trailer_info(struct strbuf *out,
>  			strbuf_addstr(out, trailer);
>  		}
>  	}
> -
> +	if (printed_first)
> +		strbuf_addstr(out, ")");
>  }
>  
>  void format_trailers_from_commit(struct strbuf *out, const char *msg,
> diff --git a/trailer.h b/trailer.h
> index b997739649..852c79d449 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -72,6 +72,7 @@ struct process_trailer_options {
>  	int only_input;
>  	int unfold;
>  	int no_divider;
> +	char *filter_trailer;
>  };
>  
>  #define PROCESS_TRAILER_OPTIONS_INIT {0}
Jeff King Oct. 29, 2018, 2:14 p.m. UTC | #2
On Sun, Oct 28, 2018 at 01:50:25PM +0100, Anders Waldenborg wrote:

> This new format placeholder allows displaying only a single
> trailer. The formatting done is similar to what is done for
> --decorate/%d using parentheses and comma separation.

Displaying a single trailer makes sense as a goal. It was one of the
things I considered when working on %(trailers), actually, but I ended
up needing something a bit more flexible (hence the ability to dump the
trailers in a parse-able format, where I feed them to another script).
But your ticket example makes sense for just ordinary log displays.

Junio's review already covered my biggest question, which is why not
something like "%(trailers:key=ticket)". And likewise making things like
comma-separation options.

But my second question is whether we want to provide something more
flexible than the always-parentheses that "%d" provides. That has been a
problem in the past when people want to format the decoration in some
other way.

We have formatting magic for "if this thing is non-empty, then show this
prefix" in the for-each-ref formatter, but I'm not sure that we do in
the commit pretty-printer beyond "% ". I wonder if we could/should add a
a placeholder for "if this thing is non-empty, put in a space and
enclose it in parentheses".

> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 6109ef09aa..a46d0c0717 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -211,6 +211,10 @@ endif::git-rev-list[]
>    If the `unfold` option is given, behave as if interpret-trailer's
>    `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
>    both.
> +- %(trailer:<t>): display the specified trailer in parentheses (like
> +  %d does for refnames). If there are multiple entries of that trailer
> +  they are shown comma separated. If there are no matching trailers
> +  nothing is displayed.

It might be worth specifying how this match is done. I'm thinking
specifically of whether it's case-sensitive, but I wonder if there
should be any allowance for other normalization (e.g., allowing a regex
to match "coauthored-by" and "co-authored-by" or something).

-Peff
Anders Waldenborg Oct. 29, 2018, 5:05 p.m. UTC | #3
On Mon, Oct 29, 2018 at 3:14 PM Jeff King <peff@peff.net> wrote:
> Junio's review already covered my biggest question, which is why not
> something like "%(trailers:key=ticket)". And likewise making things like
> comma-separation options.

Jeff, Junio,

thanks!

Your questions pretty much matches what I (and a colleague I discussed
this with before posting) was concerned about.

My first try actually had it as an option to "trailers". But it got a
bit messy with the argument parsing, and the fact that there was a
fast path making it work when only specified. I did not want to spend
lot of time reworking fixing that before I had some feedback, so I
went for a smallest possible patch to float the idea with (a patch is
worth a 1000 words).

I'll start by reworking my patch to handle %(trailers:key=X)  (I'll
assume keys never contain ')' or ','), and ignore any formatting until
the way forward there is decided (see below).

> But my second question is whether we want to provide something more
> flexible than the always-parentheses that "%d" provides. That has been a
> problem in the past when people want to format the decoration in some
> other way.

Maybe just like +/-/space can be used directly after %, a () pair can
be allowed..   E.g "%d" would just be an alias for "%()D",  and for
trailers it would be something like "%()(trailers:key=foo)"

There is another special cased placeholder %f (sanitized subject line,
suitable for a filename). Which also could be changed to be a format
specifiier, allowing sanitize any thing, e.g "%!an" for sanitized
author name.

Is even the linebreak to commaseparation a generic thing?
"% ,()(trailers:key=Ticket)"   it starts go look a bit silly.

Then there are the padding modifiers. %<() %<|(). They operate on next
placeholder. "%<(10)%s" Is that a better syntax?
"%()%(trailers:key=Ticket,comma)"

I can also imagine moving all these modifiers into a generic modifier
syntax in brackets (and keeping old for backwards compat)
%[lpad=10,ltrunc=10]s ==  %<(10,trunc)%s
%[nonempty-prefix="%n"]GS ==  %+GS
%[nonempty-prefix=" (",nonempty-suffix=")"]D ==  %d
Which would mean something like this for tickets thing:
%[nonempty-prefix=" (Tickets:",nonempty-suffix=")",commaseparatelines](trailers:key=Ticket,nokey)
which is kinda verbose.

> We have formatting magic for "if this thing is non-empty, then show this
> prefix" in the for-each-ref formatter, but I'm not sure that we do in
> the commit pretty-printer beyond "% ". I wonder if we could/should add a
> a placeholder for "if this thing is non-empty, put in a space and
> enclose it in parentheses".

Would there be any interest in consolidating those formatters? Even
though they are totally separate beasts today. I think having all
attributes available on long form (e.g "%(authorname)") in addition to
existing short forms in pretty-formatter would make sense.

 anders
Jeff King Oct. 31, 2018, 8:27 p.m. UTC | #4
On Mon, Oct 29, 2018 at 06:05:34PM +0100, Anders Waldenborg wrote:

> I'll start by reworking my patch to handle %(trailers:key=X)  (I'll
> assume keys never contain ')' or ','), and ignore any formatting until
> the way forward there is decided (see below).

IMHO that is probably an acceptable tradeoff. We haven't really made any
rules for quoting arbitrary values in other %() sequences. I think it's
something we may want to have eventually, but as long as the rule for
now is "you can't do that", I think it would be OK to loosen it later.

> > But my second question is whether we want to provide something more
> > flexible than the always-parentheses that "%d" provides. That has been a
> > problem in the past when people want to format the decoration in some
> > other way.
> 
> Maybe just like +/-/space can be used directly after %, a () pair can
> be allowed..   E.g "%d" would just be an alias for "%()D",  and for
> trailers it would be something like "%()(trailers:key=foo)"

Yeah, I was thinking that "(" was taken as a special character, but I
guess immediately followed by ")" it is easy to parse left-to-right with
no ambiguity.

Would it include the leading space, too? It would be nice if it could be
combined with "% " in an orthogonal way. I guess in theory "% ()D" would
work, but it may need some tweaks to the parsing.

> There is another special cased placeholder %f (sanitized subject line,
> suitable for a filename). Which also could be changed to be a format
> specifiier, allowing sanitize any thing, e.g "%!an" for sanitized
> author name.

Yeah, I agree we should be able to sanitize anything. It's not strictly
related to your patch, though, so you may or may not want to go down
this rabbit hole. :)

> Is even the linebreak to commaseparation a generic thing?
> "% ,()(trailers:key=Ticket)"   it starts go look a bit silly.

In theory, yeah. I agree it's getting a bit magical.

> Then there are the padding modifiers. %<() %<|(). They operate on next
> placeholder. "%<(10)%s" Is that a better syntax?
> "%()%(trailers:key=Ticket,comma)"
> 
> I can also imagine moving all these modifiers into a generic modifier
> syntax in brackets (and keeping old for backwards compat)
> %[lpad=10,ltrunc=10]s ==  %<(10,trunc)%s
> %[nonempty-prefix="%n"]GS ==  %+GS
> %[nonempty-prefix=" (",nonempty-suffix=")"]D ==  %d
> Which would mean something like this for tickets thing:
> %[nonempty-prefix=" (Tickets:",nonempty-suffix=")",commaseparatelines](trailers:key=Ticket,nokey)
> which is kinda verbose.

Yes. I had dreams of eventually stuffing all of those as options into
the placeholders themselves. So "%s" would eventually have a long-form
of "%(subject)", and in that syntax it could be:

  %(subject:lpad=10,filename)

or something. I'm not completely opposed to:

  %[lpad=10,filename]%(subject)

which keeps the "formatting" arguments out of the regular placeholders.
On the other hand, if the rule were not "this affects the next
placeholder" but had a true ending mark, then we could make a real
parse-tree out of it, and format chunks of placeholders. E.g.:

  %(format:lpad=30,filename)%(subject) %(authordate)%(end)

would pad and format the whole string with two placeholders. I know that
going down this road eventually involves reinventing XML, but I think
having an actual tree structure may not be an unreasonable thing to
shoot for.

I dunno. You certainly don't need to solve all of these issues for what
you want to do. My main concern for now is to avoid introducing new
syntax that we'll be stuck with forever, even though it may later become
redundant (or worse, create parsing ambiguities).

> > We have formatting magic for "if this thing is non-empty, then show this
> > prefix" in the for-each-ref formatter, but I'm not sure that we do in
> > the commit pretty-printer beyond "% ". I wonder if we could/should add a
> > a placeholder for "if this thing is non-empty, put in a space and
> > enclose it in parentheses".
> 
> Would there be any interest in consolidating those formatters? Even
> though they are totally separate beasts today. I think having all
> attributes available on long form (e.g "%(authorname)") in addition to
> existing short forms in pretty-formatter would make sense.

Yes, there's great interest. :)

The formats are not mutually incompatible at this point, so we should be
able to come up with a unified language that maintains backwards
compatibility. One of the tricky parts is that some of the formatters
have more information than others (for-each-ref has a ref, which may
resolve to any object type; cat-file has objects only; --pretty has only
commits).

This was the subject of last year's Outreachy work. There's still a ways
to go, but you can find some of the previous discussions and work by
searching for Olga's work in the archive:

  https://public-inbox.org/git/?q=olga+telezhnaya

I've also cc'd her here, as she's still been doing some work since then.

-Peff
Anders Waldenborg Oct. 31, 2018, 11:01 p.m. UTC | #5
Jeff King writes:

> On the other hand, if the rule were not "this affects the next
> placeholder" but had a true ending mark, then we could make a real
> parse-tree out of it, and format chunks of placeholders. E.g.:
>
>   %(format:lpad=30,filename)%(subject) %(authordate)%(end)
>
> would pad and format the whole string with two placeholders. I know that
> going down this road eventually involves reinventing XML, but I think
> having an actual tree structure may not be an unreasonable thing to
> shoot for.

Yes. I'm thinking that with [] for formatting specifiers and () for
placeholders, {} would be available for nesting. E.g:

   %[lpad=30,mangle]{%(subject) %ad%}


> My main concern for now is to avoid introducing new
> syntax that we'll be stuck with forever, even though it may later become
> redundant (or worse, create parsing ambiguities).

Agreed.

I'm planning to work on the initial "trailer:key=" part later this
week. Maybe I can play around with different formatting options and see
how it affects the parser.
Jeff King Nov. 1, 2018, 6:42 p.m. UTC | #6
On Thu, Nov 01, 2018 at 12:01:28AM +0100, Anders Waldenborg wrote:

> Jeff King writes:
> 
> > On the other hand, if the rule were not "this affects the next
> > placeholder" but had a true ending mark, then we could make a real
> > parse-tree out of it, and format chunks of placeholders. E.g.:
> >
> >   %(format:lpad=30,filename)%(subject) %(authordate)%(end)
> >
> > would pad and format the whole string with two placeholders. I know that
> > going down this road eventually involves reinventing XML, but I think
> > having an actual tree structure may not be an unreasonable thing to
> > shoot for.
> 
> Yes. I'm thinking that with [] for formatting specifiers and () for
> placeholders, {} would be available for nesting. E.g:
> 
>    %[lpad=30,mangle]{%(subject) %ad%}

Hmm. That's kind of ugly, but probably not really any uglier than any of
the things I showed. And it has the advantage that we could implement
%[] now, and later extend it (well, I guess we'd want to make sure that
"%[lpad=30]{foo}" does not treat the curly braces literally, since we'd
eventually make them syntactically significant).

> I'm planning to work on the initial "trailer:key=" part later this
> week. Maybe I can play around with different formatting options and see
> how it affects the parser.

Great! Thanks for working on this.

-Peff

Patch
diff mbox series

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 6109ef09aa..a46d0c0717 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -211,6 +211,10 @@  endif::git-rev-list[]
   If the `unfold` option is given, behave as if interpret-trailer's
   `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
   both.
+- %(trailer:<t>): display the specified trailer in parentheses (like
+  %d does for refnames). If there are multiple entries of that trailer
+  they are shown comma separated. If there are no matching trailers
+  nothing is displayed.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 8ca29e9281..61ae34ced4 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1324,6 +1324,22 @@  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 	}
 
+	if (skip_prefix(placeholder, "(trailer:", &arg)) {
+		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		opts.no_divider = 1;
+		opts.only_trailers = 1;
+		opts.unfold = 1;
+
+		const char *end = strchr(arg, ')');
+		if (!end)
+			return 0;
+
+		opts.filter_trailer = xstrndup(arg, end - arg);
+		format_trailers_from_commit(sb, msg + c->subject_off, &opts);
+		free(opts.filter_trailer);
+		return end - placeholder + 1;
+	}
+
 	return 0;	/* unknown placeholder */
 }
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66ff..e929f820e7 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,6 +598,46 @@  test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailer:foo) shows that trailer' '
+	git log --no-walk --pretty="%(trailer:Acked-By)" >actual &&
+	{
+		echo "(Acked-By: A U Thor <author@example.com>)"
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailer:nonexistant) becomes empty' '
+	git log --no-walk --pretty="x%(trailer:Nacked-By)x" >actual &&
+	{
+		echo "xx"
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '% (trailer:nonexistant) with space becomes empty' '
+	git log --no-walk --pretty="x% (trailer:Nacked-By)x" >actual &&
+	{
+		echo "xx"
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '% (trailer:foo) with space adds space before' '
+	git log --no-walk --pretty="x% (trailer:Acked-By)x" >actual &&
+	{
+		echo "x (Acked-By: A U Thor <author@example.com>)x"
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailer:foo) with multiple lines becomes comma separated and unwrapped' '
+	git log --no-walk --pretty="%(trailer:Signed-Off-By)" >actual &&
+	{
+		echo "(Signed-Off-By: A U Thor <author@example.com>, A U Thor <author@example.com>)"
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
 	git commit --allow-empty -F - <<-\EOF &&
 	this is the subject
diff --git a/trailer.c b/trailer.c
index 0796f326b3..d337bca8dd 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1138,6 +1138,7 @@  static void format_trailer_info(struct strbuf *out,
 		return;
 	}
 
+	int printed_first = 0;
 	for (i = 0; i < info->trailer_nr; i++) {
 		char *trailer = info->trailers[i];
 		ssize_t separator_pos = find_separator(trailer, separators);
@@ -1150,7 +1151,19 @@  static void format_trailer_info(struct strbuf *out,
 			if (opts->unfold)
 				unfold_value(&val);
 
-			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			if (opts->filter_trailer) {
+				if (!strcasecmp (tok.buf, opts->filter_trailer)) {
+					if (!printed_first) {
+						strbuf_addf(out, "(%s: ", opts->filter_trailer);
+						printed_first = 1;
+					} else {
+						strbuf_addstr(out, ", ");
+					}
+					strbuf_addstr(out, val.buf);
+				}
+			} else {
+				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
@@ -1158,7 +1171,8 @@  static void format_trailer_info(struct strbuf *out,
 			strbuf_addstr(out, trailer);
 		}
 	}
-
+	if (printed_first)
+		strbuf_addstr(out, ")");
 }
 
 void format_trailers_from_commit(struct strbuf *out, const char *msg,
diff --git a/trailer.h b/trailer.h
index b997739649..852c79d449 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@  struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	char *filter_trailer;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}