Git autocorrect bug

Git autocorrect bug

[David Turner]

$ cd [some existing git repo]
$ git git foo
WARNING: You called a Git command named 'git', which does not exist.
Continuing under the assumption that you meant 'init'
in 0.1 seconds automatically...
fatal: internal error: work tree has already been set
Current worktree: /home/dturner/git
New worktree: /home/dturner/git/foo

(I am extremely unlikely to fix this bug myself, since it only arises in
very rare circumstances).

Re: Git autocorrect bug

[Øystein Walle]

David Turner <dturner <at> twopensource.com> writes:

> 
> $ cd [some existing git repo]
> $ git git foo
> WARNING: You called a Git command named 'git', which does not exist.
> Continuing under the assumption that you meant 'init'
> in 0.1 seconds automatically...
> fatal: internal error: work tree has already been set
> Current worktree: /home/dturner/git
> New worktree: /home/dturner/git/foo
> 
> (I am extremely unlikely to fix this bug myself, since it only arises in
> very rare circumstances).
> 
> 

You have help.autocorrect enabled. Then this is expected behaviour (it
doesn't seem out of place to me at least)

Running `git config --unset help.autocorrect` should disable it. It
may be that you enabled it for only one repo by accident.

Øsse

Re: Git autocorrect bug

[Øystein Walle]

David Turner <dturner <at> twopensource.com> writes:

> 
> (I am extremely unlikely to fix this bug myself, since it only arises in
> very rare circumstances).
>

I see now that `git init foo` and `git git foo` (with git corrected
to init) behave differently. Is this the bug you're referring to? 

Re: Git autocorrect bug

[Duy Nguyen]

On Thu, Jun 5, 2014 at 10:49 AM, David Turner <dturner@twopensource.com> wrote:
> fatal: internal error: work tree has already been set
> Current worktree: /home/dturner/git
> New worktree: /home/dturner/git/foo

This is the part you complain about, right? I think I might know
what's going on here. But do you expect "git git foo" to turn to "git
init foo" in the first place?
-- 
Duy

Re: Git autocorrect bug

[David Turner]

On Thu, 2014-06-05 at 13:29 +0700, Duy Nguyen wrote:
> On Thu, Jun 5, 2014 at 10:49 AM, David Turner <dturner@twopensource.com> wrote:
> > fatal: internal error: work tree has already been set
> > Current worktree: /home/dturner/git
> > New worktree: /home/dturner/git/foo
> 
> This is the part you complain about, right? 

Yes.

> I think I might know
> what's going on here. But do you expect "git git foo" to turn to "git
> init foo" in the first place?

Yes.  That is, I generally expect autocorrect to work without internal
errors.  This one was a genuine typo (I typed git, got distracted, and
typed it again).  

(I actually think "git git" should just be "git", and I know some people
have a command for that; I should set that up)

Re: Git autocorrect bug

[Duy Nguyen]

On Thu, Jun 05, 2014 at 04:28:23AM -0400, David Turner wrote:
> On Thu, 2014-06-05 at 13:29 +0700, Duy Nguyen wrote:
> > On Thu, Jun 5, 2014 at 10:49 AM, David Turner <dturner@twopensource.com> wrote:
> > > fatal: internal error: work tree has already been set
> > > Current worktree: /home/dturner/git
> > > New worktree: /home/dturner/git/foo
> > 
> > This is the part you complain about, right? 
> 
> Yes.
> 
> > I think I might know
> > what's going on here. But do you expect "git git foo" to turn to "git
> > init foo" in the first place?
> 
> Yes.

I was hoping you would say no so I could get away without doing
anything :) The problem is "setup pollution". When somebody looks up
an alias (autocorrect does), $GIT_DIR must be searched because
$GIT_DIR/config may have repo-local aliases. But 'git init' (and
clone) expects a clean no-setup state.

This is a known issue. You can reproduce by aliasing init to
something, then init a new repo using that alias. In fact Jonathan
wrote a few test to catch this. The solution is we start out fresh in
a new process. The fork/exec overhead should not matter because this
is interactive session.

