Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers
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).
Show 6 quoted lines
> 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?
Show 14 quoted lines
> 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.
Show 28 quoted lines
> 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.
Show 26 quoted lines
> 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:
Show 12 quoted lines
> @@ -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.
Show 19 quoted lines
> +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.
Show 7 quoted lines
> +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
Taylor Blau
