threads / patch / 36142

v2, 2 partsGSoC micro project, use skip_prefix() in fsck_commit()

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

## tl;dr

6 messages between Mar 12, 2014 and Mar 13, 2014. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Yuxuan Shui· Mar 12, 2014, 18:51 UTC · lore

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

Yuxuan Shui· Mar 12, 2014, 18:51 UTC · re: Yuxuan Shui · lore

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

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(-)

Show changes to fsck.c +1 −1

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

Jeff King· Mar 12, 2014, 20:22 UTC · re: Yuxuan Shui · lore

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

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:

Show changes to fsck.c +1 −2

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

Yuxuan Shui· Mar 13, 2014, 04:29 UTC · re: Jeff King · lore

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

Hi,

On Thu, Mar 13, 2014 at 4:22 AM, Jeff King <peff@peff.net> wrote:

Show 8 quoted lines

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

Show 20 quoted lines

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

Yuxuan Shui· Mar 12, 2014, 18:51 UTC · re: Yuxuan Shui · lore

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

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(-)

Show changes to fsck.c +13 −11

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

Junio C Hamano· Mar 12, 2014, 19:47 UTC · re: Yuxuan Shui · lore

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

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?

Show 26 quoted lines

>
> 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");