[PATCH] pretty: Add %(trailer:X) to display single trailer

[PATCH] pretty: Add %(trailer:X) to display single trailer

[Anders Waldenborg]

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(-)

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}
-- 
2.17.1

Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

[Junio C Hamano]

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}

Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

[Jeff King]

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

Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

[Anders Waldenborg]

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

Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

[Jeff King]

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

Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

[Anders Waldenborg]

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.

Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

[Jeff King]

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 v2 0/5] %(trailers) improvements in pretty format

[Anders Waldenborg]

This adds support for three new options to %(trailers):
 * key -- show only trailers with specified key
 * nokey -- don't show key part of trailers
 * separator -- allow specifying custom separator between trailers


Anders Waldenborg (5):
  pretty: single return path in %(trailers) handling
  pretty: allow showing specific trailers
  pretty: add support for "nokey" option in %(trailers)
  pretty: extract fundamental placeholders to separate function
  pretty: add support for separator option in %(trailers)

 Documentation/pretty-formats.txt | 17 +++++---
 pretty.c                         | 71 ++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh    | 60 +++++++++++++++++++++++++++
 trailer.c                        | 28 ++++++++++---
 trailer.h                        |  3 ++
 5 files changed, 156 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] pretty: single return path in %(trailers) handling

[Anders Waldenborg]

No functional change intended.

This change may not seem useful on its own, but upcoming commits will do
memory allocation in there, and a single return path makes deallocation
easier.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index b83a3ecd2..aa03d5b23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		size_t ret = 0;
 
 		opts.no_divider = 1;
 
@@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
-			return arg - placeholder + 1;
+			ret = arg - placeholder + 1;
 		}
+		return ret;
 	}
 
 	return 0;	/* unknown placeholder */
-- 
2.17.1

[PATCH v2 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailers lines which matches the specified key.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 13 +++++----
 pretty.c                         | 15 ++++++++++-
 t/t4205-log-pretty-formats.sh    | 45 ++++++++++++++++++++++++++++++++
 trailer.c                        |  8 +++---
 trailer.h                        |  1 +
 5 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..8326fc45e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -209,11 +209,14 @@ endif::git-rev-list[]
   respectively, but padding both sides (i.e. the text is centered)
 - %(trailers[:options]): display the trailers of the body as interpreted
   by linkgit:git-interpret-trailers[1]. The `trailers` string may be
-  followed by a colon and zero or more comma-separated options. If the
-  `only` option is given, omit non-trailer lines from the trailer block.
-  If the `unfold` option is given, behave as if interpret-trailer's
-  `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
-  both.
+  followed by a colon and zero or more comma-separated options. The
+  allowed options are `only` which omits non-trailer lines from the
+  trailer block, `unfold` to make it behave as if interpret-trailer's
+  `--unfold` option was given, and `key=T` to only show trailers with
+  specified key (matching is done
+  case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and
+  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
+  unfolds and shows trailer lines with key `Reviewed-by`.
 
 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 aa03d5b23..cdca9dce2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.only_trailers = 1;
 				else if (match_placeholder_arg(arg, "unfold", &arg))
 					opts.unfold = 1;
-				else
+				else if (skip_prefix(arg, "key=", &arg)) {
+					const char *end = arg + strcspn(arg, ",)");
+
+					if (opts.filter_key)
+						free(opts.filter_key);
+
+					opts.filter_key = xstrndup(arg, end - arg);
+					arg = end;
+					if (*arg == ',')
+						arg++;
+
+					opts.only_trailers = 1;
+				} else
 					break;
 			}
 		}
@@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
 			ret = arg - placeholder + 1;
 		}
+		free(opts.filter_key);
 		return ret;
 	}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..0f5207242 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+	git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
+	{
+		echo "Acked-by: A U Thor <author@example.com>" &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+	git log --no-walk --pretty="%(trailers:key=AcKed-bY)" >actual &&
+	{
+		echo "Acked-by: A U Thor <author@example.com>" &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+	git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+	{
+		echo "xx"
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	git log --no-walk --pretty="%(trailers:key=Signed-Off-by)" >actual &&
+	{
+		grep -v patch.description <trailers | grep -v Acked-by &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	git log --no-walk --pretty="%(trailers:key=Signed-Off-by,unfold)" >actual &&
+	{
+		echo "Signed-off-by: A U Thor <author@example.com>" &&
+		echo "Signed-off-by: A U Thor <author@example.com>" &&
+		echo
+	} >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 0796f326b..cbbb553e4 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,
 			struct strbuf val = STRBUF_INIT;
 
 			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (opts->unfold)
-				unfold_value(&val);
+			if (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) {
+				if (opts->unfold)
+					unfold_value(&val);
 
-			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
diff --git a/trailer.h b/trailer.h
index b99773964..d052d02ae 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_key;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.17.1

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Eric Sunshine]

On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg <anders@0x63.nu> wrote:
> Adds a new "key=X" option to "%(trailers)" which will cause it to only
> print trailers lines which matches the specified key.
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
> ---
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> @@ -209,11 +209,14 @@ endif::git-rev-list[]
>  - %(trailers[:options]): display the trailers of the body as interpreted
>    by linkgit:git-interpret-trailers[1]. The `trailers` string may be
> +  followed by a colon and zero or more comma-separated options. The
> +  allowed options are `only` which omits non-trailer lines from the
> +  trailer block, `unfold` to make it behave as if interpret-trailer's
> +  `--unfold` option was given, and `key=T` to only show trailers with
> +  specified key (matching is done
> +  case-insensitively).

Does the user have to include the colon when specifying <val> of
'key=<val>'? I can see from peeking at the implementation that the
colon must not be used, but this should be documented. Should the code
tolerate a trailing colon? (Genuine question; it's easy to do and
would be more user-friendly.)

Does 'key=<val>', do a full or partial match on trailers? And, if
partial, is the match anchored at the start or can it match anywhere
in the trailer key? I see from the implementation that it does a full
match, but this behavior should be documented.

What happens if 'key=...' is specified multiple times? Are the
multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
implementation that it is last-wins, but this behavior should be
documented. (I wonder how painful it will be for people who want to
match multiple keys. This doesn't have to be answered yet, as the
behavior can always be loosened later to allow multiple-key matching
since the current syntax does not disallow such expansion.)

Thinking further on the last two points, should <val> be a regular expression?

> +  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
> +  unfolds and shows trailer lines with key `Reviewed-by`.
> diff --git a/pretty.c b/pretty.c
> @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
> +                                       opts.filter_key = xstrndup(arg, end - arg);
> @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>                         format_trailers_from_commit(sb, msg + c->subject_off, &opts);
>                 }
> +               free(opts.filter_key);

If I understand correctly, this is making a copy of <val> so that it
will be NUL-terminated since the code added to trailer.c uses a simple
strcasecmp() to match it. Would it make sense to avoid the copy by
adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
strncasecmp() instead? (Genuine question; not necessarily a request
for change.)

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
> +       git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
> +       {
> +               echo "Acked-by: A U Thor <author@example.com>" &&
> +               echo
> +       } >expect &&
> +       test_cmp expect actual
> +'

I guess these new tests are modeled after one or two existing tests
which use a series of 'echo' statements, but an alternative would be:

    cat <<-\EOF >expect &&
    Acked-by: A U Thor <author@example.com>

    EOF

or, even:

    test_write_lines "Acked-by: A U Thor <author@example.com>" "" &&

though, that's probably less readable.

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Does the user have to include the colon when specifying <val> of
> 'key=<val>'? I can see from peeking at the implementation that the
> colon must not be used, but this should be documented. Should the code
> tolerate a trailing colon? (Genuine question; it's easy to do and
> would be more user-friendly.)
>
> Does 'key=<val>', do a full or partial match on trailers? And, if
> partial, is the match anchored at the start or can it match anywhere
> in the trailer key? I see from the implementation that it does a full
> match, but this behavior should be documented.
>
> What happens if 'key=...' is specified multiple times? Are the
> multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
> implementation that it is last-wins, but this behavior should be
> documented. (I wonder how painful it will be for people who want to
> match multiple keys. This doesn't have to be answered yet, as the
> behavior can always be loosened later to allow multiple-key matching
> since the current syntax does not disallow such expansion.)
>
> Thinking further on the last two points, should <val> be a regular expression?

Another thing that needs to be clarified in the document would be
case sensitivity.  People sometimes spell "Signed-Off-By:" by
mistake (or is it by malice?).

I do suspect that the parser should just make a list of sought-after
keys, not doing "last-one-wins", as that won't be very difficult to
do and makes what happens when given multiple keys trivially obvious.

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Eric Sunshine]

On Sun, Nov 4, 2018 at 10:48 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Does the user have to include the colon when specifying <val> of
> > 'key=<val>'?
> > Does 'key=<val>', do a full or partial match on trailers?
> > What happens if 'key=...' is specified multiple times?
> > Thinking further on the last two points, should <val> be a regular expression?
>
> Another thing that needs to be clarified in the document would be
> case sensitivity.  People sometimes spell "Signed-Off-By:" by
> mistake (or is it by malice?).

The documentation does say parenthetically "(matching is done
case-insensitively)", so I think that's already covered. Or did you
have something else in mind?

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Eric Sunshine writes:
> Should the code tolerate a trailing colon? (Genuine question; it's
> easy to do and would be more user-friendly.)

I would make sense to allow the trailing colon, it is easy enough to
just strip that away when reading the argument.

However I'm not sure how that would fit together with the possibility to
later lifting it to a regexp, hard to strip a trailing colon from a
regexp in a generic way.


> What happens if 'key=...' is specified multiple times?

My first thought was to simply disallow that. But that seemed hard to
fit into current model where errors just means don't expand.

I would guess that most useful and intuitive to user would be to handle
multiple key arguments by showing any of those keys.



> Thinking further on the last two points, should <val> be a regular expression?

It probably would make sense. I can see how the regexp '^.*-by$' would
be useful (but glob style matching would suffice in that case).

Also handling multi-matching with an alternation group would be elegant
%(trailers:key="(A|B)"). Except for the fact that the parser would need to
understand some kind of quoting, which seems like an major undertaking.

I guess having it as a regular exception would also mean that it needs
to get some way to cache the re so it is compiled once, and not for each expansion.

>
>> +               free(opts.filter_key);
>
> If I understand correctly, this is making a copy of <val> so that it
> will be NUL-terminated since the code added to trailer.c uses a simple
> strcasecmp() to match it. Would it make sense to avoid the copy by
> adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
> strncasecmp() instead? (Genuine question; not necessarily a request
> for change.)

I'm also not very happy about that copy.

Just using strncasecmp would cause it to be prefix match, no?

But if changing matching semantics to handle multiple key options to
something else I'm thinking opts.filter_key would be replaced with a
opts.filter callback function, and that part would need to be rewritten
anyway.

>
>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
>> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
>> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
>> +       git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
>> +       {
>> +               echo "Acked-by: A U Thor <author@example.com>" &&
>> +               echo
>> +       } >expect &&
>> +       test_cmp expect actual
>> +'
>
> I guess these new tests are modeled after one or two existing tests
> which use a series of 'echo' statements

I guess I could change it to "--pretty=format:%(trailers:key=Acked-by)"
to get separator semantics and avoid that extra blank line, making it
simpler.

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Eric Sunshine]

On Mon, Nov 5, 2018 at 3:26 AM Anders Waldenborg <anders@0x63.nu> wrote:
> Eric Sunshine writes:
> > Should the code tolerate a trailing colon? (Genuine question; it's
> > easy to do and would be more user-friendly.)
>
> I would make sense to allow the trailing colon, it is easy enough to
> just strip that away when reading the argument.
>
> However I'm not sure how that would fit together with the possibility to
> later lifting it to a regexp, hard to strip a trailing colon from a
> regexp in a generic way.

Which is a good reason to think about these issues now, before being
set in stone.

> > What happens if 'key=...' is specified multiple times?
>
> My first thought was to simply disallow that. But that seemed hard to
> fit into current model where errors just means don't expand.
>
> I would guess that most useful and intuitive to user would be to handle
> multiple key arguments by showing any of those keys.

Agreed.

> > Thinking further on the last two points, should <val> be a regular expression?
>
> It probably would make sense. I can see how the regexp '^.*-by$' would
> be useful (but glob style matching would suffice in that case).
>
> Also handling multi-matching with an alternation group would be elegant
> %(trailers:key="(A|B)"). Except for the fact that the parser would need to
> understand some kind of quoting, which seems like an major undertaking.

Maybe, maybe not. As long as we're careful not to paint ourselves into
a corner, it might very well be okay to start with the current
implementation of matching the full key as a literal string and
(perhaps much) later introduce regex as an alternate way to specify
the key. For instance, 'key=literal' and 'key=/regex/' can co-exist,
and the extraction of the regex inside /.../ should not be especially
difficult.

> I guess having it as a regular exception would also mean that it needs
> to get some way to cache the re so it is compiled once, and not for each expansion.

Yes, that's something I brought up a few years ago during a GSoC
project; not regex specifically, but that this parsing of the format
is happening repeatedly rather than just once. I had suggested to the
GSoC student that the parsing could be done early, compiling the
format expression into a "machine" which could be applied repeatedly.
It's a larger job, of course, not necessarily something worth tackling
for your current needs.

> > If I understand correctly, this is making a copy of <val> so that it
> > will be NUL-terminated since the code added to trailer.c uses a simple
> > strcasecmp() to match it. Would it make sense to avoid the copy by
> > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
> > strncasecmp() instead? (Genuine question; not necessarily a request
> > for change.)
>
> I'm also not very happy about that copy.
> Just using strncasecmp would cause it to be prefix match, no?

Well, you could retain full key match by checking for NUL explicitly
with something like this:

    !strncasecmp(tok.buf, opts->filter_key, opts->filter_key_len) &&
        !tok.buf[opts->filter_key_len]

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> +				else if (skip_prefix(arg, "key=", &arg)) {
> +					const char *end = arg + strcspn(arg, ",)");
> +
> +					if (opts.filter_key)
> +						free(opts.filter_key);

Call the free() unconditionally, cf. contrib/coccinelle/free.cocci.

[PATCH v2 3/5] pretty: add support for "nokey" option in %(trailers)

[Anders Waldenborg]

With the new "key=" option to %trailers it often makes little sense to
show the key, as it by definition already is know which trailer is
printed there. This new "nokey" option makes it omit key trailer key
when printing trailers.

E.g.:
 $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,nokey)' aaaa88182
will show:
 > upload-pack: fix broken if/else chain in config callback
 > Jeff King <peff@peff.net>
 > Junio C Hamano <gitster@pobox.com>

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 11 ++++++-----
 pretty.c                         |  2 ++
 t/t4205-log-pretty-formats.sh    |  9 +++++++++
 trailer.c                        |  6 ++++--
 trailer.h                        |  1 +
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8326fc45e..e115e355d 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -212,11 +212,12 @@ endif::git-rev-list[]
   followed by a colon and zero or more comma-separated options. The
   allowed options are `only` which omits non-trailer lines from the
   trailer block, `unfold` to make it behave as if interpret-trailer's
-  `--unfold` option was given, and `key=T` to only show trailers with
-  specified key (matching is done
-  case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and
-  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
-  unfolds and shows trailer lines with key `Reviewed-by`.
+  `--unfold` option was given, `key=T` to only show trailers with
+  specified key (matching is done case-insensitively), and `nokey`
+  which makes it skip over the key part of the trailer and only show
+  value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer
+  lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
+  trailer lines with key `Reviewed-by`.
 
 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 cdca9dce2..f87ba4f18 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1323,6 +1323,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.only_trailers = 1;
 				else if (match_placeholder_arg(arg, "unfold", &arg))
 					opts.unfold = 1;
+				else if (match_placeholder_arg(arg, "nokey", &arg))
+					opts.no_key = 1;
 				else if (skip_prefix(arg, "key=", &arg)) {
 					const char *end = arg + strcspn(arg, ",)");
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 0f5207242..e7de3b18a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -643,6 +643,15 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,nokey) shows only value' '
+	git log --no-walk --pretty="%(trailers:key=Acked-by,nokey)" >actual &&
+	{
+		echo "A U Thor <author@example.com>" &&
+		echo
+	} >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 cbbb553e4..4f19c34cb 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,
 			if (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) {
 				if (opts->unfold)
 					unfold_value(&val);
-
-				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				if (opts->no_key)
+					strbuf_addf(out, "%s\n", val.buf);
+				else
+					strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
diff --git a/trailer.h b/trailer.h
index d052d02ae..83de87ee9 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int no_key;
 	char *filter_key;
 };
 
-- 
2.17.1

[PATCH v2 4/5] pretty: extract fundamental placeholders to separate function

[Anders Waldenborg]

No functional change intended

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index f87ba4f18..9fdddce9d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */
+				 const char *placeholder,
+				 void *context)
+{
+	int ch;
+
+	switch (placeholder[0]) {
+	case 'n':		/* newline */
+		strbuf_addch(sb, '\n');
+		return 1;
+	case 'x':
+		/* %x00 == NUL, %x0a == LF, etc. */
+		ch = hex2chr(placeholder + 1);
+		if (ch < 0)
+			return 0;
+		strbuf_addch(sb, ch);
+		return 3;
+	}
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const char *msg = c->message;
 	struct commit_list *p;
 	const char *arg;
-	int ch;
+	size_t res;
 
 	/* these are independent of the commit */
+	res = format_fundamental(sb, placeholder, NULL);
+	if (res)
+		return res;
+
 	switch (placeholder[0]) {
 	case 'C':
 		if (starts_with(placeholder + 1, "(auto)")) {
@@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			 */
 			return ret;
 		}
-	case 'n':		/* newline */
-		strbuf_addch(sb, '\n');
-		return 1;
-	case 'x':
-		/* %x00 == NUL, %x0a == LF, etc. */
-		ch = hex2chr(placeholder + 1);
-		if (ch < 0)
-			return 0;
-		strbuf_addch(sb, ch);
-		return 3;
 	case 'w':
 		if (placeholder[1] == '(') {
 			unsigned long width = 0, indent1 = 0, indent2 = 0;
-- 
2.17.1

Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> No functional change intended
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
> ---
>  pretty.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)

I do not think "fundamental" is the best name for this, but I agree
that it would be useful to split the helpers into one that is
"constant across commits" and the other one that is "per commit".

> diff --git a/pretty.c b/pretty.c
> index f87ba4f18..9fdddce9d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
>  	return 0;
>  }
>  
> +static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */
> +				 const char *placeholder,
> +				 void *context)
> +{
> +	int ch;
> +
> +	switch (placeholder[0]) {
> +	case 'n':		/* newline */
> +		strbuf_addch(sb, '\n');
> +		return 1;
> +	case 'x':
> +		/* %x00 == NUL, %x0a == LF, etc. */
> +		ch = hex2chr(placeholder + 1);
> +		if (ch < 0)
> +			return 0;
> +		strbuf_addch(sb, ch);
> +		return 3;
> +	}
> +	return 0;
> +}
> +
>  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  				const char *placeholder,
>  				void *context)
> @@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  	const char *msg = c->message;
>  	struct commit_list *p;
>  	const char *arg;
> -	int ch;
> +	size_t res;
>  
>  	/* these are independent of the commit */
> +	res = format_fundamental(sb, placeholder, NULL);
> +	if (res)
> +		return res;
> +
>  	switch (placeholder[0]) {
>  	case 'C':
>  		if (starts_with(placeholder + 1, "(auto)")) {
> @@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  			 */
>  			return ret;
>  		}
> -	case 'n':		/* newline */
> -		strbuf_addch(sb, '\n');
> -		return 1;
> -	case 'x':
> -		/* %x00 == NUL, %x0a == LF, etc. */
> -		ch = hex2chr(placeholder + 1);
> -		if (ch < 0)
> -			return 0;
> -		strbuf_addch(sb, ch);
> -		return 3;
>  	case 'w':
>  		if (placeholder[1] == '(') {
>  			unsigned long width = 0, indent1 = 0, indent2 = 0;

Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function

[Anders Waldenborg]

Junio C Hamano writes:
> I do not think "fundamental" is the best name for this, but I agree
> that it would be useful to split the helpers into one that is
> "constant across commits" and the other one that is "per commit".

Any suggestions for a better name?

standalone? simple? invariant? free?

Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> Junio C Hamano writes:
>> I do not think "fundamental" is the best name for this, but I agree
>> that it would be useful to split the helpers into one that is
>> "constant across commits" and the other one that is "per commit".
>
> Any suggestions for a better name?
>
> standalone? simple? invariant? free?

If these are like %n for LF or %09 for HT, perhaps they are
constants or "literals".

[PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Anders Waldenborg]

By default trailer lines are terminated by linebreaks ('\n'). By
specifying the new 'separator' option they will instead be separated by
user provided string and have separator semantics rather than terminator
semantics. The separator string can contain the fundamental formatting
codes %n and %xNN allowing it to be things that are otherwise hard to
type as %x00, or command and end-parenthesis which would break parsing.

E.g:
 $ git log --pretty='%(trailers:key=Reviewed-by,nokey,separator=%x00)'

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 13 ++++++++-----
 pretty.c                         | 13 +++++++++++++
 t/t4205-log-pretty-formats.sh    |  6 ++++++
 trailer.c                        | 20 +++++++++++++++++---
 trailer.h                        |  1 +
 5 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e115e355d..3312850e6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -213,11 +213,14 @@ endif::git-rev-list[]
   allowed options are `only` which omits non-trailer lines from the
   trailer block, `unfold` to make it behave as if interpret-trailer's
   `--unfold` option was given, `key=T` to only show trailers with
-  specified key (matching is done case-insensitively), and `nokey`
-  which makes it skip over the key part of the trailer and only show
-  value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer
-  lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
-  trailer lines with key `Reviewed-by`.
+  specified key (matching is done case-insensitively), `nokey` which
+  makes it skip over the key part of the trailer and only show value
+  and `separator` which allows specifying an alternative separator
+  than the default line
+  break. E.g. `%(trailers:only,unfold,separator=%x00)` unfolds and
+  shows all trailer lines separated by NUL character,
+  `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer lines
+  with key `Reviewed-by`.
 
 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 9fdddce9d..f73a2b0dc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1327,6 +1327,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		struct strbuf sepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 						arg++;
 
 					opts.only_trailers = 1;
+				} else if (skip_prefix(arg, "separator=", &arg)) {
+					size_t seplen = strcspn(arg, ",)");
+					strbuf_reset(&sepbuf);
+					char *fmt = xstrndup(arg, seplen);
+					strbuf_expand(&sepbuf, fmt, format_fundamental, NULL);
+					free(fmt);
+					opts.separator = &sepbuf;
+
+					arg += seplen;
+					if (*arg == ',')
+						arg++;
 				} else
 					break;
 			}
