Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

10 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:07:21 

Duy Nguyen [off-list ref] writes:

... But let me sleep on it (and everybody please
give ideas if you have any). Meanwhile, maybe reverting d95138e should
be done any way for now. Broken aliases are not as bad as broken
hooks.
OK, when/if you decide that our first step should be a revert of
d95138e please send in a patch to do so with a brief write-up of a
follow-up plan.

Thanks.

[PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:22 

Commit d95138e [1] fixes a .git file problem by setting GIT_WORK_TREE
whenever GIT_DIR is set. It sounded harmless because we handle
GIT_DIR and GIT_WORK_TREE side by side for most commands, with two
exceptions: git-init and git-clone.

"git clone" is not happy with d95138e. This command ignores GIT_DIR
but respects GIT_WORK_TREE [2] [3] which means it used to run fine
from a hook, where GIT_DIR was set but GIT_WORK_TREE was not (*). With
d95138e, GIT_WORK_TREE is set all the time and git-clone interprets
that as "I give you order to put the worktree here", usually against
the user's intention.

The solution in d95138e is reverted in this commit. Instead we reuse the
solution from c056261 [4]. c056261 fixes another setup-messed-up-by-alias
by saving and restoring env for git-clone and git-init. Now I conclude
that setup-messed-by-alias is always evil. So the env restoration is
done for _all_ commands  whenever aliases are involved. It fixes what
d95138e tries to fix. And it does not upset git-clone-inside-hooks.

The test from d95138e remains to verify it's not broken by this. A new
test is added to make sure git-clone-inside-hooks remains happy.

In future, perhaps we should look up aliases in a separate process
(provided that Windows folks are happy with one extra process). It'll be
cleanest.

(*) GIT_WORK_TREE was not set _most of the time_. In some cases
    GIT_WORK_TREE is set and git-clone will behave differently. The
    use of GIT_WORK_TREE to direct git-clone to put work tree
    elsewhere looks like a mistake because it causes surprises this
    way. But that's a separate story.

[1] d95138e (setup: set env $GIT_WORK_TREE when work tree is set, like
             $GIT_DIR - 2015-06-26)
[2] 2beebd2 (clone: create intermediate directories of destination
             repo - 2008-06-25)
[3] 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06)
[4] c056261 (git potty: restore environments after alias expansion -
             2014-06-08)

Reported-by: Anthony Sottile <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 This may be a brilliant fix, or another invitation for regressions.

 environment.c    |  2 --
 git.c            |  9 ++++-----
 t/t5601-clone.sh | 23 +++++++++++++++++++++++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/environment.c b/environment.c
index 2da7fe2..1cc4aab 100644
--- a/environment.c
+++ b/environment.c
@@ -235,8 +235,6 @@ void set_git_work_tree(const char *new_work_tree)
 	}
 	git_work_tree_initialized = 1;
 	work_tree = xstrdup(real_path(new_work_tree));
-	if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
-		die("could not set GIT_WORK_TREE to '%s'", work_tree);
 }
 
 const char *get_git_work_tree(void)
diff --git a/git.c b/git.c
index 6ce7043..e2f187d 100644
--- a/git.c
+++ b/git.c
@@ -308,7 +308,6 @@ 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;
@@ -390,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, NO_SETUP },
+	{ "clone", cmd_clone },
 	{ "column", cmd_column, RUN_SETUP_GENTLY },
 	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
@@ -416,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, NO_SETUP },
-	{ "init-db", cmd_init_db, NO_SETUP },
+	{ "init", cmd_init_db },
+	{ "init-db", cmd_init_db },
 	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
 	{ "log", cmd_log, RUN_SETUP },
 	{ "ls-files", cmd_ls_files, RUN_SETUP },
