[PATCH 0/5] Support %(trailers) arguments in for-each-ref(1)

[PATCH 0/5] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is a patch to extend Peff's recent work of adding parsing options to
"--pretty=%(trailers)" by supporting those same options in git-for-each-ref(1).

In summary, this patch adds correct behavior for the following options, when
given to git-for-each-ref(1):

  * --format="%(trailers:only)"

  * --format="%(trailers:unfold,unfold)"

  * --format="%(contents:trailers:only)"

  * --format="%(contents:trailers:unfold,unfold)"

I have changed the syntax for specifying multiple sub-arguments in
%(contents:trailers) and %(trailers) atoms to be ","-delimited instead of
%":"-delimited. This is consistent with similar atoms, and is described in
%greater detail in "pretty.c: delimit "%(trailers)" arguments with ","".

I am also new around here: this is my first patch that I am sending to the
mailing list, so this process is entirely new to me. My current focus is helping
maintain Git LFS [1] at GitHub. If I have made any mistakes in formatting these
patches or sending them, please let me know :-).

Thank you in advance.

--
- Taylor

[1]: https://git-lfs.github.com

Taylor Blau (5):
  pretty.c: delimit "%(trailers)" arguments with ","
  t6300: refactor %(trailers) tests
  ref-filter.c: add trailer options to used_atom
  ref-filter.c: use trailer_opts to format trailers
  ref-filter.c: parse trailers arguments with %(contents) atom

 Documentation/git-for-each-ref.txt |  7 ++-
 pretty.c                           | 13 +++---
 ref-filter.c                       | 38 +++++++++++-----
 t/t4205-log-pretty-formats.sh      |  4 +-
 t/t6300-for-each-ref.sh            | 88 +++++++++++++++++++++++++++++++++++++-
 5 files changed, 129 insertions(+), 21 deletions(-)

--
2.14.1.145.gb3622a4ee

[PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c
to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments given to
the trailers atom itself. It is consistent with the behavior of "%(upstream)"
and "%(push)" atoms.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pretty.c                      | 13 ++++++++-----
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0e23fe3c0..68f736912 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1265,11 +1265,14 @@ 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;
-		while (*arg == ':') {
-			if (skip_prefix(arg, ":only", &arg))
-				opts.only_trailers = 1;
-			else if (skip_prefix(arg, ":unfold", &arg))
-				opts.unfold = 1;
+		if (skip_prefix(arg, ":", &arg)) {
+			while (*arg != ')') {
+				skip_prefix(arg, ",", &arg);
+				if (skip_prefix(arg, "only", &arg))
+					opts.only_trailers = 1;
+				else if (skip_prefix(arg, "unfold", &arg))
+					opts.unfold = 1;
+			}
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with ","

[Jeff King]

On Fri, Sep 29, 2017 at 11:22:34PM -0700, Taylor Blau wrote:

> In preparation for adding consistent "%(trailers)" atom options to
> `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c
> to separate sub-arguments with a ",", instead of a ":".
> 
> Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
> "%(trailers:only,unfold)".
> 
> This change disambiguates between "top-level" arguments, and arguments given to
> the trailers atom itself. It is consistent with the behavior of "%(upstream)"
> and "%(push)" atoms.

Great. I hope we'll eventually unify the pretty.c and ref-filter.c
syntaxes and implementations, so consistency is good. I'm sure there
will be some hackery to keep backwards compatibility in all cases, but
the fewer the better.  The "every option gets its own colon" scheme was
just something I invented, because there was no prior art in pretty.c. I
didn't think to check for similar cases in ref-filter.c.

We should be safe to change this unconditionally, without worrying about
breaking compatibility. These options were added in 58311c66fd (pretty:
support normalization options for %(trailers), 2017-08-15), which hasn't
been in any released version yet.

> ---
>  pretty.c                      | 13 ++++++++-----
>  t/t4205-log-pretty-formats.sh |  4 ++--
>  2 files changed, 10 insertions(+), 7 deletions(-)

I think this needs an update to Documentation/pretty-formats.txt. Maybe:

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 6b4a12f028..40948d925b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -199,11 +199,13 @@ 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]. 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.
+- %(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.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f530102..977472f539 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&

> diff --git a/pretty.c b/pretty.c
> index 0e23fe3c0..68f736912 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1265,11 +1265,14 @@ 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;
> -		while (*arg == ':') {
> -			if (skip_prefix(arg, ":only", &arg))
> -				opts.only_trailers = 1;
> -			else if (skip_prefix(arg, ":unfold", &arg))
> -				opts.unfold = 1;
> +		if (skip_prefix(arg, ":", &arg)) {
> +			while (*arg != ')') {
> +				skip_prefix(arg, ",", &arg);
> +				if (skip_prefix(arg, "only", &arg))
> +					opts.only_trailers = 1;
> +				else if (skip_prefix(arg, "unfold", &arg))
> +					opts.unfold = 1;
> +			}
>  		}

Do we always make forward progress with this loop condition?

If arg doesn't start with a close-paren, we'll keep looping. But if it's
not a comma or one of the option strings, then skip_prefix() won't move
the pointer forward. So I think "%(trailers:foo)" would loop forever.

The original would complain about that and even almost-right things like
"%(trailers:only-the-lonely", though it's pretty subtle. The atom
parsers in ref-filter.c use string_list_split() which is way more
readable and obvious. But I don't think we can do that here. ref-filter
has a separate parsing step, so it can afford to be slow. Here we're
parsing the pretty format individually for each commit in "git log", and
allocations would be noticeable.

Here's what I came up with:

  static int match_placeholder_arg(const char *to_parse,
  				 const char *candidate,
  				 const char **end)
  {
  	const char *p;
  	if (!skip_prefix(to_parse, candidate, &p))
  		return 0;
  	if (*p == ',') {
  		*end = p + 1;
  		return 1;
  	}
  	if (*p == ')') {
  		*end = p;
  		return 1;
  	}
  	return 0;
  }

  ...

  	if (skip_prefix(placeholder, "(trailers", &arg)) {
  		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
  		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
  					break;
  			}
  		}
  		if (*arg == ')') {
  			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
  			return arg - placeholder + 1;
  		}
  	}

I hate the "for (;;)", but at least it's easy to see that each iteration
either makes progress or breaks out of the loop.

-Peff

[PATCH 2/5] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`, through
"%(contents:trailers)". In preparation for more, let's add a few things:

  - Move the commit creation step to its own test so that it can be re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers aren't
    shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded correctly
    using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..2a9fcf713 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
 cat >trailers <<EOF
 Reviewed-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
 	git add two &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	Some message contents
 
 	$(cat trailers)
 	EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
 	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
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH 2/5] t6300: refactor %(trailers) tests

[Jeff King]

On Fri, Sep 29, 2017 at 11:22:35PM -0700, Taylor Blau wrote:

> We currently have one test for %(trailers) in `git-for-each-ref(1)`, through
> "%(contents:trailers)". In preparation for more, let's add a few things:
> 
>   - Move the commit creation step to its own test so that it can be re-used.
> 
>   - Add a non-trailer to the commit's trailers to test that non-trailers aren't
>     shown using "%(trailers:only)".
> 
>   - Add a multi-line trailer to ensure that trailers are unfolded correctly
>     using "%(trailers:unfold)".

This is a minor nit, but since you invited formatting critique in your
cover letter, I feel entitled. :)

Consider wrapping your commit messages (and emails in general) at
72 characters, rather than 80. That lets them show well on an 80-column
display even when indented by "git log" or by inline quoting in an email
reply.

I'm also of the opinion that while 80 characters is fine for code, it's
a bit wide for English text. You can find various claims online[1] from
people interested in typography that a line width of about 60-70
characters is pleasant for reading.

[1] E.g., https://baymard.com/blog/line-length-readability

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  t/t6300-for-each-ref.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

The patch itself looks fine. :)

-Peff

[PATCH 3/5] ref-filter.c: add trailer options to used_atom

[Taylor Blau]

In preparation for "%(trailers)" to take trailer parsing arguments, use Jeff's
convenience structure for trailer processing options introduced in 8abc89800c.

We will later populate this field from the arguments given to %(trailers), and
then use the trailer_opts instance to format ref trailers correctly using
`format_trailer_from_commit`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ref-filter.c b/ref-filter.c
index 467c0279c..84f14093c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
 		struct {
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH 3/5] ref-filter.c: add trailer options to used_atom

[Jeff King]

On Fri, Sep 29, 2017 at 11:22:36PM -0700, Taylor Blau wrote:

> In preparation for "%(trailers)" to take trailer parsing arguments, use Jeff's
> convenience structure for trailer processing options introduced in 8abc89800c.
> 
> We will later populate this field from the arguments given to %(trailers), and
> then use the trailer_opts instance to format ref trailers correctly using
> `format_trailer_from_commit`.

I think this should probably just be squashed in with the next patch,
since this does nothing at all without adding a caller that uses it.