I'm just wondering if I should remove the "only applicable to init and
clone" check in the patch because there's another companion problem:
if we find $GIT_DIR automatically, then $GIT_DIR/config points out
that work-tree must be moved, it'll get nasty because we already set
everything up for the auto-found worktree. But maybe I already solved
that, not sure..

-- 8< --
diff --git a/compat/mingw.c b/compat/mingw.c
index e9892f8..34722fe 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1080,19 +1080,6 @@ int mingw_kill(pid_t pid, int sig)
 	return -1;
 }
 
-static char **copy_environ(void)
-{
-	char **env;
-	int i = 0;
-	while (environ[i])
-		i++;
-	env = xmalloc((i+1)*sizeof(*env));
-	for (i = 0; environ[i]; i++)
-		env[i] = xstrdup(environ[i]);
-	env[i] = NULL;
-	return env;
-}
-
 void free_environ(char **env)
 {
 	int i;
diff --git a/git-compat-util.h b/git-compat-util.h
index 76910e6..1db4dec 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -732,4 +732,6 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define gmtime_r git_gmtime_r
 #endif
 
+char **copy_environ(void);
+
 #endif
diff --git a/git.c b/git.c
index 7780572..77d9204 100644
--- a/git.c
+++ b/git.c
@@ -20,6 +20,22 @@ const char git_more_info_string[] =
 
 static struct startup_info git_startup_info;
 static int use_pager = -1;
+static char orig_cwd[PATH_MAX];
+static char **orig_environ;
+
+static void save_env(void)
+{
+	getcwd(orig_cwd, sizeof(orig_cwd));
+	orig_environ = copy_environ();
+}
+
+static void restore_env(void)
+{
+	if (*orig_cwd && chdir(orig_cwd))
+		die_errno("could not move to %s", orig_cwd);
+	if (orig_environ)
+		environ = orig_environ;
+}
 
 static void commit_pager_choice(void) {
 	switch (use_pager) {
@@ -459,7 +475,7 @@ int is_builtin(const char *s)
 	return 0;
 }
 
-static void handle_builtin(int argc, const char **argv)
+static void handle_builtin(int argc, const char **argv, int preprocessed)
 {
 	const char *cmd = argv[0];
 	int i;
@@ -484,6 +500,11 @@ static void handle_builtin(int argc, const char **argv)
 		struct cmd_struct *p = commands+i;
 		if (strcmp(p->cmd, cmd))
 			continue;
+		if (preprocessed &&
+		    (p->fn == cmd_init_db || p->fn == cmd_clone)) {
+			restore_env();
+			break;
+		}
 		exit(run_builtin(p, argc, argv));
 	}
 }
@@ -524,13 +545,13 @@ static void execv_dashed_external(const char **argv)
 	strbuf_release(&cmd);
 }
 
