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

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

Junio C Hamano <gitster@pobox.com> writes:

> "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(...);

Ah, no, even with "*arg++ == ':'.  This moves arg past "trailers" if
given "trailersandsomegarbage" and the next one in "else if" cascade
would look at "andsomegarbage"---which is not what we want.

>> +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;
>> +}

And the helper does not have such a breakage.  It looks good.

Thanks.