On Tue, Jan 16, 2018 at 08:18:14AM +0100, Christian Couder wrote:
> Using a static buffer in sha1_file_name() is error prone
> and the performance improvements it gives are not needed
> in most of the callers.
>
> So let's get rid of this static buffer and, if necessary
> or helpful, let's use one in the caller.
Missing sign-off
> ---
> cache.h | 8 +++-----
> http-walker.c | 6 ++++--
> http.c | 16 ++++++++++------
> sha1_file.c | 38 +++++++++++++++++++++++++-------------
> 4 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index d8b975a571..6db565408e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -957,12 +957,10 @@ extern void check_repository_format(void);
> #define TYPE_CHANGED 0x0040
>
> /*
> - * Return the name of the file in the local object database that would
> - * be used to store a loose object with the specified sha1. The
> - * return value is a pointer to a statically allocated buffer that is
> - * overwritten each time the function is called.
> + * Put in `buf` the name of the file in the local object database that
> + * would be used to store a loose object with the specified sha1.
> */
> -extern const char *sha1_file_name(const unsigned char *sha1);
> +extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);
>
> /*
> * Return an abbreviated sha1 unique within this repository's object
> database.
> diff --git a/http-walker.c b/http-walker.c
> index 1ae8363de2..07c2b1af82 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned
> char *sha1)
> } else if (hashcmp(obj_req->sha1, req->real_sha1)) {
> ret = error("File %s has bad hash", hex);
> } else if (req->rename < 0) {
> - ret = error("unable to write sha1 filename %s",
> - sha1_file_name(req->sha1));
> + struct strbuf buf = STRBUF_INIT;
> + sha1_file_name(&buf, req->sha1);
> + ret = error("unable to write sha1 filename %s", buf.buf);
> + strbuf_release(&buf);
> }
>
> release_http_object_request(req);
> diff --git a/http.c b/http.c
> index 5977712712..5979305bc9 100644
> --- a/http.c
> +++ b/http.c
> @@ -2168,7 +2168,7 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
> unsigned char *sha1)
> {
> char *hex = sha1_to_hex(sha1);
> - const char *filename;
> + struct strbuf filename = STRBUF_INIT;
> char prevfile[PATH_MAX];
> int prevlocal;
> char prev_buf[PREV_BUF_SIZE];
> @@ -2180,14 +2180,15 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
> hashcpy(freq->sha1, sha1);
> freq->localfile = -1;
>
> - filename = sha1_file_name(sha1);
> + sha1_file_name(&filename, sha1);
> snprintf(freq->tmpfile, sizeof(freq->tmpfile),
> - "%s.temp", filename);
> + "%s.temp", filename.buf);
>
> - snprintf(prevfile, sizeof(prevfile), "%s.prev", filename);
> + snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf);
> unlink_or_warn(prevfile);
> rename(freq->tmpfile, prevfile);
> unlink_or_warn(freq->tmpfile);
> + strbuf_release(&filename);
>
> if (freq->localfile != -1)
> error("fd leakage in start: %d", freq->localfile);
> @@ -2302,6 +2303,7 @@ void process_http_object_request(struct
> http_object_request *freq)
> int finish_http_object_request(struct http_object_request *freq)
> {
> struct stat st;
> + struct strbuf filename = STRBUF_INIT;
>
> close(freq->localfile);
> freq->localfile = -1;
> @@ -2327,8 +2329,10 @@ int finish_http_object_request(struct
> http_object_request *freq)
> unlink_or_warn(freq->tmpfile);
> return -1;
> }
> - freq->rename =
> - finalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));
> +
> + sha1_file_name(&filename, freq->sha1);
> + freq->rename = finalize_object_file(freq->tmpfile, filename.buf);
> + strbuf_release(&filename);
>
> return freq->rename;
> }
> diff --git a/sha1_file.c b/sha1_file.c
> index 3da70ac650..f66c21b2da 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const
> unsigned char *sha1)
> }
> }
>
> -const char *sha1_file_name(const unsigned char *sha1)
> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)
> {
> - static struct strbuf buf = STRBUF_INIT;
> -
> - strbuf_reset(&buf);
> - strbuf_addf(&buf, "%s/", get_object_directory());
> + strbuf_addf(buf, "%s/", get_object_directory());
>
> - fill_sha1_path(&buf, sha1);
> - return buf.buf;
> + fill_sha1_path(buf, sha1);
> }
>
> struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)
>
> static int check_and_freshen_local(const unsigned char *sha1, int freshen)
> {
> - return check_and_freshen_file(sha1_file_name(sha1), freshen);
> + static struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_reset(&buf);
> + sha1_file_name(&buf, sha1);
> +
> + return check_and_freshen_file(buf.buf, freshen);
> }
>
> static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)
> @@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1,
> struct stat *st,
> const char **path)
> {
> struct alternate_object_database *alt;
> + static struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_reset(&buf);
> + sha1_file_name(&buf, sha1);
> + *path = buf.buf;
>
> - *path = sha1_file_name(sha1);
> if (!lstat(*path, st))
> return 0;
>
> @@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1,
> const char **path)
> int fd;
> struct alternate_object_database *alt;
> int most_interesting_errno;
> + static struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_reset(&buf);
> + sha1_file_name(&buf, sha1);
> + *path = buf.buf;
>
> - *path = sha1_file_name(sha1);
> fd = git_open(*path);
> if (fd >= 0)
> return fd;
> @@ -1557,9 +1566,12 @@ static int write_loose_object(const unsigned char
> *sha1, char *hdr, int hdrlen,
> git_SHA_CTX c;
> unsigned char parano_sha1[20];
> static struct strbuf tmp_file = STRBUF_INIT;
> - const char *filename = sha1_file_name(sha1);
> + static struct strbuf filename = STRBUF_INIT;
> +
> + strbuf_reset(&filename);
> + sha1_file_name(&filename, sha1);
>
> - fd = create_tmpfile(&tmp_file, filename);
> + fd = create_tmpfile(&tmp_file, filename.buf);
> if (fd < 0) {
> if (errno == EACCES)
> return error("insufficient permission for adding an
> object to repository database %s", get_object_directory());
> @@ -1612,7 +1624,7 @@ static int write_loose_object(const unsigned char
> *sha1, char *hdr, int hdrlen,
> warning_errno("failed utime() on %s", tmp_file.buf);
> }
>
> - return finalize_object_file(tmp_file.buf, filename);
> + return finalize_object_file(tmp_file.buf, filename.buf);
> }
>
> static int freshen_loose_object(const unsigned char *sha1)
> --
> 2.16.0.rc2.2.g7757b9aa6e.dirty
>
