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

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

[Anthony Sottile]

* Short description of the problem *

It seems GIT_WORK_DIR is now exported invariantly when calling git
hooks such as pre-commit.  If these hooks involve cloning repositories
they will not fail due to this exported environment variable.  This
was not the case in prior versions (such as v2.5.0).

* Simple reproduction *

```
$ cat test.sh
#!/usr/bin/env bash
set -ex

rm -rf test

# Exit non {0, 1} to abort git bisect
make -j 8 > /dev/null || exit 2

# Put our new git on the path
PATH="$(pwd):$PATH"

git init test

pushd test
mkdir -p .git/hooks
echo 'git clone git://github.com/asottile/css-explore css-explore' >
.git/hooks/pre-commit
chmod 755 .git/hooks/pre-commit

git commit -m foo --allow-empty || exit 1
```

* Under 2.6.3 *

```
$ ./test.sh

...

+ git init test
warning: templates not found /home/anthony/share/git-core/templates
Initialized empty Git repository in /home/anthony/workspace/git/test/.git/
+ pushd test
~/workspace/git/test ~/workspace/git
+ mkdir -p .git/hooks
+ echo 'git clone git://github.com/asottile/css-explore css-explore'
+ chmod 755 .git/hooks/pre-commit
+ git commit -m foo --allow-empty
fatal: working tree '.' already exists.
+ exit 1
```

* Under 2.5 *

```
$ ./test.sh

...

+ git init test
warning: templates not found /home/anthony/share/git-core/templates
Initialized empty Git repository in /home/anthony/workspace/git/test/.git/
+ pushd test
~/workspace/git/test ~/workspace/git
+ mkdir -p .git/hooks
+ echo 'git clone git://github.com/asottile/css-explore css-explore'
+ chmod 755 .git/hooks/pre-commit
+ git commit -m foo --allow-empty
Cloning into 'css-explore'...
warning: templates not found /home/anthony/share/git-core/templates
remote: Counting objects: 214, done.
remote: Total 214 (delta 0), reused 0 (delta 0), pack-reused 214
Receiving objects: 100% (214/214), 25.89 KiB | 0 bytes/s, done.
Resolving deltas: 100% (129/129), done.
Checking connectivity... done.
[master (root-commit) 5eb999d] foo
```


* Bisect *

```
$ git bisect good v2.5.0
$ git bisect bad origin/master
$ git bisect run ./test.sh

...

d95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit
commit d95138e695d99d32dcad528a2a7974f434c51e79
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date:   Fri Jun 26 17:37:35 2015 +0700

    setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR

    In the test case, we run setup_git_dir_gently() the first time to read
    $GIT_DIR/config so that we can resolve aliases. We'll enter
    setup_discovered_git_dir() and may or may not call set_git_dir() near
    the end of the function, depending on whether the detected git dir is
    ".git" or not. This set_git_dir() will set env var $GIT_DIR.

    For normal repo, git dir detected via setup_discovered_git_dir() will be
    ".git", and set_git_dir() is not called. If .git file is used however,
    the git dir can't be ".git" and set_git_dir() is called and $GIT_DIR
    set. This is the key of this problem.

    If we expand an alias (or autocorrect command names), then
    setup_git_dir_gently() is run the second time. If $GIT_DIR is not set in
    the first run, we run the same setup_discovered_git_dir() as before.
    Nothing to see. If it is, however, we'll enter setup_explicit_git_dir()
    this time.

    This is where the "fun" is.  If $GIT_WORK_TREE is not set but
    $GIT_DIR is, you are supposed to be at the root level of the
    worktree.  But if you are in a subdir "foo/bar" (real worktree's top
    is "foo"), this rule bites you: your detected worktree is now
    "foo/bar", even though the first run correctly detected worktree as
    "foo". You get "internal error: work tree has already been set" as a
    result.

    Bottom line is, when $GIT_DIR is set, $GIT_WORK_TREE should be set too
    unless there's no work tree. But setting $GIT_WORK_TREE inside
    set_git_dir() may backfire. We don't know at that point if work tree is
    already configured by the caller. So set it when work tree is
    detected. It does not harm if $GIT_WORK_TREE is set while $GIT_DIR is
    not.

    Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com>
    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

:100644 100644 9daa0ba4a36ced9f63541203e7bcc2ab9e1eae56
36fbba57fc83afd36d99bf5d4f3a1fc3feefba09 M    environment.c
:040000 040000 1d7c4bf77e0fd49ca315271993cb69a8b055c3aa
145d85895cb6cb0810597e1854a7721ccfc8f457 M    t
bisect run success
```

Causing me a few headaches in
https://github.com/pre-commit/pre-commit/issues/300
I'm working around it in https://github.com/pre-commit/pre-commit/pull/301

Thanks,

Anthony

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

[Stefan Beller]

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

