{"thread":{"id":"45390","subject":"[PATCH 1/6] index-pack: factor out pack/idx finalization","startedAt":"2017-03-15T21:27:42Z","lastAt":"2017-03-16T18:57:41Z","messageCount":25,"participants":["Jeff King","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"314175","messageId":"20170315212733.73cw22dm36oc5im4@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH 1/6] index-pack: factor out pack/idx finalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:27:33Z","receivedAt":"2017-03-15T21:27:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The procedure for moving the \".pack\" and the \".idx\" files\ninto place is the same. Rather than repeat the code, let's\nfactor it into a helper function.\n\nThis has the added benefit of clarifying the lifetime of\n\"final_pack_name\". In the original code it is sometimes\nredirected to our reusable \"name\" buffer; after we reuse\nthat buffer, the contents are nonsense (but nobody looks at\nit, so this is not a bug, just a maintenance issue). In the\nnew code, the pointer modification is contained in the\nhelper function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 40 +++++++++++++++++++---------------------\n 1 file changed, 19 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f4b87c6c9..187c0550c 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1380,6 +1380,23 @@ static void fix_unresolved_deltas(struct sha1file *f)\n \tfree(sorted_by_pos);\n }\n \n+static void finalize_file(const char *final_name, const char *curr_name,\n+\t\t\t  unsigned char *sha1, const char *ext)\n+{\n+\tif (final_name != curr_name) {\n+\t\tchar name[PATH_MAX];\n+\t\tif (!final_name) {\n+\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.%s\",\n+\t\t\t\t get_object_directory(), sha1_to_hex(sha1),\n+\t\t\t\t ext);\n+\t\t\tfinal_name = name;\n+\t\t}\n+\t\tif (finalize_object_file(curr_name, final_name))\n+\t\t\tdie(_(\"cannot store %s file\"), ext);\n+\t} else if (from_stdin)\n+\t\tchmod(final_name, 0444);\n+}\n+\n static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t  const char *final_index_name, const char *curr_index_name,\n \t\t  const char *keep_name, const char *keep_msg,\n@@ -1422,27 +1439,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t}\n \n-\tif (final_pack_name != curr_pack_name) {\n-\t\tif (!final_pack_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n-\t\t\tfinal_pack_name = name;\n-\t\t}\n-\t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n-\t\t\tdie(_(\"cannot store pack file\"));\n-\t} else if (from_stdin)\n-\t\tchmod(final_pack_name, 0444);\n-\n-\tif (final_index_name != curr_index_name) {\n-\t\tif (!final_index_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n-\t\t\tfinal_index_name = name;\n-\t\t}\n-\t\tif (finalize_object_file(curr_index_name, final_index_name))\n-\t\t\tdie(_(\"cannot store index file\"));\n-\t} else\n-\t\tchmod(final_index_name, 0444);\n+\tfinalize_file(final_pack_name, curr_pack_name, sha1, \"pack\");\n+\tfinalize_file(final_index_name, curr_index_name, sha1, \"idx\");\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314176","messageId":"20170315212747.ome5oth5xpwquf6o@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH 2/6] move odb_* declarations out of git-compat-util.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:27:47Z","receivedAt":"2017-03-15T21:27:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These functions were originally conceived as wrapper\nfunctions similar to xmkstemp(). They were later moved by\n463db9b10 (wrapper: move odb_* to environment.c,\n2010-11-06). The more appropriate place for a declaration is\nin cache.h.\n\nWhile we're at it, let's add some basic docstrings.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h           | 12 ++++++++++++\n git-compat-util.h |  2 --\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c95826971..68ad06e15 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1634,6 +1634,18 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n \n+/*\n+ * Create a temporary file rooted in the object database directory.\n+ */\n+extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n+\n+/*\n+ * Create a pack .keep file in the object database's pack directory, for\n+ * a pack with checksum \"sha1\". The return value is a file descriptor opened\n+ * for writing, or -1 on error. The name of the keep file is written to \"name\".\n+ */\n+extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n+\n /*\n  * mmap the index file for the specified packfile (if it is not\n  * already mmapped).  Return 0 on success.\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e626851fe..8a4a3f85e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -798,8 +798,6 @@ extern FILE *xfopen(const char *path, const char *mode);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\n-extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n extern char *xgetcwd(void);\n extern FILE *fopen_for_writing(const char *path);\n \n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314179","messageId":"20170315213024.jjhikkgwuhev27ec@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH 6/6] odb_pack_keep(): stop generating keepfile name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:30:24Z","receivedAt":"2017-03-15T21:31:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The odb_pack_keep() function generates the name of a .keep\nfile and opens it. This has two problems:\n\n  1. It requires a fixed-size buffer to create the filename\n     and doesn't notice when the result is truncated.\n\n  2. Of the two callers, one sometimes wants to open a\n     filename it already has, which makes things awkward (it\n     has to do so manually, and skips the leading-directory\n     creation).\n\nInstead, let's have odb_pack_keep() just open the file.\nCallers can use odb_pack_name() separately to generate the\nname. This simplifies the callers, and lets us drop any\nbuffer-size limitations.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 12 ++++++------\n cache.h              |  8 ++++----\n environment.c        |  6 ++----\n fast-import.c        | 18 +++++++++---------\n 4 files changed, 21 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b6e7ac331..e0f70836d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1399,7 +1399,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t  unsigned char *sha1)\n {\n \tconst char *report = \"pack\";\n-\tchar name[PATH_MAX];\n \tint err;\n \n \tif (!from_stdin) {\n@@ -1412,17 +1411,17 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t}\n \n \tif (keep_msg) {\n+\t\tstruct strbuf name = STRBUF_INIT;\n \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n \n \t\tif (!keep_name)\n-\t\t\tkeep_fd = odb_pack_keep(name, sizeof(name), sha1);\n-\t\telse\n-\t\t\tkeep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);\n+\t\t\tkeep_name = odb_pack_name(&name, sha1, \"keep\");\n \n+\t\tkeep_fd = odb_pack_keep(keep_name);\n \t\tif (keep_fd < 0) {\n \t\t\tif (errno != EEXIST)\n \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\n-\t\t\t\t\t  keep_name ? keep_name : name);\n+\t\t\t\t\t  keep_name);\n \t\t} else {\n \t\t\tif (keep_msg_len > 0) {\n \t\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n@@ -1430,9 +1429,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\t}\n \t\t\tif (close(keep_fd) != 0)\n \t\t\t\tdie_errno(_(\"cannot close written keep file '%s'\"),\n-\t\t\t\t\t  keep_name ? keep_name : name);\n+\t\t\t\t\t  keep_name);\n \t\t\treport = \"keep\";\n \t\t}\n+\t\tstrbuf_release(&name);\n \t}\n \n \tfinalize_file(final_pack_name, curr_pack_name, sha1, \"pack\");\ndiff --git a/cache.h b/cache.h\nindex 97896e2e5..cb8f48f6f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1649,11 +1649,11 @@ extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n /*\n- * Create a pack .keep file in the object database's pack directory, for\n- * a pack with checksum \"sha1\". The return value is a file descriptor opened\n- * for writing, or -1 on error. The name of the keep file is written to \"name\".\n+ * Create a pack .keep file named \"name\" (which should generally be the output\n+ * of odb_pack_name). Returns a file descriptor opened for writing, or -1 on\n+ * error.\n  */\n-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n+extern int odb_pack_keep(const char *name);\n \n /*\n  * mmap the index file for the specified packfile (if it is not\ndiff --git a/environment.c b/environment.c\nindex 42dc3106d..2fdba7622 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -296,18 +296,16 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)\n \treturn xmkstemp_mode(template, mode);\n }\n \n-int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1)\n+int odb_pack_keep(const char *name)\n {\n \tint fd;\n \n-\tsnprintf(name, namesz, \"%s/pack/pack-%s.keep\",\n-\t\t get_object_directory(), sha1_to_hex(sha1));\n \tfd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n \tif (0 <= fd)\n \t\treturn fd;\n \n \t/* slow path */\n-\tsafe_create_leading_directories(name);\n+\tsafe_create_leading_directories_const(name);\n \treturn open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n }\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 61be8b76e..41a539f97 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -940,28 +940,27 @@ static const char *create_index(void)\n \n static char *keep_pack(const char *curr_index_name)\n {\n-\tstatic char name[PATH_MAX];\n \tstatic const char *keep_msg = \"fast-import\";\n+\tstruct strbuf name = STRBUF_INIT;\n \tint keep_fd;\n \n-\tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n+\todb_pack_name(&name, pack_data->sha1, \"keep\");\n+\tkeep_fd = odb_pack_keep(name.buf);\n \tif (keep_fd < 0)\n \t\tdie_errno(\"cannot create keep file\");\n \twrite_or_die(keep_fd, keep_msg, strlen(keep_msg));\n \tif (close(keep_fd))\n \t\tdie_errno(\"failed to write keep file\");\n \n-\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (finalize_object_file(pack_data->pack_name, name))\n+\todb_pack_name(&name, pack_data->sha1, \"pack\");\n+\tif (finalize_object_file(pack_data->pack_name, name.buf))\n \t\tdie(\"cannot store pack file\");\n \n-\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (finalize_object_file(curr_index_name, name))\n+\todb_pack_name(&name, pack_data->sha1, \"idx\");\n+\tif (finalize_object_file(curr_index_name, name.buf))\n \t\tdie(\"cannot store index file\");\n \tfree((void *)curr_index_name);\n-\treturn name;\n+\treturn strbuf_detach(&name, NULL);\n }\n \n static void unkeep_all_packs(void)\n@@ -1033,6 +1032,7 @@ static void end_packfile(void)\n \t\t\tdie(\"core git rejected index %s\", idx_name);\n \t\tall_packs[pack_id] = new_p;\n \t\tinstall_packed_git(new_p);\n+\t\tfree(idx_name);\n \n \t\t/* Print the boundary */\n \t\tif (pack_edges) {\n-- \n2.12.0.613.g6e7c52a0d\n"},{"id":"314180","messageId":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","threadId":"45390","inReplyTo":null,"subject":"[PATCH 0/6] minor pack-name cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:26:17Z","receivedAt":"2017-03-15T21:33:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This started by cleaning up some unchecked snprintf() calls, but grew a\nfew related cleanups nearby.  I doubt the snprintf truncation affected\nanybody in practice, but I think the result is nicer to read.\n\n  [1/6]: index-pack: factor out pack/idx finalization\n  [2/6]: move odb_* declarations out of git-compat-util.h\n  [3/6]: sha1_file.c: make pack-name helper globally accessible\n  [4/6]: index-pack: drop fixed-size buffer for pack filenames\n  [5/6]: fast-import: replace fixed buffer with odb_pack_name\n  [6/6]: odb_pack_keep(): stop generating keepfile name\n\n builtin/index-pack.c | 48 +++++++++++++++++++++---------------------------\n cache.h              | 21 +++++++++++++++++++++\n environment.c        |  6 ++----\n fast-import.c        | 26 +++++++++++++-------------\n git-compat-util.h    |  2 --\n sha1_file.c          | 17 ++++++-----------\n 6 files changed, 63 insertions(+), 57 deletions(-)\n\n-Peff\n"},{"id":"314182","messageId":"20170315212818.yitqbv3dfvfb6t3u@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH 3/6] sha1_file.c: make pack-name helper globally accessible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:28:18Z","receivedAt":"2017-03-15T21:35:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We provide sha1_pack_name() and sha1_pack_index_name(), but\nthe more generic form (which takes its own strbuf and an\narbitrary extension) is only used to implement the other\ntwo.  Let's make it available, but clean up a few things:\n\n  1. Name it odb_pack_name(), as the original\n     sha1_get_pack_name() is long but not all that\n     descriptive.\n\n  2. Switch the strbuf argument to the beginning, so that it\n     matches similar path-building functions like\n     git_path_buf().\n\n  3. Clean up the out-dated docstring and move it to the\n     public declaration.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     |  9 +++++++++\n sha1_file.c | 17 ++++++-----------\n 2 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 68ad06e15..97896e2e5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1639,6 +1639,15 @@ extern void pack_report(void);\n  */\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n \n+/*\n+ * Generate the filename to be used for a pack file with checksum \"sha1\" and\n+ * extension \"ext\". The result is written into the strbuf \"buf\", overwriting\n+ * any existing contents. A pointer to buf->buf is returned as a convenience.\n+ *\n+ * Example: odb_pack_name(out, sha1, \"idx\") => \".git/objects/pack/pack-1234..idx\"\n+ */\n+extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n+\n /*\n  * Create a pack .keep file in the object database's pack directory, for\n  * a pack with checksum \"sha1\". The return value is a file descriptor opened\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2ee3c617a..56ef09cd3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -277,31 +277,26 @@ static const char *alt_sha1_path(struct alternate_object_database *alt,\n \treturn buf->buf;\n }\n \n-/*\n- * Return the name of the pack or index file with the specified sha1\n- * in its filename.  *base and *name are scratch space that must be\n- * provided by the caller.  which should be \"pack\" or \"idx\".\n- */\n-static char *sha1_get_pack_name(const unsigned char *sha1,\n-\t\t\t\tstruct strbuf *buf,\n-\t\t\t\tconst char *which)\n+ char *odb_pack_name(struct strbuf *buf,\n+\t\t     const unsigned char *sha1,\n+\t\t     const char *ext)\n {\n \tstrbuf_reset(buf);\n \tstrbuf_addf(buf, \"%s/pack/pack-%s.%s\", get_object_directory(),\n-\t\t    sha1_to_hex(sha1), which);\n+\t\t    sha1_to_hex(sha1), ext);\n \treturn buf->buf;\n }\n \n char *sha1_pack_name(const unsigned char *sha1)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n-\treturn sha1_get_pack_name(sha1, &buf, \"pack\");\n+\treturn odb_pack_name(&buf, sha1, \"pack\");\n }\n \n char *sha1_pack_index_name(const unsigned char *sha1)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n-\treturn sha1_get_pack_name(sha1, &buf, \"idx\");\n+\treturn odb_pack_name(&buf, sha1, \"idx\");\n }\n \n struct alternate_object_database *alt_odb_list;\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314183","messageId":"20170315212856.fetjp5cyu4l6juc7@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH 4/6] index-pack: drop fixed-size buffer for pack filenames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:28:56Z","receivedAt":"2017-03-15T21:36:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We write the name of the pack filename into a fixed-size\nbuffer using snprintf(), but do not check the return value.\nAs a result, a very long object directory could cause us to\nquietly truncate the pack filename (leading to a corrupted\nrepository, as the packfile would be missing its .pack\nextension).\n\nWe can use odb_pack_name() to fix this (and make the code\nsimpler, too).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 187c0550c..b6e7ac331 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1384,13 +1384,9 @@ static void finalize_file(const char *final_name, const char *curr_name,\n \t\t\t  unsigned char *sha1, const char *ext)\n {\n \tif (final_name != curr_name) {\n-\t\tchar name[PATH_MAX];\n-\t\tif (!final_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.%s\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1),\n-\t\t\t\t ext);\n-\t\t\tfinal_name = name;\n-\t\t}\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tif (!final_name)\n+\t\t\tfinal_name = odb_pack_name(&buf, sha1, ext);\n \t\tif (finalize_object_file(curr_name, final_name))\n \t\t\tdie(_(\"cannot store %s file\"), ext);\n \t} else if (from_stdin)\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314184","messageId":"20170315212908.tw2a7fqo3kgpa33w@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH 5/6] fast-import: replace fixed buffer with odb_pack_name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T21:29:08Z","receivedAt":"2017-03-15T21:36:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This removes the possibility of truncating the filename\nwithout realizing it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fast-import.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 6c13472c4..61be8b76e 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -966,15 +966,15 @@ static char *keep_pack(const char *curr_index_name)\n \n static void unkeep_all_packs(void)\n {\n-\tstatic char name[PATH_MAX];\n+\tstruct strbuf name = STRBUF_INIT;\n \tint k;\n \n \tfor (k = 0; k < pack_id; k++) {\n \t\tstruct packed_git *p = all_packs[k];\n-\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n-\t\t\t get_object_directory(), sha1_to_hex(p->sha1));\n-\t\tunlink_or_warn(name);\n+\t\todb_pack_name(&name, p->sha1, \"keep\");\n+\t\tunlink_or_warn(name.buf);\n \t}\n+\tstrbuf_release(&name);\n }\n \n static int loosen_small_pack(const struct packed_git *p)\n-- \n2.12.0.613.g6e7c52a0d\n\n"},{"id":"314190","messageId":"f42ba0f5-a879-f63f-1cbd-de268da05966@ramsayjones.plus.com","threadId":"45390","inReplyTo":"20170315212733.73cw22dm36oc5im4@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] index-pack: factor out pack/idx finalization","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-15T22:03:56Z","receivedAt":"2017-03-15T22:05:26Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/03/17 21:27, Jeff King wrote:\n> The procedure for moving the \".pack\" and the \".idx\" files\n> into place is the same. Rather than repeat the code, let's\n> factor it into a helper function.\n> \n> This has the added benefit of clarifying the lifetime of\n> \"final_pack_name\". In the original code it is sometimes\n> redirected to our reusable \"name\" buffer; after we reuse\n> that buffer, the contents are nonsense (but nobody looks at\n> it, so this is not a bug, just a maintenance issue). In the\n> new code, the pointer modification is contained in the\n> helper function.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/index-pack.c | 40 +++++++++++++++++++---------------------\n>  1 file changed, 19 insertions(+), 21 deletions(-)\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index f4b87c6c9..187c0550c 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1380,6 +1380,23 @@ static void fix_unresolved_deltas(struct sha1file *f)\n>  \tfree(sorted_by_pos);\n>  }\n>  \n> +static void finalize_file(const char *final_name, const char *curr_name,\n> +\t\t\t  unsigned char *sha1, const char *ext)\n> +{\n> +\tif (final_name != curr_name) {\n> +\t\tchar name[PATH_MAX];\n> +\t\tif (!final_name) {\n> +\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.%s\",\n> +\t\t\t\t get_object_directory(), sha1_to_hex(sha1),\n> +\t\t\t\t ext);\n> +\t\t\tfinal_name = name;\n> +\t\t}\n> +\t\tif (finalize_object_file(curr_name, final_name))\n> +\t\t\tdie(_(\"cannot store %s file\"), ext);\n> +\t} else if (from_stdin)\n> +\t\tchmod(final_name, 0444);\n> +}\n> +\n>  static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\t  const char *final_index_name, const char *curr_index_name,\n>  \t\t  const char *keep_name, const char *keep_msg,\n> @@ -1422,27 +1439,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\t}\n>  \t}\n>  \n> -\tif (final_pack_name != curr_pack_name) {\n> -\t\tif (!final_pack_name) {\n> -\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n> -\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n> -\t\t\tfinal_pack_name = name;\n> -\t\t}\n> -\t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n> -\t\t\tdie(_(\"cannot store pack file\"));\n> -\t} else if (from_stdin)\n> -\t\tchmod(final_pack_name, 0444);\n> -\n> -\tif (final_index_name != curr_index_name) {\n> -\t\tif (!final_index_name) {\n> -\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n> -\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n> -\t\t\tfinal_index_name = name;\n> -\t\t}\n> -\t\tif (finalize_object_file(curr_index_name, final_index_name))\n> -\t\t\tdie(_(\"cannot store index file\"));\n> -\t} else\n> -\t\tchmod(final_index_name, 0444);\n\nIs from_stdin always true if final_index_name == curr_index_name?\nWas the original asymmetry deliberate?\n\n> +\tfinalize_file(final_pack_name, curr_pack_name, sha1, \"pack\");\n> +\tfinalize_file(final_index_name, curr_index_name, sha1, \"idx\");\n>  \n>  \tif (!from_stdin) {\n>  \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n> \n\nATB,\nRamsay Jones\n\n"},{"id":"314192","messageId":"20170315222223.qyrxb6ey3rz3omt4@sigill.intra.peff.net","threadId":"45390","inReplyTo":"f42ba0f5-a879-f63f-1cbd-de268da05966@ramsayjones.plus.com","subject":"Re: [PATCH 1/6] index-pack: factor out pack/idx finalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T22:22:23Z","receivedAt":"2017-03-15T22:22:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 15, 2017 at 10:03:56PM +0000, Ramsay Jones wrote:\n\n> > -\tif (final_pack_name != curr_pack_name) {\n> > -\t\tif (!final_pack_name) {\n> > -\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n> > -\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n> > -\t\t\tfinal_pack_name = name;\n> > -\t\t}\n> > -\t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n> > -\t\t\tdie(_(\"cannot store pack file\"));\n> > -\t} else if (from_stdin)\n> > -\t\tchmod(final_pack_name, 0444);\n> > -\n> > -\tif (final_index_name != curr_index_name) {\n> > -\t\tif (!final_index_name) {\n> > -\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n> > -\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n> > -\t\t\tfinal_index_name = name;\n> > -\t\t}\n> > -\t\tif (finalize_object_file(curr_index_name, final_index_name))\n> > -\t\t\tdie(_(\"cannot store index file\"));\n> > -\t} else\n> > -\t\tchmod(final_index_name, 0444);\n> \n> Is from_stdin always true if final_index_name == curr_index_name?\n> Was the original asymmetry deliberate?\n\nHrm, good question.\n\nI think the logic on the pack side is that:\n\n  git index-pack --stdin\n\nwill write a new pack, and we should chmod it as appropriate. But it\nwould be wrong to do so when generating an index for an existing pack:\n\n  git index-pack foo.pack\n\nWhatever wrote foo.pack is responsible for the chmod then.\n\nSo the flip side (and what your question is asking) is whether it is\nsafe to skip the .idx chmod in the non-stdin case.\n\nUsually when we don't have an existing .idx file, we write to a new\ntempfile.  We obviously want to chmod there, but it would fall under the\n\"final_index_name != curr_index_name\" case.\n\nBut it looks like you can give \"-o\", or we can derive the name from the\n.pack name. So if we do:\n\n  git pack-objects --all --stdout >foo.pack\n  git index-pack foo.pack\n\nThen we'd expect the newly-created .idx to have mode 0444, but it\ndoesn't. So yeah, I think the distinction does matter.\n\nI'm not sure if the best path is to include that flag in the\nfinalize_file() helper, or just ditch the helper.  Its main purpose was\ncleanup so that the odb_pack_name() refactor didn't have to happen\ntwice. But after that refactor, the amount of shared code is relatively\nsmall.\n\n-Peff\n"},{"id":"314193","messageId":"20170315223033.phxiksglwjm3j556@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315222223.qyrxb6ey3rz3omt4@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] index-pack: factor out pack/idx finalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-15T22:30:33Z","receivedAt":"2017-03-15T22:31:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 15, 2017 at 06:22:23PM -0400, Jeff King wrote:\n\n> Then we'd expect the newly-created .idx to have mode 0444, but it\n> doesn't. So yeah, I think the distinction does matter.\n> \n> I'm not sure if the best path is to include that flag in the\n> finalize_file() helper, or just ditch the helper.  Its main purpose was\n> cleanup so that the odb_pack_name() refactor didn't have to happen\n> twice. But after that refactor, the amount of shared code is relatively\n> small.\n\nI think I'm leaning towards just ditching the helper. The resulting\nchange looks pretty good:\n\n@@ -1423,22 +1424,16 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t}\n \n \tif (final_pack_name != curr_pack_name) {\n-\t\tif (!final_pack_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n-\t\t\tfinal_pack_name = name;\n-\t\t}\n+\t\tif (!final_pack_name)\n+\t\t\tfinal_pack_name = odb_pack_name(&name, sha1, \"pack\");\n \t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n \t\t\tdie(_(\"cannot store pack file\"));\n \t} else if (from_stdin)\n \t\tchmod(final_pack_name, 0444);\n \n \tif (final_index_name != curr_index_name) {\n-\t\tif (!final_index_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n-\t\t\tfinal_index_name = name;\n-\t\t}\n+\t\tif (!final_index_name)\n+\t\t\tfinal_index_name = odb_pack_name(&name, sha1, \"idx\");\n \t\tif (finalize_object_file(curr_index_name, final_index_name))\n \t\t\tdie(_(\"cannot store index file\"));\n \t} else\n\nAnd we just end up sharing the same name buffer (and the \"keep\" code\npath can use the same one, too).\n\nI'll wait until tomorrow to collect any more comments and then send out\na re-roll.\n\nThanks for reading so carefully.\n\n-Peff\n"},{"id":"314203","messageId":"xmqqzigmc8u5.fsf@gitster.mtv.corp.google.com","threadId":"45390","inReplyTo":"20170315213024.jjhikkgwuhev27ec@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] odb_pack_keep(): stop generating keepfile name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T01:24:18Z","receivedAt":"2017-03-16T01:24:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The odb_pack_keep() function generates the name of a .keep\n> file and opens it. This has two problems:\n>\n>   1. It requires a fixed-size buffer to create the filename\n>      and doesn't notice when the result is truncated.\n>\n>   2. Of the two callers, one sometimes wants to open a\n>      filename it already has, which makes things awkward (it\n>      has to do so manually, and skips the leading-directory\n>      creation).\n>\n> Instead, let's have odb_pack_keep() just open the file.\n> Callers can use odb_pack_name() separately to generate the\n> name. This simplifies the callers, and lets us drop any\n> buffer-size limitations.\n\nThat's sensible (and all the others made the resulting code much\nmore pleasant to the eyes).\n\n"},{"id":"314218","messageId":"20170316142647.t6tthkcgon3rpg4m@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net","subject":"[PATCH v2 0/5] minor pack-name cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:26:47Z","receivedAt":"2017-03-16T14:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of the series from:\n\n  http://public-inbox.org/git/20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net/\n\nThe general gist is the same, but there are a number of changes:\n\n  - I dropped the first patch factoring out finalize_file(), as Ramsay\n    pointed out a subtle difference between the index and pack handling.\n\n  - I added an extra patch on top (5/5 here) to get the same\n    pointer-aliasing safety that the factored-out function got us\n    (more so, actually, as it protects keep_msg, too).\n\n  - I re-ordered the odb_pack_keep() cleanup before the odb_pack_name()\n    cleanups, which lets us do the latter in one swoop (and avoids\n    explaining \"well, we can't do .keep yet, because...\" in the commit\n    message)\n\n  - The original had two patches doing the odb_pack_name() conversion.\n    Now that it has fewer caveats, I felt comfortable lumping it all\n    into one (patch 4/5 here).\n\n  [1/5]: move odb_* declarations out of git-compat-util.h\n  [2/5]: sha1_file.c: make pack-name helper globally accessible\n  [3/5]: odb_pack_keep(): stop generating keepfile name\n  [4/5]: replace snprintf with odb_pack_name()\n  [5/5]: index-pack: make pointer-alias fallbacks safer\n\n builtin/index-pack.c | 31 +++++++++++++++----------------\n cache.h              | 21 +++++++++++++++++++++\n environment.c        |  6 ++----\n fast-import.c        | 26 +++++++++++++-------------\n git-compat-util.h    |  2 --\n sha1_file.c          | 17 ++++++-----------\n 6 files changed, 57 insertions(+), 46 deletions(-)\n\n"},{"id":"314219","messageId":"20170316142700.llf6zpiss5l4xurs@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170316142647.t6tthkcgon3rpg4m@sigill.intra.peff.net","subject":"[PATCH v2 1/5] move odb_* declarations out of git-compat-util.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:27:00Z","receivedAt":"2017-03-16T14:27:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These functions were originally conceived as wrapper\nfunctions similar to xmkstemp(). They were later moved by\n463db9b10 (wrapper: move odb_* to environment.c,\n2010-11-06). The more appropriate place for a declaration is\nin cache.h.\n\nWhile we're at it, let's add some basic docstrings.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h           | 12 ++++++++++++\n git-compat-util.h |  2 --\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c95826971..68ad06e15 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1634,6 +1634,18 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n \n+/*\n+ * Create a temporary file rooted in the object database directory.\n+ */\n+extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n+\n+/*\n+ * Create a pack .keep file in the object database's pack directory, for\n+ * a pack with checksum \"sha1\". The return value is a file descriptor opened\n+ * for writing, or -1 on error. The name of the keep file is written to \"name\".\n+ */\n+extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n+\n /*\n  * mmap the index file for the specified packfile (if it is not\n  * already mmapped).  Return 0 on success.\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e626851fe..8a4a3f85e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -798,8 +798,6 @@ extern FILE *xfopen(const char *path, const char *mode);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\n-extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n extern char *xgetcwd(void);\n extern FILE *fopen_for_writing(const char *path);\n \n-- \n2.12.0.623.g86ec6c963\n\n"},{"id":"314220","messageId":"20170316142706.kt56a43phiuhhra7@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170316142647.t6tthkcgon3rpg4m@sigill.intra.peff.net","subject":"[PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:27:06Z","receivedAt":"2017-03-16T14:27:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We provide sha1_pack_name() and sha1_pack_index_name(), but\nthe more generic form (which takes its own strbuf and an\narbitrary extension) is only used to implement the other\ntwo.  Let's make it available, but clean up a few things:\n\n  1. Name it odb_pack_name(), as the original\n     sha1_get_pack_name() is long but not all that\n     descriptive.\n\n  2. Switch the strbuf argument to the beginning, so that it\n     matches similar path-building functions like\n     git_path_buf().\n\n  3. Clean up the out-dated docstring and move it to the\n     public declaration.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     |  9 +++++++++\n sha1_file.c | 17 ++++++-----------\n 2 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 68ad06e15..97896e2e5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1639,6 +1639,15 @@ extern void pack_report(void);\n  */\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n \n+/*\n+ * Generate the filename to be used for a pack file with checksum \"sha1\" and\n+ * extension \"ext\". The result is written into the strbuf \"buf\", overwriting\n+ * any existing contents. A pointer to buf->buf is returned as a convenience.\n+ *\n+ * Example: odb_pack_name(out, sha1, \"idx\") => \".git/objects/pack/pack-1234..idx\"\n+ */\n+extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n+\n /*\n  * Create a pack .keep file in the object database's pack directory, for\n  * a pack with checksum \"sha1\". The return value is a file descriptor opened\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2ee3c617a..56ef09cd3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -277,31 +277,26 @@ static const char *alt_sha1_path(struct alternate_object_database *alt,\n \treturn buf->buf;\n }\n \n-/*\n- * Return the name of the pack or index file with the specified sha1\n- * in its filename.  *base and *name are scratch space that must be\n- * provided by the caller.  which should be \"pack\" or \"idx\".\n- */\n-static char *sha1_get_pack_name(const unsigned char *sha1,\n-\t\t\t\tstruct strbuf *buf,\n-\t\t\t\tconst char *which)\n+ char *odb_pack_name(struct strbuf *buf,\n+\t\t     const unsigned char *sha1,\n+\t\t     const char *ext)\n {\n \tstrbuf_reset(buf);\n \tstrbuf_addf(buf, \"%s/pack/pack-%s.%s\", get_object_directory(),\n-\t\t    sha1_to_hex(sha1), which);\n+\t\t    sha1_to_hex(sha1), ext);\n \treturn buf->buf;\n }\n \n char *sha1_pack_name(const unsigned char *sha1)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n-\treturn sha1_get_pack_name(sha1, &buf, \"pack\");\n+\treturn odb_pack_name(&buf, sha1, \"pack\");\n }\n \n char *sha1_pack_index_name(const unsigned char *sha1)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n-\treturn sha1_get_pack_name(sha1, &buf, \"idx\");\n+\treturn odb_pack_name(&buf, sha1, \"idx\");\n }\n \n struct alternate_object_database *alt_odb_list;\n-- \n2.12.0.623.g86ec6c963\n\n"},{"id":"314221","messageId":"20170316142712.rg66wvz2fltazz44@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170316142647.t6tthkcgon3rpg4m@sigill.intra.peff.net","subject":"[PATCH v2 3/5] odb_pack_keep(): stop generating keepfile name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:27:12Z","receivedAt":"2017-03-16T14:27:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The odb_pack_keep() function generates the name of a .keep\nfile and opens it. This has two problems:\n\n  1. It requires a fixed-size buffer to create the filename\n     and doesn't notice when the result is truncated.\n\n  2. Of the two callers, one sometimes wants to open a\n     filename it already has, which makes things awkward (it\n     has to do so manually, and skips the leading-directory\n     creation).\n\nInstead, let's have odb_pack_keep() just open the file.\nGenerating the name isn't hard, and a future patch will\nswitch callers over to odb_pack_name() anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 6 +++---\n cache.h              | 8 ++++----\n environment.c        | 6 ++----\n fast-import.c        | 4 +++-\n 4 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f4b87c6c9..a58bc6bee 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1402,10 +1402,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n \n \t\tif (!keep_name)\n-\t\t\tkeep_fd = odb_pack_keep(name, sizeof(name), sha1);\n-\t\telse\n-\t\t\tkeep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);\n+\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n+\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n \n+\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name);\n \t\tif (keep_fd < 0) {\n \t\t\tif (errno != EEXIST)\n \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\ndiff --git a/cache.h b/cache.h\nindex 97896e2e5..cb8f48f6f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1649,11 +1649,11 @@ extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n /*\n- * Create a pack .keep file in the object database's pack directory, for\n- * a pack with checksum \"sha1\". The return value is a file descriptor opened\n- * for writing, or -1 on error. The name of the keep file is written to \"name\".\n+ * Create a pack .keep file named \"name\" (which should generally be the output\n+ * of odb_pack_name). Returns a file descriptor opened for writing, or -1 on\n+ * error.\n  */\n-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);\n+extern int odb_pack_keep(const char *name);\n \n /*\n  * mmap the index file for the specified packfile (if it is not\ndiff --git a/environment.c b/environment.c\nindex 42dc3106d..2fdba7622 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -296,18 +296,16 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)\n \treturn xmkstemp_mode(template, mode);\n }\n \n-int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1)\n+int odb_pack_keep(const char *name)\n {\n \tint fd;\n \n-\tsnprintf(name, namesz, \"%s/pack/pack-%s.keep\",\n-\t\t get_object_directory(), sha1_to_hex(sha1));\n \tfd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n \tif (0 <= fd)\n \t\treturn fd;\n \n \t/* slow path */\n-\tsafe_create_leading_directories(name);\n+\tsafe_create_leading_directories_const(name);\n \treturn open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n }\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 6c13472c4..dad697653 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -944,7 +944,9 @@ static char *keep_pack(const char *curr_index_name)\n \tstatic const char *keep_msg = \"fast-import\";\n \tint keep_fd;\n \n-\tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n+\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n+\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n+\tkeep_fd = odb_pack_keep(name);\n \tif (keep_fd < 0)\n \t\tdie_errno(\"cannot create keep file\");\n \twrite_or_die(keep_fd, keep_msg, strlen(keep_msg));\n-- \n2.12.0.623.g86ec6c963\n\n"},{"id":"314222","messageId":"20170316142715.zdoikgyy3difeafk@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170316142647.t6tthkcgon3rpg4m@sigill.intra.peff.net","subject":"[PATCH v2 4/5] replace snprintf with odb_pack_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:27:15Z","receivedAt":"2017-03-16T14:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In several places we write the name of the pack filename\ninto a fixed-size buffer using snprintf(), but do not check\nthe return value.  As a result, a very long object directory\ncould cause us to quietly truncate the pack filename\n(potentially leading to a corrupted repository, as a newly\nwritten packfile could be missing its .pack extension).\n\nWe can use odb_pack_name() to do this with a strbuf (and\nshorten the code, as well).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 27 +++++++++++----------------\n fast-import.c        | 28 +++++++++++++---------------\n 2 files changed, 24 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a58bc6bee..dcb346ab7 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1386,7 +1386,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t  unsigned char *sha1)\n {\n \tconst char *report = \"pack\";\n-\tchar name[PATH_MAX];\n+\tstruct strbuf name = STRBUF_INIT;\n \tint err;\n \n \tif (!from_stdin) {\n@@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n \n \t\tif (!keep_name)\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n+\t\t\todb_pack_name(&name, sha1, \"keep\");\n \n-\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name);\n+\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);\n \t\tif (keep_fd < 0) {\n \t\t\tif (errno != EEXIST)\n \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\n-\t\t\t\t\t  keep_name ? keep_name : name);\n+\t\t\t\t\t  keep_name ? keep_name : name.buf);\n \t\t} else {\n \t\t\tif (keep_msg_len > 0) {\n \t\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n@@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\t}\n \t\t\tif (close(keep_fd) != 0)\n \t\t\t\tdie_errno(_(\"cannot close written keep file '%s'\"),\n-\t\t\t\t\t  keep_name ? keep_name : name);\n+\t\t\t\t\t  keep_name ? keep_name : name.buf);\n \t\t\treport = \"keep\";\n \t\t}\n \t}\n \n \tif (final_pack_name != curr_pack_name) {\n-\t\tif (!final_pack_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n-\t\t\tfinal_pack_name = name;\n-\t\t}\n+\t\tif (!final_pack_name)\n+\t\t\tfinal_pack_name = odb_pack_name(&name, sha1, \"pack\");\n \t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n \t\t\tdie(_(\"cannot store pack file\"));\n \t} else if (from_stdin)\n \t\tchmod(final_pack_name, 0444);\n \n \tif (final_index_name != curr_index_name) {\n-\t\tif (!final_index_name) {\n-\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n-\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n-\t\t\tfinal_index_name = name;\n-\t\t}\n+\t\tif (!final_index_name)\n+\t\t\tfinal_index_name = odb_pack_name(&name, sha1, \"idx\");\n \t\tif (finalize_object_file(curr_index_name, final_index_name))\n \t\t\tdie(_(\"cannot store index file\"));\n \t} else\n@@ -1464,6 +1457,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\tinput_offset += err;\n \t\t}\n \t}\n+\n+\tstrbuf_release(&name);\n }\n \n static int git_index_pack_config(const char *k, const char *v, void *cb)\ndiff --git a/fast-import.c b/fast-import.c\nindex dad697653..41a539f97 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -940,43 +940,40 @@ static const char *create_index(void)\n \n static char *keep_pack(const char *curr_index_name)\n {\n-\tstatic char name[PATH_MAX];\n \tstatic const char *keep_msg = \"fast-import\";\n+\tstruct strbuf name = STRBUF_INIT;\n \tint keep_fd;\n \n-\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tkeep_fd = odb_pack_keep(name);\n+\todb_pack_name(&name, pack_data->sha1, \"keep\");\n+\tkeep_fd = odb_pack_keep(name.buf);\n \tif (keep_fd < 0)\n \t\tdie_errno(\"cannot create keep file\");\n \twrite_or_die(keep_fd, keep_msg, strlen(keep_msg));\n \tif (close(keep_fd))\n \t\tdie_errno(\"failed to write keep file\");\n \n-\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (finalize_object_file(pack_data->pack_name, name))\n+\todb_pack_name(&name, pack_data->sha1, \"pack\");\n+\tif (finalize_object_file(pack_data->pack_name, name.buf))\n \t\tdie(\"cannot store pack file\");\n \n-\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (finalize_object_file(curr_index_name, name))\n+\todb_pack_name(&name, pack_data->sha1, \"idx\");\n+\tif (finalize_object_file(curr_index_name, name.buf))\n \t\tdie(\"cannot store index file\");\n \tfree((void *)curr_index_name);\n-\treturn name;\n+\treturn strbuf_detach(&name, NULL);\n }\n \n static void unkeep_all_packs(void)\n {\n-\tstatic char name[PATH_MAX];\n+\tstruct strbuf name = STRBUF_INIT;\n \tint k;\n \n \tfor (k = 0; k < pack_id; k++) {\n \t\tstruct packed_git *p = all_packs[k];\n-\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n-\t\t\t get_object_directory(), sha1_to_hex(p->sha1));\n-\t\tunlink_or_warn(name);\n+\t\todb_pack_name(&name, p->sha1, \"keep\");\n+\t\tunlink_or_warn(name.buf);\n \t}\n+\tstrbuf_release(&name);\n }\n \n static int loosen_small_pack(const struct packed_git *p)\n@@ -1035,6 +1032,7 @@ static void end_packfile(void)\n \t\t\tdie(\"core git rejected index %s\", idx_name);\n \t\tall_packs[pack_id] = new_p;\n \t\tinstall_packed_git(new_p);\n+\t\tfree(idx_name);\n \n \t\t/* Print the boundary */\n \t\tif (pack_edges) {\n-- \n2.12.0.623.g86ec6c963\n\n"},{"id":"314223","messageId":"20170316142720.377auysntqu7ozdz@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170316142647.t6tthkcgon3rpg4m@sigill.intra.peff.net","subject":"[PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:27:20Z","receivedAt":"2017-03-16T14:27:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The final() function accepts a NULL value for certain\nparameters, and falls back to writing into a reusable \"name\"\nbuffer, and then either:\n\n  1. For \"keep_name\", requiring all uses to do \"keep_name ?\n     keep_name : name.buf\". This is awkward, and it's easy\n     to accidentally look at the maybe-NULL keep_name.\n\n  2. For \"final_index_name\" and \"final_pack_name\", aliasing\n     those pointers to the \"name\" buffer. This is easier to\n     use, but the aliased pointers become invalid after the\n     buffer is reused (this isn't a bug now, but it's a\n     potential pitfall).\n\nOne way to make this safer would be to introduce an extra\npointer to do the aliasing, and have its lifetime match the\nvalidity of the \"name\" buffer. But it's still easy to\naccidentally use the wrong name (i.e., to use\n\"final_pack_name\" instead of the aliased pointer).\n\nInstead, let's use three separate buffers that will remain\nvalid through the function. That makes it safe to alias the\npointers and use them consistently. The extra allocations\nshouldn't matter, as this function is not performance\nsensitive.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex dcb346ab7..88d205f85 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1386,7 +1386,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t  unsigned char *sha1)\n {\n \tconst char *report = \"pack\";\n-\tstruct strbuf name = STRBUF_INIT;\n+\tstruct strbuf pack_name = STRBUF_INIT;\n+\tstruct strbuf index_name = STRBUF_INIT;\n+\tstruct strbuf keep_name_buf = STRBUF_INIT;\n \tint err;\n \n \tif (!from_stdin) {\n@@ -1402,13 +1404,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n \n \t\tif (!keep_name)\n-\t\t\todb_pack_name(&name, sha1, \"keep\");\n+\t\t\tkeep_name = odb_pack_name(&keep_name_buf, sha1, \"keep\");\n \n-\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);\n+\t\tkeep_fd = odb_pack_keep(keep_name);\n \t\tif (keep_fd < 0) {\n \t\t\tif (errno != EEXIST)\n \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\n-\t\t\t\t\t  keep_name ? keep_name : name.buf);\n+\t\t\t\t\t  keep_name);\n \t\t} else {\n \t\t\tif (keep_msg_len > 0) {\n \t\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n@@ -1416,14 +1418,14 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\t}\n \t\t\tif (close(keep_fd) != 0)\n \t\t\t\tdie_errno(_(\"cannot close written keep file '%s'\"),\n-\t\t\t\t\t  keep_name ? keep_name : name.buf);\n+\t\t\t\t\t  keep_name);\n \t\t\treport = \"keep\";\n \t\t}\n \t}\n \n \tif (final_pack_name != curr_pack_name) {\n \t\tif (!final_pack_name)\n-\t\t\tfinal_pack_name = odb_pack_name(&name, sha1, \"pack\");\n+\t\t\tfinal_pack_name = odb_pack_name(&pack_name, sha1, \"pack\");\n \t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n \t\t\tdie(_(\"cannot store pack file\"));\n \t} else if (from_stdin)\n@@ -1431,7 +1433,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \n \tif (final_index_name != curr_index_name) {\n \t\tif (!final_index_name)\n-\t\t\tfinal_index_name = odb_pack_name(&name, sha1, \"idx\");\n+\t\t\tfinal_index_name = odb_pack_name(&index_name, sha1, \"idx\");\n \t\tif (finalize_object_file(curr_index_name, final_index_name))\n \t\t\tdie(_(\"cannot store index file\"));\n \t} else\n@@ -1458,7 +1460,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t}\n \n-\tstrbuf_release(&name);\n+\tstrbuf_release(&index_name);\n+\tstrbuf_release(&pack_name);\n+\tstrbuf_release(&keep_name_buf);\n }\n \n static int git_index_pack_config(const char *k, const char *v, void *cb)\n-- \n2.12.0.623.g86ec6c963\n"},{"id":"314224","messageId":"20170316143152.gdeaprlsercvwmiw@sigill.intra.peff.net","threadId":"45390","inReplyTo":"20170316142706.kt56a43phiuhhra7@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T14:31:52Z","receivedAt":"2017-03-16T15:12:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 16, 2017 at 10:27:06AM -0400, Jeff King wrote:\n\n> -/*\n> - * Return the name of the pack or index file with the specified sha1\n> - * in its filename.  *base and *name are scratch space that must be\n> - * provided by the caller.  which should be \"pack\" or \"idx\".\n> - */\n> -static char *sha1_get_pack_name(const unsigned char *sha1,\n> -\t\t\t\tstruct strbuf *buf,\n> -\t\t\t\tconst char *which)\n> + char *odb_pack_name(struct strbuf *buf,\n> +\t\t     const unsigned char *sha1,\n> +\t\t     const char *ext)\n>  {\n>  \tstrbuf_reset(buf);\n>  \tstrbuf_addf(buf, \"%s/pack/pack-%s.%s\", get_object_directory(),\n> -\t\t    sha1_to_hex(sha1), which);\n> +\t\t    sha1_to_hex(sha1), ext);\n>  \treturn buf->buf;\n>  }\n\nIncidentally, this entire function could be implemented as:\n\n  return git_path_buf(buf, \"objects/pack/pack-%s.%s\",\n                      sha1_to_hex(sha1), ext);\n\nas the git_path() functions are smart enough to replace \"objects/\" with\nthe true object directory when necessary. I don't know if people find\nthat more or less readable. Since it's buried in a helper function, I\ndoubt it matters much either way. The git_path functions do also do some\npath normalization, which might be of value.\n\n-Peff\n"},{"id":"314225","messageId":"1d148aca-9602-200c-2356-7b16b8c32df7@ramsayjones.plus.com","threadId":"45390","inReplyTo":"20170316142715.zdoikgyy3difeafk@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-16T15:37:50Z","receivedAt":"2017-03-16T15:39:08Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 16/03/17 14:27, Jeff King wrote:\n> In several places we write the name of the pack filename\n> into a fixed-size buffer using snprintf(), but do not check\n> the return value.  As a result, a very long object directory\n> could cause us to quietly truncate the pack filename\n> (potentially leading to a corrupted repository, as a newly\n> written packfile could be missing its .pack extension).\n> \n> We can use odb_pack_name() to do this with a strbuf (and\n> shorten the code, as well).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/index-pack.c | 27 +++++++++++----------------\n>  fast-import.c        | 28 +++++++++++++---------------\n>  2 files changed, 24 insertions(+), 31 deletions(-)\n> \n[snip]\n\n> diff --git a/fast-import.c b/fast-import.c\n> index dad697653..41a539f97 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -940,43 +940,40 @@ static const char *create_index(void)\n>  \n>  static char *keep_pack(const char *curr_index_name)\n>  {\n> -\tstatic char name[PATH_MAX];\n>  \tstatic const char *keep_msg = \"fast-import\";\n> +\tstruct strbuf name = STRBUF_INIT;\n>  \tint keep_fd;\n>  \n> -\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n> -\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n> -\tkeep_fd = odb_pack_keep(name);\n> +\todb_pack_name(&name, pack_data->sha1, \"keep\");\n> +\tkeep_fd = odb_pack_keep(name.buf);\n>  \tif (keep_fd < 0)\n>  \t\tdie_errno(\"cannot create keep file\");\n>  \twrite_or_die(keep_fd, keep_msg, strlen(keep_msg));\n>  \tif (close(keep_fd))\n>  \t\tdie_errno(\"failed to write keep file\");\n>  \n> -\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n> -\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n> -\tif (finalize_object_file(pack_data->pack_name, name))\n> +\todb_pack_name(&name, pack_data->sha1, \"pack\");\n> +\tif (finalize_object_file(pack_data->pack_name, name.buf))\n>  \t\tdie(\"cannot store pack file\");\n>  \n> -\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n> -\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n> -\tif (finalize_object_file(curr_index_name, name))\n> +\todb_pack_name(&name, pack_data->sha1, \"idx\");\n> +\tif (finalize_object_file(curr_index_name, name.buf))\n>  \t\tdie(\"cannot store index file\");\n>  \tfree((void *)curr_index_name);\n> -\treturn name;\n> +\treturn strbuf_detach(&name, NULL);\n\nHmm, who deletes this, ...\n\n>  }\n>  \n>  static void unkeep_all_packs(void)\n>  {\n> -\tstatic char name[PATH_MAX];\n> +\tstruct strbuf name = STRBUF_INIT;\n>  \tint k;\n>  \n>  \tfor (k = 0; k < pack_id; k++) {\n>  \t\tstruct packed_git *p = all_packs[k];\n> -\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n> -\t\t\t get_object_directory(), sha1_to_hex(p->sha1));\n> -\t\tunlink_or_warn(name);\n> +\t\todb_pack_name(&name, p->sha1, \"keep\");\n> +\t\tunlink_or_warn(name.buf);\n>  \t}\n> +\tstrbuf_release(&name);\n>  }\n>  \n>  static int loosen_small_pack(const struct packed_git *p)\n> @@ -1035,6 +1032,7 @@ static void end_packfile(void)\n>  \t\t\tdie(\"core git rejected index %s\", idx_name);\n>  \t\tall_packs[pack_id] = new_p;\n>  \t\tinstall_packed_git(new_p);\n> +\t\tfree(idx_name);\n\nAh, OK. Yep, no leaks here. ;-)\n\n>  \n>  \t\t/* Print the boundary */\n>  \t\tif (pack_edges) {\n> \n\nATB,\nRamsay Jones\n\n"},{"id":"314226","messageId":"93f87c64-5752-06a2-9350-c867f8abe06a@ramsayjones.plus.com","threadId":"45390","inReplyTo":"20170316142720.377auysntqu7ozdz@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-16T15:42:54Z","receivedAt":"2017-03-16T15:44:01Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 16/03/17 14:27, Jeff King wrote:\n> The final() function accepts a NULL value for certain\n> parameters, and falls back to writing into a reusable \"name\"\n> buffer, and then either:\n> \n>   1. For \"keep_name\", requiring all uses to do \"keep_name ?\n>      keep_name : name.buf\". This is awkward, and it's easy\n>      to accidentally look at the maybe-NULL keep_name.\n> \n>   2. For \"final_index_name\" and \"final_pack_name\", aliasing\n>      those pointers to the \"name\" buffer. This is easier to\n>      use, but the aliased pointers become invalid after the\n>      buffer is reused (this isn't a bug now, but it's a\n>      potential pitfall).\n> \n> One way to make this safer would be to introduce an extra\n> pointer to do the aliasing, and have its lifetime match the\n> validity of the \"name\" buffer. But it's still easy to\n> accidentally use the wrong name (i.e., to use\n> \"final_pack_name\" instead of the aliased pointer).\n> \n> Instead, let's use three separate buffers that will remain\n> valid through the function. That makes it safe to alias the\n> pointers and use them consistently. The extra allocations\n> shouldn't matter, as this function is not performance\n> sensitive.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/index-pack.c | 20 ++++++++++++--------\n>  1 file changed, 12 insertions(+), 8 deletions(-)\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index dcb346ab7..88d205f85 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1386,7 +1386,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\t  unsigned char *sha1)\n>  {\n>  \tconst char *report = \"pack\";\n> -\tstruct strbuf name = STRBUF_INIT;\n> +\tstruct strbuf pack_name = STRBUF_INIT;\n> +\tstruct strbuf index_name = STRBUF_INIT;\n> +\tstruct strbuf keep_name_buf = STRBUF_INIT;\n>  \tint err;\n>  \n>  \tif (!from_stdin) {\n> @@ -1402,13 +1404,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n>  \n>  \t\tif (!keep_name)\n> -\t\t\todb_pack_name(&name, sha1, \"keep\");\n> +\t\t\tkeep_name = odb_pack_name(&keep_name_buf, sha1, \"keep\");\n>  \n> -\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);\n> +\t\tkeep_fd = odb_pack_keep(keep_name);\n>  \t\tif (keep_fd < 0) {\n>  \t\t\tif (errno != EEXIST)\n>  \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\n> -\t\t\t\t\t  keep_name ? keep_name : name.buf);\n> +\t\t\t\t\t  keep_name);\n>  \t\t} else {\n>  \t\t\tif (keep_msg_len > 0) {\n>  \t\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n> @@ -1416,14 +1418,14 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\t\t}\n>  \t\t\tif (close(keep_fd) != 0)\n>  \t\t\t\tdie_errno(_(\"cannot close written keep file '%s'\"),\n> -\t\t\t\t\t  keep_name ? keep_name : name.buf);\n> +\t\t\t\t\t  keep_name);\n>  \t\t\treport = \"keep\";\n>  \t\t}\n>  \t}\n>  \n>  \tif (final_pack_name != curr_pack_name) {\n>  \t\tif (!final_pack_name)\n> -\t\t\tfinal_pack_name = odb_pack_name(&name, sha1, \"pack\");\n> +\t\t\tfinal_pack_name = odb_pack_name(&pack_name, sha1, \"pack\");\n>  \t\tif (finalize_object_file(curr_pack_name, final_pack_name))\n>  \t\t\tdie(_(\"cannot store pack file\"));\n>  \t} else if (from_stdin)\n> @@ -1431,7 +1433,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \n>  \tif (final_index_name != curr_index_name) {\n>  \t\tif (!final_index_name)\n> -\t\t\tfinal_index_name = odb_pack_name(&name, sha1, \"idx\");\n> +\t\t\tfinal_index_name = odb_pack_name(&index_name, sha1, \"idx\");\n>  \t\tif (finalize_object_file(curr_index_name, final_index_name))\n>  \t\t\tdie(_(\"cannot store index file\"));\n>  \t} else\n> @@ -1458,7 +1460,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\t}\n>  \t}\n>  \n> -\tstrbuf_release(&name);\n> +\tstrbuf_release(&index_name);\n> +\tstrbuf_release(&pack_name);\n> +\tstrbuf_release(&keep_name_buf);\n>  }\n>  \n>  static int git_index_pack_config(const char *k, const char *v, void *cb)\n> \n\nYep, much better.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"314232","messageId":"fefb69e1-325d-5ec6-0954-1445ce8ad9be@ramsayjones.plus.com","threadId":"45390","inReplyTo":"20170316143152.gdeaprlsercvwmiw@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-16T17:03:03Z","receivedAt":"2017-03-16T17:05:53Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 16/03/17 14:31, Jeff King wrote:\n> On Thu, Mar 16, 2017 at 10:27:06AM -0400, Jeff King wrote:\n> \n>> -/*\n>> - * Return the name of the pack or index file with the specified sha1\n>> - * in its filename.  *base and *name are scratch space that must be\n>> - * provided by the caller.  which should be \"pack\" or \"idx\".\n>> - */\n>> -static char *sha1_get_pack_name(const unsigned char *sha1,\n>> -\t\t\t\tstruct strbuf *buf,\n>> -\t\t\t\tconst char *which)\n>> + char *odb_pack_name(struct strbuf *buf,\n>> +\t\t     const unsigned char *sha1,\n>> +\t\t     const char *ext)\n>>  {\n>>  \tstrbuf_reset(buf);\n>>  \tstrbuf_addf(buf, \"%s/pack/pack-%s.%s\", get_object_directory(),\n>> -\t\t    sha1_to_hex(sha1), which);\n>> +\t\t    sha1_to_hex(sha1), ext);\n>>  \treturn buf->buf;\n>>  }\n> \n> Incidentally, this entire function could be implemented as:\n> \n>   return git_path_buf(buf, \"objects/pack/pack-%s.%s\",\n>                       sha1_to_hex(sha1), ext);\n> \n> as the git_path() functions are smart enough to replace \"objects/\" with\n> the true object directory when necessary. I don't know if people find\n> that more or less readable. Since it's buried in a helper function, I\n> doubt it matters much either way. The git_path functions do also do some\n> path normalization, which might be of value.\n\nHmm, I don't have strong feelings either way.\n\nHowever, I note that the only normalization going on (that I can see)\nis to remove .//* from the beginning of the resulting string. I don't\nknow why, but I guess it is to cater to people using the various\nGIT_ environment variables doing things like:\n\n   $ GIT_OBJECT_DIRECTORY=./my-objects git ....\n\nIt has always puzzled me slightly, why the git_path functions do this\nnormalization, but (for example) setup_git_env(), git_path_from_env(),\nget_common_dir(), ... don't! ;-)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"314234","messageId":"20170316174007.gwficwtsqocld4h4@sigill.intra.peff.net","threadId":"45390","inReplyTo":"fefb69e1-325d-5ec6-0954-1445ce8ad9be@ramsayjones.plus.com","subject":"Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T17:40:07Z","receivedAt":"2017-03-16T17:40:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 16, 2017 at 05:03:03PM +0000, Ramsay Jones wrote:\n\n> > Incidentally, this entire function could be implemented as:\n> > \n> >   return git_path_buf(buf, \"objects/pack/pack-%s.%s\",\n> >                       sha1_to_hex(sha1), ext);\n> > \n> > as the git_path() functions are smart enough to replace \"objects/\" with\n> > the true object directory when necessary. I don't know if people find\n> > that more or less readable. Since it's buried in a helper function, I\n> > doubt it matters much either way. The git_path functions do also do some\n> > path normalization, which might be of value.\n> \n> Hmm, I don't have strong feelings either way.\n> \n> However, I note that the only normalization going on (that I can see)\n> is to remove .//* from the beginning of the resulting string. I don't\n> know why, but I guess it is to cater to people using the various\n> GIT_ environment variables doing things like:\n> \n>    $ GIT_OBJECT_DIRECTORY=./my-objects git ....\n> \n> It has always puzzled me slightly, why the git_path functions do this\n> normalization, but (for example) setup_git_env(), git_path_from_env(),\n> get_common_dir(), ... don't! ;-)\n\nI think there's some double-slash normalization (though not consistent\nor thorough). So yeah, I'm not really sure the normalization is of great\nvalue. I am surprised that there isn't a call to \"convert_slashes()\" in\nthere on Windows, as it seems to be sprinkled other places (like\nprefix_filename()).\n\n<shrug> If nobody's complaining, I'm happy to leave it as-is for now,\nand if somebody comes up with a case where git_path normalization makes\na difference, we can look at switching it.\n\n-Peff\n"},{"id":"314239","messageId":"xmqqvar9ax6n.fsf@gitster.mtv.corp.google.com","threadId":"45390","inReplyTo":"20170316142715.zdoikgyy3difeafk@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T18:33:36Z","receivedAt":"2017-03-16T18:33:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +\tstruct strbuf name = STRBUF_INIT;\n>  \tint err;\n>  \n>  \tif (!from_stdin) {\n> @@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n>  \n>  \t\tif (!keep_name)\n> -\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n> -\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n> +\t\t\todb_pack_name(&name, sha1, \"keep\");\n>  \n> -\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name);\n> +\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);\n>  \t\tif (keep_fd < 0) {\n>  \t\t\tif (errno != EEXIST)\n>  \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\n> -\t\t\t\t\t  keep_name ? keep_name : name);\n> +\t\t\t\t\t  keep_name ? keep_name : name.buf);\n>  \t\t} else {\n>  \t\t\tif (keep_msg_len > 0) {\n>  \t\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n> @@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>  \t\t\t}\n>  \t\t\tif (close(keep_fd) != 0)\n>  \t\t\t\tdie_errno(_(\"cannot close written keep file '%s'\"),\n> -\t\t\t\t\t  keep_name ? keep_name : name);\n> +\t\t\t\t\t  keep_name ? keep_name : name.buf);\n>  \t\t\treport = \"keep\";\n>  \t\t}\n>  \t}\n\n\nThe patch as-posted is obviously with less damage to the current\ncode, but the above makes me wonder if it makes simpler to do\n\n\tif (keep_name)\n\t\tstrbuf_addstr(&name, keep_name);\n\telse\n\t\todb_pack_name(&name, sha1, \"keep\");\n\nso that we can always use name.buf without having to do \"?:\" thing.\n"},{"id":"314240","messageId":"20170316183444.t4mjdd7saobn2ut2@sigill.intra.peff.net","threadId":"45390","inReplyTo":"xmqqvar9ax6n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T18:34:45Z","receivedAt":"2017-03-16T18:35:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 16, 2017 at 11:33:36AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +\tstruct strbuf name = STRBUF_INIT;\n> >  \tint err;\n> >  \n> >  \tif (!from_stdin) {\n> > @@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> >  \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n> >  \n> >  \t\tif (!keep_name)\n> > -\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n> > -\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n> > +\t\t\todb_pack_name(&name, sha1, \"keep\");\n> >  \n> > -\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name);\n> > +\t\tkeep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);\n> >  \t\tif (keep_fd < 0) {\n> >  \t\t\tif (errno != EEXIST)\n> >  \t\t\t\tdie_errno(_(\"cannot write keep file '%s'\"),\n> > -\t\t\t\t\t  keep_name ? keep_name : name);\n> > +\t\t\t\t\t  keep_name ? keep_name : name.buf);\n> >  \t\t} else {\n> >  \t\t\tif (keep_msg_len > 0) {\n> >  \t\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n> > @@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> >  \t\t\t}\n> >  \t\t\tif (close(keep_fd) != 0)\n> >  \t\t\t\tdie_errno(_(\"cannot close written keep file '%s'\"),\n> > -\t\t\t\t\t  keep_name ? keep_name : name);\n> > +\t\t\t\t\t  keep_name ? keep_name : name.buf);\n> >  \t\t\treport = \"keep\";\n> >  \t\t}\n> >  \t}\n> \n> \n> The patch as-posted is obviously with less damage to the current\n> code, but the above makes me wonder if it makes simpler to do\n> \n> \tif (keep_name)\n> \t\tstrbuf_addstr(&name, keep_name);\n> \telse\n> \t\todb_pack_name(&name, sha1, \"keep\");\n> \n> so that we can always use name.buf without having to do \"?:\" thing.\n\nSee the next patch. :)\n\n-Peff\n"},{"id":"314243","messageId":"xmqqk27paw3f.fsf@gitster.mtv.corp.google.com","threadId":"45390","inReplyTo":"20170316183444.t4mjdd7saobn2ut2@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T18:57:08Z","receivedAt":"2017-03-16T18:57:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> so that we can always use name.buf without having to do \"?:\" thing.\n>\n> See the next patch. :)\n\nNice ;-)  Thanks, queued the whole thing.\n"}]}