-static int run_argv(int *argcp, const char ***argv)
+static int run_argv(int *argcp, const char ***argv, int done_help)
 {
 	int done_alias = 0;
 
 	while (1) {
 		/* See if it's a builtin */
-		handle_builtin(*argcp, *argv);
+		handle_builtin(*argcp, *argv, done_help || done_alias);
 
 		/* .. then try the external ones */
 		execv_dashed_external(*argv);
@@ -539,7 +560,10 @@ static int run_argv(int *argcp, const char ***argv)
 		 * of overriding "git log" with "git show" by having
 		 * alias.log = show
 		 */
-		if (done_alias || !handle_alias(argcp, argv))
+		if (done_alias)
+			break;
+		save_env();
+		if (!handle_alias(argcp, argv))
 			break;
 		done_alias = 1;
 	}
@@ -581,7 +605,7 @@ int main(int argc, char **av)
 	if (starts_with(cmd, "git-")) {
 		cmd += 4;
 		argv[0] = cmd;
-		handle_builtin(argc, argv);
+		handle_builtin(argc, argv, 0);
 		die("cannot handle %s as a builtin", cmd);
 	}
 
@@ -613,7 +637,7 @@ int main(int argc, char **av)
 	while (1) {
 		static int done_help = 0;
 		static int was_alias = 0;
-		was_alias = run_argv(&argc, &argv);
+		was_alias = run_argv(&argc, &argv, done_help);
 		if (errno != ENOENT)
 			break;
 		if (was_alias) {
@@ -623,6 +647,7 @@ int main(int argc, char **av)
 			exit(1);
 		}
 		if (!done_help) {
+			save_env();
 			cmd = argv[0] = help_unknown_cmd(cmd);
 			done_help = 1;
 		} else
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 2f30203..e62c0ff 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -56,7 +56,7 @@ test_expect_success 'plain through aliased command, outside any git repo' '
 	check_config plain-aliased/.git false unset
 '
 
-test_expect_failure 'plain nested through aliased command' '
+test_expect_success 'plain nested through aliased command' '
 	(
 		git init plain-ancestor-aliased &&
 		cd plain-ancestor-aliased &&
@@ -68,7 +68,7 @@ test_expect_failure 'plain nested through aliased command' '
 	check_config plain-ancestor-aliased/plain-nested/.git false unset
 '
 
-test_expect_failure 'plain nested in bare through aliased command' '
+test_expect_success 'plain nested in bare through aliased command' '
 	(
 		git init --bare bare-ancestor-aliased.git &&
 		cd bare-ancestor-aliased.git &&
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..8a82097 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -493,3 +493,16 @@ struct passwd *xgetpwuid_self(void)
 		    errno ? strerror(errno) : _("no such user"));
 	return pw;
 }
+
+char **copy_environ(void)
+{
+	char **env;
+	int i = 0;
+	while (environ[i])
+		i++;
+	env = xmalloc((i+1)*sizeof(*env));
+	for (i = 0; environ[i]; i++)
+		env[i] = xstrdup(environ[i]);
+	env[i] = NULL;
+	return env;
+}
-- 8< --

[PATCH] Fix "t0001: test git init when run via an alias"

[Nguyễn Thái Ngọc Duy]

Commit 4ad8332 (t0001: test git init when run via an alias -
2010-11-26) noted breakages when running init via alias. The problem
is for alias to be used, $GIT_DIR must be searched, but 'init' and
'clone' are not happy with that. So we start a new process like an
external command, with clean environment in this case. Env variables
that are set by command line (e.g. "git --git-dir=.. ") are kept.

This should also fix autocorrecting a command typo to "init" because
it's the same problem: aliases are read, then "init" is unhappy with
$GIT_DIR already set up because of that.

Reminded-by: David Turner <dturner@twopensource.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----
 t/t0001-init.sh |  4 ++--
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index 7780572..d1e49da 100644
--- a/git.c
+++ b/git.c
@@ -20,6 +20,42 @@ const char git_more_info_string[] =
 
 static struct startup_info git_startup_info;
 static int use_pager = -1;
+static char orig_cwd[PATH_MAX];
+static const char *env_names[] = {
+	GIT_DIR_ENVIRONMENT,
+	GIT_WORK_TREE_ENVIRONMENT,
+	GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
+	GIT_PREFIX_ENVIRONMENT
+};
+static char *orig_env[4];
+static int saved_environment;
+
+static void save_env(void)
+{
+	int i;
+	if (saved_environment)
+		return;
+	saved_environment = 1;
+	getcwd(orig_cwd, sizeof(orig_cwd));
+	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
+		orig_env[i] = getenv(env_names[i]);
+		if (orig_env[i])
+			orig_env[i] = xstrdup(orig_env[i]);
+	}
+}
+
+static void restore_env(void)
+{
+	int i;
+	if (*orig_cwd && chdir(orig_cwd))
+		die_errno("could not move to %s", orig_cwd);
+	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
+		if (orig_env[i])
+			setenv(env_names[i], orig_env[i], 1);
+		else
+			unsetenv(env_names[i]);
+	}
+}
 
 static void commit_pager_choice(void) {
 	switch (use_pager) {
@@ -272,6 +308,7 @@ static int handle_alias(int *argcp, const char ***argv)
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE		(1<<3)
+#define NO_SETUP		(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -352,7 +389,7 @@ static struct cmd_struct commands[] = {
 	{ "cherry", cmd_cherry, RUN_SETUP },
 	{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
 	{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
-	{ "clone", cmd_clone },
+	{ "clone", cmd_clone, NO_SETUP },
 	{ "column", cmd_column, RUN_SETUP_GENTLY },
 	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
@@ -378,8 +415,8 @@ static struct cmd_struct commands[] = {
 	{ "hash-object", cmd_hash_object },
 	{ "help", cmd_help },
 	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
-	{ "init", cmd_init_db },
-	{ "init-db", cmd_init_db },
+	{ "init", cmd_init_db, NO_SETUP },
+	{ "init-db", cmd_init_db, NO_SETUP },
 	{ "log", cmd_log, RUN_SETUP },
 	{ "ls-files", cmd_ls_files, RUN_SETUP },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
@@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)
 		struct cmd_struct *p = commands+i;
 		if (strcmp(p->cmd, cmd))
 			continue;
+		if (saved_environment && (p->option & NO_SETUP)) {
+			restore_env();
+			break;
+		}
 		exit(run_builtin(p, argc, argv));
 	}
 }
@@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)
 		 * of overriding "git log" with "git show" by having
 		 * alias.log = show
 		 */
-		if (done_alias || !handle_alias(argcp, argv))
+		if (done_alias)
+			break;
+		save_env();
+		if (!handle_alias(argcp, argv))
 			break;
 		done_alias = 1;
 	}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 2f30203..e62c0ff 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -56,7 +56,7 @@ test_expect_success 'plain through aliased command, outside any git repo' '
 	check_config plain-aliased/.git false unset
 '
 
