Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

[Junio C Hamano <gitster@pobox.com>]

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Hariom Verma <hariom18599@gmail.com>
>
> The 'contents' atom does not show any error if used with 'trailers'
> atom and semicolon is missing before trailers arguments.
>
> e.g %(contents:trailersonly) works, while it shouldn't.
>
> It is definitely not an expected behavior.
>
> Let's fix this bug.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Heba Waly <heba.waly@gmail.com>
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> ---

Nice spotting.  7a5edbdb (ref-filter.c: parse trailers arguments
with %(contents) atom, 2017-10-01) talks about being deliberate
about the case where skip_prefix(":") does not find a colon after
the "trailers" token, but from the message it is clear that it
expected that the case happens only when "trailers" is at the end of
the string.

The new helper that is overly verbose and may be overkill.

Shouldn't this be clear enough, equivalent and sufficient?

	else if (skip_prefix(arg, "trailers", &arg) &&
		 (!*arg || *arg == ':'))) {
		if (trailers_atom_parser(...);

That is, we not just make sure the string begins with "trailers",
but also make sure it either (1) ends the string (i.e. the token is
just "trailers"), or (2) is followed by a colon ':', before entering
the block to handle "trailers[:anything]".  If we later add a new
atom "trailersonly", that will not be handled here, but elsewhere in
the "else if" cascade.

>  ref-filter.c            | 21 ++++++++++++++++++---
>  t/t6300-for-each-ref.sh |  9 +++++++++
>  2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ba85869755..dc31fbbe51 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato
>  	return 0;
>  }
>  
> +static int check_format_field(const char *arg, const char *field, const char **option)
> +{
> +	const char *opt;
> +	if (skip_prefix(arg, field, &opt)) {
> +		if (*opt == '\0') {
> +			*option = NULL;
> +			return 1;
> +		}
> +		else if (*opt == ':') {
> +			*option = ++opt;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
>  				const char *arg, struct strbuf *err)
>  {
> @@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
>  		atom->u.contents.option = C_SIG;
>  	else if (!strcmp(arg, "subject"))
>  		atom->u.contents.option = C_SUB;
> -	else if (skip_prefix(arg, "trailers", &arg)) {
> -		skip_prefix(arg, ":", &arg);
> -		if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
> +	else if (check_format_field(arg, "trailers", &arg)) {
> +		if (trailers_atom_parser(format, atom, arg, err))
>  			return -1;
>  	} else if (skip_prefix(arg, "lines=", &arg)) {
>  		atom->u.contents.option = C_LINES;