{"thread":{"id":"22790","subject":"[PATCHv4 0/6] repo-local env vars handling","startedAt":"2010-02-24T07:23:10Z","lastAt":"2010-02-24T23:11:05Z","messageCount":15,"participants":["Giuseppe Bilotta","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"135533","messageId":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":null,"subject":"[PATCHv4 0/6] repo-local env vars handling","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:10Z","receivedAt":"2010-02-24T07:23:10Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Changes from the previous iteration:\n\n* moved the static list to environment.c, per Junio's suggestion\n* added a local_repo_env_size constant. It's cheap, and it helps\n prevent some horrible gimmicks when the list size needs to be known\n without walking it\n* a variation on Junio's variation on Jens' patch for submodule.c,\n using the static list _and_ the size constant.\n\nGiuseppe Bilotta (6):\n environment: static list of repo-local env vars\n connect: use static list of repo-local env vars\n rev-parse: --local-env-vars option\n shell setup: clear_local_git_env() function\n submodules: ensure clean environment when operating in a submodule\n is_submodule_modified(): clear environment properly\n\n Documentation/git-rev-parse.txt | 6 ++++++\n builtin-rev-parse.c | 8 ++++++++\n cache.h | 3 +++\n connect.c | 13 +------------\n environment.c | 15 +++++++++++++++\n git-sh-setup.sh | 7 +++++++\n git-submodule.sh | 20 ++++++++++----------\n submodule.c | 23 +++++++++++++----------\n 8 files changed, 63 insertions(+), 32 deletions(-)\n"},{"id":"135534","messageId":"1266996196-25978-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4 1/6] environment: static list of repo-local env vars","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:11Z","receivedAt":"2010-02-24T07:23:11Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta \n---\n cache.h | 3 +++\n environment.c | 15 +++++++++++++++\n 2 files changed, 18 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d454b7e..3b44fe2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -388,6 +388,9 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_NOTES_REF_ENVIRONMENT \"GIT_NOTES_REF\"\n #define GIT_NOTES_DEFAULT_REF \"refs/notes/commits\"\n \n+extern const char *const local_repo_env[];\n+extern const unsigned int local_repo_env_size;\n+\n extern int is_bare_repository_cfg;\n extern int is_bare_repository(void);\n extern int is_inside_git_dir(void);\ndiff --git a/environment.c b/environment.c\nindex 739ec27..b15352d 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -63,6 +63,21 @@ static char *work_tree;\n static const char *git_dir;\n static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;\n \n+/* Repository-local GIT_* environment variables */\n+const char *const local_repo_env[] = {\n+\tALTERNATE_DB_ENVIRONMENT,\n+\tCONFIG_ENVIRONMENT,\n+\tDB_ENVIRONMENT,\n+\tGIT_DIR_ENVIRONMENT,\n+\tGIT_WORK_TREE_ENVIRONMENT,\n+\tGRAFT_ENVIRONMENT,\n+\tINDEX_ENVIRONMENT,\n+\tNO_REPLACE_OBJECTS_ENVIRONMENT,\n+\tNULL\n+};\n+\n+const unsigned int local_repo_env_size = ARRAY_SIZE(local_repo_env);\n+\n static void setup_git_env(void)\n {\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n-- \n1.7.0.212.g4e217.dirty\n"},{"id":"135535","messageId":"1266996196-25978-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4 2/6] connect: use static list of repo-local env vars","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:12Z","receivedAt":"2010-02-24T07:23:12Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This adds the missing GIT_CONFIG to the list of env vars that\nshould be cleared.\n\nSigned-off-by: Giuseppe Bilotta \n---\n connect.c | 13 +------------\n 1 files changed, 1 insertions(+), 12 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9f39038..94b3707 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -582,18 +582,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t*arg++ = host;\n \t}\n \telse {\n-\t\t/* remove these from the environment */\n-\t\tconst char *env[] = {\n-\t\t\tALTERNATE_DB_ENVIRONMENT,\n-\t\t\tDB_ENVIRONMENT,\n-\t\t\tGIT_DIR_ENVIRONMENT,\n-\t\t\tGIT_WORK_TREE_ENVIRONMENT,\n-\t\t\tGRAFT_ENVIRONMENT,\n-\t\t\tINDEX_ENVIRONMENT,\n-\t\t\tNO_REPLACE_OBJECTS_ENVIRONMENT,\n-\t\t\tNULL\n-\t\t};\n-\t\tconn->env = env;\n+\t\tconn->env = local_repo_env;\n \t\tconn->use_shell = 1;\n \t}\n \t*arg++ = cmd.buf;\n-- \n1.7.0.212.g4e217.dirty\n"},{"id":"135537","messageId":"1266996196-25978-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4 3/6] rev-parse: --local-env-vars option","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:13Z","receivedAt":"2010-02-24T07:23:13Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This prints the list of repo-local environment variables.\n\nSigned-off-by: Giuseppe Bilotta \n---\n Documentation/git-rev-parse.txt | 6 ++++++\n builtin-rev-parse.c | 8 ++++++++\n 2 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 1a613aa..8db600f 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -148,6 +148,12 @@ shown. If the pattern does not contain a globbing character (`?`,\n --is-bare-repository::\n \tWhen the repository is bare print \"true\", otherwise \"false\".\n \n+--local-env-vars::\n+\tList the GIT_* environment variables that are local to the\n+\trepository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).\n+\tOnly the names of the variables are listed, not their value,\n+\teven if they are set.\n+\n --short::\n --short=number::\n \tInstead of outputting the full SHA1 values of object names try to\ndiff --git a/builtin-rev-parse.c b/builtin-rev-parse.c\nindex a8c5043..ab7b520 100644\n--- a/builtin-rev-parse.c\n+++ b/builtin-rev-parse.c\n@@ -455,7 +455,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tif (argc > 1 && !strcmp(\"--sq-quote\", argv[1]))\n \t\treturn cmd_sq_quote(argc - 2, argv + 2);\n \n+\tif (argc > 0 && !strcmp(\"--local-env-vars\", argv[1])) {\n+\t\tunsigned int i = 0;\n+\t\tconst char *var;\n+\t\twhile (var = local_repo_env[i++])\n+\t\t\tprintf(\"%s\\n\", var);\n+\t\treturn 0;\n+\t}\n \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n+\n \t\tusage(builtin_rev_parse_usage);\n \n \tprefix = setup_git_directory();\n-- \n1.7.0.212.g4e217.dirty\n"},{"id":"135536","messageId":"1266996196-25978-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4 4/6] shell setup: clear_local_git_env() function","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:14Z","receivedAt":"2010-02-24T07:23:14Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Introduce an auxiliary function to clear all repo-local environment\nvariables. This should be invoked by any shell script that switches\nrepository during execution, to ensure that the environment is clean\nand that things such as the git dir and worktree are set up correctly.\n\nSigned-off-by: Giuseppe Bilotta \n---\n git-sh-setup.sh | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 7a09566..6131670 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -172,6 +172,13 @@ get_author_ident_from_commit () {\n \tLANG=C LC_ALL=C sed -ne \"$pick_author_script\"\n }\n \n+# Clear repo-local GIT_* environment variables. Useful when switching to\n+# another repository (e.g. when entering a submodule). See also the env\n+# list in git_connect()\n+clear_local_git_env() {\n+\tunset $(git rev-parse --local-env-vars)\n+}\n+\n # Make sure we are in a valid repository of a vintage we understand,\n # if we require to be in a git repository.\n if test -z \"$NONGIT_OK\"\n-- \n1.7.0.212.g4e217.dirty\n"},{"id":"135538","messageId":"1266996196-25978-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4 5/6] submodules: ensure clean environment when operating in a submodule","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:15Z","receivedAt":"2010-02-24T07:23:15Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"git-submodule used to take care of clearing GIT_DIR whenever it operated\non a submodule index or configuration, but forgot to unset GIT_WORK_TREE\nor other repo-local variables. This would lead to failures e.g. when\nGIT_WORK_TREE was set.\n\nThis only happened in very unusual contexts such as operating on the\nmain worktree from outside of it, but since \"git-gui: set GIT_DIR and\nGIT_WORK_TREE after setup\" (a9fa11fe5bd5978bb) such failures could also\nbe provoked by invoking an external tool such as \"git submodule update\"\nfrom the Git Gui in a standard setup.\n\nSolve by using the newly introduced clear_local_git_env() shell function\nto ensure that all repo-local environment variables are unset.\n\nSigned-off-by: Giuseppe Bilotta \n---\n git-submodule.sh | 20 ++++++++++----------\n 1 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5869c00..1ea4143 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -222,7 +222,7 @@ cmd_add()\n \n \t\tmodule_clone \"$path\" \"$realrepo\" \"$reference\" || exit\n \t\t(\n-\t\t\tunset GIT_DIR\n+\t\t\tclear_local_git_env\n \t\t\tcd \"$path\" &&\n \t\t\t# ash fails to wordsplit ${branch:+-b \"$branch\"...}\n \t\t\tcase \"$branch\" in\n@@ -278,7 +278,7 @@ cmd_foreach()\n \t\t\tname=$(module_name \"$path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$path/\"\n-\t\t\t\tunset GIT_DIR\n+\t\t\t\tclear_local_git_env\n \t\t\t\tcd \"$path\" &&\n \t\t\t\teval \"$@\" &&\n \t\t\t\tif test -n \"$recursive\"\n@@ -434,7 +434,7 @@ cmd_update()\n \t\t\tmodule_clone \"$path\" \"$url\" \"$reference\"|| exit\n \t\t\tsubsha1=\n \t\telse\n-\t\t\tsubsha1=$(unset GIT_DIR; cd \"$path\" &&\n+\t\t\tsubsha1=$(clear_local_git_env; cd \"$path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"Unable to find current revision in submodule path '$path'\"\n \t\tfi\n@@ -454,7 +454,7 @@ cmd_update()\n \n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n-\t\t\t\t(unset GIT_DIR; cd \"$path\" &&\n+\t\t\t\t(clear_local_git_env; cd \"$path\" &&\n \t\t\t\t\tgit-fetch) ||\n \t\t\t\tdie \"Unable to fetch in submodule path '$path'\"\n \t\t\tfi\n@@ -477,14 +477,14 @@ cmd_update()\n \t\t\t\t;;\n \t\t\tesac\n \n-\t\t\t(unset GIT_DIR; cd \"$path\" && $command \"$sha1\") ||\n+\t\t\t(clear_local_git_env; cd \"$path\" && $command \"$sha1\") ||\n \t\t\tdie \"Unable to $action '$sha1' in submodule path '$path'\"\n \t\t\tsay \"Submodule path '$path': $msg '$sha1'\"\n \t\tfi\n \n \t\tif test -n \"$recursive\"\n \t\tthen\n-\t\t\t(unset GIT_DIR; cd \"$path\" && cmd_update $orig_args) ||\n+\t\t\t(clear_local_git_env; cd \"$path\" && cmd_update $orig_args) ||\n \t\t\tdie \"Failed to recurse into submodule path '$path'\"\n \t\tfi\n \tdone\n@@ -492,7 +492,7 @@ cmd_update()\n \n set_name_rev () {\n \trevname=$( (\n-\t\tunset GIT_DIR\n+\t\tclear_local_git_env\n \t\tcd \"$1\" && {\n \t\t\tgit describe \"$2\" 2>/dev/null ||\n \t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n@@ -760,7 +760,7 @@ cmd_status()\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n-\t\t\t\tsha1=$(unset GIT_DIR; cd \"$path\" && git rev-parse --verify HEAD)\n+\t\t\t\tsha1=$(clear_local_git_env; cd \"$path\" && git rev-parse --verify HEAD)\n \t\t\t\tset_name_rev \"$path\" \"$sha1\"\n \t\t\tfi\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n@@ -770,7 +770,7 @@ cmd_status()\n \t\tthen\n \t\t\t(\n \t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tunset GIT_DIR\n+\t\t\t\tclear_local_git_env\n \t\t\t\tcd \"$path\" &&\n \t\t\t\tcmd_status $orig_args\n \t\t\t) ||\n@@ -821,7 +821,7 @@ cmd_sync()\n \t\tif test -e \"$path\"/.git\n \t\tthen\n \t\t(\n-\t\t\tunset GIT_DIR\n+\t\t\tclear_local_git_env\n \t\t\tcd \"$path\"\n \t\t\tremote=$(get_default_remote)\n \t\t\tsay \"Synchronizing submodule url for '$name'\"\n-- \n1.7.0.212.g4e217.dirty\n"},{"id":"135539","messageId":"1266996196-25978-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"22790","inReplyTo":"1266996196-25978-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4 6/6] is_submodule_modified(): clear environment properly","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T07:23:16Z","receivedAt":"2010-02-24T07:23:16Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Rather than only clearing GIT_INDEX_FILE, take the list of environment\nvariables to clear from local_repo_env, appending the settings for\nGIT_DIR and GIT_WORK_TREE.\n\nSigned-off-by: Giuseppe Bilotta \n---\n submodule.c | 23 +++++++++++++----------\n 1 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7d70c4f..81cf05a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -124,15 +124,21 @@ void show_submodule_summary(FILE *f, const char *path,\n int is_submodule_modified(const char *path)\n {\n \tint len;\n+\tunsigned int i = 0;\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[local_repo_env_size+2];\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\t/* Copy local_repo_env to env, letting i\n+\t rest at the last NULL */\n+\twhile (env[i] = local_repo_env[i])\n+\t\t++i; /* do nothing */\n+\n \tstrbuf_addf(&buf, \"%s/.git/\", path);\n \tif (!is_directory(buf.buf)) {\n \t\tstrbuf_release(&buf);\n@@ -143,16 +149,14 @@ int is_submodule_modified(const char *path)\n \tstrbuf_reset(&buf);\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,8 @@ 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+\tfree((char *)env[--i]);\n+\tfree((char *)env[--i]);\n \tstrbuf_release(&buf);\n \treturn len != 0;\n }\n-- \n1.7.0.212.g4e217.dirty\n"},{"id":"135543","messageId":"4B84DDE9.8000301@viscovery.net","threadId":"22790","inReplyTo":"1266996196-25978-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4 6/6] is_submodule_modified(): clear environment properly","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-24T08:06:01Z","receivedAt":"2010-02-24T08:06:01Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Giuseppe Bilotta schrieb:\n> +\tconst char *env[local_repo_env_size+2];\n\nVariable sized arrays are prohibited.\n\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> +\t/* Copy local_repo_env to env, letting i\n> +\t rest at the last NULL */\n> +\twhile (env[i] = local_repo_env[i])\n> +\t\t++i; /* do nothing */\n> +\n\nThis looks very inconsistent: At the one hand, you use l_r_e_size to\nallocate the space, but then you iterate over it assuming that the list is\n(also) NULL-terminated. But this is only a minor nit.\n\n-- Hannes\n"},{"id":"135546","messageId":"cb7bb73a1002240055v58953dcfh3be7c4589523b14b@mail.gmail.com","threadId":"22790","inReplyTo":"4B84DDE9.8000301@viscovery.net","subject":"Re: [PATCHv4 6/6] is_submodule_modified(): clear environment properly","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T08:55:14Z","receivedAt":"2010-02-24T08:55:14Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Feb 24, 2010 at 9:06 AM, Johannes Sixt wrote:\n> Giuseppe Bilotta schrieb:\n>> +     const char *env[local_repo_env_size+2];\n>\n> Variable sized arrays are prohibited.\n\nAh, sorry. Is alloca() allowed? I don't see it being used anywhere\nelse in the code, and malloc would be a little too much for this case.\n\n>>       struct strbuf buf = STRBUF_INIT;\n>>\n>> +     /* Copy local_repo_env to env, letting i\n>> +        rest at the last NULL */\n>> +     while (env[i] = local_repo_env[i])\n>> +             ++i; /* do nothing */\n>> +\n>\n> This looks very inconsistent: At the one hand, you use l_r_e_size to\n> allocate the space, but then you iterate over it assuming that the list is\n> (also) NULL-terminated. But this is only a minor nit.\n\nWell, if you consider that using l-r-e-size is just a way to spare a\ndouble-walk, the inconsistency is tolerable. OTOH, in this particular\ncase NULL-walking the list doesn't give the usual benefit of sparing a\ncounter, so I could rework the patch to use a standard for loop. (I\nalso notice that I forgot to remove the /* do nothing */ comment from\nthe while(env[i] = local_repo_env[i++]) ; --i approach I was going\nwith at first)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"135547","messageId":"4B84EEF2.2030502@viscovery.net","threadId":"22790","inReplyTo":"cb7bb73a1002240055v58953dcfh3be7c4589523b14b@mail.gmail.com","subject":"Re: [PATCHv4 6/6] is_submodule_modified(): clear environment properly","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-24T09:18:42Z","receivedAt":"2010-02-24T09:18:42Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Giuseppe Bilotta schrieb:\n> On Wed, Feb 24, 2010 at 9:06 AM, Johannes Sixt wrote:\n>> Giuseppe Bilotta schrieb:\n>>> + const char *env[local_repo_env_size+2];\n>> Variable sized arrays are prohibited.\n> \n> Ah, sorry. Is alloca() allowed? I don't see it being used anywhere\n> else in the code, and malloc would be a little too much for this case.\n\nin cache.h:\n\n#define LOCAL_REPO_ENV_CAPACITY 20\nconst char *const l_r_e[];\n\nin environtment.c:\n\nconst char *const l_r_e[LOCAL_REPO_ENV_CAPACITY] = { ..., NULL };\n\nand the compiler will croak if the constant becomes too small. (Oh, I\nknow, you must use the 'inconsistent mode' that I nit-picked if you use\nthis approach.)\n\n-- Hannes\n"},{"id":"135549","messageId":"cb7bb73a1002240207u39176830scfa773b3e7d5c56@mail.gmail.com","threadId":"22790","inReplyTo":"4B84EEF2.2030502@viscovery.net","subject":"Re: [PATCHv4 6/6] is_submodule_modified(): clear environment properly","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T10:07:01Z","receivedAt":"2010-02-24T10:07:01Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Feb 24, 2010 at 10:18 AM, Johannes Sixt wrote:\n> Giuseppe Bilotta schrieb:\n>> On Wed, Feb 24, 2010 at 9:06 AM, Johannes Sixt wrote:\n>>> Giuseppe Bilotta schrieb:\n>>>> +     const char *env[local_repo_env_size+2];\n>>> Variable sized arrays are prohibited.\n>>\n>> Ah, sorry. Is alloca() allowed? I don't see it being used anywhere\n>> else in the code, and malloc would be a little too much for this case.\n>\n> in cache.h:\n>\n> #define LOCAL_REPO_ENV_CAPACITY 20\n> const char *const l_r_e[];\n>\n> in environtment.c:\n>\n> const char *const l_r_e[LOCAL_REPO_ENV_CAPACITY] = { ..., NULL };\n\nWhat I don't like about this approach is that (1) it allocates more\nmemory than necessary and (2) it requires recompiling everything if\nLOCAL_REPO_ENV_CAPACITY changes (since it's defined in cache.h which\nis included basically everywhere. Admittedly (2) is _very_ unlikely to\nhappen if the #define is set large enough, but still ...\n\n> and the compiler will croak if the constant becomes too small. (Oh, I\n> know, you must use the 'inconsistent mode' that I nit-picked if you use\n> this approach.)\n\n(BTW, that approach requires slightly less index juggling and less C\ncode than a indexed for loop, so I kept it anyway, with a slightly\nmore expanded comment).\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"135575","messageId":"7vhbp64odi.fsf@alter.siamese.dyndns.org","threadId":"22790","inReplyTo":"1266996196-25978-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4 1/6] environment: static list of repo-local env vars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T15:58:17Z","receivedAt":"2010-02-24T15:58:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> +/* Repository-local GIT_* environment variables */\n> +const char *const local_repo_env[] = {\n> +\tALTERNATE_DB_ENVIRONMENT,\n> +\tCONFIG_ENVIRONMENT,\n> +\tDB_ENVIRONMENT,\n> +\tGIT_DIR_ENVIRONMENT,\n> +\tGIT_WORK_TREE_ENVIRONMENT,\n> +\tGRAFT_ENVIRONMENT,\n> +\tINDEX_ENVIRONMENT,\n> +\tNO_REPLACE_OBJECTS_ENVIRONMENT,\n> +\tNULL\n> +};\n> +\n> +const unsigned int local_repo_env_size = ARRAY_SIZE(local_repo_env);\n\nThis does not look very useful for two reasons.\n\n - It counts the NULL at the tail, so the number is one-off; you cannot\n say\n\n\tfor (i = 0; i < local_repo_env_size; i++)\n\t\tdo_something_to(local_repo_env[i]);\n\n - local_repo_env_size is not a compile time constant, so you cannot do:\n\n\tconst char *env[local_repo_env_size + 20];\n\tmemcpy(env, local_repo_env, sizeof(env[0]) * local_repo_env_size);\n\tfor (i = local_repo_env_size; i < ARRAY_SIZE(env); i++)\n\t\tadd_more_to(env + i, ...);\n"},{"id":"135599","messageId":"cb7bb73a1002241120i4bbbce0bl50460c76db0c6529@mail.gmail.com","threadId":"22790","inReplyTo":"7vhbp64odi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 1/6] environment: static list of repo-local env vars","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T19:20:14Z","receivedAt":"2010-02-24T19:20:14Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Feb 24, 2010 at 4:58 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> +/* Repository-local GIT_* environment variables */\n>> +const char *const local_repo_env[] = {\n>> +     ALTERNATE_DB_ENVIRONMENT,\n>> +     CONFIG_ENVIRONMENT,\n>> +     DB_ENVIRONMENT,\n>> +     GIT_DIR_ENVIRONMENT,\n>> +     GIT_WORK_TREE_ENVIRONMENT,\n>> +     GRAFT_ENVIRONMENT,\n>> +     INDEX_ENVIRONMENT,\n>> +     NO_REPLACE_OBJECTS_ENVIRONMENT,\n>> +     NULL\n>> +};\n>> +\n>> +const unsigned int local_repo_env_size = ARRAY_SIZE(local_repo_env);\n>\n> This does not look very useful for two reasons.\n>\n>  - It counts the NULL at the tail, so the number is one-off; you cannot\n>   say\n>\n>        for (i = 0; i < local_repo_env_size; i++)\n>                do_something_to(local_repo_env[i]);\n\nThis is obviously very easy to fix by decrementing the size by 1 in\nthe definition.\n\n>  - local_repo_env_size is not a compile time constant, so you cannot do:\n>\n>        const char *env[local_repo_env_size + 20];\n>        memcpy(env, local_repo_env, sizeof(env[0]) * local_repo_env_size);\n>        for (i = local_repo_env_size; i < ARRAY_SIZE(env); i++)\n>                add_more_to(env + i, ...);\n\nIndeed, I was just discussing this on the other thread. I personally\nhave no objection to using this C99 feature.\n\nI guess #defining local_repo_env_size in cache.h, and keeping it\nup-to-date when local_repo_env is updated is the best alternative.\nThis can be done in such a way that if it the array needs expansion,\nforgetting to update its size is catched at compile time, but not for\ncontraction. Of course we don't expect that it will contract. The\ndisadvantage of this is that a change in cache.h requires an almost\ncomplete recompile because almost everything depends on it. It's a\nminor disadvantage, when compared to other approaches (double-walking\nthe list or other stuff).\n\n(Of course, I still wonder what's wrong with C99 VLAs, but anyway).\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"135608","messageId":"7vocjemnnq.fsf@alter.siamese.dyndns.org","threadId":"22790","inReplyTo":"cb7bb73a1002241120i4bbbce0bl50460c76db0c6529@mail.gmail.com","subject":"Re: [PATCHv4 1/6] environment: static list of repo-local env vars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T19:36:25Z","receivedAt":"2010-02-24T19:36:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> On Wed, Feb 24, 2010 at 4:58 PM, Junio C Hamano wrote:\n>> Giuseppe Bilotta writes:\n>>\n>>> +/* Repository-local GIT_* environment variables */\n>>> +const char *const local_repo_env[] = {\n>>> +     ALTERNATE_DB_ENVIRONMENT,\n>>> +     CONFIG_ENVIRONMENT,\n>>> +     DB_ENVIRONMENT,\n>>> +     GIT_DIR_ENVIRONMENT,\n>>> +     GIT_WORK_TREE_ENVIRONMENT,\n>>> +     GRAFT_ENVIRONMENT,\n>>> +     INDEX_ENVIRONMENT,\n>>> +     NO_REPLACE_OBJECTS_ENVIRONMENT,\n>>> +     NULL\n>>> +};\n>>> +\n>>> +const unsigned int local_repo_env_size = ARRAY_SIZE(local_repo_env);\n>>\n>> This does not look very useful for two reasons.\n\nBy the way, I said \"frustrating\" but didn't mean we should bend backwards.\nThe earlier one is more than Ok, and is certainly better than having\nlocal_repo_env_size that is not very useful in practice.\n"},{"id":"135627","messageId":"cb7bb73a1002241511w1ed2e5e7pbc4565b81a6a1039@mail.gmail.com","threadId":"22790","inReplyTo":"7vocjemnnq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 1/6] environment: static list of repo-local env vars","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-02-24T23:11:05Z","receivedAt":"2010-02-24T23:11:05Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Feb 24, 2010 at 8:36 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> On Wed, Feb 24, 2010 at 4:58 PM, Junio C Hamano wrote:\n>>> Giuseppe Bilotta writes:\n>>>\n>>>> +/* Repository-local GIT_* environment variables */\n>>>> +const char *const local_repo_env[] = {\n>>>> +     ALTERNATE_DB_ENVIRONMENT,\n>>>> +     CONFIG_ENVIRONMENT,\n>>>> +     DB_ENVIRONMENT,\n>>>> +     GIT_DIR_ENVIRONMENT,\n>>>> +     GIT_WORK_TREE_ENVIRONMENT,\n>>>> +     GRAFT_ENVIRONMENT,\n>>>> +     INDEX_ENVIRONMENT,\n>>>> +     NO_REPLACE_OBJECTS_ENVIRONMENT,\n>>>> +     NULL\n>>>> +};\n>>>> +\n>>>> +const unsigned int local_repo_env_size = ARRAY_SIZE(local_repo_env);\n>>>\n>>> This does not look very useful for two reasons.\n>\n> By the way, I said \"frustrating\" but didn't mean we should bend backwards.\n> The earlier one is more than Ok, and is certainly better than having\n> local_repo_env_size that is not very useful in practice.\n\nBut I'd like a manually-set local_repo_env_size than the gimmicks\nnecessary if it's not there (plus the runtime failure). If there are\nno other corrections, I'll send a new series (that also squashes the\nfirst two patches per Johannes' suggestion) and the #define\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}