{"thread":{"id":"36261","subject":"[PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()","startedAt":"2014-03-21T01:54:46Z","lastAt":"2014-03-21T15:42:34Z","messageCount":3,"participants":["Ashwin Jha","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237252","messageId":"1395366886-16839-1-git-send-email-ajha.dev@gmail.com","threadId":"36261","inReplyTo":null,"subject":"[PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()","fromName":"Ashwin Jha","fromEmail":"ajha.dev@gmail.com","sentAt":"2014-03-21T01:54:46Z","receivedAt":"2014-03-21T01:54:46Z","isPatch":true,"sender":{"key":"ajha.dev@gmail.com","avatar":null},"body":"modified fsck.c:fsck_commit(). Replaced memcmp() with starts_with() function.\nstarts_with() seems much more relevant than memcmp(). It uses one less argument\nand its return value makes more sense.\nskip_prefix() is not used as it uses strcmp() internally which seems unnecessarily\nfor current task. The current task can be easily done by providing offsets to the\nbuffer pointer (the way it is implemented currently).\n\nSigned-off-by: Ashwin Jha \n---\n fsck.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 64bf279..82e1640 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"strbuf.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -290,12 +291,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tint parents = 0;\n \tint err;\n \n-\tif (memcmp(buffer, \"tree \", 5))\n+\tif (!starts_with(buffer, \"tree \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n \tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n+\twhile (starts_with(buffer, \"parent \")) {\n \t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n \t\tbuffer += 48;\n@@ -322,15 +323,15 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \t\tif (p || parents)\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n \t}\n-\tif (memcmp(buffer, \"author \", 7))\n+\tif (!starts_with(buffer, \"author \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n \tbuffer += 7;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-\tif (memcmp(buffer, \"committer \", strlen(\"committer \")))\n+\tif (!starts_with(buffer, \"committer \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n-\tbuffer += strlen(\"committer \");\n+\tbuffer += 10;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-- \n1.7.9.5\n"},{"id":"237254","messageId":"CAPig+cTr9euE8=kMjVxRjvWPPVRQsF3i7TOevVPU+akSvCyJYg@mail.gmail.com","threadId":"36261","inReplyTo":"1395366886-16839-1-git-send-email-ajha.dev@gmail.com","subject":"Re: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T03:33:59Z","receivedAt":"2014-03-21T03:33:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the submission. Comments below to give you a feel for the\nGit review process...\n\nOn Thu, Mar 20, 2014 at 9:54 PM, Ashwin Jha wrote:\n> Subject: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()\n\nThe subject becomes part of the permanent Git history, but the fact\nthat this is a GSoC submission won't be meaningful to anyone months or\nyears from now. You can mention GSoC inside [...], however, as in\n[PATCH GSoC], since that gets stripped off the subject automatically\nwhen the patch is applied. Use the commentary section after the \"---\"\nline just below your sign-off to explain that this is microproject 15.\n\nThe subject itself should concisely summarize the change. \"Rewrite\nfsck.c:fsck_commit()\" doesn't say much. You might say instead:\n\n Subject: fsk_commit: replace memcmp() with starts_with()\n\n> modified fsck.c:fsck_commit(). Replaced memcmp() with starts_with() function.\n\nCapitalize start of sentence.\n\nUse imperative mood: \"modify\" rather than \"modified\"; \"Replace\" rather\nthan \"Replaced\".\n\n> starts_with() seems much more relevant than memcmp(). It uses one less argument\n> and its return value makes more sense.\n\nAs a justification, \"uses one less argument\" falls flat, and really\nhas nothing to do with the decision to make the change. The bit about\nthe return value is a slightly better but is still weak. You might\ninstead justify the change by pointing out that the name starts_with()\ndoes a better job of conveying the intention of the code, which is to\ncheck the string for a prefix, than does memcmp().\n\n> skip_prefix() is not used as it uses strcmp() internally which seems unnecessarily\n> for current task. The current task can be easily done by providing offsets to the\n> buffer pointer (the way it is implemented currently).\n\nNot sure what this means. What is the \"current task\", and what is\nimplemented where currently?\n\n> Signed-off-by: Ashwin Jha \n> ---\n> fsck.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 64bf279..82e1640 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -6,6 +6,7 @@\n> #include \"commit.h\"\n> #include \"tag.h\"\n> #include \"fsck.h\"\n> +#include \"strbuf.h\"\n>\n> static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n> {\n> @@ -290,12 +291,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n> int parents = 0;\n> int err;\n>\n> - if (memcmp(buffer, \"tree \", 5))\n> + if (!starts_with(buffer, \"tree \"))\n> return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n> if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n\nOne of the benefits of starts_with() and skip_prefix() is that they\nallow you to eliminate magic numbers, such as 5 in the memcmp()\ninvocation. However, if you look a couple lines below, you see in the\nexpression 'buffer+5' that the magic number is still present. In fact,\nthe code becomes less clear with your change because the 5 in\n'buffer+5' is much more mysterious without the preceding\nmemcmp(foo,\"bar\",5). It is possible to eliminate this magic number,\nbut starts_with() is not the answer.\n\n> return error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n> buffer += 46;\n> - while (!memcmp(buffer, \"parent \", 7)) {\n> + while (starts_with(buffer, \"parent \")) {\n> if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n\nDitto here with magic number 7 in 'buffer+7'.\n\n> return error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n> buffer += 48;\n> @@ -322,15 +323,15 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n> if (p || parents)\n> return error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n> }\n> - if (memcmp(buffer, \"author \", 7))\n> + if (!starts_with(buffer, \"author \"))\n> return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n> buffer += 7;\n\nAnd again with 7.\n\n> err = fsck_ident(&buffer, &commit->object, error_func);\n> if (err)\n> return err;\n> - if (memcmp(buffer, \"committer \", strlen(\"committer \")))\n> + if (!starts_with(buffer, \"committer \"))\n> return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n> - buffer += strlen(\"committer \");\n> + buffer += 10;\n\nAgain with 10 (newly introduced).\n\n> err = fsck_ident(&buffer, &commit->object, error_func);\n> if (err)\n> return err;\n> --\n> 1.7.9.5\n"},{"id":"237314","messageId":"532C5DEA.1030006@gmail.com","threadId":"36261","inReplyTo":"1395366886-16839-1-git-send-email-ajha.dev@gmail.com","subject":"Re: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()","fromName":"Ashwin Jha","fromEmail":"ajha.dev@gmail.com","sentAt":"2014-03-21T15:42:34Z","receivedAt":"2014-03-21T15:42:34Z","isPatch":true,"sender":{"key":"ajha.dev@gmail.com","avatar":null},"body":"\n-------- Original Message --------\nSubject: \t[PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()\nDate: \tFri, 21 Mar 2014 07:24:46 +0530\nFrom: \tAshwin Jha \nTo: \tgit@vger.kernel.org\nCC: \tAshwin Jha \n\n\n\nmodified fsck.c:fsck_commit(). Replaced memcmp() with starts_with() function.\nstarts_with() seems much more relevant than memcmp(). It uses one less argument\nand its return value makes more sense.\nskip_prefix() is not used as it uses strcmp() internally which seems unnecessarily\nfor current task. The current task can be easily done by providing offsets to the\nbuffer pointer (the way it is implemented currently).\n\nSigned-off-by: Ashwin Jha \n---\n fsck.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 64bf279..82e1640 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"strbuf.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -290,12 +291,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tint parents = 0;\n \tint err;\n \n-\tif (memcmp(buffer, \"tree \", 5))\n+\tif (!starts_with(buffer, \"tree \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n \tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n+\twhile (starts_with(buffer, \"parent \")) {\n \t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n \t\tbuffer += 48;\n@@ -322,15 +323,15 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \t\tif (p || parents)\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n \t}\n-\tif (memcmp(buffer, \"author \", 7))\n+\tif (!starts_with(buffer, \"author \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n \tbuffer += 7;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-\tif (memcmp(buffer, \"committer \", strlen(\"committer \")))\n+\tif (!starts_with(buffer, \"committer \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n-\tbuffer += strlen(\"committer \");\n+\tbuffer += 10;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-- \n1.7.9.5\n"}]}