{"thread":{"id":"36274","subject":"[PATCH] Modify fsck_commit. Replace memcmp by skip_prefix","startedAt":"2014-03-22T15:12:12Z","lastAt":"2014-03-24T01:36:38Z","messageCount":5,"participants":["Ashwin Jha","Tanay Abhra","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237393","messageId":"1395501132-12894-1-git-send-email-ajha.dev@gmail.com","threadId":"36274","inReplyTo":null,"subject":"[PATCH] Modify fsck_commit. Replace memcmp by skip_prefix","fromName":"Ashwin Jha","fromEmail":"ajha.dev@gmail.com","sentAt":"2014-03-22T15:12:12Z","receivedAt":"2014-03-22T15:12:12Z","isPatch":true,"sender":{"key":"ajha.dev@gmail.com","avatar":null},"body":"Replace memcmp by skip_prefix as it serves the dual\npurpose of checking the string for a prefix as well\nas skipping that prefix.\n\nSigned-off-by: Ashwin Jha <ajha.dev@gmail.com>\n---\n fsck.c |   25 ++++++++++++++-----------\n 1 file changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 64bf279..021b3fc 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 \"git-compat-util.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -284,21 +285,23 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n \n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tchar *buffer = commit->buffer;\n+\tchar *next_parent, *buffer = commit->buffer;\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n \tint err;\n \n-\tif (memcmp(buffer, \"tree \", 5))\n+\tbuffer = (char *)skip_prefix(buffer, \"tree \");\n+\tif (buffer == NULL)\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, tree_sha1) || buffer[40] != '\\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-\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n+\tbuffer += 41;\n+\twhile ((next_parent = (char *)skip_prefix(buffer, \"parent \")) != NULL) {\n+\t\tbuffer = next_parent;\n+\t\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n')\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n-\t\tbuffer += 48;\n+\t\tbuffer += 41;\n \t\tparents++;\n \t}\n \tgraft = lookup_commit_graft(commit->object.sha1);\n@@ -322,15 +325,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+\tbuffer = (char *)skip_prefix(buffer, \"author \");\n+\tif (buffer == NULL)\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+\tbuffer = (char *)skip_prefix(buffer, \"committer \");\n+\tif (buffer == NULL)\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 \tif (err)\n \t\treturn err;\n-- \n1.7.9.5\n"},{"id":"237411","messageId":"loom.20140323T065455-697@post.gmane.org","threadId":"36274","inReplyTo":"1395501132-12894-1-git-send-email-ajha.dev@gmail.com","subject":"Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-03-23T06:08:19Z","receivedAt":"2014-03-23T06:08:19Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nNit: In subject, patch should be like [PATCH v2] for each increasing order\nof revision.\n\nAlso, after the --- (three dashes), you can write what you changed with each\npatch revision. Also to link the previous patches if you can.\n\nSo the corrected mail subject header will look something like this,\n \n  [PATCH v2] fsck_commit.c: Replace memcmp() by skip_prefix()\n\nCheers,\nTanay Abhra.\n"},{"id":"237413","messageId":"CAPig+cRL3zFWtBq_yJQqwGa7Wc6GsdBqA_jrGm8p5ZMJVmr-yw@mail.gmail.com","threadId":"36274","inReplyTo":"1395501132-12894-1-git-send-email-ajha.dev@gmail.com","subject":"Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-23T08:40:17Z","receivedAt":"2014-03-23T08:40:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the resubmission. Comments below...\n\nOn Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@gmail.com> wrote:\n> Subject: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix\n\nIn his review, Tanay already mentioned indicating a resubmission via\n[PATCH vN]. Additionally, specify the module or function being\nmodified, followed by a colon and space. On this project, it is\ncustomary to forego capitalization in the subject (and only the\nsubject). So, you might write:\n\n    Subject: [PATCH v2] fsck_commit: replace memcmp() by skip_prefix()\n\n> Replace memcmp by skip_prefix as it serves the dual\n> purpose of checking the string for a prefix as well\n> as skipping that prefix.\n\nGood.\n\n> Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>\n> ---\n\nTanay mentioned this already: Below the \"---\" line under your sign-off\nis where you should, as a courtesy to reviewers, explain what changed\nsince the previous attempt. Also, provide a link to the previous\nversion, like this [1].\n\n[1]: http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html\n\n>  fsck.c |   25 ++++++++++++++-----------\n>  1 file changed, 14 insertions(+), 11 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 64bf279..021b3fc 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 \"git-compat-util.h\"\n>\n>  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n>  {\n> @@ -284,21 +285,23 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n>\n>  static int fsck_commit(struct commit *commit, fsck_error error_func)\n>  {\n> -       char *buffer = commit->buffer;\n> +       char *next_parent, *buffer = commit->buffer;\n\nThe name 'next_parent' doesn't seem correct. After invoking\nskip_prefix(), this variable will point at the hex representation of\nthe parent's SHA1, so a better name might be 'parent_id'.\n\n>         unsigned char tree_sha1[20], sha1[20];\n>         struct commit_graft *graft;\n>         int parents = 0;\n>         int err;\n>\n> -       if (memcmp(buffer, \"tree \", 5))\n> +       buffer = (char *)skip_prefix(buffer, \"tree \");\n\nThese casts are ugly. It should be possible to get rid of all of them.\nHint: Does this function modify the memory referenced by 'buffer'?\n(The answer is very slightly involved, though not difficult. A proper\nfix would involve turning this submission into a 2-patch series: one a\npreparatory patch, and the other this patch without the casts.)\n\n> +       if (buffer == NULL)\n\nOn this project, it is customary to say !buffer. Ditto for the\nremaining instances.\n\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, tree_sha1) || buffer[40] != '\\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> -               if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n> +       buffer += 41;\n> +       while ((next_parent = (char *)skip_prefix(buffer, \"parent \")) != NULL) {\n\nLikewise, on this project, it is customary to omit the '!= NULL'.\n\n> +               buffer = next_parent;\n> +               if (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n')\n>                         return error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n> -               buffer += 48;\n> +               buffer += 41;\n>                 parents++;\n>         }\n>         graft = lookup_commit_graft(commit->object.sha1);\n> @@ -322,15 +325,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> +       buffer = (char *)skip_prefix(buffer, \"author \");\n> +       if (buffer == NULL)\n>                 return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n> -       buffer += 7;\n>         err = fsck_ident(&buffer, &commit->object, error_func);\n>         if (err)\n>                 return err;\n> -       if (memcmp(buffer, \"committer \", strlen(\"committer \")))\n> +       buffer = (char *)skip_prefix(buffer, \"committer \");\n> +       if (buffer == NULL)\n>                 return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n> -       buffer += strlen(\"committer \");\n>         err = fsck_ident(&buffer, &commit->object, error_func);\n>         if (err)\n>                 return err;\n\nOther than the minor points mentioned above, the patch looks good. Thanks.\n\n> --\n> 1.7.9.5\n"},{"id":"237416","messageId":"532EDD5F.9020402@gmail.com","threadId":"36274","inReplyTo":"CAPig+cRL3zFWtBq_yJQqwGa7Wc6GsdBqA_jrGm8p5ZMJVmr-yw@mail.gmail.com","subject":"Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix","fromName":"Ashwin Jha","fromEmail":"ajha.dev@gmail.com","sentAt":"2014-03-23T13:10:55Z","receivedAt":"2014-03-23T13:10:55Z","isPatch":true,"sender":{"key":"ajha.dev@gmail.com","avatar":null},"body":"\nThanks for your comments, Tanay and Eric.\n\nRegarding the [PATCH v2], I will keep this in mind for the subsequent \npatches.\n\nOn 03/23/2014 02:10 PM, Eric Sunshine wrote:\n> Thanks for the resubmission. Comments below...\n>\n> On Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@gmail.com> wrote:\n>> Subject: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix\n> In his review, Tanay already mentioned indicating a resubmission via\n> [PATCH vN]. Additionally, specify the module or function being\n> modified, followed by a colon and space. On this project, it is\n> customary to forego capitalization in the subject (and only the\n> subject). So, you might write:\n>\n>      Subject: [PATCH v2] fsck_commit: replace memcmp() by skip_prefix()\n>\n>> Replace memcmp by skip_prefix as it serves the dual\n>> purpose of checking the string for a prefix as well\n>> as skipping that prefix.\n> Good.\n>\n>> Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>\n>> ---\n> Tanay mentioned this already: Below the \"---\" line under your sign-off\n> is where you should, as a courtesy to reviewers, explain what changed\n> since the previous attempt. Also, provide a link to the previous\n> version, like this [1].\n>\n> [1]: http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html\n>\n>>   fsck.c |   25 ++++++++++++++-----------\n>>   1 file changed, 14 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/fsck.c b/fsck.c\n>> index 64bf279..021b3fc 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 \"git-compat-util.h\"\n>>\n>>   static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n>>   {\n>> @@ -284,21 +285,23 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n>>\n>>   static int fsck_commit(struct commit *commit, fsck_error error_func)\n>>   {\n>> -       char *buffer = commit->buffer;\n>> +       char *next_parent, *buffer = commit->buffer;\n> The name 'next_parent' doesn't seem correct. After invoking\n> skip_prefix(), this variable will point at the hex representation of\n> the parent's SHA1, so a better name might be 'parent_id'.\n>\n>>          unsigned char tree_sha1[20], sha1[20];\n>>          struct commit_graft *graft;\n>>          int parents = 0;\n>>          int err;\n>>\n>> -       if (memcmp(buffer, \"tree \", 5))\n>> +       buffer = (char *)skip_prefix(buffer, \"tree \");\n> These casts are ugly. It should be possible to get rid of all of them.\n> Hint: Does this function modify the memory referenced by 'buffer'?\n> (The answer is very slightly involved, though not difficult. A proper\n> fix would involve turning this submission into a 2-patch series: one a\n> preparatory patch, and the other this patch without the casts.)\nEric, certainly this function does not modify the memory referenced by \n'buffer'.\nSo, 'buffer' should be declared as a const char *.\nBut, what about the argument to fsck_ident(). First argument required is \na char **.\nNow, the compiler will correctly give a warning for incorrect argument type.\n\nI have seen the code of fsck_ident(), and it is nowhere modifying the \nmemory referenced by\nbuffer. So, I know that this will not cause any problem. But, still it \nwill be a bad practice to do away with\nwarnings.\n\nAnd can you explain me a bit about a 2-patch series.\n\nOnce again thanks for your help.\n>> +       if (buffer == NULL)\n>>\n>> On this project, it is customary to say !buffer. Ditto for the\n>> remaining instances.\n>>\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, tree_sha1) || buffer[40] != '\\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>> -               if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n>> +       buffer += 41;\n>> +       while ((next_parent = (char *)skip_prefix(buffer, \"parent \")) != NULL) {\n> Likewise, on this project, it is customary to omit the '!= NULL'.\n>\n>> +               buffer = next_parent;\n>> +               if (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n')\n>>                          return error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n>> -               buffer += 48;\n>> +               buffer += 41;\n>>                  parents++;\n>>          }\n>>          graft = lookup_commit_graft(commit->object.sha1);\n>> @@ -322,15 +325,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>> +       buffer = (char *)skip_prefix(buffer, \"author \");\n>> +       if (buffer == NULL)\n>>                  return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n>> -       buffer += 7;\n>>          err = fsck_ident(&buffer, &commit->object, error_func);\n>>          if (err)\n>>                  return err;\n>> -       if (memcmp(buffer, \"committer \", strlen(\"committer \")))\n>> +       buffer = (char *)skip_prefix(buffer, \"committer \");\n>> +       if (buffer == NULL)\n>>                  return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n>> -       buffer += strlen(\"committer \");\n>>          err = fsck_ident(&buffer, &commit->object, error_func);\n>>          if (err)\n>>                  return err;\n> Other than the minor points mentioned above, the patch looks good. Thanks.\n>\n>> --\n>> 1.7.9.5\nRegards,\nAshwin\n"},{"id":"237431","messageId":"CAPig+cSyDAa2r=0dBwwn56Wck7Ci5je6hmQ=U-9ELh9JfwRq6g@mail.gmail.com","threadId":"36274","inReplyTo":"532EDD5F.9020402@gmail.com","subject":"Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-24T01:36:38Z","receivedAt":"2014-03-24T01:36:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 23, 2014 at 9:10 AM, Ashwin Jha <ajha.dev@gmail.com> wrote:\n> On 03/23/2014 02:10 PM, Eric Sunshine wrote:\n>> On Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@gmail.com> wrote:\n>>> -       if (memcmp(buffer, \"tree \", 5))\n>>> +       buffer = (char *)skip_prefix(buffer, \"tree \");\n>>\n>> These casts are ugly. It should be possible to get rid of all of them.\n>> Hint: Does this function modify the memory referenced by 'buffer'?\n>> (The answer is very slightly involved, though not difficult. A proper\n>> fix would involve turning this submission into a 2-patch series: one a\n>> preparatory patch, and the other this patch without the casts.)\n>\n> Eric, certainly this function does not modify the memory referenced by\n> 'buffer'.\n> So, 'buffer' should be declared as a const char *.\n\nCorrect.\n\n> But, what about the argument to fsck_ident(). First argument required is a\n> char **.\n> Now, the compiler will correctly give a warning for incorrect argument type.\n>\n> I have seen the code of fsck_ident(), and it is nowhere modifying the memory\n> referenced by\n> buffer. So, I know that this will not cause any problem. But, still it will\n> be a bad practice to do away with\n> warnings.\n\nYour diagnosis about fsck_ident() is correct: It doesn't modify the\nmemory referenced by its incoming (char **) argument. Therefore, to\navoid casts upon updating fsck_commit() to employ skip_prefix(),\nfsck_ident()'s argument should be decorated with 'const', as well.\nWith that done, the compiler will have no reason to warn.\n\n> And can you explain me a bit about a 2-patch series.\n\nChanges which are conceptually distinct deserve separate patches.\nFixing the const-ness of fsck_ident()'s argument and fsck_commits()'s\n'buffer' variable are distinct from updating fsck_commit() to employ\nskip_prefix(). So, a two-patch series would have:\n\npatch 1/2: add missing 'const' to those two locations\npatch 2/2: use skip_prefix() in place of memcmp()\n\n(It can be argued that fixing const-ness at those two distinct\nlocations also can be split into separate patches, but let's try to\nkeep it simple.)\n\nThe <since> or <revision range> arguments mentioned by \"git\nformat-patch\"'s manual page let you create a multi-patch series, and\nwill correctly number them ([PATCH v3 1/2], etc.). Use the -v option\nto get the v3 in there automatically. Likewise, \"git send-email\" will\nhappily send out the series. Try sending it to yourself first to make\nsure it all works; then send to the mailing list.\n\n> Once again thanks for your help.\n"}]}