{"thread":{"id":"39723","subject":"Linked workdirs break typo-correction.","startedAt":"2015-06-25T14:15:23Z","lastAt":"2015-06-27T05:57:44Z","messageCount":7,"participants":["Bjørnar Snoksrud","Nguyễn Thái Ngọc Duy","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"264849","messageId":"CA+cck7GD+JgR4O-XoBeUX1gJAG6suP9iLwASyRygK8hR4KP7pw@mail.gmail.com","threadId":"39723","inReplyTo":null,"subject":"Linked workdirs break typo-correction.","fromName":"Bjørnar Snoksrud","fromEmail":"snoksrud@gmail.com","sentAt":"2015-06-25T14:15:23Z","receivedAt":"2015-06-25T14:15:23Z","isPatch":false,"sender":{"key":"snoksrud@gmail.com","avatar":"https://gravatar.com/avatar/bd7f988a0a0941712c4bb875eb852d6038a15a6e18cb4dc880ba8f6d8c4040fd?d=mp&s=160"},"body":"This is a weird one..\n\nWhen standing in a folder inside a linked working copy, the\ntypo-correction breaks all commands.\n\nRepro:\n~/git $ git --version\ngit version 2.4.4.600.g6397abd\n~/git $ git init bar\nInitialized empty Git repository in ~/git/bar/.git/\n~/git $ cd bar\n~/git/bar (master #) $ git commit -m init --allow-empty\n[master (root-commit) 554ea84] init\n~/git/bar (master) $ mkdir folder\n~/git/bar (master) $ touch folder/file\n~/git/bar (master) $ git add folder\n~/git/bar (master +) $ git commit -m folder\n[master 8c00ba8] folder\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 folder/file\n~/git/bar (master) $ cd folder/\n~/git/bar/folder (master) $ git shw\nWARNING: You called a Git command named 'shw', which does not exist.\nContinuing under the assumption that you meant 'show'\nin 0.1 seconds automatically...\ncommit 8c00ba8d30cff0e0d1b9cf110a65ea8f33edf8b2\nAuthor: Bjørnar Snoksrud <bsnoksru@cisco.com>\nDate:   Thu Jun 25 16:08:01 2015 +0200\n\n    folder\n\ndiff --git a/folder/file b/folder/file\nnew file mode 100644\nindex 0000000..e69de29\nbsnoksru@calculon ~/git/bar/folder (master) $ git branch foo\nbsnoksru@calculon ~/git/bar/folder (master) $ git checkout foo --to\n~/git/foo\nEnter /home/bsnoksru/git/foo (identifier foo)\nSwitched to branch 'foo'\nbsnoksru@calculon ~/git/bar/folder (master) $ cd ../../foo/folder/\n~/git/foo/folder (foo) $ git shw\nWARNING: You called a Git command named 'shw', which does not exist.\nContinuing under the assumption that you meant 'show'\nin 0.1 seconds automatically...\nfatal: internal error: work tree has already been set\nCurrent worktree: ~/git/foo\nNew worktree: ~/git/foo/folder\n~/git/foo/folder (foo) $ git brnch baz\nWARNING: You called a Git command named 'brnch', which does not exist.\nContinuing under the assumption that you meant 'branch'\nin 0.1 seconds automatically...\nfatal: internal error: work tree has already been set\nCurrent worktree: /home/bsnoksru/git/foo\nNew worktree: /home/bsnoksru/git/foo/folder\n\n\n-- \nbjornar@snoksrud.no\n"},{"id":"264942","messageId":"1435315055-27011-1-git-send-email-pclouds@gmail.com","threadId":"39723","inReplyTo":"CA+cck7GD+JgR4O-XoBeUX1gJAG6suP9iLwASyRygK8hR4KP7pw@mail.gmail.com","subject":"[PATCH] 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-06-26T10:37:35Z","receivedAt":"2015-06-26T10:37:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"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\nsetup_discovered_git_dir() and may or may not call set_git_dir() near\nthe 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\nFor 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,\nthe git dir can't be \".git\" and set_git_dir() is called and $GIT_DIR\nset. This is the key of this problem.\n\nIf we expand an alias (or autocorrect command names), then\nsetup_git_dir_gently() is run the second time. If $GIT_DIR is not set in\nthe first run, we run the same setup_discovered_git_dir() as before.\nNothing to see. If it is, however, we'll enter setup_explicit_git_dir()\nthis time.\n\nThis is where the \"fun\" is. The legacy behavior is, if $GIT_WORK_TREE is\nnot set but $GIT_DIR is, cwd is chosen as worktree's top. If you happen\nto stand at worktree's top when you do this, all is well. If you are in\na subdir \"foo/bar\" (real worktree's top is \"foo\"), this behavior bites\nyou: your detected worktree is now \"foo/bar\", but the first run\ncorrectly detected worktree as \"foo\". You get \"internal error: work tree\nhas already been set\" as a result.\n\nBottom line is, when $GIT_DIR is set, $GIT_WORK_TREE should be set too\nunless there's no work tree. But setting $GIT_WORK_TREE inside\nset_git_dir() may backfire. We don't know at that point if work tree is\nalready configured by the caller. So set it when work tree is\ndetected. It does not harm if $GIT_WORK_TREE is set while $GIT_DIR is\nnot.\n\nReported-by: Bjørnar Snoksrud <snoksrud@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n environment.c      |  2 ++\n t/t0002-gitfile.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/environment.c b/environment.c\nindex 61c685b..8f1b249 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -231,6 +231,8 @@ 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\terror(\"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 37e9396..9393322 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -99,4 +99,21 @@ 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+\tgit init sgd &&\n+\t(\n+\t\tcd sgd &&\n+\t\tgit config alias.lsfi ls-files &&\n+\t\tmv .git .realgit &&\n+\t\techo \"gitdir: .realgit\" >.git &&\n+\t\tmkdir subdir &&\n+\t\tcd subdir &&\n+\t\t>foo &&\n+\t\tgit add foo &&\n+\t\tgit lsfi >actual &&\n+\t\techo foo >expected &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"264946","messageId":"20150626115603.GB4315@peff.net","threadId":"39723","inReplyTo":"1435315055-27011-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-26T11:56:03Z","receivedAt":"2015-06-26T11:56:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 26, 2015 at 05:37:35PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This is where the \"fun\" is. The legacy behavior is, if $GIT_WORK_TREE is\n> not set but $GIT_DIR is, cwd is chosen as worktree's top. If you happen\n> to stand at worktree's top when you do this, all is well. If you are in\n> a subdir \"foo/bar\" (real worktree's top is \"foo\"), this behavior bites\n> you: your detected worktree is now \"foo/bar\", but the first run\n> correctly detected worktree as \"foo\". You get \"internal error: work tree\n> has already been set\" as a result.\n\nI think this makes sense. I feel like we've dealt with this before, but\nthe two previous rounds I found were basically:\n\n  - we have GIT_IMPLICIT_WORK_TREE, but that is for the _opposite_ case.\n    I.e., when we do not have a work tree and must communicate so to\n    later code (including sub-processes).\n\n  - a discussion about switching the \"work tree defaults to '.' when\n    $GIT_DIR is set\" behavior yielded almost the identical patch:\n\n      http://article.gmane.org/gmane.comp.version-control.git/219196\n\n    but we were so wrapped up in the greater discussion we did not apply\n    that simple fix. :)\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\nYeah, it would be nicer if we could make sure we always set\nGIT_WORK_TREE along with GIT_DIR (since anything else is potentially\ndangerous), but I don't think it's feasible to do it in set_git_dir for\nthe reasons you state.\n\nI gave a quick peek through setup_explicit_git_dir to see if we missed\nother cases (spoiler: no):\n\n  - if we have core.bare set, we set $GIT_DIR without setting the\n    working tree. But that's OK, because we'll never look at\n    $GIT_WORK_TREE in that case. Good.\n\n  - if we have core.worktree set, then we call set_git_work_tree to\n    match. Good.\n\n  - If GIT_IMPLICIT_WORK_TREE is turned off, we set_git_dir but do not\n    set GIT_WORK_TREE. Good, because otherwise it would take precedence.\n\n  - there are some more calls to set_git_dir at the end, but I think in\n    those cases we will already have set up the working tree (or decided\n    we do not have one, by the above logic).\n\nSo that all seems fine. We also call set_git_dir from enter_repo, but in\nthat case we have already moved into the .git directory itself, so that\nis OK. setup_discovered_git_dir also makes some calls, but either we are\nexplicitly bare there, or we call set_git_work_tree(\".\").\n\nSo I do not see any cases that will regress, or that are uncovered. Of\ncourse, that is just from reading the code. Getting one million monkeys\nto bang on the keyboard may turn up any real problems. :)\n\n> +\tif (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))\n> +\t\terror(\"Could not set GIT_WORK_TREE to '%s'\", work_tree);\n\nShould this be die()? setenv() should basically never fail, but if it\ndoes, it could be confusing and/or dangerous to run without the variable\nset.\n\n-Peff\n"},{"id":"264973","messageId":"xmqqpp4iv1au.fsf@gitster.dls.corp.google.com","threadId":"39723","inReplyTo":"1435315055-27011-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T17:15:21Z","receivedAt":"2015-06-26T17:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\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\nHmmm, setting GIT_WORK_TREE locally without exporting it to our\nsubprocesses would not hurt even when we do not see GIT_DIR, so I\nagree \"It does not harm\" is true for _us_, the process that runs\nthis code itself.\n\nBut I am not sure if it is true for our children (e.g. hooks,\nfilters etc. that is spawned by us).  With this change, they inherit\nGIT_WORK_TREE and no GIT_DIR, in such a case.  If they set GIT_DIR\nthemselves for their own use, perhaps arranging to work in somewhere\nelse they know by chdir'ing there, they did not have to set\nGIT_WORK_TREE=. before runing git in there, but now they do, because\nwe start exporting GIT_WORK_TREE to interfere what they have been\ndoing.\n\n\"It does not harm\" is probably false for our children, I would\nthink.  The children can do new things to avoid the harm, though.\n\n\n> Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  environment.c      |  2 ++\n>  t/t0002-gitfile.sh | 17 +++++++++++++++++\n>  2 files changed, 19 insertions(+)\n>\n> diff --git a/environment.c b/environment.c\n> index 61c685b..8f1b249 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -231,6 +231,8 @@ 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\terror(\"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 37e9396..9393322 100755\n> --- a/t/t0002-gitfile.sh\n> +++ b/t/t0002-gitfile.sh\n> @@ -99,4 +99,21 @@ 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> +\tgit init sgd &&\n> +\t(\n> +\t\tcd sgd &&\n> +\t\tgit config alias.lsfi ls-files &&\n> +\t\tmv .git .realgit &&\n> +\t\techo \"gitdir: .realgit\" >.git &&\n> +\t\tmkdir subdir &&\n> +\t\tcd subdir &&\n> +\t\t>foo &&\n> +\t\tgit add foo &&\n> +\t\tgit lsfi >actual &&\n> +\t\techo foo >expected &&\n> +\t\ttest_cmp expected actual\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"264975","messageId":"xmqqlhf6v0ln.fsf@gitster.dls.corp.google.com","threadId":"39723","inReplyTo":"1435315055-27011-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T17:30:28Z","receivedAt":"2015-06-26T17:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This is where the \"fun\" is. The legacy behavior is, if $GIT_WORK_TREE is\n> not set but $GIT_DIR is, cwd is chosen as worktree's top. If you happen\n> to stand at worktree's top when you do this, all is well.\n\nIt is not legacy, though.  It is how things were designed to be\nused, and how things are supposed to work in the future.\n\nUntil we do the deprecation dance to force people to update existing\nscripts to explicitly export GIT_WORK_TREE=$(pwd) or something like\nthat, that is.\n\nAnd I wouldn't be opposed to such a transition plan; something along\nthe lines of what Peff outlined in that old thread.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/219096/focus=219197\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\nI am inclined to queue this on 'next' per \"experimental\" basis so\nthat other people with different workflows (especially those who\nuse existing scripts around Git plumbing heavily) can see if this\ndoes not have any unintended fallouts.  I cannot convince myself\nthat it is generally a safe thing to do to muck with environment\npartially, hoping nobody would care.\n\nThanks for looking into it.\n\n>  environment.c      |  2 ++\n>  t/t0002-gitfile.sh | 17 +++++++++++++++++\n>  2 files changed, 19 insertions(+)\n>\n> diff --git a/environment.c b/environment.c\n> index 61c685b..8f1b249 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -231,6 +231,8 @@ 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\terror(\"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 37e9396..9393322 100755\n> --- a/t/t0002-gitfile.sh\n> +++ b/t/t0002-gitfile.sh\n> @@ -99,4 +99,21 @@ 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> +\tgit init sgd &&\n> +\t(\n> +\t\tcd sgd &&\n> +\t\tgit config alias.lsfi ls-files &&\n> +\t\tmv .git .realgit &&\n> +\t\techo \"gitdir: .realgit\" >.git &&\n> +\t\tmkdir subdir &&\n> +\t\tcd subdir &&\n> +\t\t>foo &&\n> +\t\tgit add foo &&\n> +\t\tgit lsfi >actual &&\n> +\t\techo foo >expected &&\n> +\t\ttest_cmp expected actual\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"264985","messageId":"xmqqk2uqthrh.fsf@gitster.dls.corp.google.com","threadId":"39723","inReplyTo":"xmqqpp4iv1au.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T19:02:42Z","receivedAt":"2015-06-26T19:02:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> But I am not sure if it is true for our children (e.g. hooks,\n> filters etc. that is spawned by us).  With this change, they inherit\n> GIT_WORK_TREE and no GIT_DIR, in such a case.  If they set GIT_DIR\n> themselves for their own use, perhaps arranging to work in somewhere\n> else they know by chdir'ing there, they did not have to set\n> GIT_WORK_TREE=. before runing git in there, but now they do, because\n> we start exporting GIT_WORK_TREE to interfere what they have been\n> doing.\n>\n> \"It does not harm\" is probably false for our children, I would\n> think.  The children can do new things to avoid the harm, though.\n\nActually, I think I am overly and unnecessarily worried in the\nabove.\n\nWe do not do anything special wrt existing GIT_ environment\nvariables the user exported into us when spawning hooks or filters;\nif existing scripts wanted to visit a different repository and\ninteract with it while they do their work, they would have already\nbeen seeing these variables in their environment inherited from us,\nand they should have been protecting themselves from these settings\nanyway.  So I do not think this patch breaks anything (other than\nsuch scripts that are already broken).\n\nSo, sorry for a false alarm.  Queued.\n"},{"id":"265023","messageId":"CACsJy8AuO5PNy=oFN1W9EQKK5_=jRkSNx8KhQYjL-0mW+jvr0A@mail.gmail.com","threadId":"39723","inReplyTo":"20150626115603.GB4315@peff.net","subject":"Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-27T05:57:44Z","receivedAt":"2015-06-27T05:57:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jun 26, 2015 at 6:56 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Jun 26, 2015 at 05:37:35PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> This is where the \"fun\" is. The legacy behavior is, if $GIT_WORK_TREE is\n>> not set but $GIT_DIR is, cwd is chosen as worktree's top. If you happen\n>> to stand at worktree's top when you do this, all is well. If you are in\n>> a subdir \"foo/bar\" (real worktree's top is \"foo\"), this behavior bites\n>> you: your detected worktree is now \"foo/bar\", but the first run\n>> correctly detected worktree as \"foo\". You get \"internal error: work tree\n>> has already been set\" as a result.\n>\n> I think this makes sense. I feel like we've dealt with this before, but\n> the two previous rounds I found were basically:\n>\n>   - we have GIT_IMPLICIT_WORK_TREE, but that is for the _opposite_ case.\n>     I.e., when we do not have a work tree and must communicate so to\n>     later code (including sub-processes).\n>\n>   - a discussion about switching the \"work tree defaults to '.' when\n>     $GIT_DIR is set\" behavior yielded almost the identical patch:\n>\n>       http://article.gmane.org/gmane.comp.version-control.git/219196\n>\n>     but we were so wrapped up in the greater discussion we did not apply\n>     that simple fix. :)\n\nThere's also the patch \"[PATCH v2] setup.c: set workdir when gitdir is\nnot default\" from Max Kirillov last year (gmane down, no link), so we\nhave one patch about this every year since 2013 :)\n\nJunio if you are worried about unnecessary setenv, I think Max's\napproach is safer as he solves a specific use case. If this problem\npops up again in another use case, we can deal with that again.\n\n>> +     if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))\n>> +             error(\"Could not set GIT_WORK_TREE to '%s'\", work_tree);\n>\n> Should this be die()? setenv() should basically never fail, but if it\n> does, it could be confusing and/or dangerous to run without the variable\n> set.\n\nIt's a straight copy from set_git_dir() but I guess the situation is a\nbit different. If setenv fails in gitdir, no repo is found but if it\nfails here we may select a wrong worktree and could wipe it out by\nmistake. Will fix if Junio chooses this patch instead of Max's.\n-- \nDuy\n"}]}