[PATCH v2 0/2] GSoC micro project, use skip_prefix() in fsck_commit()

[PATCH v2 0/2] GSoC micro project, use skip_prefix() in fsck_commit()

[Yuxuan Shui]

I'm sorry for resending these patches, but the previous ones miss the sign-offs.

Yuxuan Shui (2):
  fsck.c: Change the type of fsck_ident()'s first argument
  fsck.c: Rewrite fsck_commit() to use skip_prefix()

 fsck.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

-- 
1.9.0

[PATCH v2 1/2] fsck.c: Change the type of fsck_ident()'s first argument

[Yuxuan Shui]

Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.

This change is required to rewrite fsck_commit() to use skip_prefix().

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
---
 fsck.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsck.c b/fsck.c
index 99c0497..1789c34 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 	return retval;
 }
 
-static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
 {
 	if (**ident == '<')
 		return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing space before email");
-- 
1.9.0

[PATCH v2 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()

[Yuxuan Shui]

The purpose of skip_prefix() is much clearer than memcmp(). Also
skip_prefix() takes one less argument and its return value makes more
sense.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
---
 fsck.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/fsck.c b/fsck.c
index 1789c34..7e6b829 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ 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)
 {
-	char *buffer = commit->buffer;
+	const char *buffer = commit->buffer, *tmp;
 	unsigned char tree_sha1[20], sha1[20];
 	struct commit_graft *graft;
 	int parents = 0;
@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
 	if (commit->date == ULONG_MAX)
 		return error_func(&commit->object, FSCK_ERROR, "invalid author/committer line");
 
-	if (memcmp(buffer, "tree ", 5))
+	buffer = skip_prefix(buffer, "tree ");
+	if (buffer == NULL)
 		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 ((tmp = skip_prefix(buffer, "parent "))) {
+		buffer = tmp;
+		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 +324,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 == NULL)
 		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 == NULL)
 		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

Re: [PATCH v2 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()

[Junio C Hamano]

Yuxuan Shui <yshuiv7@gmail.com> writes:

> The purpose of skip_prefix() is much clearer than memcmp().  Also
> skip_prefix() takes one less argument and its return value makes
> more sense.

Instead of justifying the change with a subjective-sounding and
vague "much clearer" and "makes more sense", perhaps you can state
what the purpose is and why it makes more sense, no?  "We are using
memcmp() to see if the buffer starts with a known constant prefix
string and skip that prefix if it does so, skip_prefix() is a better
match" or something?

>
> Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
> ---
>  fsck.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 1789c34..7e6b829 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -281,7 +281,7 @@ 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)
>  {
> -	char *buffer = commit->buffer;
> +	const char *buffer = commit->buffer, *tmp;
>  	unsigned char tree_sha1[20], sha1[20];
>  	struct commit_graft *graft;
>  	int parents = 0;
> @@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>  	if (commit->date == ULONG_MAX)
>  		return error_func(&commit->object, FSCK_ERROR, "invalid author/committer line");
>  
> -	if (memcmp(buffer, "tree ", 5))
> +	buffer = skip_prefix(buffer, "tree ");
> +	if (buffer == NULL)

if (!buffer)

>  		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line");

Re: [PATCH v2 1/2] fsck.c: Change the type of fsck_ident()'s first argument

[Jeff King]

On Thu, Mar 13, 2014 at 02:51:29AM +0800, Yuxuan Shui wrote:

> Since fsck_ident doesn't change the content of **ident, the type of
> ident could be const char **.

Unfortunately, const double-pointers in C are a bit tricky, and a
pointer to "char *" cannot automatically be passed as a pointer to
"const char *". 

I think you want this on top:

diff --git a/fsck.c b/fsck.c
index 1789c34..7776660 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ 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)
 {
-	char *buffer = commit->buffer;
+	const char *buffer = commit->buffer;
 	unsigned char tree_sha1[20], sha1[20];
 	struct commit_graft *graft;
 	int parents = 0;

Otherwise, gcc will complain about incompatible pointer types.

-Peff

Re: [PATCH v2 1/2] fsck.c: Change the type of fsck_ident()'s first argument

[Yuxuan Shui]

Hi,

On Thu, Mar 13, 2014 at 4:22 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 13, 2014 at 02:51:29AM +0800, Yuxuan Shui wrote:
>
>> Since fsck_ident doesn't change the content of **ident, the type of
>> ident could be const char **.
>
> Unfortunately, const double-pointers in C are a bit tricky, and a
> pointer to "char *" cannot automatically be passed as a pointer to
> "const char *".

Thanks for pointing this out, I split the changes in a wrong way. I'll
fix this in next version of this patch.

>
> I think you want this on top:
>
> diff --git a/fsck.c b/fsck.c
> index 1789c34..7776660 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -281,7 +281,7 @@ 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)
>  {
> -       char *buffer = commit->buffer;
> +       const char *buffer = commit->buffer;
>         unsigned char tree_sha1[20], sha1[20];
>         struct commit_graft *graft;
>         int parents = 0;
>
> Otherwise, gcc will complain about incompatible pointer types.
>
> -Peff

-- 

Regards
Yuxuan Shui