threads / bug / 36839

Git autocorrect bug

Subject: Git autocorrect bug

## tl;dr

12 messages between Jun 5, 2014 and Jun 12, 2014.

replies: 11people: 4as markdown or json

David Turner· Jun 5, 2014, 03:49 UTC · lore
$ 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).

Øystein Walle· Jun 5, 2014, 06:06 UTC · re: David Turner · lore

Re: Git autocorrect bug

David Turner <dturner <at> twopensource.com> writes:
Show 14 quoted lines
> 
> $ 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
Øystein Walle· Jun 5, 2014, 06:10 UTC · re: David Turner · lore

Re: Git autocorrect bug

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?

Duy Nguyen· Jun 5, 2014, 06:29 UTC · re: David Turner · lore

Re: Git autocorrect bug

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
David Turner· Jun 5, 2014, 08:28 UTC · re: Duy Nguyen · lore

Re: Git autocorrect bug

On Thu, 2014-06-05 at 13:29 +0700, Duy Nguyen wrote:
Show 6 quoted lines
> 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)

Duy Nguyen· Jun 6, 2014, 11:09 UTC · re: David Turner · lore

Re: Git autocorrect bug

On Thu, Jun 05, 2014 at 04:28:23AM -0400, David Turner wrote:
Show 15 quoted lines
> 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< --
Nguyễn Thái Ngọc Duy· Jun 8, 2014, 09:37 UTC · re: Duy Nguyen · lore

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

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
Junio C Hamano· Jun 10, 2014, 18:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 15 quoted lines
> 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?
Show 30 quoted lines
>  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.
Duy Nguyen· Jun 11, 2014, 10:57 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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.

Show 44 quoted lines
>
>>  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
Junio C Hamano· Jun 12, 2014, 17:11 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 23 quoted lines
> 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.

Show 8 quoted lines
>> 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.

Show 6 quoted lines
> 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.

Show 10 quoted lines
>> 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.
Junio C Hamano· Jun 10, 2014, 18:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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])
Junio C Hamano· Jun 10, 2014, 19:09 UTC · re: Junio C Hamano · lore

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

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:
Show 19 quoted lines
> 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])

← back to recent threads