[PATCH 0/2] add format specifiers to display trailers

[PATCH 0/2] add format specifiers to display trailers

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

This is based off of jt/use-trailer-api-in-commands so that we can make
use of the public trailer API that will parse a string for trailers.

I use trailers as a way to store extra commit metadata, and would like a
convenient way to obtain the trailers of a commit message easily. This
adds format specifiers to both the ref-filter API and the pretty
format specifiers, using %(trailers) for both (and also
contents:trailers for ref-filter).

Additionally, I am somewhat not a fan of the way that if you have a
series of trailers which are trailer format, but not recognized, such
as the following:

	<text>

	My-tag: my value
	My-other-tag: my other value
	[non-trailer line]
	My-tag: my third value

Git interpret-trailers will not recognize this as a trailer block
because it doesn't have any standard git tags within it.

Junio suggested that we should treat all the configured trailer prefixes
as recognized so that it would work as well, but it doesn't appear to
do this at least for jt/use-trailer-api-in-commands

I think that's the right solution, since it's extensible, though it
would mean that interpret-trailers would behave differently on different
systems... not really sure it's all bad though.

interdiff v1:
diff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt
index 9ee68a4cb64a..47b286b33e4e 100644
--- c/Documentation/pretty-formats.txt
+++ w/Documentation/pretty-formats.txt
@@ -138,7 +138,6 @@ The placeholders are:
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
 - '%b': body
-- '%bT': trailers of body as interpreted by linkgit:git-interpret-trailers[1]
 - '%B': raw body (unwrapped subject and body)
 ifndef::git-rev-list[]
 - '%N': commit notes
@@ -200,6 +199,8 @@ 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): display the trailers of the body as interpreted by
+  linkgit:git-interpret-trailers[1]
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git c/pretty.c w/pretty.c
index ea8764334865..5e683830d9d6 100644
--- c/pretty.c
+++ w/pretty.c
@@ -1300,16 +1300,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
		format_sanitized_subject(sb, msg + c->subject_off);
		return 1;
	case 'b':	/* body */
-		switch (placeholder[1]) {
-		case 'T':
-			format_trailers(sb, msg + c->subject_off);
-			return 2;
-		default:
-			break;
-		}
		strbuf_addstr(sb, msg + c->body_off);
		return 1;
	}
+
+	if (starts_with(placeholder, "(trailers)")) {
+		format_trailers(sb, msg + c->subject_off);
+		return strlen("(trailers)");
+	}
+
	return 0;	/* unknown placeholder */
 }
 
diff --git c/t/t4205-log-pretty-formats.sh w/t/t4205-log-pretty-formats.sh
index 7a35941ddcbd..21eb8c8587f2 100755
--- c/t/t4205-log-pretty-formats.sh
+++ w/t/t4205-log-pretty-formats.sh
@@ -542,7 +542,7 @@ Acked-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
 EOF
 
-test_expect_success 'pretty format %bT shows trailers' '
+test_expect_success 'pretty format %(trailers) shows trailers' '
	echo "Some contents" >trailerfile &&
	git add trailerfile &&
	git commit -F - <<-EOF &&
@@ -553,7 +553,7 @@ test_expect_success 'pretty format %bT shows trailers' '
 
	$(cat trailers)
	EOF
-	git log --no-walk --pretty="%bT" >actual &&
+	git log --no-walk --pretty="%(trailers)" >actual &&
	cat >expect <<-EOF &&
	$(cat trailers)
 

Jacob Keller (2):
  pretty: add %bT format for displaying trailers of a commit message
  ref-filter: add support to display trailers as part of contents

 Documentation/git-for-each-ref.txt |  2 ++
 Documentation/pretty-formats.txt   |  1 +
 pretty.c                           | 18 ++++++++++++++++++
 ref-filter.c                       | 22 +++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh      | 26 ++++++++++++++++++++++++++
 t/t6300-for-each-ref.sh            | 26 ++++++++++++++++++++++++++
 6 files changed, 94 insertions(+), 1 deletion(-)

-- 
2.11.0.rc2.152.g4d04e67