> diff --git a/ref-filter.c b/ref-filter.c
> index 467c0279c..84f14093c 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -82,6 +82,7 @@ static struct used_atom {
>  		} remote_ref;
>  		struct {
>  			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
> +			struct process_trailer_options trailer_opts;
>  			unsigned int nlines;
>  		} contents;

This contents struct is odd. The used_atom struct can have many types,
and has has a big union of type-specific data.  And then we break down
the "contents" type with a further enum, but don't actually put the
type-specific data into a union (not just your patch, but already
"nlines" is specific only to C_LINES).

It's probably not worth caring about, though. The point of a union is to
reduce the overall struct size, and here our type-specific data is
fairly small. It would only change the overall struct size if it were
larger than other parts of the union (and remote_ref, for example, is
pretty big).

-Peff

PS As an aside, I find the whole %(contents:...) thing a bit
   unfortunate. I understand why the _implementation_ wants to group
   similar options together so that it can avoid parsing too much. But
   the user doesn't care about that. Just "%(trailers)" should be
   sufficient (as evidenced by the fact that we added it a separate
   alias). But none of that is new to your series.

[PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

In preparation to support additional sub-arguments given to the "%(trailers)"
atom, use 'format_trailers_from_commit' in order to format trailers in the
desired manner.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  6 +++++-
 ref-filter.c                       | 31 ++++++++++++++++++++---------
 t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 03e187a10..b7325a25d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,7 +213,11 @@ 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'.
+are obtained as 'contents:trailers'. Non-trailer lines from the trailer block
+can be omitted with 'trailers:only'. Whitespace-continuations can be removed
+from trailers so that each trailer appears on a line by itself with its full
+content with 'trailers:unfold'. Both can be used together as
+'trailers:unfold,only'.
 
 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 84f14093c..8573acbed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -178,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
 
 static void trailers_atom_parser(struct used_atom *atom, const char *arg)
 {
-	if (arg)
-		die(_("%%(trailers) does not take arguments"));
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+
+	if (arg) {
+		string_list_split(&params, arg, ',', -1);
+		for (i = 0; i < params.nr; i++) {
+			const char *s = params.items[i].string;
+			if (!strcmp(s, "unfold"))
+				atom->u.contents.trailer_opts.unfold = 1;
+			else if (!strcmp(s, "only"))
+				atom->u.contents.trailer_opts.only_trailers = 1;
+			else
+				die(_("unknown %%(trailers) argument: %s"), s);
+		}
+	}
 	atom->u.contents.option = C_TRAILERS;
+	string_list_clear(&params, 0);
 }
 
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
@@ -1035,7 +1049,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, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -1060,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			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;
+			struct strbuf s = STRBUF_INIT;
 
-			/* 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);
+			/* Format the trailer info according to the trailer_opts given */
+			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+
+			v->s = strbuf_detach(&s, NULL);
 		} 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 2a9fcf713..2bd0c5da7 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,9 @@ Acked-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /'
+}
 
 test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
@@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' '
 	EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+  git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+  {
+    unfold <trailers
+    echo
+  } >expect &&
+  test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Jeff King]

On Fri, Sep 29, 2017 at 11:22:37PM -0700, Taylor Blau wrote:

> In preparation to support additional sub-arguments given to the "%(trailers)"
> atom, use 'format_trailers_from_commit' in order to format trailers in the
> desired manner.

This isn't just in preparation, is it? It looks like the options are
here (which I think is fine, but the commit message probably needs
updated).

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/git-for-each-ref.txt |  6 +++++-
>  ref-filter.c                       | 31 ++++++++++++++++++++---------
>  t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+), 10 deletions(-)

This patch didn't apply for me on top of the others. I get:

  Applying: ref-filter.c: use trailer_opts to format trailers
  error: patch failed: ref-filter.c:178
  error: ref-filter.c: patch does not apply
  Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers

And then with "am -3":

  Applying: ref-filter.c: use trailer_opts to format trailers
  error: sha1 information is lacking or useless (ref-filter.c).
  error: could not build fake ancestor
  Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers

Did it get corrupted in transit, or did you hand-edit it?

> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 03e187a10..b7325a25d 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -213,7 +213,11 @@ 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'.
> +are obtained as 'contents:trailers'. Non-trailer lines from the trailer block
> +can be omitted with 'trailers:only'. Whitespace-continuations can be removed
> +from trailers so that each trailer appears on a line by itself with its full
> +content with 'trailers:unfold'. Both can be used together as
> +'trailers:unfold,only'.

I know you copied the single-quote formatting from the existing line,
but this may be a good opportunity to switch to backticks, which is what
we usually prefer these days for literal phrases.

> diff --git a/ref-filter.c b/ref-filter.c
> index 84f14093c..8573acbed 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -178,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
>  
>  static void trailers_atom_parser(struct used_atom *atom, const char *arg)
>  {
> -	if (arg)
> -		die(_("%%(trailers) does not take arguments"));
> +	struct string_list params = STRING_LIST_INIT_DUP;
> +	int i;
> +
> +	if (arg) {
> +		string_list_split(&params, arg, ',', -1);
> +		for (i = 0; i < params.nr; i++) {
> +			const char *s = params.items[i].string;
> +			if (!strcmp(s, "unfold"))
> +				atom->u.contents.trailer_opts.unfold = 1;
> +			else if (!strcmp(s, "only"))
> +				atom->u.contents.trailer_opts.only_trailers = 1;
> +			else
> +				die(_("unknown %%(trailers) argument: %s"), s);
> +		}
> +	}
>  	atom->u.contents.option = C_TRAILERS;
> +	string_list_clear(&params, 0);
>  }

This looks good (and so much nicer than the contortions from pretty.c).
The error behavior matches what we currently do for %(align), which
makes sense.

The trailer_opts should be zero-initialized to start with due to us
calling memset on the whole used_atom struct.

>  static void contents_atom_parser(struct used_atom *atom, const char *arg)
> @@ -1035,7 +1049,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, "trailers") &&
>  		    !starts_with(name, "contents"))
>  			continue;
>  		if (!subpos)
> @@ -1060,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>  			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;
> +			struct strbuf s = STRBUF_INIT;
>  
> -			/* 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);
> +			/* Format the trailer info according to the trailer_opts given */
> +			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
> +
> +			v->s = strbuf_detach(&s, NULL);

And this all looks straightforward and correct.

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2a9fcf713..2bd0c5da7 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh

The tests are basically an adaptation of the ones from 58311c66fd
(pretty: support normalization options for %(trailers), 2017-08-15),
which make sense.

One thing I did notice:

> @@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' '
>  	EOF
>  '
>  
> +test_expect_success '%(trailers:unfold) unfolds trailers' '
> +  git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
> +  {
> +    unfold <trailers
> +    echo
> +  } >expect &&
> +  test_cmp expect actual
> +'

This looks like two-space indentation, when it should be a tab.

> +test_expect_success '%(trailers:only) shows only "key: value" trailers' '
> +	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
> +	{
> +		grep -v patch.description <trailers &&
> +		echo
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
> +	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> +	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> +	test_cmp actual reverse &&
> +	{
> +		grep -v patch.description <trailers | unfold &&
> +		echo
> +	} >expect &&
> +	test_cmp expect actual
> +'

These ones are tabs. GOod.


> +test_expect_success '%(trailers) rejects unknown trailers arguments' '
> +	cat >expect <<-EOF &&
> +	fatal: unknown %(trailers) argument: unsupported
> +	EOF
> +  test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> +  test_cmp expect actual
> +'

But this one is mixed. :)

-Peff

Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This patch didn't apply for me on top of the others. I get:
>
>   Applying: ref-filter.c: use trailer_opts to format trailers
>   error: patch failed: ref-filter.c:178
>   error: ref-filter.c: patch does not apply
>   Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers
>
> And then with "am -3":
>
>   Applying: ref-filter.c: use trailer_opts to format trailers
>   error: sha1 information is lacking or useless (ref-filter.c).
>   error: could not build fake ancestor
>   Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers
>
> Did it get corrupted in transit, or did you hand-edit it?

And the corresponding patch in v3 seems to have the same issue.

Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2a9fcf713..2bd0c5da7 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -597,6 +597,9 @@ Acked-by: A U Thor
>    <author@example.com>
>  EOF
>  
> +unfold () {
> +	perl -0pe 's/\n\s+/ /'
> +}

For the purpose of the current shape of the test, the above might be
sufficient, but the lack of "/g" at the end means that the script
will happily stop after unfolding just one line, which probably is
not what you intended.

Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Jeff King]

On Sun, Oct 01, 2017 at 06:00:25PM +0900, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> > index 2a9fcf713..2bd0c5da7 100755
> > --- a/t/t6300-for-each-ref.sh
> > +++ b/t/t6300-for-each-ref.sh
> > @@ -597,6 +597,9 @@ Acked-by: A U Thor
> >    <author@example.com>
> >  EOF
> >  
> > +unfold () {
> > +	perl -0pe 's/\n\s+/ /'
> > +}
> 
> For the purpose of the current shape of the test, the above might be
> sufficient, but the lack of "/g" at the end means that the script
> will happily stop after unfolding just one line, which probably is
> not what you intended.

