{"thread":{"id":"36839","subject":"Git autocorrect bug","startedAt":"2014-06-05T03:49:05Z","lastAt":"2014-06-12T17:11:33Z","messageCount":12,"participants":["David Turner","Øystein Walle","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"243362","messageId":"1401940145.18134.170.camel@stross","threadId":"36839","inReplyTo":null,"subject":"Git autocorrect bug","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-06-05T03:49:05Z","receivedAt":"2014-06-05T03:49:05Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"$ cd [some existing git repo]\n$ git git foo\nWARNING: You called a Git command named 'git', which does not exist.\nContinuing under the assumption that you meant 'init'\nin 0.1 seconds automatically...\nfatal: internal error: work tree has already been set\nCurrent worktree: /home/dturner/git\nNew worktree: /home/dturner/git/foo\n\n(I am extremely unlikely to fix this bug myself, since it only arises in\nvery rare circumstances).\n"},{"id":"243370","messageId":"loom.20140605T080345-353@post.gmane.org","threadId":"36839","inReplyTo":"1401940145.18134.170.camel@stross","subject":"Re: Git autocorrect bug","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-06-05T06:06:13Z","receivedAt":"2014-06-05T06:06:13Z","isPatch":false,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"David Turner <dturner <at> twopensource.com> writes:\n\n> \n> $ cd [some existing git repo]\n> $ git git foo\n> WARNING: You called a Git command named 'git', which does not exist.\n> Continuing under the assumption that you meant 'init'\n> in 0.1 seconds automatically...\n> fatal: internal error: work tree has already been set\n> Current worktree: /home/dturner/git\n> New worktree: /home/dturner/git/foo\n> \n> (I am extremely unlikely to fix this bug myself, since it only arises in\n> very rare circumstances).\n> \n> \n\nYou have help.autocorrect enabled. Then this is expected behaviour (it\ndoesn't seem out of place to me at least)\n\nRunning `git config --unset help.autocorrect` should disable it. It\nmay be that you enabled it for only one repo by accident.\n\nØsse\n"},{"id":"243376","messageId":"loom.20140605T080932-187@post.gmane.org","threadId":"36839","inReplyTo":"1401940145.18134.170.camel@stross","subject":"Re: Git autocorrect bug","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-06-05T06:10:12Z","receivedAt":"2014-06-05T06:10:12Z","isPatch":false,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"David Turner <dturner <at> twopensource.com> writes:\n\n> \n> (I am extremely unlikely to fix this bug myself, since it only arises in\n> very rare circumstances).\n>\n\nI see now that `git init foo` and `git git foo` (with git corrected\nto init) behave differently. Is this the bug you're referring to? \n"},{"id":"243377","messageId":"CACsJy8BSHAUiF_BR_Vi4_LOW0CSP-N09UpAg-UJvZJ1fvipejg@mail.gmail.com","threadId":"36839","inReplyTo":"1401940145.18134.170.camel@stross","subject":"Re: Git autocorrect bug","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-05T06:29:01Z","receivedAt":"2014-06-05T06:29:01Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 5, 2014 at 10:49 AM, David Turner <dturner@twopensource.com> wrote:\n> fatal: internal error: work tree has already been set\n> Current worktree: /home/dturner/git\n> New worktree: /home/dturner/git/foo\n\nThis is the part you complain about, right? I think I might know\nwhat's going on here. But do you expect \"git git foo\" to turn to \"git\ninit foo\" in the first place?\n-- \nDuy\n"},{"id":"243382","messageId":"1401956903.18134.173.camel@stross","threadId":"36839","inReplyTo":"CACsJy8BSHAUiF_BR_Vi4_LOW0CSP-N09UpAg-UJvZJ1fvipejg@mail.gmail.com","subject":"Re: Git autocorrect bug","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-06-05T08:28:23Z","receivedAt":"2014-06-05T08:28:23Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2014-06-05 at 13:29 +0700, Duy Nguyen wrote:\n> On Thu, Jun 5, 2014 at 10:49 AM, David Turner <dturner@twopensource.com> wrote:\n> > fatal: internal error: work tree has already been set\n> > Current worktree: /home/dturner/git\n> > New worktree: /home/dturner/git/foo\n> \n> This is the part you complain about, right? \n\nYes.\n\n> I think I might know\n> what's going on here. But do you expect \"git git foo\" to turn to \"git\n> init foo\" in the first place?\n\nYes.  That is, I generally expect autocorrect to work without internal\nerrors.  This one was a genuine typo (I typed git, got distracted, and\ntyped it again).  \n\n(I actually think \"git git\" should just be \"git\", and I know some people\nhave a command for that; I should set that up)\n"},{"id":"243454","messageId":"20140606110935.GA14446@lanh","threadId":"36839","inReplyTo":"1401956903.18134.173.camel@stross","subject":"Re: Git autocorrect bug","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-06T11:09:35Z","receivedAt":"2014-06-06T11:09:35Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 05, 2014 at 04:28:23AM -0400, David Turner wrote:\n> On Thu, 2014-06-05 at 13:29 +0700, Duy Nguyen wrote:\n> > On Thu, Jun 5, 2014 at 10:49 AM, David Turner <dturner@twopensource.com> wrote:\n> > > fatal: internal error: work tree has already been set\n> > > Current worktree: /home/dturner/git\n> > > New worktree: /home/dturner/git/foo\n> > \n> > This is the part you complain about, right? \n> \n> Yes.\n> \n> > I think I might know\n> > what's going on here. But do you expect \"git git foo\" to turn to \"git\n> > init foo\" in the first place?\n> \n> Yes.\n\nI was hoping you would say no so I could get away without doing\nanything :) The problem is \"setup pollution\". When somebody looks up\nan alias (autocorrect does), $GIT_DIR must be searched because\n$GIT_DIR/config may have repo-local aliases. But 'git init' (and\nclone) expects a clean no-setup state.\n\nThis is a known issue. You can reproduce by aliasing init to\nsomething, then init a new repo using that alias. In fact Jonathan\nwrote a few test to catch this. The solution is we start out fresh in\na new process. The fork/exec overhead should not matter because this\nis interactive session.\n\nI'm just wondering if I should remove the \"only applicable to init and\nclone\" check in the patch because there's another companion problem:\nif we find $GIT_DIR automatically, then $GIT_DIR/config points out\nthat work-tree must be moved, it'll get nasty because we already set\neverything up for the auto-found worktree. But maybe I already solved\nthat, not sure..\n\n-- 8< --\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e9892f8..34722fe 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1080,19 +1080,6 @@ int mingw_kill(pid_t pid, int sig)\n \treturn -1;\n }\n \n-static char **copy_environ(void)\n-{\n-\tchar **env;\n-\tint i = 0;\n-\twhile (environ[i])\n-\t\ti++;\n-\tenv = xmalloc((i+1)*sizeof(*env));\n-\tfor (i = 0; environ[i]; i++)\n-\t\tenv[i] = xstrdup(environ[i]);\n-\tenv[i] = NULL;\n-\treturn env;\n-}\n-\n void free_environ(char **env)\n {\n \tint i;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 76910e6..1db4dec 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -732,4 +732,6 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define gmtime_r git_gmtime_r\n #endif\n \n+char **copy_environ(void);\n+\n #endif\ndiff --git a/git.c b/git.c\nindex 7780572..77d9204 100644\n--- a/git.c\n+++ b/git.c\n@@ -20,6 +20,22 @@ const char git_more_info_string[] =\n \n static struct startup_info git_startup_info;\n static int use_pager = -1;\n+static char orig_cwd[PATH_MAX];\n+static char **orig_environ;\n+\n+static void save_env(void)\n+{\n+\tgetcwd(orig_cwd, sizeof(orig_cwd));\n+\torig_environ = copy_environ();\n+}\n+\n+static void restore_env(void)\n+{\n+\tif (*orig_cwd && chdir(orig_cwd))\n+\t\tdie_errno(\"could not move to %s\", orig_cwd);\n+\tif (orig_environ)\n+\t\tenviron = orig_environ;\n+}\n \n static void commit_pager_choice(void) {\n \tswitch (use_pager) {\n@@ -459,7 +475,7 @@ int is_builtin(const char *s)\n \treturn 0;\n }\n \n-static void handle_builtin(int argc, const char **argv)\n+static void handle_builtin(int argc, const char **argv, int preprocessed)\n {\n \tconst char *cmd = argv[0];\n \tint i;\n@@ -484,6 +500,11 @@ static void handle_builtin(int argc, const char **argv)\n \t\tstruct cmd_struct *p = commands+i;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n+\t\tif (preprocessed &&\n+\t\t    (p->fn == cmd_init_db || p->fn == cmd_clone)) {\n+\t\t\trestore_env();\n+\t\t\tbreak;\n+\t\t}\n \t\texit(run_builtin(p, argc, argv));\n \t}\n }\n@@ -524,13 +545,13 @@ static void execv_dashed_external(const char **argv)\n \tstrbuf_release(&cmd);\n }\n \n-static int run_argv(int *argcp, const char ***argv)\n+static int run_argv(int *argcp, const char ***argv, int done_help)\n {\n \tint done_alias = 0;\n \n \twhile (1) {\n \t\t/* See if it's a builtin */\n-\t\thandle_builtin(*argcp, *argv);\n+\t\thandle_builtin(*argcp, *argv, done_help || done_alias);\n \n \t\t/* .. then try the external ones */\n \t\texecv_dashed_external(*argv);\n@@ -539,7 +560,10 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t * of overriding \"git log\" with \"git show\" by having\n \t\t * alias.log = show\n \t\t */\n-\t\tif (done_alias || !handle_alias(argcp, argv))\n+\t\tif (done_alias)\n+\t\t\tbreak;\n+\t\tsave_env();\n+\t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n \t\tdone_alias = 1;\n \t}\n@@ -581,7 +605,7 @@ int main(int argc, char **av)\n \tif (starts_with(cmd, \"git-\")) {\n \t\tcmd += 4;\n \t\targv[0] = cmd;\n-\t\thandle_builtin(argc, argv);\n+\t\thandle_builtin(argc, argv, 0);\n \t\tdie(\"cannot handle %s as a builtin\", cmd);\n \t}\n \n@@ -613,7 +637,7 @@ int main(int argc, char **av)\n \twhile (1) {\n \t\tstatic int done_help = 0;\n \t\tstatic int was_alias = 0;\n-\t\twas_alias = run_argv(&argc, &argv);\n+\t\twas_alias = run_argv(&argc, &argv, done_help);\n \t\tif (errno != ENOENT)\n \t\t\tbreak;\n \t\tif (was_alias) {\n@@ -623,6 +647,7 @@ int main(int argc, char **av)\n \t\t\texit(1);\n \t\t}\n \t\tif (!done_help) {\n+\t\t\tsave_env();\n \t\t\tcmd = argv[0] = help_unknown_cmd(cmd);\n \t\t\tdone_help = 1;\n \t\t} else\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 2f30203..e62c0ff 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -56,7 +56,7 @@ test_expect_success 'plain through aliased command, outside any git repo' '\n \tcheck_config plain-aliased/.git false unset\n '\n \n-test_expect_failure 'plain nested through aliased command' '\n+test_expect_success 'plain nested through aliased command' '\n \t(\n \t\tgit init plain-ancestor-aliased &&\n \t\tcd plain-ancestor-aliased &&\n@@ -68,7 +68,7 @@ test_expect_failure 'plain nested through aliased command' '\n \tcheck_config plain-ancestor-aliased/plain-nested/.git false unset\n '\n \n-test_expect_failure 'plain nested in bare through aliased command' '\n+test_expect_success 'plain nested in bare through aliased command' '\n \t(\n \t\tgit init --bare bare-ancestor-aliased.git &&\n \t\tcd bare-ancestor-aliased.git &&\ndiff --git a/wrapper.c b/wrapper.c\nindex bc1bfb8..8a82097 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -493,3 +493,16 @@ struct passwd *xgetpwuid_self(void)\n \t\t    errno ? strerror(errno) : _(\"no such user\"));\n \treturn pw;\n }\n+\n+char **copy_environ(void)\n+{\n+\tchar **env;\n+\tint i = 0;\n+\twhile (environ[i])\n+\t\ti++;\n+\tenv = xmalloc((i+1)*sizeof(*env));\n+\tfor (i = 0; environ[i]; i++)\n+\t\tenv[i] = xstrdup(environ[i]);\n+\tenv[i] = NULL;\n+\treturn env;\n+}\n-- 8< --\n"},{"id":"243602","messageId":"1402220230-799-1-git-send-email-pclouds@gmail.com","threadId":"36839","inReplyTo":"20140606110935.GA14446@lanh","subject":"[PATCH] Fix \"t0001: test git init when run via an alias\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-08T09:37:10Z","receivedAt":"2014-06-08T09:37:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit 4ad8332 (t0001: test git init when run via an alias -\n2010-11-26) noted breakages when running init via alias. The problem\nis for alias to be used, $GIT_DIR must be searched, but 'init' and\n'clone' are not happy with that. So we start a new process like an\nexternal command, with clean environment in this case. Env variables\nthat are set by command line (e.g. \"git --git-dir=.. \") are kept.\n\nThis should also fix autocorrecting a command typo to \"init\" because\nit's the same problem: aliases are read, then \"init\" is unhappy with\n$GIT_DIR already set up because of that.\n\nReminded-by: David Turner <dturner@twopensource.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----\n t/t0001-init.sh |  4 ++--\n 2 files changed, 50 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 7780572..d1e49da 100644\n--- a/git.c\n+++ b/git.c\n@@ -20,6 +20,42 @@ const char git_more_info_string[] =\n \n static struct startup_info git_startup_info;\n static int use_pager = -1;\n+static char orig_cwd[PATH_MAX];\n+static const char *env_names[] = {\n+\tGIT_DIR_ENVIRONMENT,\n+\tGIT_WORK_TREE_ENVIRONMENT,\n+\tGIT_IMPLICIT_WORK_TREE_ENVIRONMENT,\n+\tGIT_PREFIX_ENVIRONMENT\n+};\n+static char *orig_env[4];\n+static int saved_environment;\n+\n+static void save_env(void)\n+{\n+\tint i;\n+\tif (saved_environment)\n+\t\treturn;\n+\tsaved_environment = 1;\n+\tgetcwd(orig_cwd, sizeof(orig_cwd));\n+\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n+\t\torig_env[i] = getenv(env_names[i]);\n+\t\tif (orig_env[i])\n+\t\t\torig_env[i] = xstrdup(orig_env[i]);\n+\t}\n+}\n+\n+static void restore_env(void)\n+{\n+\tint i;\n+\tif (*orig_cwd && chdir(orig_cwd))\n+\t\tdie_errno(\"could not move to %s\", orig_cwd);\n+\tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n+\t\tif (orig_env[i])\n+\t\t\tsetenv(env_names[i], orig_env[i], 1);\n+\t\telse\n+\t\t\tunsetenv(env_names[i]);\n+\t}\n+}\n \n static void commit_pager_choice(void) {\n \tswitch (use_pager) {\n@@ -272,6 +308,7 @@ static int handle_alias(int *argcp, const char ***argv)\n  * RUN_SETUP for reading from the configuration file.\n  */\n #define NEED_WORK_TREE\t\t(1<<3)\n+#define NO_SETUP\t\t(1<<4)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -352,7 +389,7 @@ static struct cmd_struct commands[] = {\n \t{ \"cherry\", cmd_cherry, RUN_SETUP },\n \t{ \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"clean\", cmd_clean, RUN_SETUP | NEED_WORK_TREE },\n-\t{ \"clone\", cmd_clone },\n+\t{ \"clone\", cmd_clone, NO_SETUP },\n \t{ \"column\", cmd_column, RUN_SETUP_GENTLY },\n \t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n@@ -378,8 +415,8 @@ static struct cmd_struct commands[] = {\n \t{ \"hash-object\", cmd_hash_object },\n \t{ \"help\", cmd_help },\n \t{ \"index-pack\", cmd_index_pack, RUN_SETUP_GENTLY },\n-\t{ \"init\", cmd_init_db },\n-\t{ \"init-db\", cmd_init_db },\n+\t{ \"init\", cmd_init_db, NO_SETUP },\n+\t{ \"init-db\", cmd_init_db, NO_SETUP },\n \t{ \"log\", cmd_log, RUN_SETUP },\n \t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n \t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n@@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)\n \t\tstruct cmd_struct *p = commands+i;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n+\t\tif (saved_environment && (p->option & NO_SETUP)) {\n+\t\t\trestore_env();\n+\t\t\tbreak;\n+\t\t}\n \t\texit(run_builtin(p, argc, argv));\n \t}\n }\n@@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t * of overriding \"git log\" with \"git show\" by having\n \t\t * alias.log = show\n \t\t */\n-\t\tif (done_alias || !handle_alias(argcp, argv))\n+\t\tif (done_alias)\n+\t\t\tbreak;\n+\t\tsave_env();\n+\t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n \t\tdone_alias = 1;\n \t}\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 2f30203..e62c0ff 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -56,7 +56,7 @@ test_expect_success 'plain through aliased command, outside any git repo' '\n \tcheck_config plain-aliased/.git false unset\n '\n \n-test_expect_failure 'plain nested through aliased command' '\n+test_expect_success 'plain nested through aliased command' '\n \t(\n \t\tgit init plain-ancestor-aliased &&\n \t\tcd plain-ancestor-aliased &&\n@@ -68,7 +68,7 @@ test_expect_failure 'plain nested through aliased command' '\n \tcheck_config plain-ancestor-aliased/plain-nested/.git false unset\n '\n \n-test_expect_failure 'plain nested in bare through aliased command' '\n+test_expect_success 'plain nested in bare through aliased command' '\n \t(\n \t\tgit init --bare bare-ancestor-aliased.git &&\n \t\tcd bare-ancestor-aliased.git &&\n-- \n1.9.1.346.ga2b5940\n"},{"id":"243753","messageId":"xmqq4mzsa9ej.fsf@gitster.dls.corp.google.com","threadId":"36839","inReplyTo":"1402220230-799-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Fix \"t0001: test git init when run via an alias\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T18:48:04Z","receivedAt":"2014-06-10T18:48:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Commit 4ad8332 (t0001: test git init when run via an alias -\n> 2010-11-26) noted breakages when running init via alias. The problem\n> is for alias to be used, $GIT_DIR must be searched, but 'init' and\n> 'clone' are not happy with that. So we start a new process like an\n> external command, with clean environment in this case. Env variables\n> that are set by command line (e.g. \"git --git-dir=.. \") are kept.\n>\n> This should also fix autocorrecting a command typo to \"init\" because\n> it's the same problem: aliases are read, then \"init\" is unhappy with\n> $GIT_DIR already set up because of that.\n>\n> Reminded-by: David Turner <dturner@twopensource.com>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----\n\nThis goes far deeper than \"Fix t0001\", doesn't it?\n\n>  t/t0001-init.sh |  4 ++--\n>  2 files changed, 50 insertions(+), 6 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 7780572..d1e49da 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)\n>  \t\tstruct cmd_struct *p = commands+i;\n>  \t\tif (strcmp(p->cmd, cmd))\n>  \t\t\tcontinue;\n> +\t\tif (saved_environment && (p->option & NO_SETUP)) {\n> +\t\t\trestore_env();\n> +\t\t\tbreak;\n> +\t\t}\n>  \t\texit(run_builtin(p, argc, argv));\n>  \t}\n>  }\n> @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)\n>  \t\t * of overriding \"git log\" with \"git show\" by having\n>  \t\t * alias.log = show\n>  \t\t */\n> -\t\tif (done_alias || !handle_alias(argcp, argv))\n> +\t\tif (done_alias)\n> +\t\t\tbreak;\n> +\t\tsave_env();\n> +\t\tif (!handle_alias(argcp, argv))\n>  \t\t\tbreak;\n>  \t\tdone_alias = 1;\n>  \t}\n\nSo the save-env kicks in only after we tried the builtins and the\nexternals and didn't find any, and before expanding the alias (which\nhas to look at the config, which means we need to do discovery and\nmay contaminate the environment and the globals), and then when we\nretry with the expanded alias, we restore when we know the command\nwill misbehave if we didn't do so?\n\nThat does not sound so bad.  Even though I wonder if that \"save and\nthen restore\" sequence logically belongs around handle_alias(), you\nwould not have sufficient clue to let you cheat by not restoring the\nenvironment for commands that you happen to know that they do not\ncare, so that may be a reasonable optimization.\n\nIs it too brittle a solution to force people to mark problematic\nsubcommands with NO_SETUP, though?  What kind of change to a\nsubcommand that currently does not have to be marked with NO_SETUP\nwould make it necessary to mark it with NO_SETUP?\n\nThanks.\n"},{"id":"243757","messageId":"xmqqzjhk8ukk.fsf@gitster.dls.corp.google.com","threadId":"36839","inReplyTo":"1402220230-799-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Fix \"t0001: test git init when run via an alias\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T18:53:47Z","receivedAt":"2014-06-10T18:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'd squash this in, though.\n\n git.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git.c b/git.c\nindex 6bb2043..9bfa8fb 100644\n--- a/git.c\n+++ b/git.c\n@@ -36,7 +36,8 @@ static void save_env(void)\n \tif (saved_environment)\n \t\treturn;\n \tsaved_environment = 1;\n-\tgetcwd(orig_cwd, sizeof(orig_cwd));\n+\tif (getcwd(orig_cwd, sizeof(orig_cwd)))\n+\t\tdie_errno(\"cannot getcwd\");\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n \t\tif (orig_env[i])\n"},{"id":"243759","messageId":"CAPc5daW-1SbppkQicew4a60pQv-cfGXTim8zPrUBxvJ7MVXQcw@mail.gmail.com","threadId":"36839","inReplyTo":"xmqqzjhk8ukk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Fix \"t0001: test git init when run via an alias\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T19:09:59Z","receivedAt":"2014-06-10T19:09:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oops; obviously the check must be for NULL-ness, i.e.\n\n if (!getcwd(...))\n   die_errno(...);\n\n\nOn Tue, Jun 10, 2014 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> I'd squash this in, though.\n>\n>  git.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/git.c b/git.c\n> index 6bb2043..9bfa8fb 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -36,7 +36,8 @@ static void save_env(void)\n>         if (saved_environment)\n>                 return;\n>         saved_environment = 1;\n> -       getcwd(orig_cwd, sizeof(orig_cwd));\n> +       if (getcwd(orig_cwd, sizeof(orig_cwd)))\n> +               die_errno(\"cannot getcwd\");\n>         for (i = 0; i < ARRAY_SIZE(env_names); i++) {\n>                 orig_env[i] = getenv(env_names[i]);\n>                 if (orig_env[i])\n"},{"id":"243901","messageId":"CACsJy8Dh9miVOGM0JLWXs6kzzEnum1ggnc2qm0gKaCwX+9iZYQ@mail.gmail.com","threadId":"36839","inReplyTo":"xmqq4mzsa9ej.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Fix \"t0001: test git init when run via an alias\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-11T10:57:50Z","receivedAt":"2014-06-11T10:57:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> Commit 4ad8332 (t0001: test git init when run via an alias -\n>> 2010-11-26) noted breakages when running init via alias. The problem\n>> is for alias to be used, $GIT_DIR must be searched, but 'init' and\n>> 'clone' are not happy with that. So we start a new process like an\n>> external command, with clean environment in this case. Env variables\n>> that are set by command line (e.g. \"git --git-dir=.. \") are kept.\n>>\n>> This should also fix autocorrecting a command typo to \"init\" because\n>> it's the same problem: aliases are read, then \"init\" is unhappy with\n>> $GIT_DIR already set up because of that.\n>>\n>> Reminded-by: David Turner <dturner@twopensource.com>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----\n>\n> This goes far deeper than \"Fix t0001\", doesn't it?\n\nI followed the way git-revert creates the subject line, thinking that\nthis fixes an old commit so do similarly. Probably better rephrase it.\n\n>\n>>  t/t0001-init.sh |  4 ++--\n>>  2 files changed, 50 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/git.c b/git.c\n>> index 7780572..d1e49da 100644\n>> --- a/git.c\n>> +++ b/git.c\n>> @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)\n>>               struct cmd_struct *p = commands+i;\n>>               if (strcmp(p->cmd, cmd))\n>>                       continue;\n>> +             if (saved_environment && (p->option & NO_SETUP)) {\n>> +                     restore_env();\n>> +                     break;\n>> +             }\n>>               exit(run_builtin(p, argc, argv));\n>>       }\n>>  }\n>> @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)\n>>                * of overriding \"git log\" with \"git show\" by having\n>>                * alias.log = show\n>>                */\n>> -             if (done_alias || !handle_alias(argcp, argv))\n>> +             if (done_alias)\n>> +                     break;\n>> +             save_env();\n>> +             if (!handle_alias(argcp, argv))\n>>                       break;\n>>               done_alias = 1;\n>>       }\n>\n> So the save-env kicks in only after we tried the builtins and the\n> externals and didn't find any, and before expanding the alias (which\n> has to look at the config, which means we need to do discovery and\n> may contaminate the environment and the globals), and then when we\n> retry with the expanded alias, we restore when we know the command\n> will misbehave if we didn't do so?\n>\n> That does not sound so bad.  Even though I wonder if that \"save and\n> then restore\" sequence logically belongs around handle_alias(), you\n> would not have sufficient clue to let you cheat by not restoring the\n> environment for commands that you happen to know that they do not\n> care, so that may be a reasonable optimization.\n\nThe save code definitely belongs to handle_alias(). I'm not so\nconfident about always restoring at the end of handle_alias(). The\nrestore procedure is just enough not to propagate wrong info to the\nchild process. For that purpose, guarding cwd and environm are enough.\nIf after we return from handle_alias() and we run the builtin command\nanyway, that' may not be clean enough (e.g. static variables may be\nalready initialized..)\n\n> Is it too brittle a solution to force people to mark problematic\n> subcommands with NO_SETUP, though?  What kind of change to a\n> subcommand that currently does not have to be marked with NO_SETUP\n> would make it necessary to mark it with NO_SETUP?\n\nIf I had a clear answer here, I could have undone the setup effects\ncaused by handle_alias() and not resort to spawning a new process :)\nSo my answer is mostly trial and error. We have evidence that clone\nand init do not work with contaminated environment. So we fix them and\nwait for new bugs to show up.\n-- \nDuy\n"},{"id":"243953","messageId":"xmqqr42u6oje.fsf@gitster.dls.corp.google.com","threadId":"36839","inReplyTo":"CACsJy8Dh9miVOGM0JLWXs6kzzEnum1ggnc2qm0gKaCwX+9iZYQ@mail.gmail.com","subject":"Re: [PATCH] Fix \"t0001: test git init when run via an alias\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-12T17:11:33Z","receivedAt":"2014-06-12T17:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>>\n>>> Commit 4ad8332 (t0001: test git init when run via an alias -\n>>> 2010-11-26) noted breakages when running init via alias. The problem\n>>> is for alias to be used, $GIT_DIR must be searched, but 'init' and\n>>> 'clone' are not happy with that. So we start a new process like an\n>>> external command, with clean environment in this case. Env variables\n>>> that are set by command line (e.g. \"git --git-dir=.. \") are kept.\n>>>\n>>> This should also fix autocorrecting a command typo to \"init\" because\n>>> it's the same problem: aliases are read, then \"init\" is unhappy with\n>>> $GIT_DIR already set up because of that.\n>>>\n>>> Reminded-by: David Turner <dturner@twopensource.com>\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>>> ---\n>>>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----\n>>\n>> This goes far deeper than \"Fix t0001\", doesn't it?\n>\n> I followed the way git-revert creates the subject line, thinking that\n> this fixes an old commit so do similarly. Probably better rephrase it.\n\nYeah, I didn't realize the whole thing was in dq and by that you\nmeant that you are referring to the commit.\n\n>> That does not sound so bad.  Even though I wonder if that \"save and\n>> then restore\" sequence logically belongs around handle_alias(), you\n>> would not have sufficient clue to let you cheat by not restoring the\n>> environment for commands that you happen to know that they do not\n>> care, so that may be a reasonable optimization.\n>\n> The save code definitely belongs to handle_alias(). I'm not so\n> confident about always restoring at the end of handle_alias().\n\nOh, I am not saying we should restore unconditionally.  I was just\nreading your patch that does not have the restore at the end of\nhandle_alias and trying to rationalize it myself---that code does\nnot know (yet) if the command to be run wants to get the environment\nrestored.\n\n> The\n> restore procedure is just enough not to propagate wrong info to the\n> child process. For that purpose, guarding cwd and environm are enough.\n> If after we return from handle_alias() and we run the builtin command\n> anyway, that' may not be clean enough (e.g. static variables may be\n> already initialized..)\n\nVery true.  The \"contamination\" by the discovery process has got\nbad enough over time.\n\n>> Is it too brittle a solution to force people to mark problematic\n>> subcommands with NO_SETUP, though?  What kind of change to a\n>> subcommand that currently does not have to be marked with NO_SETUP\n>> would make it necessary to mark it with NO_SETUP?\n>\n> If I had a clear answer here, I could have undone the setup effects\n> caused by handle_alias() and not resort to spawning a new process :)\n> So my answer is mostly trial and error. We have evidence that clone\n> and init do not work with contaminated environment. So we fix them and\n> wait for new bugs to show up.\n\nOK.\n"}]}