{"thread":{"id":"35981","subject":"[PATCH] GSoC2014 Microproject rewrite finish_bulk_checkin()","startedAt":"2014-02-27T19:02:20Z","lastAt":"2014-02-27T22:35:00Z","messageCount":3,"participants":["Faiz Kothari","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235489","messageId":"1393527740-16765-1-git-send-email-faiz.off93@gmail.com","threadId":"35981","inReplyTo":null,"subject":"[PATCH] GSoC2014 Microproject rewrite finish_bulk_checkin()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-02-27T19:02:20Z","receivedAt":"2014-02-27T19:02:20Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n---\n bulk-checkin.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 118c625..feeff9f 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n static void finish_bulk_checkin(struct bulk_checkin_state *state)\n {\n \tunsigned char sha1[20];\n-\tchar packname[PATH_MAX];\n+\tstruct strbuf packname;\n \tint i;\n \n \tif (!state->f)\n@@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n \t\t\t\t\t state->offset);\n \t\tclose(fd);\n \t}\n-\n-\tsprintf(packname, \"%s/pack/pack-\", get_object_directory());\n-\tfinish_tmp_packfile(packname, state->pack_tmp_name,\n+\t\n+\tpackname.len = packname.alloc = 64 + strlen(get_object_directory());\n+\tpackname.buf = (char *)malloc(packname.len * sizeof(char));\n+\tsprintf(packname.buf, \"%s/pack/pack-\", get_object_directory());\n+\tfinish_tmp_packfile(packname.buf, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n \t\t\t &state->pack_idx_opts, sha1);\n \tfor (i = 0; i < state->nr_written; i++)\n@@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n clear_exit:\n \tfree(state->written);\n \tmemset(state, 0, sizeof(*state));\n-\n+\tfree(packname.buf);\n \t/* Make objects we just wrote available to ourselves */\n \treprepare_packed_git();\n }\n-- \n1.7.9.5\n\n> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.\n> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.\n\nAdding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) and itself.\nUsing the APIs for strbuf is giving me test failures(12/15) during t1050-large.sh \nSo, I used the malloc() and free() instead.\nInstead of having packname on stack and cause stackoverflow because of MAX_PATH ~ 4KB, have it on heap.\nCan have first parameter to pack-write.c:finish_tmp_packfile() as const because packname is not required to be modified.\n\nI apologise for my two earlier patches not being in proper format. I have finally got it working properly. Will make sure,\nit does not happen again.\n"},{"id":"235519","messageId":"530FB96F.5080406@alum.mit.edu","threadId":"35981","inReplyTo":"1393527740-16765-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] GSoC2014 Microproject rewrite finish_bulk_checkin()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-02-27T22:17:19Z","receivedAt":"2014-02-27T22:17:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/27/2014 08:02 PM, Faiz Kothari wrote:\n> Signed-off-by: Faiz Kothari \n> ---\n> bulk-checkin.c | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 118c625..feeff9f 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n> static void finish_bulk_checkin(struct bulk_checkin_state *state)\n> {\n> \tunsigned char sha1[20];\n> -\tchar packname[PATH_MAX];\n> +\tstruct strbuf packname;\n> \tint i;\n> \n> \tif (!state->f)\n> @@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n> \t\t\t\t\t state->offset);\n> \t\tclose(fd);\n> \t}\n> -\n> -\tsprintf(packname, \"%s/pack/pack-\", get_object_directory());\n> -\tfinish_tmp_packfile(packname, state->pack_tmp_name,\n> +\t\n> +\tpackname.len = packname.alloc = 64 + strlen(get_object_directory());\n> +\tpackname.buf = (char *)malloc(packname.len * sizeof(char));\n> +\tsprintf(packname.buf, \"%s/pack/pack-\", get_object_directory());\n> +\tfinish_tmp_packfile(packname.buf, state->pack_tmp_name,\n> \t\t\t state->written, state->nr_written,\n> \t\t\t &state->pack_idx_opts, sha1);\n> \tfor (i = 0; i < state->nr_written; i++)\n> @@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n> clear_exit:\n> \tfree(state->written);\n> \tmemset(state, 0, sizeof(*state));\n> -\n> +\tfree(packname.buf);\n> \t/* Make objects we just wrote available to ourselves */\n> \treprepare_packed_git();\n> }\n> -- 1.7.9.5\n>> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.\n>> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.\n> \n> Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) and itself.\n> Using the APIs for strbuf is giving me test failures(12/15) during t1050-large.sh \n> So, I used the malloc() and free() instead.\n\nThis is not OK. I promise you, the strbuf API works correctly if it is\nused correctly. (And if it really *were* broken, you should fix the\nproblem or at least diagnose and document it rather than working around it.)\n\n> Instead of having packname on stack and cause stackoverflow because of MAX_PATH ~ 4KB, have it on heap.\n> Can have first parameter to pack-write.c:finish_tmp_packfile() as const because packname is not required to be modified.\n> \n> I apologise for my two earlier patches not being in proper format. I have finally got it working properly. Will make sure,\n> it does not happen again.\n\nAlmost. This last set of comments should be moved to directly after the\n\"---\" line.\n\nBut: please rather stick to *one* microproject and get it perfect, and\nleave the others to other students.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"235522","messageId":"CAFbjVcm2us9QfR4qfr14MxRUmv03RN1C5NjyYVzVRw7bKoNrEg@mail.gmail.com","threadId":"35981","inReplyTo":"530FB96F.5080406@alum.mit.edu","subject":"Re: [PATCH] GSoC2014 Microproject rewrite finish_bulk_checkin()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-02-27T22:35:00Z","receivedAt":"2014-02-27T22:35:00Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Hi,\nThanks for the remarks.\nI'll stick to this micro project and follow the guidelines.\nYes, the strbuf API is perfectly OK. I was not getting to work it\nproperly, so I used malloc() / free() instead. My bad.\nI'll resubmit the patch.\nThanks.\n\nOn Fri, Feb 28, 2014 at 3:47 AM, Michael Haggerty wrote:\n> On 02/27/2014 08:02 PM, Faiz Kothari wrote:\n>> Signed-off-by: Faiz Kothari \n>> ---\n>> bulk-checkin.c | 12 +++++++-----\n>> 1 file changed, 7 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/bulk-checkin.c b/bulk-checkin.c\n>> index 118c625..feeff9f 100644\n>> --- a/bulk-checkin.c\n>> +++ b/bulk-checkin.c\n>> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {\n>> static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>> {\n>> unsigned char sha1[20];\n>> - char packname[PATH_MAX];\n>> + struct strbuf packname;\n>> int i;\n>>\n>> if (!state->f)\n>> @@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>> state->offset);\n>> close(fd);\n>> }\n>> -\n>> - sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n>> - finish_tmp_packfile(packname, state->pack_tmp_name,\n>> +\n>> + packname.len = packname.alloc = 64 + strlen(get_object_directory());\n>> + packname.buf = (char *)malloc(packname.len * sizeof(char));\n>> + sprintf(packname.buf, \"%s/pack/pack-\", get_object_directory());\n>> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,\n>> state->written, state->nr_written,\n>> &state->pack_idx_opts, sha1);\n>> for (i = 0; i < state->nr_written; i++)\n>> @@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>> clear_exit:\n>> free(state->written);\n>> memset(state, 0, sizeof(*state));\n>> -\n>> + free(packname.buf);\n>> /* Make objects we just wrote available to ourselves */\n>> reprepare_packed_git();\n>> }\n>> -- 1.7.9.5\n>>> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.\n>>> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.\n>>\n>> Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) and itself.\n>> Using the APIs for strbuf is giving me test failures(12/15) during t1050-large.sh\n>> So, I used the malloc() and free() instead.\n>\n> This is not OK. I promise you, the strbuf API works correctly if it is\n> used correctly. (And if it really *were* broken, you should fix the\n> problem or at least diagnose and document it rather than working around it.)\n>\n>> Instead of having packname on stack and cause stackoverflow because of MAX_PATH ~ 4KB, have it on heap.\n>> Can have first parameter to pack-write.c:finish_tmp_packfile() as const because packname is not required to be modified.\n>>\n>> I apologise for my two earlier patches not being in proper format. I have finally got it working properly. Will make sure,\n>> it does not happen again.\n>\n> Almost. This last set of comments should be moved to directly after the\n> \"---\" line.\n>\n> But: please rather stick to *one* microproject and get it perfect, and\n> leave the others to other students.\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"}]}