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

[Jeff King <peff@peff.net>]

On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:

> On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> > diff --git a/environment.c b/environment.c
> > index 4dac5e9..4de7b81 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -135,15 +135,11 @@ static void setup_git_env(void)
> >         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 = xmalloc(strlen(git_dir) + 9);
> > -               sprintf(git_object_dir, "%s/objects", git_dir);
> > -       }
> 
> If DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because
> getenv's return value is not guaranteed persistent. Since you're touch
> this area, perhaps do it too (in this, or another patch)?

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 <peff@peff.net>
---
 environment.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/environment.c b/environment.c
index 4dac5e9..efb2fa0 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(path);
+}
+
 static void setup_git_env(void)
 {
 	const char *gitfile;
@@ -134,19 +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 = xmalloc(strlen(git_dir) + 9);
-		sprintf(git_object_dir, "%s/objects", git_dir);
-	}
-	git_index_file = getenv(INDEX_ENVIRONMENT);
-	if (!git_index_file) {
-		git_index_file = xmalloc(strlen(git_dir) + 7);
-		sprintf(git_index_file, "%s/index", git_dir);
-	}
-	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.566.gfe3e6b2