@@ -1360,6 +1372,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
 			ret = arg - placeholder + 1;
 		}
+		strbuf_release (&sepbuf);
 		free(opts.filter_key);
 		return ret;
 	}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e7de3b18a..71218d22e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -652,6 +652,12 @@ test_expect_success '%(trailers:key=foo,nokey) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:separator) changes separator' '
+	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
+	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >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 4f19c34cb..a79e4e36a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,
 				const struct trailer_info *info,
 				const struct process_trailer_options *opts)
 {
+	int first_printed = 0;
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold) {
+	if (!opts->only_trailers && !opts->unfold && !opts->separator) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1150,16 +1151,29 @@ static void format_trailer_info(struct strbuf *out,
 			if (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) {
 				if (opts->unfold)
 					unfold_value(&val);
+				if (opts->separator && first_printed)
+					strbuf_addbuf(out, opts->separator);
 				if (opts->no_key)
-					strbuf_addf(out, "%s\n", val.buf);
+					strbuf_addf(out, "%s", val.buf);
 				else
-					strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+					strbuf_addf(out, "%s: %s", tok.buf, val.buf);
+				if (!opts->separator)
+					strbuf_addch(out, '\n');
+
+				first_printed = 1;
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
 		} else if (!opts->only_trailers) {
+			if (opts->separator && first_printed) {
+				strbuf_addbuf(out, opts->separator);
+			}
 			strbuf_addstr(out, trailer);
+			if (opts->separator) {
+				strbuf_rtrim(out);
+			}
+			first_printed = 1;
 		}
 	}
 
diff --git a/trailer.h b/trailer.h
index 83de87ee9..0e9d89660 100644
--- a/trailer.h
+++ b/trailer.h
@@ -74,6 +74,7 @@ struct process_trailer_options {
 	int no_divider;
 	int no_key;
 	char *filter_key;
+	const struct strbuf *separator;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.17.1

Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  						arg++;
>  
>  					opts.only_trailers = 1;
> +				} else if (skip_prefix(arg, "separator=", &arg)) {
> +					size_t seplen = strcspn(arg, ",)");
> +					strbuf_reset(&sepbuf);
> +					char *fmt = xstrndup(arg, seplen);
> +					strbuf_expand(&sepbuf, fmt, format_fundamental, NULL);

This somehow feels akin to using end-user supplied param to printf(3)
as its format argument e.g.

	int main(int ac, char *av) {
		printf(av[1]);
		return 0;
	}

which is not a good idea.  Is there a mechanism with which we can
ensure that the separator=<what> specification will never come from
potentially malicious sources (e.g. not used to show things on webpage
allowing random folks who access he site to supply custom format)?

Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Anders Waldenborg]

Junio C Hamano writes:
> Anders Waldenborg <anders@0x63.nu> writes:
>
>> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>>  						arg++;
>>
>>  					opts.only_trailers = 1;
>> +				} else if (skip_prefix(arg, "separator=", &arg)) {
>> +					size_t seplen = strcspn(arg, ",)");
>> +					strbuf_reset(&sepbuf);
>> +					char *fmt = xstrndup(arg, seplen);
>> +					strbuf_expand(&sepbuf, fmt, format_fundamental, NULL);
>
> This somehow feels akin to using end-user supplied param to printf(3)
> as its format argument e.g.
>
> 	int main(int ac, char *av) {
> 		printf(av[1]);
> 		return 0;
> 	}
>
> which is not a good idea.  Is there a mechanism with which we can
> ensure that the separator=<what> specification will never come from
> potentially malicious sources (e.g. not used to show things on webpage
> allowing random folks who access he site to supply custom format)?

I can't see a case where this could add anything that isn't already
possible.

AFAICU strbuf_expand doesn't suffer from the worst things that printf(3)
suffers from wrt untrusted format string (i.e no printf style %n which
can write to memory, and no vaargs on stack which allows leaking random
stuff).

The separator option is part of the full format string. If a malicious
user can specify that, they can't really do anything new, as the
separator only can expand %n and %xNN, which they already can do in the
full string.

But maybe I'm missing something?

Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> AFAICU strbuf_expand doesn't suffer from the worst things that printf(3)
> suffers from wrt untrusted format string (i.e no printf style %n which
> can write to memory, and no vaargs on stack which allows leaking random
> stuff).
>
> The separator option is part of the full format string. If a malicious
> user can specify that, they can't really do anything new, as the
> separator only can expand %n and %xNN, which they already can do in the
> full string.
>
> But maybe I'm missing something?

I just wanted to make sure somebody thought it through (and hoped
that that somebody might be you).  I do not offhand see a readily
usable exploit vector myself.

Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> +				if (opts->separator && first_printed)
> +					strbuf_addbuf(out, opts->separator);
>  				if (opts->no_key)
> -					strbuf_addf(out, "%s\n", val.buf);
> +					strbuf_addf(out, "%s", val.buf);

Avoid addf with "%s" alone as a formatter; instead say

	strbuf_addstr(out, val.buf);

cf. contrib/coccinelle/strbuf.cocci

Re: [PATCH v2 0/5] %(trailers) improvements in pretty format

[Eric Sunshine]

On Sun, Nov 4, 2018 at 10:23 AM Anders Waldenborg <anders@0x63.nu> wrote:
> This adds support for three new options to %(trailers):
>  * key -- show only trailers with specified key
>  * nokey -- don't show key part of trailers
>  * separator -- allow specifying custom separator between trailers

If "key" is for including particular trailers, intuition might lead
people to think that "nokey" is for excluding certain trailers.
Perhaps a different name for "nokey", such as "valueonly" or
"stripkey", would be better.

Re: [PATCH v2 0/5] %(trailers) improvements in pretty format

[Anders Waldenborg]

Eric Sunshine writes:
> If "key" is for including particular trailers, intuition might lead
> people to think that "nokey" is for excluding certain trailers.
> Perhaps a different name for "nokey", such as "valueonly" or
> "stripkey", would be better.

Good point. I guess "valueonly" would be preferred as it says what it
shows, not what it hides.

[PATCH v3 0/5] %(trailers) improvements in pretty format

[Anders Waldenborg]

Updated since v2:
 * Allow trailing colon in 'key=' argument
 * Clarify documentation on how matching is done
 * Rename option to "valueonly"
 * Make trailing matching a callback function
 * Avoid copying match string
 * Simplify generation of "expected" in tests
 * Rename function to strbuf_expand_literal_cb
 * cocci suggested fixes



Anders Waldenborg (5):
  pretty: single return path in %(trailers) handling
  pretty: allow showing specific trailers
  pretty: add support for "valueonly" option in %(trailers)
  strbuf: separate callback for strbuf_expand:ing literals
  pretty: add support for separator option in %(trailers)

 Documentation/pretty-formats.txt | 26 ++++++++---
 pretty.c                         | 68 ++++++++++++++++++++++------
 strbuf.c                         | 21 +++++++++
 strbuf.h                         |  8 ++++
 t/t4205-log-pretty-formats.sh    | 78 ++++++++++++++++++++++++++++++++
 trailer.c                        | 25 ++++++++--
 trailer.h                        |  4 ++
 7 files changed, 206 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] pretty: single return path in %(trailers) handling

[Anders Waldenborg]

No functional change intended.

This change may not seem useful on its own, but upcoming commits will do
memory allocation in there, and a single return path makes deallocation
easier.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index b83a3ecd2..aa03d5b23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		size_t ret = 0;
 
 		opts.no_divider = 1;
 
@@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
-			return arg - placeholder + 1;
+			ret = arg - placeholder + 1;
 		}
