Brandon Casey <bcasey@xxxxxxxxxx> writes: > On 2/12/2013 11:36 AM, Junio C Hamano wrote: >> Brandon Casey <bcasey@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html