Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 19, 2020, 19:07 UTC
- Message-ID
- <xmqqmu2qtpxp.fsf@gitster.c.googlers.com>
- In-Reply-To
- <xmqqv9hettag.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> "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.
Show 15 quoted lines
>> +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.