On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile <asottile@umich.edu> wrote:
> * Short description of the problem *
>
> It seems GIT_WORK_DIR is now exported invariantly when calling git
> hooks such as pre-commit.  If these hooks involve cloning repositories
> they will not fail due to this exported environment variable.  This
> was not the case in prior versions (such as v2.5.0).
>
> * Simple reproduction *
>
> ```
> $ cat test.sh
> #!/usr/bin/env bash
> set -ex
>
> rm -rf test
>
> # Exit non {0, 1} to abort git bisect
> make -j 8 > /dev/null || exit 2
>
> # Put our new git on the path
> PATH="$(pwd):$PATH"
>
> git init test
>
> pushd test
> mkdir -p .git/hooks
> echo 'git clone git://github.com/asottile/css-explore css-explore' >
> .git/hooks/pre-commit
> chmod 755 .git/hooks/pre-commit
>
> git commit -m foo --allow-empty || exit 1
> ```
>
> * Under 2.6.3 *
>
> ```
> $ ./test.sh
>
> ...
>
> + git init test
> warning: templates not found /home/anthony/share/git-core/templates
> Initialized empty Git repository in /home/anthony/workspace/git/test/.git/
> + pushd test
> ~/workspace/git/test ~/workspace/git
> + mkdir -p .git/hooks
> + echo 'git clone git://github.com/asottile/css-explore css-explore'
> + chmod 755 .git/hooks/pre-commit
> + git commit -m foo --allow-empty
> fatal: working tree '.' already exists.
> + exit 1
> ```
>
> * Under 2.5 *
>
> ```
> $ ./test.sh
>
> ...
>
> + git init test
> warning: templates not found /home/anthony/share/git-core/templates
> Initialized empty Git repository in /home/anthony/workspace/git/test/.git/
> + pushd test
> ~/workspace/git/test ~/workspace/git
> + mkdir -p .git/hooks
> + echo 'git clone git://github.com/asottile/css-explore css-explore'
> + chmod 755 .git/hooks/pre-commit
> + git commit -m foo --allow-empty
> Cloning into 'css-explore'...
> warning: templates not found /home/anthony/share/git-core/templates
> remote: Counting objects: 214, done.
> remote: Total 214 (delta 0), reused 0 (delta 0), pack-reused 214
> Receiving objects: 100% (214/214), 25.89 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (129/129), done.
> Checking connectivity... done.
> [master (root-commit) 5eb999d] foo
> ```
>
>
> * Bisect *
>
> ```
> $ git bisect good v2.5.0
> $ git bisect bad origin/master
> $ git bisect run ./test.sh
>
> ...
>
> d95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit
> commit d95138e695d99d32dcad528a2a7974f434c51e79
> Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Date:   Fri Jun 26 17:37:35 2015 +0700
>
>     setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR
>
>     In the test case, we run setup_git_dir_gently() the first time to read
>     $GIT_DIR/config so that we can resolve aliases. We'll enter
>     setup_discovered_git_dir() and may or may not call set_git_dir() near
>     the end of the function, depending on whether the detected git dir is
>     ".git" or not. This set_git_dir() will set env var $GIT_DIR.
>
>     For normal repo, git dir detected via setup_discovered_git_dir() will be
>     ".git", and set_git_dir() is not called. If .git file is used however,
>     the git dir can't be ".git" and set_git_dir() is called and $GIT_DIR
>     set. This is the key of this problem.
>
>     If we expand an alias (or autocorrect command names), then
>     setup_git_dir_gently() is run the second time. If $GIT_DIR is not set in
>     the first run, we run the same setup_discovered_git_dir() as before.
>     Nothing to see. If it is, however, we'll enter setup_explicit_git_dir()
>     this time.
>
>     This is where the "fun" is.  If $GIT_WORK_TREE is not set but
>     $GIT_DIR is, you are supposed to be at the root level of the
>     worktree.  But if you are in a subdir "foo/bar" (real worktree's top
>     is "foo"), this rule bites you: your detected worktree is now
>     "foo/bar", even though the first run correctly detected worktree as
>     "foo". You get "internal error: work tree has already been set" as a
>     result.
>
>     Bottom line is, when $GIT_DIR is set, $GIT_WORK_TREE should be set too
>     unless there's no work tree. But setting $GIT_WORK_TREE inside
>     set_git_dir() may backfire. We don't know at that point if work tree is
>     already configured by the caller. So set it when work tree is
>     detected. It does not harm if $GIT_WORK_TREE is set while $GIT_DIR is
>     not.
>
>     Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com>
>     Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> :100644 100644 9daa0ba4a36ced9f63541203e7bcc2ab9e1eae56
> 36fbba57fc83afd36d99bf5d4f3a1fc3feefba09 M    environment.c
> :040000 040000 1d7c4bf77e0fd49ca315271993cb69a8b055c3aa
> 145d85895cb6cb0810597e1854a7721ccfc8f457 M    t
> bisect run success
> ```
>
> Causing me a few headaches in
> https://github.com/pre-commit/pre-commit/issues/300
> I'm working around it in https://github.com/pre-commit/pre-commit/pull/301
>
> Thanks,
>
> Anthony
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Duy Nguyen]

On Tue, Nov 24, 2015 at 6:57 PM, Stefan Beller <sbeller@google.com> wrote:
> +to Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile <asottile@umich.edu> wrote:
>> * Short description of the problem *
>>
>> It seems GIT_WORK_DIR is now exported invariantly when calling git
>> hooks such as pre-commit.  If these hooks involve cloning repositories
>> they will not fail due to this exported environment variable.  This
>> was not the case in prior versions (such as v2.5.0).

I'm getting good at fixing one bug and adding ten more. I don't think
the cited commit is the problem. It just exposes another bug. I did

> ~/w/git $ GIT_WORK_TREE=abc ./git clone .git /tmp/def

and what I got was really surprising, /tmp/def contains the git
repository while the true worktree is in "abc". It does not make
sense, at least from the first sight, unless it inherits this from
git-init, where we do(?)  want GIT_WORK_TREE to specify a separate
worktree. No time to dig to the bottom yet..
-- 
Duy

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

