threads / patch / 22777

patchis_submodule_modified(): clear environment properly

Subject: [PATCH] is_submodule_modified(): clear environment properly

## tl;dr

4 messages between Feb 23, 2010 and Feb 24, 2010. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Jens Lehmann· Feb 23, 2010, 21:47 UTC · lore

Only GIT_INDEX_FILE was cleared until now, but other environment variables have to be cleared too before running git status in a submodule.

And while at it, don't allocate a new strbuf for GIT_INDEX_FILE but use a string constant instead.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

I think it makes sense to clear the same environment variables here as Giuseppe Bilotta's patch did for the git shell commands, even though i am not aware of any bug reports yet.

 submodule.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

Show changes to submodule.c +12 −5

diff --git a/submodule.c b/submodule.c
index 7d70c4f..f5707c2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -130,7 +130,7 @@ int is_submodule_modified(const char *path)
 		"--porcelain",
 		NULL,
 	};
-	char *env[4];
+	char *env[9];
 	struct strbuf buf = STRBUF_INIT;

 	strbuf_addf(&buf, "%s/.git/", path);
@@ -146,9 +146,17 @@ int is_submodule_modified(const char *path)
 	env[0] = strbuf_detach(&buf, NULL);
 	strbuf_addf(&buf, "GIT_DIR=%s/.git", path);
 	env[1] = strbuf_detach(&buf, NULL);
-	strbuf_addf(&buf, "GIT_INDEX_FILE");
-	env[2] = strbuf_detach(&buf, NULL);
-	env[3] = NULL;
+	/*
+	 * Clear repo-local GIT_* environment variables (Also see
+	 * clear_local_git_env() in git-sh-setup.sh)
+	 */
+	env[2] = "GIT_ALTERNATE_OBJECT_DIRECTORIES";
+	env[3] = "GIT_CONFIG";
+	env[4] = "GIT_GRAFT_FILE";
+	env[5] = "GIT_INDEX_FILE";
+	env[6] = "GIT_NO_REPLACE_OBJECTS";
+	env[7] = "GIT_OBJECT_DIRECTORY";
+	env[8] = NULL;

 	memset(&cp, 0, sizeof(cp));
 	cp.argv = argv;
@@ -167,7 +175,6 @@ int is_submodule_modified(const char *path)

 	free(env[0]);
 	free(env[1]);
-	free(env[2]);
 	strbuf_release(&buf);
 	return len != 0;
 }

-- 
1.7.0.199.g508d1.dirty

Giuseppe Bilotta· Feb 23, 2010, 22:13 UTC · re: Jens Lehmann · lore

Re: [PATCH] is_submodule_modified(): clear environment properly

On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:

Show 13 quoted lines

> Only GIT_INDEX_FILE was cleared until now, but other environment variables
> have to be cleared too before running git status in a submodule.
>
> And while at it, don't allocate a new strbuf for GIT_INDEX_FILE but use
> a string constant instead.
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
>
>
> I think it makes sense to clear the same environment variables here
> as Giuseppe Bilotta's patch did for the git shell commands, even
> though i am not aware of any bug reports yet.

I think this makes sense. This would be the third user of the "list of repo-local vars", together with the shell function and the connect.c code highlighted elsewhere, so I believe this really puts some more pressure on a refactoring of the list generation. Maybe a static array exportable via git-rev-parse?

Of course this third use-case has a difference in that GIT_DIR is actually set to something else and not just cleared, but this particular case could just deep-copy the array modifying the appropriate entry.

-- 
Giuseppe "Oblomov" Bilotta

Junio C Hamano· Feb 24, 2010, 03:32 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH] is_submodule_modified(): clear environment properly

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Show 8 quoted lines

> On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> Only GIT_INDEX_FILE was cleared until now, but other environment variables
>> have to be cleared too before running git status in a submodule.
>>
> Of course this third use-case has a difference in that GIT_DIR is
> actually set to something else and not just cleared, but this
> particular case could just deep-copy the array modifying the
> appropriate entry.

Something like this, I guess.

It is a bit frustrating that local_repo_env[] is a NULL terminated array and we cannot use ARRAY_SIZE(local_repo_env) as a compile-time constant.

 submodule.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

Show changes to submodule.c +15 −11

diff --git a/submodule.c b/submodule.c
index 7d70c4f..5bd70c4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -123,14 +123,14 @@ void show_submodule_summary(FILE *f, const char *path,
 
 int is_submodule_modified(const char *path)
 {
-	int len;
+	int len, i, dynbase;
 	struct child_process cp;
 	const char *argv[] = {
 		"status",
 		"--porcelain",
 		NULL,
 	};
-	char *env[4];
+	const char *env[20];
 	struct strbuf buf = STRBUF_INIT;
 
 	strbuf_addf(&buf, "%s/.git/", path);
@@ -142,17 +142,21 @@ int is_submodule_modified(const char *path)
 	}
 	strbuf_reset(&buf);
 
+	for (i = 0; local_repo_env[i] && i < ARRAY_SIZE(env); i++)
+		env[i] = local_repo_env[i];
+	if (ARRAY_SIZE(env) <= i + 3)
+		die("Bug: recompile submodule.c with larger array size");
+	dynbase = i;
+
 	strbuf_addf(&buf, "GIT_WORK_TREE=%s", path);
-	env[0] = strbuf_detach(&buf, NULL);
+	env[i++] = strbuf_detach(&buf, NULL);
 	strbuf_addf(&buf, "GIT_DIR=%s/.git", path);
-	env[1] = strbuf_detach(&buf, NULL);
-	strbuf_addf(&buf, "GIT_INDEX_FILE");
-	env[2] = strbuf_detach(&buf, NULL);
-	env[3] = NULL;
+	env[i++] = strbuf_detach(&buf, NULL);
+	env[i++] = NULL;
 
 	memset(&cp, 0, sizeof(cp));
 	cp.argv = argv;
-	cp.env = (const char *const *)env;
+	cp.env = env;
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
@@ -165,9 +169,9 @@ int is_submodule_modified(const char *path)
 	if (finish_command(&cp))
 		die("git status --porcelain failed");
 
-	free(env[0]);
-	free(env[1]);
-	free(env[2]);
+	for (i = dynbase; env[i]; i++)
+		free((char *)env[i]);
+
 	strbuf_release(&buf);
 	return len != 0;
 }

Giuseppe Bilotta· Feb 24, 2010, 06:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] is_submodule_modified(): clear environment properly

On Wed, Feb 24, 2010 at 4:32 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> Only GIT_INDEX_FILE was cleared until now, but other environment variables
>>> have to be cleared too before running git status in a submodule.
>>>
>> Of course this third use-case has a difference in that GIT_DIR is
>> actually set to something else and not just cleared, but this
>> particular case could just deep-copy the array modifying the
>> appropriate entry.
>
> Something like this, I guess.
>
> It is a bit frustrating that local_repo_env[] is a NULL terminated array
> and we cannot use ARRAY_SIZE(local_repo_env) as a compile-time constant.

I have a very simple solution for this, coming in the next revision of my patchset, together with the environment.c move

-- 
Giuseppe "Oblomov" Bilotta