{"thread":{"id":"22509","subject":"[PATCH] grep: simple test for operation in a bare repository","startedAt":"2010-02-03T18:16:28Z","lastAt":"2010-02-06T18:47:29Z","messageCount":7,"participants":["René Scharfe","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133494","messageId":"4B69BD7C.4010608@lsrfire.ath.cx","threadId":"22509","inReplyTo":null,"subject":"[PATCH] grep: simple test for operation in a bare repository","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-03T18:16:28Z","receivedAt":"2010-02-03T18:16:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n t/t7002-grep.sh |   17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\nindex bf4d4dc..8cf958d 100755\n--- a/t/t7002-grep.sh\n+++ b/t/t7002-grep.sh\n@@ -45,6 +45,23 @@ test_expect_success 'grep should not segfault with a bad input' '\n \ttest_must_fail git grep \"(\"\n '\n \n+bare_repo=.git/bare_test_repo\n+test_expect_success 'setup bare repo' '\n+\tgit clone --bare . $bare_repo\n+'\n+\n+test_expect_success \"grep HEAD (t-1), bare repo\" '(\n+\tcd $bare_repo &&\n+\techo \"HEAD:t/t:1:test\" >expected &&\n+\tgit grep -n -e test HEAD >actual &&\n+\tdiff expected actual\n+)'\n+\n+test_expect_success \"grep (t-1), bare repo, must fail\" '(\n+\tcd $bare_repo &&\n+\ttest_must_fail git grep -n -e test\n+)'\n+\n for H in HEAD ''\n do\n \tcase \"$H\" in\n-- \n1.7.0.rc1\n"},{"id":"133552","messageId":"4B6A0BDE.2050908@lsrfire.ath.cx","threadId":"22509","inReplyTo":"4B69BD7C.4010608@lsrfire.ath.cx","subject":"Re: [PATCH] grep: simple test for operation in a bare repository","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-03T23:50:54Z","receivedAt":"2010-02-03T23:50:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.02.2010 19:16, schrieb René Scharfe:\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n>  t/t7002-grep.sh |   17 +++++++++++++++++\n>  1 files changed, 17 insertions(+), 0 deletions(-)\n\nErr, no, that won't do.  Sorry.\n\nThe test script fails to demonstrate the issue I've run into.  It runs\nsuccessfully, but running git grep manually fails:\n\n\t$ cd t/trash\\ directory.t7002-grep/.git/bare_test_repo/\n\t$ git grep bla HEAD\n\tfatal: This operation must be run in a work tree\n\nI have to dig a bit deeper and try to come back with a better test script.\n\nRené\n"},{"id":"133648","messageId":"4B6B653A.8060909@lsrfire.ath.cx","threadId":"22509","inReplyTo":"4B6A0BDE.2050908@lsrfire.ath.cx","subject":"Re: [PATCH] grep: simple test for operation in a bare repository","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-05T00:24:26Z","receivedAt":"2010-02-05T00:24:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.02.2010 00:50, schrieb René Scharfe:\n> Am 03.02.2010 19:16, schrieb René Scharfe:\n>> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n>> ---\n>>  t/t7002-grep.sh |   17 +++++++++++++++++\n>>  1 files changed, 17 insertions(+), 0 deletions(-)\n> \n> Err, no, that won't do.  Sorry.\n> \n> The test script fails to demonstrate the issue I've run into.  It runs\n> successfully, but running git grep manually fails:\n> \n> \t$ cd t/trash\\ directory.t7002-grep/.git/bare_test_repo/\n> \t$ git grep bla HEAD\n> \tfatal: This operation must be run in a work tree\n> \n> I have to dig a bit deeper and try to come back with a better test script.\n\nOK, I have to admit defeat: I can't come up with a test script.  But\nthe issue is reproducible: git grep in a bare repository fails when\nrun with a pager.\n\n\t$ mkdir /tmp/a\n\t$ cd /tmp/a\n\t$ git init\n\tInitialized empty Git repository in /tmp/a/.git/\n\t$ echo a >a\n\t$ git add a\n\t$ git commit -m.\n\t[master (root-commit) e11f955] .\n\t 1 files changed, 1 insertions(+), 0 deletions(-)\n\t create mode 100644 a\n\n\t$ git clone --bare . ../b\n\tInitialized empty Git repository in /tmp/b/\n\t$ cd /tmp/b\n\n\t$ git grep a HEAD\n\tfatal: This operation must be run in a work tree\n\t$ git grep a HEAD | cat\n\tHEAD:a:a\n\t$ git --no-pager grep a HEAD\n\tHEAD:a:a\n\nReverting 7e622650 (grep: prepare to run outside of a work tree), or\nrather just setting the flag RUN_SETUP for grep in git.c again, makes\nthe first git grep call succeed, too.\n\nAs does the following patch, but I don't know why.  The call chain is\nquite deep.  It seems that without the patch the static variable\ngit_dir in environment.c isn't updated when git finds out that it runs\nin a bare repo -- but only if a pager is used.\n\nThere are five more sites in git.c, path.c and setup.c where $GIT_DIR\nis set directly with setenv().  I wonder if they should better call\nset_git_dir() instead, too.\n\n\ndiff --git a/setup.c b/setup.c\nindex 710e2f3..5fb9b25 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -406,7 +406,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tcwd[offset] = '\\0';\n \t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 1);\n \t\t\t} else\n-\t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\t\t\tset_git_dir(\".\");\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\n"},{"id":"133659","messageId":"fcaeb9bf1002041840l4d8e8ac9k3ad5e9e8761aa1b@mail.gmail.com","threadId":"22509","inReplyTo":"4B6B653A.8060909@lsrfire.ath.cx","subject":"Re: [PATCH] grep: simple test for operation in a bare repository","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-02-05T02:40:43Z","receivedAt":"2010-02-05T02:40:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 5, 2010 at 7:24 AM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> OK, I have to admit defeat: I can't come up with a test script.  But\n> the issue is reproducible: git grep in a bare repository fails when\n> run with a pager.\n>\n>        $ mkdir /tmp/a\n>        $ cd /tmp/a\n>        $ git init\n>        Initialized empty Git repository in /tmp/a/.git/\n>        $ echo a >a\n>        $ git add a\n>        $ git commit -m.\n>        [master (root-commit) e11f955] .\n>         1 files changed, 1 insertions(+), 0 deletions(-)\n>         create mode 100644 a\n>\n>        $ git clone --bare . ../b\n>        Initialized empty Git repository in /tmp/b/\n>        $ cd /tmp/b\n>\n>        $ git grep a HEAD\n>        fatal: This operation must be run in a work tree\n>        $ git grep a HEAD | cat\n>        HEAD:a:a\n>        $ git --no-pager grep a HEAD\n>        HEAD:a:a\n>\n> Reverting 7e622650 (grep: prepare to run outside of a work tree), or\n> rather just setting the flag RUN_SETUP for grep in git.c again, makes\n> the first git grep call succeed, too.\n>\n> As does the following patch, but I don't know why.  The call chain is\n> quite deep.  It seems that without the patch the static variable\n> git_dir in environment.c isn't updated when git finds out that it runs\n> in a bare repo -- but only if a pager is used.\n\nsetup_pager() calls git_config(), which indirectly calls get_git_dir()\nand sets git_dir in stone. Changing GIT_DIR environment variable alone\nwon't work, as you have seen.\n\nWhen RUN_SETUP is set, setup_git_directory() would be called before\nsetup_pager() can kick in, so everything is properly set.\n\n> There are five more sites in git.c, path.c and setup.c where $GIT_DIR\n> is set directly with setenv().  I wonder if they should better call\n> set_git_dir() instead, too.\n\nYes, they should.\n-- \nDuy\n"},{"id":"133667","messageId":"7vd40kjhvm.fsf@alter.siamese.dyndns.org","threadId":"22509","inReplyTo":"4B6B653A.8060909@lsrfire.ath.cx","subject":"Re: [PATCH] grep: simple test for operation in a bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-05T06:50:37Z","receivedAt":"2010-02-05T06:50:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> There are five more sites in git.c, path.c and setup.c where $GIT_DIR\n> is set directly with setenv().  I wonder if they should better call\n> set_git_dir() instead, too.\n>\n>\n> diff --git a/setup.c b/setup.c\n> index 710e2f3..5fb9b25 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -406,7 +406,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\t\t\tcwd[offset] = '\\0';\n>  \t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 1);\n>  \t\t\t} else\n> -\t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n> +\t\t\t\tset_git_dir(\".\");\n>  \t\t\tcheck_repository_format_gently(nongit_ok);\n>  \t\t\treturn NULL;\n>  \t\t}\n\nYeah, shouldn't the other setenv() we see in the context need a similar\nchange as well?\n"},{"id":"133787","messageId":"4B6D37D7.6010505@lsrfire.ath.cx","threadId":"22509","inReplyTo":"fcaeb9bf1002041840l4d8e8ac9k3ad5e9e8761aa1b@mail.gmail.com","subject":"Re: [PATCH] grep: simple test for operation in a bare repository","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-06T09:35:19Z","receivedAt":"2010-02-06T09:35:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.02.2010 03:40, schrieb Nguyen Thai Ngoc Duy:\n> setup_pager() calls git_config(), which indirectly calls get_git_dir()\n> and sets git_dir in stone. Changing GIT_DIR environment variable alone\n> won't work, as you have seen.\n> \n> When RUN_SETUP is set, setup_git_directory() would be called before\n> setup_pager() can kick in, so everything is properly set.\n> \n>> There are five more sites in git.c, path.c and setup.c where $GIT_DIR\n>> is set directly with setenv().  I wonder if they should better call\n>> set_git_dir() instead, too.\n> \n> Yes, they should.\n\nThis patch converts the setenv() calls in path.c and setup.c.  After\nthe call, git grep with a pager works again in bare repos.\n\nIt leaves the setenv(GIT_DIR_ENVIRONMENT, ...) calls in git.c alone, as\nthey respond to command line switches that emulate the effect of setting\nthe environment variable directly.\n\nThe remaining site in environment.c is in set_git_dir() and is left\nalone, too, of course.  Finally, builtin-init-db.c is left changed\nbecause the repo is still being carefully constructed when the\nenvironment variable is set.\n\nThis fixes git shortlog when run inside a git directory, which had been\nbroken by abe549e1.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nSince it's doesn't fix a regression (abe549e1 was committed in March\n2008), this patch doesn't have to go in at this point in the release\ncycle.  And perhaps it's even superseded by the more general fix Duy\nis working on?\n\n path.c  |    2 +-\n setup.c |    4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 79aa104..0005df3 100644\n--- a/path.c\n+++ b/path.c\n@@ -336,7 +336,7 @@ char *enter_repo(char *path, int strict)\n  \tif (access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\n \t    validate_headref(\"HEAD\") == 0) {\n-\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\tset_git_dir(\".\");\n \t\tcheck_repository_format();\n \t\treturn path;\n \t}\ndiff --git a/setup.c b/setup.c\nindex 710e2f3..b38cbee 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -404,9 +404,9 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tinside_work_tree = 0;\n \t\t\tif (offset != len) {\n \t\t\t\tcwd[offset] = '\\0';\n-\t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 1);\n+\t\t\t\tset_git_dir(cwd);\n \t\t\t} else\n-\t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\t\t\tset_git_dir(\".\");\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\n"},{"id":"133818","messageId":"7v7hqqyzem.fsf@alter.siamese.dyndns.org","threadId":"22509","inReplyTo":"4B6D37D7.6010505@lsrfire.ath.cx","subject":"Re: [PATCH] grep: simple test for operation in a bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-06T18:47:29Z","receivedAt":"2010-02-06T18:47:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> This patch converts the setenv() calls in path.c and setup.c.  After\n> the call, git grep with a pager works again in bare repos.\n>\n> It leaves the setenv(GIT_DIR_ENVIRONMENT, ...) calls in git.c alone, as\n> they respond to command line switches that emulate the effect of setting\n> the environment variable directly.\n>\n> The remaining site in environment.c is in set_git_dir() and is left\n> alone, too, of course.  Finally, builtin-init-db.c is left changed\n> because the repo is still being carefully constructed when the\n> environment variable is set.\n\nThanks for a thorough analysis.  The patch looks correct and I am very\ntempted to put it into 1.7.0, though I'd stop at queuing it to 'next'.\n"}]}