[Duy Nguyen]

On Wed, Nov 25, 2015 at 9:13 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Nov 24, 2015 at 6:57 PM, Stefan Beller <sbeller@google.com> wrote:
>> +to Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile <asottile@umich.edu> wrote:
>>> * Short description of the problem *
>>>
>>> It seems GIT_WORK_DIR is now exported invariantly when calling git
>>> hooks such as pre-commit.  If these hooks involve cloning repositories
>>> they will not fail due to this exported environment variable.  This
>>> was not the case in prior versions (such as v2.5.0).
>
> I'm getting good at fixing one bug and adding ten more. I don't think
> the cited commit is the problem. It just exposes another bug. I did
>
>> ~/w/git $ GIT_WORK_TREE=abc ./git clone .git /tmp/def
>
> and what I got was really surprising, /tmp/def contains the git
> repository while the true worktree is in "abc". It does not make
> sense, at least from the first sight, unless it inherits this from
> git-init, where we do(?)  want GIT_WORK_TREE to specify a separate
> worktree. No time to dig to the bottom yet..

I was wrong, GIT_WORK_TREE support was added in git-clone many years
ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So
my change accidentally triggers an (undocumented) feature. We could
add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't
think people will like it. I don't really like reverting d95138e
(setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -
2015-06-26) because another bug reappears. So I'm out of options..
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> I was wrong, GIT_WORK_TREE support was added in git-clone many years
> ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So
> my change accidentally triggers an (undocumented) feature. We could
> add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't
> think people will like it. I don't really like reverting d95138e
> (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -
> 2015-06-26) because another bug reappears.

> So I'm out of options..

Perhaps d95138e can be reverted and then the bug it tried to fix can
be fixed in a different way somehow?

(I am not quite up to speed yet, so the above may turn out to be
infeasible--take it with a large grain of salt please).

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

[Duy Nguyen]

On Mon, Nov 30, 2015 at 9:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> I was wrong, GIT_WORK_TREE support was added in git-clone many years
>> ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So
>> my change accidentally triggers an (undocumented) feature. We could
>> add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't
>> think people will like it. I don't really like reverting d95138e
>> (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -
>> 2015-06-26) because another bug reappears.
>
>> So I'm out of options..
>
> Perhaps d95138e can be reverted and then the bug it tried to fix can
> be fixed in a different way somehow?
>
> (I am not quite up to speed yet, so the above may turn out to be
> infeasible--take it with a large grain of salt please).

That would mean we do not set $GIT_DIR too early. While it sounds
good, it could be just another trap, and could be a lot of
reorganizing setup code. I'm more tempted to revert 20ccef4, with
deprecation warning for some releases, and a new git-clone option for
the same functionality. 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.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> 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 1/2] git.c: make it clear save_env() is for alias handling only

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..

[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 <asottile@umich.edu>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.

;-)

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

[Duy Nguyen]

On Fri, Dec 4, 2015 at 9:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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

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

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

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 <gabriel.ganne@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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 3/2] git.c: make sure we do not leak GIT_* to alias scripts

[Junio C Hamano]

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

>  Let's hope there will be no third report about this commit..

Hmm, why does this additional test fail only under prove but pass
without it?
> 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

This is more about coding discipline than style, but piping grep
output to sed is wasteful.  "sed -ne '/^GIT_/s/=.*//p'" or something
like that, perhaps?

I wondered what happens if the user has an unrelated stray variable
whose name happens to begin with GIT_ in her environment, but it
turns out that we cleanse them in test-lib.sh fairly early, so that
would be fine.  You need to tighten your "grep" pattern, though.

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

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

[Duy Nguyen]

On Mon, Dec 7, 2015 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>>  Let's hope there will be no third report about this commit..
>
> Hmm, why does this additional test fail only under prove but pass
> without it?

It passes with prove for me. Some mysterious variable leaks through somehow?

>> +     env | grep GIT_ | sed "s/=.*//" | sort >actual
>
> This is more about coding discipline than style, but piping grep
> output to sed is wasteful.  "sed -ne '/^GIT_/s/=.*//p'" or something
> like that, perhaps?

OK will fix.

> I wondered what happens if the user has an unrelated stray variable
> whose name happens to begin with GIT_ in her environment, but it
> turns out that we cleanse them in test-lib.sh fairly early, so that
> would be fine.  You need to tighten your "grep" pattern, though.

OK
-- 
Duy

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

[Jeff King]

On Tue, Dec 08, 2015 at 05:55:20PM +0100, Duy Nguyen wrote:

> On Mon, Dec 7, 2015 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> >
> >>  Let's hope there will be no third report about this commit..
> >
> > Hmm, why does this additional test fail only under prove but pass
> > without it?
> 
> It passes with prove for me. Some mysterious variable leaks through somehow?

It fails for me when run via "make" (with prove or without) but not as
"./t0001-init.sh". Looks like extra variables from my config.mak leak
through:

  $ make t0001-init.sh GIT_TEST_OPTS="-v -i"
  [...]
  --- expected    2015-12-08 17:18:06.304699181 +0000
  +++ actual      2015-12-08 17:18:06.312699180 +0000
  @@ -9,5 +9,9 @@
   GIT_MERGE_VERBOSITY
   GIT_PREFIX
   GIT_TEMPLATE_DIR
  +GIT_TEST_GIT_DAEMON
  +GIT_TEST_HTTPD
  +GIT_TEST_OPTS
   GIT_TEXTDOMAINDIR
   GIT_TRACE_BARE
  +MAKEFLAGS
  not ok 6 - No extra GIT_* on alias scripts