+		return ret;
 	}
 
 	return 0;	/* unknown placeholder */
-- 
2.17.1

[PATCH v3 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailers lines which match the specified key.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 17 +++++++++------
 pretty.c                         | 31 ++++++++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh    | 36 ++++++++++++++++++++++++++++++++
 trailer.c                        |  8 ++++---
 trailer.h                        |  2 ++
 5 files changed, 84 insertions(+), 10 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..75c2e838d 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -207,13 +207,18 @@ endif::git-rev-list[]
   than given and there are spaces on its left, use those spaces
 - '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)'
   respectively, but padding both sides (i.e. the text is centered)
-- %(trailers[:options]): display the trailers of the body as interpreted
+- '%(trailers[:options])': display the trailers of the body as interpreted
   by linkgit:git-interpret-trailers[1]. The `trailers` string may be
-  followed by a colon and zero or more comma-separated options. If the
-  `only` option is given, omit non-trailer lines from the trailer block.
-  If the `unfold` option is given, behave as if interpret-trailer's
-  `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
-  both.
+  followed by a colon and zero or more comma-separated options:
+  ** 'only': omit non-trailer lines from the trailer block.
+  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
+     option was given.
+  ** 'key=<K>': only show trailers with specified key. Matching is
+     done case-insensitively and trailing colon is optional. If option
+     is given multiple times only last one is used.
+  ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
+     lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
+     trailer lines with key `Reviewed-by`.
 
 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 aa03d5b23..aaedc8447 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+struct format_trailer_match_data {
+	const char *trailer;
+	size_t trailer_len;
+};
+
+static int format_trailer_match_cb(const struct strbuf *sb, void *ud)
+{
+	struct format_trailer_match_data *data = ud;
+	return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1312,6 +1323,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		struct format_trailer_match_data filter_data;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.only_trailers = 1;
 				else if (match_placeholder_arg(arg, "unfold", &arg))
 					opts.unfold = 1;
-				else
+				else if (skip_prefix(arg, "key=", &arg)) {
+					const char *end = arg + strcspn(arg, ",)");
+
+					filter_data.trailer = arg;
+					filter_data.trailer_len = end - arg;
+
+					if (filter_data.trailer_len &&
+					    filter_data.trailer[filter_data.trailer_len - 1] == ':')
+						filter_data.trailer_len--;
+
+					opts.filter = format_trailer_match_cb;
+					opts.filter_data = &filter_data;
+					opts.only_trailers = 1;
+
+					arg = end;
+					if (*arg == ',')
+						arg++;
+				} else
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..aba7ba206 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,6 +598,42 @@ test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+	git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+	git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+	echo "xx" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual &&
+	grep -v patch.description <trailers | grep -v Acked-by >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual &&
+	unfold <trailers | grep Signed-off-by >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 0796f326b..97c8f2762 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,
 			struct strbuf val = STRBUF_INIT;
 
 			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (opts->unfold)
-				unfold_value(&val);
+			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+				if (opts->unfold)
+					unfold_value(&val);
 
-			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
diff --git a/trailer.h b/trailer.h
index b99773964..5255b676d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,8 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int (*filter)(const struct strbuf *, void *);
+	void *filter_data;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.17.1

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> +  followed by a colon and zero or more comma-separated options:
> +  ** 'only': omit non-trailer lines from the trailer block.
> +  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
> +     option was given.
> +  ** 'key=<K>': only show trailers with specified key. Matching is
> +     done case-insensitively and trailing colon is optional. If option
> +     is given multiple times only last one is used.
> +  ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
> +     lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
> +     trailer lines with key `Reviewed-by`.

The last "Examples" item does not logically belong to the other
three, which bothers me a bit.

> diff --git a/pretty.c b/pretty.c
> index aa03d5b23..aaedc8447 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
>  	return 0;
>  }
>  
> +struct format_trailer_match_data {
> +	const char *trailer;
> +	size_t trailer_len;
> +};
> +
> +static int format_trailer_match_cb(const struct strbuf *sb, void *ud)
> +{
> +	struct format_trailer_match_data *data = ud;
> +	return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);
> +}

>  	if (skip_prefix(placeholder, "(trailers", &arg)) {
>  		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> +		struct format_trailer_match_data filter_data;
>  		size_t ret = 0;
>  
>  		opts.no_divider = 1;
> @@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  					opts.only_trailers = 1;
>  				else if (match_placeholder_arg(arg, "unfold", &arg))
>  					opts.unfold = 1;
> -				else
> +				else if (skip_prefix(arg, "key=", &arg)) {
> +					const char *end = arg + strcspn(arg, ",)");
> +
> +					filter_data.trailer = arg;
> +					filter_data.trailer_len = end - arg;
> +
> +					if (filter_data.trailer_len &&
> +					    filter_data.trailer[filter_data.trailer_len - 1] == ':')
> +						filter_data.trailer_len--;
> +
> +					opts.filter = format_trailer_match_cb;
> +					opts.filter_data = &filter_data;
> +					opts.only_trailers = 1;
> +
> +					arg = end;
> +					if (*arg == ',')
> +						arg++;
> +				} else
>  					break;
>  			}

This is part of a loop that is entered after seeing "%(trailers:"
and existing one looks for 'unfold' and 'only' by using the
match_placeholder_arg() helper, which

 - returns false if the keyword is not what is being sought after;
 - returns 1 if it finds the keyword, followed by ',' or ')', and
   updates the end pointer to point at either the closing ')' or one
   place after the ','.

The new part cannot directly reuse the same helper because it
expects some non-constant string after "key=", but shouldn't we be
introducing a similar helper with extended feature to help this part
of the code to stay readable?  Exposing the minute details of the
logic to parse "key=<value>,..." while hiding the counterpart to
parse "(only|unfold),..." makes the implementation of the above loop
uneven and hard to follow.

I wonder if a helper like this would help:

static int match_placeholder(const char *to_parse, const char *keyword,
			     const char **value, size_t *valuelen,
			     const char **end)
{
	const char *p;

	if (!(skip_prefix(to_parse, keyword, &p)))
		return 0;

	if (value && valuelen) {
		/* expect "<keyword>=<value>" */
		size_t vlen;
		if (*p++ != '=')
			return 1;
		vlen = strcspn(p, ",)");
		if (!p[vlen])
			return 0;
		*value = p;
		*valuelen = vlen;
		p = p + vlen;
	}

	if (*p == ',') {
		*end = p + 1;
		return 1;
	}
	if (*p == ')') {
		*end = p;
		return 1;
	}
	return 0;
}

which would allow the existing one to become a thin wrapper

static int match_placeholder_arg(const char *a, const char *b, const char **c)
{
	return match_placeholder(a, b, NULL, NULL, c);
}

or can simply be eliminated by updating its only two callsites.

In the version you wrote, it is not clear what would happen if the
format string ends with "%(trailers:key=" (no value, no comma, not
even the closing paren).  I do not think it would fall into infinite
loop, but I do not think the code structure without the helper that
makes the loop's logic uniform would allow it to properly diagnose
such a malformed string.

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> +  followed by a colon and zero or more comma-separated options:
> +  ** 'only': omit non-trailer lines from the trailer block.
> +  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
> +     option was given.
> +  ** 'key=<K>': only show trailers with specified key. Matching is
> +     done case-insensitively and trailing colon is optional. If option
> +     is given multiple times only last one is used.

It would be good to allow multiple keys, as

	%(trailers:key=signed-off-by,key=helped-by)

and

	%(trailers:key=signed-off-by)%(trailers:key=helped-by)

would mean quite a different thing.  The former can preserve the
order of these sign-offs and helped-bys in the original, while the
latter would have to show all the sign-offs before showing the
helped-bys, and I am not convinced if the latter is the only valid
use case.

Also, use of 'key=' automatically turns on 'only' as described, and
I tend to agree that it would a convenient default mode (i.e. when
picking certain trailers only with this mechanism, it is likely that
the user is willing to use %(subject) etc. to fill in what was lost
by the implicit use of 'only'), but at the same time, it makes me
wonder if we need to have a way to countermand an 'only' (or
'unfold' for that matter) that was given earlier, e.g.

	%(trailers:key=signed-off-by,only=no)

Thanks.

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Junio C Hamano writes:
> Also, use of 'key=' automatically turns on 'only' as described, and
> I tend to agree that it would a convenient default mode (i.e. when
> picking certain trailers only with this mechanism, it is likely that
> the user is willing to use %(subject) etc. to fill in what was lost
> by the implicit use of 'only'), but at the same time, it makes me
> wonder if we need to have a way to countermand an 'only' (or
> 'unfold' for that matter) that was given earlier, e.g.
>
> 	%(trailers:key=signed-off-by,only=no)
>
> Thanks.

I'm not sure what that would mean. The non-trailer lines in the trailer
block doesn't match the key.

Take this commit as an example:

$ git show -s --pretty=format:'%(trailers)' b4d065df03049bacfbc40467b60b13e804b7d289
Helped-by: Jeff King <peff@peff.net>
[jc: took idea and log message from peff]
Signed-off-by: Junio C Hamano <gitster@pobox.com>

With 'only' it shows:
$ git show -s --pretty=format:'%(trailers:only)' b4d065df03049bacfbc40467b60b13e804b7d289
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Now with a "key=signed-off-by" option I would imagine that as adding a
"| grep -i '^signed-off-by:'" to the end. In both cases (with and
without 'only') that would give the same result:
"Signed-off-by: Junio C Hamano <gitster@pobox.com>"


 anders

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> Junio C Hamano writes:
>> Also, use of 'key=' automatically turns on 'only' as described, and
>> I tend to agree that it would a convenient default mode (i.e. when
>> picking certain trailers only with this mechanism, it is likely that
>> the user is willing to use %(subject) etc. to fill in what was lost
>> by the implicit use of 'only'), but at the same time, it makes me
>> wonder if we need to have a way to countermand an 'only' (or
>> 'unfold' for that matter) that was given earlier, e.g.
>>
>> 	%(trailers:key=signed-off-by,only=no)
>>
>> Thanks.
>
> I'm not sure what that would mean. The non-trailer lines in the trailer
> block doesn't match the key.

I was confused by the "only" stuff.

When you give a key (or two), they cannot possibly name non-trailer
lines, so while it may be possible to ask "oh, by the way, I also
want non-trailer lines in addition to signed-off-by and cc lines",
the value of being able to make such a request is dubious.

The value is dubious, but logically it makes it more consistent with
the current %(trailers) that lack 'only', which is "oh by the way, I
also want non-trailer lines in addition to the trailers with
keyword", to allow a way to countermand the 'only' you flip on by
default when keywords are given.

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Junio C Hamano writes:
> I was confused by the "only" stuff.
>
> When you give a key (or two), they cannot possibly name non-trailer
> lines, so while it may be possible to ask "oh, by the way, I also
> want non-trailer lines in addition to signed-off-by and cc lines",
> the value of being able to make such a request is dubious.
>
> The value is dubious, but logically it makes it more consistent with
> the current %(trailers) that lack 'only', which is "oh by the way, I
> also want non-trailer lines in addition to the trailers with
> keyword", to allow a way to countermand the 'only' you flip on by
> default when keywords are given.


Would it feel less inconsistent if it did not set the 'only_trailers'
option?

Now that I look at it again setting 'only_trailers' is more of an
implementation trick/hack to make sure it doesn't take the fast-path in
format_trailer_info (and by documenting it it justifies that hack). If
instead the 'filter' option is checked in the relevant places there
would be no need to mix up 'only' with 'filter'.

That is, do you think something like this should be squashed in?

diff --git a/pretty.c b/pretty.c
index 302e67fa8..f45ccaf51 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1360,7 +1360,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */

                                        opts.filter = format_trailer_match_cb;
                                        opts.filter_data = &filter_list;
-                                       opts.only_trailers = 1;
                                } else
                                        break;
                        }
diff --git a/trailer.c b/trailer.c
index 97c8f2762..07ca2b2c6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
        size_t i;

        /* If we want the whole block untouched, we can take the fast path. */
-       if (!opts->only_trailers && !opts->unfold) {
+       if (!opts->only_trailers && !opts->unfold && !opts->filter) {
                strbuf_add(out, info->trailer_start,
                           info->trailer_end - info->trailer_start);
                return;
@@ -1156,7 +1156,7 @@ static void format_trailer_info(struct strbuf *out,
                        strbuf_release(&tok);
                        strbuf_release(&val);

-               } else if (!opts->only_trailers) {
+               } else if (!opts->only_trailers && !opts->filter) {
                        strbuf_addstr(out, trailer);
                }
        }
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 7548e1d38..ea3cd5b28 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -228,9 +228,9 @@ endif::git-rev-list[]
 ** '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. Non-trailer lines in the trailer block are also hidden
-   (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
-   shows trailer lines with key `Reviewed-by`.
+   shown. Non-trailer lines in the trailer block are also hidden.
+   E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
+   `Reviewed-by`.
 ** 'only': omit non-trailer lines from the trailer block.
 ** 'unfold': make it behave as if interpret-trailer's `--unfold`
    option was given. E.g., `%(trailers:only,unfold)` unfolds and

Re: [PATCH v3 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> Would it feel less inconsistent if it did not set the 'only_trailers'
> option?

If %(trailers:key=...) did not automatically imply 'only', it would
be very consistent.

But as I already said, I think it would be less convenient, as I do
suspect that those who want specific keys would want to see only
those trailers with specific keys.

And if we want that convinience, we'd probably want a way to
optionally disable that 'only' bit when the user wants to.

And...

> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -228,9 +228,9 @@ endif::git-rev-list[]
>  ** '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. Non-trailer lines in the trailer block are also hidden
> -   (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
> -   shows trailer lines with key `Reviewed-by`.
> +   shown. Non-trailer lines in the trailer block are also hidden.
> +   E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
> +   `Reviewed-by`.

... I do not think this change reduces the puzzlement felt by
readers who would have wondered how that implied 'only' can be
countermanded with the old text.  It just makes it look even less
explained to them.

If we assume that nobody would ever want to mix non-trailers when
asking specific keywords, then "them" in the above paragraph would
become an empty set, and we do not have to worry about them.  I am
not sure if Git is still such a small project to allow us rely on
such an assumption, though.

>  ** 'only': omit non-trailer lines from the trailer block.
>  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
>     option was given. E.g., `%(trailers:only,unfold)` unfolds and

[PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers)

[Anders Waldenborg]

With the new "key=" option to %(trailers) it often makes little sense to
show the key, as it by definition already is know which trailer is
printed there. This new "valueonly" option makes it omit the key when
printing trailers.

E.g.:
 $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182
will show:
 > upload-pack: fix broken if/else chain in config callback
 > Jeff King <peff@peff.net>
 > Junio C Hamano <gitster@pobox.com>

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

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 75c2e838d..8cc8c3f9f 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -216,6 +216,8 @@ endif::git-rev-list[]
   ** 'key=<K>': only show trailers with specified key. Matching is
      done case-insensitively and trailing colon is optional. If option
      is given multiple times only last one is used.
+  ** 'valueonly': skip over the key part of the trailer and only show
+     the its value part.
   ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
      lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
      trailer lines with key `Reviewed-by`.
diff --git a/pretty.c b/pretty.c
index aaedc8447..2e99f2418 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1335,6 +1335,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.only_trailers = 1;
 				else if (match_placeholder_arg(arg, "unfold", &arg))
 					opts.unfold = 1;
+				else if (match_placeholder_arg(arg, "valueonly", &arg))
+					opts.value_only = 1;
 				else if (skip_prefix(arg, "key=", &arg)) {
 					const char *end = arg + strcspn(arg, ",)");
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index aba7ba206..095208d6b 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -634,6 +634,12 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
+	echo "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 97c8f2762..662c7ff03 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->unfold)
 					unfold_value(&val);
-
-				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				if (!opts->value_only)
+					strbuf_addf(out, "%s: ", tok.buf);
+				strbuf_addbuf(out, &val);
+				strbuf_addch(out, '\n');
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
diff --git a/trailer.h b/trailer.h
index 5255b676d..06d417fe9 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int value_only;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.17.1

Re: [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers)

[Eric Sunshine]

On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg <anders@0x63.nu> wrote:
> With the new "key=" option to %(trailers) it often makes little sense to
> show the key, as it by definition already is know which trailer is

s/know/known/

> printed there. This new "valueonly" option makes it omit the key when
> printing trailers.
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>

[PATCH v3 4/5] strbuf: separate callback for strbuf_expand:ing literals

[Anders Waldenborg]

Expanding '%n' and '%xNN' is generic functionality, so extract that from
the pretty.c formatter into a callback that can be reused.

No functional change intended

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 16 +++++-----------
 strbuf.c | 21 +++++++++++++++++++++
 strbuf.h |  8 ++++++++
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index 2e99f2418..819c5c50a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1094,9 +1094,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const char *msg = c->message;
 	struct commit_list *p;
 	const char *arg;
-	int ch;
+	size_t res;
 
 	/* these are independent of the commit */
+	res = strbuf_expand_literal_cb(sb, placeholder, NULL);
+	if (res)
+		return res;
+
 	switch (placeholder[0]) {
 	case 'C':
 		if (starts_with(placeholder + 1, "(auto)")) {
@@ -1115,16 +1119,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			 */
 			return ret;
 		}
-	case 'n':		/* newline */
-		strbuf_addch(sb, '\n');
-		return 1;
-	case 'x':
-		/* %x00 == NUL, %x0a == LF, etc. */
-		ch = hex2chr(placeholder + 1);
-		if (ch < 0)
-			return 0;
-		strbuf_addch(sb, ch);
-		return 3;
 	case 'w':
 		if (placeholder[1] == '(') {
 			unsigned long width = 0, indent1 = 0, indent2 = 0;
diff --git a/strbuf.c b/strbuf.c
index f6a6cf78b..78eecd29f 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 	}
 }
 
+size_t strbuf_expand_literal_cb(struct strbuf *sb,
+				const char *placeholder,
+				void *context)
+{
+	int ch;
+
+	switch (placeholder[0]) {
+	case 'n':		/* newline */
+		strbuf_addch(sb, '\n');
+		return 1;
+	case 'x':
+		/* %x00 == NUL, %x0a == LF, etc. */
+		ch = hex2chr(placeholder + 1);
+		if (ch < 0)
+			return 0;
+		strbuf_addch(sb, ch);
+		return 3;
+	}
+	return 0;
+}
+
 size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 		void *context)
 {
diff --git a/strbuf.h b/strbuf.h
index fc40873b6..52e44c9ab 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb,
 		   expand_fn_t fn,
 		   void *context);
 
+/**
+ * Used as callback for `strbuf_expand` to only expand literals
+ * (i.e. %n and %xNN). The context argument is ignored.
+ */
+size_t strbuf_expand_literal_cb(struct strbuf *sb,
+				const char *placeholder,
+				void *context);
+
 /**
  * Used as callback for `strbuf_expand()`, expects an array of
  * struct strbuf_expand_dict_entry as context, i.e. pairs of
-- 
2.17.1

[PATCH v3 5/5] pretty: add support for separator option in %(trailers)

[Anders Waldenborg]

By default trailer lines are terminated by linebreaks ('\n'). By
specifying the new 'separator' option they will instead be separated by
user provided string and have separator semantics rather than terminator
semantics. The separator string can contain the literal formatting codes
%n and %xNN allowing it to be things that are otherwise hard to type as
%x00, or comma and end-parenthesis which would break parsing.

E.g:
 $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)'

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 13 +++++++++---
 pretty.c                         | 15 +++++++++++++
 t/t4205-log-pretty-formats.sh    | 36 ++++++++++++++++++++++++++++++++
 trailer.c                        | 15 +++++++++++--
 trailer.h                        |  1 +
 5 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8cc8c3f9f..30e238338 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -218,9 +218,16 @@ endif::git-rev-list[]
      is given multiple times only last one is used.
   ** 'valueonly': skip over the key part of the trailer and only show
      the its value part.
-  ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
-     lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
-     trailer lines with key `Reviewed-by`.
+  ** 'separator=<SEP>': specifying an alternative separator than the
+     default line feed character. SEP may can contain the literal
+     formatting codes %n and %xNN allowing it to contain characters
+     that are hard to type such as %x00, or comma and end-parenthesis
+     which would break parsing. If option is given multiple times only
+     the last one is used.
+  ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and
+     shows all trailer lines separated by NUL character,
+     `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer
+     lines with key `Reviewed-by`.
 
 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 819c5c50a..5b22a7237 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1318,6 +1318,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 		struct format_trailer_match_data filter_data;
+		struct strbuf sepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1348,6 +1349,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					arg = end;
 					if (*arg == ',')
 						arg++;
+				} else if (skip_prefix(arg, "separator=", &arg)) {
+					size_t seplen = strcspn(arg, ",)");
+					char *fmt;
+
+					strbuf_reset(&sepbuf);
+					fmt = xstrndup(arg, seplen);
+					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
+					free(fmt);
+					opts.separator = &sepbuf;
+
+					arg += seplen;
+					if (*arg == ',')
+						arg++;
 				} else
 					break;
 			}
