Skip to contentSkip to conversation

is_submodule_modified(): clear environment properly

PATCH In review Jens Lehmann 3 participants 4 messages Jun 15, 2016

lore
Share & tools
lore

Jens Lehmann

author
#0 lore raw

PATCH 1/1 is_submodule_modified(): clear environment properly

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 <[email protected]>

---

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(-)
submodule.c +12 -5
index 7d70c4f..f5707c2 100644
@@ -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;
 }
Signature · 1 line
1.7.0.199.g508d1.dirty
#1 lore raw
Quoted 6 lines · Jens Lehmann

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.

Quoted 7 lines
Signed-off-by Jens Lehmann <[email protected]>

---

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.

Signature · 1 line
Giuseppe "Oblomov" Bilotta
#2 lore raw
Quoted 8 lines · Giuseppe Bilotta
Quoted 3 lines · Jens Lehmann

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(-)
submodule.c +15 -11
index 7d70c4f..5bd70c4 100644
@@ -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;
 }
#3 lore raw
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Giuseppe Bilotta
Quoted 3 lines · Jens Lehmann

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.

Quoted 4 lines

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

Signature · 1 line
Giuseppe "Oblomov" Bilotta