{"thread":{"id":"36267","subject":"[PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()","startedAt":"2014-03-21T16:37:18Z","lastAt":"2014-03-21T19:33:44Z","messageCount":4,"participants":["blacksimit","Junio C Hamano","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"237320","messageId":"1395419838-25527-1-git-send-email-cengoguzhanunlu@gmail.com","threadId":"36267","inReplyTo":null,"subject":"[PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()","fromName":"blacksimit","fromEmail":"cengoguzhanunlu@gmail.com","sentAt":"2014-03-21T16:37:18Z","receivedAt":"2014-03-21T16:37:18Z","isPatch":true,"sender":{"key":"cengoguzhanunlu@gmail.com","avatar":null},"body":"From: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n\nMy 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().   \n\nSigned-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n---\n I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks!\n fsck.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex d43be98..4e5ca30 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tstruct commit_graft *graft;\n \tint parents = 0;\n \tint err;\n-\n+    int magic_num;\n+    \n+    magic_num = strlen(\"tree \"); /* magic_num is 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+\tif (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\\n')\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n \tbuffer += 46;\n+    magic_num = strlen(\"parent \"); /* magic_num is 7 */\n \twhile (starts_with(buffer, \"parent \")) {\n-\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n+\t\tif (get_sha1_hex(buffer+magic_num, 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 \t\tparents++;\n@@ -322,15 +325,17 @@ 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+    magic_num = strlen(\"author \"); /* magic_num is 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+\tbuffer += magic_num;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n+    magic_num = strlen(\"committer\"); /* magic_num is 7 */\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 += magic_num;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-- \n1.9.1.286.g5172cb3\n"},{"id":"237344","messageId":"xmqqmwgjtmsf.fsf@gitster.dls.corp.google.com","threadId":"36267","inReplyTo":"1395419838-25527-1-git-send-email-cengoguzhanunlu@gmail.com","subject":"Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-21T18:43:44Z","receivedAt":"2014-03-21T18:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"blacksimit <cengoguzhanunlu@gmail.com> writes:\n\n> From: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n>\n> 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().   \nEric told you in $gmane/244637:\n\n> Wrap messages to 65-70 characters.\n\nI do not think replacing 5 (or 7) with a variable makes anything\nclearer; in fact, by forcing people to check what the last value\nassigned to the variable every time they see \"+magic_num\", the\nresulting code is much harder to read, I would think.\n\nAmong the various submissions, I found this one one of the cleaner\nsolutions:\n\n    http://thread.gmane.org/gmane.comp.version-control.git/244019/focus=244020\n\nand it has been queued as 2d820a61 (fsck.c:fsck_commit(): use\nskip_prefix() to verify and skip constant, 2014-03-13) on 'pu'.\n\n> Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n> ---\n>  I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks!\n>  fsck.c | 15 ++++++++++-----\n>  1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index d43be98..4e5ca30 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n>  \tstruct commit_graft *graft;\n>  \tint parents = 0;\n>  \tint err;\n> -\n> +    int magic_num;\n> +    \n> +    magic_num = strlen(\"tree \"); /* magic_num is 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> +\tif (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\\n')\n>  \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n>  \tbuffer += 46;\n> +    magic_num = strlen(\"parent \"); /* magic_num is 7 */\n>  \twhile (starts_with(buffer, \"parent \")) {\n> -\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n> +\t\tif (get_sha1_hex(buffer+magic_num, 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>  \t\tparents++;\n> @@ -322,15 +325,17 @@ 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> +    magic_num = strlen(\"author \"); /* magic_num is 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> +\tbuffer += magic_num;\n>  \terr = fsck_ident(&buffer, &commit->object, error_func);\n>  \tif (err)\n>  \t\treturn err;\n> +    magic_num = strlen(\"committer\"); /* magic_num is 7 */\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 += magic_num;\n>  \terr = fsck_ident(&buffer, &commit->object, error_func);\n>  \tif (err)\n>  \t\treturn err;\n"},{"id":"237349","messageId":"vpqk3bngyuy.fsf@anie.imag.fr","threadId":"36267","inReplyTo":"1395419838-25527-1-git-send-email-cengoguzhanunlu@gmail.com","subject":"Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-03-21T19:01:25Z","receivedAt":"2014-03-21T19:01:25Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"blacksimit <cengoguzhanunlu@gmail.com> writes:\n\n\n> -\n> +    int magic_num;\n> +    \n> +    magic_num = strlen(\"tree \"); /* magic_num is 5 */\n>  \tif (!starts_with(buffer, \"tree \"))\n\nWhitespace damage. It seems you have set your tab-width to something\nother than 8, and indented with spaces. Please don't do either.\n\n> +    magic_num = strlen(\"committer\"); /* magic_num is 7 */\n\nTypical example of a counter-productive comment. A good comment usually\nexplains _why_ the code is as it is, and not _what_ it is doing. C is a\nmuch better lanuage than english to describe algorithm, so if you want\nmagic_num to become equal to 7, then write \"magic_num = 7\" in code, not\nin a comment.\n\nHere, the reader has to spend time and energy to check the\ncorrespondance between the code and the redundant comment ... and see\nthan they do not match!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"237352","messageId":"CAPig+cRe-L5QHSVu=U9EWTeoCsjS++TGZvu9ZZSss7_mBVX0AA@mail.gmail.com","threadId":"36267","inReplyTo":"1395419838-25527-1-git-send-email-cengoguzhanunlu@gmail.com","subject":"Re: [PATCH v2] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T19:33:44Z","receivedAt":"2014-03-21T19:33:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In addition to the valuable review comments by Junio and Matthieu, see\na few more below...\n\nOn Fri, Mar 21, 2014 at 12:37 PM, blacksimit <cengoguzhanunlu@gmail.com> wrote:\n> From: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n>\n> 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().\n\nThe process you describe here is effectively what skip_prefix() does\nexcept that you are doing the bookkeeping manually, whereas\nskip_prefix() handles the details for you. This should be a good clue\nthat skip_prefix() is a better solution.\n\nWhen writing a commit message, try to keep it impersonal; omit\nstatements like \"My solution\" and \"I defined\".\n\nUse imperative mood, which means that the commit message should tell\nthe code how to change itself. For instance \"replace X with Y\" or\n\"retry operation X upon failure\".\n\nLet the code speak for itself. It's obvious that skip_prefix() takes\none less argument than memcmp(), so there is no need to talk about it\nin the commit message.\n\nFinally, explain why the change is a good thing.\n\n> Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>\n> ---\n>  I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks!\n>  fsck.c | 15 ++++++++++-----\n>  1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index d43be98..4e5ca30 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n>         struct commit_graft *graft;\n>         int parents = 0;\n>         int err;\n> -\n> +    int magic_num;\n> +\n> +    magic_num = strlen(\"tree \"); /* magic_num is 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> +       if (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\\n')\n>                 return error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n>         buffer += 46;\n> +    magic_num = strlen(\"parent \"); /* magic_num is 7 */\n>         while (starts_with(buffer, \"parent \")) {\n> -               if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n> +               if (get_sha1_hex(buffer+magic_num, sha1) || buffer[47] != '\\n')\n>                         return error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n>                 buffer += 48;\n>                 parents++;\n> @@ -322,15 +325,17 @@ 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> +    magic_num = strlen(\"author \"); /* magic_num is 7 */\n>         if (!starts_with(buffer, \"author \"))\n>                 return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n> -       buffer += 7;\n> +       buffer += magic_num;\n>         err = fsck_ident(&buffer, &commit->object, error_func);\n>         if (err)\n>                 return err;\n> +    magic_num = strlen(\"committer\"); /* magic_num is 7 */\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 += magic_num;\n>         err = fsck_ident(&buffer, &commit->object, error_func);\n>         if (err)\n>                 return err;\n> --\n> 1.9.1.286.g5172cb3\n>\n"}]}