@@ -1356,6 +1370,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
 			ret = arg - placeholder + 1;
 		}
+		strbuf_release(&sepbuf);
 		return ret;
 	}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 095208d6b..562b56dda 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -640,6 +640,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:separator) changes separator' '
+	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
+	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '
+	git commit --allow-empty -F - <<-\EOF &&
+	Important fix
+
+	The fix is explained here
+
+	Closes: #1234
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	Another fix
+
+	The fix is explained here
+
+	Closes: #567
+	Closes: #890
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	Does not close any tickets
+	EOF
+
+	git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)" HEAD~3.. >actual &&
+	test_write_lines \
+		"Does not close any tickets" \
+		"Another fix #567, #890" \
+		"Important fix #1234" >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 662c7ff03..85cd2e52e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,
 				const struct trailer_info *info,
 				const struct process_trailer_options *opts)
 {
+	size_t origlen = out->len;
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold) {
+	if (!opts->only_trailers && !opts->unfold && !opts->separator) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out,
 			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)
 					strbuf_addf(out, "%s: ", tok.buf);
 				strbuf_addbuf(out, &val);
-				strbuf_addch(out, '\n');
+				if (!opts->separator)
+					strbuf_addch(out, '\n');
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
 		} else if (!opts->only_trailers) {
+			if (opts->separator && out->len != origlen) {
+				strbuf_addbuf(out, opts->separator);
+			}
 			strbuf_addstr(out, trailer);
+			if (opts->separator) {
+				strbuf_rtrim(out);
+			}
 		}
 	}
 
