Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

Jeff King [off-list ref] writes:

Here's a replacement patch that handles this (and just drops the ugly
mallocs as a side effect).

-- >8 --
Subject: [PATCH] setup_git_env: copy getenv return value

The return value of getenv is not guaranteed to survive
across further invocations of setenv or even getenv. When we
are assigning it to globals that last the lifetime of the
program, we should make our own copy.

Signed-off-by: Jeff King <redacted>
---

Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
into next, 2014-06-23) with about 20 hours of lag.

I'd make it relative like the attached on top of the series.  Note
that I tweaked the args to git_pathdup() to avoid the "are you sure
you want to give a variable format string to git_pathdup() which you
said is like printf(3)?" warning from the compiler.

Thanks.

-- >8 --
From: Jeff King <redacted>
Date: Tue, 24 Jun 2014 16:58:15 -0400
Subject: [PATCH] setup_git_env(): introduce git_path_from_env() helper

"Check the value of an environment and fall back to a known path
inside $GIT_DIR" is repeated a few times to determine the location
of the data store, the index and the graft file, but the return
value of getenv is not guaranteed to survive across further
invocations of setenv or even getenv.

Make sure to xstrdup() the value we receive from getenv(3), and
encapsulate the pattern into a helper function.

Signed-off-by: Jeff King <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 environment.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/environment.c b/environment.c
index 4de7b81..565f652 100644
--- a/environment.c
+++ b/environment.c

@@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)
 	return strbuf_detach(&buf, NULL);
 }
 
+static char *git_path_from_env(const char *envvar, const char *path)
+{
+	const char *value = getenv(envvar);
+	return value ? xstrdup(value) : git_pathdup("%s", path);
+}
+
 static void setup_git_env(void)
 {
 	const char *gitfile;

@@ -134,15 +140,9 @@ static void setup_git_env(void)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	gitfile = read_gitfile(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
-	git_object_dir = getenv(DB_ENVIRONMENT);
-	if (!git_object_dir)
-		git_object_dir = git_pathdup("objects");
-	git_index_file = getenv(INDEX_ENVIRONMENT);
-	if (!git_index_file)
-		git_index_file = git_pathdup("index");
-	git_graft_file = getenv(GRAFT_ENVIRONMENT);
-	if (!git_graft_file)
-		git_graft_file = git_pathdup("info/grafts");
+	git_object_dir = git_path_from_env(DB_ENVIRONMENT, "objects");
+	git_index_file = git_path_from_env(INDEX_ENVIRONMENT, "index");
+	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts");
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		check_replace_refs = 0;
 	namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));

-- 
2.0.0-641-g934bf98

On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

Here's a replacement patch that handles this (and just drops the ugly
mallocs as a side effect).

-- >8 --
Subject: [PATCH] setup_git_env: copy getenv return value

The return value of getenv is not guaranteed to survive
across further invocations of setenv or even getenv. When we
are assigning it to globals that last the lifetime of the
program, we should make our own copy.

Signed-off-by: Jeff King <redacted>
---

Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
into next, 2014-06-23) with about 20 hours of lag.

Ah, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged
yet, but I didn't check when responding to Duy.

I'd make it relative like the attached on top of the series.  Note
that I tweaked the args to git_pathdup() to avoid the "are you sure
you want to give a variable format string to git_pathdup() which you
said is like printf(3)?" warning from the compiler.

Both changes look good to me. Thanks for taking care of it.

-Peff