{"thread":{"id":"40865","subject":"Fwd: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","startedAt":"2015-11-24T02:22:11Z","lastAt":"2015-12-29T21:34:41Z","messageCount":39,"participants":["Anthony Sottile","Stefan Beller","Duy Nguyen","Junio C Hamano","Nguyễn Thái Ngọc Duy","Jeff King","Eric Sunshine","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"273631","messageId":"CA+dzEB=XiGVFg+AhuJM-jUCPmgZKCJHTp3sinrFt8yzXeC_63Q@mail.gmail.com","threadId":"40865","inReplyTo":"CA+dzEB=2LJXiLSTqyLw8AeHNwdQicwvEiMg=hVEX0-_s1bySpA@mail.gmail.com","subject":"Fwd: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2015-11-24T02:22:11Z","receivedAt":"2015-11-24T02:22:11Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"* Short description of the problem *\n\nIt seems GIT_WORK_DIR is now exported invariantly when calling git\nhooks such as pre-commit. If these hooks involve cloning repositories\nthey will not fail due to this exported environment variable. This\nwas not the case in prior versions (such as v2.5.0).\n\n* Simple reproduction *\n\n```\n$ cat test.sh\n#!/usr/bin/env bash\nset -ex\n\nrm -rf test\n\n# Exit non {0, 1} to abort git bisect\nmake -j 8 > /dev/null || exit 2\n\n# Put our new git on the path\nPATH=\"$(pwd):$PATH\"\n\ngit init test\n\npushd test\nmkdir -p .git/hooks\necho 'git clone git://github.com/asottile/css-explore css-explore' >\n.git/hooks/pre-commit\nchmod 755 .git/hooks/pre-commit\n\ngit commit -m foo --allow-empty || exit 1\n```\n\n* Under 2.6.3 *\n\n```\n$ ./test.sh\n\n...\n\n+ git init test\nwarning: templates not found /home/anthony/share/git-core/templates\nInitialized empty Git repository in /home/anthony/workspace/git/test/.git/\n+ pushd test\n~/workspace/git/test ~/workspace/git\n+ mkdir -p .git/hooks\n+ echo 'git clone git://github.com/asottile/css-explore css-explore'\n+ chmod 755 .git/hooks/pre-commit\n+ git commit -m foo --allow-empty\nfatal: working tree '.' already exists.\n+ exit 1\n```\n\n* Under 2.5 *\n\n```\n$ ./test.sh\n\n...\n\n+ git init test\nwarning: templates not found /home/anthony/share/git-core/templates\nInitialized empty Git repository in /home/anthony/workspace/git/test/.git/\n+ pushd test\n~/workspace/git/test ~/workspace/git\n+ mkdir -p .git/hooks\n+ echo 'git clone git://github.com/asottile/css-explore css-explore'\n+ chmod 755 .git/hooks/pre-commit\n+ git commit -m foo --allow-empty\nCloning into 'css-explore'...\nwarning: templates not found /home/anthony/share/git-core/templates\nremote: Counting objects: 214, done.\nremote: Total 214 (delta 0), reused 0 (delta 0), pack-reused 214\nReceiving objects: 100% (214/214), 25.89 KiB | 0 bytes/s, done.\nResolving deltas: 100% (129/129), done.\nChecking connectivity... done.\n[master (root-commit) 5eb999d] foo\n```\n\n\n* Bisect *\n\n```\n$ git bisect good v2.5.0\n$ git bisect bad origin/master\n$ git bisect run ./test.sh\n\n...\n\nd95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit\ncommit d95138e695d99d32dcad528a2a7974f434c51e79\nAuthor: Nguyễn Thái Ngọc Duy \nDate: Fri Jun 26 17:37:35 2015 +0700\n\n setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\n\n In the test case, we run setup_git_dir_gently() the first time to read\n $GIT_DIR/config so that we can resolve aliases. We'll enter\n setup_discovered_git_dir() and may or may not call set_git_dir() near\n the end of the function, depending on whether the detected git dir is\n \".git\" or not. This set_git_dir() will set env var $GIT_DIR.\n\n For normal repo, git dir detected via setup_discovered_git_dir() will be\n \".git\", and set_git_dir() is not called. If .git file is used however,\n the git dir can't be \".git\" and set_git_dir() is called and $GIT_DIR\n set. This is the key of this problem.\n\n If we expand an alias (or autocorrect command names), then\n setup_git_dir_gently() is run the second time. If $GIT_DIR is not set in\n the first run, we run the same setup_discovered_git_dir() as before.\n Nothing to see. If it is, however, we'll enter setup_explicit_git_dir()\n this time.\n\n This is where the \"fun\" is. If $GIT_WORK_TREE is not set but\n $GIT_DIR is, you are supposed to be at the root level of the\n worktree. But if you are in a subdir \"foo/bar\" (real worktree's top\n is \"foo\"), this rule bites you: your detected worktree is now\n \"foo/bar\", even though the first run correctly detected worktree as\n \"foo\". You get \"internal error: work tree has already been set\" as a\n result.\n\n Bottom line is, when $GIT_DIR is set, $GIT_WORK_TREE should be set too\n unless there's no work tree. But setting $GIT_WORK_TREE inside\n set_git_dir() may backfire. We don't know at that point if work tree is\n already configured by the caller. So set it when work tree is\n detected. It does not harm if $GIT_WORK_TREE is set while $GIT_DIR is\n not.\n\n Reported-by: Bjørnar Snoksrud \n Signed-off-by: Nguyễn Thái Ngọc Duy \n Signed-off-by: Junio C Hamano \n\n:100644 100644 9daa0ba4a36ced9f63541203e7bcc2ab9e1eae56\n36fbba57fc83afd36d99bf5d4f3a1fc3feefba09 M environment.c\n:040000 040000 1d7c4bf77e0fd49ca315271993cb69a8b055c3aa\n145d85895cb6cb0810597e1854a7721ccfc8f457 M t\nbisect run success\n```\n\nCausing me a few headaches in\nhttps://github.com/pre-commit/pre-commit/issues/300\nI'm working around it in https://github.com/pre-commit/pre-commit/pull/301\n\nThanks,\n\nAnthony\n"},{"id":"273644","messageId":"CAGZ79kY=t9SeoXjgeJjfCMD2=6g3JJxDxcnY6JeJCpUqaN+eOA@mail.gmail.com","threadId":"40865","inReplyTo":"CA+dzEB=XiGVFg+AhuJM-jUCPmgZKCJHTp3sinrFt8yzXeC_63Q@mail.gmail.com","subject":"Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-24T17:57:35Z","receivedAt":"2015-11-24T17:57:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+to Nguyễn Thái Ngọc Duy \n\nOn Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile wrote:\n> * Short description of the problem *\n>\n> It seems GIT_WORK_DIR is now exported invariantly when calling git\n> hooks such as pre-commit. If these hooks involve cloning repositories\n> they will not fail due to this exported environment variable. This\n> was not the case in prior versions (such as v2.5.0).\n>\n> * Simple reproduction *\n>\n> ```\n> $ cat test.sh\n> #!/usr/bin/env bash\n> set -ex\n>\n> rm -rf test\n>\n> # Exit non {0, 1} to abort git bisect\n> make -j 8 > /dev/null || exit 2\n>\n> # Put our new git on the path\n> PATH=\"$(pwd):$PATH\"\n>\n> git init test\n>\n> pushd test\n> mkdir -p .git/hooks\n> echo 'git clone git://github.com/asottile/css-explore css-explore' >\n> .git/hooks/pre-commit\n> chmod 755 .git/hooks/pre-commit\n>\n> git commit -m foo --allow-empty || exit 1\n> ```\n>\n> * Under 2.6.3 *\n>\n> ```\n> $ ./test.sh\n>\n> ...\n>\n> + git init test\n> warning: templates not found /home/anthony/share/git-core/templates\n> Initialized empty Git repository in /home/anthony/workspace/git/test/.git/\n> + pushd test\n> ~/workspace/git/test ~/workspace/git\n> + mkdir -p .git/hooks\n> + echo 'git clone git://github.com/asottile/css-explore css-explore'\n> + chmod 755 .git/hooks/pre-commit\n> + git commit -m foo --allow-empty\n> fatal: working tree '.' already exists.\n> + exit 1\n> ```\n>\n> * Under 2.5 *\n>\n> ```\n> $ ./test.sh\n>\n> ...\n>\n> + git init test\n> warning: templates not found /home/anthony/share/git-core/templates\n> Initialized empty Git repository in /home/anthony/workspace/git/test/.git/\n> + pushd test\n> ~/workspace/git/test ~/workspace/git\n> + mkdir -p .git/hooks\n> + echo 'git clone git://github.com/asottile/css-explore css-explore'\n> + chmod 755 .git/hooks/pre-commit\n> + git commit -m foo --allow-empty\n> Cloning into 'css-explore'...\n> warning: templates not found /home/anthony/share/git-core/templates\n> remote: Counting objects: 214, done.\n> remote: Total 214 (delta 0), reused 0 (delta 0), pack-reused 214\n> Receiving objects: 100% (214/214), 25.89 KiB | 0 bytes/s, done.\n> Resolving deltas: 100% (129/129), done.\n> Checking connectivity... done.\n> [master (root-commit) 5eb999d] foo\n> ```\n>\n>\n> * Bisect *\n>\n> ```\n> $ git bisect good v2.5.0\n> $ git bisect bad origin/master\n> $ git bisect run ./test.sh\n>\n> ...\n>\n> d95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit\n> commit d95138e695d99d32dcad528a2a7974f434c51e79\n> Author: Nguyễn Thái Ngọc Duy \n> Date: Fri Jun 26 17:37:35 2015 +0700\n>\n> setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\n>\n> In the test case, we run setup_git_dir_gently() the first time to read\n> $GIT_DIR/config so that we can resolve aliases. We'll enter\n> setup_discovered_git_dir() and may or may not call set_git_dir() near\n> the end of the function, depending on whether the detected git dir is\n> \".git\" or not. This set_git_dir() will set env var $GIT_DIR.\n>\n> For normal repo, git dir detected via setup_discovered_git_dir() will be\n> \".git\", and set_git_dir() is not called. If .git file is used however,\n> the git dir can't be \".git\" and set_git_dir() is called and $GIT_DIR\n> set. This is the key of this problem.\n>\n> If we expand an alias (or autocorrect command names), then\n> setup_git_dir_gently() is run the second time. If $GIT_DIR is not set in\n> the first run, we run the same setup_discovered_git_dir() as before.\n> Nothing to see. If it is, however, we'll enter setup_explicit_git_dir()\n> this time.\n>\n> This is where the \"fun\" is. If $GIT_WORK_TREE is not set but\n> $GIT_DIR is, you are supposed to be at the root level of the\n> worktree. But if you are in a subdir \"foo/bar\" (real worktree's top\n> is \"foo\"), this rule bites you: your detected worktree is now\n> \"foo/bar\", even though the first run correctly detected worktree as\n> \"foo\". You get \"internal error: work tree has already been set\" as a\n> result.\n>\n> Bottom line is, when $GIT_DIR is set, $GIT_WORK_TREE should be set too\n> unless there's no work tree. But setting $GIT_WORK_TREE inside\n> set_git_dir() may backfire. We don't know at that point if work tree is\n> already configured by the caller. So set it when work tree is\n> detected. It does not harm if $GIT_WORK_TREE is set while $GIT_DIR is\n> not.\n>\n> Reported-by: Bjørnar Snoksrud \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Junio C Hamano \n>\n> :100644 100644 9daa0ba4a36ced9f63541203e7bcc2ab9e1eae56\n> 36fbba57fc83afd36d99bf5d4f3a1fc3feefba09 M environment.c\n> :040000 040000 1d7c4bf77e0fd49ca315271993cb69a8b055c3aa\n> 145d85895cb6cb0810597e1854a7721ccfc8f457 M t\n> bisect run success\n> ```\n>\n> Causing me a few headaches in\n> https://github.com/pre-commit/pre-commit/issues/300\n> I'm working around it in https://github.com/pre-commit/pre-commit/pull/301\n>\n> Thanks,\n>\n> Anthony\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"273760","messageId":"CACsJy8C7xoV9faGpbn_5XGt-CmCj--fgXaCFR-ngs=-pWUnrCA@mail.gmail.com","threadId":"40865","inReplyTo":"CAGZ79kY=t9SeoXjgeJjfCMD2=6g3JJxDxcnY6JeJCpUqaN+eOA@mail.gmail.com","subject":"Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-11-25T20:13:05Z","receivedAt":"2015-11-25T20:13:05Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 24, 2015 at 6:57 PM, Stefan Beller wrote:\n> +to Nguyễn Thái Ngọc Duy \n>\n> On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile wrote:\n>> * Short description of the problem *\n>>\n>> It seems GIT_WORK_DIR is now exported invariantly when calling git\n>> hooks such as pre-commit. If these hooks involve cloning repositories\n>> they will not fail due to this exported environment variable. This\n>> was not the case in prior versions (such as v2.5.0).\n\nI'm getting good at fixing one bug and adding ten more. I don't think\nthe cited commit is the problem. It just exposes another bug. I did\n\n> ~/w/git $ GIT_WORK_TREE=abc ./git clone .git /tmp/def\n\nand what I got was really surprising, /tmp/def contains the git\nrepository while the true worktree is in \"abc\". It does not make\nsense, at least from the first sight, unless it inherits this from\ngit-init, where we do(?) want GIT_WORK_TREE to specify a separate\nworktree. No time to dig to the bottom yet..\n-- \nDuy\n"},{"id":"273832","messageId":"CACsJy8Ciuirgk9D_fbQ5pgo-8u1AnM+zBdKUHcz_HLfRqM9QxQ@mail.gmail.com","threadId":"40865","inReplyTo":"CACsJy8C7xoV9faGpbn_5XGt-CmCj--fgXaCFR-ngs=-pWUnrCA@mail.gmail.com","subject":"Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-11-30T19:01:07Z","receivedAt":"2015-11-30T19:01:07Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 25, 2015 at 9:13 PM, Duy Nguyen wrote:\n> On Tue, Nov 24, 2015 at 6:57 PM, Stefan Beller wrote:\n>> +to Nguyễn Thái Ngọc Duy \n>>\n>> On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile wrote:\n>>> * Short description of the problem *\n>>>\n>>> It seems GIT_WORK_DIR is now exported invariantly when calling git\n>>> hooks such as pre-commit. If these hooks involve cloning repositories\n>>> they will not fail due to this exported environment variable. This\n>>> was not the case in prior versions (such as v2.5.0).\n>\n> I'm getting good at fixing one bug and adding ten more. I don't think\n> the cited commit is the problem. It just exposes another bug. I did\n>\n>> ~/w/git $ GIT_WORK_TREE=abc ./git clone .git /tmp/def\n>\n> and what I got was really surprising, /tmp/def contains the git\n> repository while the true worktree is in \"abc\". It does not make\n> sense, at least from the first sight, unless it inherits this from\n> git-init, where we do(?) want GIT_WORK_TREE to specify a separate\n> worktree. No time to dig to the bottom yet..\n\nI was wrong, GIT_WORK_TREE support was added in git-clone many years\nago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So\nmy change accidentally triggers an (undocumented) feature. We could\nadd a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't\nthink people will like it. I don't really like reverting d95138e\n(setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -\n2015-06-26) because another bug reappears. So I'm out of options..\n-- \nDuy\n"},{"id":"273842","messageId":"xmqqwpsz6y5b.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"CACsJy8Ciuirgk9D_fbQ5pgo-8u1AnM+zBdKUHcz_HLfRqM9QxQ@mail.gmail.com","subject":"Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-30T20:16:32Z","receivedAt":"2015-11-30T20:16:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I was wrong, GIT_WORK_TREE support was added in git-clone many years\n> ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So\n> my change accidentally triggers an (undocumented) feature. We could\n> add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't\n> think people will like it. I don't really like reverting d95138e\n> (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -\n> 2015-06-26) because another bug reappears.\n\n> So I'm out of options..\n\nPerhaps d95138e can be reverted and then the bug it tried to fix can\nbe fixed in a different way somehow?\n\n(I am not quite up to speed yet, so the above may turn out to be\ninfeasible--take it with a large grain of salt please).\n"},{"id":"273862","messageId":"CACsJy8A0WJYb7k6+JUOk9Y_hOMhinWP1T30GHG8fLqkmgMdbhQ@mail.gmail.com","threadId":"40865","inReplyTo":"xmqqwpsz6y5b.fsf@gitster.mtv.corp.google.com","subject":"Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-01T17:59:50Z","receivedAt":"2015-12-01T17:59:50Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 30, 2015 at 9:16 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> I was wrong, GIT_WORK_TREE support was added in git-clone many years\n>> ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So\n>> my change accidentally triggers an (undocumented) feature. We could\n>> add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't\n>> think people will like it. I don't really like reverting d95138e\n>> (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -\n>> 2015-06-26) because another bug reappears.\n>\n>> So I'm out of options..\n>\n> Perhaps d95138e can be reverted and then the bug it tried to fix can\n> be fixed in a different way somehow?\n>\n> (I am not quite up to speed yet, so the above may turn out to be\n> infeasible--take it with a large grain of salt please).\n\nThat would mean we do not set $GIT_DIR too early. While it sounds\ngood, it could be just another trap, and could be a lot of\nreorganizing setup code. I'm more tempted to revert 20ccef4, with\ndeprecation warning for some releases, and a new git-clone option for\nthe same functionality. But let me sleep on it (and everybody please\ngive ideas if you have any). Meanwhile, maybe reverting d95138e should\nbe done any way for now. Broken aliases are not as bad as broken\nhooks.\n-- \nDuy\n"},{"id":"273914","messageId":"xmqqh9k06all.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"CACsJy8A0WJYb7k6+JUOk9Y_hOMhinWP1T30GHG8fLqkmgMdbhQ@mail.gmail.com","subject":"Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-02T17:09:42Z","receivedAt":"2015-12-02T17:09:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> ... But let me sleep on it (and everybody please\n> give ideas if you have any). Meanwhile, maybe reverting d95138e should\n> be done any way for now. Broken aliases are not as bad as broken\n> hooks.\n\nOK, when/if you decide that our first step should be a revert of\nd95138e please send in a patch to do so with a brief write-up of a\nfollow-up plan.\n\nThanks.\n"},{"id":"273982","messageId":"1449166676-30845-1-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"xmqqh9k06all.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/2] git.c: make it clear save_env() is for alias handling only","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-03T18:17:55Z","receivedAt":"2015-12-03T18:17:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n git.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 6ed824c..6ce7043 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,14 +25,14 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_environment;\n+static int saved_env_before_alias;\n \n-static void save_env(void)\n+static void save_env_before_alias(void)\n {\n \tint i;\n-\tif (saved_environment)\n+\tif (saved_env_before_alias)\n \t\treturn;\n-\tsaved_environment = 1;\n+\tsaved_env_before_alias = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -233,6 +233,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \tchar *alias_string;\n \tint unused_nongit;\n \n+\tsave_env_before_alias();\n \tsubdir = setup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n@@ -530,7 +531,7 @@ static void handle_builtin(int argc, const char **argv)\n \n \tbuiltin = get_builtin(cmd);\n \tif (builtin) {\n-\t\tif (saved_environment && (builtin->option & NO_SETUP))\n+\t\tif (saved_env_before_alias && (builtin->option & NO_SETUP))\n \t\t\trestore_env();\n \t\telse\n \t\t\texit(run_builtin(builtin, argc, argv));\n@@ -590,7 +591,6 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t */\n \t\tif (done_alias)\n \t\t\tbreak;\n-\t\tsave_env();\n \t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n \t\tdone_alias = 1;\n-- \n2.2.0.513.g477eb31\n"},{"id":"273981","messageId":"1449166676-30845-2-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"1449166676-30845-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-03T18:17:56Z","receivedAt":"2015-12-03T18:17:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit d95138e [1] fixes a .git file problem by setting GIT_WORK_TREE\nwhenever GIT_DIR is set. It sounded harmless because we handle\nGIT_DIR and GIT_WORK_TREE side by side for most commands, with two\nexceptions: git-init and git-clone.\n\n\"git clone\" is not happy with d95138e. This command ignores GIT_DIR\nbut respects GIT_WORK_TREE [2] [3] which means it used to run fine\nfrom a hook, where GIT_DIR was set but GIT_WORK_TREE was not (*). With\nd95138e, GIT_WORK_TREE is set all the time and git-clone interprets\nthat as \"I give you order to put the worktree here\", usually against\nthe user's intention.\n\nThe solution in d95138e is reverted in this commit. Instead we reuse the\nsolution from c056261 [4]. c056261 fixes another setup-messed-up-by-alias\nby saving and restoring env for git-clone and git-init. Now I conclude\nthat setup-messed-by-alias is always evil. So the env restoration is\ndone for _all_ commands whenever aliases are involved. It fixes what\nd95138e tries to fix. And it does not upset git-clone-inside-hooks.\n\nThe test from d95138e remains to verify it's not broken by this. A new\ntest is added to make sure git-clone-inside-hooks remains happy.\n\nIn future, perhaps we should look up aliases in a separate process\n(provided that Windows folks are happy with one extra process). It'll be\ncleanest.\n\n(*) GIT_WORK_TREE was not set _most of the time_. In some cases\n GIT_WORK_TREE is set and git-clone will behave differently. The\n use of GIT_WORK_TREE to direct git-clone to put work tree\n elsewhere looks like a mistake because it causes surprises this\n way. But that's a separate story.\n\n[1] d95138e (setup: set env $GIT_WORK_TREE when work tree is set, like\n $GIT_DIR - 2015-06-26)\n[2] 2beebd2 (clone: create intermediate directories of destination\n repo - 2008-06-25)\n[3] 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06)\n[4] c056261 (git potty: restore environments after alias expansion -\n 2014-06-08)\n\nReported-by: Anthony Sottile \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This may be a brilliant fix, or another invitation for regressions.\n\n environment.c | 2 --\n git.c | 9 ++++-----\n t/t5601-clone.sh | 23 +++++++++++++++++++++++\n 3 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 2da7fe2..1cc4aab 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -235,8 +235,6 @@ void set_git_work_tree(const char *new_work_tree)\n \t}\n \tgit_work_tree_initialized = 1;\n \twork_tree = xstrdup(real_path(new_work_tree));\n-\tif (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))\n-\t\tdie(\"could not set GIT_WORK_TREE to '%s'\", work_tree);\n }\n \n const char *get_git_work_tree(void)\ndiff --git a/git.c b/git.c\nindex 6ce7043..e2f187d 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,7 +308,6 @@ static int handle_alias(int *argcp, const char ***argv)\n * RUN_SETUP for reading from the configuration file.\n */\n #define NEED_WORK_TREE\t\t(1<<3)\n-#define NO_SETUP\t\t(1<<4)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -390,7 +389,7 @@ static struct cmd_struct commands[] = {\n \t{ \"cherry\", cmd_cherry, RUN_SETUP },\n \t{ \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"clean\", cmd_clean, RUN_SETUP | NEED_WORK_TREE },\n-\t{ \"clone\", cmd_clone, NO_SETUP },\n+\t{ \"clone\", cmd_clone },\n \t{ \"column\", cmd_column, RUN_SETUP_GENTLY },\n \t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n@@ -416,8 +415,8 @@ static struct cmd_struct commands[] = {\n \t{ \"hash-object\", cmd_hash_object },\n \t{ \"help\", cmd_help },\n \t{ \"index-pack\", cmd_index_pack, RUN_SETUP_GENTLY },\n-\t{ \"init\", cmd_init_db, NO_SETUP },\n-\t{ \"init-db\", cmd_init_db, NO_SETUP },\n+\t{ \"init\", cmd_init_db },\n+\t{ \"init-db\", cmd_init_db },\n \t{ \"interpret-trailers\", cmd_interpret_trailers, RUN_SETUP_GENTLY },\n \t{ \"log\", cmd_log, RUN_SETUP },\n \t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n@@ -531,7 +530,7 @@ static void handle_builtin(int argc, const char **argv)\n \n \tbuiltin = get_builtin(cmd);\n \tif (builtin) {\n-\t\tif (saved_env_before_alias && (builtin->option & NO_SETUP))\n+\t\tif (saved_env_before_alias)\n \t\t\trestore_env();\n \t\telse\n \t\t\texit(run_builtin(builtin, argc, argv));\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 9b34f3c..31b4658 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -65,6 +65,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone from hooks' '\n+\n+\ttest_create_repo r0 &&\n+\tcd r0 &&\n+\ttest_commit initial &&\n+\tcd .. &&\n+\tgit init r1 &&\n+\tcd r1 &&\n+\tcat >.git/hooks/pre-commit <<-\\EOF &&\n+\t#!/bin/sh\n+\tgit clone ../r0 ../r2\n+\texit 1\n+\tEOF\n+\tchmod u+x .git/hooks/pre-commit &&\n+\t: >file &&\n+\tgit add file &&\n+\ttest_must_fail git commit -m invoke-hook &&\n+\tcd .. &&\n+\ttest_cmp r0/.git/HEAD r2/.git/HEAD &&\n+\ttest_cmp r0/initial.t r2/initial.t\n+\n+'\n+\n test_expect_success 'clone creates intermediate directories' '\n \n \tgit clone src long/path/to/dst &&\n-- \n2.2.0.513.g477eb31\n"},{"id":"274020","messageId":"xmqqa8pqrlz0.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"1449166676-30845-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-04T20:35:15Z","receivedAt":"2015-12-04T20:35:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> ... Now I conclude\n> that setup-messed-by-alias is always evil. So the env restoration is\n> done for _all_ commands whenever aliases are involved.\n\nSo a side effect of this is whenever an alias is involved, all\ncommands are re-spawned, not just the external ones but builtins as\nwell.\n\n> This may be a brilliant fix, or another invitation for regressions.\n\n;-)\n"},{"id":"274046","messageId":"CACsJy8BXy03EMLRcXNKnkAj_kxreEVZYRVRTqNz2EMNZ-bfYrA@mail.gmail.com","threadId":"40865","inReplyTo":"xmqqa8pqrlz0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-05T05:48:44Z","receivedAt":"2015-12-05T05:48:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 4, 2015 at 9:35 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> ... Now I conclude\n>> that setup-messed-by-alias is always evil. So the env restoration is\n>> done for _all_ commands whenever aliases are involved.\n>\n> So a side effect of this is whenever an alias is involved, all\n> commands are re-spawned, not just the external ones but builtins as\n> well.\n\nI missed that while re-reading c056261, but yes that's true. So\nWindows folks will be grumpy anyway. Maybe we can avoid that in\ncertain (safe) cases, when we know the second setup_git_... will be\nexecuted by the builtin command and won't have any side effects, which\nis probably the common case. But let's see how it goes.\n-- \nDuy\n"},{"id":"274056","messageId":"1449329538-18623-1-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"1449166676-30845-2-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-05T15:32:18Z","receivedAt":"2015-12-05T15:32:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The unfortunate commit d95138e (setup: set env $GIT_WORK_TREE when\nwork tree is set, like $GIT_DIR - 2015-06-26) exposes another problem,\nbesides git-clone that's described in the previous commit. If\nGIT_WORK_TREE (or even GIT_DIR) is exported to an alias script, it may\nmislead git commands in the script where the repo is. Granted, most\nscripts work on the repo where the alias is summoned from. But nowhere\ndo we forbid the script to visit another repository.\n\nThe revert of d95138e in the previous commit is sufficient as a\nfix. However, to protect us from accidentally leaking GIT_*\nenvironment variables again, we restore certain sensitive env before\ncalling the external script.\n\nGIT_PREFIX is let through because there's another setup side effect\nthat we simply accepted so far: current working directory is\nmoved. Maybe in future we can introduce a new alias format that\nguarantees no cwd move, then we can unexport GIT_PREFIX.\n\nReported-by: Gabriel Ganne \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Let's hope there will be no third report about this commit..\n\n git.c | 10 +++++++---\n t/t0001-init.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex e2f187d..83b6c56 100644\n--- a/git.c\n+++ b/git.c\n@@ -41,13 +41,16 @@ static void save_env_before_alias(void)\n \t}\n }\n \n-static void restore_env(void)\n+static void restore_env(int external_alias)\n {\n \tint i;\n-\tif (orig_cwd && chdir(orig_cwd))\n+\tif (!external_alias && orig_cwd && chdir(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfree(orig_cwd);\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n+\t\tif (external_alias &&\n+\t\t !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n+\t\t\tcontinue;\n \t\tif (orig_env[i])\n \t\t\tsetenv(env_names[i], orig_env[i], 1);\n \t\telse\n@@ -244,6 +247,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tint argc = *argcp, i;\n \n \t\t\tcommit_pager_choice();\n+\t\t\trestore_env(1);\n \n \t\t\t/* build alias_argv */\n \t\t\talias_argv = xmalloc(sizeof(*alias_argv) * (argc + 1));\n@@ -531,7 +535,7 @@ static void handle_builtin(int argc, const char **argv)\n \tbuiltin = get_builtin(cmd);\n \tif (builtin) {\n \t\tif (saved_env_before_alias)\n-\t\t\trestore_env();\n+\t\t\trestore_env(0);\n \t\telse\n \t\t\texit(run_builtin(builtin, argc, argv));\n \t}\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex f91bbcf..19539fc 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -87,6 +87,33 @@ test_expect_success 'plain nested in bare through aliased command' '\n \tcheck_config bare-ancestor-aliased.git/plain-nested/.git false unset\n '\n \n+test_expect_success 'No extra GIT_* on alias scripts' '\n+\tcat <<-\\EOF >expected &&\n+\tGIT_ATTR_NOSYSTEM\n+\tGIT_AUTHOR_EMAIL\n+\tGIT_AUTHOR_NAME\n+\tGIT_COMMITTER_EMAIL\n+\tGIT_COMMITTER_NAME\n+\tGIT_CONFIG_NOSYSTEM\n+\tGIT_EXEC_PATH\n+\tGIT_MERGE_AUTOEDIT\n+\tGIT_MERGE_VERBOSITY\n+\tGIT_PREFIX\n+\tGIT_TEMPLATE_DIR\n+\tGIT_TEXTDOMAINDIR\n+\tGIT_TRACE_BARE\n+\tEOF\n+\tcat <<-\\EOF >script &&\n+\t#!/bin/sh\n+\tenv | grep GIT_ | sed \"s/=.*//\" | sort >actual\n+\texit 0\n+\tEOF\n+\tchmod 755 script &&\n+\tgit config alias.script \\!./script &&\n+\t( mkdir sub && cd sub && git script ) &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'plain with GIT_WORK_TREE' '\n \tmkdir plain-wt &&\n \ttest_must_fail env GIT_WORK_TREE=\"$(pwd)/plain-wt\" git init plain-wt\n-- \n2.2.0.513.g477eb31\n"},{"id":"274057","messageId":"CACsJy8BKbzzE7D0a2HqjU3kvz1SitcJyVxZH=aWAc+bBd8MmQg@mail.gmail.com","threadId":"40865","inReplyTo":"1449166676-30845-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-05T19:12:17Z","receivedAt":"2015-12-05T19:12:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 3, 2015 at 7:17 PM, Nguyễn Thái Ngọc Duy wrote:\n> The solution in d95138e is reverted in this commit. Instead we reuse the\n> solution from c056261 [4]. c056261 fixes another setup-messed-up-by-alias\n> by saving and restoring env for git-clone and git-init. Now I conclude\n> that setup-messed-by-alias is always evil. So the env restoration is\n> done for _all_ commands whenever aliases are involved. It fixes what\n> d95138e tries to fix. And it does not upset git-clone-inside-hooks.\n\n(Reviewer hat on) I wrote env restoration is done for all commands,\nbut the patch is actually about all _builtin_ commands. External ones\ndo not receive this treatment. FIx is in the next reroll.\n-- \nDuy\n"},{"id":"274119","messageId":"xmqq4mfunm75.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"CACsJy8BKbzzE7D0a2HqjU3kvz1SitcJyVxZH=aWAc+bBd8MmQg@mail.gmail.com","subject":"Re: [PATCH 2/2] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T18:33:02Z","receivedAt":"2015-12-07T18:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, Dec 3, 2015 at 7:17 PM, Nguyễn Thái Ngọc Duy wrote:\n>> The solution in d95138e is reverted in this commit. Instead we reuse the\n>> solution from c056261 [4]. c056261 fixes another setup-messed-up-by-alias\n>> by saving and restoring env for git-clone and git-init. Now I conclude\n>> that setup-messed-by-alias is always evil. So the env restoration is\n>> done for _all_ commands whenever aliases are involved. It fixes what\n>> d95138e tries to fix. And it does not upset git-clone-inside-hooks.\n>\n> (Reviewer hat on) I wrote env restoration is done for all commands,\n> but the patch is actually about all _builtin_ commands. External ones\n> do not receive this treatment. FIx is in the next reroll.\n\nThanks for being careful.\n"},{"id":"274122","messageId":"xmqqzixmm6na.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"1449329538-18623-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T18:54:17Z","receivedAt":"2015-12-07T18:54:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Let's hope there will be no third report about this commit..\n\nHmm, why does this additional test fail only under prove but pass\nwithout it?\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index f91bbcf..19539fc 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -87,6 +87,33 @@ test_expect_success 'plain nested in bare through aliased command' '\n> \tcheck_config bare-ancestor-aliased.git/plain-nested/.git false unset\n> '\n> \n> +test_expect_success 'No extra GIT_* on alias scripts' '\n> +\tcat <<-\\EOF >expected &&\n> +\tGIT_ATTR_NOSYSTEM\n> +\tGIT_AUTHOR_EMAIL\n> +\tGIT_AUTHOR_NAME\n> +\tGIT_COMMITTER_EMAIL\n> +\tGIT_COMMITTER_NAME\n> +\tGIT_CONFIG_NOSYSTEM\n> +\tGIT_EXEC_PATH\n> +\tGIT_MERGE_AUTOEDIT\n> +\tGIT_MERGE_VERBOSITY\n> +\tGIT_PREFIX\n> +\tGIT_TEMPLATE_DIR\n> +\tGIT_TEXTDOMAINDIR\n> +\tGIT_TRACE_BARE\n> +\tEOF\n> +\tcat <<-\\EOF >script &&\n> +\t#!/bin/sh\n> +\tenv | grep GIT_ | sed \"s/=.*//\" | sort >actual\n\nThis is more about coding discipline than style, but piping grep\noutput to sed is wasteful. \"sed -ne '/^GIT_/s/=.*//p'\" or something\nlike that, perhaps?\n\nI wondered what happens if the user has an unrelated stray variable\nwhose name happens to begin with GIT_ in her environment, but it\nturns out that we cleanse them in test-lib.sh fairly early, so that\nwould be fine. You need to tighten your \"grep\" pattern, though.\n\n> +\texit 0\n> +\tEOF\n> +\tchmod 755 script &&\n> +\tgit config alias.script \\!./script &&\n> +\t( mkdir sub && cd sub && git script ) &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success 'plain with GIT_WORK_TREE' '\n> \tmkdir plain-wt &&\n> \ttest_must_fail env GIT_WORK_TREE=\"$(pwd)/plain-wt\" git init plain-wt\n"},{"id":"274174","messageId":"CACsJy8BHT2+_i4SoG6xv40wGX_gxwZFk=3WYOH196ZiHA7raWQ@mail.gmail.com","threadId":"40865","inReplyTo":"xmqqzixmm6na.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-08T16:55:20Z","receivedAt":"2015-12-08T16:55:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 7, 2015 at 7:54 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Let's hope there will be no third report about this commit..\n>\n> Hmm, why does this additional test fail only under prove but pass\n> without it?\n\nIt passes with prove for me. Some mysterious variable leaks through somehow?\n\n>> + env | grep GIT_ | sed \"s/=.*//\" | sort >actual\n>\n> This is more about coding discipline than style, but piping grep\n> output to sed is wasteful. \"sed -ne '/^GIT_/s/=.*//p'\" or something\n> like that, perhaps?\n\nOK will fix.\n\n> I wondered what happens if the user has an unrelated stray variable\n> whose name happens to begin with GIT_ in her environment, but it\n> turns out that we cleanse them in test-lib.sh fairly early, so that\n> would be fine. You need to tighten your \"grep\" pattern, though.\n\nOK\n-- \nDuy\n"},{"id":"274184","messageId":"20151208172010.GA12886@sigill.intra.peff.net","threadId":"40865","inReplyTo":"CACsJy8BHT2+_i4SoG6xv40wGX_gxwZFk=3WYOH196ZiHA7raWQ@mail.gmail.com","subject":"Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-08T17:20:10Z","receivedAt":"2015-12-08T17:20:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 08, 2015 at 05:55:20PM +0100, Duy Nguyen wrote:\n\n> On Mon, Dec 7, 2015 at 7:54 PM, Junio C Hamano wrote:\n> > Nguyễn Thái Ngọc Duy writes:\n> >\n> >> Let's hope there will be no third report about this commit..\n> >\n> > Hmm, why does this additional test fail only under prove but pass\n> > without it?\n> \n> It passes with prove for me. Some mysterious variable leaks through somehow?\n\nIt fails for me when run via \"make\" (with prove or without) but not as\n\"./t0001-init.sh\". Looks like extra variables from my config.mak leak\nthrough:\n\n $ make t0001-init.sh GIT_TEST_OPTS=\"-v -i\"\n [...]\n --- expected 2015-12-08 17:18:06.304699181 +0000\n +++ actual 2015-12-08 17:18:06.312699180 +0000\n @@ -9,5 +9,9 @@\n GIT_MERGE_VERBOSITY\n GIT_PREFIX\n GIT_TEMPLATE_DIR\n +GIT_TEST_GIT_DAEMON\n +GIT_TEST_HTTPD\n +GIT_TEST_OPTS\n GIT_TEXTDOMAINDIR\n GIT_TRACE_BARE\n +MAKEFLAGS\n not ok 6 - No extra GIT_* on alias scripts\n\nAny GIT_TEST_* is allowed through by test-lib.sh.\n\nFor the MAKEFLAGS one, I suspect it's hitting\n\n MAKEFLAGS=GIT_TEST_OPTS=...\n\nYou should probably anchor your regex. :)\n\n-Peff\n"},{"id":"274197","messageId":"xmqq1tawjy1f.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"20151208172010.GA12886@sigill.intra.peff.net","subject":"Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-08T23:55:24Z","receivedAt":"2015-12-08T23:55:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It fails for me when run via \"make\" (with prove or without) but not as\n> \"./t0001-init.sh\". Looks like extra variables from my config.mak leak\n> through:\n>\n> $ make t0001-init.sh GIT_TEST_OPTS=\"-v -i\"\n> [...]\n> --- expected 2015-12-08 17:18:06.304699181 +0000\n> +++ actual 2015-12-08 17:18:06.312699180 +0000\n> @@ -9,5 +9,9 @@\n> GIT_MERGE_VERBOSITY\n> GIT_PREFIX\n> GIT_TEMPLATE_DIR\n> +GIT_TEST_GIT_DAEMON\n> +GIT_TEST_HTTPD\n> +GIT_TEST_OPTS\n> GIT_TEXTDOMAINDIR\n> GIT_TRACE_BARE\n> +MAKEFLAGS\n> not ok 6 - No extra GIT_* on alias scripts\n>\n> Any GIT_TEST_* is allowed through by test-lib.sh.\n\nAlso GIT_PROVE_OPTS (which caused false positive for me).\n\nPerhaps grab \"env\" output outside the alias to create the expected\n(instead of a random handcrafted list that you have to maintain as\nthe test suite evolves), and compare it from within the alias, or\nsomething?\n"},{"id":"274775","messageId":"1450597819-26278-1-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"1449166676-30845-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-20T07:50:16Z","receivedAt":"2015-12-20T07:50:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Changes since v1:\n\n - make sure we save/restore env for external commands in 2/3\n - fix t0001 test in 3/3\n\nInterdiff:\n\ndiff --git b/git.c a/git.c\nindex 83b6c56..da278c3 100644\n--- b/git.c\n+++ a/git.c\n@@ -229,7 +229,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n static int handle_alias(int *argcp, const char ***argv)\n {\n \tint envchanged = 0, ret = 0, saved_errno = errno;\n-\tconst char *subdir;\n \tint count, option_count;\n \tconst char **new_argv;\n \tconst char *alias_command;\n@@ -237,7 +236,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \tint unused_nongit;\n \n \tsave_env_before_alias();\n-\tsubdir = setup_git_directory_gently(&unused_nongit);\n+\tsetup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n \talias_string = alias_lookup(alias_command);\n@@ -296,8 +295,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\tif (subdir && chdir(subdir))\n-\t\tdie_errno(\"Cannot change to '%s'\", subdir);\n+\trestore_env(0);\n \n \terrno = saved_errno;\n \n@@ -534,9 +532,13 @@ static void handle_builtin(int argc, const char **argv)\n \n \tbuiltin = get_builtin(cmd);\n \tif (builtin) {\n-\t\tif (saved_env_before_alias)\n-\t\t\trestore_env(0);\n-\t\telse\n+\t\t/*\n+\t\t * XXX: if we can figure out cases where it is _safe_\n+\t\t * to do, we can avoid spawning a new process when\n+\t\t * saved_env_before_alias is true\n+\t\t * (i.e. setup_git_dir* has been run once)\n+\t\t */\n+\t\tif (!saved_env_before_alias)\n \t\t\texit(run_builtin(builtin, argc, argv));\n \t}\n }\ndiff --git b/t/t0001-init.sh a/t/t0001-init.sh\nindex 19539fc..295aa59 100755\n--- b/t/t0001-init.sh\n+++ a/t/t0001-init.sh\n@@ -88,24 +88,14 @@ test_expect_success 'plain nested in bare through aliased command' '\n '\n \n test_expect_success 'No extra GIT_* on alias scripts' '\n-\tcat <<-\\EOF >expected &&\n-\tGIT_ATTR_NOSYSTEM\n-\tGIT_AUTHOR_EMAIL\n-\tGIT_AUTHOR_NAME\n-\tGIT_COMMITTER_EMAIL\n-\tGIT_COMMITTER_NAME\n-\tGIT_CONFIG_NOSYSTEM\n-\tGIT_EXEC_PATH\n-\tGIT_MERGE_AUTOEDIT\n-\tGIT_MERGE_VERBOSITY\n-\tGIT_PREFIX\n-\tGIT_TEMPLATE_DIR\n-\tGIT_TEXTDOMAINDIR\n-\tGIT_TRACE_BARE\n-\tEOF\n+\t(\n+\t\tenv | sed -ne \"/^GIT_/s/=.*//p\" &&\n+\t\techo GIT_PREFIX && # setup.c\n+\t\techo GIT_TEXTDOMAINDIR # wrapper-for-bin.sh\n+\t) | sort | uniq >expected &&\n \tcat <<-\\EOF >script &&\n \t#!/bin/sh\n-\tenv | grep GIT_ | sed \"s/=.*//\" | sort >actual\n+\tenv | sed -ne \"/^GIT_/s/=.*//p\" | sort >actual\n \texit 0\n \tEOF\n \tchmod 755 script &&\n\nNguyễn Thái Ngọc Duy (3):\n git.c: make it clear save_env() is for alias handling only\n setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..\n git.c: make sure we do not leak GIT_* to alias scripts\n\n environment.c | 2 --\n git.c | 41 +++++++++++++++++++++++------------------\n t/t0001-init.sh | 17 +++++++++++++++++\n t/t5601-clone.sh | 23 +++++++++++++++++++++++\n 4 files changed, 63 insertions(+), 20 deletions(-)\n\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"274774","messageId":"1450597819-26278-2-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"1450597819-26278-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/3] git.c: make it clear save_env() is for alias handling only","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-20T07:50:17Z","receivedAt":"2015-12-20T07:50:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n git.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 6ed824c..6ce7043 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,14 +25,14 @@ static const char *env_names[] = {\n \tGIT_PREFIX_ENVIRONMENT\n };\n static char *orig_env[4];\n-static int saved_environment;\n+static int saved_env_before_alias;\n \n-static void save_env(void)\n+static void save_env_before_alias(void)\n {\n \tint i;\n-\tif (saved_environment)\n+\tif (saved_env_before_alias)\n \t\treturn;\n-\tsaved_environment = 1;\n+\tsaved_env_before_alias = 1;\n \torig_cwd = xgetcwd();\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -233,6 +233,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \tchar *alias_string;\n \tint unused_nongit;\n \n+\tsave_env_before_alias();\n \tsubdir = setup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n@@ -530,7 +531,7 @@ static void handle_builtin(int argc, const char **argv)\n \n \tbuiltin = get_builtin(cmd);\n \tif (builtin) {\n-\t\tif (saved_environment && (builtin->option & NO_SETUP))\n+\t\tif (saved_env_before_alias && (builtin->option & NO_SETUP))\n \t\t\trestore_env();\n \t\telse\n \t\t\texit(run_builtin(builtin, argc, argv));\n@@ -590,7 +591,6 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t */\n \t\tif (done_alias)\n \t\t\tbreak;\n-\t\tsave_env();\n \t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n \t\tdone_alias = 1;\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"274776","messageId":"1450597819-26278-3-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"1450597819-26278-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-20T07:50:18Z","receivedAt":"2015-12-20T07:50:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit d95138e [1] fixes a .git file problem by setting GIT_WORK_TREE\nwhenever GIT_DIR is set. It sounded harmless because we handle\nGIT_DIR and GIT_WORK_TREE side by side for most commands, with two\nexceptions: git-init and git-clone.\n\n\"git clone\" is not happy with d95138e. This command ignores GIT_DIR\nbut respects GIT_WORK_TREE [2] [3] which means it used to run fine\nfrom a hook, where GIT_DIR was set but GIT_WORK_TREE was not (*). With\nd95138e, GIT_WORK_TREE is set all the time and git-clone interprets\nthat as \"I give you order to put the worktree here\", usually against\nthe user's intention.\n\nThe solution in d95138e is reverted in this commit. Instead we reuse\nthe solution from c056261 [4]. c056261 fixes another setup-messed-\nup-by-alias by saving and restoring env and spawning a new process,\nbut for git-clone and git-init only. Now I conclude that setup-messed-\nup-by-alias is always evil. So the env restoration is done for _all_\ncommands, including external ones, whenever aliases are involved. It\nfixes what d95138e tries to fix. And it does not upset git-clone-\ninside-hooks.\n\nThe test from d95138e remains to verify it's not broken by this. A new\ntest is added to make sure git-clone-inside-hooks remains happy.\n\n(*) GIT_WORK_TREE was not set _most of the time_. In some cases\n GIT_WORK_TREE is set and git-clone will behave differently. The\n use of GIT_WORK_TREE to direct git-clone to put work tree\n elsewhere looks like a mistake because it causes surprises this\n way. But that's a separate story.\n\n[1] d95138e (setup: set env $GIT_WORK_TREE when work tree is set, like\n $GIT_DIR - 2015-06-26)\n[2] 2beebd2 (clone: create intermediate directories of destination\n repo - 2008-06-25)\n[3] 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06)\n[4] c056261 (git potty: restore environments after alias expansion -\n 2014-06-08)\n\nReported-by: Anthony Sottile \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n environment.c | 2 --\n git.c | 23 ++++++++++++-----------\n t/t5601-clone.sh | 23 +++++++++++++++++++++++\n 3 files changed, 35 insertions(+), 13 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 2da7fe2..1cc4aab 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -235,8 +235,6 @@ void set_git_work_tree(const char *new_work_tree)\n \t}\n \tgit_work_tree_initialized = 1;\n \twork_tree = xstrdup(real_path(new_work_tree));\n-\tif (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))\n-\t\tdie(\"could not set GIT_WORK_TREE to '%s'\", work_tree);\n }\n \n const char *get_git_work_tree(void)\ndiff --git a/git.c b/git.c\nindex 6ce7043..1a7d399 100644\n--- a/git.c\n+++ b/git.c\n@@ -226,7 +226,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n static int handle_alias(int *argcp, const char ***argv)\n {\n \tint envchanged = 0, ret = 0, saved_errno = errno;\n-\tconst char *subdir;\n \tint count, option_count;\n \tconst char **new_argv;\n \tconst char *alias_command;\n@@ -234,7 +233,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \tint unused_nongit;\n \n \tsave_env_before_alias();\n-\tsubdir = setup_git_directory_gently(&unused_nongit);\n+\tsetup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n \talias_string = alias_lookup(alias_command);\n@@ -292,8 +291,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\tif (subdir && chdir(subdir))\n-\t\tdie_errno(\"Cannot change to '%s'\", subdir);\n+\trestore_env();\n \n \terrno = saved_errno;\n \n@@ -308,7 +306,6 @@ static int handle_alias(int *argcp, const char ***argv)\n * RUN_SETUP for reading from the configuration file.\n */\n #define NEED_WORK_TREE\t\t(1<<3)\n-#define NO_SETUP\t\t(1<<4)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -390,7 +387,7 @@ static struct cmd_struct commands[] = {\n \t{ \"cherry\", cmd_cherry, RUN_SETUP },\n \t{ \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"clean\", cmd_clean, RUN_SETUP | NEED_WORK_TREE },\n-\t{ \"clone\", cmd_clone, NO_SETUP },\n+\t{ \"clone\", cmd_clone },\n \t{ \"column\", cmd_column, RUN_SETUP_GENTLY },\n \t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n@@ -416,8 +413,8 @@ static struct cmd_struct commands[] = {\n \t{ \"hash-object\", cmd_hash_object },\n \t{ \"help\", cmd_help },\n \t{ \"index-pack\", cmd_index_pack, RUN_SETUP_GENTLY },\n-\t{ \"init\", cmd_init_db, NO_SETUP },\n-\t{ \"init-db\", cmd_init_db, NO_SETUP },\n+\t{ \"init\", cmd_init_db },\n+\t{ \"init-db\", cmd_init_db },\n \t{ \"interpret-trailers\", cmd_interpret_trailers, RUN_SETUP_GENTLY },\n \t{ \"log\", cmd_log, RUN_SETUP },\n \t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n@@ -531,9 +528,13 @@ static void handle_builtin(int argc, const char **argv)\n \n \tbuiltin = get_builtin(cmd);\n \tif (builtin) {\n-\t\tif (saved_env_before_alias && (builtin->option & NO_SETUP))\n-\t\t\trestore_env();\n-\t\telse\n+\t\t/*\n+\t\t * XXX: if we can figure out cases where it is _safe_\n+\t\t * to do, we can avoid spawning a new process when\n+\t\t * saved_env_before_alias is true\n+\t\t * (i.e. setup_git_dir* has been run once)\n+\t\t */\n+\t\tif (!saved_env_before_alias)\n \t\t\texit(run_builtin(builtin, argc, argv));\n \t}\n }\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 9b34f3c..31b4658 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -65,6 +65,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n+test_expect_success 'clone from hooks' '\n+\n+\ttest_create_repo r0 &&\n+\tcd r0 &&\n+\ttest_commit initial &&\n+\tcd .. &&\n+\tgit init r1 &&\n+\tcd r1 &&\n+\tcat >.git/hooks/pre-commit <<-\\EOF &&\n+\t#!/bin/sh\n+\tgit clone ../r0 ../r2\n+\texit 1\n+\tEOF\n+\tchmod u+x .git/hooks/pre-commit &&\n+\t: >file &&\n+\tgit add file &&\n+\ttest_must_fail git commit -m invoke-hook &&\n+\tcd .. &&\n+\ttest_cmp r0/.git/HEAD r2/.git/HEAD &&\n+\ttest_cmp r0/initial.t r2/initial.t\n+\n+'\n+\n test_expect_success 'clone creates intermediate directories' '\n \n \tgit clone src long/path/to/dst &&\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"274777","messageId":"1450597819-26278-4-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"1450597819-26278-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] git.c: make sure we do not leak GIT_* to alias scripts","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-20T07:50:19Z","receivedAt":"2015-12-20T07:50:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The unfortunate commit d95138e (setup: set env $GIT_WORK_TREE when\nwork tree is set, like $GIT_DIR - 2015-06-26) exposes another problem,\nbesides git-clone that's described in the previous commit. If\nGIT_WORK_TREE (or even GIT_DIR) is exported to an alias script, it may\nmislead git commands in the script where the repo is. Granted, most\nscripts work on the repo where the alias is summoned from. But nowhere\ndo we forbid the script to visit another repository.\n\nThe revert of d95138e in the previous commit is sufficient as a\nfix. However, to protect us from accidentally leaking GIT_*\nenvironment variables again, we restore certain sensitive env before\ncalling the external script.\n\nGIT_PREFIX is let through because there's another setup side effect\nthat we simply accepted so far: current working directory is\nmoved. Maybe in future we can introduce a new alias format that\nguarantees no cwd move, then we can unexport GIT_PREFIX.\n\nReported-by: Gabriel Ganne \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n git.c | 10 +++++++---\n t/t0001-init.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 1a7d399..da278c3 100644\n--- a/git.c\n+++ b/git.c\n@@ -41,13 +41,16 @@ static void save_env_before_alias(void)\n \t}\n }\n \n-static void restore_env(void)\n+static void restore_env(int external_alias)\n {\n \tint i;\n-\tif (orig_cwd && chdir(orig_cwd))\n+\tif (!external_alias && orig_cwd && chdir(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfree(orig_cwd);\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n+\t\tif (external_alias &&\n+\t\t !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))\n+\t\t\tcontinue;\n \t\tif (orig_env[i])\n \t\t\tsetenv(env_names[i], orig_env[i], 1);\n \t\telse\n@@ -243,6 +246,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tint argc = *argcp, i;\n \n \t\t\tcommit_pager_choice();\n+\t\t\trestore_env(1);\n \n \t\t\t/* build alias_argv */\n \t\t\talias_argv = xmalloc(sizeof(*alias_argv) * (argc + 1));\n@@ -291,7 +295,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\trestore_env();\n+\trestore_env(0);\n \n \terrno = saved_errno;\n \ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex f91bbcf..295aa59 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -87,6 +87,23 @@ test_expect_success 'plain nested in bare through aliased command' '\n \tcheck_config bare-ancestor-aliased.git/plain-nested/.git false unset\n '\n \n+test_expect_success 'No extra GIT_* on alias scripts' '\n+\t(\n+\t\tenv | sed -ne \"/^GIT_/s/=.*//p\" &&\n+\t\techo GIT_PREFIX && # setup.c\n+\t\techo GIT_TEXTDOMAINDIR # wrapper-for-bin.sh\n+\t) | sort | uniq >expected &&\n+\tcat <<-\\EOF >script &&\n+\t#!/bin/sh\n+\tenv | sed -ne \"/^GIT_/s/=.*//p\" | sort >actual\n+\texit 0\n+\tEOF\n+\tchmod 755 script &&\n+\tgit config alias.script \\!./script &&\n+\t( mkdir sub && cd sub && git script ) &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'plain with GIT_WORK_TREE' '\n \tmkdir plain-wt &&\n \ttest_must_fail env GIT_WORK_TREE=\"$(pwd)/plain-wt\" git init plain-wt\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"274799","messageId":"1450693372-6863-1-git-send-email-pclouds@gmail.com","threadId":"40865","inReplyTo":"xmqqh9k06all.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] Revert \"setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-21T10:22:52Z","receivedAt":"2015-12-21T10:22:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This commit has caused three regression reports so far. All of them are\nabout spawning git subprocesses, where the new presence of GIT_WORK_TREE\neither changes command behaviour (git-init or git-clone), or how\nrepo/worktree is detected (from aliases), with or without $GIT_DIR.\nThe original bug will be re-fixed another way.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano wrote:\n > OK, when/if you decide that our first step should be a revert of\n > d95138e please send in a patch to do so with a brief write-up of a\n > follow-up plan.\n\n Three reports to me are enough. And I obviously could not push the\n fix out fast enough. So if you want to revert it, here's the patch on\n maint.\n\n environment.c | 2 --\n t/t0002-gitfile.sh | 2 +-\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 23a38e4..f1a2a49 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -238,8 +238,6 @@ void set_git_work_tree(const char *new_work_tree)\n \t}\n \tgit_work_tree_initialized = 1;\n \twork_tree = xstrdup(real_path(new_work_tree));\n-\tif (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))\n-\t\tdie(\"could not set GIT_WORK_TREE to '%s'\", work_tree);\n }\n \n const char *get_git_work_tree(void)\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex 9670e8c..3afe012 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -99,7 +99,7 @@ test_expect_success 'check rev-list' '\n \ttest \"$SHA\" = \"$(git rev-list HEAD)\"\n '\n \n-test_expect_success 'setup_git_dir twice in subdir' '\n+test_expect_failure 'setup_git_dir twice in subdir' '\n \tgit init sgd &&\n \t(\n \t\tcd sgd &&\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"274811","messageId":"xmqqegefhftq.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"1450693372-6863-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Revert \"setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T17:28:01Z","receivedAt":"2015-12-21T17:28:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> This commit has caused three regression reports so far. All of them are\n> about spawning git subprocesses, where the new presence of GIT_WORK_TREE\n> either changes command behaviour (git-init or git-clone), or how\n> repo/worktree is detected (from aliases), with or without $GIT_DIR.\n> The original bug will be re-fixed another way.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano wrote:\n> > OK, when/if you decide that our first step should be a revert of\n> > d95138e please send in a patch to do so with a brief write-up of a\n> > follow-up plan.\n>\n> Three reports to me are enough. And I obviously could not push the\n> fix out fast enough. So if you want to revert it, here's the patch on\n> maint.\n\nThe timing of this is a bit unfortunate, as d95138e is a regression\nsince v2.5.1 throughout v2.6 series. I'll push this out on maint\nand onwards for now. I do not think we added new things that rely\non the post-d95138e \"broken\" behaviour in-tree, but this reversion\nmight introduce another unexpected regression X-<. We'll see.\n\nThanks.\n\n>\n> environment.c | 2 --\n> t/t0002-gitfile.sh | 2 +-\n> 2 files changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/environment.c b/environment.c\n> index 23a38e4..f1a2a49 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -238,8 +238,6 @@ void set_git_work_tree(const char *new_work_tree)\n> \t}\n> \tgit_work_tree_initialized = 1;\n> \twork_tree = xstrdup(real_path(new_work_tree));\n> -\tif (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))\n> -\t\tdie(\"could not set GIT_WORK_TREE to '%s'\", work_tree);\n> }\n> \n> const char *get_git_work_tree(void)\n> diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\n> index 9670e8c..3afe012 100755\n> --- a/t/t0002-gitfile.sh\n> +++ b/t/t0002-gitfile.sh\n> @@ -99,7 +99,7 @@ test_expect_success 'check rev-list' '\n> \ttest \"$SHA\" = \"$(git rev-list HEAD)\"\n> '\n> \n> -test_expect_success 'setup_git_dir twice in subdir' '\n> +test_expect_failure 'setup_git_dir twice in subdir' '\n> \tgit init sgd &&\n> \t(\n> \t\tcd sgd &&\n"},{"id":"274817","messageId":"xmqqtwnbfybj.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"xmqqegefhftq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Revert \"setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T18:31:28Z","receivedAt":"2015-12-21T18:31:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> This commit has caused three regression reports so far. All of them are\n>> about spawning git subprocesses, where the new presence of GIT_WORK_TREE\n>> either changes command behaviour (git-init or git-clone), or how\n>> repo/worktree is detected (from aliases), with or without $GIT_DIR.\n>> The original bug will be re-fixed another way.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano wrote:\n>> > OK, when/if you decide that our first step should be a revert of\n>> > d95138e please send in a patch to do so with a brief write-up of a\n>> > follow-up plan.\n>>\n>> Three reports to me are enough. And I obviously could not push the\n>> fix out fast enough. So if you want to revert it, here's the patch on\n>> maint.\n\nAlso, can you reference these three reports for future reference?\n\nThanks.\n"},{"id":"274828","messageId":"xmqq4mfbfqla.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"1450597819-26278-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T21:18:25Z","receivedAt":"2015-12-21T21:18:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. I wiggled these three on top of the \"Revert the earlier\none\"; while I think the result is correct, I'd appreciate if you can\ndouble check the result when I push the topic out later today.\n"},{"id":"274836","messageId":"CACsJy8DP7ugh1UAU=iGzzHTSwUm5ZPkQhS0fTvkOpQV6vP1Jxg@mail.gmail.com","threadId":"40865","inReplyTo":"xmqqtwnbfybj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Revert \"setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-22T01:06:20Z","receivedAt":"2015-12-22T01:06:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 22, 2015 at 1:31 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>>> This commit has caused three regression reports so far. All of them are\n>>> about spawning git subprocesses, where the new presence of GIT_WORK_TREE\n>>> either changes command behaviour (git-init or git-clone), or how\n>>> repo/worktree is detected (from aliases), with or without $GIT_DIR.\n>>> The original bug will be re-fixed another way.\n>>>\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>> ---\n>>> On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano wrote:\n>>> > OK, when/if you decide that our first step should be a revert of\n>>> > d95138e please send in a patch to do so with a brief write-up of a\n>>> > follow-up plan.\n>>>\n>>> Three reports to me are enough. And I obviously could not push the\n>>> fix out fast enough. So if you want to revert it, here's the patch on\n>>> maint.\n>\n> Also, can you reference these three reports for future reference?\n\nhttp://article.gmane.org/gmane.comp.version-control.git/281608\nhttp://article.gmane.org/gmane.comp.version-control.git/281979\nhttp://article.gmane.org/gmane.comp.version-control.git/282691\n\nThe last one is not confirmed by the reporter yet. But I'm pretty sure\ni'll trigger the special case \"when GIT_WORK_TREE is set but GIT_DIR\nis not\" in setup code\n-- \nDuy\n"},{"id":"274849","messageId":"CACsJy8DFmZSa2x4y2fDwVsvwa5uAuMJn8v=utvYtAPTGFbdWPg@mail.gmail.com","threadId":"40865","inReplyTo":"xmqq4mfbfqla.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-22T10:57:09Z","receivedAt":"2015-12-22T10:57:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano wrote:\n> Thanks. I wiggled these three on top of the \"Revert the earlier\n> one\"; while I think the result is correct, I'd appreciate if you can\n> double check the result when I push the topic out later today.\n\nLooks good. \"prove\" passed too by the way.\n-- \nDuy\n"},{"id":"274852","messageId":"CACsJy8A5AcRj2HiLe3PQijhYcHMzJ6eEuMyeVTMvPtXvMg_Sug@mail.gmail.com","threadId":"40865","inReplyTo":"CACsJy8DFmZSa2x4y2fDwVsvwa5uAuMJn8v=utvYtAPTGFbdWPg@mail.gmail.com","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-22T11:53:16Z","receivedAt":"2015-12-22T11:53:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 22, 2015 at 5:57 PM, Duy Nguyen wrote:\n> On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano wrote:\n>> Thanks. I wiggled these three on top of the \"Revert the earlier\n>> one\"; while I think the result is correct, I'd appreciate if you can\n>> double check the result when I push the topic out later today.\n>\n> Looks good. \"prove\" passed too by the way.\n\nAnother by the way, this \"forcing aliases as external commands\" now\nshows something like \"error: git-log died of signal 13\" when the pager\nexits early, for an alias like \"l1 = log --oneline\". It's probably not\na regression because other external git commands with pager should\nshow the same message. But I haven't checked thoroughly yet.\n-- \nDuy\n"},{"id":"274902","messageId":"xmqqd1tye4i8.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"CACsJy8A5AcRj2HiLe3PQijhYcHMzJ6eEuMyeVTMvPtXvMg_Sug@mail.gmail.com","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-22T18:13:03Z","receivedAt":"2015-12-22T18:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Dec 22, 2015 at 5:57 PM, Duy Nguyen wrote:\n>> On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano wrote:\n>>> Thanks. I wiggled these three on top of the \"Revert the earlier\n>>> one\"; while I think the result is correct, I'd appreciate if you can\n>>> double check the result when I push the topic out later today.\n>>\n>> Looks good. \"prove\" passed too by the way.\n>\n> Another by the way, this \"forcing aliases as external commands\" now\n> shows something like \"error: git-log died of signal 13\" when the pager\n> exits early, for an alias like \"l1 = log --oneline\".\n\n... and we do not show that when we directly call \"git log\" is...?\n\nWe do signal this with non-zero exit status like so:\n\n\t$ GIT_PAGER=true git log --oneline ; echo $?\n 141\n\nand it is not surprising that the one that is catching the exit\nstatus of what was spawned and reporting \"signal 13\".\n"},{"id":"274919","messageId":"xmqqk2o6cfvv.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"CACsJy8DP7ugh1UAU=iGzzHTSwUm5ZPkQhS0fTvkOpQV6vP1Jxg@mail.gmail.com","subject":"Re: [PATCH] Revert \"setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-22T21:50:12Z","receivedAt":"2015-12-22T21:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Dec 22, 2015 at 1:31 AM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> Nguyễn Thái Ngọc Duy writes:\n>>>\n>>>> This commit has caused three regression reports so far. All of them are\n>>>> about spawning git subprocesses, where the new presence of GIT_WORK_TREE\n>>>> either changes command behaviour (git-init or git-clone), or how\n>>>> repo/worktree is detected (from aliases), with or without $GIT_DIR.\n>>>> The original bug will be re-fixed another way.\n>>>>\n>>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>>> ---\n>>>> On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano wrote:\n>>>> > OK, when/if you decide that our first step should be a revert of\n>>>> > d95138e please send in a patch to do so with a brief write-up of a\n>>>> > follow-up plan.\n>>>>\n>>>> Three reports to me are enough. And I obviously could not push the\n>>>> fix out fast enough. So if you want to revert it, here's the patch on\n>>>> maint.\n>>\n>> Also, can you reference these three reports for future reference?\n>\n> http://article.gmane.org/gmane.comp.version-control.git/281608\n> http://article.gmane.org/gmane.comp.version-control.git/281979\n> http://article.gmane.org/gmane.comp.version-control.git/282691\n>\n> The last one is not confirmed by the reporter yet. But I'm pretty sure\n> i'll trigger the special case \"when GIT_WORK_TREE is set but GIT_DIR\n> is not\" in setup code\n\nThanks, I'll leave these breadcrumbs in the log message for future\nreference.\n\nI think the last sentence of the original commit is telling how this\nbug came about. \"It does not harm if $GIT_WORK_TREE is set while\n$GIT_DIR is not.\" forgets to consider the possibility that scripts\nmay be relying on the \"Go to the top of the working tree and setting\nGIT_DIR would give you a reasonable environment\". That is true if\nGIT_WORK_TREE is not set, and these scripts weren't getting the\nenvironment exported [*1*]. These scripts now have to unset\nGIT_WORK_TREE themselves (or set it to their $cwd if they are indeed\nat the top), just in case the process that calls them exports it\nX-<.\n\nThanks.\n\n\n[Footnote]\n\n*1* If the end user has GIT_WORK_TREE in the environment, even if\n Git stops exporting it by reverting d95138e, such a script may\n break. So in that sense, d95138e did not quite change the rule\n of the game for these scripts, but made it more obvious when\n these scripts were written sloppily.\n"},{"id":"274937","messageId":"20151223093700.GA13386@sigill.intra.peff.net","threadId":"40865","inReplyTo":"xmqqd1tye4i8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-23T09:37:00Z","receivedAt":"2015-12-23T09:37:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 22, 2015 at 10:13:03AM -0800, Junio C Hamano wrote:\n\n> > Another by the way, this \"forcing aliases as external commands\" now\n> > shows something like \"error: git-log died of signal 13\" when the pager\n> > exits early, for an alias like \"l1 = log --oneline\".\n> \n> ... and we do not show that when we directly call \"git log\" is...?\n> \n> We do signal this with non-zero exit status like so:\n> \n> \t$ GIT_PAGER=true git log --oneline ; echo $?\n> 141\n> \n> and it is not surprising that the one that is catching the exit\n> status of what was spawned and reporting \"signal 13\".\n\nThis sounded very familiar. Apparently I've been running with the patch\nbelow for almost 3 years and never got around to re-examining it.\n\nThe original discussion is in:\n\n http://thread.gmane.org/gmane.comp.version-control.git/212630\n\nHaving skimmed through the arguments there, and given that we applied\nthe patch from the middle of the thread, I think this is a good change.\n\n-- >8 --\nDate: Fri, 4 Jan 2013 07:19:35 -0500\nSubject: [PATCH] avoid SIGPIPE warnings for aliases\n\nWhen git executes an alias that specifies an external\ncommand, it will complain if the alias dies due to a signal.\nThis is usually a good thing, as signal deaths are\nunexpected. However, SIGPIPE is not unexpected for many\ncommands which produce a lot of output; it is intended that\nthe user closing the pager would kill them them via SIGPIPE.\n\nAs a result, the user might see annoying messages in a\nscenario like this:\n\n $ cat ~/.gitconfig\n [alias]\n lgbase = log --some-options\n lg = !git lgbase --more-options\n lg2 = !git lgbase --other-options\n\n $ git lg -p\n [user hits 'q' to exit pager]\n error: git lgbase --more-options died of signal 13\n fatal: While expanding alias 'lg': 'git lgbase --more-options': Success\n\nMany users won't see this, because we execute the external\ncommand with the shell, and a POSIX shell will silently\nrewrite the signal-death exit code into 128+signal, and we\nwill treat it like a normal exit code. However, this does\nnot always happen:\n\n 1. If the sub-command does not have shell metacharacters,\n we will skip the shell and exec it directly, getting\n the signal code.\n\n 2. Some shells do not do this rewriting; for example,\n setting SHELL_PATH set to zsh will reveal this problem.\n\nThis patch squelches the message, and lets git exit silently\n(with the same exit code that a shell would use in case of a\nsignal).\n\nThe first line of the message comes from run-command's\nwait_or_whine. To silence that message, we remain quiet\nanytime we see SIGPIPE.\n\nThe second line comes from handle_alias itself. It calls\ndie_errno whenever run_command returns a negative value.\nHowever, only -1 indicates a syscall error where errno has\nsomething useful (note that it says the useless \"success\"\nabove). Instead, we treat negative returns from run_command\n(except for -1) as a normal code to be passed to exit.\n\nSigned-off-by: Jeff King \n---\n git.c | 2 +-\n run-command.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 6ed824c..34a18a3 100644\n--- a/git.c\n+++ b/git.c\n@@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\talias_argv[argc] = NULL;\n \n \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n-\t\t\tif (ret >= 0) /* normal exit */\n+\t\t\tif (ret != -1) /* normal exit */\n \t\t\t\texit(ret);\n \n \t\t\tdie_errno(\"While expanding alias '%s': '%s'\",\ndiff --git a/run-command.c b/run-command.c\nindex 13fa452..694a6ff 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n+\t\tif (code != SIGINT && code != SIGQUIT && code != SIGPIPE)\n \t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n-- \n2.7.0.rc2.368.g1cbb535\n"},{"id":"274940","messageId":"CACsJy8AwoohWmUF9QCDg+xvNifDh0o+PBnExxwv=qxz2Mh4a1w@mail.gmail.com","threadId":"40865","inReplyTo":"20151223093700.GA13386@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-23T10:20:30Z","receivedAt":"2015-12-23T10:20:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 23, 2015 at 4:37 PM, Jeff King wrote:\n> On Tue, Dec 22, 2015 at 10:13:03AM -0800, Junio C Hamano wrote:\n>\n>> > Another by the way, this \"forcing aliases as external commands\" now\n>> > shows something like \"error: git-log died of signal 13\" when the pager\n>> > exits early, for an alias like \"l1 = log --oneline\".\n>>\n>> ... and we do not show that when we directly call \"git log\" is...?\n>>\n>> We do signal this with non-zero exit status like so:\n>>\n>> $ GIT_PAGER=true git log --oneline ; echo $?\n>> 141\n>>\n>> and it is not surprising that the one that is catching the exit\n>> status of what was spawned and reporting \"signal 13\".\n>\n> This sounded very familiar. Apparently I've been running with the patch\n> below for almost 3 years and never got around to re-examining it.\n>\n> The original discussion is in:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/212630\n>\n> Having skimmed through the arguments there, and given that we applied\n> the patch from the middle of the thread, I think this is a good change.\n\nYep. The new thing here is the \"died of\" message now also comes from\nexecv_dashed_external() for non-external aliases, so it can show up a\nlot more often. I wasn't sure if excluding SIGPIPE in wait_and_whine\nwas safe. But I _guess_ it is, based on your having no problem with\nthe patch for a long time and the past discussion.\n\n>\n> -- >8 --\n> Date: Fri, 4 Jan 2013 07:19:35 -0500\n> Subject: [PATCH] avoid SIGPIPE warnings for aliases\n>\n> When git executes an alias that specifies an external\n> command, it will complain if the alias dies due to a signal.\n> This is usually a good thing, as signal deaths are\n> unexpected. However, SIGPIPE is not unexpected for many\n> commands which produce a lot of output; it is intended that\n> the user closing the pager would kill them them via SIGPIPE.\n>\n> As a result, the user might see annoying messages in a\n> scenario like this:\n>\n> $ cat ~/.gitconfig\n> [alias]\n> lgbase = log --some-options\n> lg = !git lgbase --more-options\n> lg2 = !git lgbase --other-options\n>\n> $ git lg -p\n> [user hits 'q' to exit pager]\n> error: git lgbase --more-options died of signal 13\n> fatal: While expanding alias 'lg': 'git lgbase --more-options': Success\n>\n> Many users won't see this, because we execute the external\n> command with the shell, and a POSIX shell will silently\n> rewrite the signal-death exit code into 128+signal, and we\n> will treat it like a normal exit code. However, this does\n> not always happen:\n>\n> 1. If the sub-command does not have shell metacharacters,\n> we will skip the shell and exec it directly, getting\n> the signal code.\n>\n> 2. Some shells do not do this rewriting; for example,\n> setting SHELL_PATH set to zsh will reveal this problem.\n>\n> This patch squelches the message, and lets git exit silently\n> (with the same exit code that a shell would use in case of a\n> signal).\n>\n> The first line of the message comes from run-command's\n> wait_or_whine. To silence that message, we remain quiet\n> anytime we see SIGPIPE.\n>\n> The second line comes from handle_alias itself. It calls\n> die_errno whenever run_command returns a negative value.\n> However, only -1 indicates a syscall error where errno has\n> something useful (note that it says the useless \"success\"\n> above). Instead, we treat negative returns from run_command\n> (except for -1) as a normal code to be passed to exit.\n>\n> Signed-off-by: Jeff King \n> ---\n> git.c | 2 +-\n> run-command.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 6ed824c..34a18a3 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)\n> alias_argv[argc] = NULL;\n>\n> ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n> - if (ret >= 0) /* normal exit */\n> + if (ret != -1) /* normal exit */\n> exit(ret);\n>\n> die_errno(\"While expanding alias '%s': '%s'\",\n> diff --git a/run-command.c b/run-command.c\n> index 13fa452..694a6ff 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n> error(\"waitpid is confused (%s)\", argv0);\n> } else if (WIFSIGNALED(status)) {\n> code = WTERMSIG(status);\n> - if (code != SIGINT && code != SIGQUIT)\n> + if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)\n> error(\"%s died of signal %d\", argv0, code);\n> /*\n> * This return value is chosen so that code & 0xff\n> --\n> 2.7.0.rc2.368.g1cbb535\n>\n\n\n\n-- \nDuy\n"},{"id":"274957","messageId":"CAPig+cRndwJ-W1Tz8j7Bk8cFxsmDfv+e3xjbosNvON=+fKOVXg@mail.gmail.com","threadId":"40865","inReplyTo":"20151223093700.GA13386@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-23T16:17:57Z","receivedAt":"2015-12-23T16:17:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 23, 2015 at 4:37 AM, Jeff King wrote:\n> Subject: [PATCH] avoid SIGPIPE warnings for aliases\n>\n> When git executes an alias that specifies an external\n> command, it will complain if the alias dies due to a signal.\n> This is usually a good thing, as signal deaths are\n> unexpected. However, SIGPIPE is not unexpected for many\n> commands which produce a lot of output; it is intended that\n> the user closing the pager would kill them them via SIGPIPE.\n\ns/them them/them/\n\n> As a result, the user might see annoying messages in a\n> scenario like this:\n>\n> $ cat ~/.gitconfig\n> [alias]\n> lgbase = log --some-options\n> lg = !git lgbase --more-options\n> lg2 = !git lgbase --other-options\n>\n> $ git lg -p\n> [user hits 'q' to exit pager]\n> error: git lgbase --more-options died of signal 13\n> fatal: While expanding alias 'lg': 'git lgbase --more-options': Success\n>\n> Many users won't see this, because we execute the external\n> command with the shell, and a POSIX shell will silently\n> rewrite the signal-death exit code into 128+signal, and we\n> will treat it like a normal exit code. However, this does\n> not always happen:\n>\n> 1. If the sub-command does not have shell metacharacters,\n> we will skip the shell and exec it directly, getting\n> the signal code.\n>\n> 2. Some shells do not do this rewriting; for example,\n> setting SHELL_PATH set to zsh will reveal this problem.\n>\n> This patch squelches the message, and lets git exit silently\n> (with the same exit code that a shell would use in case of a\n> signal).\n>\n> The first line of the message comes from run-command's\n> wait_or_whine. To silence that message, we remain quiet\n> anytime we see SIGPIPE.\n>\n> The second line comes from handle_alias itself. It calls\n> die_errno whenever run_command returns a negative value.\n> However, only -1 indicates a syscall error where errno has\n> something useful (note that it says the useless \"success\"\n> above). Instead, we treat negative returns from run_command\n> (except for -1) as a normal code to be passed to exit.\n>\n> Signed-off-by: Jeff King \n"},{"id":"274962","messageId":"567B05F0.5020604@kdbg.org","threadId":"40865","inReplyTo":"20151223093700.GA13386@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-12-23T20:37:04Z","receivedAt":"2015-12-23T20:37:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.12.2015 um 10:37 schrieb Jeff King:\n> The second line comes from handle_alias itself. It calls\n> die_errno whenever run_command returns a negative value.\n> However, only -1 indicates a syscall error where errno has\n> something useful (note that it says the useless \"success\"\n> above). Instead, we treat negative returns from run_command\n> (except for -1) as a normal code to be passed to exit.\n>\n> Signed-off-by: Jeff King \n> ---\n> git.c | 2 +-\n> run-command.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 6ed824c..34a18a3 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)\n> \t\t\talias_argv[argc] = NULL;\n>\n> \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n> -\t\t\tif (ret >= 0) /* normal exit */\n> +\t\t\tif (ret != -1) /* normal exit */\n\nWhy does this make a difference? We only ever return -1, zero, or a \npositive value from run_command/finish_command/wait_or_whine, as far as \nI can see.\n\n> \t\t\t\texit(ret);\n>\n> \t\t\tdie_errno(\"While expanding alias '%s': '%s'\",\n\n-- Hannes\n"},{"id":"274975","messageId":"20151223213140.GB21277@sigill.intra.peff.net","threadId":"40865","inReplyTo":"567B05F0.5020604@kdbg.org","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-23T21:31:40Z","receivedAt":"2015-12-23T21:31:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 23, 2015 at 09:37:04PM +0100, Johannes Sixt wrote:\n\n> >--- a/git.c\n> >+++ b/git.c\n> >@@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)\n> > \t\t\talias_argv[argc] = NULL;\n> >\n> > \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n> >-\t\t\tif (ret >= 0) /* normal exit */\n> >+\t\t\tif (ret != -1) /* normal exit */\n> \n> Why does this make a difference? We only ever return -1, zero, or a positive\n> value from run_command/finish_command/wait_or_whine, as far as I can see.\n\nYeah, you're right. This bit predates 709ca73 (run-command: encode\nsignal death as a positive integer, 2013-01-05), which came out of the\nsame discussion. So I'd agree this hunk can simply be dropped.\n\nThat leaves the ignoring of SIGPIPE in wait_or_whine. I started to\nrewrite the commit message to drop the first hunk, but I found I\ncouldn't replicate the problem in the second either!\n\nDoing:\n\n GIT_PAGER=false git -c alias.foo='!git log -p' foo\n\ndoesn't trigger it. We run the alias through a shell, so we see only the\nmunged \"141\" value from the shell's exit code.\n\nSomething like:\n\n GIT_PAGER=false git -p -c alias.foo='!yes' foo\n\ndoes generate the error message. But we've redirected stderr into the\npager at that point, so by definition it can never be shown.\n\nSo I think we would need a case where:\n\n - the outer git doesn't run the pager that dies; instead the pager is\n run inside the alias. But...\n\n - inside the alias cannot be a shell pipeline, since \"foo | less\" will\n report the exit code of \"less\", not \"foo\" (we make special arrangements\n in git to propagate the exit code of \"foo\"). So it pretty much has\n to be a git invocation inside the alias. But...\n\n - The git invocation will convert signal death in the sub-process into\n 141, like a shell would.\n\nSo I'm not sure if this is triggerable at all with an alias.\n\nI did manage to trigger it with an external command, like:\n\n $ cat $(which git-yes)\n #!/bin/sh\n # This _has_ to be exec, otherwise the shell converts SIGPIPE death\n # into 141.\n exec yes\n\nand then if you run your _own_ pager, like this:\n\n $ git yes | false\n error: git-yes died of signal 13\n\nyou see it. But if git starts the pager, you don't:\n\n $ GIT_PAGER=false git -p yes\n\nBecause the stderr of the outer git process is going to the same dead\npipe.\n\nSo my takeaways are:\n\n 1. Complaining about signal death in general is going to be flaky,\n because it's so easy for shells or git to rewrite the exit code and\n not trigger WIFSIGNALED() in the first place.\n\n 2. I doubt anybody is actually seeing this in practice anymore. But\n maybe I am misunderstanding something in Duy's series that changes\n this.\n\n-Peff\n"},{"id":"274984","messageId":"CACsJy8Aqu9wTigWKC1-74qLUimV8PvmbDG1XeqYcbW0ZSMLSTg@mail.gmail.com","threadId":"40865","inReplyTo":"20151223213140.GB21277@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-24T09:35:33Z","receivedAt":"2015-12-24T09:35:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 24, 2015 at 4:31 AM, Jeff King wrote:\n> 2. I doubt anybody is actually seeing this in practice anymore. But\n> maybe I am misunderstanding something in Duy's series that changes\n> this.\n\nThere are two parts in your patch, one (that you two seemed to focus\non) about return code with \"!\" aliases. Another, suppressing SIGPIPE,\naffects more than \"!\" aliases. In my case it's\nexecv_dashed_external(). Non-\"!\" aliases are now forced to use that\nfunction.\n\nThis is the output with 'pu'. Notice git-log is executed separately\n\n> ~/w/git $ GIT_TRACE=2 PAGER=true ./git --exec-path=. l\n16:28:19.167040 git.c:567 trace: exec: 'git-l'\n16:28:19.167088 run-command.c:345 trace: run_command: 'git-l'\n16:28:19.168599 git.c:286 trace: alias expansion: l =>\n'log' '--stat'\n16:28:19.168633 git.c:567 trace: exec: 'git-log' '--stat'\n16:28:19.168649 run-command.c:345 trace: run_command: 'git-log' '--stat'\n16:28:19.170420 git.c:350 trace: built-in: git 'log' '--stat'\n16:28:19.210074 run-command.c:345 trace: run_command: 'true'\n16:28:19.210382 run-command.c:204 trace: exec: 'true'\nerror: git-log died of signal 13\n\nWith your patch on top, \"git-log died of..\" goes away. And this is\nwith 'master', where the builtin version of git-log is used\n\n> ~/w/git $ GIT_TRACE=2 PAGER=true ./git --exec-path=. l\n16:29:17.546382 git.c:561 trace: exec: 'git-l'\n16:29:17.546428 run-command.c:343 trace: run_command: 'git-l'\n16:29:17.547485 git.c:282 trace: alias expansion: l =>\n'log' '--stat'\n16:29:17.548482 git.c:348 trace: built-in: git 'log' '--stat'\n16:29:17.586457 run-command.c:343 trace: run_command: 'true'\n16:29:17.586770 run-command.c:202 trace: exec: 'true'\n\nSo, in practice, people will see \"died of signal 13\" a lot more often\nif my series is merged and released (I assume that people often use\naliases for paged commands like git-log, git-diff...)\n-- \nDuy\n"},{"id":"275110","messageId":"20151229081222.GB9254@sigill.intra.peff.net","threadId":"40865","inReplyTo":"CACsJy8Aqu9wTigWKC1-74qLUimV8PvmbDG1XeqYcbW0ZSMLSTg@mail.gmail.com","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-29T08:12:22Z","receivedAt":"2015-12-29T08:12:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 24, 2015 at 04:35:33PM +0700, Duy Nguyen wrote:\n\n> On Thu, Dec 24, 2015 at 4:31 AM, Jeff King wrote:\n> > 2. I doubt anybody is actually seeing this in practice anymore. But\n> > maybe I am misunderstanding something in Duy's series that changes\n> > this.\n> \n> There are two parts in your patch, one (that you two seemed to focus\n> on) about return code with \"!\" aliases. Another, suppressing SIGPIPE,\n> affects more than \"!\" aliases.\n\nSorry if I was confusing; most of the examples in my previous message\nare about the SIGPIPE thing. I was having trouble triggering the message\nin practice, even for externals (because the error message goes to the\npager, too!).\n\n> In my case it's execv_dashed_external(). Non-\"!\" aliases are now\n> forced to use that function.\n\nThanks, this is the part I was missing.\n\nThe outer git wrapper doesn't start the pager, so its stderr still gets\nseen by the user. But the _inner_ git-log does start the pager, and then\ndies of SIGPIPE.\n\nSo yeah, I think we want something like this on top of\nnd/clear-gitenv-upon-use-of-alias.\n\n-- >8 --\nSubject: [PATCH] run-command: don't warn on SIGPIPE deaths\n\nWhen git executes a sub-command, we print a warning if the\ncommand dies due to a signal, but make an exception for\n\"uninteresting\" cases like SIGINT and SIGQUIT (since the\nuser presumably just hit ^C).\n\nWe should make a similar exception for SIGPIPE, because it's\nan expected and uninteresting return in most cases; it\ngenerally means the user quit the pager before git had\nfinished generating all output. This used to be very hard\nto trigger in practice, because:\n\n 1. We only complain if we see a real SIGPIPE death, not\n the shell-induced 141 exit code. This means that\n anything we run via the shell does not trigger the\n warning, which includes most non-trivial aliases.\n\n 2. The common case for SIGPIPE is the user quitting the\n pager before git has finished generating all output.\n But if the user triggers a pager with \"-p\", we redirect\n the git wrapper's stderr to that pager, too. Since the\n pager is dead, it means that the message goes nowhere.\n\n 3. You can see it if you run your own pager, like\n \"git foo | head\". But that only happens if \"foo\" is a\n non-builtin (so it doesn't work with \"log\", for\n example).\n\nHowever, it may become more common after 86d26f2, which\nteaches alias to re-exec builtins rather than running them\nin the same process. This case doesn't trigger (1), as we\ndon't need a shell to run a git command. It doesn't trigger\n(2), because the pager is not started by the original git,\nbut by the inner re-exec of git. And it doesn't trigger (3),\nbecause builtins are treated more like non-builtins in this\ncase.\n\nGiven how flaky this message already is (e.g., you cannot\neven know whether you will see it, as git optimizes out some\nshell invocations behind the scenes based on the contents of\nthe command!), and that it is unlikely to ever provide\nuseful information, let's suppress it for all cases of\nSIGPIPE.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 13fa452..694a6ff 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n+\t\tif (code != SIGINT && code != SIGQUIT && code != SIGPIPE)\n \t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n-- \n2.7.0.rc3.367.g09631da\n"},{"id":"275152","messageId":"xmqqlh8dgcr2.fsf@gitster.mtv.corp.google.com","threadId":"40865","inReplyTo":"20151229081222.GB9254@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T21:34:41Z","receivedAt":"2015-12-29T21:34:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The outer git wrapper doesn't start the pager, so its stderr still gets\n> seen by the user. But the _inner_ git-log does start the pager, and then\n> dies of SIGPIPE.\n>\n> So yeah, I think we want something like this on top of\n> nd/clear-gitenv-upon-use-of-alias.\n\nThat makes sense to me.\n\n> -- >8 --\n> Subject: [PATCH] run-command: don't warn on SIGPIPE deaths\n>\n> When git executes a sub-command, we print a warning if the\n> command dies due to a signal, but make an exception for\n> \"uninteresting\" cases like SIGINT and SIGQUIT (since the\n> user presumably just hit ^C).\n>\n> We should make a similar exception for SIGPIPE, because it's\n> an expected and uninteresting return in most cases; it\n> generally means the user quit the pager before git had\n> finished generating all output. This used to be very hard\n> to trigger in practice, because:\n>\n> 1. We only complain if we see a real SIGPIPE death, not\n> the shell-induced 141 exit code. This means that\n> anything we run via the shell does not trigger the\n> warning, which includes most non-trivial aliases.\n>\n> 2. The common case for SIGPIPE is the user quitting the\n> pager before git has finished generating all output.\n> But if the user triggers a pager with \"-p\", we redirect\n> the git wrapper's stderr to that pager, too. Since the\n> pager is dead, it means that the message goes nowhere.\n>\n> 3. You can see it if you run your own pager, like\n> \"git foo | head\". But that only happens if \"foo\" is a\n> non-builtin (so it doesn't work with \"log\", for\n> example).\n>\n> However, it may become more common after 86d26f2, which\n> teaches alias to re-exec builtins rather than running them\n> in the same process. This case doesn't trigger (1), as we\n> don't need a shell to run a git command. It doesn't trigger\n> (2), because the pager is not started by the original git,\n> but by the inner re-exec of git. And it doesn't trigger (3),\n> because builtins are treated more like non-builtins in this\n> case.\n>\n> Given how flaky this message already is (e.g., you cannot\n> even know whether you will see it, as git optimizes out some\n> shell invocations behind the scenes based on the contents of\n> the command!), and that it is unlikely to ever provide\n> useful information, let's suppress it for all cases of\n> SIGPIPE.\n>\n> Signed-off-by: Jeff King \n> ---\n> run-command.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 13fa452..694a6ff 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n> \t\terror(\"waitpid is confused (%s)\", argv0);\n> \t} else if (WIFSIGNALED(status)) {\n> \t\tcode = WTERMSIG(status);\n> -\t\tif (code != SIGINT && code != SIGQUIT)\n> +\t\tif (code != SIGINT && code != SIGQUIT && code != SIGPIPE)\n> \t\t\terror(\"%s died of signal %d\", argv0, code);\n> \t\t/*\n> \t\t * This return value is chosen so that code & 0xff\n"}]}