diff --git a/trailer.h b/trailer.h
index 06d417fe9..203acf4ee 100644
--- a/trailer.h
+++ b/trailer.h
@@ -73,6 +73,7 @@ struct process_trailer_options {
 	int unfold;
 	int no_divider;
 	int value_only;
+	const struct strbuf *separator;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.17.1

Re: [PATCH v3 5/5] pretty: add support for separator option in %(trailers)

[Eric Sunshine]

On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg <anders@0x63.nu> wrote:
> By default trailer lines are terminated by linebreaks ('\n'). By
> specifying the new 'separator' option they will instead be separated by
> user provided string and have separator semantics rather than terminator
> semantics. The separator string can contain the literal formatting codes
> %n and %xNN allowing it to be things that are otherwise hard to type as
> %x00, or comma and end-parenthesis which would break parsing.
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
> ---
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> @@ -218,9 +218,16 @@ endif::git-rev-list[]
> +  ** 'separator=<SEP>': specifying an alternative separator than the
> +     default line feed character. SEP may can contain the literal
> +     formatting codes %n and %xNN allowing it to contain characters
> +     that are hard to type such as %x00, or comma and end-parenthesis
> +     which would break parsing. If option is given multiple times only
> +     the last one is used.

It's not clear from this documentation what constitutes a valid <SEP>.
Is it restricted to a single character? Can it be an arbitrary string?
If a string, does it need to be quoted? Does it support backslash
escaping?

Although I was able to guess that %xNN allowed hex input of a 7- or
8-bit value, I found myself wondering what I was supposed to replace
'n' with in "%n". I didn't fathom that "%n" was meant to be typed
literally to specify a newline character.

> +  ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and
> +     shows all trailer lines separated by NUL character,
> +     `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer
> +     lines with key `Reviewed-by`.

[PATCH v4 0/7] %(trailers) improvements in pretty format

[Anders Waldenborg]

Updated since v3:
 * multiple 'key=' matches any
 * allow overriding implicit 'only' when using key
 * minor grammar and spelling fixes
 * documentation restructuring
 * Helper functions for parsing options

Anders Waldenborg (7):
  doc: group pretty-format.txt placeholders descriptions
  pretty: allow %(trailers) options with explicit value
  pretty: single return path in %(trailers) handling
  pretty: allow showing specific trailers
  pretty: add support for "valueonly" option in %(trailers)
  strbuf: separate callback for strbuf_expand:ing literals
  pretty: add support for separator option in %(trailers)

 Documentation/pretty-formats.txt | 260 ++++++++++++++++++-------------
 pretty.c                         | 104 ++++++++++---
 strbuf.c                         |  21 +++
 strbuf.h                         |   8 +
 t/t4205-log-pretty-formats.sh    | 111 +++++++++++++
 trailer.c                        |  25 ++-
 trailer.h                        |   4 +
 7 files changed, 400 insertions(+), 133 deletions(-)

-- 
2.17.1

[PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions

[Anders Waldenborg]

The placeholders can be grouped into three kinds:
 * literals
 * affecting formatting of later placeholders
 * expanding to information in commit

Also change the list to a definition list (using '::')

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 235 ++++++++++++++++---------------
 1 file changed, 125 insertions(+), 110 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd8..86d804fe97 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -102,118 +102,133 @@ The title was >>t4119: test autocomputing -p<n> for traditional diff input.<<
 +
 The placeholders are:
 
-- '%H': commit hash
-- '%h': abbreviated commit hash
-- '%T': tree hash
-- '%t': abbreviated tree hash
-- '%P': parent hashes
-- '%p': abbreviated parent hashes
-- '%an': author name
-- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1]
-  or linkgit:git-blame[1])
-- '%ae': author email
-- '%aE': author email (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ad': author date (format respects --date= option)
-- '%aD': author date, RFC2822 style
-- '%ar': author date, relative
-- '%at': author date, UNIX timestamp
-- '%ai': author date, ISO 8601-like format
-- '%aI': author date, strict ISO 8601 format
-- '%cn': committer name
-- '%cN': committer name (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ce': committer email
-- '%cE': committer email (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%cd': committer date (format respects --date= option)
-- '%cD': committer date, RFC2822 style
-- '%cr': committer date, relative
-- '%ct': committer date, UNIX timestamp
-- '%ci': committer date, ISO 8601-like format
-- '%cI': committer date, strict ISO 8601 format
-- '%d': ref names, like the --decorate option of linkgit:git-log[1]
-- '%D': ref names without the " (", ")" wrapping.
-- '%e': encoding
-- '%s': subject
-- '%f': sanitized subject line, suitable for a filename
-- '%b': body
-- '%B': raw body (unwrapped subject and body)
+- Placeholders that expand to a single literal character:
+'%n':: newline
+'%%':: a raw '%'
+'%x00':: print a byte from a hex code
+
+- Placeholders that affect formatting of later placeholders:
+'%Cred':: switch color to red
+'%Cgreen':: switch color to green
+'%Cblue':: switch color to blue
+'%Creset':: reset color
+'%C(...)':: color specification, as described under Values in the
+            "CONFIGURATION FILE" section of linkgit:git-config[1].  By
+            default, colors are shown only when enabled for log output
+            (by `color.diff`, `color.ui`, or `--color`, and respecting
+            the `auto` settings of the former if we are going to a
+            terminal). `%C(auto,...)` is accepted as a historical
+            synonym for the default (e.g., `%C(auto,red)`). Specifying
+            `%C(always,...) will show the colors even when color is
+            not otherwise enabled (though consider just using
+            `--color=always` to enable color for the whole output,
+            including this format and anything else git might color).
+            `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
+            on the next placeholders until the color is switched
+            again.
+'%m':: left (`<`), right (`>`) or boundary (`-`) mark
+'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of
+                            linkgit:git-shortlog[1].
+'%<(<N>[,trunc|ltrunc|mtrunc])':: make the next placeholder take at
+                                  least N columns, padding spaces on
+                                  the right if necessary.  Optionally
+                                  truncate at the beginning (ltrunc),
+                                  the middle (mtrunc) or the end
+                                  (trunc) if the output is longer than
+                                  N columns.  Note that truncating
+                                  only works correctly with N >= 2.
+'%<|(<N>)':: make the next placeholder take at least until Nth
+             columns, padding spaces on the right if necessary
+'%>(<N>)', '%>|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' respectively,
+                        but padding spaces on the left
+'%>>(<N>)', '%>>|(<N>)':: similar to '%>(<N>)', '%>|(<N>)'
+                          respectively, except that if the next
+                          placeholder takes more spaces than given and
+                          there are spaces on its left, use those
+                          spaces
+'%><(<N>)', '%><|(<N>)':: similar to '%<(<N>)', '%<|(<N>)'
+                          respectively, but padding both sides
+                          (i.e. the text is centered)
+
+- Placeholders that expand to information extracted from the commit:
+'%H':: commit hash
+'%h':: abbreviated commit hash
+'%T':: tree hash
+'%t':: abbreviated tree hash
+'%P':: parent hashes
+'%p':: abbreviated parent hashes
+'%an':: author name
+'%aN':: author name (respecting .mailmap, see linkgit:git-shortlog[1]
+        or linkgit:git-blame[1])
+'%ae':: author email
+'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
+        or linkgit:git-blame[1])
+'%ad':: author date (format respects --date= option)
+'%aD':: author date, RFC2822 style
+'%ar':: author date, relative
+'%at':: author date, UNIX timestamp
+'%ai':: author date, ISO 8601-like format
+'%aI':: author date, strict ISO 8601 format
+'%cn':: committer name
+'%cN':: committer name (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%ce':: committer email
+'%cE':: committer email (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%cd':: committer date (format respects --date= option)
+'%cD':: committer date, RFC2822 style
+'%cr':: committer date, relative
+'%ct':: committer date, UNIX timestamp
+'%ci':: committer date, ISO 8601-like format
+'%cI':: committer date, strict ISO 8601 format
+'%d':: ref names, like the --decorate option of linkgit:git-log[1]
+'%D':: ref names without the " (", ")" wrapping.
+'%e':: encoding
+'%s':: subject
+'%f':: sanitized subject line, suitable for a filename
+'%b':: body
+'%B':: raw body (unwrapped subject and body)
 ifndef::git-rev-list[]
-- '%N': commit notes
+'%N':: commit notes
 endif::git-rev-list[]
-- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature,
-  "B" for a bad signature,
-  "U" for a good signature with unknown validity,
-  "X" for a good signature that has expired,
-  "Y" for a good signature made by an expired key,
-  "R" for a good signature made by a revoked key,
-  "E" if the signature cannot be checked (e.g. missing key)
-  and "N" for no signature
-- '%GS': show the name of the signer for a signed commit
-- '%GK': show the key used to sign a signed commit
-- '%GF': show the fingerprint of the key used to sign a signed commit
-- '%GP': show the fingerprint of the primary key whose subkey was used
-  to sign a signed commit
-- '%gD': reflog selector, e.g., `refs/stash@{1}` or
-  `refs/stash@{2 minutes ago`}; the format follows the rules described
-  for the `-g` option. The portion before the `@` is the refname as
-  given on the command line (so `git log -g refs/heads/master` would
-  yield `refs/heads/master@{0}`).
-- '%gd': shortened reflog selector; same as `%gD`, but the refname
-  portion is shortened for human readability (so `refs/heads/master`
-  becomes just `master`).
-- '%gn': reflog identity name
-- '%gN': reflog identity name (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ge': reflog identity email
-- '%gE': reflog identity email (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%gs': reflog subject
-- '%Cred': switch color to red
-- '%Cgreen': switch color to green
-- '%Cblue': switch color to blue
-- '%Creset': reset color
-- '%C(...)': color specification, as described under Values in the
-  "CONFIGURATION FILE" section of linkgit:git-config[1].
-  By default, colors are shown only when enabled for log output (by
-  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
-  settings of the former if we are going to a terminal). `%C(auto,...)`
-  is accepted as a historical synonym for the default (e.g.,
-  `%C(auto,red)`). Specifying `%C(always,...) will show the colors
-  even when color is not otherwise enabled (though consider
-  just using `--color=always` to enable color for the whole output,
-  including this format and anything else git might color).  `auto`
-  alone (i.e. `%C(auto)`) will turn on auto coloring on the next
-  placeholders until the color is switched again.
-- '%m': left (`<`), right (`>`) or boundary (`-`) mark
-- '%n': newline
-- '%%': a raw '%'
-- '%x00': print a byte from a hex code
-- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
-  linkgit:git-shortlog[1].
-- '%<(<N>[,trunc|ltrunc|mtrunc])': make the next placeholder take at
-  least N columns, padding spaces on the right if necessary.
-  Optionally truncate at the beginning (ltrunc), the middle (mtrunc)
-  or the end (trunc) if the output is longer than N columns.
-  Note that truncating only works correctly with N >= 2.
-- '%<|(<N>)': make the next placeholder take at least until Nth
-  columns, padding spaces on the right if necessary
-- '%>(<N>)', '%>|(<N>)': similar to '%<(<N>)', '%<|(<N>)'
-  respectively, but padding spaces on the left
-- '%>>(<N>)', '%>>|(<N>)': similar to '%>(<N>)', '%>|(<N>)'
-  respectively, except that if the next placeholder takes more spaces
-  than given and there are spaces on its left, use those spaces
-- '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)'
-  respectively, but padding both sides (i.e. the text is centered)
-- %(trailers[:options]): display the trailers of the body as interpreted
-  by linkgit:git-interpret-trailers[1]. The `trailers` string may be
-  followed by a colon and zero or more comma-separated options. If the
-  `only` option is given, omit non-trailer lines from the trailer block.
-  If the `unfold` option is given, behave as if interpret-trailer's
-  `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
-  both.
+'%GG':: raw verification message from GPG for a signed commit
+'%G?':: show "G" for a good (valid) signature,
+        "B" for a bad signature,
+        "U" for a good signature with unknown validity,
+        "X" for a good signature that has expired,
+        "Y" for a good signature made by an expired key,
+        "R" for a good signature made by a revoked key,
+        "E" if the signature cannot be checked (e.g. missing key)
+        and "N" for no signature
+'%GS':: show the name of the signer for a signed commit
+'%GK':: show the key used to sign a signed commit
+'%GF':: show the fingerprint of the key used to sign a signed commit
+'%GP':: show the fingerprint of the primary key whose subkey was used
+        to sign a signed commit
+'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2
+        minutes ago`}; the format follows the rules described for the
+        `-g` option. The portion before the `@` is the refname as
+        given on the command line (so `git log -g refs/heads/master`
+        would yield `refs/heads/master@{0}`).
+'%gd':: shortened reflog selector; same as `%gD`, but the refname
+        portion is shortened for human readability (so
+        `refs/heads/master` becomes just `master`).
+'%gn':: reflog identity name
+'%gN':: reflog identity name (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%ge':: reflog identity email
+'%gE':: reflog identity email (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%gs':: reflog subject
+'%(trailers[:options])':: display the trailers of the body as
+                          interpreted by
+                          linkgit:git-interpret-trailers[1]. The
+                          `trailers` string may be followed by a colon
+                          and zero or more comma-separated options:
+** 'only': omit non-trailer lines from the trailer block.
+** 'unfold': make it behave as if interpret-trailer's `--unfold`
+   option was given. E.g., `%(trailers:only,unfold)` unfolds and
+   shows all trailer lines.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.17.1

[PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Anders Waldenborg]

In addition to old %(trailers:only) it is now allowed to write
%(trailers:only=yes)

By itself this only gives (the not quite so useful) possibility to have
users change their mind in the middle of a formatting
string (%(trailers:only=true,only=false)). However, it gives users the
opportunity to override defaults from future options.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 14 ++++++++++----
 pretty.c                         | 32 +++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh    | 18 ++++++++++++++++++
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 86d804fe97..d33b072eb2 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -225,10 +225,16 @@ endif::git-rev-list[]
                           linkgit:git-interpret-trailers[1]. The
                           `trailers` string may be followed by a colon
                           and zero or more comma-separated options:
-** 'only': omit non-trailer lines from the trailer block.
-** 'unfold': make it behave as if interpret-trailer's `--unfold`
-   option was given. E.g., `%(trailers:only,unfold)` unfolds and
-   shows all trailer lines.
+** '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.
+** '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.
 
 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 b83a3ecd23..26efdba73a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1074,6 +1074,31 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
+				      const char **end, int *val)
+{
+	const char *p;
+	if (!skip_prefix(to_parse, candidate, &p))
+		return 0;
+
+	if (match_placeholder_arg(p, "=no", end) ||
+	    match_placeholder_arg(p, "=off", end) ||
+	    match_placeholder_arg(p, "=false", end)) {
+		*val = 0;
+		return 1;
+	}
+
+	if (match_placeholder_arg(p, "", end) ||
+	    match_placeholder_arg(p, "=yes", end) ||
+	    match_placeholder_arg(p, "=on", end) ||
+	    match_placeholder_arg(p, "=true", end)) {
+		*val = 1;
+		return 1;
+	}
+
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1318,11 +1343,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		if (*arg == ':') {
 			arg++;
 			for (;;) {
-				if (match_placeholder_arg(arg, "only", &arg))
-					opts.only_trailers = 1;
-				else if (match_placeholder_arg(arg, "unfold", &arg))
-					opts.unfold = 1;
-				else
+				if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
+				    !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66ff..63730a4ec0 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no) shows all trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=no)" >actual &&
+	cat trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:unfold) unfolds trailers' '
 	git log --no-walk --pretty="%(trailers:unfold)" >actual &&
 	{
-- 
2.17.1

Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> In addition to old %(trailers:only) it is now allowed to write
> %(trailers:only=yes)

s/$/. Similarly the unfold option can take a boolean./

> By itself this only gives (the not quite so useful) possibility to have
> users change their mind in the middle of a formatting
> string (%(trailers:only=true,only=false)). However, it gives users the
> opportunity to override defaults from future options.

Makes sense.

> +** '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.

> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
> +				      const char **end, int *val)
> +{
> +	const char *p;
> +	if (!skip_prefix(to_parse, candidate, &p))
> +		return 0;
> +
> +	if (match_placeholder_arg(p, "=no", end) ||
> +	    match_placeholder_arg(p, "=off", end) ||
> +	    match_placeholder_arg(p, "=false", end)) {
> +		*val = 0;
> +		return 1;
> +	}
> +
> +	if (match_placeholder_arg(p, "", end) ||
> +	    match_placeholder_arg(p, "=yes", end) ||
> +	    match_placeholder_arg(p, "=on", end) ||
> +	    match_placeholder_arg(p, "=true", end)) {
> +		*val = 1;
> +		return 1;
> +	}

Hmph.  Is there a possibility to arrenge the code so that we do not
have to maintain these variants of true/false representations here,
when we should already have one in config.c?

The match_placeholder_arg() function is a bit too limiting as it can
only recognize the value that we know about for a thing like this.
Instead, perhaps we can cut what follows "=" syntactically, looking
for either NUL, ',', or ')', and then call git_parse_maybe_bool() on
it.  That way, we can handle %(trailers:only=bogo) more sensibly,
no?  Syntactically we can recognize that the user wanted to give
'bogo' as the value to 'only', and say "'bogo' is not a boolean" if
we did so.

> +	return 0;
> +}
> +

Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Anders Waldenborg]

Junio C Hamano writes:
> That way, we can handle %(trailers:only=bogo) more sensibly,
> no?  Syntactically we can recognize that the user wanted to give
> 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if
> we did so.

I agree that proper error reporting for the pretty formatting strings
would be great. But that would depart from the current extremely crude
error handling where incorrect formatting placeholders are just left
unexpanded. How would such change in error handling be done safely, wrt
backwards compatibility changes?

To get good diagnostics for incorrect formatting strings I think the way
forward is to have the formatting strings parsed once into some kind of
AST or machine (as also mentioned by Jeff) that is just executed many
times, instead of parsed each time like today.

 anders

Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Jeff King]

On Tue, Dec 18, 2018 at 10:30:04PM +0100, Anders Waldenborg wrote:

> 
> Junio C Hamano writes:
> > That way, we can handle %(trailers:only=bogo) more sensibly,
> > no?  Syntactically we can recognize that the user wanted to give
> > 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if
> > we did so.
> 
> I agree that proper error reporting for the pretty formatting strings
> would be great. But that would depart from the current extremely crude
> error handling where incorrect formatting placeholders are just left
> unexpanded. How would such change in error handling be done safely, wrt
> backwards compatibility changes?

I think we'd want to move in the direction of enforcing valid
expressions for %(foo) placeholders. There's some small value in leaving
%X alone if we do not understand "X" (not to mention the backwards
%compatibility you mentioned), but I think %() is a pretty
deliberate indication that a placeholder was meant there.

We already do this for ref-filter expansions:

  $ git for-each-ref --format='%(foo)'
  fatal: unknown field name: foo

We don't for "--pretty" formats, but I do wonder if anybody would be
really mad (after all, we have declared ourselves free to add new
placeholders, so such formats are not future-proof).

-Peff

Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Anders Waldenborg]

Jeff King writes:
> There's some small value in leaving
> %X alone if we do not understand "X" (not to mention the backwards
> %compatibility you mentioned), but I think %() is a pretty
> deliberate indication that a placeholder was meant there.

Good point.

> We already do this for ref-filter expansions:
>
>   $ git for-each-ref --format='%(foo)'
>   fatal: unknown field name: foo
>
> We don't for "--pretty" formats, but I do wonder if anybody would be
> really mad (after all, we have declared ourselves free to add new
> placeholders, so such formats are not future-proof).

Oh my. I wasn't aware that there was a totally separate string
interpolation implementation used for ref filters. That one has
separated parsing, making it more amenable to good error handling.
I wonder if that could be generalized and reused for pretty formats.

However I doubt I will have time to dig deeper into that in near time.

Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Anders Waldenborg]

Оля Тележная writes:
>> Oh my. I wasn't aware that there was a totally separate string
>> interpolation implementation used for ref filters. That one has
>> separated parsing, making it more amenable to good error handling.
>> I wonder if that could be generalized and reused for pretty formats.
>>
>> However I doubt I will have time to dig deeper into that in near time.
>>
>
> Sorry, I haven't read your patch in details. If you will be at Git Merge
> tomorrow, you could ask me any questions, I can explain how for-each-ref
> formatting works amd maybe even give you some ideas how to use its logic in
> pretty, I was thinking about it a bit.

No, unfortunately I'm not at Git Merge.

I think I got how the formatting work. But if you have ideas on how to
reuse the logic I'm all ears.

Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

[Оля Тележная]

чт, 31 янв. 2019 г. в 21:47, Anders Waldenborg <anders@0x63.nu>:
>
>
> Оля Тележная writes:
> >> Oh my. I wasn't aware that there was a totally separate string
> >> interpolation implementation used for ref filters. That one has
> >> separated parsing, making it more amenable to good error handling.
> >> I wonder if that could be generalized and reused for pretty formats.
> >>
> >> However I doubt I will have time to dig deeper into that in near time.
> >>
> >
> > Sorry, I haven't read your patch in details. If you will be at Git Merge
> > tomorrow, you could ask me any questions, I can explain how for-each-ref
> > formatting works amd maybe even give you some ideas how to use its logic in
> > pretty, I was thinking about it a bit.
>
> No, unfortunately I'm not at Git Merge.
>
> I think I got how the formatting work. But if you have ideas on how to
> reuse the logic I'm all ears.

 Maybe my advices will be not suitable: I know how ref-filter works,
but I know only a few about pretty system. The main point is that we
need to save backward compatibility, and that means that we can't just
drop pretty logic and start using ref-filter one there. I guess it's
not so hard to make like translation table between pretty commands and
ref-filter one. For example, 'short' in pretty means 'commit
%(objectname)%0aAuthor: %(author)' in ref-filter. So if I wanted to
change pretty logic, I would add support of all ref-filter commands
and make translation table for backward compatibility. Hope it was
helpful.



>

[PATCH v4 3/7] pretty: single return path in %(trailers) handling

[Anders Waldenborg]

No functional change intended.

This change may not seem useful on its own, but upcoming commits will do
memory allocation in there, and a single return path makes deallocation
easier.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 26efdba73a..044447e6c0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1337,6 +1337,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		size_t ret = 0;
 
 		opts.no_divider = 1;
 
@@ -1350,8 +1351,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
-			return arg - placeholder + 1;
+			ret = arg - placeholder + 1;
 		}
+		return ret;
 	}
 
 	return 0;	/* unknown placeholder */
-- 
2.17.1

[PATCH v4 4/7] pretty: allow showing specific trailers