This is indirectly my fault, since this was copied from my t4205
version. It might be worth fixing while we're thinking about it, as it's
a potential trap for future changes.

-Peff

Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

On Mon, Oct 02, 2017 at 01:05:07AM -0400, Jeff King wrote:
> On Sun, Oct 01, 2017 at 06:00:25PM +0900, Junio C Hamano wrote:
>
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> > > index 2a9fcf713..2bd0c5da7 100755
> > > --- a/t/t6300-for-each-ref.sh
> > > +++ b/t/t6300-for-each-ref.sh
> > > @@ -597,6 +597,9 @@ Acked-by: A U Thor
> > >    <author@example.com>
> > >  EOF
> > >
> > > +unfold () {
> > > +	perl -0pe 's/\n\s+/ /'
> > > +}
> >
> > For the purpose of the current shape of the test, the above might be
> > sufficient, but the lack of "/g" at the end means that the script
> > will happily stop after unfolding just one line, which probably is
> > not what you intended.
>
> This is indirectly my fault, since this was copied from my t4205
> version. It might be worth fixing while we're thinking about it, as it's
> a potential trap for future changes.

I agree. I'll include a fix for this as an additional commit in v6.

--
- Taylor

[PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since "trailers"
is one of those fields, extend contents_atom_parser to parse "trailers"'s
arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given (see:
`parse_ref_filter_atom`). To simulate this behavior without teaching
trailers_atom_parser to accept strings with length zero, conditionally pass
NULL to trailers_atom_parser if the arguments portion of the argument to
%(contents) is empty.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  9 +++++----
 ref-filter.c                       |  6 ++++--
 t/t6300-for-each-ref.sh            | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b7325a25d..0aaac8af9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,10 +214,11 @@ 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'. Non-trailer lines from the trailer block
-can be omitted with 'trailers:only'. Whitespace-continuations can be removed
-from trailers so that each trailer appears on a line by itself with its full
-content with 'trailers:unfold'. Both can be used together as
-'trailers:unfold,only'.
+can be omitted with 'trailers:only', or 'contents:trailers:only'.
+Whitespace-continuations can be removed from trailers so that each trailer
+appears on a line by itself with its full content with 'trailers:unfold' or
+'contents:trailers:unfold'. Both can be used together as 'trailers:unfold,only',
+or 'contents:trailers:unfold,only'.
 
 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 8573acbed..a8d4a52bd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -207,8 +207,10 @@ 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, "trailers", &arg)) {
+		skip_prefix(arg, ":", &arg);
+		trailers_atom_parser(atom, strlen(arg) ? arg : NULL);
+	}
 	else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2bd0c5da7..d9b71589f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+  git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
+  {
+    unfold <trailers
+    echo
+  } >expect &&
+  test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
@@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom

[Jeff King]

On Fri, Sep 29, 2017 at 11:22:38PM -0700, Taylor Blau wrote:

> The %(contents) atom takes a contents "field" as its argument. Since "trailers"
> is one of those fields, extend contents_atom_parser to parse "trailers"'s
> arguments when used through "%(contents)", like:
> 
>   %(contents:trailers:unfold,only)
> 
> A caveat: trailers_atom_parser expects NULL when no arguments are given (see:
> `parse_ref_filter_atom`). To simulate this behavior without teaching
> trailers_atom_parser to accept strings with length zero, conditionally pass
> NULL to trailers_atom_parser if the arguments portion of the argument to
> %(contents) is empty.

Yeah, this is a weird effect of trailers_atom_parser() doing double-duty
to parse both "%(contents:trailers)" and "%(trailers)".

Though I think trailers_atom_parser() does do the sensible thing with an
empty string (there are no options, so nothing to parse). I.e., I'd
expect the same thing out of:

  %(trailers:)

and

  %(trailers)

even though one gets a NULL "arg" field and the other gets an empty
string.

> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index b7325a25d..0aaac8af9 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -214,10 +214,11 @@ 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'. Non-trailer lines from the trailer block
> -can be omitted with 'trailers:only'. Whitespace-continuations can be removed
> -from trailers so that each trailer appears on a line by itself with its full
> -content with 'trailers:unfold'. Both can be used together as
> -'trailers:unfold,only'.
> +can be omitted with 'trailers:only', or 'contents:trailers:only'.
> +Whitespace-continuations can be removed from trailers so that each trailer
> +appears on a line by itself with its full content with 'trailers:unfold' or
> +'contents:trailers:unfold'. Both can be used together as 'trailers:unfold,only',
> +or 'contents:trailers:unfold,only'.

Rather than enumerate each, we might do better to just say explicitly
"contents:trailers" and "trailers" are aliases of one another. It looks
like we don't even document %(trailers) at all here. I'd actually be in
favor of just declaring %(trailers) the official spelling, and calling
"%(contents:trailers)" a historical alias.

> diff --git a/ref-filter.c b/ref-filter.c
> index 8573acbed..a8d4a52bd 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -207,8 +207,10 @@ 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, "trailers", &arg)) {
> +		skip_prefix(arg, ":", &arg);
> +		trailers_atom_parser(atom, strlen(arg) ? arg : NULL);
> +	}

We usually spell "is this an empty string?" as "*arg" rather than
calling strlen().

However, I'm not sure we need to check. As I said above, I think
trailers_atom_parser() does the sensible thing with an empty string.

And if we _did_ want to distinguish between

  %(contents:trailers:)

and

  %(contents:trailers)

