{"thread":{"id":"35990","subject":"[PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","startedAt":"2014-02-28T08:28:24Z","lastAt":"2014-03-01T00:13:40Z","messageCount":8,"participants":["Sun He","Eric Sunshine","孙赫","He Sun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235555","messageId":"1393576104-1758-1-git-send-email-sunheehnus@gmail.com","threadId":"35990","inReplyTo":null,"subject":"[PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Sun He","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-02-28T08:28:24Z","receivedAt":"2014-02-28T08:28:24Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"Signed-off-by: Sun He \n---\n builtin/pack-objects.c | 17 +++++++----------\n bulk-checkin.c | 8 +++++---\n pack-write.c | 20 ++++++++++++--------\n pack.h | 2 +-\n 4 files changed, 25 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c733379..72fb82b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -803,7 +803,7 @@ static void write_pack_file(void)\n \n \t\tif (!pack_to_stdout) {\n \t\t\tstruct stat st;\n-\t\t\tchar tmpname[PATH_MAX];\n+\t\t\tstruct strbuf tmpname = STRBUF_INIT;\n \n \t\t\t/*\n \t\t\t * Packs are runtime accessed in their mtime\n@@ -823,26 +823,22 @@ static void write_pack_file(void)\n \t\t\t\tutb.modtime = --last_mtime;\n \t\t\t\tif (utime(pack_tmp_name, &utb) < 0)\n \t\t\t\t\twarning(\"failed utime() on %s: %s\",\n-\t\t\t\t\t\ttmpname, strerror(errno));\n+\t\t\t\t\t\tpack_tmp_name, strerror(errno));\n \t\t\t}\n \n-\t\t\t/* Enough space for \"-.pack\"? */\n-\t\t\tif (sizeof(tmpname) <= strlen(base_name) + 50)\n-\t\t\t\tdie(\"pack base name '%s' too long\", base_name);\n-\t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-\", base_name);\n+\t\t\tstrbuf_addf(&tmpname, \"%s-\", base_name);\n \n \t\t\tif (write_bitmap_index) {\n \t\t\t\tbitmap_writer_set_checksum(sha1);\n \t\t\t\tbitmap_writer_build_type_index(written_list, nr_written);\n \t\t\t}\n \n-\t\t\tfinish_tmp_packfile(tmpname, pack_tmp_name,\n+\t\t\tfinish_tmp_packfile(&tmpname, pack_tmp_name,\n \t\t\t\t\t written_list, nr_written,\n \t\t\t\t\t &pack_idx_opts, sha1);\n \n \t\t\tif (write_bitmap_index) {\n-\t\t\t\tchar *end_of_name_prefix = strrchr(tmpname, 0);\n-\t\t\t\tsprintf(end_of_name_prefix, \"%s.bitmap\", sha1_to_hex(sha1));\n+\t\t\t\tstrbuf_addf(&tmpname, \"%s.bitmap\" ,sha1_to_hex(sha1));\n \n \t\t\t\tstop_progress(&progress_state);\n \n@@ -851,10 +847,11 @@ static void write_pack_file(void)\n \t\t\t\tbitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);\n \t\t\t\tbitmap_writer_build(&to_pack);\n \t\t\t\tbitmap_writer_finish(written_list, nr_written,\n-\t\t\t\t\t\t tmpname, write_bitmap_options);\n+\t\t\t\t\t\t tmpname.buf, write_bitmap_options);\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t}\n \n+\t\t\tstrbuf_release(&tmpname);\n \t\t\tfree(pack_tmp_name);\n \t\t\tputs(sha1_to_hex(sha1));\n \t\t}\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 118c625..98e651c 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -4,6 +4,7 @@\n #include \"bulk-checkin.h\"\n #include \"csum-file.h\"\n #include \"pack.h\"\n+#include \"strbuf.h\"\n \n static int pack_compression_level = Z_DEFAULT_COMPRESSION;\n \n@@ -23,7 +24,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 = STRBUF_INIT;\n \tint i;\n \n \tif (!state->f)\n@@ -43,8 +44,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\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+\tstrbuf_addf(&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 \t\t\t &state->pack_idx_opts, sha1);\n \tfor (i = 0; i < state->nr_written; i++)\n@@ -54,6 +55,7 @@ clear_exit:\n \tfree(state->written);\n \tmemset(state, 0, sizeof(*state));\n \n+\tstrbuf_release(&packname);\n \t/* Make objects we just wrote available to ourselves */\n \treprepare_packed_git();\n }\ndiff --git a/pack-write.c b/pack-write.c\nindex 9b8308b..2204aa9 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n \treturn sha1fd(fd, *pack_tmp_name);\n }\n \n-void finish_tmp_packfile(char *name_buffer,\n+void finish_tmp_packfile(struct strbuf *name_buffer,\n \t\t\t const char *pack_tmp_name,\n \t\t\t struct pack_idx_entry **written_list,\n \t\t\t uint32_t nr_written,\n@@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,\n \t\t\t unsigned char sha1[])\n {\n \tconst char *idx_tmp_name;\n-\tchar *end_of_name_prefix = strrchr(name_buffer, 0);\n+\tint pre_len = name_buffer->len;\n \n \tif (adjust_shared_perm(pack_tmp_name))\n \t\tdie_errno(\"unable to make temporary pack file readable\");\n@@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,\n \tif (adjust_shared_perm(idx_tmp_name))\n \t\tdie_errno(\"unable to make temporary index file readable\");\n \n-\tsprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n-\tfree_pack_by_name(name_buffer);\n+\tstrbuf_addf(name_buffer, \"%s.pack\", sha1_to_hex(sha1));\n+\tfree_pack_by_name(name_buffer->buf);\n \n-\tif (rename(pack_tmp_name, name_buffer))\n+\tif (rename(pack_tmp_name, name_buffer->buf))\n \t\tdie_errno(\"unable to rename temporary pack file\");\n \n-\tsprintf(end_of_name_prefix, \"%s.idx\", sha1_to_hex(sha1));\n-\tif (rename(idx_tmp_name, name_buffer))\n+\t/* remove added suffix string(sha1.pack) */\n+\tstrbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n+\n+\tstrbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n+\tif (rename(idx_tmp_name, name_buffer->buf))\n \t\tdie_errno(\"unable to rename temporary index file\");\n \n-\t*end_of_name_prefix = '\\0';\n+\t/* remove added suffix string(sha1.idx) */\n+\tstrbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n \n \tfree((void *)idx_tmp_name);\n }\ndiff --git a/pack.h b/pack.h\nindex 12d9516..3223f5a 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -91,6 +91,6 @@ extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned ch\n extern int read_pack_header(int fd, struct pack_header *);\n \n extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);\n-extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);\n+extern void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);\n \n #endif\n-- \n1.9.0.138.g2de3478.dirty\n\nHello,\nThis is my patch for the GSoC microproject #2\n\nI follow the suggestion of Junio to use the strbuf_addf to deal with this thing.\nAnd the usage of strbuf_addf needs to change the function finish_tmp_packfile, I search all over the source code ($ find .| xargs grep \"finish_tmp_packfile\")\nThe following files contains finish_tmp_packfile:\n\tbulk-checkin.c\n\tbuiltin/pack-object.c\n\tpack-write.c\n\tpack.h\nAnd I do some change to fix them.\nI changed my vimrc so that tabs will not be changed into spaces automatically.\n\nAnd I came across a bug when I was doing my microproject.\nposition: builtin/pack-objects.c: write_pack_file: if(!pack_to_stdout): first else in it\n warning() function used an uninitialized array, and I changed it to pack_tmp_name.\n\nThank you all for all your suggestions.\n\nCheers,\nHe Sun\n"},{"id":"235579","messageId":"CAPig+cSvkmZH2qEqKd=sjaMO8bfnxiKiuTKtfOuMCDwDfCDciw@mail.gmail.com","threadId":"35990","inReplyTo":"1393576104-1758-1-git-send-email-sunheehnus@gmail.com","subject":"Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-28T09:46:55Z","receivedAt":"2014-02-28T09:46:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 28, 2014 at 3:28 AM, Sun He wrote:\n> Signed-off-by: Sun He \n> ---\n\nNicely done.\n\nDue to the necessary changes to finish_tmp_packfile(), the focus of\nthis patch has changed slightly from that suggested by the\nmicroproject. It's really now about finish_tmp_packfile() rather than\nfinish_bulk_checkin(). As such, it may make sense to adjust the patch\nsubject to reflect this. For instance:\n\n Subject: finish_tmp_packfile(): use strbuf for pathname construction\n\nMore comments below.\n\n]> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index c733379..72fb82b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -803,7 +803,7 @@ static void write_pack_file(void)\n>\n> if (!pack_to_stdout) {\n> struct stat st;\n> - char tmpname[PATH_MAX];\n> + struct strbuf tmpname = STRBUF_INIT;\n>\n> /*\n> * Packs are runtime accessed in their mtime\n> @@ -823,26 +823,22 @@ static void write_pack_file(void)\n> utb.modtime = --last_mtime;\n> if (utime(pack_tmp_name, &utb) < 0)\n> warning(\"failed utime() on %s: %s\",\n> - tmpname, strerror(errno));\n> + pack_tmp_name, strerror(errno));\n\nGood catch finding this bug (as your commentary below states).\nIdeally, each conceptual change should be presented distinctly from\nothers, so this bug should have its own patch (even though it's just a\none-liner).\n\n> }\n>\n> - /* Enough space for \"-.pack\"? */\n> - if (sizeof(tmpname) <= strlen(base_name) + 50)\n> - die(\"pack base name '%s' too long\", base_name);\n> - snprintf(tmpname, sizeof(tmpname), \"%s-\", base_name);\n> + strbuf_addf(&tmpname, \"%s-\", base_name);\n>\n> if (write_bitmap_index) {\n> bitmap_writer_set_checksum(sha1);\n> bitmap_writer_build_type_index(written_list, nr_written);\n> }\n>\n> - finish_tmp_packfile(tmpname, pack_tmp_name,\n> + finish_tmp_packfile(&tmpname, pack_tmp_name,\n> written_list, nr_written,\n> &pack_idx_opts, sha1);\n>\n> if (write_bitmap_index) {\n> - char *end_of_name_prefix = strrchr(tmpname, 0);\n> - sprintf(end_of_name_prefix, \"%s.bitmap\", sha1_to_hex(sha1));\n> + strbuf_addf(&tmpname, \"%s.bitmap\" ,sha1_to_hex(sha1));\n>\n> stop_progress(&progress_state);\n>\n> diff --git a/pack-write.c b/pack-write.c\n> index 9b8308b..2204aa9 100644\n> --- a/pack-write.c\n> +++ b/pack-write.c\n> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n> return sha1fd(fd, *pack_tmp_name);\n> }\n>\n> -void finish_tmp_packfile(char *name_buffer,\n> +void finish_tmp_packfile(struct strbuf *name_buffer,\n> const char *pack_tmp_name,\n> struct pack_idx_entry **written_list,\n> uint32_t nr_written,\n> @@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,\n> unsigned char sha1[])\n> {\n> const char *idx_tmp_name;\n> - char *end_of_name_prefix = strrchr(name_buffer, 0);\n> + int pre_len = name_buffer->len;\n\nMinor: Perhaps basename_len (or some such) would convey a bit more\nmeaning than pre_len.\n\n> if (adjust_shared_perm(pack_tmp_name))\n> die_errno(\"unable to make temporary pack file readable\");\n> @@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,\n> if (adjust_shared_perm(idx_tmp_name))\n> die_errno(\"unable to make temporary index file readable\");\n>\n> - sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n> - free_pack_by_name(name_buffer);\n> + strbuf_addf(name_buffer, \"%s.pack\", sha1_to_hex(sha1));\n> + free_pack_by_name(name_buffer->buf);\n>\n> - if (rename(pack_tmp_name, name_buffer))\n> + if (rename(pack_tmp_name, name_buffer->buf))\n> die_errno(\"unable to rename temporary pack file\");\n>\n> - sprintf(end_of_name_prefix, \"%s.idx\", sha1_to_hex(sha1));\n> - if (rename(idx_tmp_name, name_buffer))\n> + /* remove added suffix string(sha1.pack) */\n> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n\nSince you are merely truncating the buffer back to pre_len, perhaps\n\n strbuf_setlen(name_buffer, pre_len)\n\nwould be more idiomatic and easier to read (and would allow you to\ndrop the comment).\n\n> +\n> + strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n> + if (rename(idx_tmp_name, name_buffer->buf))\n> die_errno(\"unable to rename temporary index file\");\n>\n> - *end_of_name_prefix = '\\0';\n> + /* remove added suffix string(sha1.idx) */\n> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n\nDitto.\n\n> free((void *)idx_tmp_name);\n> }\n> --\n> 1.9.0.138.g2de3478.dirty\n>\n> Hello,\n> This is my patch for the GSoC microproject #2\n>\n> I follow the suggestion of Junio to use the strbuf_addf to deal with this thing.\n> And the usage of strbuf_addf needs to change the function finish_tmp_packfile, I search all over the source code ($ find .| xargs grep \"finish_tmp_packfile\")\n> The following files contains finish_tmp_packfile:\n> bulk-checkin.c\n> builtin/pack-object.c\n> pack-write.c\n> pack.h\n> And I do some change to fix them.\n> I changed my vimrc so that tabs will not be changed into spaces automatically.\n>\n> And I came across a bug when I was doing my microproject.\n> position: builtin/pack-objects.c: write_pack_file: if(!pack_to_stdout): first else in it\n> warning() function used an uninitialized array, and I changed it to pack_tmp_name.\n>\n> Thank you all for all your suggestions.\n\nThis section explaining your changes is important for the ongoing\nemail discussion, however, it is customary (and \"git am\"-friendly) to\nplace these notes just below the \"---\" line following your signoff\nnear the top of the email.\n"},{"id":"235606","messageId":"CAPig+cSN1WGj9aMKHEo+JO6xN2dT_+0fEjPG3bK=ZsA4dwyWBw@mail.gmail.com","threadId":"35990","inReplyTo":"CAPig+cSvkmZH2qEqKd=sjaMO8bfnxiKiuTKtfOuMCDwDfCDciw@mail.gmail.com","subject":"Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-28T13:10:57Z","receivedAt":"2014-02-28T13:10:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine wrote:\n> On Fri, Feb 28, 2014 at 3:28 AM, Sun He wrote:\n>> Signed-off-by: Sun He \n>> ---\n>\n> Nicely done.\n>\n> Due to the necessary changes to finish_tmp_packfile(), the focus of\n> this patch has changed slightly from that suggested by the\n> microproject. It's really now about finish_tmp_packfile() rather than\n> finish_bulk_checkin(). As such, it may make sense to adjust the patch\n> subject to reflect this. For instance:\n>\n> Subject: finish_tmp_packfile(): use strbuf for pathname construction\n\nYou may also want your commit message to explain why you chose this\napproach over other legitimate approaches. For instance, your change\nplaces extra burden on callers of finish_tmp_packfile() by leaking a\ndetail of its implementation: namely that it wants to manipulate\npathnames easily (via strbuf). An equally valid and more encapsulated\napproach would be for finish_tmp_packfile() to accept a 'const char *'\nand construct its own strbuf internally.\n\nIf the extra strbuf creation in the alternate approach is measurably\nslower, then you could use that fact to justify your implementation\nchoice in the commit message. On the other hand, if it's not\nmeasurably slower, then perhaps the more encapsulated approach with\ncleaner API might be preferable.\n\n> More comments below.\n>\n> ]> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index c733379..72fb82b 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -803,7 +803,7 @@ static void write_pack_file(void)\n>>\n>> if (!pack_to_stdout) {\n>> struct stat st;\n>> - char tmpname[PATH_MAX];\n>> + struct strbuf tmpname = STRBUF_INIT;\n>>\n>> /*\n>> * Packs are runtime accessed in their mtime\n>> @@ -823,26 +823,22 @@ static void write_pack_file(void)\n>> utb.modtime = --last_mtime;\n>> if (utime(pack_tmp_name, &utb) < 0)\n>> warning(\"failed utime() on %s: %s\",\n>> - tmpname, strerror(errno));\n>> + pack_tmp_name, strerror(errno));\n>\n> Good catch finding this bug (as your commentary below states).\n> Ideally, each conceptual change should be presented distinctly from\n> others, so this bug should have its own patch (even though it's just a\n> one-liner).\n>\n>> }\n>>\n>> - /* Enough space for \"-.pack\"? */\n>> - if (sizeof(tmpname) <= strlen(base_name) + 50)\n>> - die(\"pack base name '%s' too long\", base_name);\n>> - snprintf(tmpname, sizeof(tmpname), \"%s-\", base_name);\n>> + strbuf_addf(&tmpname, \"%s-\", base_name);\n>>\n>> if (write_bitmap_index) {\n>> bitmap_writer_set_checksum(sha1);\n>> bitmap_writer_build_type_index(written_list, nr_written);\n>> }\n>>\n>> - finish_tmp_packfile(tmpname, pack_tmp_name,\n>> + finish_tmp_packfile(&tmpname, pack_tmp_name,\n>> written_list, nr_written,\n>> &pack_idx_opts, sha1);\n>>\n>> if (write_bitmap_index) {\n>> - char *end_of_name_prefix = strrchr(tmpname, 0);\n>> - sprintf(end_of_name_prefix, \"%s.bitmap\", sha1_to_hex(sha1));\n>> + strbuf_addf(&tmpname, \"%s.bitmap\" ,sha1_to_hex(sha1));\n>>\n>> stop_progress(&progress_state);\n>>\n>> diff --git a/pack-write.c b/pack-write.c\n>> index 9b8308b..2204aa9 100644\n>> --- a/pack-write.c\n>> +++ b/pack-write.c\n>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n>> return sha1fd(fd, *pack_tmp_name);\n>> }\n>>\n>> -void finish_tmp_packfile(char *name_buffer,\n>> +void finish_tmp_packfile(struct strbuf *name_buffer,\n>> const char *pack_tmp_name,\n>> struct pack_idx_entry **written_list,\n>> uint32_t nr_written,\n>> @@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,\n>> unsigned char sha1[])\n>> {\n>> const char *idx_tmp_name;\n>> - char *end_of_name_prefix = strrchr(name_buffer, 0);\n>> + int pre_len = name_buffer->len;\n>\n> Minor: Perhaps basename_len (or some such) would convey a bit more\n> meaning than pre_len.\n>\n>> if (adjust_shared_perm(pack_tmp_name))\n>> die_errno(\"unable to make temporary pack file readable\");\n>> @@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,\n>> if (adjust_shared_perm(idx_tmp_name))\n>> die_errno(\"unable to make temporary index file readable\");\n>>\n>> - sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n>> - free_pack_by_name(name_buffer);\n>> + strbuf_addf(name_buffer, \"%s.pack\", sha1_to_hex(sha1));\n>> + free_pack_by_name(name_buffer->buf);\n>>\n>> - if (rename(pack_tmp_name, name_buffer))\n>> + if (rename(pack_tmp_name, name_buffer->buf))\n>> die_errno(\"unable to rename temporary pack file\");\n>>\n>> - sprintf(end_of_name_prefix, \"%s.idx\", sha1_to_hex(sha1));\n>> - if (rename(idx_tmp_name, name_buffer))\n>> + /* remove added suffix string(sha1.pack) */\n>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n>\n> Since you are merely truncating the buffer back to pre_len, perhaps\n>\n> strbuf_setlen(name_buffer, pre_len)\n>\n> would be more idiomatic and easier to read (and would allow you to\n> drop the comment).\n>\n>> +\n>> + strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n>> + if (rename(idx_tmp_name, name_buffer->buf))\n>> die_errno(\"unable to rename temporary index file\");\n>>\n>> - *end_of_name_prefix = '\\0';\n>> + /* remove added suffix string(sha1.idx) */\n>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n>\n> Ditto.\n>\n>> free((void *)idx_tmp_name);\n>> }\n>> --\n>> 1.9.0.138.g2de3478.dirty\n>>\n>> Hello,\n>> This is my patch for the GSoC microproject #2\n>>\n>> I follow the suggestion of Junio to use the strbuf_addf to deal with this thing.\n>> And the usage of strbuf_addf needs to change the function finish_tmp_packfile, I search all over the source code ($ find .| xargs grep \"finish_tmp_packfile\")\n>> The following files contains finish_tmp_packfile:\n>> bulk-checkin.c\n>> builtin/pack-object.c\n>> pack-write.c\n>> pack.h\n>> And I do some change to fix them.\n>> I changed my vimrc so that tabs will not be changed into spaces automatically.\n>>\n>> And I came across a bug when I was doing my microproject.\n>> position: builtin/pack-objects.c: write_pack_file: if(!pack_to_stdout): first else in it\n>> warning() function used an uninitialized array, and I changed it to pack_tmp_name.\n>>\n>> Thank you all for all your suggestions.\n>\n> This section explaining your changes is important for the ongoing\n> email discussion, however, it is customary (and \"git am\"-friendly) to\n> place these notes just below the \"---\" line following your signoff\n> near the top of the email.\n"},{"id":"235613","messageId":"CAJr59C2YTW-k=ujioUDfebf5koT4viB7NLtPk-do4T5Vo1Aqjw@mail.gmail.com","threadId":"35990","inReplyTo":"CAPig+cSN1WGj9aMKHEo+JO6xN2dT_+0fEjPG3bK=ZsA4dwyWBw@mail.gmail.com","subject":"Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"孙赫","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-02-28T14:17:47Z","receivedAt":"2014-02-28T14:17:47Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]\n:\n> On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine <[hidden email]> wrote:\n>\n>> On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:\n>>> Signed-off-by: Sun He <[hidden email]>\n>>> ---\n>>\n>> Nicely done.\n>>\n>> Due to the necessary changes to finish_tmp_packfile(), the focus of\n>> this patch has changed slightly from that suggested by the\n>> microproject. It's really now about finish_tmp_packfile() rather than\n>> finish_bulk_checkin(). As such, it may make sense to adjust the patch\n>> subject to reflect this. For instance:\n>>\n>> Subject: finish_tmp_packfile(): use strbuf for pathname construction\n>\n> You may also want your commit message to explain why you chose this\n> approach over other legitimate approaches. For instance, your change\n> places extra burden on callers of finish_tmp_packfile() by leaking a\n> detail of its implementation: namely that it wants to manipulate\n> pathnames easily (via strbuf). An equally valid and more encapsulated\n> approach would be for finish_tmp_packfile() to accept a 'const char *'\n> and construct its own strbuf internally.\n>\n> If the extra strbuf creation in the alternate approach is measurably\n> slower, then you could use that fact to justify your implementation\n> choice in the commit message. On the other hand, if it's not\n> measurably slower, then perhaps the more encapsulated approach with\n> cleaner API might be preferable.\n>\n\nThank you for your explaination. I get the point.\nAnd I think if it is proved that the strbuf way is measurably slower.\nWe should add a check for the length of string before we sprintf().\n\n>> More comments below.\n>>\n>> ]> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>> index c733379..72fb82b 100644\n>>> --- a/builtin/pack-objects.c\n>>> +++ b/builtin/pack-objects.c\n>>> @@ -803,7 +803,7 @@ static void write_pack_file(void)\n>>>\n>>> if (!pack_to_stdout) {\n>>> struct stat st;\n>>> - char tmpname[PATH_MAX];\n>>> + struct strbuf tmpname = STRBUF_INIT;\n>>>\n>>> /*\n>>> * Packs are runtime accessed in their mtime\n>>> @@ -823,26 +823,22 @@ static void write_pack_file(void)\n>>> utb.modtime = --last_mtime;\n>>> if (utime(pack_tmp_name, &utb) < 0)\n>>> warning(\"failed utime() on %s:\n>>> %s\",\n>>> - tmpname,\n>>> strerror(errno));\n>>> + pack_tmp_name,\n>>> strerror(errno));\n>>\n>> Good catch finding this bug (as your commentary below states).\n>> Ideally, each conceptual change should be presented distinctly from\n>> others, so this bug should have its own patch (even though it's just a\n>> one-liner).\n>>\n>>> }\n>>>\n>>> - /* Enough space for \"-.pack\"? */\n>>> - if (sizeof(tmpname) <= strlen(base_name) + 50)\n>>> - die(\"pack base name '%s' too long\",\n>>> base_name);\n>>> - snprintf(tmpname, sizeof(tmpname), \"%s-\",\n>>> base_name);\n>>> + strbuf_addf(&tmpname, \"%s-\", base_name);\n>>>\n>>> if (write_bitmap_index) {\n>>> bitmap_writer_set_checksum(sha1);\n>>>\n>>> bitmap_writer_build_type_index(written_list, nr_written);\n>>> }\n>>>\n>>> - finish_tmp_packfile(tmpname, pack_tmp_name,\n>>> + finish_tmp_packfile(&tmpname, pack_tmp_name,\n>>> written_list, nr_written,\n>>> &pack_idx_opts, sha1);\n>>>\n>>> if (write_bitmap_index) {\n>>> - char *end_of_name_prefix =\n>>> strrchr(tmpname, 0);\n>>> - sprintf(end_of_name_prefix, \"%s.bitmap\",\n>>> sha1_to_hex(sha1));\n>>> + strbuf_addf(&tmpname, \"%s.bitmap\"\n>>> ,sha1_to_hex(sha1));\n>>>\n>>> stop_progress(&progress_state);\n>>>\n>>> diff --git a/pack-write.c b/pack-write.c\n>>> index 9b8308b..2204aa9 100644\n>>> --- a/pack-write.c\n>>> +++ b/pack-write.c\n>>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char\n>>> **pack_tmp_name)\n>>> return sha1fd(fd, *pack_tmp_name);\n>>> }\n>>>\n>>> -void finish_tmp_packfile(char *name_buffer,\n>>> +void finish_tmp_packfile(struct strbuf *name_buffer,\n>>> const char *pack_tmp_name,\n>>> struct pack_idx_entry **written_list,\n>>> uint32_t nr_written,\n>>> @@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,\n>>> unsigned char sha1[])\n>>> {\n>>> const char *idx_tmp_name;\n>>> - char *end_of_name_prefix = strrchr(name_buffer, 0);\n>>> + int pre_len = name_buffer->len;\n>>\n>> Minor: Perhaps basename_len (or some such) would convey a bit more\n>> meaning than pre_len.\n>>\n>>> if (adjust_shared_perm(pack_tmp_name))\n>>> die_errno(\"unable to make temporary pack file readable\");\n>>> @@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,\n>>> if (adjust_shared_perm(idx_tmp_name))\n>>> die_errno(\"unable to make temporary index file\n>>> readable\");\n>>>\n>>> - sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n>>> - free_pack_by_name(name_buffer);\n>>> + strbuf_addf(name_buffer, \"%s.pack\", sha1_to_hex(sha1));\n>>> + free_pack_by_name(name_buffer->buf);\n>>>\n>>> - if (rename(pack_tmp_name, name_buffer))\n>>> + if (rename(pack_tmp_name, name_buffer->buf))\n>>> die_errno(\"unable to rename temporary pack file\");\n>>>\n>>> - sprintf(end_of_name_prefix, \"%s.idx\", sha1_to_hex(sha1));\n>>> - if (rename(idx_tmp_name, name_buffer))\n>>> + /* remove added suffix string(sha1.pack) */\n>>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n>>\n>> Since you are merely truncating the buffer back to pre_len, perhaps\n>>\n>> strbuf_setlen(name_buffer, pre_len)\n>>\n>> would be more idiomatic and easier to read (and would allow you to\n>> drop the comment).\n>>\n>>> +\n>>> + strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n>>> + if (rename(idx_tmp_name, name_buffer->buf))\n>>> die_errno(\"unable to rename temporary index file\");\n>>>\n>>> - *end_of_name_prefix = '\\0';\n>>> + /* remove added suffix string(sha1.idx) */\n>>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n>>\n>> Ditto.\n>>\n>>> free((void *)idx_tmp_name);\n>>> }\n>>> --\n>>> 1.9.0.138.g2de3478.dirty\n>>>\n>>> Hello,\n>>> This is my patch for the GSoC microproject #2\n>>>\n>>> I follow the suggestion of Junio to use the strbuf_addf to deal with this\n>>> thing.\n>>> And the usage of strbuf_addf needs to change the function\n>>> finish_tmp_packfile, I search all over the source code ($ find .| xargs grep\n>>> \"finish_tmp_packfile\")\n>>> The following files contains finish_tmp_packfile:\n>>> bulk-checkin.c\n>>> builtin/pack-object.c\n>>> pack-write.c\n>>> pack.h\n>>> And I do some change to fix them.\n>>> I changed my vimrc so that tabs will not be changed into spaces\n>>> automatically.\n>>>\n>>> And I came across a bug when I was doing my microproject.\n>>> position: builtin/pack-objects.c: write_pack_file: if(!pack_to_stdout):\n>>> first else in it\n>>> warning() function used an uninitialized array, and I changed it to\n>>> pack_tmp_name.\n>>>\n>>> Thank you all for all your suggestions.\n>>\n>> This section explaining your changes is important for the ongoing\n>> email discussion, however, it is customary (and \"git am\"-friendly) to\n>> place these notes just below the \"---\" line following your signoff\n>> near the top of the email.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to [hidden email]\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>\n>\n> ________________________________\n> If you reply to this email, your message will be added to the discussion\n> below:\n> http://git.661346.n2.nabble.com/PATCH-Rewrite-bulk-checkin-c-finish-bulk-checkin-to-use-a-strbuf-for-handling-packname-tp7604449p7604500.html\n> To start a new topic under git, email\n> ml-node+s661346n661346h27@n2.nabble.com\n> To unsubscribe from git, click here.\n> NAML\nCheers,\nHe Sun\n"},{"id":"235629","messageId":"CAJr59C1ezbeG5+x2VHCnxbfATXgVO1gq9+b3amgvKUqZa0Swkg@mail.gmail.com","threadId":"35990","inReplyTo":"CAJr59C2Uw+tnRSuHbMnyJjGSE+XNpepPdode5MvcHb4X31e+qQ@mail.gmail.com","subject":"Fwd: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"孙赫","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-02-28T15:54:44Z","receivedAt":"2014-02-28T15:54:44Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"---------- Forwarded message ----------\nFrom: 孙赫 \nDate: 2014-02-28 21:37 GMT+08:00\nSubject: Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to\nuse a strbuf for handling packname\nTo: \"Eric Sunshine [via git]\" \n\n\n2014-02-28 17:47 GMT+08:00 Eric Sunshine [via git]\n:\n> On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:\n>> Signed-off-by: Sun He <[hidden email]>\n>> ---\n>\n> Nicely done.\n>\n> Due to the necessary changes to finish_tmp_packfile(), the focus of\n> this patch has changed slightly from that suggested by the\n> microproject. It's really now about finish_tmp_packfile() rather than\n> finish_bulk_checkin(). As such, it may make sense to adjust the patch\n> subject to reflect this. For instance:\n>\n> Subject: finish_tmp_packfile(): use strbuf for pathname construction\n>\n\nThat's great, I will adjust it as you suggested.\n\n> More comments below.\n>\n> ]> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>\n>> index c733379..72fb82b 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -803,7 +803,7 @@ static void write_pack_file(void)\n>>\n>> if (!pack_to_stdout) {\n>> struct stat st;\n>> - char tmpname[PATH_MAX];\n>> + struct strbuf tmpname = STRBUF_INIT;\n>>\n>> /*\n>> * Packs are runtime accessed in their mtime\n>> @@ -823,26 +823,22 @@ static void write_pack_file(void)\n>> utb.modtime = --last_mtime;\n>> if (utime(pack_tmp_name, &utb) < 0)\n>> warning(\"failed utime() on %s:\n>> %s\",\n>> - tmpname, strerror(errno));\n>> + pack_tmp_name,\n>> strerror(errno));\n>\n> Good catch finding this bug (as your commentary below states).\n> Ideally, each conceptual change should be presented distinctly from\n> others, so this bug should have its own patch (even though it's just a\n> one-liner).\n>\n\nOK. Should I send this patch?\n\n>> }\n>>\n>> - /* Enough space for \"-.pack\"? */\n>> - if (sizeof(tmpname) <= strlen(base_name) + 50)\n>> - die(\"pack base name '%s' too long\",\n>> base_name);\n>> - snprintf(tmpname, sizeof(tmpname), \"%s-\",\n>> base_name);\n>> + strbuf_addf(&tmpname, \"%s-\", base_name);\n>>\n>> if (write_bitmap_index) {\n>> bitmap_writer_set_checksum(sha1);\n>>\n>> bitmap_writer_build_type_index(written_list, nr_written);\n>> }\n>>\n>> - finish_tmp_packfile(tmpname, pack_tmp_name,\n>> + finish_tmp_packfile(&tmpname, pack_tmp_name,\n>> written_list, nr_written,\n>> &pack_idx_opts, sha1);\n>>\n>> if (write_bitmap_index) {\n>> - char *end_of_name_prefix =\n>> strrchr(tmpname, 0);\n>> - sprintf(end_of_name_prefix, \"%s.bitmap\",\n>> sha1_to_hex(sha1));\n>> + strbuf_addf(&tmpname, \"%s.bitmap\"\n>> ,sha1_to_hex(sha1));\n>>\n>> stop_progress(&progress_state);\n>>\n>> diff --git a/pack-write.c b/pack-write.c\n>> index 9b8308b..2204aa9 100644\n>> --- a/pack-write.c\n>> +++ b/pack-write.c\n>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char\n>> **pack_tmp_name)\n>> return sha1fd(fd, *pack_tmp_name);\n>> }\n>>\n>> -void finish_tmp_packfile(char *name_buffer,\n>> +void finish_tmp_packfile(struct strbuf *name_buffer,\n>> const char *pack_tmp_name,\n>> struct pack_idx_entry **written_list,\n>> uint32_t nr_written,\n>> @@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,\n>> unsigned char sha1[])\n>> {\n>> const char *idx_tmp_name;\n>> - char *end_of_name_prefix = strrchr(name_buffer, 0);\n>> + int pre_len = name_buffer->len;\n>\n> Minor: Perhaps basename_len (or some such) would convey a bit more\n> meaning than pre_len.\n>\n\nIt's pretty good to work with you next suggestion. THX\n\n>> if (adjust_shared_perm(pack_tmp_name))\n>> die_errno(\"unable to make temporary pack file readable\");\n>> @@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,\n>> if (adjust_shared_perm(idx_tmp_name))\n>> die_errno(\"unable to make temporary index file readable\");\n>>\n>> - sprintf(end_of_name_prefix, \"%s.pack\", sha1_to_hex(sha1));\n>> - free_pack_by_name(name_buffer);\n>> + strbuf_addf(name_buffer, \"%s.pack\", sha1_to_hex(sha1));\n>> + free_pack_by_name(name_buffer->buf);\n>>\n>> - if (rename(pack_tmp_name, name_buffer))\n>> + if (rename(pack_tmp_name, name_buffer->buf))\n>> die_errno(\"unable to rename temporary pack file\");\n>>\n>> - sprintf(end_of_name_prefix, \"%s.idx\", sha1_to_hex(sha1));\n>> - if (rename(idx_tmp_name, name_buffer))\n>> + /* remove added suffix string(sha1.pack) */\n>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n>\n> Since you are merely truncating the buffer back to pre_len, perhaps\n>\n> strbuf_setlen(name_buffer, pre_len)\n>\n> would be more idiomatic and easier to read (and would allow you to\n> drop the comment).\n>\n\nYeah, yeah. It's more elegent. I will fix it later.\n\n>> +\n>> + strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n>> + if (rename(idx_tmp_name, name_buffer->buf))\n>> die_errno(\"unable to rename temporary index file\");\n>>\n>> - *end_of_name_prefix = '\\0';\n>> + /* remove added suffix string(sha1.idx) */\n>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);\n>\n> Ditto.\n>\n>> free((void *)idx_tmp_name);\n>> }\n>> --\n>> 1.9.0.138.g2de3478.dirty\n>>\n>> Hello,\n>> This is my patch for the GSoC microproject #2\n>>\n>> I follow the suggestion of Junio to use the strbuf_addf to deal with this\n>> thing.\n>> And the usage of strbuf_addf needs to change the function\n>> finish_tmp_packfile, I search all over the source code ($ find .| xargs grep\n>> \"finish_tmp_packfile\")\n>> The following files contains finish_tmp_packfile:\n>> bulk-checkin.c\n>> builtin/pack-object.c\n>> pack-write.c\n>> pack.h\n>> And I do some change to fix them.\n>> I changed my vimrc so that tabs will not be changed into spaces\n>> automatically.\n>>\n>> And I came across a bug when I was doing my microproject.\n>> position: builtin/pack-objects.c: write_pack_file: if(!pack_to_stdout):\n>> first else in it\n>> warning() function used an uninitialized array, and I changed it to\n>> pack_tmp_name.\n>>\n>> Thank you all for all your suggestions.\n>\n> This section explaining your changes is important for the ongoing\n> email discussion, however, it is customary (and \"git am\"-friendly) to\n> place these notes just below the \"---\" line following your signoff\n> near the top of the email.\n> --\nGot it.\nThank you very very much for your code review and great suggestions.\n\nHe Sun\n"},{"id":"235665","messageId":"CAPig+cTdMZw=tiJyr-0v_ZE_P92Yjc1g2YhhBF_Jv+ML87kz2Q@mail.gmail.com","threadId":"35990","inReplyTo":"CAJr59C2YTW-k=ujioUDfebf5koT4viB7NLtPk-do4T5Vo1Aqjw@mail.gmail.com","subject":"Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-28T20:42:02Z","receivedAt":"2014-02-28T20:42:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 28, 2014 at 9:17 AM, 孙赫 wrote:\n> 2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]\n> :\n>> On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine <[hidden email]> wrote:\n>>\n>>> On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:\n>>>> Signed-off-by: Sun He <[hidden email]>\n>>>> ---\n>>>\n>>> Nicely done.\n>>>\n>>> Due to the necessary changes to finish_tmp_packfile(), the focus of\n>>> this patch has changed slightly from that suggested by the\n>>> microproject. It's really now about finish_tmp_packfile() rather than\n>>> finish_bulk_checkin(). As such, it may make sense to adjust the patch\n>>> subject to reflect this. For instance:\n>>>\n>>> Subject: finish_tmp_packfile(): use strbuf for pathname construction\n>>\n>> You may also want your commit message to explain why you chose this\n>> approach over other legitimate approaches. For instance, your change\n>> places extra burden on callers of finish_tmp_packfile() by leaking a\n>> detail of its implementation: namely that it wants to manipulate\n>> pathnames easily (via strbuf). An equally valid and more encapsulated\n>> approach would be for finish_tmp_packfile() to accept a 'const char *'\n>> and construct its own strbuf internally.\n>>\n>> If the extra strbuf creation in the alternate approach is measurably\n>> slower, then you could use that fact to justify your implementation\n>> choice in the commit message. On the other hand, if it's not\n>> measurably slower, then perhaps the more encapsulated approach with\n>> cleaner API might be preferable.\n>>\n>\n> Thank you for your explaination. I get the point.\n> And I think if it is proved that the strbuf way is measurably slower.\n> We should add a check for the length of string before we sprintf().\n\nI'm not sure what you mean by checking the string length before sprintf().\n\nMy point was that if there are multiple ways of solving the same\nproblem, it can be helpful for reviewers if your commit message\nexplains why the solution you chose is better than the others.\n\nSlowness and/or cleanliness of API were just examples you might use in\nyour commit message for justifying why you chose one approach over\nanother.\n"},{"id":"235666","messageId":"CAPig+cT8ziBV3w9gTR_GM=xupmJiZ=2hvizagk+SR3GY_wMGUw@mail.gmail.com","threadId":"35990","inReplyTo":"CAJr59C1ezbeG5+x2VHCnxbfATXgVO1gq9+b3amgvKUqZa0Swkg@mail.gmail.com","subject":"Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-28T20:47:11Z","receivedAt":"2014-02-28T20:47:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 28, 2014 at 10:54 AM, 孙赫 wrote:\n> 2014-02-28 17:47 GMT+08:00 Eric Sunshine [via git]\n> :\n>> On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:\n>>> Signed-off-by: Sun He <[hidden email]>\n>>> ---\n>> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>\n>>> index c733379..72fb82b 100644\n>>> --- a/builtin/pack-objects.c\n>>> +++ b/builtin/pack-objects.c\n>>> @@ -803,7 +803,7 @@ static void write_pack_file(void)\n>>>\n>>> if (!pack_to_stdout) {\n>>> struct stat st;\n>>> - char tmpname[PATH_MAX];\n>>> + struct strbuf tmpname = STRBUF_INIT;\n>>>\n>>> /*\n>>> * Packs are runtime accessed in their mtime\n>>> @@ -823,26 +823,22 @@ static void write_pack_file(void)\n>>> utb.modtime = --last_mtime;\n>>> if (utime(pack_tmp_name, &utb) < 0)\n>>> warning(\"failed utime() on %s:\n>>> %s\",\n>>> - tmpname, strerror(errno));\n>>> + pack_tmp_name,\n>>> strerror(errno));\n>>\n>> Good catch finding this bug (as your commentary below states).\n>> Ideally, each conceptual change should be presented distinctly from\n>> others, so this bug should have its own patch (even though it's just a\n>> one-liner).\n>>\n>\n> OK. Should I send this patch?\n\nYes, it is perfectly acceptable (and encouraged) to send this patch as\na submission distinct from your microproject.\n"},{"id":"235682","messageId":"CAJr59C3VE=kuSUsFF8Hq+KU6+a1JOEt4edg3YhBnnc_EsPcZZQ@mail.gmail.com","threadId":"35990","inReplyTo":"CAPig+cTdMZw=tiJyr-0v_ZE_P92Yjc1g2YhhBF_Jv+ML87kz2Q@mail.gmail.com","subject":"Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-01T00:13:40Z","receivedAt":"2014-03-01T00:13:40Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 4:42 GMT+08:00 Eric Sunshine :\n> On Fri, Feb 28, 2014 at 9:17 AM, 孙赫 wrote:\n>> 2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]\n>> :\n>>> On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine <[hidden email]> wrote:\n>>>\n>>>> On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:\n>>>>> Signed-off-by: Sun He <[hidden email]>\n>>>>> ---\n>>>>\n>>>> Nicely done.\n>>>>\n>>>> Due to the necessary changes to finish_tmp_packfile(), the focus of\n>>>> this patch has changed slightly from that suggested by the\n>>>> microproject. It's really now about finish_tmp_packfile() rather than\n>>>> finish_bulk_checkin(). As such, it may make sense to adjust the patch\n>>>> subject to reflect this. For instance:\n>>>>\n>>>> Subject: finish_tmp_packfile(): use strbuf for pathname construction\n>>>\n>>> You may also want your commit message to explain why you chose this\n>>> approach over other legitimate approaches. For instance, your change\n>>> places extra burden on callers of finish_tmp_packfile() by leaking a\n>>> detail of its implementation: namely that it wants to manipulate\n>>> pathnames easily (via strbuf). An equally valid and more encapsulated\n>>> approach would be for finish_tmp_packfile() to accept a 'const char *'\n>>> and construct its own strbuf internally.\n>>>\n>>> If the extra strbuf creation in the alternate approach is measurably\n>>> slower, then you could use that fact to justify your implementation\n>>> choice in the commit message. On the other hand, if it's not\n>>> measurably slower, then perhaps the more encapsulated approach with\n>>> cleaner API might be preferable.\n>>>\n>>\n>> Thank you for your explaination. I get the point.\n>> And I think if it is proved that the strbuf way is measurably slower.\n>> We should add a check for the length of string before we sprintf().\n>\n> I'm not sure what you mean by checking the string length before sprintf().\n>\n\nThat's because one is not certain of the length of get_object_directory()\n\nMicheal Mhaggerty explained this to me.\n Saving stack space is nice, though given that it takes more time to\n allocate space on the heap, it is nonetheless usually preferred to use\n the stack for temporary variables of this size.\n\n The problem has more to do with the fact that the old version fixes a\n maximum length on the buffer, which could be a problem if one is not\n certain of the length of get_object_directory().\n\n The other point of strbuf is that you don't have to do the error-prone\n bookkeeping yourself. So it would be preferable to use strbuf_addf().\n\nIt is the same as yours about the space and time costs. ^_^\n\n> My point was that if there are multiple ways of solving the same\n> problem, it can be helpful for reviewers if your commit message\n> explains why the solution you chose is better than the others.\n>\n> Slowness and/or cleanliness of API were just examples you might use in\n> your commit message for justifying why you chose one approach over\n> another.\n\nOK, OK, Got it. Thank you very much.\n\nCheers,\nHe Sun\n"}]}