[PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()
From: Oguzhan Unlu cengoguzhanu...@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 cengoguzhanu...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()
blacksimit cengoguzhanu...@gmail.com writes:
From: Oguzhan Unlu cengoguzhanu...@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 cengoguzhanu...@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;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()
blacksimit cengoguzhanu...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()
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 cengoguzhanu...@gmail.com wrote:
From: Oguzhan Unlu cengoguzhanu...@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 cengoguzhanu...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