then I don't think this quite does it. It passes NULL for both cases. So
if we really want to emulate how parse_ref_filter_atom calls it, we'd
want:

  if (!skip_prefix(arg, ":", &arg))
	arg = NULL;
  trailers_atom_parser(atom, arg);

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 2bd0c5da7..d9b71589f 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
> +  git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
> +  {

This has the same spaces/tabs thing going on as the previous commit.

-Peff

Re: [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1)

[Jeff King]

On Fri, Sep 29, 2017 at 11:22:33PM -0700, Taylor Blau wrote:

> Attached is a patch to extend Peff's recent work of adding parsing options to
> "--pretty=%(trailers)" by supporting those same options in git-for-each-ref(1).

Thanks for working on this. The direction and general sketch of the
implementation look right to me. I noted a few problems inline, so this
isn't quite right to be picked up, but I think a v2 should get us pretty
close, if not all the way there.

-Peff

[PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is v2 of my patch-set "Support %(trailers) arguments in
for-each-ref(1)".

It includes the following changes since v1:

  * Accept "empty-sub-argument" lists, like %(contents:trailers:).

  * Fix incorrect tabs/spaces formatting in t6300.

  * Modern-ize code fencing in Documentation/git-for-each-ref.txt

  * Squash commit adding trailer_opts to used_atom structure.

---

  [1/6]: pretty.c: delimit "%(trailers)" arguments with ","
  [2/6]: t6300: refactor %(trailers) tests
  [3/6]: doc: 'trailers' is the preferred way to format trailers
  [4/6]: doc: use modern "`"-style code fencing
  [5/6]: ref-filter.c: use trailer_opts to format trailers
  [6/6]: ref-filter.c: parse trailers arguments with %(contents) atom

Thanks in advance :-).

--
- Taylor

[PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pretty.c                      | 34 +++++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0e23fe3c0..eeb51746c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1036,6 +1036,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)
+{
+	const char *p;
+	if (!(skip_prefix(to_parse, candidate, &p)))
+		return 0;
+	if (*p == ',') {
+		*end = p + 1;
+		return 1;
+	}
+	if (*p == ')') {
+		*end = p;
+		return 1;
+	}
+	return 0;
+}
+
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1265,11 +1284,16 @@ 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;
-		while (*arg == ':') {
-			if (skip_prefix(arg, ":only", &arg))
-				opts.only_trailers = 1;
-			else if (skip_prefix(arg, ":unfold", &arg))
-				opts.unfold = 1;
+		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
+					break;
+			}
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..2a9fcf713 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
 cat >trailers <<EOF
 Reviewed-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
 	git add two &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	Some message contents
 
 	$(cat trailers)
 	EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
 	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
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 03e187a10..6b38d9a22 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,7 +213,8 @@ 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'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

"'"- (single-quote) styled code fencing is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code fencing to
use "`"-style fencing instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6b38d9a22..b6492820b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -209,12 +209,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 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
+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 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 ref-filter.c                       | 32 +++++++++++++++++++++---------
 t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b6492820b..a1d964182 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,7 +214,10 @@ 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 `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 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 467c0279c..8573acbed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
 		struct {
@@ -177,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
 
 static void trailers_atom_parser(struct used_atom *atom, const char *arg)
 {
-	if (arg)
-		die(_("%%(trailers) does not take arguments"));
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+
+	if (arg) {
+		string_list_split(&params, arg, ',', -1);
+		for (i = 0; i < params.nr; i++) {
+			const char *s = params.items[i].string;
+			if (!strcmp(s, "unfold"))
+				atom->u.contents.trailer_opts.unfold = 1;
+			else if (!strcmp(s, "only"))
+				atom->u.contents.trailer_opts.only_trailers = 1;
+			else
+				die(_("unknown %%(trailers) argument: %s"), s);
+		}
+	}
 	atom->u.contents.option = C_TRAILERS;
+	string_list_clear(&params, 0);
 }
 
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
@@ -1034,7 +1049,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, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -1059,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			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;
+			struct strbuf s = STRBUF_INIT;
 
-			/* 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);
+			/* Format the trailer info according to the trailer_opts given */
+			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+
+			v->s = strbuf_detach(&s, NULL);
 		} 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 2a9fcf713..6538e7b90 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,9 @@ Acked-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /'
+}
 
 test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
@@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' '
 	EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v2 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). To simulate this behavior without
teaching trailers_atom_parser to accept strings with length zero,
conditionally pass NULL to trailers_atom_parser if the arguments portion
of the argument to %(contents) is empty.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c            |  6 ++++--
 t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8573acbed..f5bde79f3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -207,8 +207,10 @@ 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, "trailers", &arg)) {
+		skip_prefix(arg, ":", &arg);
+		trailers_atom_parser(atom, arg);
+	}
 	else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6538e7b90..8c960ec24 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
@@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is v3 of my patch-set "Support %(trailers) arguments in
for-each-ref(1)".

It includes all of the changes in V2:
>   * Accept "empty-sub-argument" lists, like %(contents:trailers:).
>
>   * Fix incorrect tabs/spaces formatting in t6300.
>
>   * Modern-ize code fencing in Documentation/git-for-each-ref.txt
>
>   * Squash commit adding trailer_opts to used_atom structure.

As well as fixing:

  * Incorrect indentation in pretty.c (specifically: "pretty.c: delimit
    "%(trailers)" arguments with ","")

  * Issues with --format="%(contents:trailers:)", where
    trailers_atom_parser did not correctly handle empty string ("") and
    NULL's the same way.

Thanks in advance :-).

--
- Taylor

[PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pretty.c                      | 34 +++++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0e23fe3c0..9f8d75a84 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1036,6 +1036,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)
+{
+	const char *p;
+	if (!(skip_prefix(to_parse, candidate, &p)))
+		return 0;
+	if (*p == ',') {
+		*end = p + 1;
+		return 1;
+	}
+	if (*p == ')') {
+		*end = p;
+		return 1;
+	}
+	return 0;
+}
+
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1265,11 +1284,16 @@ 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;
-		while (*arg == ':') {
-			if (skip_prefix(arg, ":only", &arg))
-				opts.only_trailers = 1;
-			else if (skip_prefix(arg, ":unfold", &arg))
-				opts.unfold = 1;
+		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
+					break;
+			}
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 834a9ed16..2a9fcf713 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
 cat >trailers <<EOF
 Reviewed-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
 	git add two &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	Some message contents
 
 	$(cat trailers)
 	EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
 	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
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 03e187a10..6b38d9a22 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,7 +213,8 @@ 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'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

"'"- (single-quote) styled code fencing is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code fencing to
use "`"-style fencing instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6b38d9a22..b6492820b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -209,12 +209,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 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
+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 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 ref-filter.c                       | 32 +++++++++++++++++++++---------
 t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b6492820b..a1d964182 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,7 +214,10 @@ 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 `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 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 467c0279c..8573acbed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
 		struct {
@@ -177,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
 
 static void trailers_atom_parser(struct used_atom *atom, const char *arg)
 {
-	if (arg)
-		die(_("%%(trailers) does not take arguments"));
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+
+	if (arg) {
+		string_list_split(&params, arg, ',', -1);
+		for (i = 0; i < params.nr; i++) {
+			const char *s = params.items[i].string;
+			if (!strcmp(s, "unfold"))
+				atom->u.contents.trailer_opts.unfold = 1;
+			else if (!strcmp(s, "only"))
+				atom->u.contents.trailer_opts.only_trailers = 1;
+			else
+				die(_("unknown %%(trailers) argument: %s"), s);
+		}
+	}
 	atom->u.contents.option = C_TRAILERS;
+	string_list_clear(&params, 0);
 }
 
 static void contents_atom_parser(struct used_atom *atom, const char *arg)
@@ -1034,7 +1049,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, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -1059,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			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;
+			struct strbuf s = STRBUF_INIT;
 
-			/* 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);
+			/* Format the trailer info according to the trailer_opts given */
+			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+
+			v->s = strbuf_detach(&s, NULL);
 		} 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 2a9fcf713..6538e7b90 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,9 @@ Acked-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /'
+}
 
 test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
@@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' '
 	EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v3 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). To simulate this behavior without
teaching trailers_atom_parser to accept strings with length zero,
conditionally pass NULL to trailers_atom_parser if the arguments portion
of the argument to %(contents) is empty.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c            |  6 ++++--
 t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8573acbed..b5747e969 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -207,8 +207,10 @@ 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, "trailers", &arg)) {
+		skip_prefix(arg, ":", &arg);
+		trailers_atom_parser(atom, *arg ? NULL : arg);
+	}
 	else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6538e7b90..8c960ec24 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
@@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is the fourth revision of my patch-set "Support %(trailers)
arguments in for-each-ref(1)".

It includes the following changes since v3:

  * Teach unfold() to unfold multiple lines.

  * Rebase patches to apply on top of master.

Thanks in advance, and thanks for all of your help thus far :-).


- Taylor

[PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pretty.c                      | 34 +++++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 94eab5c89..eec128bc1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,6 +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)
+{
+	const char *p;
+	if (!(skip_prefix(to_parse, candidate, &p)))
+		return 0;
+	if (*p == ',') {
+		*end = p + 1;
+		return 1;
+	}
+	if (*p == ')') {
+		*end = p;
+		return 1;
+	}
+	return 0;
+}
+
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1285,11 +1304,16 @@ 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;
-		while (*arg == ':') {
-			if (skip_prefix(arg, ":only", &arg))
-				opts.only_trailers = 1;
-			else if (skip_prefix(arg, ":unfold", &arg))
-				opts.unfold = 1;
+		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
+					break;
+			}
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v4 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..39431908d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
 cat >trailers <<EOF
 Reviewed-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
 	git add two &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	Some message contents
 
 	$(cat trailers)
 	EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
 	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
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v4 2/6] t6300: refactor %(trailers) tests

[Junio C Hamano]

Looks good to me, thanks.

[PATCH v4 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 66b4e0a40..323ce07de 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -217,7 +217,8 @@ 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'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v4 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

"'"- (single-quote) styled code fencing is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code fencing to
use "`"-style fencing instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 323ce07de..1279b9733 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,12 +213,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 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
+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 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> "'"- (single-quote) styled code fencing is no longer considered modern
> within the "Documentation/" subtree.
>
> In preparation for adding additional information to this section of
> git-for-each-ref(1)'s documentation, update old-style code fencing to
> use "`"-style fencing instead.

Is this just me who wants to do s/fenc/quot/g?  Unless somebody
objects, I'd do so while queuing.

Thanks.

>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/git-for-each-ref.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 323ce07de..1279b9733 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -213,12 +213,12 @@ and `date` to extract the named component.
>  The complete message in a commit and tag object is `contents`.
>  Its first line is `contents:subject`, where subject is the concatenation
>  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
> +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 'trailers' (or by using the historical alias
> -'contents:trailers').
> +are obtained as `trailers` (or by using the historical alias
> +`contents:trailers`).
>  
>  For sorting purposes, fields with numeric values sort in numeric order
>  (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).

Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing

[Taylor Blau]

On Mon, Oct 02, 2017 at 08:55:53AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > "'"- (single-quote) styled code fencing is no longer considered modern
> > within the "Documentation/" subtree.
> >
> > In preparation for adding additional information to this section of
> > git-for-each-ref(1)'s documentation, update old-style code fencing to
> > use "`"-style fencing instead.
>
> Is this just me who wants to do s/fenc/quot/g?  Unless somebody
> objects, I'd do so while queuing.

I don't object, I think that fencing is less appropriate than quoting.
I couldn't find the term myself when writing this commit :-).

I am happy to send out v5 of this patch series with this commit
re-written, or you can change it while queuing. Whichever is easier for
you.

Thanks again.


--
- Taylor

Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> Is this just me who wants to do s/fenc/quot/g?  Unless somebody
>> objects, I'd do so while queuing.
>
> I don't object, I think that fencing is less appropriate than quoting.
> I couldn't find the term myself when writing this commit :-).
>
> I am happy to send out v5 of this patch series with this commit
> re-written, or you can change it while queuing. Whichever is easier for
> you.

Just FYI, here is what I ended up with.  I do not think this is
about "modern" vs "old style"; it is more about using the more
appropriate mark-up and our desire has always been to use `literal`
for things that users need to type literally.

I notice that I didn't remove 'modern' from the title in the versoin
below, though.

Thanks.

commit a184d836bcf1a557520095a90b22edf659ee88f2
Author: Taylor Blau <me@ttaylorr.com>
Date:   Sun Oct 1 09:18:50 2017 -0700

    doc: use modern "`<literal>`"-style quoting for literal strings
    
    "'<string>'"-style quoting is not appropriate when quoting literal
    strings in the "Documentation/" subtree.
    
    In preparation for adding additional information to this section of
    git-for-each-ref(1)'s documentation, update them to use "`<literal>`"
    instead.
    
    Signed-off-by: Taylor Blau <me@ttaylorr.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing

[Jeff King]

On Mon, Oct 02, 2017 at 10:35:45AM +0900, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> >> Is this just me who wants to do s/fenc/quot/g?  Unless somebody
> >> objects, I'd do so while queuing.
> >
> > I don't object, I think that fencing is less appropriate than quoting.
> > I couldn't find the term myself when writing this commit :-).
> >
> > I am happy to send out v5 of this patch series with this commit
> > re-written, or you can change it while queuing. Whichever is easier for
> > you.
> 
> Just FYI, here is what I ended up with.  I do not think this is
> about "modern" vs "old style"; it is more about using the more
> appropriate mark-up and our desire has always been to use `literal`
> for things that users need to type literally.

I think it was my earlier comment that led to the modern/old notion. A
lot of the old code does use single-quotes haphazardly. I'm not sure if
it's "old style" or just "we were less careful" then. Either way, the
patch looks good to me.

I also think fencing isn't the right word; that word implies to me a
multi-line block like

---------------------------
this is fenced
---------------------------

-Peff

[PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 ref-filter.c                       | 32 +++++++++++++++++++++---------
 t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 1279b9733..4a2c851e6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -218,7 +218,10 @@ 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 `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 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 bc591f4f3..43ed10a5e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
 		struct {
@@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format *format, struct used_ato
 
 static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
-	if (arg)
-		die(_("%%(trailers) does not take arguments"));
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+
+	if (arg) {
+		string_list_split(&params, arg, ',', -1);
+		for (i = 0; i < params.nr; i++) {
+			const char *s = params.items[i].string;
+			if (!strcmp(s, "unfold"))
+				atom->u.contents.trailer_opts.unfold = 1;
+			else if (!strcmp(s, "only"))
+				atom->u.contents.trailer_opts.only_trailers = 1;
+			else
+				die(_("unknown %%(trailers) argument: %s"), s);
+		}
+	}
 	atom->u.contents.option = C_TRAILERS;
+	string_list_clear(&params, 0);
 }
 
 static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
@@ -1042,7 +1057,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, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			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;
+			struct strbuf s = STRBUF_INIT;
 
-			/* 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);
+			/* Format the trailer info according to the trailer_opts given */
+			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+
+			v->s = strbuf_detach(&s, NULL);
 		} 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 39431908d..e4ce9c550 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -610,6 +610,9 @@ Acked-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /g'
+}
 
 test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
@@ -623,6 +626,43 @@ test_expect_success 'set up trailers for next test' '
 	EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> +test_expect_success '%(trailers) rejects unknown trailers arguments' '
> +	cat >expect <<-EOF &&
> +	fatal: unknown %(trailers) argument: unsupported
> +	EOF
> +  test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> +  test_cmp expect actual

I'll have to remember to fix these two lines up before queuing.
Please yell at me if the version I'll push out later forgets to do
so.


Thanks.

[PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). To simulate this behavior without
teaching trailers_atom_parser to accept strings with length zero,
conditionally pass NULL to trailers_atom_parser if the arguments portion
of the argument to %(contents) is empty.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c            |  6 ++++--
 t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 43ed10a5e..2c03c2bf5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -212,8 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
 		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, "trailers", &arg)) {
+		skip_prefix(arg, ":", &arg);
+		trailers_atom_parser(atom, *arg ? NULL : arg);
+	}
 	else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index e4ce9c550..ab95fe427 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
@@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
   test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
+  test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> The %(contents) atom takes a contents "field" as its argument. Since
> "trailers" is one of those fields, extend contents_atom_parser to parse
> "trailers"'s arguments when used through "%(contents)", like:
>
>   %(contents:trailers:unfold,only)
>
> A caveat: trailers_atom_parser expects NULL when no arguments are given
> (see: `parse_ref_filter_atom`). To simulate this behavior without
> teaching trailers_atom_parser to accept strings with length zero,
> conditionally pass NULL to trailers_atom_parser if the arguments portion
> of the argument to %(contents) is empty.

I got an impression during the earlier rounds' review discussion
that trailers_atom_parser() will happily accept an empty string and
work correctly, so this paragraph and the conditional *arg ? NULL :
arg were both unneeded.

 - arg == "" is not NULL, so we first do string_list_split() on ','
 - which yields an empty list i.e. params.nr == 0
 - we do not loop and leave atom->u.contents.trailer_opts untouched.
 - and we set u.contents.option to C_TRAILERS
 - and we clear &params string list before we leave.

which is exactly the same as what happens when arg == NULL.  Sooo....

>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  ref-filter.c            |  6 ++++--
>  t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 43ed10a5e..2c03c2bf5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -212,8 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
>  		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, "trailers", &arg)) {
> +		skip_prefix(arg, ":", &arg);
> +		trailers_atom_parser(atom, *arg ? NULL : arg);
> +	}
>  	else if (skip_prefix(arg, "lines=", &arg)) {

Merge these two lines i.e.

	} else if (skip_prefix(arg, "lines=", ...) {

> +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> +	cat >expect <<-EOF &&
> +	fatal: unknown %(trailers) argument: unsupported
> +	EOF
> +  test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
> +  test_cmp expect actual

Funny indentation.

Re: [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> In preparation for adding consistent "%(trailers)" atom options to
> `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
> pretty.c to separate sub-arguments with a ",", instead of a ":".
>
> Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
> "%(trailers:only,unfold)".
>
> This change disambiguates between "top-level" arguments, and arguments
> given to the trailers atom itself. It is consistent with the behavior of
> "%(upstream)" and "%(push)" atoms.

Looks good.  

Ignore the remainder unless you are interested in the recent "make
style" discussion.

>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pretty.c                      | 34 +++++++++++++++++++++++++++++-----
>  t/t4205-log-pretty-formats.sh |  4 ++--
>  2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 94eab5c89..eec128bc1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1056,6 +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)

"make style" wants to format this like so:

static int match_placeholder_arg(const char *to_parse, const char *candidate,
				 const char **end)

I think I can live with either versions ;-)

