{"thread":{"id":"35976","subject":"[PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","startedAt":"2014-02-27T14:20:42Z","lastAt":"2014-02-27T21:34:02Z","messageCount":2,"participants":["Sun He","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235478","messageId":"1393510842-31769-1-git-send-email-sunheehnus@gmail.com","threadId":"35976","inReplyTo":null,"subject":"[PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Sun He","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-02-27T14:20:42Z","receivedAt":"2014-02-27T14:20:42Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"\nSigned-off-by: Sun He <sunheehnus@gmail.com>\n---\n bulk-checkin.c |   10 +++++++++-\n 1 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 118c625..e3c7fb2 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -23,7 +23,8 @@ 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+    char *packname;\n+    struct strbuf sb;\n \tint i;\n \n \tif (!state->f)\n@@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n \t\tclose(fd);\n \t}\n \n+     /* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(\".pack\") */\n+    strbuf_init(&sb,strlen(get_object_directory())+64);\n+    packname = sb.buf;\n+\n \tsprintf(packname, \"%s/pack/pack-\", get_object_directory());\n \tfinish_tmp_packfile(packname, state->pack_tmp_name,\n \t\t\t    state->written, state->nr_written,\n@@ -54,6 +59,9 @@ clear_exit:\n \tfree(state->written);\n \tmemset(state, 0, sizeof(*state));\n \n+    /* release sb space */\n+    strbuf_release(&sb);\n+\n \t/* Make objects we just wrote available to ourselves */\n \treprepare_packed_git();\n }\n-- \n1.7.1\n\n> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.\n\nchar packname[PATH_MAX] (PATH_MAX=4096) in a local function costs too much space, it may cause stack overflow.\nUsing strbuf to deal with packname is more space-friendly.\nI only use the API strbuf_init to alloc enough space for packname. I didn't use other APIs because I want to make the change as little as possible that I don't have to fix other functions which may import new bugs.\nBecause the space spared for sb is on heap, we must release it after it is not useful.\n\n> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.\n\nTht first argument of pack-write.c:finish_tmp_packfile() can be made const because we didn't use *name_buffer to change anything.\n\nCheers,\nHe Sun\n\nPS: \nWhy I cannot sent email to git@vger.kernel.org via my Firefox?\nCan you receive my former emails?\nThanks.\n"},{"id":"235513","messageId":"530FAF4A.1070102@alum.mit.edu","threadId":"35976","inReplyTo":"1393510842-31769-1-git-send-email-sunheehnus@gmail.com","subject":"Re: [PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-02-27T21:34:02Z","receivedAt":"2014-02-27T21:34:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/27/2014 03:20 PM, Sun He wrote:\n> Signed-off-by: Sun He <sunheehnus@gmail.com>\n> ---\n>  bulk-checkin.c |   10 +++++++++-\n>  1 files changed, 9 insertions(+), 1 deletions(-)\n> \n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 118c625..e3c7fb2 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -23,7 +23,8 @@ 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> +    char *packname;\n> +    struct strbuf sb;\n>  \tint i;\n>  \n>  \tif (!state->f)\n> @@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>  \t\tclose(fd);\n>  \t}\n>  \n> +     /* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(\".pack\") */\n> +    strbuf_init(&sb,strlen(get_object_directory())+64);\n> +    packname = sb.buf;\n\nWhy is packname still needed?  Can't you just use sb.buf wherever\npackname was used before?  And then you can also rename \"sb\", which is a\nmostly meaningless name, to \"packname\".\n\n> +\n>  \tsprintf(packname, \"%s/pack/pack-\", get_object_directory());\n\n\n\n>  \tfinish_tmp_packfile(packname, state->pack_tmp_name,\n>  \t\t\t    state->written, state->nr_written,\n> @@ -54,6 +59,9 @@ clear_exit:\n>  \tfree(state->written);\n>  \tmemset(state, 0, sizeof(*state));\n>  \n> +    /* release sb space */\n> +    strbuf_release(&sb);\n> +\n>  \t/* Make objects we just wrote available to ourselves */\n>  \treprepare_packed_git();\n>  }\n> \n\nPlease also adhere to the CodingGuidelines.  Either you or your mailer\nhave used spaces rather than tabs for indentation.  We also usually use\nspaces around \"+\".\n\nThe following comments belong earlier, just after the \"---\" line.\n\n> -- 1.7.1\n>> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.\n> \n> char packname[PATH_MAX] (PATH_MAX=4096) in a local function costs too much space, it may cause stack overflow.\n> Using strbuf to deal with packname is more space-friendly.\n> I only use the API strbuf_init to alloc enough space for packname. I didn't use other APIs because I want to make the change as little as possible that I don't have to fix other functions which may import new bugs.\n> Because the space spared for sb is on heap, we must release it after it is not useful.\n\nSaving stack space is nice, though given that it takes more time to\nallocate space on the heap, it is nonetheless usually preferred to use\nthe stack for temporary variables of this size.\n\nThe problem has more to do with the fact that the old version fixes a\nmaximum length on the buffer, which could be a problem if one is not\ncertain of the length of get_object_directory().\n\nThe other point of strbuf is that you don't have to do the error-prone\nbookkeeping yourself.  So it would be preferable to use strbuf_addf().\n\n>> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.\n> \n> Tht first argument of pack-write.c:finish_tmp_packfile() can be made const because we didn't use *name_buffer to change anything.\n\nSo, why don't you include a second patch making this change?\n\n> Cheers,\n> He Sun\n> \n> PS: \n> Why I cannot sent email to git@vger.kernel.org via my Firefox?\n> Can you receive my former emails?\n> Thanks.\n\nAt least one earlier mail arrived at the list.  You can check the\nmailing list archives at gmane.org to verify things like this.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}