From: Michael Haggerty Date: Tue, 18 Mar 2014 23:16:16 GMT Subject: Re: [PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix() Message-ID: <5328D3C0.5080009@alum.mit.edu> In-Reply-To: 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 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 >> --- >> >> 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/