Any GIT_TEST_* is allowed through by test-lib.sh.

For the MAKEFLAGS one, I suspect it's hitting

  MAKEFLAGS=GIT_TEST_OPTS=...

You should probably anchor your regex. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It fails for me when run via "make" (with prove or without) but not as
> "./t0001-init.sh". Looks like extra variables from my config.mak leak
> through:
>
>   $ make t0001-init.sh GIT_TEST_OPTS="-v -i"
>   [...]
>   --- expected    2015-12-08 17:18:06.304699181 +0000
>   +++ actual      2015-12-08 17:18:06.312699180 +0000
>   @@ -9,5 +9,9 @@
>    GIT_MERGE_VERBOSITY
>    GIT_PREFIX
>    GIT_TEMPLATE_DIR
>   +GIT_TEST_GIT_DAEMON
>   +GIT_TEST_HTTPD
>   +GIT_TEST_OPTS
>    GIT_TEXTDOMAINDIR
>    GIT_TRACE_BARE
>   +MAKEFLAGS
>   not ok 6 - No extra GIT_* on alias scripts
>
> Any GIT_TEST_* is allowed through by test-lib.sh.

Also GIT_PROVE_OPTS (which caused false positive for me).

Perhaps grab "env" output outside the alias to create the expected
(instead of a random handcrafted list that you have to maintain as
the test suite evolves), and compare it from within the alias, or
something?

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

[Duy Nguyen]

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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

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

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

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 1/3] git.c: make it clear save_env() is for alias handling only

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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 2/3] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..

