Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2013, 19:58 UTC
Message-ID: <7vpq05fgn4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <511A9CDB.9060008@nvidia.com>

Brandon Casey <bcasey@nvidia.com> writes:

Show 30 quoted lines

> On 2/12/2013 11:36 AM, Junio C Hamano wrote:
>> Brandon Casey <bcasey@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.