-test_expect_failure 'plain nested through aliased command' '
+test_expect_success 'plain nested through aliased command' '
 	(
 		git init plain-ancestor-aliased &&
 		cd plain-ancestor-aliased &&
@@ -68,7 +68,7 @@ test_expect_failure 'plain nested through aliased command' '
 	check_config plain-ancestor-aliased/plain-nested/.git false unset
 '
 
-test_expect_failure 'plain nested in bare through aliased command' '
+test_expect_success 'plain nested in bare through aliased command' '
 	(
 		git init --bare bare-ancestor-aliased.git &&
 		cd bare-ancestor-aliased.git &&
-- 
1.9.1.346.ga2b5940

Re: [PATCH] Fix "t0001: test git init when run via an alias"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Commit 4ad8332 (t0001: test git init when run via an alias -
> 2010-11-26) noted breakages when running init via alias. The problem
> is for alias to be used, $GIT_DIR must be searched, but 'init' and
> 'clone' are not happy with that. So we start a new process like an
> external command, with clean environment in this case. Env variables
> that are set by command line (e.g. "git --git-dir=.. ") are kept.
>
> This should also fix autocorrecting a command typo to "init" because
> it's the same problem: aliases are read, then "init" is unhappy with
> $GIT_DIR already set up because of that.
>
> Reminded-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----

This goes far deeper than "Fix t0001", doesn't it?

>  t/t0001-init.sh |  4 ++--
>  2 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/git.c b/git.c
> index 7780572..d1e49da 100644
> --- a/git.c
> +++ b/git.c
> @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)
>  		struct cmd_struct *p = commands+i;
>  		if (strcmp(p->cmd, cmd))
>  			continue;
> +		if (saved_environment && (p->option & NO_SETUP)) {
> +			restore_env();
> +			break;
> +		}
>  		exit(run_builtin(p, argc, argv));
>  	}
>  }
> @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)
>  		 * of overriding "git log" with "git show" by having
>  		 * alias.log = show
>  		 */
> -		if (done_alias || !handle_alias(argcp, argv))
> +		if (done_alias)
> +			break;
> +		save_env();
> +		if (!handle_alias(argcp, argv))
>  			break;
>  		done_alias = 1;
>  	}

