{"thread":{"id":"36211","subject":"[PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()","startedAt":"2014-03-18T22:41:52Z","lastAt":"2014-03-19T09:33:02Z","messageCount":5,"participants":["Othman Darraz","Eric Sunshine","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237021","messageId":"1395182512-17179-1-git-send-email-darrazo16@gmail.com","threadId":"36211","inReplyTo":null,"subject":"[PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()","fromName":"Othman Darraz","fromEmail":"darrazo16@gmail.com","sentAt":"2014-03-18T22:41:52Z","receivedAt":"2014-03-18T22:41:52Z","isPatch":true,"sender":{"key":"darrazo16@gmail.com","avatar":null},"body":"use of starts_with() instead of memcmp()\n\nuse of skip_prefix instead of memcmp() and strlen()\n\nSigned-off-by: Othman Darraz \n---\n\nI am planning to apply to GSOC 214\n fsck.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 64bf279..5eae856 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -290,7 +290,7 @@ 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@@ -322,15 +322,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+\tbuffer = (char* )skip_prefix(buffer,\"committer \");\n+\tif (!buffer)\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.9.0.258.g00eda23.dirty\n"},{"id":"237023","messageId":"CAPig+cRoRzZKjW3cY86iQxz9iZ_TKT-yyu=6Va5aV_DsCAQquA@mail.gmail.com","threadId":"36211","inReplyTo":"1395182512-17179-1-git-send-email-darrazo16@gmail.com","subject":"Re: [PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T23:09:06Z","receivedAt":"2014-03-18T23:09:06Z","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 taste of the\nGit review process...\n\nOn Tue, Mar 18, 2014 at 6:41 PM, Othman Darraz wrote:\n> use of starts_with() instead of memcmp()\n>\n> use of skip_prefix instead of memcmp() and strlen()\n\nWrite proper sentences to explain and justify the change; not sentence\nfragments.\n\n> Signed-off-by: Othman Darraz \n> ---\n>\n> I am planning to apply to GSOC 214\n\nYour logic in these changes is rather severely flawed. Running the\ntest suite (t1450, in particular), as the GSoC microproject page\nsuggests, would have clued you in that something was amiss.\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..5eae856 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -290,7 +290,7 @@ 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 reasons for using starts_with() rather than memcmp() is\nthat it allows you to eliminate magic numbers, such as 5. However, if\nyou look closely at this code fragment, you will see that the magic\nnumber is still present in the expression 'buffer+5'. starts_with(),\nmight be a better fit.\n\n> return error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n> @@ -322,15 +322,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\nSame problem here with magic number 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> + buffer = (char* )skip_prefix(buffer,\"committer \");\n\nStyle: (char *)\nStyle: whitespace: skip_prefix(foo, \"bar\")\n\nRegarding the (char *) cast: Is 'buffer' ever modified in this\nfunction? If not, would it make sense to make it 'const' (and fix any\nother small fallout from that change)?\n\n> + if (!buffer)\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> --\n> 1.9.0.258.g00eda23.dirty\n"},{"id":"237024","messageId":"CAPig+cQko2fdCVmNyTufpGW=RRCxVt5G4xdWC7OrQmC9nXfKzg@mail.gmail.com","threadId":"36211","inReplyTo":"CAPig+cRoRzZKjW3cY86iQxz9iZ_TKT-yyu=6Va5aV_DsCAQquA@mail.gmail.com","subject":"Re: [PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T23:12:48Z","receivedAt":"2014-03-18T23:12:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 18, 2014 at 7:09 PM, Eric Sunshine wrote:\n>> diff --git a/fsck.c b/fsck.c\n>> index 64bf279..5eae856 100644\n>> --- a/fsck.c\n>> +++ b/fsck.c\n>> @@ -290,7 +290,7 @@ 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>\n> One of the reasons for using starts_with() rather than memcmp() is\n> that it allows you to eliminate magic numbers, such as 5. However, if\n> you look closely at this code fragment, you will see that the magic\n> number is still present in the expression 'buffer+5'. starts_with(),\n> might be a better fit.\n\nOf course, I meant \"skip_prefix() might be a better fit\".\n"},{"id":"237025","messageId":"5328D3C0.5080009@alum.mit.edu","threadId":"36211","inReplyTo":"CAPig+cRoRzZKjW3cY86iQxz9iZ_TKT-yyu=6Va5aV_DsCAQquA@mail.gmail.com","subject":"Re: [PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-18T23:16:16Z","receivedAt":"2014-03-18T23:16:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/19/2014 12:09 AM, Eric Sunshine wrote:\n> Thanks for the submission. Comments below to give you a taste of the\n> Git review process...\n> \n> On Tue, Mar 18, 2014 at 6:41 PM, Othman Darraz wrote:\n>> use of starts_with() instead of memcmp()\n>>\n>> use of skip_prefix instead of memcmp() and strlen()\n> \n> Write proper sentences to explain and justify the change; not sentence\n> fragments.\n> \n>> Signed-off-by: Othman Darraz \n>> ---\n>>\n>> I am planning to apply to GSOC 214\n> \n> Your logic in these changes is rather severely flawed. Running the\n> test suite (t1450, in particular), as the GSoC microproject page\n> suggests, would have clued you in that something was amiss.\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..5eae856 100644\n>> --- a/fsck.c\n>> +++ b/fsck.c\n>> @@ -290,7 +290,7 @@ 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> \n> One of the reasons for using starts_with() rather than memcmp() is\n> that it allows you to eliminate magic numbers, such as 5. However, if\n> you look closely at this code fragment, you will see that the magic\n> number is still present in the expression 'buffer+5'. starts_with(),\n> might be a better fit.\n\nEric, I think you meant to say that *skip_prefix()* might be a better fit.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"237052","messageId":"CAPig+cQ=3tjC47FgPRFkDW1dgE9p1uKsAiZ6uZoNXR0mBMjuBA@mail.gmail.com","threadId":"36211","inReplyTo":"CAOfd7q0p5gtaPMuds8LkO8q3FJnAJgb_2-6cKeNiqFqEvP52Ww@mail.gmail.com","subject":"Re: [PATCH] fsck.c:fsck_commit() use starts_with() and skip_prefix()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-19T09:33:02Z","receivedAt":"2014-03-19T09:33:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 19, 2014 at 5:04 AM, Othman Darraz wrote:\n> 2014-03-19 0:12 GMT+01:00 Eric Sunshine :\n>> On Tue, Mar 18, 2014 at 7:09 PM, Eric Sunshine \n>> wrote:\n>>\n>> >> diff --git a/fsck.c b/fsck.c\n>> >> index 64bf279..5eae856 100644\n>> >> --- a/fsck.c\n>> >> +++ b/fsck.c\n>> >> @@ -290,7 +290,7 @@ static int fsck_commit(struct commit *commit,\n>> >> 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\n>> >> format - expected 'tree' line\");\n>> >> if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n>> >\n>> > One of the reasons for using starts_with() rather than memcmp() is\n>> > that it allows you to eliminate magic numbers, such as 5. However, if\n>> > you look closely at this code fragment, you will see that the magic\n>> > number is still present in the expression 'buffer+5'. starts_with(),\n>> > might be a better fit.\n>>\n>> Of course, I meant \"skip_prefix() might be a better fit\".\n>\n> Thank you for your review and feedbacks.\n> Actually I made a mistake because I misunderstood how to run the tests, I\n> was using the wrong Makefile.\n\nFor quick initial testing, you can just run the single test script.\nFor instance:\n\n (cd t && ./t1450-fsck.sh)\n\nOnce you have that running correctly, then run the entire test suite\nto ensure that your changes didn't break anything else.\n\n> Secondly I think , as well, that skip_prefix()\n> is a better fit. Nevertheless, as the variable buffer change in this\n> function, using skip_prefix() implies the use of cast.\n\nYes, the variable named 'buffer' changes, but does the content of the\nmemory referenced by 'buffer' ever change? If not, then 'buffer' could\nbe made 'const', thus eliminating the need for the cast. (Note that\nchanging it to 'const' might involve a bit of extra work, but the\nquestion is still pertinent.)\n\n> I will make the\n> changes right now.\n>\n> Thank you for your time.\n>\n> Othman DARRAZ\n"}]}