Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2016, 01:17 UTC
Message-ID: <xmqqa8nhjmtn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cRmfNjP8PYoQFZ7YrECgt03aE1=QynG58-+cd9ORJneZw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 15 quoted lines

>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>                  * shouldn't be used for checking against the valid_atom
>>                  * table.
>>                  */
>> -               const char *formatp = strchr(sp, ':');
>> -               if (!formatp || ep < formatp)
>> -                       formatp = ep;
>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>> +               arg = memchr(sp, ':', ep - sp);
>
> Why this change from strchr() to memchr()? I understand that you're
> taking advantage of the fact that you know the extent of the string
> via 'sp' and 'ep', however, was the original strchr() doing extra
> work? Even if this change is desirable, it seems somewhat unrelated to
> the overall purpose of this patch, thus might deserves its own.

I think the original strchr() is a bug. If you are given a substring as a range, you shouldn't be allowing strchr() to go beyond ep to find a NUL that may or may not exist. That is not a performance thing, but more about the best practice to ensure correctness. The caller of this function may not have such a problem, but imagine the case where the bytes beyond ep did not have any NUL and there is an unmapped page after that.