[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 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 <asottile@umich.edu>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

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 <gabriel.ganne@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

[Junio C Hamano]

Thanks.  I wiggled these three on top of the "Revert the earlier
one"; while I think the result is correct, I'd appreciate if you can
double check the result when I push the topic out later today.

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

[Duy Nguyen]

On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.  I wiggled these three on top of the "Revert the earlier
> one"; while I think the result is correct, I'd appreciate if you can
> double check the result when I push the topic out later today.

Looks good. "prove" passed too by the way.
-- 
Duy

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

[Duy Nguyen]

On Tue, Dec 22, 2015 at 5:57 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Thanks.  I wiggled these three on top of the "Revert the earlier
>> one"; while I think the result is correct, I'd appreciate if you can
>> double check the result when I push the topic out later today.
>
> Looks good. "prove" passed too by the way.

Another by the way, this "forcing aliases as external commands" now
shows something like "error: git-log died of signal 13" when the pager
exits early, for an alias like "l1 = log --oneline". It's probably not
a regression because other external git commands with pager should
show the same message. But I haven't checked thoroughly yet.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Dec 22, 2015 at 5:57 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Thanks.  I wiggled these three on top of the "Revert the earlier
>>> one"; while I think the result is correct, I'd appreciate if you can
>>> double check the result when I push the topic out later today.
>>
>> Looks good. "prove" passed too by the way.
>
> Another by the way, this "forcing aliases as external commands" now
> shows something like "error: git-log died of signal 13" when the pager
> exits early, for an alias like "l1 = log --oneline".

... and we do not show that when we directly call "git log" is...?

We do signal this with non-zero exit status like so:

	$ GIT_PAGER=true git log --oneline ; echo $?
        141

and it is not surprising that the one that is catching the exit
status of what was spawned and reporting "signal 13".

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

[Jeff King]

On Tue, Dec 22, 2015 at 10:13:03AM -0800, Junio C Hamano wrote:

> > Another by the way, this "forcing aliases as external commands" now
> > shows something like "error: git-log died of signal 13" when the pager
> > exits early, for an alias like "l1 = log --oneline".
> 
> ... and we do not show that when we directly call "git log" is...?
> 
> We do signal this with non-zero exit status like so:
> 
> 	$ GIT_PAGER=true git log --oneline ; echo $?
>         141
> 
> and it is not surprising that the one that is catching the exit
> status of what was spawned and reporting "signal 13".

This sounded very familiar. Apparently I've been running with the patch
below for almost 3 years and never got around to re-examining it.

The original discussion is in:

  http://thread.gmane.org/gmane.comp.version-control.git/212630

Having skimmed through the arguments there, and given that we applied
the patch from the middle of the thread, I think this is a good change.

-- >8 --
Date: Fri, 4 Jan 2013 07:19:35 -0500
Subject: [PATCH] avoid SIGPIPE warnings for aliases

When git executes an alias that specifies an external
command, it will complain if the alias dies due to a signal.
This is usually a good thing, as signal deaths are
unexpected. However, SIGPIPE is not unexpected for many
commands which produce a lot of output; it is intended that
the user closing the pager would kill them them via SIGPIPE.

As a result, the user might see annoying messages in a
scenario like this:

  $ cat ~/.gitconfig
  [alias]
  lgbase = log --some-options
  lg = !git lgbase --more-options
  lg2 = !git lgbase --other-options

  $ git lg -p
  [user hits 'q' to exit pager]
  error: git lgbase --more-options died of signal 13
  fatal: While expanding alias 'lg': 'git lgbase --more-options': Success

Many users won't see this, because we execute the external
command with the shell, and a POSIX shell will silently
rewrite the signal-death exit code into 128+signal, and we
will treat it like a normal exit code. However, this does
not always happen:

  1. If the sub-command does not have shell metacharacters,
     we will skip the shell and exec it directly, getting
     the signal code.

  2. Some shells do not do this rewriting; for example,
     setting SHELL_PATH set to zsh will reveal this problem.

This patch squelches the message, and lets git exit silently
(with the same exit code that a shell would use in case of a
signal).

The first line of the message comes from run-command's
wait_or_whine. To silence that message, we remain quiet
anytime we see SIGPIPE.

The second line comes from handle_alias itself. It calls
die_errno whenever run_command returns a negative value.
However, only -1 indicates a syscall error where errno has
something useful (note that it says the useless "success"
above). Instead, we treat negative returns from run_command
(except for -1) as a normal code to be passed to exit.

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c         | 2 +-
 run-command.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index 6ed824c..34a18a3 100644
--- a/git.c
+++ b/git.c
@@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			alias_argv[argc] = NULL;
 
 			ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
-			if (ret >= 0)   /* normal exit */
+			if (ret != -1)  /* normal exit */
 				exit(ret);
 
 			die_errno("While expanding alias '%s': '%s'",
diff --git a/run-command.c b/run-command.c
index 13fa452..694a6ff 100644
--- a/run-command.c
+++ b/run-command.c
@@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
 		code = WTERMSIG(status);
-		if (code != SIGINT && code != SIGQUIT)
+		if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)
 			error("%s died of signal %d", argv0, code);
 		/*
 		 * This return value is chosen so that code & 0xff
-- 
2.7.0.rc2.368.g1cbb535

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

[Duy Nguyen]

On Wed, Dec 23, 2015 at 4:37 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 22, 2015 at 10:13:03AM -0800, Junio C Hamano wrote:
>
>> > Another by the way, this "forcing aliases as external commands" now
>> > shows something like "error: git-log died of signal 13" when the pager
>> > exits early, for an alias like "l1 = log --oneline".
>>
>> ... and we do not show that when we directly call "git log" is...?
>>
>> We do signal this with non-zero exit status like so:
>>
>>       $ GIT_PAGER=true git log --oneline ; echo $?
>>         141
>>
>> and it is not surprising that the one that is catching the exit
>> status of what was spawned and reporting "signal 13".
>
> This sounded very familiar. Apparently I've been running with the patch
> below for almost 3 years and never got around to re-examining it.
>
> The original discussion is in:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/212630
>
> Having skimmed through the arguments there, and given that we applied
> the patch from the middle of the thread, I think this is a good change.

Yep. The new thing here is the "died of" message now also comes from
execv_dashed_external() for non-external aliases, so it can show up a
lot more often. I wasn't sure if excluding SIGPIPE in wait_and_whine
was safe. But I _guess_ it is, based on your having no problem with
the patch for a long time and the past discussion.

>
> -- >8 --
> Date: Fri, 4 Jan 2013 07:19:35 -0500
> Subject: [PATCH] avoid SIGPIPE warnings for aliases
>
> When git executes an alias that specifies an external
> command, it will complain if the alias dies due to a signal.
> This is usually a good thing, as signal deaths are
> unexpected. However, SIGPIPE is not unexpected for many
> commands which produce a lot of output; it is intended that
> the user closing the pager would kill them them via SIGPIPE.
>
> As a result, the user might see annoying messages in a
> scenario like this:
>
>   $ cat ~/.gitconfig
>   [alias]
>   lgbase = log --some-options
>   lg = !git lgbase --more-options
>   lg2 = !git lgbase --other-options
>
>   $ git lg -p
>   [user hits 'q' to exit pager]
>   error: git lgbase --more-options died of signal 13
>   fatal: While expanding alias 'lg': 'git lgbase --more-options': Success
>
> Many users won't see this, because we execute the external
> command with the shell, and a POSIX shell will silently
> rewrite the signal-death exit code into 128+signal, and we
> will treat it like a normal exit code. However, this does
> not always happen:
>
>   1. If the sub-command does not have shell metacharacters,
>      we will skip the shell and exec it directly, getting
>      the signal code.
>
>   2. Some shells do not do this rewriting; for example,
>      setting SHELL_PATH set to zsh will reveal this problem.
>
> This patch squelches the message, and lets git exit silently
> (with the same exit code that a shell would use in case of a
> signal).
>
> The first line of the message comes from run-command's
> wait_or_whine. To silence that message, we remain quiet
> anytime we see SIGPIPE.
>
> The second line comes from handle_alias itself. It calls
> die_errno whenever run_command returns a negative value.
> However, only -1 indicates a syscall error where errno has
> something useful (note that it says the useless "success"
> above). Instead, we treat negative returns from run_command
> (except for -1) as a normal code to be passed to exit.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  git.c         | 2 +-
>  run-command.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git.c b/git.c
> index 6ed824c..34a18a3 100644
> --- a/git.c
> +++ b/git.c
> @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)
>                         alias_argv[argc] = NULL;
>
>                         ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
> -                       if (ret >= 0)   /* normal exit */
> +                       if (ret != -1)  /* normal exit */
>                                 exit(ret);
>
>                         die_errno("While expanding alias '%s': '%s'",
> diff --git a/run-command.c b/run-command.c
> index 13fa452..694a6ff 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>                 error("waitpid is confused (%s)", argv0);
>         } else if (WIFSIGNALED(status)) {
>                 code = WTERMSIG(status);
> -               if (code != SIGINT && code != SIGQUIT)
> +               if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)
>                         error("%s died of signal %d", argv0, code);
>                 /*
>                  * This return value is chosen so that code & 0xff
> --
> 2.7.0.rc2.368.g1cbb535
>



-- 
Duy

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

[Eric Sunshine]

On Wed, Dec 23, 2015 at 4:37 AM, Jeff King <peff@peff.net> wrote:
> Subject: [PATCH] avoid SIGPIPE warnings for aliases
>
> When git executes an alias that specifies an external
> command, it will complain if the alias dies due to a signal.
> This is usually a good thing, as signal deaths are
> unexpected. However, SIGPIPE is not unexpected for many
> commands which produce a lot of output; it is intended that
> the user closing the pager would kill them them via SIGPIPE.

s/them them/them/

> As a result, the user might see annoying messages in a
> scenario like this:
>
>   $ cat ~/.gitconfig
>   [alias]
>   lgbase = log --some-options
>   lg = !git lgbase --more-options
>   lg2 = !git lgbase --other-options
>
>   $ git lg -p
>   [user hits 'q' to exit pager]
>   error: git lgbase --more-options died of signal 13
>   fatal: While expanding alias 'lg': 'git lgbase --more-options': Success
>
> Many users won't see this, because we execute the external
> command with the shell, and a POSIX shell will silently
> rewrite the signal-death exit code into 128+signal, and we
> will treat it like a normal exit code. However, this does
> not always happen:
>
>   1. If the sub-command does not have shell metacharacters,
>      we will skip the shell and exec it directly, getting
>      the signal code.
>
>   2. Some shells do not do this rewriting; for example,
>      setting SHELL_PATH set to zsh will reveal this problem.
>
> This patch squelches the message, and lets git exit silently
> (with the same exit code that a shell would use in case of a
> signal).
>
> The first line of the message comes from run-command's
> wait_or_whine. To silence that message, we remain quiet
> anytime we see SIGPIPE.
>
> The second line comes from handle_alias itself. It calls
> die_errno whenever run_command returns a negative value.
> However, only -1 indicates a syscall error where errno has
> something useful (note that it says the useless "success"
> above). Instead, we treat negative returns from run_command
> (except for -1) as a normal code to be passed to exit.
>
> Signed-off-by: Jeff King <peff@peff.net>

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

[Johannes Sixt]

Am 23.12.2015 um 10:37 schrieb Jeff King:
> The second line comes from handle_alias itself. It calls
> die_errno whenever run_command returns a negative value.
> However, only -1 indicates a syscall error where errno has
> something useful (note that it says the useless "success"
> above). Instead, we treat negative returns from run_command
> (except for -1) as a normal code to be passed to exit.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   git.c         | 2 +-
>   run-command.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git.c b/git.c
> index 6ed824c..34a18a3 100644
> --- a/git.c
> +++ b/git.c
> @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)
>   			alias_argv[argc] = NULL;
>
>   			ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
> -			if (ret >= 0)   /* normal exit */
> +			if (ret != -1)  /* normal exit */

Why does this make a difference? We only ever return -1, zero, or a 
positive value from run_command/finish_command/wait_or_whine, as far as 
I can see.

>   				exit(ret);
>
>   			die_errno("While expanding alias '%s': '%s'",

-- Hannes

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

[Jeff King]

On Wed, Dec 23, 2015 at 09:37:04PM +0100, Johannes Sixt wrote:

> >--- a/git.c
> >+++ b/git.c
> >@@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)
> >  			alias_argv[argc] = NULL;
> >
> >  			ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
> >-			if (ret >= 0)   /* normal exit */
> >+			if (ret != -1)  /* normal exit */
> 
> Why does this make a difference? We only ever return -1, zero, or a positive
> value from run_command/finish_command/wait_or_whine, as far as I can see.

Yeah, you're right. This bit predates 709ca73 (run-command: encode
signal death as a positive integer, 2013-01-05), which came out of the
same discussion. So I'd agree this hunk can simply be dropped.

