threads / patch / 36261

patchGSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

Subject: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

## tl;dr

3 messages between Mar 21, 2014 and Mar 21, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Ashwin Jha· Mar 21, 2014, 01:54 UTC · lore

modified fsck.c:fsck_commit(). Replaced memcmp() with starts_with() function. starts_with() seems much more relevant than memcmp(). It uses one less argument and its return value makes more sense. skip_prefix() is not used as it uses strcmp() internally which seems unnecessarily for current task. The current task can be easily done by providing offsets to the buffer pointer (the way it is implemented currently).

Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>
---
 fsck.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Show changes to fsck.c +6 −5

diff --git a/fsck.c b/fsck.c
index 64bf279..82e1640 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "fsck.h"
+#include "strbuf.h"
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -290,12 +291,12 @@ 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");
 	buffer += 46;
-	while (!memcmp(buffer, "parent ", 7)) {
+	while (starts_with(buffer, "parent ")) {
 		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
 			return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
 		buffer += 48;
@@ -322,15 +323,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 ")))
+	if (!starts_with(buffer, "committer "))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
-	buffer += strlen("committer ");
+	buffer += 10;
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
 		return err;

-- 
1.7.9.5

Eric Sunshine· Mar 21, 2014, 03:33 UTC · re: Ashwin Jha · lore

Re: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

Thanks for the submission. Comments below to give you a feel for the Git review process...

On Thu, Mar 20, 2014 at 9:54 PM, Ashwin Jha <ajha.dev@gmail.com> wrote:

> Subject: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

The subject becomes part of the permanent Git history, but the fact that this is a GSoC submission won't be meaningful to anyone months or years from now. You can mention GSoC inside [...], however, as in [PATCH GSoC], since that gets stripped off the subject automatically when the patch is applied. Use the commentary section after the "---" line just below your sign-off to explain that this is microproject 15.

The subject itself should concisely summarize the change. "Rewrite fsck.c:fsck_commit()" doesn't say much. You might say instead:

    Subject: fsk_commit: replace memcmp() with starts_with()

> modified fsck.c:fsck_commit(). Replaced memcmp() with starts_with() function.

Capitalize start of sentence.

Use imperative mood: "modify" rather than "modified"; "Replace" rather than "Replaced".

> starts_with() seems much more relevant than memcmp(). It uses one less argument
> and its return value makes more sense.

As a justification, "uses one less argument" falls flat, and really has nothing to do with the decision to make the change. The bit about the return value is a slightly better but is still weak. You might instead justify the change by pointing out that the name starts_with() does a better job of conveying the intention of the code, which is to check the string for a prefix, than does memcmp().

> skip_prefix() is not used as it uses strcmp() internally which seems unnecessarily
> for current task. The current task can be easily done by providing offsets to the
> buffer pointer (the way it is implemented currently).

Not sure what this means. What is the "current task", and what is implemented where currently?

Show 25 quoted lines

> Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>
> ---
>  fsck.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 64bf279..82e1640 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -6,6 +6,7 @@
>  #include "commit.h"
>  #include "tag.h"
>  #include "fsck.h"
> +#include "strbuf.h"
>
>  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
>  {
> @@ -290,12 +291,12 @@ 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 benefits of starts_with() and skip_prefix() is that they allow you to eliminate magic numbers, such as 5 in the memcmp() invocation. However, if you look a couple lines below, you see in the expression 'buffer+5' that the magic number is still present. In fact, the code becomes less clear with your change because the 5 in 'buffer+5' is much more mysterious without the preceding memcmp(foo,"bar",5). It is possible to eliminate this magic number, but starts_with() is not the answer.

Show 5 quoted lines

>                 return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
>         buffer += 46;
> -       while (!memcmp(buffer, "parent ", 7)) {
> +       while (starts_with(buffer, "parent ")) {
>                 if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')

Ditto here with magic number 7 in 'buffer+7'.

Show 10 quoted lines

>                         return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
>                 buffer += 48;
> @@ -322,15 +323,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;

And again with 7.

Show 8 quoted lines

>         err = fsck_ident(&buffer, &commit->object, error_func);
>         if (err)
>                 return err;
> -       if (memcmp(buffer, "committer ", strlen("committer ")))
> +       if (!starts_with(buffer, "committer "))
>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
> -       buffer += strlen("committer ");
> +       buffer += 10;

Again with 10 (newly introduced).

Show 5 quoted lines

>         err = fsck_ident(&buffer, &commit->object, error_func);
>         if (err)
>                 return err;
> --
> 1.7.9.5

Ashwin Jha· Mar 21, 2014, 15:42 UTC · re: Ashwin Jha · lore

Re: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

-------- Original Message --------
Subject: 	[PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()
Date: 	Fri, 21 Mar 2014 07:24:46 +0530
From: 	Ashwin Jha <ajha.dev@gmail.com>
To: 	git@vger.kernel.org
CC: 	Ashwin Jha <ajha.dev@gmail.com>

Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>
---
  fsck.c |   11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)

Show changes to fsck.c +6 −5

diff --git a/fsck.c b/fsck.c
index 64bf279..82e1640 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
  #include "commit.h"
  #include "tag.h"
  #include "fsck.h"
+#include "strbuf.h"
  
  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
  {
@@ -290,12 +291,12 @@ 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");
  	buffer += 46;
-	while (!memcmp(buffer, "parent ", 7)) {
+	while (starts_with(buffer, "parent ")) {
  		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
  			return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
  		buffer += 48;
@@ -322,15 +323,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 ")))
+	if (!starts_with(buffer, "committer "))
  		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
-	buffer += strlen("committer ");
+	buffer += 10;
  	err = fsck_ident(&buffer, &commit->object, error_func);
  	if (err)
  		return err;

-- 
1.7.9.5