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(¶ms, 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(¶ms, 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