So the save-env kicks in only after we tried the builtins and the
externals and didn't find any, and before expanding the alias (which
has to look at the config, which means we need to do discovery and
may contaminate the environment and the globals), and then when we
retry with the expanded alias, we restore when we know the command
will misbehave if we didn't do so?

That does not sound so bad.  Even though I wonder if that "save and
then restore" sequence logically belongs around handle_alias(), you
would not have sufficient clue to let you cheat by not restoring the
environment for commands that you happen to know that they do not
care, so that may be a reasonable optimization.

Is it too brittle a solution to force people to mark problematic
subcommands with NO_SETUP, though?  What kind of change to a
subcommand that currently does not have to be marked with NO_SETUP
would make it necessary to mark it with NO_SETUP?

Thanks.

Re: [PATCH] Fix "t0001: test git init when run via an alias"

[Junio C Hamano]

I'd squash this in, though.

 git.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 6bb2043..9bfa8fb 100644
--- a/git.c
+++ b/git.c
@@ -36,7 +36,8 @@ static void save_env(void)
 	if (saved_environment)
 		return;
 	saved_environment = 1;
-	getcwd(orig_cwd, sizeof(orig_cwd));
+	if (getcwd(orig_cwd, sizeof(orig_cwd)))
+		die_errno("cannot getcwd");
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		orig_env[i] = getenv(env_names[i]);
 		if (orig_env[i])

Re: [PATCH] Fix "t0001: test git init when run via an alias"

[Junio C Hamano]

Oops; obviously the check must be for NULL-ness, i.e.

 if (!getcwd(...))
   die_errno(...);


On Tue, Jun 10, 2014 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I'd squash this in, though.
>
>  git.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git.c b/git.c
> index 6bb2043..9bfa8fb 100644
> --- a/git.c
> +++ b/git.c
> @@ -36,7 +36,8 @@ static void save_env(void)
>         if (saved_environment)
>                 return;
>         saved_environment = 1;
> -       getcwd(orig_cwd, sizeof(orig_cwd));
> +       if (getcwd(orig_cwd, sizeof(orig_cwd)))
> +               die_errno("cannot getcwd");
>         for (i = 0; i < ARRAY_SIZE(env_names); i++) {
>                 orig_env[i] = getenv(env_names[i]);
>                 if (orig_env[i])

Re: [PATCH] Fix "t0001: test git init when run via an alias"

[Duy Nguyen]

On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Commit 4ad8332 (t0001: test git init when run via an alias -
>> 2010-11-26) noted breakages when running init via alias. The problem
>> is for alias to be used, $GIT_DIR must be searched, but 'init' and
>> 'clone' are not happy with that. So we start a new process like an
>> external command, with clean environment in this case. Env variables
>> that are set by command line (e.g. "git --git-dir=.. ") are kept.
>>
>> This should also fix autocorrecting a command typo to "init" because
>> it's the same problem: aliases are read, then "init" is unhappy with
>> $GIT_DIR already set up because of that.
>>
>> Reminded-by: David Turner <dturner@twopensource.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----
>
> This goes far deeper than "Fix t0001", doesn't it?

I followed the way git-revert creates the subject line, thinking that
this fixes an old commit so do similarly. Probably better rephrase it.

>
>>  t/t0001-init.sh |  4 ++--
>>  2 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/git.c b/git.c
>> index 7780572..d1e49da 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)
>>               struct cmd_struct *p = commands+i;
>>               if (strcmp(p->cmd, cmd))
>>                       continue;
>> +             if (saved_environment && (p->option & NO_SETUP)) {
>> +                     restore_env();
>> +                     break;
>> +             }
>>               exit(run_builtin(p, argc, argv));
>>       }
>>  }
>> @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)
>>                * of overriding "git log" with "git show" by having
>>                * alias.log = show
>>                */
>> -             if (done_alias || !handle_alias(argcp, argv))
>> +             if (done_alias)
>> +                     break;
>> +             save_env();
>> +             if (!handle_alias(argcp, argv))
>>                       break;
>>               done_alias = 1;
>>       }
>
> So the save-env kicks in only after we tried the builtins and the
> externals and didn't find any, and before expanding the alias (which
> has to look at the config, which means we need to do discovery and
> may contaminate the environment and the globals), and then when we
> retry with the expanded alias, we restore when we know the command
> will misbehave if we didn't do so?
>
> That does not sound so bad.  Even though I wonder if that "save and
> then restore" sequence logically belongs around handle_alias(), you
> would not have sufficient clue to let you cheat by not restoring the
> environment for commands that you happen to know that they do not
> care, so that may be a reasonable optimization.