[PATCH v2 1/2] pretty: add %(trailers) format for displaying trailers of a commit message

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Recent patches have expanded on the trailers.c code and we have the
builtin commant git-interpret-trailers which can be used to add or
modify trailer lines. However, there is no easy way to simply display
the trailers of a commit message.

Add support for %(trailers) format modifier which will use the
trailer_info_get() calls to read trailers in an identical way as git
interpret-trailers does. Use a long format option instead of a short
name so that future work can more easily unify ref-filter and pretty
formats.

Add documentation and tests for the same.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/pretty-formats.txt |  2 ++
 pretty.c                         | 17 +++++++++++++++++
 t/t4205-log-pretty-formats.sh    | 26 ++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3bcee2ddb124..47b286b33e4e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -199,6 +199,8 @@ 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): display the trailers of the body as interpreted by
+  linkgit:git-interpret-trailers[1]
 
 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 37b2c3b1f995..5e683830d9d6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -10,6 +10,7 @@
 #include "color.h"
 #include "reflog-walk.h"
 #include "gpg-interface.h"
+#include "trailer.h"
 
 static char *user_format;
 static struct cmt_fmt_map {
@@ -889,6 +890,16 @@ const char *format_subject(struct strbuf *sb, const char *msg,
 	return msg;
 }
 
+static void format_trailers(struct strbuf *sb, const char *msg)
+{
+	struct trailer_info info;
+
+	trailer_info_get(&info, msg);
+	strbuf_add(sb, info.trailer_start,
+		   info.trailer_end - info.trailer_start);
+	trailer_info_release(&info);
+}
+
 static void parse_commit_message(struct format_commit_context *c)
 {
 	const char *msg = c->message + c->message_off;
@@ -1292,6 +1303,12 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		strbuf_addstr(sb, msg + c->body_off);
 		return 1;
 	}
+
+	if (starts_with(placeholder, "(trailers)")) {
+		format_trailers(sb, msg + c->subject_off);
+		return strlen("(trailers)");
+	}
+
 	return 0;	/* unknown placeholder */
 }
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index f5435fd250ba..21eb8c8587f2 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -535,4 +535,30 @@ test_expect_success 'clean log decoration' '
 	test_cmp expected actual1
 '
 
+cat >trailers <<EOF
+Signed-off-by: A U Thor <author@example.com>
+Acked-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Signed-off-by: A U Thor <author@example.com>
+EOF
+
+test_expect_success 'pretty format %(trailers) shows trailers' '
+	echo "Some contents" >trailerfile &&
+	git add trailerfile &&
+	git commit -F - <<-EOF &&
+	trailers: this commit message has trailers
+
+	This commit is a test commit with trailers at the end. We parse this
+	message and display the trailers using %bT
+
+	$(cat trailers)
+	EOF
+	git log --no-walk --pretty="%(trailers)" >actual &&
+	cat >expect <<-EOF &&
+	$(cat trailers)
+
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.rc2.152.g4d04e67

[PATCH v2 2/2] ref-filter: add support to display trailers as part of contents

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Add %(trailers) and %(contents:trailers) to display the trailers as
interpreted by trailer_info_get. Update documentation and add a test for
the new feature.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-for-each-ref.txt |  2 ++
 ref-filter.c                       | 22 +++++++++++++++++++++-
 t/t6300-for-each-ref.sh            | 26 ++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f57e69bc83e3..e5807eede787 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -165,6 +165,8 @@ of all lines of the commit message up to the first blank line.  The next
 line is 'contents:body', where body is all of the lines after the first
 blank line.  The optional GPG signature is `contents:signature`.  The
 first `N` lines of the message is obtained using `contents:lines=N`.
+Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
+are obtained as 'contents:trailers'.
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index d4c2931f3aab..b6f1bb73ed37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -13,6 +13,7 @@
 #include "utf8.h"
 #include "git-compat-util.h"
 #include "version.h"
+#include "trailer.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -40,7 +41,7 @@ static struct used_atom {
 		enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
 			remote_ref;
 		struct {
-			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
 			unsigned int nlines;
 		} contents;
 		enum { O_FULL, O_SHORT } objectname;
@@ -85,6 +86,13 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
 	atom->u.contents.option = C_SUB;
 }
 
