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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2017, 00:19 UTC
Message-ID: <xmqqy3ous8nx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171001161852.84571-6-me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 11 quoted lines

> 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....

Show 22 quoted lines

>
> 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=", ...) {

Show 6 quoted 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.