> +{
> +	const char *p;
> +	if (!(skip_prefix(to_parse, candidate, &p)))
> +		return 0;
> +	if (*p == ',') {
> +		*end = p + 1;
> +		return 1;
> +	}
> +	if (*p == ')') {
> +		*end = p;
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +

"make style" seems to be unhappy to see double blank here, and I
tend to agree.

>  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  				const char *placeholder,
>  				void *context)
> @@ -1285,11 +1304,16 @@ 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;
> -		while (*arg == ':') {
> -			if (skip_prefix(arg, ":only", &arg))
> -				opts.only_trailers = 1;
> -			else if (skip_prefix(arg, ":unfold", &arg))
> -				opts.unfold = 1;
> +		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
> +					break;
> +			}
>  		}
>  		if (*arg == ')') {
>  			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index ec5f53010..977472f53 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
>  '
>  
>  test_expect_success ':only and :unfold work together' '
> -	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
> -	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
> +	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
> +	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
>  	test_cmp actual reverse &&
>  	{
>  		grep -v patch.description <trailers | unfold &&

Re: [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Jeff King]

On Mon, Oct 02, 2017 at 09:11:50AM +0900, Junio C Hamano wrote:

> > diff --git a/pretty.c b/pretty.c
> > index 94eab5c89..eec128bc1 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -1056,6 +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)
> 
> "make style" wants to format this like so:
> 
> static int match_placeholder_arg(const char *to_parse, const char *candidate,
> 				 const char **end)
> 
> I think I can live with either versions ;-)

I can live with either, too, though I do prefer the 3-line form (which
is unsurprising since it comes from what I wrote in an earlier thread).
I don't think it conveys a huge amount of meaning here, but there are
definitely times when I would tweak the wrapping of long argument lists
to convey meaning. E.g.:

  void some_function(struct foo *foo,
                     const char *buf, size_t len,
		     unsigned flags);

rather than

  void some_function(struct foo *foo, const char *buf,
                     size_t len, unsigned flags);

It's a little sad to be beholden to a tool that would mindlessly drop
that grouping. But at the same time, that tool may be the lesser evil if
it saves time adjusting (and reviewing) style. I'll continue to observe
before forming more inclusions. Thanks for reporting on your "make
style" experiments.

-Peff