That leaves the ignoring of SIGPIPE in wait_or_whine. I started to
rewrite the commit message to drop the first hunk, but I found I
couldn't replicate the problem in the second either!

Doing:

  GIT_PAGER=false git -c alias.foo='!git log -p' foo

doesn't trigger it. We run the alias through a shell, so we see only the
munged "141" value from the shell's exit code.

Something like:

  GIT_PAGER=false git -p -c alias.foo='!yes' foo

does generate the error message. But we've redirected stderr into the
pager at that point, so by definition it can never be shown.

So I think we would need a case where:

  - the outer git doesn't run the pager that dies; instead the pager is
    run inside the alias. But...

  - inside the alias cannot be a shell pipeline, since "foo | less" will
    report the exit code of "less", not "foo" (we make special arrangements
    in git to propagate the exit code of "foo"). So it pretty much has
    to be a git invocation inside the alias. But...

  - The git invocation will convert signal death in the sub-process into
    141, like a shell would.

So I'm not sure if this is triggerable at all with an alias.

I did manage to trigger it with an external command, like:

  $ cat $(which git-yes)
  #!/bin/sh
  # This _has_ to be exec, otherwise the shell converts SIGPIPE death
  # into 141.
  exec yes

and then if you run your _own_ pager, like this:

  $ git yes | false
  error: git-yes died of signal 13

you see it. But if git starts the pager, you don't:

  $ GIT_PAGER=false git -p yes

Because the stderr of the outer git process is going to the same dead
pipe.

