{"thread":{"id":"37484","subject":"[PATCH] setup.c: set workdir when gitdir is not default","startedAt":"2014-09-03T22:42:00Z","lastAt":"2014-09-04T14:23:22Z","messageCount":5,"participants":["Max Kirillov","Duy Nguyen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"248820","messageId":"1409784120-2228-1-git-send-email-max@max630.net","threadId":"37484","inReplyTo":null,"subject":"[PATCH] setup.c: set workdir when gitdir is not default","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-09-03T22:42:00Z","receivedAt":"2014-09-03T22:42:00Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"When gitfile is used, git sets GIT_DIR environment variable for\nsubsequent commands, and that commands start working in mode \"GIT_DIR\nset, workdir current\", which is incorrect for the case when git runs\nfrom subdirectory of repository. This can be observed at least for\nrunning aliases - git fails with message \"internal error: work tree has\nalready been set\"\n\nFix by setting GIT_WORK_TREE environment also.\n\nAdd test which demonstrates problem with alias.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n setup.c            | 4 +++-\n t/t0002-gitfile.sh | 7 +++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 0a22f8b..bcf4e31 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -508,8 +508,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n \n \t/* #0, #1, #5, #8, #9, #12, #13 */\n \tset_git_work_tree(\".\");\n-\tif (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))\n+\tif (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT)) {\n \t\tset_git_dir(gitdir);\n+\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, get_git_work_tree(), 1);\n+\t}\n \tinside_git_dir = 0;\n \tinside_work_tree = 1;\n \tif (offset == len)\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex 37e9396..428cfdc 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -99,4 +99,11 @@ test_expect_success 'check rev-list' '\n \ttest \"$SHA\" = \"$(git rev-list HEAD)\"\n '\n \n+test_expect_success 'check alias call from subdirectory' '\n+\tgit config alias.testalias \"rev-parse HEAD\" &&\n+\tmkdir -p subdir &&\n+\tcd subdir &&\n+\tgit testalias\n+'\n+\n test_done\n-- \n2.0.1.1697.g73c6810\n"},{"id":"248887","messageId":"CAPig+cQMNyurCB73CzGBLcfXqap5=kZsYq0O8nD29S8QsHtrBA@mail.gmail.com","threadId":"37484","inReplyTo":"1409784120-2228-1-git-send-email-max@max630.net","subject":"Re: [PATCH] setup.c: set workdir when gitdir is not default","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-04T10:44:08Z","receivedAt":"2014-09-04T10:44:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 3, 2014 at 6:42 PM, Max Kirillov <max@max630.net> wrote:\n> When gitfile is used, git sets GIT_DIR environment variable for\n> subsequent commands, and that commands start working in mode \"GIT_DIR\n> set, workdir current\", which is incorrect for the case when git runs\n> from subdirectory of repository. This can be observed at least for\n> running aliases - git fails with message \"internal error: work tree has\n> already been set\"\n>\n> Fix by setting GIT_WORK_TREE environment also.\n>\n> Add test which demonstrates problem with alias.\n>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n>  setup.c            | 4 +++-\n>  t/t0002-gitfile.sh | 7 +++++++\n>  2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 0a22f8b..bcf4e31 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -508,8 +508,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n>\n>         /* #0, #1, #5, #8, #9, #12, #13 */\n>         set_git_work_tree(\".\");\n> -       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))\n> +       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT)) {\n>                 set_git_dir(gitdir);\n> +               setenv(GIT_WORK_TREE_ENVIRONMENT, get_git_work_tree(), 1);\n> +       }\n>         inside_git_dir = 0;\n>         inside_work_tree = 1;\n>         if (offset == len)\n> diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\n> index 37e9396..428cfdc 100755\n> --- a/t/t0002-gitfile.sh\n> +++ b/t/t0002-gitfile.sh\n> @@ -99,4 +99,11 @@ test_expect_success 'check rev-list' '\n>         test \"$SHA\" = \"$(git rev-list HEAD)\"\n>  '\n>\n> +test_expect_success 'check alias call from subdirectory' '\n> +       git config alias.testalias \"rev-parse HEAD\" &&\n\nPerhaps use test_config here?\n\n> +       mkdir -p subdir &&\n> +       cd subdir &&\n> +       git testalias\n\nIf a new test is added following this one, it will be run from within\n'subdir', which might come as a surprise as the author of the new\ntest. Wrapping the 'cd' and following code in a subshell is a good way\nto avoid the problem.\n\n    mkdir -p subdir &&\n    (\n        cd subdir &&\n        git testalias\n    )\n\n> +'\n> +\n>  test_done\n> --\n> 2.0.1.1697.g73c6810\n"},{"id":"248828","messageId":"CACsJy8CS3jC54bTh8ySmpAmbQOycMvvnH=yMES0LJQ4zWkqLrA@mail.gmail.com","threadId":"37484","inReplyTo":"1409784120-2228-1-git-send-email-max@max630.net","subject":"Re: [PATCH] setup.c: set workdir when gitdir is not default","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-04T10:53:34Z","receivedAt":"2014-09-04T10:53:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Sep 4, 2014 at 5:42 AM, Max Kirillov <max@max630.net> wrote:\n> diff --git a/setup.c b/setup.c\n> index 0a22f8b..bcf4e31 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -508,8 +508,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n>\n>         /* #0, #1, #5, #8, #9, #12, #13 */\n>         set_git_work_tree(\".\");\n\nI wonder if we should setenv(GIT_WORK_TREE_) from inside this function\ninstead. A quick glance over 'git grep set_git_work_tree' gives me the\nimpression that it's safe to do so, and could cover future bugs\nsimilar to this.\n\n> -       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))\n> +       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT)) {\n>                 set_git_dir(gitdir);\n> +               setenv(GIT_WORK_TREE_ENVIRONMENT, get_git_work_tree(), 1);\n> +       }\n>         inside_git_dir = 0;\n>         inside_work_tree = 1;\n>         if (offset == len)\n-- \nDuy\n"},{"id":"248838","messageId":"20140904142015.GA8894@wheezy.local","threadId":"37484","inReplyTo":"CACsJy8CS3jC54bTh8ySmpAmbQOycMvvnH=yMES0LJQ4zWkqLrA@mail.gmail.com","subject":"Re: [PATCH] setup.c: set workdir when gitdir is not default","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-09-04T14:20:15Z","receivedAt":"2014-09-04T14:20:15Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Sep 04, 2014 at 05:53:34PM +0700, Duy Nguyen wrote:\n> On Thu, Sep 4, 2014 at 5:42 AM, Max Kirillov <max@max630.net> wrote:\n>>         /* #0, #1, #5, #8, #9, #12, #13 */\n>>         set_git_work_tree(\".\");\n> \n> I wonder if we should setenv(GIT_WORK_TREE_) from inside this function\n> instead. A quick glance over 'git grep set_git_work_tree' gives me the\n> impression that it's safe to do so, and could cover future bugs\n> similar to this.\n\nActually, I've been thinking about just the oppozite: remove\nthe setenv(GIT_DIR) from the discovery code at all. Because\nbackend performs the same discovery, and it would find the\nsame git directory itself. And leave environment variables to\nbe only set by user.\n\nCurrently environment variables are used for both purposes:\nuser can use them for hacky stuff, and git itself use them\nfor passing internal information. But, as we can see,\nsetting the variables can have bigger effect than expected.\nWhen they are set by user, it's ok, let him try, fail and\nset some else variable to adapt for his needs. But for git\ncode there is the discovery routines, lets just use them\nalways, it will cause a couple of extra (very few) IO calls\nbut should always give same (correct) result\n\n-- \nMax\n"},{"id":"248839","messageId":"20140904142322.GB8894@wheezy.local","threadId":"37484","inReplyTo":"CAPig+cQMNyurCB73CzGBLcfXqap5=kZsYq0O8nD29S8QsHtrBA@mail.gmail.com","subject":"Re: [PATCH] setup.c: set workdir when gitdir is not default","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-09-04T14:23:22Z","receivedAt":"2014-09-04T14:23:22Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Sep 04, 2014 at 06:44:08AM -0400, Eric Sunshine wrote:\n> > +       mkdir -p subdir &&\n> > +       cd subdir &&\n> > +       git testalias\n> \n> If a new test is added following this one, it will be run from within\n> 'subdir', which might come as a surprise as the author of the new\n> test. Wrapping the 'cd' and following code in a subshell is a good way\n> to avoid the problem.\n\nSure, my mistake. Will fix this and the other issue you\nmentioned. Thank you.\n\n-- \nMax\n"}]}