Re: [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is the fifth revision of my patch-set "Support %(trailers)
arguments in for-each-ref(1)".

It includes the following changes since v4:

  * Clarified "ref-filter.c: parse trailers arguments with %(contents)
    atom" to include reasoning for passing NULL as "" empty string in
    contents_atom_parser.

  * Changed instances "fencing" to "quoting" in "doc: use modern
    "`"-style code fencing".

  * Changed indentation in "pretty.c: delimit "%(trailers)" arguments
    with ","" to silence the output from `make style`.

  * Fix incorrect indentation in "ref-filter.c: use trailer_opts to
    format trailers".

Thanks again.


- Taylor

[PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pretty.c                      | 32 +++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 94eab5c89..7500fe694 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,6 +1056,23 @@ 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)
+{
+	const char *p;
+	if (!(skip_prefix(to_parse, candidate, &p)))
+		return 0;
+	if (*p == ',') {
+		*end = p + 1;
+		return 1;
+	}
+	if (*p == ')') {
+		*end = p;
+		return 1;
+	}
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1285,11 +1302,16 @@ 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;
-		while (*arg == ':') {
-			if (skip_prefix(arg, ":only", &arg))
-				opts.only_trailers = 1;
-			else if (skip_prefix(arg, ":unfold", &arg))
-				opts.unfold = 1;
+		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
+					break;
+			}
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 2/6] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..39431908d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
 cat >trailers <<EOF
 Reviewed-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
 	git add two &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	Some message contents
 
 	$(cat trailers)
 	EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
 	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
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 3/6] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 66b4e0a40..323ce07de 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -217,7 +217,8 @@ 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'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 4/6] doc: use modern "`"-style code quoting

[Taylor Blau]

"'"- (single-quote) styled code quoting is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code quoting to
use "`"-style quoting instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 323ce07de..1279b9733 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,12 +213,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 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
+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 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 5/6] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 ref-filter.c                       | 32 +++++++++++++++++++++---------
 t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 1279b9733..4a2c851e6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -218,7 +218,10 @@ 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 `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 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 bc591f4f3..43ed10a5e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
 		struct {
@@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format *format, struct used_ato
 
 static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
-	if (arg)
-		die(_("%%(trailers) does not take arguments"));
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+
+	if (arg) {
+		string_list_split(&params, arg, ',', -1);
+		for (i = 0; i < params.nr; i++) {
+			const char *s = params.items[i].string;
+			if (!strcmp(s, "unfold"))
+				atom->u.contents.trailer_opts.unfold = 1;
+			else if (!strcmp(s, "only"))
+				atom->u.contents.trailer_opts.only_trailers = 1;
+			else
+				die(_("unknown %%(trailers) argument: %s"), s);
+		}
+	}
 	atom->u.contents.option = C_TRAILERS;
+	string_list_clear(&params, 0);
 }
 
 static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
@@ -1042,7 +1057,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, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			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;
+			struct strbuf s = STRBUF_INIT;
 
-			/* 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);
+			/* Format the trailer info according to the trailer_opts given */
+			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+
+			v->s = strbuf_detach(&s, NULL);
 		} 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 39431908d..54e52d4e9 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -610,6 +610,9 @@ Acked-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /g'
+}
 
 test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
@@ -623,6 +626,43 @@ test_expect_success 'set up trailers for next test' '
 	EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). This is because string_list_split (given
a maxsplit of -1) returns a 1-ary string_list* containing the given
string if the delimiter could not be found using `strchr`.

To simulate this behavior without teaching trailers_atom_parser to
accept strings with length zero, conditionally pass NULL to
trailers_atom_parser if the arguments portion of the argument to
%(contents) is empty.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c            |  7 ++++---
 t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 43ed10a5e..5b64de6ea 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
 		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)) {
+	else if (skip_prefix(arg, "trailers", &arg)) {
+		skip_prefix(arg, ":", &arg);
+		trailers_atom_parser(atom, *arg ? NULL : arg);
+	} else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
 			die(_("positive value expected contents:lines=%s"), arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 54e52d4e9..872973b95 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
@@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> A caveat: trailers_atom_parser expects NULL when no arguments are given
> (see: `parse_ref_filter_atom`). This is because string_list_split (given
> a maxsplit of -1) returns a 1-ary string_list* containing the given
> string if the delimiter could not be found using `strchr`.

OK.  That's unfortunate, but the solution here is far cleaner and
simpler than fixing string-list-split.  Thanks for an updated
explanation here.

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
>  		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)) {
> +	else if (skip_prefix(arg, "trailers", &arg)) {
> +		skip_prefix(arg, ":", &arg);
> +		trailers_atom_parser(atom, *arg ? NULL : arg);

A parameter for the call is missing.  I think you want 'format'
there.

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

On Mon, Oct 02, 2017 at 01:51:11PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
> >  		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)) {
> > +	else if (skip_prefix(arg, "trailers", &arg)) {
> > +		skip_prefix(arg, ":", &arg);
> > +		trailers_atom_parser(atom, *arg ? NULL : arg);
>
> A parameter for the call is missing.  I think you want 'format'
> there.

Thanks for pointing this out. I have fixed this in v6, which I am
sending out shortly. I think that this revision should be ready for
queueing.

--
- Taylor

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Jeff King]

On Sun, Oct 01, 2017 at 05:33:04PM -0700, Taylor Blau wrote:

> The %(contents) atom takes a contents "field" as its argument. Since
> "trailers" is one of those fields, extend contents_atom_parser to parse
> "trailers"'s arguments when used through "%(contents)", like:
> 
>   %(contents:trailers:unfold,only)
> 
> A caveat: trailers_atom_parser expects NULL when no arguments are given
> (see: `parse_ref_filter_atom`). This is because string_list_split (given
> a maxsplit of -1) returns a 1-ary string_list* containing the given
> string if the delimiter could not be found using `strchr`.
> 
> To simulate this behavior without teaching trailers_atom_parser to
> accept strings with length zero, conditionally pass NULL to
> trailers_atom_parser if the arguments portion of the argument to
> %(contents) is empty.

Doh, that string_list behavior is what I was missing in my earlier
comments. I agree this is probably the best way of doing it. I'm tempted
to say that parse_ref_filter_atom() should do a similar thing. Right now
we've got:

  $ git for-each-ref --format='%(refname)' | wc
     2206    2206   79929
  $ git for-each-ref --format='%(refname:short)' | wc
     2206    2206   53622
  $ git for-each-ref --format='%(refname:)' | wc
  fatal: unrecognized %(refname:) argument:
      0       0       0

which seems a bit unfriendly. As we're on v6 here, I don't want to
suggest it as part of this series. But maybe a #leftoverbits candidate,
if others agree it's a good direction.

-Peff

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

On Mon, Oct 02, 2017 at 01:03:51AM -0400, Jeff King wrote:
> On Sun, Oct 01, 2017 at 05:33:04PM -0700, Taylor Blau wrote:
>
> > The %(contents) atom takes a contents "field" as its argument. Since
> > "trailers" is one of those fields, extend contents_atom_parser to parse
> > "trailers"'s arguments when used through "%(contents)", like:
> >
> >   %(contents:trailers:unfold,only)
> >
> > A caveat: trailers_atom_parser expects NULL when no arguments are given
> > (see: `parse_ref_filter_atom`). This is because string_list_split (given
> > a maxsplit of -1) returns a 1-ary string_list* containing the given
> > string if the delimiter could not be found using `strchr`.
> >
> > To simulate this behavior without teaching trailers_atom_parser to
> > accept strings with length zero, conditionally pass NULL to
> > trailers_atom_parser if the arguments portion of the argument to
> > %(contents) is empty.
>
> Doh, that string_list behavior is what I was missing in my earlier
> comments. I agree this is probably the best way of doing it. I'm tempted
> to say that parse_ref_filter_atom() should do a similar thing. Right now
> we've got:
>
>   $ git for-each-ref --format='%(refname)' | wc
>      2206    2206   79929
>   $ git for-each-ref --format='%(refname:short)' | wc
>      2206    2206   53622
>   $ git for-each-ref --format='%(refname:)' | wc
>   fatal: unrecognized %(refname:) argument:
>       0       0       0
>
> which seems a bit unfriendly. As we're on v6 here, I don't want to
> suggest it as part of this series. But maybe a #leftoverbits candidate,
> if others agree it's a good direction.

I think #leftoverbits is fine here, but I think addressing this before
2.15 is reasonable. I can take a look at this in a future patch series.

--
- Taylor

Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

[Jeff King]

On Sun, Oct 01, 2017 at 10:12:16PM -0700, Taylor Blau wrote:

> > Doh, that string_list behavior is what I was missing in my earlier
> > comments. I agree this is probably the best way of doing it. I'm tempted
> > to say that parse_ref_filter_atom() should do a similar thing. Right now
> > we've got:
> >
> >   $ git for-each-ref --format='%(refname)' | wc
> >      2206    2206   79929
> >   $ git for-each-ref --format='%(refname:short)' | wc
> >      2206    2206   53622
> >   $ git for-each-ref --format='%(refname:)' | wc
> >   fatal: unrecognized %(refname:) argument:
> >       0       0       0
> >
> > which seems a bit unfriendly. As we're on v6 here, I don't want to
> > suggest it as part of this series. But maybe a #leftoverbits candidate,
> > if others agree it's a good direction.
> 
> I think #leftoverbits is fine here, but I think addressing this before
> 2.15 is reasonable. I can take a look at this in a future patch series.

I think it would be to just do it as a standalone patch immediately. I
just didn't want to hold your series hostage to a potential
disagreement.

-Peff

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

Hi,

Attached is the sixth revision of my patch-set "Support %(trailers)
arguments in for-each-ref(1)".

In includes the following changes since v5:

  * Added an additional patch to change t4205 to harden `unfold()`
    against multi-line trailer folding.

  * Added a missing parameter call in ref-filter.c to
    `trailers_atom_parser` through `contents_atom_parser`.

I believe that this version of the series should be ready for queueing.

I am going to address Peff's follow-up for teaching
`parse_ref_atom_filter` to accept "empty" argument lists as
`%(refname:)` in a follow-up series later this evening.

Thanks again :-).


--
- Taylor

[PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with ","

[Taylor Blau]

In preparation for adding consistent "%(trailers)" atom options to
`git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in
pretty.c to separate sub-arguments with a ",", instead of a ":".

