[PATCH v3 2/2] fsck.c:fsck_commit replace memcmp by skip_prefix

[PATCH v3 2/2] fsck.c:fsck_commit replace memcmp by skip_prefix

[Ashwin Jha]

Replace memcmp by skip_prefix as it serves the dual
purpose of checking the string for a prefix as well
as skipping that prefix.

Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>
---

fsck_commit(): After the first patch in this series, it is now safe to replace
memcmp() with skip_prefix().

Previous versions can be found at [v1] and [v2]:
[v1]: http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html
[v2]: http://git.661346.n2.nabble.com/PATCH-Modify-fsck-commit-Replace-memcmp-by-skip-prefix-td7606321.html

 fsck.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fsck.c b/fsck.c
index b5f017f..2232ce3 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "fsck.h"
+#include "git-compat-util.h"
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -284,21 +285,23 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f
 
 static int fsck_commit(struct commit *commit, fsck_error error_func)
 {
-	const char *buffer = commit->buffer;
+	const char *parent_id, *buffer = commit->buffer;
 	unsigned char tree_sha1[20], sha1[20];
 	struct commit_graft *graft;
 	int parents = 0;
 	int err;
 
-	if (memcmp(buffer, "tree ", 5))
+	buffer = skip_prefix(buffer, "tree ");
+	if (!buffer)
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line");
-	if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
+	if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
 		return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
-	buffer += 46;
-	while (!memcmp(buffer, "parent ", 7)) {
-		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+	buffer += 41;
+	while ((parent_id = skip_prefix(buffer, "parent "))) {
+		buffer = parent_id;
+		if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
 			return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
-		buffer += 48;
+		buffer += 41;
 		parents++;
 	}
 	graft = lookup_commit_graft(commit->object.sha1);
@@ -322,15 +325,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))
+	buffer = skip_prefix(buffer, "author ");
+	if (!buffer)
 		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 = 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.1

Re: [PATCH v3 2/2] fsck.c:fsck_commit replace memcmp by skip_prefix

[Eric Sunshine]

On Mon, Mar 24, 2014 at 8:09 AM, Ashwin Jha <ajha.dev@gmail.com> wrote:
> Replace memcmp by skip_prefix as it serves the dual
> purpose of checking the string for a prefix as well
> as skipping that prefix.
>
> Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>
> ---
>
> fsck_commit(): After the first patch in this series, it is now safe to replace
> memcmp() with skip_prefix().

The above commentary might be a bit easer to understand if written
instead something like this:

    Changes since v2:

    New patch 1/2 adds missing 'const's, so this patch no longer
    needs to cast-away the constness of the result of skip_prefix().

The patch itself looks fine.

You probably don't need to resend. My comments on these two patches
can serve as inspiration (or not) for patches you may send in the
future. What's important is that you have had a taste of the Git
review process, and the GSoC mentors have had a chance to observe your
abilities and reviewer interaction skills.

> Previous versions can be found at [v1] and [v2]:
> [v1]: http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html
> [v2]: http://git.661346.n2.nabble.com/PATCH-Modify-fsck-commit-Replace-memcmp-by-skip-prefix-td7606321.html
>
>  fsck.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index b5f017f..2232ce3 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -6,6 +6,7 @@
>  #include "commit.h"
>  #include "tag.h"
>  #include "fsck.h"
> +#include "git-compat-util.h"
>
>  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
>  {
> @@ -284,21 +285,23 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f
>
>  static int fsck_commit(struct commit *commit, fsck_error error_func)
>  {
> -       const char *buffer = commit->buffer;
> +       const char *parent_id, *buffer = commit->buffer;
>         unsigned char tree_sha1[20], sha1[20];
>         struct commit_graft *graft;
>         int parents = 0;
>         int err;
>
> -       if (memcmp(buffer, "tree ", 5))
> +       buffer = skip_prefix(buffer, "tree ");
> +       if (!buffer)
>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line");
> -       if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
> +       if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
>                 return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
> -       buffer += 46;
> -       while (!memcmp(buffer, "parent ", 7)) {
> -               if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
> +       buffer += 41;
> +       while ((parent_id = skip_prefix(buffer, "parent "))) {
> +               buffer = parent_id;
> +               if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
>                         return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
> -               buffer += 48;
> +               buffer += 41;
>                 parents++;
>         }
>         graft = lookup_commit_graft(commit->object.sha1);
> @@ -322,15 +325,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))
> +       buffer = skip_prefix(buffer, "author ");
> +       if (!buffer)
>                 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 = 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.1