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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 19, 2014, 09:33 UTC
Message-ID: <CAPig+cQ=3tjC47FgPRFkDW1dgE9p1uKsAiZ6uZoNXR0mBMjuBA@mail.gmail.com>
In-Reply-To: <CAOfd7q0p5gtaPMuds8LkO8q3FJnAJgb_2-6cKeNiqFqEvP52Ww@mail.gmail.com>

On Wed, Mar 19, 2014 at 5:04 AM, Othman Darraz <darrazo16@gmail.com> wrote:

Show 30 quoted lines

> 2014-03-19 0:12 GMT+01:00 Eric Sunshine <sunshine@sunshineco.com>:
>> On Tue, Mar 18, 2014 at 7:09 PM, Eric Sunshine <sunshine@sunshineco.com>
>> wrote:
>>
>> >> 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.
>>
>> Of course, I meant "skip_prefix() might be a better fit".
>
> Thank you for your review and feedbacks.
>  Actually I made a mistake because I misunderstood how to run the tests, I
> was using the wrong Makefile.

For quick initial testing, you can just run the single test script. For instance:

    (cd t && ./t1450-fsck.sh)

Once you have that running correctly, then run the entire test suite to ensure that your changes didn't break anything else.

> Secondly I think , as well, that skip_prefix()
> is a better fit. Nevertheless, as the variable buffer change in this
> function, using skip_prefix() implies the use of cast.

Yes, the variable named 'buffer' changes, but does the content of the memory referenced by 'buffer' ever change? If not, then 'buffer' could be made 'const', thus eliminating the need for the cast. (Note that changing it to 'const' might involve a bit of extra work, but the question is still pertinent.)

Show 6 quoted lines

> I will make the
> changes right now.
>
> Thank you for your time.
>
> Othman DARRAZ