Multiple sub-arguments are given either as "%(trailers:unfold,only)" or
"%(trailers:only,unfold)".

This change disambiguates between "top-level" arguments, and arguments
given to the trailers atom itself. It is consistent with the behavior of
"%(upstream)" and "%(push)" atoms.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pretty.c                      | 32 +++++++++++++++++++++++++++-----
 t/t4205-log-pretty-formats.sh |  4 ++--
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/pretty.c b/pretty.c
index 94eab5c89..7500fe694 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,6 +1056,23 @@ 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)
+{
+	const char *p;
+	if (!(skip_prefix(to_parse, candidate, &p)))
+		return 0;
+	if (*p == ',') {
+		*end = p + 1;
+		return 1;
+	}
+	if (*p == ')') {
+		*end = p;
+		return 1;
+	}
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1285,11 +1302,16 @@ 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;
-		while (*arg == ':') {
-			if (skip_prefix(arg, ":only", &arg))
-				opts.only_trailers = 1;
-			else if (skip_prefix(arg, ":unfold", &arg))
-				opts.unfold = 1;
+		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
+					break;
+			}
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f53010..977472f53 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 '
 
 test_expect_success ':only and :unfold work together' '
-	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
-	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	git log --no-walk --pretty="%(trailers:only,unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold,only)" >reverse &&
 	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 2/7] t4205: unfold across multiple lines

[Taylor Blau]

Tests in t4205 test the following:

  git log --format='%(trailers:unfold)' ...

By ensuring the multi-line trailers are unfolded back onto the same
line. t4205 only includes tests for 2-line trailers, but `unfold()` will
fail for folded trailers on 3 or more lines.

In preparation for adding subsequent tests in t6300 that test similar
behavior in `git-for-each-ref(1)`, let's harden t4205 (and make it
consistent with the changes in t6300) by ensuring that 3 or more
line folded trailers are unfolded correctly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t4205-log-pretty-formats.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 977472f53..591f35daa 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -544,7 +544,7 @@ Signed-off-by: A U Thor
 EOF
 
 unfold () {
-	perl -0pe 's/\n\s+/ /'
+	perl -0pe 's/\n\s+/ /g'
 }
 
 test_expect_success 'set up trailer tests' '
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 3/7] doc: 'trailers' is the preferred way to format trailers

[Taylor Blau]

The documentation makes reference to 'contents:trailers' as an example
to dig the trailers out of a commit. 'trailers' is an unmentioned
alternative, which is treated as an alias of 'contents:trailers'.

Since 'trailers' is easier to type, prefer that as the designated way to
dig out trailers information.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 66b4e0a40..323ce07de 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -217,7 +217,8 @@ 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'.
+are obtained as 'trailers' (or by using the historical alias
+'contents:trailers').
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 4/7] doc: use modern "`"-style code quoting

[Taylor Blau]

"'"- (single-quote) styled code quoting is no longer considered modern
within the "Documentation/" subtree.

In preparation for adding additional information to this section of
git-for-each-ref(1)'s documentation, update old-style code quoting to
use "`"-style quoting instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 323ce07de..1279b9733 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -213,12 +213,12 @@ and `date` to extract the named component.
 The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 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
+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 'trailers' (or by using the historical alias
-'contents:trailers').
+are obtained as `trailers` (or by using the historical alias
+`contents:trailers`).
 
 For sorting purposes, fields with numeric values sort in numeric order
 (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 5/7] t6300: refactor %(trailers) tests

[Taylor Blau]

We currently have one test for %(trailers) in `git-for-each-ref(1)`,
through "%(contents:trailers)". In preparation for more, let's add a few
things:

  - Move the commit creation step to its own test so that it can be
  re-used.

  - Add a non-trailer to the commit's trailers to test that non-trailers
  aren't shown using "%(trailers:only)".

  - Add a multi-line trailer to ensure that trailers are unfolded
  correctly using "%(trailers:unfold)".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6300-for-each-ref.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..39431908d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
 cat >trailers <<EOF
 Reviewed-by: A U Thor <author@example.com>
 Signed-off-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Acked-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'basic atom: head contents:trailers' '
+
+test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
 	git add two &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	Some message contents
 
 	$(cat trailers)
 	EOF
+'
+
+test_expect_success 'basic atom: head contents:trailers' '
 	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
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 6/7] ref-filter.c: use trailer_opts to format trailers

[Taylor Blau]

Fill trailer_opts with "unfold" and "only" to match the sub-arguments
given to the "%(trailers)" atom. Then, let's use the filled trailer_opts
instance with 'format_trailers_from_commit' in order to format trailers
in the desired manner.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 ref-filter.c                       | 32 +++++++++++++++++++++---------
 t/t6300-for-each-ref.sh            | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 1279b9733..4a2c851e6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -218,7 +218,10 @@ 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 `trailers` (or by using the historical alias
-`contents:trailers`).
+`contents:trailers`).  Non-trailer lines from the trailer block can be omitted
+with `trailers:only`. Whitespace-continuations can be removed from trailers so
+that each trailer appears on a line by itself with its full content with
+`trailers:unfold`. Both can be used together as `trailers:unfold,only`.
 
 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 bc591f4f3..43ed10a5e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,6 +82,7 @@ static struct used_atom {
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
 		struct {
@@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format *format, struct used_ato
 
 static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
-	if (arg)
-		die(_("%%(trailers) does not take arguments"));
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+
+	if (arg) {
+		string_list_split(&params, arg, ',', -1);
+		for (i = 0; i < params.nr; i++) {
+			const char *s = params.items[i].string;
+			if (!strcmp(s, "unfold"))
+				atom->u.contents.trailer_opts.unfold = 1;
+			else if (!strcmp(s, "only"))
+				atom->u.contents.trailer_opts.only_trailers = 1;
+			else
+				die(_("unknown %%(trailers) argument: %s"), s);
+		}
+	}
 	atom->u.contents.option = C_TRAILERS;
+	string_list_clear(&params, 0);
 }
 
 static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
@@ -1042,7 +1057,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, "trailers") &&
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
@@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			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;
+			struct strbuf s = STRBUF_INIT;
 
-			/* 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);
+			/* Format the trailer info according to the trailer_opts given */
+			format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+
+			v->s = strbuf_detach(&s, NULL);
 		} 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 39431908d..54e52d4e9 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -610,6 +610,9 @@ Acked-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /g'
+}
 
 test_expect_success 'set up trailers for next test' '
 	echo "Some contents" > two &&
@@ -623,6 +626,43 @@ test_expect_success 'set up trailers for next test' '
 	EOF
 '
 
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

[PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

The %(contents) atom takes a contents "field" as its argument. Since
"trailers" is one of those fields, extend contents_atom_parser to parse
"trailers"'s arguments when used through "%(contents)", like:

  %(contents:trailers:unfold,only)

A caveat: trailers_atom_parser expects NULL when no arguments are given
(see: `parse_ref_filter_atom`). This is because string_list_split (given
a maxsplit of -1) returns a 1-ary string_list* containing the given
string if the delimiter could not be found using `strchr`.

To simulate this behavior without teaching trailers_atom_parser to
accept strings with length zero, conditionally pass NULL to
trailers_atom_parser if the arguments portion of the argument to
%(contents) is empty.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c            |  7 ++++---
 t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 43ed10a5e..6c26b4733 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
 		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)) {
+	else if (skip_prefix(arg, "trailers", &arg)) {
+		skip_prefix(arg, ":", &arg);
+		trailers_atom_parser(format, atom, *arg ? NULL : arg);
+	} else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
 			die(_("positive value expected contents:lines=%s"), arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 54e52d4e9..872973b95 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
+	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
+	{
+		unfold <trailers
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
@@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	cat >expect <<-EOF &&
+	fatal: unknown %(trailers) argument: unsupported
+	EOF
+	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'basic atom: head contents:trailers' '
 	git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
 	sanitize_pgp <actual >actual.clean &&
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Jeff King]

On Sun, Oct 01, 2017 at 10:25:24PM -0700, Taylor Blau wrote:

> The %(contents) atom takes a contents "field" as its argument. Since
> "trailers" is one of those fields, extend contents_atom_parser to parse
> "trailers"'s arguments when used through "%(contents)", like:
> 
>   %(contents:trailers:unfold,only)
> 
> A caveat: trailers_atom_parser expects NULL when no arguments are given
> (see: `parse_ref_filter_atom`). This is because string_list_split (given
> a maxsplit of -1) returns a 1-ary string_list* containing the given
> string if the delimiter could not be found using `strchr`.
> 
> To simulate this behavior without teaching trailers_atom_parser to
> accept strings with length zero, conditionally pass NULL to
> trailers_atom_parser if the arguments portion of the argument to
> %(contents) is empty.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  ref-filter.c            |  7 ++++---
>  t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 3 deletions(-)

This patch seems to fail a bunch of tests in t6300.

> diff --git a/ref-filter.c b/ref-filter.c
> index 43ed10a5e..6c26b4733 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
>  		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)) {
> +	else if (skip_prefix(arg, "trailers", &arg)) {
> +		skip_prefix(arg, ":", &arg);
> +		trailers_atom_parser(format, atom, *arg ? NULL : arg);

I think your logic is flipped. You want "*arg ? arg : NULL";

-Peff

Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  		atom->u.contents.option = C_SUB;
>> -	else if (!strcmp(arg, "trailers"))
>> -		atom->u.contents.option = C_TRAILERS;
>> -	else if (skip_prefix(arg, "lines=", &arg)) {
>> +	else if (skip_prefix(arg, "trailers", &arg)) {
>> +		skip_prefix(arg, ":", &arg);
>> +		trailers_atom_parser(format, atom, *arg ? NULL : arg);
>
> I think your logic is flipped. You want "*arg ? arg : NULL";

Ahh, I was blind.  Will fix locally and push the results out.

Thanks.

Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Taylor Blau]

On Mon, Oct 02, 2017 at 02:51:00AM -0400, Jeff King wrote:
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 43ed10a5e..6c26b4733 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
> >  		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)) {
> > +	else if (skip_prefix(arg, "trailers", &arg)) {
> > +		skip_prefix(arg, ":", &arg);
> > +		trailers_atom_parser(format, atom, *arg ? NULL : arg);
>
> I think your logic is flipped. You want "*arg ? arg : NULL";

Thank you for pointing this out. I should have run "make test" on this
patch set (or, as you suggested, `git rebase -x "make test" HEAD~7`)
before sending it out. I appreciate you catching my mistake, and I'll
make sure to run "make test" more diligently in the future :-).

It sounds like Junio picked this up while queueing.

--
- Taylor

Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Thank you for pointing this out. I should have run "make test" on this
> patch set (or, as you suggested, `git rebase -x "make test" HEAD~7`)
> before sending it out. I appreciate you catching my mistake, and I'll
> make sure to run "make test" more diligently in the future :-).
>
> It sounds like Junio picked this up while queueing.

"Yup" in the sense that "you do not have to worry about it unless
there are other things you want to fix by rerolling" but I didn't
spot it myself and noticed the breakage; I tweaked it only after
Peff pointed out what is wrong.

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Jeff King]

On Sun, Oct 01, 2017 at 10:23:26PM -0700, Taylor Blau wrote:

> Attached is the sixth revision of my patch-set "Support %(trailers)
> arguments in for-each-ref(1)".
> 
> In includes the following changes since v5:
> 
>   * Added an additional patch to change t4205 to harden `unfold()`
>     against multi-line trailer folding.
> 
>   * Added a missing parameter call in ref-filter.c to
>     `trailers_atom_parser` through `contents_atom_parser`.
> 
> I believe that this version of the series should be ready for queueing.

This looks good to me, modulo the flipped logic in the final patch.

Since that's the only thing I noticed, let's hold off on a reroll for
the moment to see if there are any more comments (and I won't be
surprised if Junio just picks it up with the tweak, but we'll see).

Please do make sure that "make test" runs clean before posting (I
usually run it on each commit to catch any "oops, we broke this and then
refixed it" in the middle of the series, too).

-Peff

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Since that's the only thing I noticed, let's hold off on a reroll for
> the moment to see if there are any more comments (and I won't be
> surprised if Junio just picks it up with the tweak, but we'll see).
>
> Please do make sure that "make test" runs clean before posting (I
> usually run it on each commit to catch any "oops, we broke this and then
> refixed it" in the middle of the series, too).

OK, I think fixes for both the "flipped" and the "gettext-poison"
breakages are solved locally in my tree already, so I guess this is
ready to be merged to 'next'.

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Jeff King]

On Tue, Oct 03, 2017 at 03:24:41PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Since that's the only thing I noticed, let's hold off on a reroll for
> > the moment to see if there are any more comments (and I won't be
> > surprised if Junio just picks it up with the tweak, but we'll see).
> >
> > Please do make sure that "make test" runs clean before posting (I
> > usually run it on each commit to catch any "oops, we broke this and then
> > refixed it" in the middle of the series, too).
> 
> OK, I think fixes for both the "flipped" and the "gettext-poison"
> breakages are solved locally in my tree already, so I guess this is
> ready to be merged to 'next'.

Yes, I think so.

Out of curiosity, do you frequently test with GETTEXT_POISON, or did you
just guess at a potential problem after reading the tests?  Proper use
of test_i18ncmp is definitely something we ought to be looking for
during review, but I confess it's something I often miss.

-Peff

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Out of curiosity, do you frequently test with GETTEXT_POISON, or did you
> just guess at a potential problem after reading the tests?  Proper use
> of test_i18ncmp is definitely something we ought to be looking for
> during review, but I confess it's something I often miss.

I learn of breakages after the fact, when Travis reports.

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Attached is the sixth revision of my patch-set "Support %(trailers)
> arguments in for-each-ref(1)".
>
> In includes the following changes since v5:
>
>   * Added an additional patch to change t4205 to harden `unfold()`
>     against multi-line trailer folding.
>
>   * Added a missing parameter call in ref-filter.c to
>     `trailers_atom_parser` through `contents_atom_parser`.
>
> I believe that this version of the series should be ready for queueing.
>
> I am going to address Peff's follow-up for teaching
> `parse_ref_atom_filter` to accept "empty" argument lists as
> `%(refname:)` in a follow-up series later this evening.
>
> Thanks again :-).

Thanks.  t6300 seems to show that %(contents:trailers:unfold) does
not quite work, among other things.

  https://travis-ci.org/git/git/jobs/282126607#L3658

I didn't have a chance to look into it myself.

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks.  t6300 seems to show that %(contents:trailers:unfold) does
> not quite work, among other things.
>
>   https://travis-ci.org/git/git/jobs/282126607#L3658
>
> I didn't have a chance to look into it myself.

Peff's "oops, your logic is backwards" fixes the above failure.

We also need this on top to pass the gettext-poison build.

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 872973b954..3bdfa02559 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) w
 '
 
 test_expect_success '%(trailers) rejects unknown trailers arguments' '
+	# error message cannot be checked under i18n
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
 	EOF
 	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
+	# error message cannot be checked under i18n
 	cat >expect <<-EOF &&
 	fatal: unknown %(trailers) argument: unsupported
 	EOF
 	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 test_expect_success 'basic atom: head contents:trailers' '

Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)

[Taylor Blau]

On Mon, Oct 02, 2017 at 09:15:00PM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Thanks.  t6300 seems to show that %(contents:trailers:unfold) does
> > not quite work, among other things.
> >
> >   https://travis-ci.org/git/git/jobs/282126607#L3658
> >
> > I didn't have a chance to look into it myself.
>
> Peff's "oops, your logic is backwards" fixes the above failure.
>
> We also need this on top to pass the gettext-poison build.
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 872973b954..3bdfa02559 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) w
>  '
>
>  test_expect_success '%(trailers) rejects unknown trailers arguments' '
> +	# error message cannot be checked under i18n
>  	cat >expect <<-EOF &&
>  	fatal: unknown %(trailers) argument: unsupported
>  	EOF
>  	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> -	test_cmp expect actual
> +	test_i18ncmp expect actual
>  '
>
>  test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> +	# error message cannot be checked under i18n
>  	cat >expect <<-EOF &&
>  	fatal: unknown %(trailers) argument: unsupported
>  	EOF
>  	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
> -	test_cmp expect actual
> +	test_i18ncmp expect actual
>  '
>
>  test_expect_success 'basic atom: head contents:trailers' '

Thank you for pointing this out. I am not well-versed on gettext, and
its usage within Git. I am happy to send out v7 of this series, or you
can apply these changes in queueing. Whichever is easier :-).

--
- Taylor