# [PATCH] is_submodule_modified(): clear environment properly 4 messages from 2010-02-23 to 2010-02-24. Participants: Jens Lehmann, Giuseppe Bilotta, Junio C Hamano. Thread: https://gitlist.dev/t/22777 ## Jens Lehmann, 2010-02-23 21:47 Subject: [PATCH] is_submodule_modified(): clear environment properly Message-ID: <4B844D07.8070406@web.de> URL: https://gitlist.dev/e/4B844D07.8070406%40web.de ``` 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 --- 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(-) 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, 2010-02-23 22:13 Subject: Re: [PATCH] is_submodule_modified(): clear environment properly Message-ID: URL: https://gitlist.dev/e/cb7bb73a1002231413n3a38e344hce8188671763c17b%40mail.gmail.com In-Reply-To: <4B844D07.8070406@web.de> ``` On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann 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. > > 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 > --- > > > 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, 2010-02-24 03:32 Subject: Re: [PATCH] is_submodule_modified(): clear environment properly Message-ID: <7veikbjokw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veikbjokw.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Giuseppe Bilotta writes: > On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann 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(-) 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, 2010-02-24 06:43 Subject: Re: [PATCH] is_submodule_modified(): clear environment properly Message-ID: URL: https://gitlist.dev/e/cb7bb73a1002232243w2f5f63bcw45662bb80cb9e52%40mail.gmail.com In-Reply-To: <7veikbjokw.fsf@alter.siamese.dyndns.org> ``` On Wed, Feb 24, 2010 at 4:32 AM, Junio C Hamano wrote: > Giuseppe Bilotta writes: > >> On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann 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 ```