[PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()

[PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()

[blacksimit]

From: Oguzhan Unlu <cengoguzhanunlu@gmail.com>

My solution to make lines containing buffer += a_number; clearer to anyone is following; I defined a new int, magic_num, then assigned lengths of used strings to magic_num and then changed assignment lines through using magic_num so that where the number which is added to buffer is known although I eliminated 3rd parameter of memcmp() when using starts_with().   

Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>
---
 I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks!
 fsck.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fsck.c b/fsck.c
index d43be98..4e5ca30 100644
--- a/fsck.c
+++ b/fsck.c
@@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
 	struct commit_graft *graft;
 	int parents = 0;
 	int err;
-
+    int magic_num;
+    
+    magic_num = strlen("tree "); /* magic_num is 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')
+	if (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\n')
 		return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
 	buffer += 46;
+    magic_num = strlen("parent "); /* magic_num is 7 */
 	while (starts_with(buffer, "parent ")) {
-		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+		if (get_sha1_hex(buffer+magic_num, sha1) || buffer[47] != '\n')
 			return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
 		buffer += 48;
 		parents++;
@@ -322,15 +325,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
 		if (p || parents)
 			return error_func(&commit->object, FSCK_ERROR, "parent objects missing");
 	}
+    magic_num = strlen("author "); /* magic_num is 7 */
 	if (!starts_with(buffer, "author "))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line");
-	buffer += 7;
+	buffer += magic_num;
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
 		return err;
+    magic_num = strlen("committer"); /* magic_num is 7 */
 	if (!starts_with(buffer, "committer "))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
-	buffer += strlen("committer ");
+	buffer += magic_num;
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
 		return err;
-- 
1.9.1.286.g5172cb3

Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()

[Junio C Hamano]

blacksimit <cengoguzhanunlu@gmail.com> writes:

> From: Oguzhan Unlu <cengoguzhanunlu@gmail.com>
>
> My solution to make lines containing buffer += a_number; clearer to anyone is following; I defined a new int, magic_num, then assigned lengths of used strings to magic_num and then changed assignment lines through using magic_num so that where the number which is added to buffer is known although I eliminated 3rd parameter of memcmp() when using starts_with().   
Eric told you in $gmane/244637:

> Wrap messages to 65-70 characters.

I do not think replacing 5 (or 7) with a variable makes anything
clearer; in fact, by forcing people to check what the last value
assigned to the variable every time they see "+magic_num", the
resulting code is much harder to read, I would think.

Among the various submissions, I found this one one of the cleaner
solutions:

    http://thread.gmane.org/gmane.comp.version-control.git/244019/focus=244020

and it has been queued as 2d820a61 (fsck.c:fsck_commit(): use
skip_prefix() to verify and skip constant, 2014-03-13) on 'pu'.

> Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>
> ---
>  I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks!
>  fsck.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index d43be98..4e5ca30 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>  	struct commit_graft *graft;
>  	int parents = 0;
>  	int err;
> -
> +    int magic_num;
> +    
> +    magic_num = strlen("tree "); /* magic_num is 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')
> +	if (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\n')
>  		return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
>  	buffer += 46;
> +    magic_num = strlen("parent "); /* magic_num is 7 */
>  	while (starts_with(buffer, "parent ")) {
> -		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
> +		if (get_sha1_hex(buffer+magic_num, sha1) || buffer[47] != '\n')
>  			return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
>  		buffer += 48;
>  		parents++;
> @@ -322,15 +325,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>  		if (p || parents)
>  			return error_func(&commit->object, FSCK_ERROR, "parent objects missing");
>  	}
> +    magic_num = strlen("author "); /* magic_num is 7 */
>  	if (!starts_with(buffer, "author "))
>  		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line");
> -	buffer += 7;
> +	buffer += magic_num;
>  	err = fsck_ident(&buffer, &commit->object, error_func);
>  	if (err)
>  		return err;
> +    magic_num = strlen("committer"); /* magic_num is 7 */
>  	if (!starts_with(buffer, "committer "))
>  		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
> -	buffer += strlen("committer ");
> +	buffer += magic_num;
>  	err = fsck_ident(&buffer, &commit->object, error_func);
>  	if (err)
>  		return err;

Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()

[Matthieu Moy]

blacksimit <cengoguzhanunlu@gmail.com> writes:


> -
> +    int magic_num;
> +    
> +    magic_num = strlen("tree "); /* magic_num is 5 */
>  	if (!starts_with(buffer, "tree "))

Whitespace damage. It seems you have set your tab-width to something
other than 8, and indented with spaces. Please don't do either.

> +    magic_num = strlen("committer"); /* magic_num is 7 */

Typical example of a counter-productive comment. A good comment usually
explains _why_ the code is as it is, and not _what_ it is doing. C is a
much better lanuage than english to describe algorithm, so if you want
magic_num to become equal to 7, then write "magic_num = 7" in code, not
in a comment.

Here, the reader has to spend time and energy to check the
correspondance between the code and the redundant comment ... and see
than they do not match!

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()

[Eric Sunshine]

In addition to the valuable review comments by Junio and Matthieu, see
a few more below...

On Fri, Mar 21, 2014 at 12:37 PM, blacksimit <cengoguzhanunlu@gmail.com> wrote:
> From: Oguzhan Unlu <cengoguzhanunlu@gmail.com>
>
> My solution to make lines containing buffer += a_number; clearer to anyone is following; I defined a new int, magic_num, then assigned lengths of used strings to magic_num and then changed assignment lines through using magic_num so that where the number which is added to buffer is known although I eliminated 3rd parameter of memcmp() when using starts_with().

The process you describe here is effectively what skip_prefix() does
except that you are doing the bookkeeping manually, whereas
skip_prefix() handles the details for you. This should be a good clue
that skip_prefix() is a better solution.

When writing a commit message, try to keep it impersonal; omit
statements like "My solution" and "I defined".

Use imperative mood, which means that the commit message should tell
the code how to change itself. For instance "replace X with Y" or
"retry operation X upon failure".

Let the code speak for itself. It's obvious that skip_prefix() takes
one less argument than memcmp(), so there is no need to talk about it
in the commit message.

Finally, explain why the change is a good thing.

> Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>
> ---
>  I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks!
>  fsck.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index d43be98..4e5ca30 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>         struct commit_graft *graft;
>         int parents = 0;
>         int err;
> -
> +    int magic_num;
> +
> +    magic_num = strlen("tree "); /* magic_num is 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')
> +       if (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\n')
>                 return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
>         buffer += 46;
> +    magic_num = strlen("parent "); /* magic_num is 7 */
>         while (starts_with(buffer, "parent ")) {
> -               if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
> +               if (get_sha1_hex(buffer+magic_num, sha1) || buffer[47] != '\n')
>                         return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
>                 buffer += 48;
>                 parents++;
> @@ -322,15 +325,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>                 if (p || parents)
>                         return error_func(&commit->object, FSCK_ERROR, "parent objects missing");
>         }
> +    magic_num = strlen("author "); /* magic_num is 7 */
>         if (!starts_with(buffer, "author "))
>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line");
> -       buffer += 7;
> +       buffer += magic_num;
>         err = fsck_ident(&buffer, &commit->object, error_func);
>         if (err)
>                 return err;
> +    magic_num = strlen("committer"); /* magic_num is 7 */
>         if (!starts_with(buffer, "committer "))
>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
> -       buffer += strlen("committer ");
> +       buffer += magic_num;
>         err = fsck_ident(&buffer, &commit->object, error_func);
>         if (err)
>                 return err;
> --
> 1.9.1.286.g5172cb3
>