{"thread":{"id":"22777","subject":"[PATCH] is_submodule_modified(): clear environment properly","startedAt":"2010-02-23T21:47:51Z","lastAt":"2010-02-24T06:43:28Z","messageCount":4,"participants":["Jens Lehmann","Giuseppe Bilotta","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135472","messageId":"4B844D07.8070406@web.de","threadId":"22777","inReplyTo":null,"subject":"[PATCH] is_submodule_modified(): clear environment properly","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-02-23T21:47:51Z","receivedAt":"2010-02-23T21:47:51Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Only GIT_INDEX_FILE was cleared until now, but other environment variables\nhave to be cleared too before running git status in a submodule.\n\nAnd while at it, don't allocate a new strbuf for GIT_INDEX_FILE but use\na string constant instead.\n\nSigned-off-by: Jens Lehmann \n---\n\n\nI think it makes sense to clear the same environment variables here\nas Giuseppe Bilotta's patch did for the git shell commands, even\nthough i am not aware of any bug reports yet.\n\n\n submodule.c | 17 ++++++++++++-----\n 1 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7d70c4f..f5707c2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -130,7 +130,7 @@ int is_submodule_modified(const char *path)\n \t\t\"--porcelain\",\n \t\tNULL,\n \t};\n-\tchar *env[4];\n+\tchar *env[9];\n \tstruct strbuf buf = STRBUF_INIT;\n\n \tstrbuf_addf(&buf, \"%s/.git/\", path);\n@@ -146,9 +146,17 @@ int is_submodule_modified(const char *path)\n \tenv[0] = strbuf_detach(&buf, NULL);\n \tstrbuf_addf(&buf, \"GIT_DIR=%s/.git\", path);\n \tenv[1] = strbuf_detach(&buf, NULL);\n-\tstrbuf_addf(&buf, \"GIT_INDEX_FILE\");\n-\tenv[2] = strbuf_detach(&buf, NULL);\n-\tenv[3] = NULL;\n+\t/*\n+\t * Clear repo-local GIT_* environment variables (Also see\n+\t * clear_local_git_env() in git-sh-setup.sh)\n+\t */\n+\tenv[2] = \"GIT_ALTERNATE_OBJECT_DIRECTORIES\";\n+\tenv[3] = \"GIT_CONFIG\";\n+\tenv[4] = \"GIT_GRAFT_FILE\";\n+\tenv[5] = \"GIT_INDEX_FILE\";\n+\tenv[6] = \"GIT_NO_REPLACE_OBJECTS\";\n+\tenv[7] = \"GIT_OBJECT_DIRECTORY\";\n+\tenv[8] = NULL;\n\n \tmemset(&cp, 0, sizeof(cp));\n \tcp.argv = argv;\n@@ -167,7 +175,6 @@ int is_submodule_modified(const char *path)\n\n \tfree(env[0]);\n \tfree(env[1]);\n-\tfree(env[2]);\n \tstrbuf_release(&buf);\n \treturn len != 0;\n }\n-- \n1.7.0.199.g508d1.dirty\n"},{"id":"135484","messageId":"cb7bb73a1002231413n3a38e344hce8188671763c17b@mail.gmail.com","threadId":"22777","inReplyTo":"4B844D07.8070406@web.de","subject":"Re: [PATCH] is_submodule_modified(): clear environment properly","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-23T22:13:36Z","receivedAt":"2010-02-23T22:13:36Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann wrote:\n> Only GIT_INDEX_FILE was cleared until now, but other environment variables\n> have to be cleared too before running git status in a submodule.\n>\n> And while at it, don't allocate a new strbuf for GIT_INDEX_FILE but use\n> a string constant instead.\n>\n> Signed-off-by: Jens Lehmann \n> ---\n>\n>\n> I think it makes sense to clear the same environment variables here\n> as Giuseppe Bilotta's patch did for the git shell commands, even\n> though i am not aware of any bug reports yet.\n\nI think this makes sense. This would be the third user of the \"list of\nrepo-local vars\", together with the shell function and the connect.c\ncode highlighted elsewhere, so I believe this really puts some more\npressure on a refactoring of the list generation. Maybe a static array\nexportable via git-rev-parse?\n\nOf course this third use-case has a difference in that GIT_DIR is\nactually set to something else and not just cleared, but this\nparticular case could just deep-copy the array modifying the\nappropriate entry.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"135520","messageId":"7veikbjokw.fsf@alter.siamese.dyndns.org","threadId":"22777","inReplyTo":"cb7bb73a1002231413n3a38e344hce8188671763c17b@mail.gmail.com","subject":"Re: [PATCH] is_submodule_modified(): clear environment properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T03:32:31Z","receivedAt":"2010-02-24T03:32:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann wrote:\n>> Only GIT_INDEX_FILE was cleared until now, but other environment variables\n>> have to be cleared too before running git status in a submodule.\n>>\n> Of course this third use-case has a difference in that GIT_DIR is\n> actually set to something else and not just cleared, but this\n> particular case could just deep-copy the array modifying the\n> appropriate entry.\n\nSomething like this, I guess.\n\nIt is a bit frustrating that local_repo_env[] is a NULL terminated array\nand we cannot use ARRAY_SIZE(local_repo_env) as a compile-time constant.\n\n submodule.c | 26 +++++++++++++++-----------\n 1 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7d70c4f..5bd70c4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -123,14 +123,14 @@ void show_submodule_summary(FILE *f, const char *path,\n \n int is_submodule_modified(const char *path)\n {\n-\tint len;\n+\tint len, i, dynbase;\n \tstruct child_process cp;\n \tconst char *argv[] = {\n \t\t\"status\",\n \t\t\"--porcelain\",\n \t\tNULL,\n \t};\n-\tchar *env[4];\n+\tconst char *env[20];\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_addf(&buf, \"%s/.git/\", path);\n@@ -142,17 +142,21 @@ int is_submodule_modified(const char *path)\n \t}\n \tstrbuf_reset(&buf);\n \n+\tfor (i = 0; local_repo_env[i] && i < ARRAY_SIZE(env); i++)\n+\t\tenv[i] = local_repo_env[i];\n+\tif (ARRAY_SIZE(env) <= i + 3)\n+\t\tdie(\"Bug: recompile submodule.c with larger array size\");\n+\tdynbase = i;\n+\n \tstrbuf_addf(&buf, \"GIT_WORK_TREE=%s\", path);\n-\tenv[0] = strbuf_detach(&buf, NULL);\n+\tenv[i++] = strbuf_detach(&buf, NULL);\n \tstrbuf_addf(&buf, \"GIT_DIR=%s/.git\", path);\n-\tenv[1] = strbuf_detach(&buf, NULL);\n-\tstrbuf_addf(&buf, \"GIT_INDEX_FILE\");\n-\tenv[2] = strbuf_detach(&buf, NULL);\n-\tenv[3] = NULL;\n+\tenv[i++] = strbuf_detach(&buf, NULL);\n+\tenv[i++] = NULL;\n \n \tmemset(&cp, 0, sizeof(cp));\n \tcp.argv = argv;\n-\tcp.env = (const char *const *)env;\n+\tcp.env = env;\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n@@ -165,9 +169,9 @@ int is_submodule_modified(const char *path)\n \tif (finish_command(&cp))\n \t\tdie(\"git status --porcelain failed\");\n \n-\tfree(env[0]);\n-\tfree(env[1]);\n-\tfree(env[2]);\n+\tfor (i = dynbase; env[i]; i++)\n+\t\tfree((char *)env[i]);\n+\n \tstrbuf_release(&buf);\n \treturn len != 0;\n }\n"},{"id":"135531","messageId":"cb7bb73a1002232243w2f5f63bcw45662bb80cb9e52@mail.gmail.com","threadId":"22777","inReplyTo":"7veikbjokw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] is_submodule_modified(): clear environment properly","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T06:43:28Z","receivedAt":"2010-02-24T06:43:28Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Feb 24, 2010 at 4:32 AM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> On Tue, Feb 23, 2010 at 10:47 PM, Jens Lehmann wrote:\n>>> Only GIT_INDEX_FILE was cleared until now, but other environment variables\n>>> have to be cleared too before running git status in a submodule.\n>>>\n>> Of course this third use-case has a difference in that GIT_DIR is\n>> actually set to something else and not just cleared, but this\n>> particular case could just deep-copy the array modifying the\n>> appropriate entry.\n>\n> Something like this, I guess.\n>\n> It is a bit frustrating that local_repo_env[] is a NULL terminated array\n> and we cannot use ARRAY_SIZE(local_repo_env) as a compile-time constant.\n\nI have a very simple solution for this, coming in the next revision of\nmy patchset, together with the environment.c move\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}