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

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

Eric Sunshine <sunshine@sunshineco.com> writes:

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