Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
Jeff King writes: > On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote: > >> Jeff King 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 >> > --- >> >> 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
On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote: > Jeff King 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 > > --- > > 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
Jeff King 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
> ---
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
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
Signed-off-by: Junio C Hamano
---
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
On Wed, Jun 25, 2014 at 3:58 AM, Jeff King 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
> ---
> 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
On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:
> On Fri, Jun 20, 2014 at 4:28 AM, Jeff King 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
---
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
While it's about malloc..
On Fri, Jun 20, 2014 at 4:28 AM, Jeff King 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
On Mon, Jun 23, 2014 at 5:21 PM, Eric Sunshine wrote: > On Thu, Jun 19, 2014 at 5:28 PM, Jeff King wrote: >> This is shorter, harder to get wrong, and more clearly >> captures the intent. >> >> Signed-off-by: Jeff King >> --- >> 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
Eric Sunshine writes:
> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King wrote:
>> This is shorter, harder to get wrong, and more clearly
>> captures the intent.
>>
>> Signed-off-by: Jeff King
>> ---
>> 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
Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
On Thu, Jun 19, 2014 at 5:28 PM, Jeff King wrote:
> This is shorter, harder to get wrong, and more clearly
> captures the intent.
>
> Signed-off-by: Jeff King
> ---
> 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
[PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
This is shorter, harder to get wrong, and more clearly
captures the intent.
Signed-off-by: Jeff King
---
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
