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

2015-12-07 Thread Junio C Hamano
Duy Nguyen  writes:

> On Thu, Dec 3, 2015 at 7:17 PM, Nguyễn Thái Ngọc Duy  
> 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.

Thanks for being careful.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-12-05 Thread Duy Nguyen
On Thu, Dec 3, 2015 at 7:17 PM, Nguyễn Thái Ngọc Duy  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-12-04 Thread Duy Nguyen
On Fri, Dec 4, 2015 at 9:35 PM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> ... Now I conclude
>> that setup-messed-by-alias is always evil. So the env restoration is
>> done for _all_ commands  whenever aliases are involved.
>
> So a side effect of this is whenever an alias is involved, all
> commands are re-spawned, not just the external ones but builtins as
> well.

I missed that while re-reading c056261, but yes that's true. So
Windows folks will be grumpy anyway. Maybe we can avoid that in
certain (safe) cases, when we know the second setup_git_... will be
executed by the builtin command and won't have any side effects, which
is probably the common case. But let's see how it goes.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-12-04 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy   writes:

> ... Now I conclude
> that setup-messed-by-alias is always evil. So the env restoration is
> done for _all_ commands  whenever aliases are involved.

So a side effect of this is whenever an alias is involved, all
commands are re-spawned, not just the external ones but builtins as
well.

>  This may be a brilliant fix, or another invitation for regressions.

;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-12-03 Thread Nguyễn Thái Ngọc Duy
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 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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-