[Anders Waldenborg]

Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailer lines which match any of the specified keys.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt |  8 +++++
 pretty.c                         | 47 ++++++++++++++++++++++++++---
 t/t4205-log-pretty-formats.sh    | 51 ++++++++++++++++++++++++++++++++
 trailer.c                        | 10 ++++---
 trailer.h                        |  2 ++
 5 files changed, 110 insertions(+), 8 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d33b072eb2..d6add831c0 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -225,6 +225,14 @@ endif::git-rev-list[]
                           linkgit:git-interpret-trailers[1]. The
                           `trailers` string may be followed by a colon
                           and zero or more comma-separated options:
+** '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
diff --git a/pretty.c b/pretty.c
index 044447e6c0..541a553ccc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,13 +1056,20 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 	return 0;
 }
 
-static int match_placeholder_arg(const char *to_parse, const char *candidate,
-				 const char **end)
+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
+				       const char **end, const char **valuestart, size_t *valuelen)
 {
 	const char *p;
 
 	if (!(skip_prefix(to_parse, candidate, &p)))
 		return 0;
+	if (valuestart) {
+		if (*p != '=')
+			return 0;
+		*valuestart = p + 1;
+		*valuelen = strcspn(*valuestart, ",)");
+		p = *valuestart + *valuelen;
+	}
 	if (*p == ',') {
 		*end = p + 1;
 		return 1;
@@ -1074,6 +1081,12 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+static int match_placeholder_arg(const char *to_parse, const char *candidate,
+				 const char **end)
+{
+	return match_placeholder_arg_value(to_parse, candidate, end, NULL, NULL);
+}
+
 static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
 				      const char **end, int *val)
 {
@@ -1095,7 +1108,19 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat
 		*val = 1;
 		return 1;
 	}
+	return 0;
+}
 
+static int format_trailer_match_cb(const struct strbuf *key, void *ud)
+{
+	const struct string_list *list = ud;
+	const struct string_list_item *item;
+
+	for_each_string_list_item (item, list) {
+		if (key->len == (uintptr_t)item->util &&
+		    !strncasecmp (item->string, key->buf, key->len))
+			return 1;
+	}
 	return 0;
 }
 
@@ -1337,6 +1362,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		struct string_list filter_list = STRING_LIST_INIT_NODUP;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1344,8 +1370,20 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		if (*arg == ':') {
 			arg++;
 			for (;;) {
-				if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
-				    !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
+				const char *argval;
+				size_t arglen;
+
+				if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) {
+					uintptr_t len = arglen;
+					if (len && argval[len - 1] == ':')
+						len--;
+					string_list_append(&filter_list, argval)->util = (char *)len;
+
+					opts.filter = format_trailer_match_cb;
+					opts.filter_data = &filter_list;
+					opts.only_trailers = 1;
+				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
+					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
 					break;
 			}
 		}
@@ -1353,6 +1391,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
 			ret = arg - placeholder + 1;
 		}
+		string_list_clear (&filter_list, 0);
 		return ret;
 	}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 63730a4ec0..54239290cf 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -616,6 +616,57 @@ test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+	git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) multiple keys' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by:,key=Signed-off-By)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+	git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+	echo "xx" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual &&
+	grep -v patch.description <trailers | grep -v Acked-by >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual &&
+	unfold <trailers | grep Signed-off-by >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,only=no)" >actual &&
+	{
+		echo "Acked-by: A U Thor <author@example.com>" &&
+		grep patch.description <trailers
+	} >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..d6da555cd7 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold) {
+	if (!opts->only_trailers && !opts->unfold && !opts->filter) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,
 			struct strbuf val = STRBUF_INIT;
 
 			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (opts->unfold)
-				unfold_value(&val);
+			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+				if (opts->unfold)
+					unfold_value(&val);
 
-			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
diff --git a/trailer.h b/trailer.h
index b997739649..5255b676de 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,8 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int (*filter)(const struct strbuf *, void *);
+	void *filter_data;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.17.1

Re: [PATCH v4 4/7] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> -static int match_placeholder_arg(const char *to_parse, const char *candidate,
> -				 const char **end)
> +static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
> +				       const char **end, const char **valuestart, size_t *valuelen)
>  {
>  	const char *p;
>  
>  	if (!(skip_prefix(to_parse, candidate, &p)))
>  		return 0;
> +	if (valuestart) {
> +		if (*p != '=')
> +			return 0;
> +		*valuestart = p + 1;
> +		*valuelen = strcspn(*valuestart, ",)");
> +		p = *valuestart + *valuelen;
> +	}
>  	if (*p == ',') {
>  		*end = p + 1;
>  		return 1;
> @@ -1074,6 +1081,12 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
>  	return 0;
>  }
>  
> +static int match_placeholder_arg(const char *to_parse, const char *candidate,
> +				 const char **end)
> +{
> +	return match_placeholder_arg_value(to_parse, candidate, end, NULL, NULL);
> +}
> +

OK.  The unified parsing of boolean value I mentioned on an earlier
step can naturally be done using martch_placeholder_arg_value(), I
think, in match_placeholder_bool_arg().

> +static int format_trailer_match_cb(const struct strbuf *key, void *ud)
> +{
> +	const struct string_list *list = ud;
> +	const struct string_list_item *item;
> +
> +	for_each_string_list_item (item, list) {
> +		if (key->len == (uintptr_t)item->util &&
> +		    !strncasecmp (item->string, key->buf, key->len))

Remove SP after strncasecmp.

We won't have too many elements in this string list, so O(N*M)
search like this one would be OK.

> +			return 1;
> +	}
>  	return 0;
>  }
>  
> @@ -1337,6 +1362,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  
>  	if (skip_prefix(placeholder, "(trailers", &arg)) {
>  		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> +		struct string_list filter_list = STRING_LIST_INIT_NODUP;
>  		size_t ret = 0;
>  
>  		opts.no_divider = 1;
> @@ -1344,8 +1370,20 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  		if (*arg == ':') {
>  			arg++;
>  			for (;;) {
> -				if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
> -				    !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
> +				const char *argval;
> +				size_t arglen;
> +
> +				if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) {
> +					uintptr_t len = arglen;
> +					if (len && argval[len - 1] == ':')
> +						len--;
> +					string_list_append(&filter_list, argval)->util = (char *)len;
> +
> +					opts.filter = format_trailer_match_cb;
> +					opts.filter_data = &filter_list;
> +					opts.only_trailers = 1;
> +				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
> +					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
>  					break;
>  			}
>  		}
> @@ -1353,6 +1391,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
>  			ret = arg - placeholder + 1;
>  		}
> +		string_list_clear (&filter_list, 0);

Remove SP after string_list_clear.

>  		return ret;
>  	}
>  

[PATCH v4 5/7] pretty: add support for "valueonly" option in %(trailers)

[Anders Waldenborg]

With the new "key=" option to %(trailers) it often makes little sense to
show the key, as it by definition already is knows which trailer is
printed there. This new "valueonly" option makes it omit the key when
printing trailers.

E.g.:
 $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182
will show:
 > upload-pack: fix broken if/else chain in config callback
 > Jeff King <peff@peff.net>
 > Junio C Hamano <gitster@pobox.com>

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

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d6add831c0..a920dd15b1 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -243,6 +243,8 @@ endif::git-rev-list[]
    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.
 
 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 541a553ccc..c508357606 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1383,7 +1383,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.filter_data = &filter_list;
 					opts.only_trailers = 1;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
-					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
+					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
+					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 54239290cf..22336c5485 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -667,6 +667,12 @@ test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes non
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
+	echo "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 d6da555cd7..d0d9e91631 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->unfold)
 					unfold_value(&val);
-
-				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				if (!opts->value_only)
+					strbuf_addf(out, "%s: ", tok.buf);
+				strbuf_addbuf(out, &val);
+				strbuf_addch(out, '\n');
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
diff --git a/trailer.h b/trailer.h
index 5255b676de..06d417fe93 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int value_only;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.17.1

[PATCH v4 6/7] strbuf: separate callback for strbuf_expand:ing literals

[Anders Waldenborg]

Expanding '%n' and '%xNN' is generic functionality, so extract that from
the pretty.c formatter into a callback that can be reused.

No functional change intended

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 16 +++++-----------
 strbuf.c | 21 +++++++++++++++++++++
 strbuf.h |  8 ++++++++
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index c508357606..50d0b5830d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1133,9 +1133,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const char *msg = c->message;
 	struct commit_list *p;
 	const char *arg;
-	int ch;
+	size_t res;
 
 	/* these are independent of the commit */
+	res = strbuf_expand_literal_cb(sb, placeholder, NULL);
+	if (res)
+		return res;
+
 	switch (placeholder[0]) {
 	case 'C':
 		if (starts_with(placeholder + 1, "(auto)")) {
@@ -1154,16 +1158,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			 */
 			return ret;
 		}
-	case 'n':		/* newline */
-		strbuf_addch(sb, '\n');
-		return 1;
-	case 'x':
-		/* %x00 == NUL, %x0a == LF, etc. */
-		ch = hex2chr(placeholder + 1);
-		if (ch < 0)
-			return 0;
-		strbuf_addch(sb, ch);
-		return 3;
 	case 'w':
 		if (placeholder[1] == '(') {
 			unsigned long width = 0, indent1 = 0, indent2 = 0;
diff --git a/strbuf.c b/strbuf.c
index f6a6cf78b9..78eecd29f7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 	}
 }
 
+size_t strbuf_expand_literal_cb(struct strbuf *sb,
+				const char *placeholder,
+				void *context)
+{
+	int ch;
+
+	switch (placeholder[0]) {
+	case 'n':		/* newline */
+		strbuf_addch(sb, '\n');
+		return 1;
+	case 'x':
+		/* %x00 == NUL, %x0a == LF, etc. */
+		ch = hex2chr(placeholder + 1);
+		if (ch < 0)
+			return 0;
+		strbuf_addch(sb, ch);
+		return 3;
+	}
+	return 0;
+}
+
 size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 		void *context)
 {
diff --git a/strbuf.h b/strbuf.h
index fc40873b65..52e44c9ab8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb,
 		   expand_fn_t fn,
 		   void *context);
 
