Re: [PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/19/2014 12:09 AM, Eric Sunshine wrote:
> Thanks for the submission. Comments below to give you a taste of the
> Git review process...
> 
> On Tue, Mar 18, 2014 at 6:41 PM, Othman Darraz <darrazo16@gmail.com> wrote:
>> use of starts_with() instead of memcmp()
>>
>> use of skip_prefix instead of memcmp() and strlen()
> 
> Write proper sentences to explain and justify the change; not sentence
> fragments.
> 
>> Signed-off-by: Othman Darraz <darrazo16@gmail.com>
>> ---
>>
>> I am planning to apply to GSOC 214
> 
> Your logic in these changes is rather severely flawed. Running the
> test suite (t1450, in particular), as the GSoC microproject page
> suggests, would have clued you in that something was amiss.
> 
>>  fsck.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fsck.c b/fsck.c
>> index 64bf279..5eae856 100644
>> --- a/fsck.c
>> +++ b/fsck.c
>> @@ -290,7 +290,7 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>>         int parents = 0;
>>         int err;
>>
>> -       if (memcmp(buffer, "tree ", 5))
>> +       if (starts_with(buffer, "tree "))
>>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line");
>>         if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
> 
> One of the reasons for using starts_with() rather than memcmp() is
> that it allows you to eliminate magic numbers, such as 5. However, if
> you look closely at this code fragment, you will see that the magic
> number is still present in the expression 'buffer+5'. starts_with(),
> might be a better fit.

Eric, I think you meant to say that *skip_prefix()* might be a better fit.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/