+static void trailers_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (arg)
+		die(_("%%(trailers) does not take arguments"));
+	atom->u.contents.option = C_TRAILERS;
+}
+
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
 {
 	if (!arg)
@@ -95,6 +103,8 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
 		atom->u.contents.option = C_SIG;
 	else if (!strcmp(arg, "subject"))
 		atom->u.contents.option = C_SUB;
+	else if (!strcmp(arg, "trailers"))
+		atom->u.contents.option = C_TRAILERS;
 	else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
@@ -194,6 +204,7 @@ static struct {
 	{ "creatordate", FIELD_TIME },
 	{ "subject", FIELD_STR, subject_atom_parser },
 	{ "body", FIELD_STR, body_atom_parser },
+	{ "trailers", FIELD_STR, trailers_atom_parser },
 	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
@@ -785,6 +796,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			name++;
 		if (strcmp(name, "subject") &&
 		    strcmp(name, "body") &&
+		    strcmp(name, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -808,6 +820,14 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			/*  Size is the length of the message after removing the signature */
 			append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);
 			v->s = strbuf_detach(&s, NULL);
+		} else if (atom->u.contents.option == C_TRAILERS) {
+			struct trailer_info info;
+
+			/* Search for trailer info */
+			trailer_info_get(&info, subpos);
+			v->s = xmemdupz(info.trailer_start,
+					info.trailer_end - info.trailer_start);
+			trailer_info_release(&info);
 		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
 	}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 19a2823025e7..eb4bac0fe477 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -553,4 +553,30 @@ test_expect_success 'Verify sort with multiple keys' '
 		refs/tags/bogo refs/tags/master > actual &&
 	test_cmp expected actual
 '
+
+cat >trailers <<EOF
+Reviewed-by: A U Thor <author@example.com>
+Signed-off-by: A U Thor <author@example.com>
+EOF
+
+test_expect_success 'basic atom: head contents:trailers' '
+	echo "Some contents" > two &&
+	git add two &&
+	git commit -F - <<-EOF &&
+	trailers: this commit message has trailers
+
+	Some message contents
+
+	$(cat trailers)
+	EOF
+	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
+	sanitize_pgp <actual >actual.clean &&
+	# git for-each-ref ends with a blank line
+	cat >expect <<-EOF &&
+	$(cat trailers)
+
+	EOF
+	test_cmp expect actual.clean
+'
+
 test_done
-- 
2.11.0.rc2.152.g4d04e67

[PATCH 0/2] add format specifiers to display trailers

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

This is based off of jt/use-trailer-api-in-commands so that we can make
use of the public trailer API that will parse a string for trailers.

I use trailers as a way to store extra commit metadata, and would like a
convenient way to obtain the trailers of a commit message easily. This
adds format specifiers to both the ref-filter API and the pretty
formats. I am not a fan of %bT but %t and %T were already taken. I don't
really know if it's ok to use %bT, since I think we used to allow "%bT"
format, though i don't think this is likely used much in practice.

I am open to suggestions for the pretty format specifier.

Additionally, I am somewhat not a fan of the way that if you have a
series of trailers which are trailer format, but not recognized, such
as the following:

<text>

My-tag: my value
My-other-tag: my other value
[non-trailer line]
My-tag: my third value

---

Git interpret-trailers will not recognize this as a trailer block
because it doesn't have any standard git tags within it. Would it be ok
to augment the trailer interpretation to say that if we have over 75%
trailers in the block that we accept it even if it doesn't have any real
recognized tags?

I say this because I regularly use extra tags in my git projects to
represent change metadata, and it would be nice if the tag block could
be recognized even if it has 1-2 lines of non-trailer formatting in
it...

Thoughts?

Jacob Keller (2):
  pretty: add %bT format for displaying trailers of a commit message
  ref-filter: add support to display trailers as part of contents

 Documentation/git-for-each-ref.txt |  2 ++
 Documentation/pretty-formats.txt   |  1 +
 pretty.c                           | 18 ++++++++++++++++++
 ref-filter.c                       | 22 +++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh      | 26 ++++++++++++++++++++++++++
 t/t6300-for-each-ref.sh            | 26 ++++++++++++++++++++++++++
 6 files changed, 94 insertions(+), 1 deletion(-)

-- 
2.11.0.rc2.152.g4d04e67

Re: [PATCH 0/2] add format specifiers to display trailers

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> Git interpret-trailers will not recognize this as a trailer block
> because it doesn't have any standard git tags within it. Would it be ok
> to augment the trailer interpretation to say that if we have over 75%
> trailers in the block that we accept it even if it doesn't have any real
> recognized tags?

I thought the documented way to do this is to configure one of your
custom trailer as such.  Jonathan?

>   pretty: add %bT format for displaying trailers of a commit message

Are %(...) taken already?  In longer term, it would be nice if we
can unify the --pretty formats and for-each-ref formats, so it is
probably better if we avoid adding any new short ones to the former.

We have %s and %b so that we can reconstruct the whole thing by
using both.  It is unclear how %bT fits in this picture.  I wonder
if we also need another placeholder that expands to the body of the
message without the trailer---otherwise the whole set would become
incoherent, no?

Re: [PATCH 0/2] add format specifiers to display trailers

[Jacob Keller]

On Fri, Nov 18, 2016 at 3:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> Git interpret-trailers will not recognize this as a trailer block
>> because it doesn't have any standard git tags within it. Would it be ok
>> to augment the trailer interpretation to say that if we have over 75%
>> trailers in the block that we accept it even if it doesn't have any real
>> recognized tags?
>
> I thought the documented way to do this is to configure one of your
> custom trailer as such.  Jonathan?
>

That would be fine then, if that works.

>>   pretty: add %bT format for displaying trailers of a commit message
>
> Are %(...) taken already?  In longer term, it would be nice if we
> can unify the --pretty formats and for-each-ref formats, so it is
> probably better if we avoid adding any new short ones to the former.
>

Oh, I hadn't considered adding a longer one. I'll rework this to use
longer ones.

> We have %s and %b so that we can reconstruct the whole thing by
> using both.  It is unclear how %bT fits in this picture.  I wonder
> if we also need another placeholder that expands to the body of the
> message without the trailer---otherwise the whole set would become
> incoherent, no?
>

I'm not entirely sure what to do here. I just wanted a way to easily
format "just the trailers" of a message. We could add something that
formats just the non-trailers, that's not too difficult. Not really
sure what I'd call it though.

Thanks,
Jake

Re: [PATCH 0/2] add format specifiers to display trailers

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

>> We have %s and %b so that we can reconstruct the whole thing by
>> using both.  It is unclear how %bT fits in this picture.  I wonder
>> if we also need another placeholder that expands to the body of the
>> message without the trailer---otherwise the whole set would become
>> incoherent, no?
>
> I'm not entirely sure what to do here. I just wanted a way to easily
> format "just the trailers" of a message. We could add something that
> formats just the non-trailers, that's not too difficult. Not really
> sure what I'd call it though.

I was wondering if %(log:<name of a part>) was a better way to go.

%(log:title) and %(log:body) would be equivalents of traditional %s
and %b, and %(log:body) in turn would be a shorter way to write
%(log:description)%+(log:trailer), i.e. show the message body, and
if there is a trailer block, add it after adding a blank line.

Or something like that?

Re: [PATCH 0/2] add format specifiers to display trailers

[Keller, Jacob E]

On Mon, 2016-11-21 at 09:23 -0800, Junio C Hamano wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> > > We have %s and %b so that we can reconstruct the whole thing by
> > > using both.  It is unclear how %bT fits in this picture.  I
> > > wonder
> > > if we also need another placeholder that expands to the body of
> > > the
> > > message without the trailer---otherwise the whole set would
> > > become
> > > incoherent, no?
> > 
> > I'm not entirely sure what to do here. I just wanted a way to
> > easily
> > format "just the trailers" of a message. We could add something
> > that
> > formats just the non-trailers, that's not too difficult. Not really
> > sure what I'd call it though.
> 
> I was wondering if %(log:<name of a part>) was a better way to go.
> 
> %(log:title) and %(log:body) would be equivalents of traditional %s
> and %b, and %(log:body) in turn would be a shorter way to write
> %(log:description)%+(log:trailer), i.e. show the message body, and
> if there is a trailer block, add it after adding a blank line.
> 
> Or something like that?

That would work for me.

Thanks,
Jake