+/**
+ * Used as callback for `strbuf_expand` to only expand literals
+ * (i.e. %n and %xNN). The context argument is ignored.
+ */
+size_t strbuf_expand_literal_cb(struct strbuf *sb,
+				const char *placeholder,
+				void *context);
+
 /**
  * Used as callback for `strbuf_expand()`, expects an array of
  * struct strbuf_expand_dict_entry as context, i.e. pairs of
-- 
2.17.1

[PATCH v4 7/7] pretty: add support for separator option in %(trailers)

[Anders Waldenborg]

By default trailer lines are terminated by linebreaks ('\n'). By
specifying the new 'separator' option they will instead be separated by
user provided string and have separator semantics rather than terminator
semantics. The separator string can contain the literal formatting codes
%n and %xNN allowing it to be things that are otherwise hard to type
such as %x00, or comma and end-parenthesis which would break parsing.

E.g:
 $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)'

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt |  9 ++++++++
 pretty.c                         | 10 +++++++++
 t/t4205-log-pretty-formats.sh    | 36 ++++++++++++++++++++++++++++++++
 trailer.c                        | 15 +++++++++++--
 trailer.h                        |  1 +
 5 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a920dd15b1..ce087dee80 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -239,6 +239,15 @@ endif::git-rev-list[]
    `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 described above. 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
+   and a space.
 ** '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.,
diff --git a/pretty.c b/pretty.c
index 50d0b5830d..c7609493ee 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1357,6 +1357,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
+		struct strbuf sepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1376,6 +1377,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.filter = format_trailer_match_cb;
 					opts.filter_data = &filter_list;
 					opts.only_trailers = 1;
+				} else if (match_placeholder_arg_value(arg, "separator", &arg, &argval, &arglen)) {
+					char *fmt;
+
+					strbuf_reset(&sepbuf);
+					fmt = xstrndup(argval, arglen);
+					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
+					free(fmt);
+					opts.separator = &sepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
 					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
@@ -1387,6 +1396,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			ret = arg - placeholder + 1;
 		}
 		string_list_clear (&filter_list, 0);
+		strbuf_release(&sepbuf);
 		return ret;
 	}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 22336c5485..282369dac0 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -673,6 +673,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:separator) changes separator' '
+	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
+	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '
+	git commit --allow-empty -F - <<-\EOF &&
+	Important fix
+
+	The fix is explained here
+
+	Closes: #1234
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	Another fix
+
+	The fix is explained here
+
+	Closes: #567
+	Closes: #890
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	Does not close any tickets
+	EOF
+
+	git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)" HEAD~3.. >actual &&
+	test_write_lines \
+		"Does not close any tickets" \
+		"Another fix #567, #890" \
+		"Important fix #1234" >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 d0d9e91631..0c414f2fed 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,
 				const struct trailer_info *info,
 				const struct process_trailer_options *opts)
 {
+	size_t origlen = out->len;
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter) {
+	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out,
 			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)
 					strbuf_addf(out, "%s: ", tok.buf);
 				strbuf_addbuf(out, &val);
-				strbuf_addch(out, '\n');
+				if (!opts->separator)
+					strbuf_addch(out, '\n');
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
 		} else if (!opts->only_trailers) {
+			if (opts->separator && out->len != origlen) {
+				strbuf_addbuf(out, opts->separator);
+			}
 			strbuf_addstr(out, trailer);
+			if (opts->separator) {
+				strbuf_rtrim(out);
+			}
 		}
 	}
 
diff --git a/trailer.h b/trailer.h
index 06d417fe93..203acf4ee1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -73,6 +73,7 @@ struct process_trailer_options {
 	int unfold;
 	int no_divider;
 	int value_only;
+	const struct strbuf *separator;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.17.1

[PATCH v5 0/7] %(trailers) improvements in pretty format

[Anders Waldenborg]

Updates since v4:
 * Coding style fixes
 * Reuse git_parse_maybe_bool for bool parsing

Anders Waldenborg (7):
  doc: group pretty-format.txt placeholders descriptions
  pretty: Allow %(trailers) options with explicit value
  pretty: single return path in %(trailers) handling
  pretty: allow showing specific trailers
  pretty: add support for "valueonly" option in %(trailers)
  strbuf: separate callback for strbuf_expand:ing literals
  pretty: add support for separator option in %(trailers)

 Documentation/pretty-formats.txt | 260 ++++++++++++++++++-------------
 pretty.c                         | 113 +++++++++++---
 strbuf.c                         |  21 +++
 strbuf.h                         |   8 +
 t/t4205-log-pretty-formats.sh    | 117 ++++++++++++++
 trailer.c                        |  25 ++-
 trailer.h                        |   4 +
 7 files changed, 415 insertions(+), 133 deletions(-)

-- 
2.17.1

[PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions

[Anders Waldenborg]

The placeholders can be grouped into three kinds:
 * literals
 * affecting formatting of later placeholders
 * expanding to information in commit

Also change the list to a definition list (using '::')

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 235 ++++++++++++++++---------------
 1 file changed, 125 insertions(+), 110 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd8..86d804fe97 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -102,118 +102,133 @@ The title was >>t4119: test autocomputing -p<n> for traditional diff input.<<
 +
 The placeholders are:
 
-- '%H': commit hash
-- '%h': abbreviated commit hash
-- '%T': tree hash
-- '%t': abbreviated tree hash
-- '%P': parent hashes
-- '%p': abbreviated parent hashes
-- '%an': author name
-- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1]
-  or linkgit:git-blame[1])
-- '%ae': author email
-- '%aE': author email (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ad': author date (format respects --date= option)
-- '%aD': author date, RFC2822 style
-- '%ar': author date, relative
-- '%at': author date, UNIX timestamp
-- '%ai': author date, ISO 8601-like format
-- '%aI': author date, strict ISO 8601 format
-- '%cn': committer name
-- '%cN': committer name (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ce': committer email
-- '%cE': committer email (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%cd': committer date (format respects --date= option)
-- '%cD': committer date, RFC2822 style
-- '%cr': committer date, relative
-- '%ct': committer date, UNIX timestamp
-- '%ci': committer date, ISO 8601-like format
-- '%cI': committer date, strict ISO 8601 format
-- '%d': ref names, like the --decorate option of linkgit:git-log[1]
-- '%D': ref names without the " (", ")" wrapping.
-- '%e': encoding
-- '%s': subject
-- '%f': sanitized subject line, suitable for a filename
-- '%b': body
-- '%B': raw body (unwrapped subject and body)
+- Placeholders that expand to a single literal character:
+'%n':: newline
+'%%':: a raw '%'
+'%x00':: print a byte from a hex code
+
+- Placeholders that affect formatting of later placeholders:
+'%Cred':: switch color to red
+'%Cgreen':: switch color to green
+'%Cblue':: switch color to blue
+'%Creset':: reset color
+'%C(...)':: color specification, as described under Values in the
+            "CONFIGURATION FILE" section of linkgit:git-config[1].  By
+            default, colors are shown only when enabled for log output
+            (by `color.diff`, `color.ui`, or `--color`, and respecting
+            the `auto` settings of the former if we are going to a
+            terminal). `%C(auto,...)` is accepted as a historical
+            synonym for the default (e.g., `%C(auto,red)`). Specifying
+            `%C(always,...) will show the colors even when color is
+            not otherwise enabled (though consider just using
+            `--color=always` to enable color for the whole output,
+            including this format and anything else git might color).
+            `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
+            on the next placeholders until the color is switched
+            again.
+'%m':: left (`<`), right (`>`) or boundary (`-`) mark
+'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of
+                            linkgit:git-shortlog[1].
+'%<(<N>[,trunc|ltrunc|mtrunc])':: make the next placeholder take at
+                                  least N columns, padding spaces on
+                                  the right if necessary.  Optionally
+                                  truncate at the beginning (ltrunc),
+                                  the middle (mtrunc) or the end
+                                  (trunc) if the output is longer than
+                                  N columns.  Note that truncating
+                                  only works correctly with N >= 2.
+'%<|(<N>)':: make the next placeholder take at least until Nth
+             columns, padding spaces on the right if necessary
+'%>(<N>)', '%>|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' respectively,
+                        but padding spaces on the left
+'%>>(<N>)', '%>>|(<N>)':: similar to '%>(<N>)', '%>|(<N>)'
+                          respectively, except that if the next
+                          placeholder takes more spaces than given and
+                          there are spaces on its left, use those
+                          spaces
+'%><(<N>)', '%><|(<N>)':: similar to '%<(<N>)', '%<|(<N>)'
+                          respectively, but padding both sides
+                          (i.e. the text is centered)
+
+- Placeholders that expand to information extracted from the commit:
+'%H':: commit hash
+'%h':: abbreviated commit hash
+'%T':: tree hash
+'%t':: abbreviated tree hash
+'%P':: parent hashes
+'%p':: abbreviated parent hashes
+'%an':: author name
+'%aN':: author name (respecting .mailmap, see linkgit:git-shortlog[1]
+        or linkgit:git-blame[1])
+'%ae':: author email
+'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
+        or linkgit:git-blame[1])
+'%ad':: author date (format respects --date= option)
+'%aD':: author date, RFC2822 style
+'%ar':: author date, relative
+'%at':: author date, UNIX timestamp
+'%ai':: author date, ISO 8601-like format
+'%aI':: author date, strict ISO 8601 format
+'%cn':: committer name
+'%cN':: committer name (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%ce':: committer email
+'%cE':: committer email (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%cd':: committer date (format respects --date= option)
+'%cD':: committer date, RFC2822 style
+'%cr':: committer date, relative
+'%ct':: committer date, UNIX timestamp
+'%ci':: committer date, ISO 8601-like format
+'%cI':: committer date, strict ISO 8601 format
+'%d':: ref names, like the --decorate option of linkgit:git-log[1]
+'%D':: ref names without the " (", ")" wrapping.
+'%e':: encoding
+'%s':: subject
+'%f':: sanitized subject line, suitable for a filename
+'%b':: body
+'%B':: raw body (unwrapped subject and body)
 ifndef::git-rev-list[]
-- '%N': commit notes
+'%N':: commit notes
 endif::git-rev-list[]
-- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature,
-  "B" for a bad signature,
-  "U" for a good signature with unknown validity,
-  "X" for a good signature that has expired,
-  "Y" for a good signature made by an expired key,
-  "R" for a good signature made by a revoked key,
-  "E" if the signature cannot be checked (e.g. missing key)
-  and "N" for no signature
-- '%GS': show the name of the signer for a signed commit
-- '%GK': show the key used to sign a signed commit
-- '%GF': show the fingerprint of the key used to sign a signed commit
-- '%GP': show the fingerprint of the primary key whose subkey was used
-  to sign a signed commit
-- '%gD': reflog selector, e.g., `refs/stash@{1}` or
-  `refs/stash@{2 minutes ago`}; the format follows the rules described
-  for the `-g` option. The portion before the `@` is the refname as
-  given on the command line (so `git log -g refs/heads/master` would
-  yield `refs/heads/master@{0}`).
-- '%gd': shortened reflog selector; same as `%gD`, but the refname
-  portion is shortened for human readability (so `refs/heads/master`
-  becomes just `master`).
-- '%gn': reflog identity name
-- '%gN': reflog identity name (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ge': reflog identity email
-- '%gE': reflog identity email (respecting .mailmap, see
-  linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%gs': reflog subject
-- '%Cred': switch color to red
-- '%Cgreen': switch color to green
-- '%Cblue': switch color to blue
-- '%Creset': reset color
-- '%C(...)': color specification, as described under Values in the
-  "CONFIGURATION FILE" section of linkgit:git-config[1].
-  By default, colors are shown only when enabled for log output (by
-  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
-  settings of the former if we are going to a terminal). `%C(auto,...)`
-  is accepted as a historical synonym for the default (e.g.,
-  `%C(auto,red)`). Specifying `%C(always,...) will show the colors
-  even when color is not otherwise enabled (though consider
-  just using `--color=always` to enable color for the whole output,
-  including this format and anything else git might color).  `auto`
-  alone (i.e. `%C(auto)`) will turn on auto coloring on the next
-  placeholders until the color is switched again.
-- '%m': left (`<`), right (`>`) or boundary (`-`) mark
-- '%n': newline
-- '%%': a raw '%'
-- '%x00': print a byte from a hex code
-- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
-  linkgit:git-shortlog[1].
-- '%<(<N>[,trunc|ltrunc|mtrunc])': make the next placeholder take at
-  least N columns, padding spaces on the right if necessary.
-  Optionally truncate at the beginning (ltrunc), the middle (mtrunc)
-  or the end (trunc) if the output is longer than N columns.
-  Note that truncating only works correctly with N >= 2.
-- '%<|(<N>)': make the next placeholder take at least until Nth
-  columns, padding spaces on the right if necessary
-- '%>(<N>)', '%>|(<N>)': similar to '%<(<N>)', '%<|(<N>)'
-  respectively, but padding spaces on the left
-- '%>>(<N>)', '%>>|(<N>)': similar to '%>(<N>)', '%>|(<N>)'
-  respectively, except that if the next placeholder takes more spaces
-  than given and there are spaces on its left, use those spaces
-- '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)'
-  respectively, but padding both sides (i.e. the text is centered)
-- %(trailers[:options]): display the trailers of the body as interpreted
-  by linkgit:git-interpret-trailers[1]. The `trailers` string may be
-  followed by a colon and zero or more comma-separated options. If the
-  `only` option is given, omit non-trailer lines from the trailer block.
-  If the `unfold` option is given, behave as if interpret-trailer's
-  `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
-  both.
+'%GG':: raw verification message from GPG for a signed commit
+'%G?':: show "G" for a good (valid) signature,
+        "B" for a bad signature,
+        "U" for a good signature with unknown validity,
+        "X" for a good signature that has expired,
+        "Y" for a good signature made by an expired key,
+        "R" for a good signature made by a revoked key,
+        "E" if the signature cannot be checked (e.g. missing key)
+        and "N" for no signature
+'%GS':: show the name of the signer for a signed commit
+'%GK':: show the key used to sign a signed commit
+'%GF':: show the fingerprint of the key used to sign a signed commit
+'%GP':: show the fingerprint of the primary key whose subkey was used
+        to sign a signed commit
+'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2
+        minutes ago`}; the format follows the rules described for the
+        `-g` option. The portion before the `@` is the refname as
+        given on the command line (so `git log -g refs/heads/master`
+        would yield `refs/heads/master@{0}`).
+'%gd':: shortened reflog selector; same as `%gD`, but the refname
+        portion is shortened for human readability (so
+        `refs/heads/master` becomes just `master`).
+'%gn':: reflog identity name
+'%gN':: reflog identity name (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%ge':: reflog identity email
+'%gE':: reflog identity email (respecting .mailmap, see
+        linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%gs':: reflog subject
+'%(trailers[:options])':: display the trailers of the body as
+                          interpreted by
+                          linkgit:git-interpret-trailers[1]. The
+                          `trailers` string may be followed by a colon
+                          and zero or more comma-separated options:
+** 'only': omit non-trailer lines from the trailer block.
+** 'unfold': make it behave as if interpret-trailer's `--unfold`
+   option was given. E.g., `%(trailers:only,unfold)` unfolds and
+   shows all trailer lines.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.17.1

[PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

[Anders Waldenborg]

In addition to old %(trailers:only) it is now allowed to write
%(trailers:only=yes)

By itself this only gives (the not quite so useful) possibility to have
users change their mind in the middle of a formatting
string (%(trailers:only=true,only=false)). However, it gives users the
opportunity to override defaults from future options.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 14 ++++++---
 pretty.c                         | 52 +++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh    | 18 +++++++++++
 3 files changed, 73 insertions(+), 11 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 86d804fe97..d33b072eb2 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -225,10 +225,16 @@ endif::git-rev-list[]
                           linkgit:git-interpret-trailers[1]. The
                           `trailers` string may be followed by a colon
                           and zero or more comma-separated options:
-** 'only': omit non-trailer lines from the trailer block.
-** 'unfold': make it behave as if interpret-trailer's `--unfold`
-   option was given. E.g., `%(trailers:only,unfold)` unfolds and
-   shows all trailer lines.
+** '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.
+** '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.
 
 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 b83a3ecd23..b8d71a57c9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 	return 0;
 }
 
-static int match_placeholder_arg(const char *to_parse, const char *candidate,
-				 const char **end)
+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
+				       const char **end, const char **valuestart, size_t *valuelen)
 {
 	const char *p;
 
 	if (!(skip_prefix(to_parse, candidate, &p)))
 		return 0;
+	if (valuestart) {
+		if (*p == '=') {
+			*valuestart = p + 1;
+			*valuelen = strcspn(*valuestart, ",)");
+			p = *valuestart + *valuelen;
+		} else {
+			if (*p != ',' && *p != ')')
+				return 0;
+			*valuestart = NULL;
+			*valuelen = 0;
+		}
+	}
 	if (*p == ',') {
 		*end = p + 1;
 		return 1;
@@ -1074,6 +1086,35 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
+				      const char **end, int *val)
+{
+	char buf[8];
+	const char *strval;
+	size_t len;
+	int v;
+
+	if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))
+		return 0;
+
+	if (!strval) {
+		*val = 1;
+		return 1;
+	}
+
+	strlcpy(buf, strval, sizeof(buf));
+	if (len < sizeof(buf))
+		buf[len] = 0;
+
+	v = git_parse_maybe_bool(buf);
+	if (v == -1)
+		return 0;
+
+	*val = v;
+
+	return 1;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		if (*arg == ':') {
 			arg++;
 			for (;;) {
-				if (match_placeholder_arg(arg, "only", &arg))
-					opts.only_trailers = 1;
-				else if (match_placeholder_arg(arg, "unfold", &arg))
-					opts.unfold = 1;
-				else
+				if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
+				    !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66ff..63730a4ec0 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no) shows all trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=no)" >actual &&
+	cat trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:unfold) unfolds trailers' '
 	git log --no-walk --pretty="%(trailers:unfold)" >actual &&
 	{
-- 
2.17.1

Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

[Junio C Hamano]

Anders Waldenborg <anders@0x63.nu> writes:

> Subject: Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

Style: s/pretty: Allow/pretty: allow/ (haven't I said this often enough?)

> +** '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.
> +** '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.

Sounds sensible.

> diff --git a/pretty.c b/pretty.c
> index b83a3ecd23..b8d71a57c9 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
>  	return 0;
>  }
>  
> -static int match_placeholder_arg(const char *to_parse, const char *candidate,
> -				 const char **end)
> +static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
> +				       const char **end, const char **valuestart, size_t *valuelen)

An overlong line here.

> ...
> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
> +				      const char **end, int *val)
> +{
> +	char buf[8];
> +	const char *strval;
> +	size_t len;
> +	int v;
> +
> +	if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))
> +		return 0;
> +
> +	if (!strval) {
> +		*val = 1;
> +		return 1;
> +	}
> +
> +	strlcpy(buf, strval, sizeof(buf));
> +	if (len < sizeof(buf))
> +		buf[len] = 0;

Doesn't strlcpy() terminate buf[len] if len is short enough?
Even if the strval is longer than buf[], strlcpy() would truncate
and make sure buf[] is NUL terminated, no?

> +	v = git_parse_maybe_bool(buf);

Why?

This function would simply be buggy and incapable of parsing a
representation of a boolean value that is longer than 8 bytes (if
such a representation exists), so chomping an overlong string at the
end and feeding it to git_parse_maybe_bool() is a nonsense, isn't
it?

In this particular case, strlcpy() is inviting a buggy programming.
If there were a 7-letter representation of falsehood, strval may be
that 7-letter thing, in which case you would want to feed it to
git_parse_maybe_bool() to receive "false" from it, or strval may
have that 7-letter thing followed by a 'x' (so as a token, that is
not a correctly spelled falsehood), but strlcpy() would chomp and
show the same 7-letter falsehood to git_parse_maybe_bool().  That
robs you from an opportunity to diagnose such a bogus input as an
error.

Instead of using "char buf[8]", just using a strbuf and avoidng
strlcpy() would make the code much better, I would think.

Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

[Anders Waldenborg]

Junio C Hamano writes:

>> ...
>> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
>> +				      const char **end, int *val)
>> +{
>> +	char buf[8];
>> +	const char *strval;
>> +	size_t len;
>> +	int v;
>> +
>> +	if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))
>> +		return 0;
>> +
>> +	if (!strval) {
>> +		*val = 1;
>> +		return 1;
>> +	}
>> +
>> +	strlcpy(buf, strval, sizeof(buf));
>> +	if (len < sizeof(buf))
>> +		buf[len] = 0;
>
> Doesn't strlcpy() terminate buf[len] if len is short enough?
> Even if the strval is longer than buf[], strlcpy() would truncate
> and make sure buf[] is NUL terminated, no?

Yes, but no. strval is not NUL-terminated at len. E.g strval would point
to "false,something=true". `buf[len] = 0` makes sure it becomes "false".

> Instead of using "char buf[8]", just using a strbuf and avoidng
> strlcpy() would make the code much better, I would think.

Yes, taking the heap allocation hit would most likely make the intent
clearer.

Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value

[Jeff King]

On Tue, Jan 29, 2019 at 07:45:06AM +0100, Anders Waldenborg wrote:

> > Instead of using "char buf[8]", just using a strbuf and avoidng
> > strlcpy() would make the code much better, I would think.
> 
> Yes, taking the heap allocation hit would most likely make the intent
> clearer.

If you can reuse the same struct and strbuf_reset() it each time, then
that amortizes the cost of the heap (to basically once per program run,
instead of once per commit).

-Peff

Re: [PATCH v5 2/7 update] pretty: allow %(trailers) options with explicit value

[Anders Waldenborg]

In addition to old %(trailers:only) it is now allowed to write
%(trailers:only=yes)

By itself this only gives (the not quite so useful) possibility to have
users change their mind in the middle of a formatting
string (%(trailers:only=true,only=false)). However, it gives users the
opportunity to override defaults from future options.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 14 ++++++---
 pretty.c                         | 52 +++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh    | 18 +++++++++++
 3 files changed, 73 insertions(+), 11 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 86d804fe97..d33b072eb2 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -225,10 +225,16 @@ endif::git-rev-list[]
                           linkgit:git-interpret-trailers[1]. The
                           `trailers` string may be followed by a colon
                           and zero or more comma-separated options:
-** 'only': omit non-trailer lines from the trailer block.
-** 'unfold': make it behave as if interpret-trailer's `--unfold`
-   option was given. E.g., `%(trailers:only,unfold)` unfolds and
-   shows all trailer lines.
+** '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.
+** '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.
 
 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 b83a3ecd23..4dfbd38cf6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,13 +1056,26 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 	return 0;
 }
 
-static int match_placeholder_arg(const char *to_parse, const char *candidate,
-				 const char **end)
+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
+				       const char **end, const char **valuestart,
+				       size_t *valuelen)
 {
 	const char *p;
 
 	if (!(skip_prefix(to_parse, candidate, &p)))
 		return 0;
+	if (valuestart) {
+		if (*p == '=') {
+			*valuestart = p + 1;
+			*valuelen = strcspn(*valuestart, ",)");
+			p = *valuestart + *valuelen;
+		} else {
+			if (*p != ',' && *p != ')')
+				return 0;
+			*valuestart = NULL;
+			*valuelen = 0;
+		}
+	}
 	if (*p == ',') {
 		*end = p + 1;
 		return 1;
@@ -1074,6 +1087,34 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
+				      const char **end, int *val)
+{
+	const char *argval;
+	char *strval;
+	size_t arglen;
+	int v;
+
+	if (!match_placeholder_arg_value(to_parse, candidate, end, &argval, &arglen))
+		return 0;
+
+	if (!argval) {
+		*val = 1;
+		return 1;
+	}
+
+	strval = xstrndup(argval, arglen);
+	v = git_parse_maybe_bool(strval);
+	free(strval);
+
+	if (v == -1)
+		return 0;
+
+	*val = v;
+
+	return 1;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		if (*arg == ':') {
 			arg++;
 			for (;;) {
-				if (match_placeholder_arg(arg, "only", &arg))
-					opts.only_trailers = 1;
-				else if (match_placeholder_arg(arg, "unfold", &arg))
-					opts.unfold = 1;
-				else
+				if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
+				    !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66ff..63730a4ec0 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no) shows all trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=no)" >actual &&
+	cat trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' '
+	git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:unfold) unfolds trailers' '
 	git log --no-walk --pretty="%(trailers:unfold)" >actual &&
 	{
-- 
2.17.1

[PATCH v5 3/7] pretty: single return path in %(trailers) handling

[Anders Waldenborg]

No functional change intended.

This change may not seem useful on its own, but upcoming commits will do
memory allocation in there, and a single return path makes deallocation
easier.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index b8d71a57c9..65a1b9bd82 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1353,6 +1353,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		size_t ret = 0;
 
 		opts.no_divider = 1;
 
@@ -1366,8 +1367,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
-			return arg - placeholder + 1;
+			ret = arg - placeholder + 1;
 		}
+		return ret;
 	}
 
 	return 0;	/* unknown placeholder */