@@ -531,7 +530,7 @@ static void handle_builtin(int argc, const char **argv)
 
 	builtin = get_builtin(cmd);
 	if (builtin) {
-		if (saved_env_before_alias && (builtin->option & NO_SETUP))
+		if (saved_env_before_alias)
 			restore_env();
 		else
 			exit(run_builtin(builtin, argc, argv));
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9b34f3c..31b4658 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -65,6 +65,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
+test_expect_success 'clone from hooks' '
+
+	test_create_repo r0 &&
+	cd r0 &&
+	test_commit initial &&
+	cd .. &&
+	git init r1 &&
+	cd r1 &&
+	cat >.git/hooks/pre-commit <<-\EOF &&
+	#!/bin/sh
+	git clone ../r0 ../r2
+	exit 1
+	EOF
+	chmod u+x .git/hooks/pre-commit &&
+	: >file &&
+	git add file &&
+	test_must_fail git commit -m invoke-hook &&
+	cd .. &&
+	test_cmp r0/.git/HEAD r2/.git/HEAD &&
+	test_cmp r0/initial.t r2/initial.t
+
+'
+
 test_expect_success 'clone creates intermediate directories' '
 
 	git clone src long/path/to/dst &&
-- 
2.2.0.513.g477eb31

[PATCH 1/2] git.c: make it clear save_env() is for alias handling only

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:22 

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 git.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index 6ed824c..6ce7043 100644
--- a/git.c
+++ b/git.c
@@ -25,14 +25,14 @@ static const char *env_names[] = {
 	GIT_PREFIX_ENVIRONMENT
 };
 static char *orig_env[4];
-static int saved_environment;
+static int saved_env_before_alias;
 
-static void save_env(void)
+static void save_env_before_alias(void)
 {
 	int i;
-	if (saved_environment)
+	if (saved_env_before_alias)
 		return;
-	saved_environment = 1;
+	saved_env_before_alias = 1;
 	orig_cwd = xgetcwd();
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		orig_env[i] = getenv(env_names[i]);
@@ -233,6 +233,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	char *alias_string;
 	int unused_nongit;
 
+	save_env_before_alias();
 	subdir = setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
@@ -530,7 +531,7 @@ static void handle_builtin(int argc, const char **argv)
 
 	builtin = get_builtin(cmd);
 	if (builtin) {
-		if (saved_environment && (builtin->option & NO_SETUP))
+		if (saved_env_before_alias && (builtin->option & NO_SETUP))
 			restore_env();
 		else
 			exit(run_builtin(builtin, argc, argv));
@@ -590,7 +591,6 @@ static int run_argv(int *argcp, const char ***argv)
 		 */
 		if (done_alias)
 			break;
-		save_env();
 		if (!handle_alias(argcp, argv))
 			break;
 		done_alias = 1;
-- 
2.2.0.513.g477eb31

[PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:23 

The unfortunate commit d95138e (setup: set env $GIT_WORK_TREE when
work tree is set, like $GIT_DIR - 2015-06-26) exposes another problem,
besides git-clone that's described in the previous commit. If
GIT_WORK_TREE (or even GIT_DIR) is exported to an alias script, it may
mislead git commands in the script where the repo is. Granted, most
scripts work on the repo where the alias is summoned from. But nowhere
do we forbid the script to visit another repository.

The revert of d95138e in the previous commit is sufficient as a
fix. However, to protect us from accidentally leaking GIT_*
environment variables again, we restore certain sensitive env before
calling the external script.

GIT_PREFIX is let through because there's another setup side effect
that we simply accepted so far: current working directory is
moved. Maybe in future we can introduce a new alias format that
guarantees no cwd move, then we can unexport GIT_PREFIX.

Reported-by: Gabriel Ganne <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Let's hope there will be no third report about this commit..

 git.c           | 10 +++++++---
 t/t0001-init.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index e2f187d..83b6c56 100644
--- a/git.c
+++ b/git.c
@@ -41,13 +41,16 @@ static void save_env_before_alias(void)
 	}
 }
 
-static void restore_env(void)
+static void restore_env(int external_alias)
 {
 	int i;
-	if (orig_cwd && chdir(orig_cwd))
+	if (!external_alias && orig_cwd && chdir(orig_cwd))
 		die_errno("could not move to %s", orig_cwd);
 	free(orig_cwd);
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
+		if (external_alias &&
+		    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
+			continue;
 		if (orig_env[i])
 			setenv(env_names[i], orig_env[i], 1);
 		else
@@ -244,6 +247,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			int argc = *argcp, i;
 
 			commit_pager_choice();
+			restore_env(1);
 
 			/* build alias_argv */
 			alias_argv = xmalloc(sizeof(*alias_argv) * (argc + 1));
@@ -531,7 +535,7 @@ static void handle_builtin(int argc, const char **argv)
 	builtin = get_builtin(cmd);
 	if (builtin) {
 		if (saved_env_before_alias)
-			restore_env();
+			restore_env(0);
 		else
 			exit(run_builtin(builtin, argc, argv));
 	}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index f91bbcf..19539fc 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -87,6 +87,33 @@ test_expect_success 'plain nested in bare through aliased command' '
 	check_config bare-ancestor-aliased.git/plain-nested/.git false unset
 '
 
+test_expect_success 'No extra GIT_* on alias scripts' '
+	cat <<-\EOF >expected &&
+	GIT_ATTR_NOSYSTEM
+	GIT_AUTHOR_EMAIL
+	GIT_AUTHOR_NAME
+	GIT_COMMITTER_EMAIL
+	GIT_COMMITTER_NAME
+	GIT_CONFIG_NOSYSTEM
+	GIT_EXEC_PATH
+	GIT_MERGE_AUTOEDIT
+	GIT_MERGE_VERBOSITY
+	GIT_PREFIX
+	GIT_TEMPLATE_DIR
+	GIT_TEXTDOMAINDIR
+	GIT_TRACE_BARE
+	EOF
+	cat <<-\EOF >script &&
+	#!/bin/sh
+	env | grep GIT_ | sed "s/=.*//" | sort >actual
+	exit 0
+	EOF
+	chmod 755 script &&
+	git config alias.script \!./script &&
+	( mkdir sub && cd sub && git script ) &&
+	test_cmp expected actual
+'
+
 test_expect_success 'plain with GIT_WORK_TREE' '
 	mkdir plain-wt &&
 	test_must_fail env GIT_WORK_TREE="$(pwd)/plain-wt" git init plain-wt
-- 
2.2.0.513.g477eb31

Re: [PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..

From: Duy Nguyen <hidden>
Date: 2016-06-15 23:07:23 

On Thu, Dec 3, 2015 at 7:17 PM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
The solution in d95138e is reverted in this commit. Instead we reuse the
solution from c056261 [4]. c056261 fixes another setup-messed-up-by-alias
by saving and restoring env for git-clone and git-init. Now I conclude
that setup-messed-by-alias is always evil. So the env restoration is
done for _all_ commands  whenever aliases are involved. It fixes what
d95138e tries to fix. And it does not upset git-clone-inside-hooks.
(Reviewer hat on) I wrote env restoration is done for all commands,
but the patch is actually about all _builtin_ commands. External ones
do not receive this treatment. FIx is in the next reroll.
-- 
Duy

[PATCH v2 1/3] git.c: make it clear save_env() is for alias handling only

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:31 

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 git.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index 6ed824c..6ce7043 100644
--- a/git.c
+++ b/git.c
@@ -25,14 +25,14 @@ static const char *env_names[] = {
 	GIT_PREFIX_ENVIRONMENT
 };
 static char *orig_env[4];
-static int saved_environment;
+static int saved_env_before_alias;
 
-static void save_env(void)
+static void save_env_before_alias(void)
 {
 	int i;
-	if (saved_environment)
+	if (saved_env_before_alias)
 		return;
-	saved_environment = 1;
+	saved_env_before_alias = 1;
 	orig_cwd = xgetcwd();
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		orig_env[i] = getenv(env_names[i]);
@@ -233,6 +233,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	char *alias_string;
 	int unused_nongit;
 
+	save_env_before_alias();
 	subdir = setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
@@ -530,7 +531,7 @@ static void handle_builtin(int argc, const char **argv)
 
 	builtin = get_builtin(cmd);
 	if (builtin) {
-		if (saved_environment && (builtin->option & NO_SETUP))
+		if (saved_env_before_alias && (builtin->option & NO_SETUP))
 			restore_env();
 		else
 			exit(run_builtin(builtin, argc, argv));
@@ -590,7 +591,6 @@ static int run_argv(int *argcp, const char ***argv)
 		 */
 		if (done_alias)
 			break;
-		save_env();
 		if (!handle_alias(argcp, argv))
 			break;
 		done_alias = 1;
-- 
2.3.0.rc1.137.g477eb31

[PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:31 

Changes since v1:

 - make sure we save/restore env for external commands in 2/3
 - fix t0001 test in 3/3

Interdiff:

diff --git b/git.c a/git.c
index 83b6c56..da278c3 100644
--- b/git.c
+++ a/git.c
@@ -229,7 +229,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 static int handle_alias(int *argcp, const char ***argv)
 {
 	int envchanged = 0, ret = 0, saved_errno = errno;
-	const char *subdir;
 	int count, option_count;
 	const char **new_argv;
 	const char *alias_command;
@@ -237,7 +236,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	int unused_nongit;
 
 	save_env_before_alias();
-	subdir = setup_git_directory_gently(&unused_nongit);
+	setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
@@ -296,8 +295,7 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	if (subdir && chdir(subdir))
-		die_errno("Cannot change to '%s'", subdir);
+	restore_env(0);
 
 	errno = saved_errno;
 
@@ -534,9 +532,13 @@ static void handle_builtin(int argc, const char **argv)
 
 	builtin = get_builtin(cmd);
 	if (builtin) {
-		if (saved_env_before_alias)
-			restore_env(0);
-		else
+		/*
+		 * XXX: if we can figure out cases where it is _safe_
+		 * to do, we can avoid spawning a new process when
+		 * saved_env_before_alias is true
+		 * (i.e. setup_git_dir* has been run once)
+		 */
+		if (!saved_env_before_alias)
 			exit(run_builtin(builtin, argc, argv));
 	}
 }
diff --git b/t/t0001-init.sh a/t/t0001-init.sh
index 19539fc..295aa59 100755
--- b/t/t0001-init.sh
+++ a/t/t0001-init.sh
@@ -88,24 +88,14 @@ test_expect_success 'plain nested in bare through aliased command' '
 '
 
 test_expect_success 'No extra GIT_* on alias scripts' '
-	cat <<-\EOF >expected &&
-	GIT_ATTR_NOSYSTEM
-	GIT_AUTHOR_EMAIL
-	GIT_AUTHOR_NAME
-	GIT_COMMITTER_EMAIL
-	GIT_COMMITTER_NAME
-	GIT_CONFIG_NOSYSTEM
-	GIT_EXEC_PATH
-	GIT_MERGE_AUTOEDIT
-	GIT_MERGE_VERBOSITY
-	GIT_PREFIX
-	GIT_TEMPLATE_DIR
-	GIT_TEXTDOMAINDIR
-	GIT_TRACE_BARE
-	EOF
+	(
+		env | sed -ne "/^GIT_/s/=.*//p" &&
+		echo GIT_PREFIX &&        # setup.c
+		echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh
+	) | sort | uniq >expected &&
 	cat <<-\EOF >script &&
 	#!/bin/sh
-	env | grep GIT_ | sed "s/=.*//" | sort >actual
+	env | sed -ne "/^GIT_/s/=.*//p" | sort >actual
 	exit 0
 	EOF
 	chmod 755 script &&

Nguyễn Thái Ngọc Duy (3):
  git.c: make it clear save_env() is for alias handling only
  setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..
  git.c: make sure we do not leak GIT_* to alias scripts

 environment.c    |  2 --
 git.c            | 41 +++++++++++++++++++++++------------------
 t/t0001-init.sh  | 17 +++++++++++++++++
 t/t5601-clone.sh | 23 +++++++++++++++++++++++
 4 files changed, 63 insertions(+), 20 deletions(-)

-- 
2.3.0.rc1.137.g477eb31

[PATCH v2 2/3] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:31 

Commit d95138e [1] fixes a .git file problem by setting GIT_WORK_TREE
whenever GIT_DIR is set. It sounded harmless because we handle
GIT_DIR and GIT_WORK_TREE side by side for most commands, with two
exceptions: git-init and git-clone.

"git clone" is not happy with d95138e. This command ignores GIT_DIR
but respects GIT_WORK_TREE [2] [3] which means it used to run fine
from a hook, where GIT_DIR was set but GIT_WORK_TREE was not (*). With
d95138e, GIT_WORK_TREE is set all the time and git-clone interprets
that as "I give you order to put the worktree here", usually against
the user's intention.

The solution in d95138e is reverted in this commit. Instead we reuse
the solution from c056261 [4]. c056261 fixes another setup-messed-
up-by-alias by saving and restoring env and spawning a new process,
but for git-clone and git-init only. Now I conclude that setup-messed-
up-by-alias is always evil. So the env restoration is done for _all_
commands, including external ones, whenever aliases are involved. It
fixes what d95138e tries to fix. And it does not upset git-clone-
inside-hooks.

The test from d95138e remains to verify it's not broken by this. A new
test is added to make sure git-clone-inside-hooks remains happy.

(*) GIT_WORK_TREE was not set _most of the time_. In some cases
    GIT_WORK_TREE is set and git-clone will behave differently. The
    use of GIT_WORK_TREE to direct git-clone to put work tree
    elsewhere looks like a mistake because it causes surprises this
    way. But that's a separate story.

[1] d95138e (setup: set env $GIT_WORK_TREE when work tree is set, like
             $GIT_DIR - 2015-06-26)
[2] 2beebd2 (clone: create intermediate directories of destination
             repo - 2008-06-25)
[3] 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06)
[4] c056261 (git potty: restore environments after alias expansion -
             2014-06-08)

Reported-by: Anthony Sottile <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 environment.c    |  2 --
 git.c            | 23 ++++++++++++-----------
 t/t5601-clone.sh | 23 +++++++++++++++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/environment.c b/environment.c
index 2da7fe2..1cc4aab 100644
--- a/environment.c
+++ b/environment.c
@@ -235,8 +235,6 @@ void set_git_work_tree(const char *new_work_tree)
 	}
 	git_work_tree_initialized = 1;
 	work_tree = xstrdup(real_path(new_work_tree));
-	if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
-		die("could not set GIT_WORK_TREE to '%s'", work_tree);
 }
 
 const char *get_git_work_tree(void)
diff --git a/git.c b/git.c
index 6ce7043..1a7d399 100644
--- a/git.c
+++ b/git.c
@@ -226,7 +226,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 static int handle_alias(int *argcp, const char ***argv)
 {
 	int envchanged = 0, ret = 0, saved_errno = errno;
-	const char *subdir;
 	int count, option_count;
 	const char **new_argv;
 	const char *alias_command;
@@ -234,7 +233,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	int unused_nongit;
 
 	save_env_before_alias();
-	subdir = setup_git_directory_gently(&unused_nongit);
+	setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
@@ -292,8 +291,7 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	if (subdir && chdir(subdir))
-		die_errno("Cannot change to '%s'", subdir);
+	restore_env();
 
 	errno = saved_errno;
 
@@ -308,7 +306,6 @@ 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;
@@ -390,7 +387,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, NO_SETUP },
+	{ "clone", cmd_clone },
 	{ "column", cmd_column, RUN_SETUP_GENTLY },
 	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
@@ -416,8 +413,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, NO_SETUP },
-	{ "init-db", cmd_init_db, NO_SETUP },
+	{ "init", cmd_init_db },
+	{ "init-db", cmd_init_db },
 	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
 	{ "log", cmd_log, RUN_SETUP },
 	{ "ls-files", cmd_ls_files, RUN_SETUP },
@@ -531,9 +528,13 @@ static void handle_builtin(int argc, const char **argv)
 
 	builtin = get_builtin(cmd);
 	if (builtin) {
-		if (saved_env_before_alias && (builtin->option & NO_SETUP))
-			restore_env();
-		else
+		/*
+		 * XXX: if we can figure out cases where it is _safe_
+		 * to do, we can avoid spawning a new process when
+		 * saved_env_before_alias is true
+		 * (i.e. setup_git_dir* has been run once)
+		 */
+		if (!saved_env_before_alias)
 			exit(run_builtin(builtin, argc, argv));
 	}
 }
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9b34f3c..31b4658 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -65,6 +65,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
+test_expect_success 'clone from hooks' '
+
+	test_create_repo r0 &&
+	cd r0 &&
+	test_commit initial &&
+	cd .. &&
+	git init r1 &&
+	cd r1 &&
+	cat >.git/hooks/pre-commit <<-\EOF &&
+	#!/bin/sh
+	git clone ../r0 ../r2
+	exit 1
+	EOF
+	chmod u+x .git/hooks/pre-commit &&
+	: >file &&
+	git add file &&
+	test_must_fail git commit -m invoke-hook &&
+	cd .. &&
+	test_cmp r0/.git/HEAD r2/.git/HEAD &&
+	test_cmp r0/initial.t r2/initial.t
+
+'
+
 test_expect_success 'clone creates intermediate directories' '
 
 	git clone src long/path/to/dst &&
-- 
2.3.0.rc1.137.g477eb31

[PATCH v2 3/3] git.c: make sure we do not leak GIT_* to alias scripts

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:31 

The unfortunate commit d95138e (setup: set env $GIT_WORK_TREE when
work tree is set, like $GIT_DIR - 2015-06-26) exposes another problem,
besides git-clone that's described in the previous commit. If
GIT_WORK_TREE (or even GIT_DIR) is exported to an alias script, it may
mislead git commands in the script where the repo is. Granted, most
scripts work on the repo where the alias is summoned from. But nowhere
do we forbid the script to visit another repository.

The revert of d95138e in the previous commit is sufficient as a
fix. However, to protect us from accidentally leaking GIT_*
environment variables again, we restore certain sensitive env before
calling the external script.

GIT_PREFIX is let through because there's another setup side effect
that we simply accepted so far: current working directory is
moved. Maybe in future we can introduce a new alias format that
guarantees no cwd move, then we can unexport GIT_PREFIX.

Reported-by: Gabriel Ganne <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 git.c           | 10 +++++++---
 t/t0001-init.sh | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 1a7d399..da278c3 100644
--- a/git.c
+++ b/git.c
@@ -41,13 +41,16 @@ static void save_env_before_alias(void)
 	}
 }
 
-static void restore_env(void)
+static void restore_env(int external_alias)
 {
 	int i;
-	if (orig_cwd && chdir(orig_cwd))
+	if (!external_alias && orig_cwd && chdir(orig_cwd))
 		die_errno("could not move to %s", orig_cwd);
 	free(orig_cwd);
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
+		if (external_alias &&
+		    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
+			continue;
 		if (orig_env[i])
 			setenv(env_names[i], orig_env[i], 1);
 		else
@@ -243,6 +246,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			int argc = *argcp, i;
 
 			commit_pager_choice();
+			restore_env(1);
 
 			/* build alias_argv */
 			alias_argv = xmalloc(sizeof(*alias_argv) * (argc + 1));
@@ -291,7 +295,7 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	restore_env();
+	restore_env(0);
 
 	errno = saved_errno;
 
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index f91bbcf..295aa59 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -87,6 +87,23 @@ test_expect_success 'plain nested in bare through aliased command' '
 	check_config bare-ancestor-aliased.git/plain-nested/.git false unset
 '
 
+test_expect_success 'No extra GIT_* on alias scripts' '
+	(
+		env | sed -ne "/^GIT_/s/=.*//p" &&
+		echo GIT_PREFIX &&        # setup.c
+		echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh
+	) | sort | uniq >expected &&
+	cat <<-\EOF >script &&
+	#!/bin/sh
+	env | sed -ne "/^GIT_/s/=.*//p" | sort >actual
+	exit 0
+	EOF
+	chmod 755 script &&
+	git config alias.script \!./script &&
+	( mkdir sub && cd sub && git script ) &&
+	test_cmp expected actual
+'
+
 test_expect_success 'plain with GIT_WORK_TREE' '
 	mkdir plain-wt &&
 	test_must_fail env GIT_WORK_TREE="$(pwd)/plain-wt" git init plain-wt
-- 
2.3.0.rc1.137.g477eb31

[PATCH] Revert "setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR"

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:07:31 

This commit has caused three regression reports so far. All of them are
about spawning git subprocesses, where the new presence of GIT_WORK_TREE
either changes command behaviour (git-init or git-clone), or how
repo/worktree is detected (from aliases), with or without $GIT_DIR.
The original bug will be re-fixed another way.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano [off-list ref] wrote:
 > OK, when/if you decide that our first step should be a revert of
 > d95138e please send in a patch to do so with a brief write-up of a
 > follow-up plan.

 Three reports to me are enough. And I obviously could not push the
 fix out fast enough. So if you want to revert it, here's the patch on
 maint.

 environment.c      | 2 --
 t/t0002-gitfile.sh | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/environment.c b/environment.c
index 23a38e4..f1a2a49 100644
--- a/environment.c
+++ b/environment.c
@@ -238,8 +238,6 @@ void set_git_work_tree(const char *new_work_tree)
 	}
 	git_work_tree_initialized = 1;
 	work_tree = xstrdup(real_path(new_work_tree));
-	if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
-		die("could not set GIT_WORK_TREE to '%s'", work_tree);
 }
 
 const char *get_git_work_tree(void)
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 9670e8c..3afe012 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -99,7 +99,7 @@ test_expect_success 'check rev-list' '
 	test "$SHA" = "$(git rev-list HEAD)"
 '
 
-test_expect_success 'setup_git_dir twice in subdir' '
+test_expect_failure 'setup_git_dir twice in subdir' '
 	git init sgd &&
 	(
 		cd sgd &&
-- 
2.3.0.rc1.137.g477eb31
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help