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

2014-06-25 Thread Duy Nguyen 
On Wed, Jun 25, 2014 at 3:58 AM, Jeff King p...@peff.net wrote:
 Here's a replacement patch that handles this (and just drops the ugly
 mallocs as a side effect).

Shortly after I wrote my email, I thought about getenvdup() and look
for similar getenv/xstrdup patterns. But I saw only one in config.c.
So let's forget about it. Your patch looks good.


 -- 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 p...@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




-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-25 Thread Junio C Hamano 
Jeff King p...@peff.net 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 p...@peff.net
 ---

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 p...@peff.net
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 p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 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


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

 Jeff King p...@peff.net 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 p...@peff.net
  ---
 
 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-25 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

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

 Jeff King p...@peff.net 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 p...@peff.net
  ---
 
 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.

Sorry to have sighed --- crossing e-mails happen all the time.  No
need to feel sorry.

 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.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-24 Thread Duy Nguyen 
On Mon, Jun 23, 2014 at 5:21 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
 On Thu, Jun 19, 2014 at 5:28 PM, Jeff King p...@peff.net wrote:
 This is shorter, harder to get wrong, and more clearly
 captures the intent.

 Signed-off-by: Jeff King p...@peff.net
 ---
 I wondered if there was a reason to avoid this (because we are in
 setup_git_env, which can potentially be called by git_pathdup). But the
 git_graft_file initialization below already uses it, and I
 double-checked that it is safe once git_dir is set.

 This patch will conflict textually with patch 6/28 of Duy's
 nd/multiple-work-trees series [1].

I'll just steal the conflicted bit about git_object_dir and put in the
re-roll. It's a better way anyway. If git_index_file change still
results in conflicts, Junio can resolve it easily (I don't touch it in
nd/multiple-work-trees).
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-24 Thread Duy Nguyen 
While it's about malloc..

On Fri, Jun 20, 2014 at 4:28 AM, Jeff King p...@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)?
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-24 Thread Jeff King 
On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:

 On Fri, Jun 20, 2014 at 4:28 AM, Jeff King p...@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 p...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-23 Thread Eric Sunshine 
On Thu, Jun 19, 2014 at 5:28 PM, Jeff King p...@peff.net wrote:
 This is shorter, harder to get wrong, and more clearly
 captures the intent.

 Signed-off-by: Jeff King p...@peff.net
 ---
 I wondered if there was a reason to avoid this (because we are in
 setup_git_env, which can potentially be called by git_pathdup). But the
 git_graft_file initialization below already uses it, and I
 double-checked that it is safe once git_dir is set.

This patch will conflict textually with patch 6/28 of Duy's
nd/multiple-work-trees series [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649

  environment.c | 12 
  1 file changed, 4 insertions(+), 8 deletions(-)

 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 (!git_object_dir)
 +   git_object_dir = git_pathdup(objects);
 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);
 -   }
 +   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);
 --
 2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-23 Thread Junio C Hamano 
Eric Sunshine sunsh...@sunshineco.com writes:

 On Thu, Jun 19, 2014 at 5:28 PM, Jeff King p...@peff.net wrote:
 This is shorter, harder to get wrong, and more clearly
 captures the intent.

 Signed-off-by: Jeff King p...@peff.net
 ---
 I wondered if there was a reason to avoid this (because we are in
 setup_git_env, which can potentially be called by git_pathdup). But the
 git_graft_file initialization below already uses it, and I
 double-checked that it is safe once git_dir is set.

 This patch will conflict textually with patch 6/28 of Duy's
 nd/multiple-work-trees series [1].

Thanks; I noticed that and dropped the other topic tentatively, as
it is being rerolled anyway.  In addition to that, because this
series seems fairly focused and well done, and the owners of two
topics known to be competent and active folks, I do not think there
is not much to be worried about ;-).


 [1]: 
 http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649

  environment.c | 12 
  1 file changed, 4 insertions(+), 8 deletions(-)

 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 (!git_object_dir)
 +   git_object_dir = git_pathdup(objects);
 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);
 -   }
 +   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);
 --
 2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-06-19 Thread Jeff King 
This is shorter, harder to get wrong, and more clearly
captures the intent.

Signed-off-by: Jeff King p...@peff.net
---
I wondered if there was a reason to avoid this (because we are in
setup_git_env, which can potentially be called by git_pathdup). But the
git_graft_file initialization below already uses it, and I
double-checked that it is safe once git_dir is set.

 environment.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

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 (!git_object_dir)
+   git_object_dir = git_pathdup(objects);
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);
-   }
+   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);
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html