{"thread":{"id":"36032","subject":"[PATCH v3] finish_tmp_packfile():use strbuf for pathname construction","startedAt":"2014-03-03T09:24:29Z","lastAt":"2014-03-03T09:24:29Z","messageCount":1,"participants":["Sun He"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"235854","messageId":"1393838669-6876-1-git-send-email-sunheehnus@gmail.com","threadId":"36032","inReplyTo":null,"subject":"[PATCH v3] finish_tmp_packfile():use strbuf for pathname construction","fromName":"Sun He","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-03T09:24:29Z","receivedAt":"2014-03-03T09:24:29Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"The old version fixes a maximum length on the buffer, which could be a problem\nif one is not certain of the length of get_object_directory().\nUsing strbuf can avoid the protential bug.\n\nHelped-by: Michael Haggerty <mhagger@alum.mit.edu>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Sun He <sunheehnus@gmail.com>\n---\n\nPATCH v3 adds the reason why we should apply this patch.\n Thanks to Micheal Haggerty.\nPATCH v3 transposes the space and comma before the third argument as \nEric Sunshine suggested to meet the style of existing code.\n Thanks to Eric Sunshine.\nPATCH v3 fixes the order of Helped-by and Signed-off-by.\n\nPATCH v2 follows the suggestions of Eric Sunshine to use strbuf_setlen() \ninstead of strbuf_remove(), etc.\n Thanks to Eric Sunshine.\n\nThis patch has assumed that you have already fix the bug of\ntmpname in builtin/pack-objects.c:write_pack_file() warning()\n\n\n builtin/pack-objects.c | 15 ++++++---------\n bulk-checkin.c         |  8 +++++---\n pack-write.c           | 18 ++++++++++--------\n pack.h                 |  2 +-\n 4 files changed, 22 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c733379..099d6ed 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@@ -826,23 +826,19 @@ static void write_pack_file(void)\n \t\t\t\t\t\ttmpname, 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 \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..9ccf804 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 basename_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,19 @@ 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_setlen(name_buffer, basename_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+\tstrbuf_setlen(name_buffer, basename_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"}]}