So my takeaways are:

  1. Complaining about signal death in general is going to be flaky,
     because it's so easy for shells or git to rewrite the exit code and
     not trigger WIFSIGNALED() in the first place.

  2. I doubt anybody is actually seeing this in practice anymore. But
     maybe I am misunderstanding something in Duy's series that changes
     this.

-Peff

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

[Duy Nguyen]

On Thu, Dec 24, 2015 at 4:31 AM, Jeff King <peff@peff.net> wrote:
>   2. I doubt anybody is actually seeing this in practice anymore. But
>      maybe I am misunderstanding something in Duy's series that changes
>      this.

There are two parts in your patch, one (that you two seemed to focus
on) about return code with "!" aliases. Another, suppressing SIGPIPE,
affects more than "!" aliases. In my case it's
execv_dashed_external(). Non-"!" aliases are now forced to use that
function.

This is the output with 'pu'. Notice git-log is executed separately

> ~/w/git $ GIT_TRACE=2 PAGER=true ./git --exec-path=. l
16:28:19.167040 git.c:567               trace: exec: 'git-l'
16:28:19.167088 run-command.c:345       trace: run_command: 'git-l'
16:28:19.168599 git.c:286               trace: alias expansion: l =>
'log' '--stat'
16:28:19.168633 git.c:567               trace: exec: 'git-log' '--stat'
16:28:19.168649 run-command.c:345       trace: run_command: 'git-log' '--stat'
16:28:19.170420 git.c:350               trace: built-in: git 'log' '--stat'
16:28:19.210074 run-command.c:345       trace: run_command: 'true'
16:28:19.210382 run-command.c:204       trace: exec: 'true'
error: git-log died of signal 13

With your patch on top, "git-log died of.." goes away. And this is
with 'master', where the builtin version of git-log is used

> ~/w/git $ GIT_TRACE=2 PAGER=true ./git --exec-path=. l
16:29:17.546382 git.c:561               trace: exec: 'git-l'
16:29:17.546428 run-command.c:343       trace: run_command: 'git-l'
16:29:17.547485 git.c:282               trace: alias expansion: l =>
'log' '--stat'
16:29:17.548482 git.c:348               trace: built-in: git 'log' '--stat'
16:29:17.586457 run-command.c:343       trace: run_command: 'true'
16:29:17.586770 run-command.c:202       trace: exec: 'true'

So, in practice, people will see "died of signal 13" a lot more often
if my series is merged and released (I assume that people often use
aliases for paged commands like git-log, git-diff...)
-- 
Duy

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

[Jeff King]

On Thu, Dec 24, 2015 at 04:35:33PM +0700, Duy Nguyen wrote:

> On Thu, Dec 24, 2015 at 4:31 AM, Jeff King <peff@peff.net> wrote:
> >   2. I doubt anybody is actually seeing this in practice anymore. But
> >      maybe I am misunderstanding something in Duy's series that changes
> >      this.
> 
> There are two parts in your patch, one (that you two seemed to focus
> on) about return code with "!" aliases. Another, suppressing SIGPIPE,
> affects more than "!" aliases.

Sorry if I was confusing; most of the examples in my previous message
are about the SIGPIPE thing. I was having trouble triggering the message
in practice, even for externals (because the error message goes to the
pager, too!).

> In my case it's execv_dashed_external(). Non-"!" aliases are now
> forced to use that function.

Thanks, this is the part I was missing.

The outer git wrapper doesn't start the pager, so its stderr still gets
seen by the user. But the _inner_ git-log does start the pager, and then
dies of SIGPIPE.

So yeah, I think we want something like this on top of
nd/clear-gitenv-upon-use-of-alias.

-- >8 --
Subject: [PATCH] run-command: don't warn on SIGPIPE deaths

When git executes a sub-command, we print a warning if the
command dies due to a signal, but make an exception for
"uninteresting" cases like SIGINT and SIGQUIT (since the
user presumably just hit ^C).

We should make a similar exception for SIGPIPE, because it's
an expected and uninteresting return in most cases; it
generally means the user quit the pager before git had
finished generating all output.  This used to be very hard
to trigger in practice, because:

  1. We only complain if we see a real SIGPIPE death, not
     the shell-induced 141 exit code. This means that
     anything we run via the shell does not trigger the
     warning, which includes most non-trivial aliases.

  2. The common case for SIGPIPE is the user quitting the
     pager before git has finished generating all output.
     But if the user triggers a pager with "-p", we redirect
     the git wrapper's stderr to that pager, too.  Since the
     pager is dead, it means that the message goes nowhere.

  3. You can see it if you run your own pager, like
     "git foo | head". But that only happens if "foo" is a
     non-builtin (so it doesn't work with "log", for
     example).

However, it may become more common after 86d26f2, which
teaches alias to re-exec builtins rather than running them
in the same process. This case doesn't trigger (1), as we
don't need a shell to run a git command. It doesn't trigger
(2), because the pager is not started by the original git,
but by the inner re-exec of git. And it doesn't trigger (3),
because builtins are treated more like non-builtins in this
case.

Given how flaky this message already is (e.g., you cannot
even know whether you will see it, as git optimizes out some
shell invocations behind the scenes based on the contents of
the command!), and that it is unlikely to ever provide
useful information, let's suppress it for all cases of
SIGPIPE.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 13fa452..694a6ff 100644
--- a/run-command.c
+++ b/run-command.c
@@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
 		code = WTERMSIG(status);