The save code definitely belongs to handle_alias(). I'm not so
confident about always restoring at the end of handle_alias(). The
restore procedure is just enough not to propagate wrong info to the
child process. For that purpose, guarding cwd and environm are enough.
If after we return from handle_alias() and we run the builtin command
anyway, that' may not be clean enough (e.g. static variables may be
already initialized..)

> Is it too brittle a solution to force people to mark problematic
> subcommands with NO_SETUP, though?  What kind of change to a
> subcommand that currently does not have to be marked with NO_SETUP
> would make it necessary to mark it with NO_SETUP?

If I had a clear answer here, I could have undone the setup effects
caused by handle_alias() and not resort to spawning a new process :)
So my answer is mostly trial and error. We have evidence that clone
and init do not work with contaminated environment. So we fix them and
wait for new bugs to show up.
-- 
Duy

Re: [PATCH] Fix "t0001: test git init when run via an alias"

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Commit 4ad8332 (t0001: test git init when run via an alias -
>>> 2010-11-26) noted breakages when running init via alias. The problem
>>> is for alias to be used, $GIT_DIR must be searched, but 'init' and
>>> 'clone' are not happy with that. So we start a new process like an
>>> external command, with clean environment in this case. Env variables
>>> that are set by command line (e.g. "git --git-dir=.. ") are kept.
>>>
>>> This should also fix autocorrecting a command typo to "init" because
>>> it's the same problem: aliases are read, then "init" is unhappy with
>>> $GIT_DIR already set up because of that.
>>>
>>> Reminded-by: David Turner <dturner@twopensource.com>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>
>> This goes far deeper than "Fix t0001", doesn't it?
>
> I followed the way git-revert creates the subject line, thinking that
> this fixes an old commit so do similarly. Probably better rephrase it.

Yeah, I didn't realize the whole thing was in dq and by that you
meant that you are referring to the commit.

>> That does not sound so bad.  Even though I wonder if that "save and
>> then restore" sequence logically belongs around handle_alias(), you
>> would not have sufficient clue to let you cheat by not restoring the
>> environment for commands that you happen to know that they do not
>> care, so that may be a reasonable optimization.
>
> The save code definitely belongs to handle_alias(). I'm not so
> confident about always restoring at the end of handle_alias().

Oh, I am not saying we should restore unconditionally.  I was just
reading your patch that does not have the restore at the end of
handle_alias and trying to rationalize it myself---that code does
not know (yet) if the command to be run wants to get the environment
restored.

> The
> restore procedure is just enough not to propagate wrong info to the
> child process. For that purpose, guarding cwd and environm are enough.
> If after we return from handle_alias() and we run the builtin command
> anyway, that' may not be clean enough (e.g. static variables may be
> already initialized..)

Very true.  The "contamination" by the discovery process has got
bad enough over time.

>> Is it too brittle a solution to force people to mark problematic
>> subcommands with NO_SETUP, though?  What kind of change to a
>> subcommand that currently does not have to be marked with NO_SETUP
>> would make it necessary to mark it with NO_SETUP?
>
> If I had a clear answer here, I could have undone the setup effects
> caused by handle_alias() and not resort to spawning a new process :)
> So my answer is mostly trial and error. We have evidence that clone
> and init do not work with contaminated environment. So we fix them and
> wait for new bugs to show up.

OK.