-- 
2.17.1

[PATCH v5 4/7] pretty: allow showing specific trailers

[Anders Waldenborg]

Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailer lines which match any of the specified keys.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt |  8 +++++
 pretty.c                         | 36 ++++++++++++++++++--
 t/t4205-log-pretty-formats.sh    | 57 ++++++++++++++++++++++++++++++++
 trailer.c                        | 10 +++---
 trailer.h                        |  2 ++
 5 files changed, 107 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d33b072eb2..d6add831c0 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -225,6 +225,14 @@ endif::git-rev-list[]
                           linkgit:git-interpret-trailers[1]. The
                           `trailers` string may be followed by a colon
                           and zero or more comma-separated options:
+** '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
diff --git a/pretty.c b/pretty.c
index 65a1b9bd82..d3dd2d6254 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1115,6 +1115,19 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat
 	return 1;
 }
 
+static int format_trailer_match_cb(const struct strbuf *key, void *ud)
+{
+	const struct string_list *list = ud;
+	const struct string_list_item *item;
+
+	for_each_string_list_item (item, list) {
+		if (key->len == (uintptr_t)item->util &&
+		    !strncasecmp(item->string, key->buf, key->len))
+			return 1;
+	}
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1353,6 +1366,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		struct string_list filter_list = STRING_LIST_INIT_NODUP;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1360,8 +1374,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		if (*arg == ':') {
 			arg++;
 			for (;;) {
-				if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
-				    !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
+				const char *argval;
+				size_t arglen;
+
+				if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) {
+					uintptr_t len = arglen;
+
+					if (!argval)
+						goto trailer_out;
+
+					if (len && argval[len - 1] == ':')
+						len--;
+					string_list_append(&filter_list, argval)->util = (char *)len;
+
+					opts.filter = format_trailer_match_cb;
+					opts.filter_data = &filter_list;
+					opts.only_trailers = 1;
+				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
+					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
 					break;
 			}
 		}
@@ -1369,6 +1399,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
 			ret = arg - placeholder + 1;
 		}
+	trailer_out:
+		string_list_clear(&filter_list, 0);
 		return ret;
 	}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 63730a4ec0..d87201afbe 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -616,6 +616,63 @@ test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+	git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) multiple keys' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by:,key=Signed-off-By)" >actual &&
+	grep -v patch.description <trailers >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+	git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+	echo "xx" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual &&
+	grep -v patch.description <trailers | grep -v Acked-by >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual &&
+	unfold <trailers | grep Signed-off-by >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,only=no)" >actual &&
+	{
+		echo "Acked-by: A U Thor <author@example.com>" &&
+		grep patch.description <trailers
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key) without value is error' '
+	git log --no-walk --pretty="tformat:%(trailers:key)" >actual &&
+	echo "%(trailers:key)" >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..d6da555cd7 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold) {
+	if (!opts->only_trailers && !opts->unfold && !opts->filter) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,
 			struct strbuf val = STRBUF_INIT;
 
 			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (opts->unfold)
-				unfold_value(&val);
+			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+				if (opts->unfold)
+					unfold_value(&val);
 
-			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
diff --git a/trailer.h b/trailer.h
index b997739649..5255b676de 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,8 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int (*filter)(const struct strbuf *, void *);
+	void *filter_data;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.17.1

[PATCH v5 5/7] pretty: add support for "valueonly" option in %(trailers)

[Anders Waldenborg]

With the new "key=" option to %(trailers) it often makes little sense to
show the key, as it by definition already is knows which trailer is
printed there. This new "valueonly" option makes it omit the key when
printing trailers.

E.g.:
 $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182
will show:
 > upload-pack: fix broken if/else chain in config callback
 > Jeff King <peff@peff.net>
 > Junio C Hamano <gitster@pobox.com>

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

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d6add831c0..a920dd15b1 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -243,6 +243,8 @@ endif::git-rev-list[]
    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.
 
 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 d3dd2d6254..ed25845c98 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1391,7 +1391,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.filter_data = &filter_list;
 					opts.only_trailers = 1;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
-					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
+					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
+					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d87201afbe..1ad6834781 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -673,6 +673,12 @@ test_expect_success '%(trailers:key) without value is error' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
+	echo "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 d6da555cd7..d0d9e91631 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->unfold)
 					unfold_value(&val);
-
-				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				if (!opts->value_only)
+					strbuf_addf(out, "%s: ", tok.buf);
+				strbuf_addbuf(out, &val);
+				strbuf_addch(out, '\n');
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
diff --git a/trailer.h b/trailer.h
index 5255b676de..06d417fe93 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int value_only;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.17.1

[PATCH v5 6/7] strbuf: separate callback for strbuf_expand:ing literals

[Anders Waldenborg]

Expanding '%n' and '%xNN' is generic functionality, so extract that from
the pretty.c formatter into a callback that can be reused.

No functional change intended

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 pretty.c | 16 +++++-----------
 strbuf.c | 21 +++++++++++++++++++++
 strbuf.h |  8 ++++++++
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index ed25845c98..7baa4c1c26 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1137,9 +1137,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const char *msg = c->message;
 	struct commit_list *p;
 	const char *arg;
-	int ch;
+	size_t res;
 
 	/* these are independent of the commit */
+	res = strbuf_expand_literal_cb(sb, placeholder, NULL);
+	if (res)
+		return res;
+
 	switch (placeholder[0]) {
 	case 'C':
 		if (starts_with(placeholder + 1, "(auto)")) {
@@ -1158,16 +1162,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			 */
 			return ret;
 		}
-	case 'n':		/* newline */
-		strbuf_addch(sb, '\n');
-		return 1;
-	case 'x':
-		/* %x00 == NUL, %x0a == LF, etc. */
-		ch = hex2chr(placeholder + 1);
-		if (ch < 0)
-			return 0;
-		strbuf_addch(sb, ch);
-		return 3;
 	case 'w':
 		if (placeholder[1] == '(') {
 			unsigned long width = 0, indent1 = 0, indent2 = 0;
diff --git a/strbuf.c b/strbuf.c
index f6a6cf78b9..78eecd29f7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 	}
 }
 
+size_t strbuf_expand_literal_cb(struct strbuf *sb,
+				const char *placeholder,
+				void *context)
+{
+	int ch;
+
+	switch (placeholder[0]) {
+	case 'n':		/* newline */
+		strbuf_addch(sb, '\n');
+		return 1;
+	case 'x':
+		/* %x00 == NUL, %x0a == LF, etc. */
+		ch = hex2chr(placeholder + 1);
+		if (ch < 0)
+			return 0;
+		strbuf_addch(sb, ch);
+		return 3;
+	}
+	return 0;
+}
+
 size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 		void *context)
 {
diff --git a/strbuf.h b/strbuf.h
index fc40873b65..52e44c9ab8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb,
 		   expand_fn_t fn,
 		   void *context);
 
+/**
+ * Used as callback for `strbuf_expand` to only expand literals
+ * (i.e. %n and %xNN). The context argument is ignored.
+ */
+size_t strbuf_expand_literal_cb(struct strbuf *sb,
+				const char *placeholder,
+				void *context);
+
 /**
  * Used as callback for `strbuf_expand()`, expects an array of
  * struct strbuf_expand_dict_entry as context, i.e. pairs of
-- 
2.17.1

[PATCH v5 7/7] pretty: add support for separator option in %(trailers)

[Anders Waldenborg]

By default trailer lines are terminated by linebreaks ('\n'). By
specifying the new 'separator' option they will instead be separated by
user provided string and have separator semantics rather than terminator
semantics. The separator string can contain the literal formatting codes
%n and %xNN allowing it to be things that are otherwise hard to type
such as %x00, or comma and end-parenthesis which would break parsing.

E.g:
 $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)'

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt |  9 ++++++++
 pretty.c                         | 10 +++++++++
 t/t4205-log-pretty-formats.sh    | 36 ++++++++++++++++++++++++++++++++
 trailer.c                        | 15 +++++++++++--
 trailer.h                        |  1 +
 5 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a920dd15b1..ce087dee80 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -239,6 +239,15 @@ endif::git-rev-list[]
    `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 described above. 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
+   and a space.
 ** '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.,
diff --git a/pretty.c b/pretty.c
index 7baa4c1c26..99b66ccf5a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1361,6 +1361,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
+		struct strbuf sepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1384,6 +1385,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.filter = format_trailer_match_cb;
 					opts.filter_data = &filter_list;
 					opts.only_trailers = 1;
+				} else if (match_placeholder_arg_value(arg, "separator", &arg, &argval, &arglen)) {
+					char *fmt;
+
+					strbuf_reset(&sepbuf);
+					fmt = xstrndup(argval, arglen);
+					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
+					free(fmt);
+					opts.separator = &sepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
 					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
@@ -1396,6 +1405,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		}
 	trailer_out:
 		string_list_clear(&filter_list, 0);
+		strbuf_release(&sepbuf);
 		return ret;
 	}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 1ad6834781..99f50fa401 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -679,6 +679,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:separator) changes separator' '
+	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
+	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '
+	git commit --allow-empty -F - <<-\EOF &&
+	Important fix
+
+	The fix is explained here
+
+	Closes: #1234
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	Another fix
+
+	The fix is explained here
+
+	Closes: #567
+	Closes: #890
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	Does not close any tickets
+	EOF
+
+	git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)" HEAD~3.. >actual &&
+	test_write_lines \
+		"Does not close any tickets" \
+		"Another fix #567, #890" \
+		"Important fix #1234" >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 d0d9e91631..0c414f2fed 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,
 				const struct trailer_info *info,
 				const struct process_trailer_options *opts)
 {
+	size_t origlen = out->len;
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter) {
+	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out,
 			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)
 					strbuf_addf(out, "%s: ", tok.buf);
 				strbuf_addbuf(out, &val);
-				strbuf_addch(out, '\n');
+				if (!opts->separator)
+					strbuf_addch(out, '\n');
 			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
 		} else if (!opts->only_trailers) {
+			if (opts->separator && out->len != origlen) {
+				strbuf_addbuf(out, opts->separator);
+			}
 			strbuf_addstr(out, trailer);
+			if (opts->separator) {
+				strbuf_rtrim(out);
+			}
 		}
 	}
 
diff --git a/trailer.h b/trailer.h
index 06d417fe93..203acf4ee1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -73,6 +73,7 @@ struct process_trailer_options {
 	int unfold;
 	int no_divider;
 	int value_only;
+	const struct strbuf *separator;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.17.1