Thread (54 messages) flat view 54 messages, 2 authors, 2017-10-02

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

From: Jeff King <hidden>
Date: 2017-09-30 07:21:51

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 <redacted>
---
 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?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
 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.
quoted hunk ↗ jump to hunk
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:
quoted hunk ↗ jump to hunk
@@ -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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help