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

use of starts_with() instead of memcmp()

use of skip_prefix instead of memcmp() and strlen()

Signed-off-by: Othman Darraz <redacted>
---

I am planning to apply to GSOC 214
 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')
 		return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");

@@ -322,15 +322,15 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
 		if (p || parents)
 			return error_func(&commit->object, FSCK_ERROR, "parent objects missing");
 	}
-	if (memcmp(buffer, "author ", 7))
+	if (starts_with(buffer, "author "))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line");
 	buffer += 7;
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
 		return err;
-	if (memcmp(buffer, "committer ", strlen("committer ")))
+	buffer = (char* )skip_prefix(buffer,"committer ");
+	if (!buffer)
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
-	buffer += strlen("committer ");
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
 		return err;

-- 
1.9.0.258.g00eda23.dirty

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 [off-list ref] 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 <redacted>
---

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.

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.

Same problem here with magic number 7.

        err = fsck_ident(&buffer, &commit->object, error_func);
        if (err)
                return err;
-       if (memcmp(buffer, "committer ", strlen("committer ")))
+       buffer = (char* )skip_prefix(buffer,"committer ");

Style: (char *)
Style: whitespace: skip_prefix(foo, "bar")

Regarding the (char *) cast: Is 'buffer' ever modified in this
function? If not, would it make sense to make it 'const' (and fix any
other small fallout from that change)?

+       if (!buffer)
                return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
-       buffer += strlen("committer ");
        err = fsck_ident(&buffer, &commit->object, error_func);
        if (err)
                return err;
--
1.9.0.258.g00eda23.dirty

On Tue, Mar 18, 2014 at 7:09 PM, Eric Sunshine [off-list ref] wrote:

quoted

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".

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 [off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Othman Darraz <redacted>
---

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.

quoted

 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/