{"thread":{"id":"8509","subject":"[PATCH] make git barf when an alias changes environment variables","startedAt":"2007-06-08T20:57:55Z","lastAt":"2007-06-13T05:41:36Z","messageCount":3,"participants":["Matthias Lederhofer","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"44374","messageId":"20070608205755.GA21901@moooo.ath.cx","threadId":"8509","inReplyTo":null,"subject":"[PATCH] make git barf when an alias changes environment variables","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2007-06-08T20:57:55Z","receivedAt":"2007-06-08T20:57:55Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Aliases changing environment variables (GIT_DIR or\nGIT_WORK_TREE) can cause problems:\ngit has to use GIT_DIR to read the aliases from the config.\nAfter running handle_options for the alias the options of the\nalias may have changed environment variables.  Depending on\nthe implementation of setenv the memory location obtained\nthrough getenv earlier may contain the old value or the new\nvalue (or even be used for something else?).  To avoid these\nproblems git errors out if an alias uses any option which\nchanges environment variables.\n\nSigned-off-by: Matthias Lederhofer <matled@gmx.net>\n---\nThis is on top of ml/worktree even though the problem is also present in\nmaster.  It could also be split into one patch which can be applied to\nmaster directly and another one adding the missing parts in the\nml/worktree branch.\n\nAnother option instead of dying would be to execute git again.  I\ndecided not to because of\n(1) It causes problems with aliases starting the pager: After the exec\n    git wouldn't know that the pager is running and colors would be\n    missing.\n(2) Up to now aliases not starting with '!' have recursion detection\n    and are only evaluated as aliases once.  Executing git again would\n    break this.\n\nExample:\n\nHere is an example how to break git aliases, it works with libc of\nFreeBSD but not with glibc.  libc of FreeBSD reuses the old location\nfor the environment variable if the new value is smaller than the old\none.\n\nThere is a repository in a/.git and a repository in b/.git.  Both have\nvalid HEADs:\n\n% git --git-dir a/.git rev-list --pretty=oneline HEAD\n496a0d181f6878ddda8926103a7cf28a668c46ef a\n\n% git --git-dir b/.git rev-list --pretty=oneline HEAD\n3c2858eded8ae7ff964549f85f479808d185283c b\n\nSet up some aliases using --git-dir:\n\n% git --git-dir a/.git config alias.works \\\n'--git-dir a/.git rev-list --pretty=oneline HEAD'\n\n% git --git-dir a/.git config alias.breaks \\\n'--git-dir b/.git rev-list --pretty=oneline HEAD'\n\n% git --git-dir a/.git config alias.good\n'!git --git-dir b/.git rev-list --pretty=oneline HEAD'\n\nThe first one works because the value of the environment variable does\nnot change, the second one does not work because git reads the ref\nHEAD from b/.git but searches the object in a/.git.  The third one\nalways works.\n\n% git --git-dir a/.git works\n496a0d181f6878ddda8926103a7cf28a668c46ef a\n\n% git --git-dir a/.git breaks\nfatal: bad object HEAD\n[1]    15643 exit 128   git --git-dir a/.git breaks\n\n% git --git-dir a/.git good  \n3c2858eded8ae7ff964549f85f479808d185283c b\n\nWith the patch:\n\n% git --git-dir a/.git breaks       \nfatal: alias 'breaks' changes environment variables\nYou can use '!git' in the alias to do this.\n[1]    17295 exit 128   git --git-dir a/.git breaks\n\n---\n git.c |   22 ++++++++++++++++++----\n 1 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex cd3910a..33edd62 100644\n--- a/git.c\n+++ b/git.c\n@@ -28,7 +28,7 @@ static void prepend_to_path(const char *dir, int len)\n \tfree(path);\n }\n \n-static int handle_options(const char*** argv, int* argc)\n+static int handle_options(const char*** argv, int* argc, int* envchanged)\n {\n \tint handled = 0;\n \n@@ -64,24 +64,34 @@ static int handle_options(const char*** argv, int* argc)\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t\thandled++;\n \t\t} else if (!prefixcmp(cmd, \"--git-dir=\")) {\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n \t\t\tif (*argc < 2) {\n \t\t\t\tfprintf(stderr, \"No directory given for --work-tree.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n \t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else if (!prefixcmp(cmd, \"--work-tree=\")) {\n \t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--bare\")) {\n \t\t\tstatic char git_dir[PATH_MAX+1];\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else {\n \t\t\tfprintf(stderr, \"Unknown option: %s\\n\", cmd);\n \t\t\tusage(git_usage_string);\n@@ -160,7 +170,7 @@ static int split_cmdline(char *cmdline, const char ***argv)\n \n static int handle_alias(int *argcp, const char ***argv)\n {\n-\tint nongit = 0, ret = 0, saved_errno = errno;\n+\tint nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;\n \tconst char *subdir;\n \tint count, option_count;\n \tconst char** new_argv;\n@@ -181,7 +191,11 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\t    alias_string + 1, alias_command);\n \t\t}\n \t\tcount = split_cmdline(alias_string, &new_argv);\n-\t\toption_count = handle_options(&new_argv, &count);\n+\t\toption_count = handle_options(&new_argv, &count, &envchanged);\n+\t\tif (envchanged)\n+\t\t\tdie(\"alias '%s' changes environment variables\\n\"\n+\t\t\t\t \"You can use '!git' in the alias to do this.\",\n+\t\t\t\t alias_command);\n \t\tmemmove(new_argv - option_count, new_argv,\n \t\t\t\tcount * sizeof(char *));\n \t\tnew_argv -= option_count;\n@@ -375,7 +389,7 @@ int main(int argc, const char **argv, char **envp)\n \t/* Look for flags.. */\n \targv++;\n \targc--;\n-\thandle_options(&argv, &argc);\n+\thandle_options(&argv, &argc, NULL);\n \tif (argc > 0) {\n \t\tif (!prefixcmp(argv[0], \"--\"))\n \t\t\targv[0] += 2;\n-- \n1.5.2.1.888.gd5e4e\n"},{"id":"44887","messageId":"7vejkgh1jf.fsf@assigned-by-dhcp.pobox.com","threadId":"8509","inReplyTo":"20070608205755.GA21901@moooo.ath.cx","subject":"Re: [PATCH] make git barf when an alias changes environment variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-13T05:21:56Z","receivedAt":"2007-06-13T05:21:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"What does this patch do when you do this?\n\n\t: Because I usually work with this repository...\n\t$ GIT_DIR=$some_repository\n        $ export GIT_DIR\n\n        : Then much later, I happen to visit another repository\n        : to take a peek...\n\t$ cd $somewhere_else\n        $ git --git-dir .git some-command\n\nI think --git-dir request on the command line is primarily to\nallow overriding whatever the user has in the environment as the\nfallback default, so if the patch makes it barf that is not very\nnice.\n\n\n                \n"},{"id":"44888","messageId":"20070613054136.GA27476@moooo.ath.cx","threadId":"8509","inReplyTo":"7vejkgh1jf.fsf@assigned-by-dhcp.pobox.com","subject":"Re: [PATCH] make git barf when an alias changes environment variables","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2007-06-13T05:41:36Z","receivedAt":"2007-06-13T05:41:36Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> What does this patch do when you do this?\n> \n> \t: Because I usually work with this repository...\n>       $ GIT_DIR=$some_repository\n>       $ export GIT_DIR\n> \n>       : Then much later, I happen to visit another repository\n>       : to take a peek...\n> \t$ cd $somewhere_else\n>       $ git --git-dir .git some-command\n> \n\nThis one has no alias involved as far as I see, so there is no change.\n\nThe patch makes git barf when an alias changes the environment (i.e.\nan alias uses --git-dir/--work-tree/--bare).  When git has read the\nconfiguration file to find out about aliases setup_git_env has been\ncalled and a pointer returned by getenv(\"GIT_DIR\") is already stored\nin a static pointer.  If the environment variable is changed\nafterwards git might or might not use the new value depending on the\nimplementation.\n\nThat is what my example shows: FreeBSD libc reuses the old memory\nlocation if the new value of an environment variable fits in the old\nplace.  Because setup_git_env uses xstrdup on the object dir and some\nother directories the old value is used for those but the new one is\nused for git_dir (which is just a pointer returned by getenv).  This\nway HEAD is read from the new repository but objects are searched in\nthe old repository.\n"}]}