Brandon Casey <bca...@nvidia.com> writes:
> On 2/12/2013 11:36 AM, Junio C Hamano wrote:
>> Brandon Casey <bca...@nvidia.com> writes:
>>
>>>>> + return len > strlen(cherry_picked_prefix) + 1 &&
>>>>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>>>> +}
>>>>
>>>> Does the first "is it longer than the prefix?" check matter? If it
>>>> is not, prefixcmp() would not match anyway, no?
>>>
>>> Probably not in practice, but technically we should only be accessing
>>> len characters in buf even though buf may be longer than len. So the
>>> check is just making sure the function doesn't access chars it's not
>>> supposed to.
>>
>> Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],
>> which would never match cherry_picked_prefix even if len is shorter
>> than the prefix?
>
> Heh, I almost pointed that out in my reply. Yes, buf will be terminated
> with LF at buf[len]. And yes, that means that we will never get a false
> positive from prefixcmp even if the comparison overruns buf+len while
> doing its comparison. That's why the check doesn't matter in practice,
> i.e. based on the way that is_cherry_picked_from_line is being called
> right now and the content of cherry_picked_prefix.
>
> But, hasn't is_cherry_picked_from_line entered into a contract with the
> caller and said "I will not access more than len characters"?
>
> It's ok with me if you think it reads better without the check.
As Jonathan says, if you rewrite it to
return buf[len - 1] == ')' && !prefixcmp(buf, cherry_picked_prefix);
then the code can keep its promise without the length check, because
it knows there is no ')' in cherry-picked-prefix, and it also knows
prefixcmp() stops at the first difference.
It is not a huge deal; I was primarily reacting to the ugly multi-line
boolean expresion that is not inside a pair of parentheses (and because
this is a "return" statement, there is no good reason to have parentheses
except that this is a multi-line expression), which looked odd.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
