{"thread":{"id":"47617","subject":"[PATCH] sha1_file: remove static strbuf from sha1_file_name()","startedAt":"2018-01-16T07:18:40Z","lastAt":"2018-01-17T17:55:29Z","messageCount":5,"participants":["Christian Couder","Derrick Stolee","Kevin Daudt","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336646","messageId":"20180116071814.19884-1-chriscool@tuxfamily.org","threadId":"47617","inReplyTo":null,"subject":"[PATCH] sha1_file: remove static strbuf from sha1_file_name()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-16T07:18:14Z","receivedAt":"2018-01-16T07:18:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Using a static buffer in sha1_file_name() is error prone\nand the performance improvements it gives are not needed\nin most of the callers.\n\nSo let's get rid of this static buffer and, if necessary\nor helpful, let's use one in the caller.\n---\n cache.h       |  8 +++-----\n http-walker.c |  6 ++++--\n http.c        | 16 ++++++++++------\n sha1_file.c   | 38 +++++++++++++++++++++++++-------------\n 4 files changed, 42 insertions(+), 26 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d8b975a571..6db565408e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -957,12 +957,10 @@ extern void check_repository_format(void);\n #define TYPE_CHANGED    0x0040\n \n /*\n- * Return the name of the file in the local object database that would\n- * be used to store a loose object with the specified sha1.  The\n- * return value is a pointer to a statically allocated buffer that is\n- * overwritten each time the function is called.\n+ * Put in `buf` the name of the file in the local object database that\n+ * would be used to store a loose object with the specified sha1.\n  */\n-extern const char *sha1_file_name(const unsigned char *sha1);\n+extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);\n \n /*\n  * Return an abbreviated sha1 unique within this repository's object database.\ndiff --git a/http-walker.c b/http-walker.c\nindex 1ae8363de2..07c2b1af82 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)\n \t} else if (hashcmp(obj_req->sha1, req->real_sha1)) {\n \t\tret = error(\"File %s has bad hash\", hex);\n \t} else if (req->rename < 0) {\n-\t\tret = error(\"unable to write sha1 filename %s\",\n-\t\t\t    sha1_file_name(req->sha1));\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tsha1_file_name(&buf, req->sha1);\n+\t\tret = error(\"unable to write sha1 filename %s\", buf.buf);\n+\t\tstrbuf_release(&buf);\n \t}\n \n \trelease_http_object_request(req);\ndiff --git a/http.c b/http.c\nindex 5977712712..5979305bc9 100644\n--- a/http.c\n+++ b/http.c\n@@ -2168,7 +2168,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tunsigned char *sha1)\n {\n \tchar *hex = sha1_to_hex(sha1);\n-\tconst char *filename;\n+\tstruct strbuf filename = STRBUF_INIT;\n \tchar prevfile[PATH_MAX];\n \tint prevlocal;\n \tchar prev_buf[PREV_BUF_SIZE];\n@@ -2180,14 +2180,15 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \thashcpy(freq->sha1, sha1);\n \tfreq->localfile = -1;\n \n-\tfilename = sha1_file_name(sha1);\n+\tsha1_file_name(&filename, sha1);\n \tsnprintf(freq->tmpfile, sizeof(freq->tmpfile),\n-\t\t \"%s.temp\", filename);\n+\t\t \"%s.temp\", filename.buf);\n \n-\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename);\n+\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename.buf);\n \tunlink_or_warn(prevfile);\n \trename(freq->tmpfile, prevfile);\n \tunlink_or_warn(freq->tmpfile);\n+\tstrbuf_release(&filename);\n \n \tif (freq->localfile != -1)\n \t\terror(\"fd leakage in start: %d\", freq->localfile);\n@@ -2302,6 +2303,7 @@ void process_http_object_request(struct http_object_request *freq)\n int finish_http_object_request(struct http_object_request *freq)\n {\n \tstruct stat st;\n+\tstruct strbuf filename = STRBUF_INIT;\n \n \tclose(freq->localfile);\n \tfreq->localfile = -1;\n@@ -2327,8 +2329,10 @@ int finish_http_object_request(struct http_object_request *freq)\n \t\tunlink_or_warn(freq->tmpfile);\n \t\treturn -1;\n \t}\n-\tfreq->rename =\n-\t\tfinalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));\n+\n+\tsha1_file_name(&filename, freq->sha1);\n+\tfreq->rename = finalize_object_file(freq->tmpfile, filename.buf);\n+\tstrbuf_release(&filename);\n \n \treturn freq->rename;\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3da70ac650..f66c21b2da 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n \t}\n }\n \n-const char *sha1_file_name(const unsigned char *sha1)\n+void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n {\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"%s/\", get_object_directory());\n+\tstrbuf_addf(buf, \"%s/\", get_object_directory());\n \n-\tfill_sha1_path(&buf, sha1);\n-\treturn buf.buf;\n+\tfill_sha1_path(buf, sha1);\n }\n \n struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n@@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)\n \n static int check_and_freshen_local(const unsigned char *sha1, int freshen)\n {\n-\treturn check_and_freshen_file(sha1_file_name(sha1), freshen);\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_reset(&buf);\n+\tsha1_file_name(&buf, sha1);\n+\n+\treturn check_and_freshen_file(buf.buf, freshen);\n }\n \n static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n@@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,\n \t\t\t  const char **path)\n {\n \tstruct alternate_object_database *alt;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_reset(&buf);\n+\tsha1_file_name(&buf, sha1);\n+\t*path = buf.buf;\n \n-\t*path = sha1_file_name(sha1);\n \tif (!lstat(*path, st))\n \t\treturn 0;\n \n@@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)\n \tint fd;\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_reset(&buf);\n+\tsha1_file_name(&buf, sha1);\n+\t*path = buf.buf;\n \n-\t*path = sha1_file_name(sha1);\n \tfd = git_open(*path);\n \tif (fd >= 0)\n \t\treturn fd;\n@@ -1557,9 +1566,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tgit_SHA_CTX c;\n \tunsigned char parano_sha1[20];\n \tstatic struct strbuf tmp_file = STRBUF_INIT;\n-\tconst char *filename = sha1_file_name(sha1);\n+\tstatic struct strbuf filename = STRBUF_INIT;\n+\n+\tstrbuf_reset(&filename);\n+\tsha1_file_name(&filename, sha1);\n \n-\tfd = create_tmpfile(&tmp_file, filename);\n+\tfd = create_tmpfile(&tmp_file, filename.buf);\n \tif (fd < 0) {\n \t\tif (errno == EACCES)\n \t\t\treturn error(\"insufficient permission for adding an object to repository database %s\", get_object_directory());\n@@ -1612,7 +1624,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t\t\twarning_errno(\"failed utime() on %s\", tmp_file.buf);\n \t}\n \n-\treturn finalize_object_file(tmp_file.buf, filename);\n+\treturn finalize_object_file(tmp_file.buf, filename.buf);\n }\n \n static int freshen_loose_object(const unsigned char *sha1)\n-- \n2.16.0.rc2.2.g7757b9aa6e.dirty\n\n"},{"id":"336658","messageId":"65c81aae-5652-281f-565a-f6116d94abc1@gmail.com","threadId":"47617","inReplyTo":"20180116071814.19884-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] sha1_file: remove static strbuf from sha1_file_name()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-01-16T14:01:30Z","receivedAt":"2018-01-16T14:01:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/16/2018 2:18 AM, Christian Couder wrote:\n> Using a static buffer in sha1_file_name() is error prone\n> and the performance improvements it gives are not needed\n> in most of the callers.\n>\n> So let's get rid of this static buffer and, if necessary\n> or helpful, let's use one in the caller.\n\nFirst: this is a good change for preventing bugs in the future. Do not \nlet my next thought deter you from making this change.\n\nSecond: I wonder if there is any perf hit now that we are allocating \nbuffers much more often. Also, how often does get_object_directory() \nchange, so in some cases we could cache the buffer and only append the \nparts for the loose object (and not reallocate because the filenames \nwill have equal length).\n\nI'm concerned about the perf implications when inspecting many loose \nobjects (100k+) but these code paths seem to be involved with more \nsubstantial work, such as opening and parsing the objects, so keeping a \nbuffer in-memory is probably unnecessary.\n\n> ---\n>   cache.h       |  8 +++-----\n>   http-walker.c |  6 ++++--\n>   http.c        | 16 ++++++++++------\n>   sha1_file.c   | 38 +++++++++++++++++++++++++-------------\n>   4 files changed, 42 insertions(+), 26 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index d8b975a571..6db565408e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -957,12 +957,10 @@ extern void check_repository_format(void);\n>   #define TYPE_CHANGED    0x0040\n>   \n>   /*\n> - * Return the name of the file in the local object database that would\n> - * be used to store a loose object with the specified sha1.  The\n> - * return value is a pointer to a statically allocated buffer that is\n> - * overwritten each time the function is called.\n> + * Put in `buf` the name of the file in the local object database that\n> + * would be used to store a loose object with the specified sha1.\n>    */\n> -extern const char *sha1_file_name(const unsigned char *sha1);\n> +extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);\n>   \n>   /*\n>    * Return an abbreviated sha1 unique within this repository's object database.\n> diff --git a/http-walker.c b/http-walker.c\n> index 1ae8363de2..07c2b1af82 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)\n>   \t} else if (hashcmp(obj_req->sha1, req->real_sha1)) {\n>   \t\tret = error(\"File %s has bad hash\", hex);\n>   \t} else if (req->rename < 0) {\n> -\t\tret = error(\"unable to write sha1 filename %s\",\n> -\t\t\t    sha1_file_name(req->sha1));\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\tsha1_file_name(&buf, req->sha1);\n> +\t\tret = error(\"unable to write sha1 filename %s\", buf.buf);\n> +\t\tstrbuf_release(&buf);\n>   \t}\n>   \n>   \trelease_http_object_request(req);\n> diff --git a/http.c b/http.c\n> index 5977712712..5979305bc9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2168,7 +2168,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>   \tunsigned char *sha1)\n>   {\n>   \tchar *hex = sha1_to_hex(sha1);\n> -\tconst char *filename;\n> +\tstruct strbuf filename = STRBUF_INIT;\n>   \tchar prevfile[PATH_MAX];\n>   \tint prevlocal;\n>   \tchar prev_buf[PREV_BUF_SIZE];\n> @@ -2180,14 +2180,15 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>   \thashcpy(freq->sha1, sha1);\n>   \tfreq->localfile = -1;\n>   \n> -\tfilename = sha1_file_name(sha1);\n> +\tsha1_file_name(&filename, sha1);\n>   \tsnprintf(freq->tmpfile, sizeof(freq->tmpfile),\n> -\t\t \"%s.temp\", filename);\n> +\t\t \"%s.temp\", filename.buf);\n>   \n> -\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename);\n> +\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename.buf);\n>   \tunlink_or_warn(prevfile);\n>   \trename(freq->tmpfile, prevfile);\n>   \tunlink_or_warn(freq->tmpfile);\n> +\tstrbuf_release(&filename);\n>   \n>   \tif (freq->localfile != -1)\n>   \t\terror(\"fd leakage in start: %d\", freq->localfile);\n> @@ -2302,6 +2303,7 @@ void process_http_object_request(struct http_object_request *freq)\n>   int finish_http_object_request(struct http_object_request *freq)\n>   {\n>   \tstruct stat st;\n> +\tstruct strbuf filename = STRBUF_INIT;\n>   \n>   \tclose(freq->localfile);\n>   \tfreq->localfile = -1;\n> @@ -2327,8 +2329,10 @@ int finish_http_object_request(struct http_object_request *freq)\n>   \t\tunlink_or_warn(freq->tmpfile);\n>   \t\treturn -1;\n>   \t}\n> -\tfreq->rename =\n> -\t\tfinalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));\n> +\n> +\tsha1_file_name(&filename, freq->sha1);\n> +\tfreq->rename = finalize_object_file(freq->tmpfile, filename.buf);\n> +\tstrbuf_release(&filename);\n>   \n>   \treturn freq->rename;\n>   }\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 3da70ac650..f66c21b2da 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n>   \t}\n>   }\n>   \n> -const char *sha1_file_name(const unsigned char *sha1)\n> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n>   {\n> -\tstatic struct strbuf buf = STRBUF_INIT;\n> -\n> -\tstrbuf_reset(&buf);\n> -\tstrbuf_addf(&buf, \"%s/\", get_object_directory());\n> +\tstrbuf_addf(buf, \"%s/\", get_object_directory());\n>   \n> -\tfill_sha1_path(&buf, sha1);\n> -\treturn buf.buf;\n> +\tfill_sha1_path(buf, sha1);\n>   }\n\nCould you change this to use strbuf_addstr(buf, get_object_directory()) \nfollowed by a strbuf_addch(buf, '/')? This format string is unnecessary \nand could become slow if this method is called in a tight loop.\n\n>   \n>   struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)\n>   \n>   static int check_and_freshen_local(const unsigned char *sha1, int freshen)\n>   {\n> -\treturn check_and_freshen_file(sha1_file_name(sha1), freshen);\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tsha1_file_name(&buf, sha1);\n> +\n> +\treturn check_and_freshen_file(buf.buf, freshen);\n>   }\n>   \n>   static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n> @@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,\n>   \t\t\t  const char **path)\n>   {\n>   \tstruct alternate_object_database *alt;\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tsha1_file_name(&buf, sha1);\n> +\t*path = buf.buf;\n>   \n> -\t*path = sha1_file_name(sha1);\n>   \tif (!lstat(*path, st))\n>   \t\treturn 0;\n>   \n> @@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)\n>   \tint fd;\n>   \tstruct alternate_object_database *alt;\n>   \tint most_interesting_errno;\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tsha1_file_name(&buf, sha1);\n> +\t*path = buf.buf;\n>   \n> -\t*path = sha1_file_name(sha1);\n>   \tfd = git_open(*path);\n>   \tif (fd >= 0)\n>   \t\treturn fd;\n\nThe check_and_freshen_nonlocal() and open_sha1_file() method comments \nwarn about the 'path' pointer and how it could be overwritten with \nanother call to sha1_file_name(). Your patch fixes that issue, so the \ncomments can be removed.\n\nSince these methods are currently being extra careful about the 'path' \nparameter, perhaps these are candidates to keep a static buffer around?\n\n> @@ -1557,9 +1566,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>   \tgit_SHA_CTX c;\n>   \tunsigned char parano_sha1[20];\n>   \tstatic struct strbuf tmp_file = STRBUF_INIT;\n> -\tconst char *filename = sha1_file_name(sha1);\n> +\tstatic struct strbuf filename = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&filename);\n> +\tsha1_file_name(&filename, sha1);\n>   \n> -\tfd = create_tmpfile(&tmp_file, filename);\n> +\tfd = create_tmpfile(&tmp_file, filename.buf);\n>   \tif (fd < 0) {\n>   \t\tif (errno == EACCES)\n>   \t\t\treturn error(\"insufficient permission for adding an object to repository database %s\", get_object_directory());\n> @@ -1612,7 +1624,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>   \t\t\twarning_errno(\"failed utime() on %s\", tmp_file.buf);\n>   \t}\n>   \n> -\treturn finalize_object_file(tmp_file.buf, filename);\n> +\treturn finalize_object_file(tmp_file.buf, filename.buf);\n>   }\n>   \n>   static int freshen_loose_object(const unsigned char *sha1)\n\nThanks for this cleanup!\n-Stolee\n"},{"id":"336661","messageId":"20180116172556.GA5618@alpha.vpn.ikke.info","threadId":"47617","inReplyTo":"20180116071814.19884-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] sha1_file: remove static strbuf from sha1_file_name()","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2018-01-16T17:25:56Z","receivedAt":"2018-01-16T17:26:03Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Jan 16, 2018 at 08:18:14AM +0100, Christian Couder wrote:\n> Using a static buffer in sha1_file_name() is error prone\n> and the performance improvements it gives are not needed\n> in most of the callers.\n> \n> So let's get rid of this static buffer and, if necessary\n> or helpful, let's use one in the caller.\n\nMissing sign-off\n\n> ---\n>  cache.h       |  8 +++-----\n>  http-walker.c |  6 ++++--\n>  http.c        | 16 ++++++++++------\n>  sha1_file.c   | 38 +++++++++++++++++++++++++-------------\n>  4 files changed, 42 insertions(+), 26 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index d8b975a571..6db565408e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -957,12 +957,10 @@ extern void check_repository_format(void);\n>  #define TYPE_CHANGED    0x0040\n>  \n>  /*\n> - * Return the name of the file in the local object database that would\n> - * be used to store a loose object with the specified sha1.  The\n> - * return value is a pointer to a statically allocated buffer that is\n> - * overwritten each time the function is called.\n> + * Put in `buf` the name of the file in the local object database that\n> + * would be used to store a loose object with the specified sha1.\n>   */\n> -extern const char *sha1_file_name(const unsigned char *sha1);\n> +extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);\n>  \n>  /*\n>   * Return an abbreviated sha1 unique within this repository's object database.\n> diff --git a/http-walker.c b/http-walker.c\n> index 1ae8363de2..07c2b1af82 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)\n>  \t} else if (hashcmp(obj_req->sha1, req->real_sha1)) {\n>  \t\tret = error(\"File %s has bad hash\", hex);\n>  \t} else if (req->rename < 0) {\n> -\t\tret = error(\"unable to write sha1 filename %s\",\n> -\t\t\t    sha1_file_name(req->sha1));\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\tsha1_file_name(&buf, req->sha1);\n> +\t\tret = error(\"unable to write sha1 filename %s\", buf.buf);\n> +\t\tstrbuf_release(&buf);\n>  \t}\n>  \n>  \trelease_http_object_request(req);\n> diff --git a/http.c b/http.c\n> index 5977712712..5979305bc9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2168,7 +2168,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>  \tunsigned char *sha1)\n>  {\n>  \tchar *hex = sha1_to_hex(sha1);\n> -\tconst char *filename;\n> +\tstruct strbuf filename = STRBUF_INIT;\n>  \tchar prevfile[PATH_MAX];\n>  \tint prevlocal;\n>  \tchar prev_buf[PREV_BUF_SIZE];\n> @@ -2180,14 +2180,15 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>  \thashcpy(freq->sha1, sha1);\n>  \tfreq->localfile = -1;\n>  \n> -\tfilename = sha1_file_name(sha1);\n> +\tsha1_file_name(&filename, sha1);\n>  \tsnprintf(freq->tmpfile, sizeof(freq->tmpfile),\n> -\t\t \"%s.temp\", filename);\n> +\t\t \"%s.temp\", filename.buf);\n>  \n> -\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename);\n> +\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename.buf);\n>  \tunlink_or_warn(prevfile);\n>  \trename(freq->tmpfile, prevfile);\n>  \tunlink_or_warn(freq->tmpfile);\n> +\tstrbuf_release(&filename);\n>  \n>  \tif (freq->localfile != -1)\n>  \t\terror(\"fd leakage in start: %d\", freq->localfile);\n> @@ -2302,6 +2303,7 @@ void process_http_object_request(struct http_object_request *freq)\n>  int finish_http_object_request(struct http_object_request *freq)\n>  {\n>  \tstruct stat st;\n> +\tstruct strbuf filename = STRBUF_INIT;\n>  \n>  \tclose(freq->localfile);\n>  \tfreq->localfile = -1;\n> @@ -2327,8 +2329,10 @@ int finish_http_object_request(struct http_object_request *freq)\n>  \t\tunlink_or_warn(freq->tmpfile);\n>  \t\treturn -1;\n>  \t}\n> -\tfreq->rename =\n> -\t\tfinalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));\n> +\n> +\tsha1_file_name(&filename, freq->sha1);\n> +\tfreq->rename = finalize_object_file(freq->tmpfile, filename.buf);\n> +\tstrbuf_release(&filename);\n>  \n>  \treturn freq->rename;\n>  }\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 3da70ac650..f66c21b2da 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n>  \t}\n>  }\n>  \n> -const char *sha1_file_name(const unsigned char *sha1)\n> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n>  {\n> -\tstatic struct strbuf buf = STRBUF_INIT;\n> -\n> -\tstrbuf_reset(&buf);\n> -\tstrbuf_addf(&buf, \"%s/\", get_object_directory());\n> +\tstrbuf_addf(buf, \"%s/\", get_object_directory());\n>  \n> -\tfill_sha1_path(&buf, sha1);\n> -\treturn buf.buf;\n> +\tfill_sha1_path(buf, sha1);\n>  }\n>  \n>  struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)\n>  \n>  static int check_and_freshen_local(const unsigned char *sha1, int freshen)\n>  {\n> -\treturn check_and_freshen_file(sha1_file_name(sha1), freshen);\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tsha1_file_name(&buf, sha1);\n> +\n> +\treturn check_and_freshen_file(buf.buf, freshen);\n>  }\n>  \n>  static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n> @@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,\n>  \t\t\t  const char **path)\n>  {\n>  \tstruct alternate_object_database *alt;\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tsha1_file_name(&buf, sha1);\n> +\t*path = buf.buf;\n>  \n> -\t*path = sha1_file_name(sha1);\n>  \tif (!lstat(*path, st))\n>  \t\treturn 0;\n>  \n> @@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)\n>  \tint fd;\n>  \tstruct alternate_object_database *alt;\n>  \tint most_interesting_errno;\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&buf);\n> +\tsha1_file_name(&buf, sha1);\n> +\t*path = buf.buf;\n>  \n> -\t*path = sha1_file_name(sha1);\n>  \tfd = git_open(*path);\n>  \tif (fd >= 0)\n>  \t\treturn fd;\n> @@ -1557,9 +1566,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>  \tgit_SHA_CTX c;\n>  \tunsigned char parano_sha1[20];\n>  \tstatic struct strbuf tmp_file = STRBUF_INIT;\n> -\tconst char *filename = sha1_file_name(sha1);\n> +\tstatic struct strbuf filename = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&filename);\n> +\tsha1_file_name(&filename, sha1);\n>  \n> -\tfd = create_tmpfile(&tmp_file, filename);\n> +\tfd = create_tmpfile(&tmp_file, filename.buf);\n>  \tif (fd < 0) {\n>  \t\tif (errno == EACCES)\n>  \t\t\treturn error(\"insufficient permission for adding an object to repository database %s\", get_object_directory());\n> @@ -1612,7 +1624,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>  \t\t\twarning_errno(\"failed utime() on %s\", tmp_file.buf);\n>  \t}\n>  \n> -\treturn finalize_object_file(tmp_file.buf, filename);\n> +\treturn finalize_object_file(tmp_file.buf, filename.buf);\n>  }\n>  \n>  static int freshen_loose_object(const unsigned char *sha1)\n> -- \n> 2.16.0.rc2.2.g7757b9aa6e.dirty\n> \n"},{"id":"336668","messageId":"b956f893-74c4-3b0e-1f0a-4be1b01dc649@jeffhostetler.com","threadId":"47617","inReplyTo":"65c81aae-5652-281f-565a-f6116d94abc1@gmail.com","subject":"Re: [PATCH] sha1_file: remove static strbuf from sha1_file_name()","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-01-16T19:00:05Z","receivedAt":"2018-01-16T19:00:18Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 1/16/2018 9:01 AM, Derrick Stolee wrote:\n> On 1/16/2018 2:18 AM, Christian Couder wrote:\n>> Using a static buffer in sha1_file_name() is error prone\n>> and the performance improvements it gives are not needed\n>> in most of the callers.\n>>\n>> So let's get rid of this static buffer and, if necessary\n>> or helpful, let's use one in the caller.\n> \n> First: this is a good change for preventing bugs in the future. Do not let my next thought deter you from making this change.\n> \n> Second: I wonder if there is any perf hit now that we are allocating buffers much more often. Also, how often does get_object_directory() change, so in some cases we could cache the buffer and only append the parts for the loose object (and not reallocate because the filenames will have equal length).\n> \n> I'm concerned about the perf implications when inspecting many loose objects (100k+) but these code paths seem to be involved with more substantial work, such as opening and parsing the objects, so keeping a buffer in-memory is probably unnecessary.\n> \n>> ---\n>>   cache.h       |  8 +++-----\n>>   http-walker.c |  6 ++++--\n>>   http.c        | 16 ++++++++++------\n>>   sha1_file.c   | 38 +++++++++++++++++++++++++-------------\n>>   4 files changed, 42 insertions(+), 26 deletions(-)\n>>\n>> diff --git a/cache.h b/cache.h\n>> index d8b975a571..6db565408e 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -957,12 +957,10 @@ extern void check_repository_format(void);\n>>   #define TYPE_CHANGED    0x0040\n>>   /*\n>> - * Return the name of the file in the local object database that would\n>> - * be used to store a loose object with the specified sha1.  The\n>> - * return value is a pointer to a statically allocated buffer that is\n>> - * overwritten each time the function is called.\n>> + * Put in `buf` the name of the file in the local object database that\n>> + * would be used to store a loose object with the specified sha1.\n>>    */\n>> -extern const char *sha1_file_name(const unsigned char *sha1);\n>> +extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);\n>>   /*\n>>    * Return an abbreviated sha1 unique within this repository's object database.\n>> diff --git a/http-walker.c b/http-walker.c\n>> index 1ae8363de2..07c2b1af82 100644\n>> --- a/http-walker.c\n>> +++ b/http-walker.c\n>> @@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)\n>>       } else if (hashcmp(obj_req->sha1, req->real_sha1)) {\n>>           ret = error(\"File %s has bad hash\", hex);\n>>       } else if (req->rename < 0) {\n>> -        ret = error(\"unable to write sha1 filename %s\",\n>> -                sha1_file_name(req->sha1));\n>> +        struct strbuf buf = STRBUF_INIT;\n>> +        sha1_file_name(&buf, req->sha1);\n>> +        ret = error(\"unable to write sha1 filename %s\", buf.buf);\n>> +        strbuf_release(&buf);\n>>       }\n>>       release_http_object_request(req);\n>> diff --git a/http.c b/http.c\n>> index 5977712712..5979305bc9 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -2168,7 +2168,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>>       unsigned char *sha1)\n>>   {\n>>       char *hex = sha1_to_hex(sha1);\n>> -    const char *filename;\n>> +    struct strbuf filename = STRBUF_INIT;\n>>       char prevfile[PATH_MAX];\n>>       int prevlocal;\n>>       char prev_buf[PREV_BUF_SIZE];\n>> @@ -2180,14 +2180,15 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>>       hashcpy(freq->sha1, sha1);\n>>       freq->localfile = -1;\n>> -    filename = sha1_file_name(sha1);\n>> +    sha1_file_name(&filename, sha1);\n>>       snprintf(freq->tmpfile, sizeof(freq->tmpfile),\n>> -         \"%s.temp\", filename);\n>> +         \"%s.temp\", filename.buf);\n>> -    snprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename);\n>> +    snprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename.buf);\n>>       unlink_or_warn(prevfile);\n>>       rename(freq->tmpfile, prevfile);\n>>       unlink_or_warn(freq->tmpfile);\n>> +    strbuf_release(&filename);\n>>       if (freq->localfile != -1)\n>>           error(\"fd leakage in start: %d\", freq->localfile);\n>> @@ -2302,6 +2303,7 @@ void process_http_object_request(struct http_object_request *freq)\n>>   int finish_http_object_request(struct http_object_request *freq)\n>>   {\n>>       struct stat st;\n>> +    struct strbuf filename = STRBUF_INIT;\n>>       close(freq->localfile);\n>>       freq->localfile = -1;\n>> @@ -2327,8 +2329,10 @@ int finish_http_object_request(struct http_object_request *freq)\n>>           unlink_or_warn(freq->tmpfile);\n>>           return -1;\n>>       }\n>> -    freq->rename =\n>> -        finalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));\n>> +\n>> +    sha1_file_name(&filename, freq->sha1);\n>> +    freq->rename = finalize_object_file(freq->tmpfile, filename.buf);\n>> +    strbuf_release(&filename);\n>>       return freq->rename;\n>>   }\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 3da70ac650..f66c21b2da 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n>>       }\n>>   }\n>> -const char *sha1_file_name(const unsigned char *sha1)\n>> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n>>   {\n>> -    static struct strbuf buf = STRBUF_INIT;\n>> -\n>> -    strbuf_reset(&buf);\n>> -    strbuf_addf(&buf, \"%s/\", get_object_directory());\n>> +    strbuf_addf(buf, \"%s/\", get_object_directory());\n>> -    fill_sha1_path(&buf, sha1);\n>> -    return buf.buf;\n>> +    fill_sha1_path(buf, sha1);\n>>   }\n> \n> Could you change this to use strbuf_addstr(buf, get_object_directory()) followed by a strbuf_addch(buf, '/')? This format string is unnecessary and could become slow if this method is called in a tight loop.\n\nYes, an _addstr() and _addch() would avoid a sprintf and\nwe've seen perf problems with this before.\n\nCould we also add seed the initial buffer size to avoid\nany need to realloc as the buffer is filled in?\n\nSomething like:\n\tsize_t len = strlen(get_object_directory()) + GIT_MAX_HEXSZ + 3;\n\tstrbuf_reset(&buf);\n\tif (len > strbuf_avail(&buf))\n\t\tstrbuf_grow(&buf, len);\n\tstrbuf_addstr(&buf, ...);\n\n> \n>>   struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n>> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)\n>>   static int check_and_freshen_local(const unsigned char *sha1, int freshen)\n>>   {\n>> -    return check_and_freshen_file(sha1_file_name(sha1), freshen);\n>> +    static struct strbuf buf = STRBUF_INIT;\n>> +\n>> +    strbuf_reset(&buf);\n>> +    sha1_file_name(&buf, sha1);\n>> +\n>> +    return check_and_freshen_file(buf.buf, freshen);\n>>   }\n\nDoes \"buf\" really need to be static here?  Doesn't this just move the problem\nfrom sha1_file_name() to here?\n\n\n>>   static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n>> @@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,\n>>                 const char **path)\n>>   {\n>>       struct alternate_object_database *alt;\n>> +    static struct strbuf buf = STRBUF_INIT;\n>> +\n>> +    strbuf_reset(&buf);\n>> +    sha1_file_name(&buf, sha1);\n>> +    *path = buf.buf;\n>> -    *path = sha1_file_name(sha1);\n>>       if (!lstat(*path, st))\n>>           return 0;\n\nAgain, making \"buf\" static here feels wrong.  Perhaps, change the signature\nof the static function to drop the const on the \"path\" and strbuf_detach buf.buf\nand give it to the caller (if path is not NULL) and otherwise release it here.\n\n\n>> @@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)\n>>       int fd;\n>>       struct alternate_object_database *alt;\n>>       int most_interesting_errno;\n>> +    static struct strbuf buf = STRBUF_INIT;\n>> +\n>> +    strbuf_reset(&buf);\n>> +    sha1_file_name(&buf, sha1);\n>> +    *path = buf.buf;\n>> -    *path = sha1_file_name(sha1);\n>>       fd = git_open(*path);\n>>       if (fd >= 0)\n>>           return fd;\n\nSame comment here about the static strbuf.\n\n  \n> The check_and_freshen_nonlocal() and open_sha1_file() method comments warn about the 'path' pointer and how it could be overwritten with another call to sha1_file_name(). Your patch fixes that issue, so the comments can be removed.\n\nWait, we've eliminated the static buffer inside sha1_file_name(), but\nall of these callers currently define their own static buffers, so the\ncomments still apply, right?\n\nThere is also an effectively static buffer in each of the alternates\nthat is used by alt_sha1_path().\n\n  \n> Since these methods are currently being extra careful about the 'path' parameter, perhaps these are candidates to keep a static buffer around?\n\nIf I have multiple threads (as part of say a threaded checkout), we're\nstill going to stumble over these static loose object pathname buffers,\nright?\n\n\n> \n>> @@ -1557,9 +1566,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>>       git_SHA_CTX c;\n>>       unsigned char parano_sha1[20];\n>>       static struct strbuf tmp_file = STRBUF_INIT;\n>> -    const char *filename = sha1_file_name(sha1);\n>> +    static struct strbuf filename = STRBUF_INIT;\n>> +\n>> +    strbuf_reset(&filename);\n>> +    sha1_file_name(&filename, sha1);\n>> -    fd = create_tmpfile(&tmp_file, filename);\n>> +    fd = create_tmpfile(&tmp_file, filename.buf);\n>>       if (fd < 0) {\n>>           if (errno == EACCES)\n>>               return error(\"insufficient permission for adding an object to repository database %s\", get_object_directory());\n>> @@ -1612,7 +1624,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>>               warning_errno(\"failed utime() on %s\", tmp_file.buf);\n>>       }\n>> -    return finalize_object_file(tmp_file.buf, filename);\n>> +    return finalize_object_file(tmp_file.buf, filename.buf);\n>>   }\n>>   static int freshen_loose_object(const unsigned char *sha1)\n> \n> Thanks for this cleanup!\n> -Stolee\n\nYes, thanks for digging into this!\nJeff\n\n"},{"id":"336703","messageId":"CAP8UFD3bH+VqgPs6KNRc9UHLJiTVmHs5XX3fsh8wS+micGdJLQ@mail.gmail.com","threadId":"47617","inReplyTo":"b956f893-74c4-3b0e-1f0a-4be1b01dc649@jeffhostetler.com","subject":"Re: [PATCH] sha1_file: remove static strbuf from sha1_file_name()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-17T17:55:22Z","receivedAt":"2018-01-17T17:55:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jan 16, 2018 at 8:00 PM, Jeff Hostetler <git@jeffhostetler.com> wrote:\n>\n>\n> On 1/16/2018 9:01 AM, Derrick Stolee wrote:\n>>\n>> On 1/16/2018 2:18 AM, Christian Couder wrote:\n>>>\n>>> Using a static buffer in sha1_file_name() is error prone\n>>> and the performance improvements it gives are not needed\n>>> in most of the callers.\n>>>\n>>> So let's get rid of this static buffer and, if necessary\n>>> or helpful, let's use one in the caller.\n>>\n>>\n>> First: this is a good change for preventing bugs in the future. Do not let\n>> my next thought deter you from making this change.\n>>\n>> Second: I wonder if there is any perf hit now that we are allocating\n>> buffers much more often.\n\nWhen I though that the caller might be performance sensitive, I used a\n\"static struct strbuf\" in the caller to avoid any performance\nregression.\n\nYeah, that means that further work is needed if we want to get rid of\nall the static buffers, but that is not my goal. I am just concerned\nwith cleaning up sha1_file_name() before changing it, and avoiding\nsome possible trouble when using it.\n\nFeel free to improve on that or even take over this series, otherwise\nit can be part of the #leftoverbits.\n\n>> Also, how often does get_object_directory() change,\n>> so in some cases we could cache the buffer and only append the parts for the\n>> loose object (and not reallocate because the filenames will have equal\n>> length).\n\nAgain feel free to work on this kind of optimizations on top of this series.\n\n>> I'm concerned about the perf implications when inspecting many loose\n>> objects (100k+) but these code paths seem to be involved with more\n>> substantial work, such as opening and parsing the objects, so keeping a\n>> buffer in-memory is probably unnecessary.\n\nYeah, I also think it is not necessary to optimize too much.\n\n>>> diff --git a/sha1_file.c b/sha1_file.c\n>>> index 3da70ac650..f66c21b2da 100644\n>>> --- a/sha1_file.c\n>>> +++ b/sha1_file.c\n>>> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf,\n>>> const unsigned char *sha1)\n>>>       }\n>>>   }\n>>> -const char *sha1_file_name(const unsigned char *sha1)\n>>> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n>>>   {\n>>> -    static struct strbuf buf = STRBUF_INIT;\n>>> -\n>>> -    strbuf_reset(&buf);\n>>> -    strbuf_addf(&buf, \"%s/\", get_object_directory());\n>>> +    strbuf_addf(buf, \"%s/\", get_object_directory());\n>>> -    fill_sha1_path(&buf, sha1);\n>>> -    return buf.buf;\n>>> +    fill_sha1_path(buf, sha1);\n>>>   }\n>>\n>>\n>> Could you change this to use strbuf_addstr(buf, get_object_directory())\n>> followed by a strbuf_addch(buf, '/')? This format string is unnecessary and\n>> could become slow if this method is called in a tight loop.\n>\n> Yes, an _addstr() and _addch() would avoid a sprintf and\n> we've seen perf problems with this before.\n>\n> Could we also add seed the initial buffer size to avoid\n> any need to realloc as the buffer is filled in?\n>\n> Something like:\n>         size_t len = strlen(get_object_directory()) + GIT_MAX_HEXSZ + 3;\n>         strbuf_reset(&buf);\n>         if (len > strbuf_avail(&buf))\n>                 strbuf_grow(&buf, len);\n>         strbuf_addstr(&buf, ...);\n\nOk, I did something like that in another patch on top of the first\npatch which is just about using a \"struct strbuf *\" passed as an\nargument instead of a static buffer.\n\n>>>   struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n>>> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int\n>>> freshen)\n>>>   static int check_and_freshen_local(const unsigned char *sha1, int\n>>> freshen)\n>>>   {\n>>> -    return check_and_freshen_file(sha1_file_name(sha1), freshen);\n>>> +    static struct strbuf buf = STRBUF_INIT;\n>>> +\n>>> +    strbuf_reset(&buf);\n>>> +    sha1_file_name(&buf, sha1);\n>>> +\n>>> +    return check_and_freshen_file(buf.buf, freshen);\n>>>   }\n>\n> Does \"buf\" really need to be static here?  Doesn't this just move the\n> problem from sha1_file_name() to here?\n\nYes, but maybe check_and_freshen_local() is performance sensitive, so\nI think it is safer performance wise to still use a static buf.\n\nIf there is a consensus that it is ok to not use one here, I am ok to\nchange that. On the other hand the change could also be part of\nanother patch on top of this one...\n\n>>>   static int check_and_freshen_nonlocal(const unsigned char *sha1, int\n>>> freshen)\n>>> @@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1,\n>>> struct stat *st,\n>>>                 const char **path)\n>>>   {\n>>>       struct alternate_object_database *alt;\n>>> +    static struct strbuf buf = STRBUF_INIT;\n>>> +\n>>> +    strbuf_reset(&buf);\n>>> +    sha1_file_name(&buf, sha1);\n>>> +    *path = buf.buf;\n>>> -    *path = sha1_file_name(sha1);\n>>>       if (!lstat(*path, st))\n>>>           return 0;\n>\n> Again, making \"buf\" static here feels wrong.  Perhaps, change the signature\n> of the static function to drop the const on the \"path\" and strbuf_detach\n> buf.buf\n> and give it to the caller (if path is not NULL) and otherwise release it\n> here.\n\nI don't think it is a good idea to do too many things in this patch,\nso I think it should be part of another patch to change the way\nstat_sha1_file() works.\n\n>>> @@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1,\n>>> const char **path)\n>>>       int fd;\n>>>       struct alternate_object_database *alt;\n>>>       int most_interesting_errno;\n>>> +    static struct strbuf buf = STRBUF_INIT;\n>>> +\n>>> +    strbuf_reset(&buf);\n>>> +    sha1_file_name(&buf, sha1);\n>>> +    *path = buf.buf;\n>>> -    *path = sha1_file_name(sha1);\n>>>       fd = git_open(*path);\n>>>       if (fd >= 0)\n>>>           return fd;\n>\n> Same comment here about the static strbuf.\n\nSame comment for me too.\n\n>> The check_and_freshen_nonlocal() and open_sha1_file() method comments\n\nActually I think it is stat_sha1_file() and open_sha1_file() that have\nthose comments.\n\n>> warn\n>> about the 'path' pointer and how it could be overwritten with another call\n>> to sha1_file_name(). Your patch fixes that issue, so the comments can be\n>> removed.\n>\n> Wait, we've eliminated the static buffer inside sha1_file_name(), but\n> all of these callers currently define their own static buffers, so the\n> comments still apply, right?\n\nYeah right, that's why I didn't change the comments.\n\n> There is also an effectively static buffer in each of the alternates\n> that is used by alt_sha1_path().\n\nYeah and I didn't change that, so anyway, if you want to get rid of\nall the static buffers, other work than just this patch is needed.\n\n>> Since these methods are currently being extra careful about the 'path'\n>> parameter, perhaps these are candidates to keep a static buffer around?\n>\n> If I have multiple threads (as part of say a threaded checkout), we're\n> still going to stumble over these static loose object pathname buffers,\n> right?\n\nI agree but again getting rid of all the static buffers is not the\ngoal of this patch series.\n\n[...]\n\n>> Thanks for this cleanup!\n>\n> Yes, thanks for digging into this!\n\nThanks for your comments,\nChristian.\n"}]}