{"thread":{"id":"36243","subject":"[PATCH] Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()","startedAt":"2014-03-20T05:48:28Z","lastAt":"2014-03-21T04:18:03Z","messageCount":2,"participants":["blacksimit","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237144","messageId":"1395294508-18301-1-git-send-email-cengoguzhanunlu@gmail.com","threadId":"36243","inReplyTo":null,"subject":"[PATCH] Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()","fromName":"blacksimit","fromEmail":"cengoguzhanunlu@gmail.com","sentAt":"2014-03-20T05:48:28Z","receivedAt":"2014-03-20T05:48:28Z","isPatch":true,"sender":{"key":"cengoguzhanunlu@gmail.com","avatar":null},"body":"Hi, I've done a microproject, number 15, \"Rewrite fsck.c:fsck_commit() to use starts_with() and/or skip_prefix().\" I used only starts_with().\n\nmemcmp() returns 0 when both are equal, therefore when replacing with starts_with() , I used \"!\" or deleted where appropriate.\n\nI plan to apply for the GSoC 2014. I expect your feedbacks!\n\nSigned-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n---\n fsck.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 64bf279..d43be98 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -290,12 +290,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,13 +322,13 @@ 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 \terr = fsck_ident(&buffer, &commit->object, error_func);\n-- \n1.9.1.286.g5172cb3\n"},{"id":"237260","messageId":"CAPig+cQtSxFNPGx5N5JhTtKiwi6DZAx5MUXJfxw6CNe7r0Z5cA@mail.gmail.com","threadId":"36243","inReplyTo":"1395294508-18301-1-git-send-email-cengoguzhanunlu@gmail.com","subject":"Re: [PATCH] Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T04:18:03Z","receivedAt":"2014-03-21T04:18:03Z","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 1:48 AM, blacksimit <cengoguzhanunlu@gmail.com> wrote:\n> Subject: Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()\n\nUse imperative mood (\"rewrite\" instead of \"rewritten\"). Keep it short\nbut informative. Possibly like this:\n\n    Subject: fsck_commit: replace memcmp() with starts_with()\n\n> Hi, I've done a microproject, number 15, \"Rewrite fsck.c:fsck_commit() to use starts_with() and/or skip_prefix().\" I used only starts_with().\n\nWrap messages to 65-70 characters.\n\nUse the commit message for explaining and justifying the change.\nCommentary about GSoC and the fact that you avoided skip_prefix()\nwon't be meaningful to someone reading the commit message in the\npermanent project history months or years from now. Place such\ncommentary after the \"---\" line just below your sign-off.\n\n> memcmp() returns 0 when both are equal, therefore when replacing with starts_with() , I used \"!\" or deleted where appropriate.\n\nGood. Some applicants have tripped over this. Nevertheless, it's not\nnecessary to mention this in either the commit message or the\ncommentary below the \"---\" line since the inversion is an obvious\nrequirement of using starts_with() in place of memcmp().\n\n> I plan to apply for the GSoC 2014. I expect your feedbacks!\n\nPlace this below the \"---\" line after your sign-off.\n\n> Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n> ---\n>  fsck.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 64bf279..d43be98 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -290,12 +290,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,13 +322,13 @@ 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\nThis is a similar case. You eliminated the strlen(\"committer \") in the\nline you changed, but then it appears again a couple lines below. This\ncan be improved.\n\n>         err = fsck_ident(&buffer, &commit->object, error_func);\n> --\n> 1.9.1.286.g5172cb3\n"}]}