Re: Git autocorrect bug

[Duy Nguyen <pclouds@gmail.com>]

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< --