{"thread":{"id":"36007","subject":"[PATCH] rewrite finish_bulk_checkin() using strbuf","startedAt":"2014-03-01T06:46:29Z","lastAt":"2014-03-01T11:20:37Z","messageCount":5,"participants":["Faiz Kothari","He Sun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235697","messageId":"1393656389-31300-1-git-send-email-faiz.off93@gmail.com","threadId":"36007","inReplyTo":null,"subject":"[PATCH] rewrite finish_bulk_checkin() using strbuf","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-01T06:46:29Z","receivedAt":"2014-03-01T06:46:29Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"From: Faiz Kotahri <faiz.off93@gmail.com>\n\nSigned-off-by: Faiz Kothari <faiz.off93@gmail.com>\n---\nSticking with implementation involving changing the prototype for\npack-write.c:finish_tmp_packfile()\nFixing a small bug in Sun He's implementation which caused a fail in some tests.\n\n builtin/pack-objects.c |   25 ++++++++++++-------------\n bulk-checkin.c         |    9 ++++++---\n pack-write.c           |   19 ++++++++++---------\n pack.h                 |    3 ++-\n 4 files changed, 30 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c733379..4b59bba 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -20,6 +20,7 @@\n #include \"streaming.h\"\n #include \"thread-utils.h\"\n #include \"pack-bitmap.h\"\n+#include \"strbuf.h\"\n \n static const char *pack_usage[] = {\n \tN_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n@@ -803,8 +804,8 @@ 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-\n+\t\t\tstruct strbuf tmpname = STRBUF_INIT;\n+\t\t\tint ini_length;\n \t\t\t/*\n \t\t\t * Packs are runtime accessed in their mtime\n \t\t\t * order since newer packs are more likely to contain\n@@ -823,26 +824,24 @@ 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 \"-<sha-1>.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+\t\t\tini_length = tmpname.len;\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\t\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_remove(&tmpname, ini_length, tmpname.len - ini_length);\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 +850,10 @@ 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..248454c 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,16 +44,18 @@ 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+\t\n \tfor (i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n clear_exit:\n \tfree(state->written);\n \tmemset(state, 0, sizeof(*state));\n+\tstrbuf_release(&packname);\n \n \t/* Make objects we just wrote available to ourselves */\n \treprepare_packed_git();\ndiff --git a/pack-write.c b/pack-write.c\nindex 9b8308b..60f5734 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pack.h\"\n #include \"csum-file.h\"\n+#include \"strbuf.h\"\n \n void reset_pack_idx_option(struct pack_idx_option *opts)\n {\n@@ -336,7 +337,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 +345,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 ini_length = 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 +355,17 @@ 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+\tstrbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);\n+\tstrbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n+\t\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-\n \tfree((void *)idx_tmp_name);\n }\ndiff --git a/pack.h b/pack.h\nindex 12d9516..0afe8d1 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -3,6 +3,7 @@\n \n #include \"object.h\"\n #include \"csum-file.h\"\n+#include \"strbuf.h\"\n \n /*\n  * Packed object header\n@@ -91,6 +92,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.7.9.5\n"},{"id":"235701","messageId":"CAJr59C35jQoUfFVQSOticjGQmj_jKVrkR1=Uhej0JNhsZ-2LHA@mail.gmail.com","threadId":"36007","inReplyTo":"1393656389-31300-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] rewrite finish_bulk_checkin() using strbuf","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-01T07:10:30Z","receivedAt":"2014-03-01T07:10:30Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 14:46 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:\n> From: Faiz Kotahri <faiz.off93@gmail.com>\n>\n> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>\n> ---\n> Sticking with implementation involving changing the prototype for\n> pack-write.c:finish_tmp_packfile()\n> Fixing a small bug in Sun He's implementation which caused a fail in some tests.\n>\n>  builtin/pack-objects.c |   25 ++++++++++++-------------\n>  bulk-checkin.c         |    9 ++++++---\n>  pack-write.c           |   19 ++++++++++---------\n>  pack.h                 |    3 ++-\n>  4 files changed, 30 insertions(+), 26 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index c733379..4b59bba 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -20,6 +20,7 @@\n>  #include \"streaming.h\"\n>  #include \"thread-utils.h\"\n>  #include \"pack-bitmap.h\"\n> +#include \"strbuf.h\"\n>\n>  static const char *pack_usage[] = {\n>         N_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n> @@ -803,8 +804,8 @@ static void write_pack_file(void)\n>\n>                 if (!pack_to_stdout) {\n>                         struct stat st;\n> -                       char tmpname[PATH_MAX];\n> -\n> +                       struct strbuf tmpname = STRBUF_INIT;\n> +                       int ini_length;\n>                         /*\n>                          * Packs are runtime accessed in their mtime\n>                          * order since newer packs are more likely to contain\n> @@ -823,26 +824,24 @@ 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>\n> -                       /* Enough space for \"-<sha-1>.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> +                       ini_length = tmpname.len;\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> +\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_remove(&tmpname, ini_length, tmpname.len - ini_length);\n\nIs this the position where you think may import bugs?\nI think it should be the duty of finish_tmp_packfile to ensure that the tmpname\nis the same as it is input as a param.\nAnd in my original code, I have used strbuf_remove() at the end of\nfinish_tmp_packfile.\nEric Sunshine <sunshine@sunshineco.com> came up with a more elegant way to\nfinish this task. That's using  strbuf_setlen() instead of strbuf_remove().\n\n> +                               strbuf_addf(&tmpname, \"%s.bitmap\", sha1_to_hex(sha1));\n>\n>                                 stop_progress(&progress_state);\n>\n> @@ -851,10 +850,10 @@ static void write_pack_file(void)\n>                                 bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);\n>                                 bitmap_writer_build(&to_pack);\n>                                 bitmap_writer_finish(written_list, nr_written,\n> -                                                    tmpname, write_bitmap_options);\n> +                                                    tmpname.buf, write_bitmap_options);\n>                                 write_bitmap_index = 0;\n>                         }\n> -\n> +                       strbuf_release(&tmpname);\n>                         free(pack_tmp_name);\n>                         puts(sha1_to_hex(sha1));\n>                 }\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 118c625..248454c 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>         unsigned char sha1[20];\n> -       char packname[PATH_MAX];\n> +       struct strbuf packname = STRBUF_INIT;\n>         int i;\n>\n>         if (!state->f)\n> @@ -43,16 +44,18 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>                 close(fd);\n>         }\n>\n> -       sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n> -       finish_tmp_packfile(packname, state->pack_tmp_name,\n> +       strbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n> +       finish_tmp_packfile(&packname, state->pack_tmp_name,\n>                             state->written, state->nr_written,\n>                             &state->pack_idx_opts, sha1);\n> +\n>         for (i = 0; i < state->nr_written; i++)\n>                 free(state->written[i]);\n>\n>  clear_exit:\n>         free(state->written);\n>         memset(state, 0, sizeof(*state));\n> +       strbuf_release(&packname);\n>\n>         /* Make objects we just wrote available to ourselves */\n>         reprepare_packed_git();\n> diff --git a/pack-write.c b/pack-write.c\n> index 9b8308b..60f5734 100644\n> --- a/pack-write.c\n> +++ b/pack-write.c\n> @@ -1,6 +1,7 @@\n>  #include \"cache.h\"\n>  #include \"pack.h\"\n>  #include \"csum-file.h\"\n> +#include \"strbuf.h\"\n>\n>  void reset_pack_idx_option(struct pack_idx_option *opts)\n>  {\n> @@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;\n>\n>         if (adjust_shared_perm(pack_tmp_name))\n>                 die_errno(\"unable to make temporary pack file readable\");\n> @@ -354,17 +355,17 @@ 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> +       strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);\n> +       strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n> +\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> -\n>         free((void *)idx_tmp_name);\n>  }\n> diff --git a/pack.h b/pack.h\n> index 12d9516..0afe8d1 100644\n> --- a/pack.h\n> +++ b/pack.h\n> @@ -3,6 +3,7 @@\n>\n>  #include \"object.h\"\n>  #include \"csum-file.h\"\n> +#include \"strbuf.h\"\n>\n>  /*\n>   * Packed object header\n> @@ -91,6 +92,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> --\n> 1.7.9.5\n>\nCheers,\nHe Sun\n"},{"id":"235702","messageId":"CAJr59C2F93qUFk3g201ykmQbbQmPGN60k=9--3rOuYAvdZHapA@mail.gmail.com","threadId":"36007","inReplyTo":"1393656389-31300-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] rewrite finish_bulk_checkin() using strbuf","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-01T07:10:37Z","receivedAt":"2014-03-01T07:10:37Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 14:46 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:\n> From: Faiz Kotahri <faiz.off93@gmail.com>\n>\n> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>\n> ---\n> Sticking with implementation involving changing the prototype for\n> pack-write.c:finish_tmp_packfile()\n> Fixing a small bug in Sun He's implementation which caused a fail in some tests.\n>\n>  builtin/pack-objects.c |   25 ++++++++++++-------------\n>  bulk-checkin.c         |    9 ++++++---\n>  pack-write.c           |   19 ++++++++++---------\n>  pack.h                 |    3 ++-\n>  4 files changed, 30 insertions(+), 26 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index c733379..4b59bba 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -20,6 +20,7 @@\n>  #include \"streaming.h\"\n>  #include \"thread-utils.h\"\n>  #include \"pack-bitmap.h\"\n> +#include \"strbuf.h\"\n>\n>  static const char *pack_usage[] = {\n>         N_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n> @@ -803,8 +804,8 @@ static void write_pack_file(void)\n>\n>                 if (!pack_to_stdout) {\n>                         struct stat st;\n> -                       char tmpname[PATH_MAX];\n> -\n> +                       struct strbuf tmpname = STRBUF_INIT;\n> +                       int ini_length;\n>                         /*\n>                          * Packs are runtime accessed in their mtime\n>                          * order since newer packs are more likely to contain\n> @@ -823,26 +824,24 @@ 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>\n> -                       /* Enough space for \"-<sha-1>.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> +                       ini_length = tmpname.len;\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> +\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_remove(&tmpname, ini_length, tmpname.len - ini_length);\n\nIs this the position where you think may import bugs?\nI think it should be the duty of finish_tmp_packfile to ensure that the tmpname\nis the same as it is input as a param.\nAnd in my original code, I have used strbuf_remove() at the end of\nfinish_tmp_packfile.\nThere is a more elegant way to finish this task.[According to ]\n\n\n> +                               strbuf_addf(&tmpname, \"%s.bitmap\", sha1_to_hex(sha1));\n>\n>                                 stop_progress(&progress_state);\n>\n> @@ -851,10 +850,10 @@ static void write_pack_file(void)\n>                                 bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);\n>                                 bitmap_writer_build(&to_pack);\n>                                 bitmap_writer_finish(written_list, nr_written,\n> -                                                    tmpname, write_bitmap_options);\n> +                                                    tmpname.buf, write_bitmap_options);\n>                                 write_bitmap_index = 0;\n>                         }\n> -\n> +                       strbuf_release(&tmpname);\n>                         free(pack_tmp_name);\n>                         puts(sha1_to_hex(sha1));\n>                 }\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 118c625..248454c 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>         unsigned char sha1[20];\n> -       char packname[PATH_MAX];\n> +       struct strbuf packname = STRBUF_INIT;\n>         int i;\n>\n>         if (!state->f)\n> @@ -43,16 +44,18 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>                 close(fd);\n>         }\n>\n> -       sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n> -       finish_tmp_packfile(packname, state->pack_tmp_name,\n> +       strbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n> +       finish_tmp_packfile(&packname, state->pack_tmp_name,\n>                             state->written, state->nr_written,\n>                             &state->pack_idx_opts, sha1);\n> +\n>         for (i = 0; i < state->nr_written; i++)\n>                 free(state->written[i]);\n>\n>  clear_exit:\n>         free(state->written);\n>         memset(state, 0, sizeof(*state));\n> +       strbuf_release(&packname);\n>\n>         /* Make objects we just wrote available to ourselves */\n>         reprepare_packed_git();\n> diff --git a/pack-write.c b/pack-write.c\n> index 9b8308b..60f5734 100644\n> --- a/pack-write.c\n> +++ b/pack-write.c\n> @@ -1,6 +1,7 @@\n>  #include \"cache.h\"\n>  #include \"pack.h\"\n>  #include \"csum-file.h\"\n> +#include \"strbuf.h\"\n>\n>  void reset_pack_idx_option(struct pack_idx_option *opts)\n>  {\n> @@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;\n>\n>         if (adjust_shared_perm(pack_tmp_name))\n>                 die_errno(\"unable to make temporary pack file readable\");\n> @@ -354,17 +355,17 @@ 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> +       strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);\n> +       strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n> +\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> -\n>         free((void *)idx_tmp_name);\n>  }\n> diff --git a/pack.h b/pack.h\n> index 12d9516..0afe8d1 100644\n> --- a/pack.h\n> +++ b/pack.h\n> @@ -3,6 +3,7 @@\n>\n>  #include \"object.h\"\n>  #include \"csum-file.h\"\n> +#include \"strbuf.h\"\n>\n>  /*\n>   * Packed object header\n> @@ -91,6 +92,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> --\n> 1.7.9.5\n>\n"},{"id":"235710","messageId":"CAJr59C2SpT_=-tDq1yQSt8j1TNy9vOugw-tusC=KBUnYfjYfeg@mail.gmail.com","threadId":"36007","inReplyTo":"CAFbjVcmghtj80NetKoBeCKfMcECotiC=Q7ZmBR9ASuAtT_wiWg@mail.gmail.com","subject":"Re: [PATCH] rewrite finish_bulk_checkin() using strbuf","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-01T11:03:09Z","receivedAt":"2014-03-01T11:03:09Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 15:18 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:\n> Hi,\n> Yup, at that position.\n> I don't know, but it failed a few tests on my machine related to bitmap.\n> Another thing to use would be strbuf_splice()\n\nEric Sunshine<sunshine@sunshineco.com> has came up with a more\nelegant way to finish this task. That's using strbuf_setlen() instead of\nstrbuf_remove(). Because of the unstable network of this afternoon. The\nformer email is sent without the above information.\nSorry about it.\n\nI find out that you didn't attach strbuf_remove() in your finish_bulk_checkin().\nThat may cause problems not only related to bitmap, because the input\npackname is different from the output packname..\n\nCheers,\nHe Sun\n\n> Anyways, no worries :)\n>\n> Cheers,\n> Faiz\n>\n> On Sat, Mar 1, 2014 at 12:40 PM, He Sun <sunheehnus@gmail.com> wrote:\n>> 2014-03-01 14:46 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:\n>>> From: Faiz Kotahri <faiz.off93@gmail.com>\n>>>\n>>> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>\n>>> ---\n>>> Sticking with implementation involving changing the prototype for\n>>> pack-write.c:finish_tmp_packfile()\n>>> Fixing a small bug in Sun He's implementation which caused a fail in some tests.\n>>>\n>>>  builtin/pack-objects.c |   25 ++++++++++++-------------\n>>>  bulk-checkin.c         |    9 ++++++---\n>>>  pack-write.c           |   19 ++++++++++---------\n>>>  pack.h                 |    3 ++-\n>>>  4 files changed, 30 insertions(+), 26 deletions(-)\n>>>\n>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>> index c733379..4b59bba 100644\n>>> --- a/builtin/pack-objects.c\n>>> +++ b/builtin/pack-objects.c\n>>> @@ -20,6 +20,7 @@\n>>>  #include \"streaming.h\"\n>>>  #include \"thread-utils.h\"\n>>>  #include \"pack-bitmap.h\"\n>>> +#include \"strbuf.h\"\n>>>\n>>>  static const char *pack_usage[] = {\n>>>         N_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n>>> @@ -803,8 +804,8 @@ static void write_pack_file(void)\n>>>\n>>>                 if (!pack_to_stdout) {\n>>>                         struct stat st;\n>>> -                       char tmpname[PATH_MAX];\n>>> -\n>>> +                       struct strbuf tmpname = STRBUF_INIT;\n>>> +                       int ini_length;\n>>>                         /*\n>>>                          * Packs are runtime accessed in their mtime\n>>>                          * order since newer packs are more likely to contain\n>>> @@ -823,26 +824,24 @@ 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>>>\n>>> -                       /* Enough space for \"-<sha-1>.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>>> +                       ini_length = tmpname.len;\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>>> +\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_remove(&tmpname, ini_length, tmpname.len - ini_length);\n>>\n>> Is this the position where you think may import bugs?\n>> I think it should be the duty of finish_tmp_packfile to ensure that the tmpname\n>> is the same as it is input as a param.\n>> And in my original code, I have used strbuf_remove() at the end of\n>> finish_tmp_packfile.\n>> There is a more elegant way to finish this task.[According to ]\n>>\n>>\n>>> +                               strbuf_addf(&tmpname, \"%s.bitmap\", sha1_to_hex(sha1));\n>>>\n>>>                                 stop_progress(&progress_state);\n>>>\n>>> @@ -851,10 +850,10 @@ static void write_pack_file(void)\n>>>                                 bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);\n>>>                                 bitmap_writer_build(&to_pack);\n>>>                                 bitmap_writer_finish(written_list, nr_written,\n>>> -                                                    tmpname, write_bitmap_options);\n>>> +                                                    tmpname.buf, write_bitmap_options);\n>>>                                 write_bitmap_index = 0;\n>>>                         }\n>>> -\n>>> +                       strbuf_release(&tmpname);\n>>>                         free(pack_tmp_name);\n>>>                         puts(sha1_to_hex(sha1));\n>>>                 }\n>>> diff --git a/bulk-checkin.c b/bulk-checkin.c\n>>> index 118c625..248454c 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>>>         unsigned char sha1[20];\n>>> -       char packname[PATH_MAX];\n>>> +       struct strbuf packname = STRBUF_INIT;\n>>>         int i;\n>>>\n>>>         if (!state->f)\n>>> @@ -43,16 +44,18 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n>>>                 close(fd);\n>>>         }\n>>>\n>>> -       sprintf(packname, \"%s/pack/pack-\", get_object_directory());\n>>> -       finish_tmp_packfile(packname, state->pack_tmp_name,\n>>> +       strbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n>>> +       finish_tmp_packfile(&packname, state->pack_tmp_name,\n>>>                             state->written, state->nr_written,\n>>>                             &state->pack_idx_opts, sha1);\n>>> +\n>>>         for (i = 0; i < state->nr_written; i++)\n>>>                 free(state->written[i]);\n>>>\n>>>  clear_exit:\n>>>         free(state->written);\n>>>         memset(state, 0, sizeof(*state));\n>>> +       strbuf_release(&packname);\n>>>\n>>>         /* Make objects we just wrote available to ourselves */\n>>>         reprepare_packed_git();\n>>> diff --git a/pack-write.c b/pack-write.c\n>>> index 9b8308b..60f5734 100644\n>>> --- a/pack-write.c\n>>> +++ b/pack-write.c\n>>> @@ -1,6 +1,7 @@\n>>>  #include \"cache.h\"\n>>>  #include \"pack.h\"\n>>>  #include \"csum-file.h\"\n>>> +#include \"strbuf.h\"\n>>>\n>>>  void reset_pack_idx_option(struct pack_idx_option *opts)\n>>>  {\n>>> @@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;\n>>>\n>>>         if (adjust_shared_perm(pack_tmp_name))\n>>>                 die_errno(\"unable to make temporary pack file readable\");\n>>> @@ -354,17 +355,17 @@ 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>>> +       strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);\n>>> +       strbuf_addf(name_buffer, \"%s.idx\", sha1_to_hex(sha1));\n>>> +\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>>> -\n>>>         free((void *)idx_tmp_name);\n>>>  }\n>>> diff --git a/pack.h b/pack.h\n>>> index 12d9516..0afe8d1 100644\n>>> --- a/pack.h\n>>> +++ b/pack.h\n>>> @@ -3,6 +3,7 @@\n>>>\n>>>  #include \"object.h\"\n>>>  #include \"csum-file.h\"\n>>> +#include \"strbuf.h\"\n>>>\n>>>  /*\n>>>   * Packed object header\n>>> @@ -91,6 +92,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>>> --\n>>> 1.7.9.5\n>>>\n"},{"id":"235711","messageId":"CAFbjVcnM8a9pOmkt1ERZ-5mxek6+uP5ZPabwz1EqBFxO=VCJVw@mail.gmail.com","threadId":"36007","inReplyTo":"CAJr59C2SpT_=-tDq1yQSt8j1TNy9vOugw-tusC=KBUnYfjYfeg@mail.gmail.com","subject":"Re: [PATCH] rewrite finish_bulk_checkin() using strbuf","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-01T11:20:37Z","receivedAt":"2014-03-01T11:20:37Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"On Sat, Mar 1, 2014 at 4:33 PM, He Sun <sunheehnus@gmail.com> wrote:\n> 2014-03-01 15:18 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:\n>> Hi,\n>> Yup, at that position.\n>> I don't know, but it failed a few tests on my machine related to bitmap.\n>> Another thing to use would be strbuf_splice()\n>\n> Eric Sunshine<sunshine@sunshineco.com> has came up with a more\n> elegant way to finish this task. That's using strbuf_setlen() instead of\n> strbuf_remove(). Because of the unstable network of this afternoon. The\n> former email is sent without the above information.\n> Sorry about it.\nYou mean first use strbuf_setlen() to reduce len and then add \".whatever\" to it?\nstrbuf_splice can do strbuf_remove() and add string in one shot as per the doc.\nIf using strbuf_setlen() is still better, I'll implement using that :)\n\n> I find out that you didn't attach strbuf_remove() in your finish_bulk_checkin().\n> That may cause problems not only related to bitmap, because the input\n> packname is different from the output packname..\n>\nI am not sure how that is causing the problem, because tests run just fine.\nI'll still look into it. Thanks for pointing that out :)\n\nCheers,\nFaiz\n"}]}