-		if (code != SIGINT && code != SIGQUIT)
+		if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)
 			error("%s died of signal %d", argv0, code);
 		/*
 		 * This return value is chosen so that code & 0xff
-- 
2.7.0.rc3.367.g09631da

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The outer git wrapper doesn't start the pager, so its stderr still gets
> seen by the user. But the _inner_ git-log does start the pager, and then
> dies of SIGPIPE.
>
> So yeah, I think we want something like this on top of
> nd/clear-gitenv-upon-use-of-alias.

That makes sense to me.

> -- >8 --
> Subject: [PATCH] run-command: don't warn on SIGPIPE deaths
>
> When git executes a sub-command, we print a warning if the
> command dies due to a signal, but make an exception for
> "uninteresting" cases like SIGINT and SIGQUIT (since the
> user presumably just hit ^C).
>
> We should make a similar exception for SIGPIPE, because it's
> an expected and uninteresting return in most cases; it
> generally means the user quit the pager before git had
> finished generating all output.  This used to be very hard
> to trigger in practice, because:
>
>   1. We only complain if we see a real SIGPIPE death, not
>      the shell-induced 141 exit code. This means that
>      anything we run via the shell does not trigger the
>      warning, which includes most non-trivial aliases.
>
>   2. The common case for SIGPIPE is the user quitting the
>      pager before git has finished generating all output.
>      But if the user triggers a pager with "-p", we redirect
>      the git wrapper's stderr to that pager, too.  Since the
>      pager is dead, it means that the message goes nowhere.
>
>   3. You can see it if you run your own pager, like
>      "git foo | head". But that only happens if "foo" is a
>      non-builtin (so it doesn't work with "log", for
>      example).
>
> However, it may become more common after 86d26f2, which
> teaches alias to re-exec builtins rather than running them
> in the same process. This case doesn't trigger (1), as we
> don't need a shell to run a git command. It doesn't trigger
> (2), because the pager is not started by the original git,
> but by the inner re-exec of git. And it doesn't trigger (3),
> because builtins are treated more like non-builtins in this
> case.
>
> Given how flaky this message already is (e.g., you cannot
> even know whether you will see it, as git optimizes out some
> shell invocations behind the scenes based on the contents of
> the command!), and that it is unlikely to ever provide
> useful information, let's suppress it for all cases of
> SIGPIPE.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  run-command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index 13fa452..694a6ff 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>  		error("waitpid is confused (%s)", argv0);
>  	} else if (WIFSIGNALED(status)) {
>  		code = WTERMSIG(status);
> -		if (code != SIGINT && code != SIGQUIT)
> +		if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)
>  			error("%s died of signal %d", argv0, code);
>  		/*
>  		 * This return value is chosen so that code & 0xff

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

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

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 <pclouds@gmail.com>
---
 On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano <gitster@pobox.com> 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

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

[Junio C Hamano]

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

> 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 <pclouds@gmail.com>
> ---
>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano <gitster@pobox.com> 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.

The timing of this is a bit unfortunate, as d95138e is a regression
since v2.5.1 throughout v2.6 series.  I'll push this out on maint
and onwards for now.  I do not think we added new things that rely
on the post-d95138e "broken" behaviour in-tree, but this reversion
might introduce another unexpected regression X-<.  We'll see.

Thanks.

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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 <pclouds@gmail.com>
>> ---
>>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano <gitster@pobox.com> 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.

Also, can you reference these three reports for future reference?

Thanks.

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

[Duy Nguyen]

On Tue, Dec 22, 2015 at 1:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> 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 <pclouds@gmail.com>
>>> ---
>>>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano <gitster@pobox.com> 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.
>
> Also, can you reference these three reports for future reference?

http://article.gmane.org/gmane.comp.version-control.git/281608
http://article.gmane.org/gmane.comp.version-control.git/281979
http://article.gmane.org/gmane.comp.version-control.git/282691

The last one is not confirmed by the reporter yet. But I'm pretty sure
i'll trigger the special case "when GIT_WORK_TREE is set but GIT_DIR
is not" in setup code
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Dec 22, 2015 at 1:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>>> 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 <pclouds@gmail.com>
>>>> ---
>>>>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano <gitster@pobox.com> 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.
>>
>> Also, can you reference these three reports for future reference?
>
> http://article.gmane.org/gmane.comp.version-control.git/281608
> http://article.gmane.org/gmane.comp.version-control.git/281979
> http://article.gmane.org/gmane.comp.version-control.git/282691
>
> The last one is not confirmed by the reporter yet. But I'm pretty sure
> i'll trigger the special case "when GIT_WORK_TREE is set but GIT_DIR
> is not" in setup code

Thanks, I'll leave these breadcrumbs in the log message for future
reference.

I think the last sentence of the original commit is telling how this
bug came about.  "It does not harm if $GIT_WORK_TREE is set while
$GIT_DIR is not." forgets to consider the possibility that scripts
may be relying on the "Go to the top of the working tree and setting
GIT_DIR would give you a reasonable environment".  That is true if
GIT_WORK_TREE is not set, and these scripts weren't getting the
environment exported [*1*].  These scripts now have to unset
GIT_WORK_TREE themselves (or set it to their $cwd if they are indeed
at the top), just in case the process that calls them exports it
X-<.

Thanks.


[Footnote]

*1* If the end user has GIT_WORK_TREE in the environment, even if
    Git stops exporting it by reverting d95138e, such a script may
    break.  So in that sense, d95138e did not quite change the rule
    of the game for these scripts, but made it more obvious when
    these scripts were written sloppily.