{"thread":{"id":"22531","subject":"[PATCH] Remove .git auto detection from setup_git_env()","startedAt":"2010-02-05T11:47:48Z","lastAt":"2010-02-05T20:29:34Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133678","messageId":"1265370468-6147-1-git-send-email-pclouds@gmail.com","threadId":"22531","inReplyTo":null,"subject":"[PATCH] Remove .git auto detection from setup_git_env()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-02-05T11:47:48Z","receivedAt":"2010-02-05T11:47:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When GIT_DIR environment variable is not specified, .git will be\nsearched if a repository is needed. Currently this can be done in two\nplaces: setup_git_directory_gently() and setup_git_env().\n\nThe one in setup_git_env() is no longer correct and should IMHO have\nbeen removed since the introduction of setup_git_directory_gently() in\nd288a70. Having two ways of auto detection may lead to obscure errors\nbecause .git may be misdetected by setup_git_env(),\nautomatically called via git_path(), which is all over the place.\n\nThis patch makes setup_git_env() die if GIT_DIR is not explictly\nset. That's setup_git_directory_gently()'s job. If you ever want to\ntouch things inside $GIT_DIR, you should have already called\nsetup_git_directory_gently().\n\nThis patch breaks commands (in a good way) and obviously not for\nmainline. I still have to go through \"make test\" to see how many are\nimpacted. But I think this is a good change. Am I missing something?\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n environment.c |    4 +---\n setup.c       |    4 +++-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 739ec27..b609569 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -67,9 +67,7 @@ static void setup_git_env(void)\n {\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n \tif (!git_dir)\n-\t\tgit_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);\n-\tif (!git_dir)\n-\t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\t\tdie(\"GIT_DIR not properly set\");\n \tgit_object_dir = getenv(DB_ENVIRONMENT);\n \tif (!git_object_dir) {\n \t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\ndiff --git a/setup.c b/setup.c\nindex 710e2f3..ae1ba52 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -396,8 +396,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tdie(\"Repository setup failed\");\n \t\t\tbreak;\n \t\t}\n-\t\tif (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))\n+\t\tif (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT)) {\n+\t\t\tset_git_dir(DEFAULT_GIT_DIR_ENVIRONMENT);\n \t\t\tbreak;\n+\t\t}\n \t\tif (is_git_directory(\".\")) {\n \t\t\tinside_git_dir = 1;\n \t\t\tif (!work_tree_env)\n-- \n1.7.0.rc0.54.gd33ef\n"},{"id":"133684","messageId":"20100205151239.GC14116@coredump.intra.peff.net","threadId":"22531","inReplyTo":"1265370468-6147-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Remove .git auto detection from setup_git_env()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-05T15:12:40Z","receivedAt":"2010-02-05T15:12:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 05, 2010 at 06:47:48PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> When GIT_DIR environment variable is not specified, .git will be\n> searched if a repository is needed. Currently this can be done in two\n> places: setup_git_directory_gently() and setup_git_env().\n> \n> The one in setup_git_env() is no longer correct and should IMHO have\n> been removed since the introduction of setup_git_directory_gently() in\n> d288a70. Having two ways of auto detection may lead to obscure errors\n> because .git may be misdetected by setup_git_env(),\n> automatically called via git_path(), which is all over the place.\n> \n> This patch makes setup_git_env() die if GIT_DIR is not explictly\n> set. That's setup_git_directory_gently()'s job. If you ever want to\n> touch things inside $GIT_DIR, you should have already called\n> setup_git_directory_gently().\n> \n> This patch breaks commands (in a good way) and obviously not for\n> mainline. I still have to go through \"make test\" to see how many are\n> impacted. But I think this is a good change. Am I missing something?\n\nIt has been a while since I looked at this code, but I think this is a\ngood direction forward. I remember trying something like this and\ngetting discouraged at all of the ensuing breakage. So if you can track\ndown all of the fallouts and fix them, I think we will be better off in\nthe long run.\n\n-Peff\n"},{"id":"133696","messageId":"7vy6j7a98f.fsf@alter.siamese.dyndns.org","threadId":"22531","inReplyTo":"20100205151239.GC14116@coredump.intra.peff.net","subject":"Re: [PATCH] Remove .git auto detection from setup_git_env()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-05T17:22:08Z","receivedAt":"2010-02-05T17:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It has been a while since I looked at this code, but I think this is a\n> good direction forward. I remember trying something like this and\n> getting discouraged at all of the ensuing breakage. So if you can track\n> down all of the fallouts and fix them, I think we will be better off in\n> the long run.\n\nI agree.\n\nThis topic started from \"We broke 'git grep' in .git directory\" and I\nthink it is the sanest to revert 3081623 (grep --no-index: allow use of\n\"git grep\" outside a git repository, 2010-01-15) which nobody has used so\nfar in any released version of git, until we sort this out at least.\n\nEvery time we touch work-tree related codepath (like René's patch does) we\nseem to inadvertently break something else.  It's too late for that kind\nof fixes for this cycle and \"grep  --no-index\" should go.\n\n-- >8 --\nSubject: [PATCH] Revert 308162372d0aa202ff45743e02253e20a4fac4d7\n\nIt seems that we have bad interaction with the code related to\nGIT_WORK_TREE and \"grep --no-index\", and broke running grep inside\nthe .git directory.  For now, just revert it and resurrect it after\n1.7.0 ships.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/RelNotes-1.7.0.txt |    3 --\n builtin-grep.c                   |   33 ------------------------\n t/t7002-grep.sh                  |   52 --------------------------------------\n 3 files changed, 0 insertions(+), 88 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.7.0.txt b/Documentation/RelNotes-1.7.0.txt\nindex 255666f..f632662 100644\n--- a/Documentation/RelNotes-1.7.0.txt\n+++ b/Documentation/RelNotes-1.7.0.txt\n@@ -133,9 +133,6 @@ Updates since v1.6.6\n  * \"git grep\" does not rely on external grep anymore.  It can use more than\n    one threads to accelerate the operation.\n \n- * \"git grep\" learned \"--no-index\" option, to search inside contents that\n-   are not managed by git.\n-\n  * \"git grep\" learned \"--quiet\" option.\n \n  * \"git log\" and friends learned \"--glob=heads/*\" syntax that is a more\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 0ef849c..bff1e68 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -14,7 +14,6 @@\n #include \"userdiff.h\"\n #include \"grep.h\"\n #include \"quote.h\"\n-#include \"dir.h\"\n \n #ifndef NO_PTHREADS\n #include \"thread-utils.h\"\n@@ -646,24 +645,6 @@ static int grep_object(struct grep_opt *opt, const char **paths,\n \tdie(\"unable to grep from object of type %s\", typename(obj->type));\n }\n \n-static int grep_directory(struct grep_opt *opt, const char **paths)\n-{\n-\tstruct dir_struct dir;\n-\tint i, hit = 0;\n-\n-\tmemset(&dir, 0, sizeof(dir));\n-\tsetup_standard_excludes(&dir);\n-\n-\tfill_directory(&dir, paths);\n-\tfor (i = 0; i < dir.nr; i++) {\n-\t\thit |= grep_file(opt, dir.entries[i]->name);\n-\t\tif (hit && opt->status_only)\n-\t\t\tbreak;\n-\t}\n-\tfree_grep_patterns(opt);\n-\treturn hit;\n-}\n-\n static int context_callback(const struct option *opt, const char *arg,\n \t\t\t    int unset)\n {\n@@ -762,8 +743,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n \t\t\t\"search in index instead of in the work tree\"),\n-\t\tOPT_BOOLEAN(0, \"index\", &use_index,\n-\t\t\t\"--no-index finds in contents not managed by git\"),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_BOOLEAN('v', \"invert-match\", &opt.invert,\n \t\t\t\"show non-matching lines\"),\n@@ -950,18 +929,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tpaths[1] = NULL;\n \t}\n \n-\tif (!use_index) {\n-\t\tint hit;\n-\t\tif (cached)\n-\t\t\tdie(\"--cached cannot be used with --no-index.\");\n-\t\tif (list.nr)\n-\t\t\tdie(\"--no-index cannot be used with revs.\");\n-\t\thit = grep_directory(&opt, paths);\n-\t\tif (use_threads)\n-\t\t\thit |= wait_all();\n-\t\treturn !hit;\n-\t}\n-\n \tif (!list.nr) {\n \t\tint hit;\n \t\tif (!cached)\ndiff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\nindex bf4d4dc..7144f81 100755\n--- a/t/t7002-grep.sh\n+++ b/t/t7002-grep.sh\n@@ -434,56 +434,4 @@ test_expect_success 'grep -Fi' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'outside of git repository' '\n-\trm -fr non &&\n-\tmkdir -p non/git/sub &&\n-\techo hello >non/git/file1 &&\n-\techo world >non/git/sub/file2 &&\n-\techo \".*o*\" >non/git/.gitignore &&\n-\t{\n-\t\techo file1:hello &&\n-\t\techo sub/file2:world\n-\t} >non/expect.full &&\n-\techo file2:world >non/expect.sub\n-\t(\n-\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non/git\" &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\tcd non/git &&\n-\t\ttest_must_fail git grep o &&\n-\t\tgit grep --no-index o >../actual.full &&\n-\t\ttest_cmp ../expect.full ../actual.full\n-\t\tcd sub &&\n-\t\ttest_must_fail git grep o &&\n-\t\tgit grep --no-index o >../../actual.sub &&\n-\t\ttest_cmp ../../expect.sub ../../actual.sub\n-\t)\n-'\n-\n-test_expect_success 'inside git repository but with --no-index' '\n-\trm -fr is &&\n-\tmkdir -p is/git/sub &&\n-\techo hello >is/git/file1 &&\n-\techo world >is/git/sub/file2 &&\n-\techo \".*o*\" >is/git/.gitignore &&\n-\t{\n-\t\techo file1:hello &&\n-\t\techo sub/file2:world\n-\t} >is/expect.full &&\n-\t: >is/expect.empty &&\n-\techo file2:world >is/expect.sub\n-\t(\n-\t\tcd is/git &&\n-\t\tgit init &&\n-\t\ttest_must_fail git grep o >../actual.full &&\n-\t\ttest_cmp ../expect.empty ../actual.full &&\n-\t\tgit grep --no-index o >../actual.full &&\n-\t\ttest_cmp ../expect.full ../actual.full &&\n-\t\tcd sub &&\n-\t\ttest_must_fail git grep o >../../actual.sub &&\n-\t\ttest_cmp ../../expect.empty ../../actual.sub &&\n-\t\tgit grep --no-index o >../../actual.sub &&\n-\t\ttest_cmp ../../expect.sub ../../actual.sub\n-\t)\n-'\n-\n test_done\n-- \n1.7.0.rc1.204.gb96e5\n"},{"id":"133697","messageId":"7vljf7a901.fsf@alter.siamese.dyndns.org","threadId":"22531","inReplyTo":"7vy6j7a98f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove .git auto detection from setup_git_env()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-05T17:27:10Z","receivedAt":"2010-02-05T17:27:10Z","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> This topic started from \"We broke 'git grep' in .git directory\" and I\n> think it is the sanest to revert 3081623 (grep --no-index: allow use of\n> \"git grep\" outside a git repository, 2010-01-15) which nobody has used so\n> far in any released version of git, until we sort this out at least.\n\nSorry; spoke too fast.  We'd also need to revert the one to git.c (7e62265\n(grep: prepare to run outside of a work tree, 2010-01-15) as well.\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex bff1e68..26d4deb 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -739,7 +739,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tconst char **paths = NULL;\n \tint i;\n \tint dummy;\n-\tint nongit = 0, use_index = 1;\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n \t\t\t\"search in index instead of in the work tree\"),\n@@ -825,8 +824,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tprefix = setup_git_directory_gently(&nongit);\n-\n \t/*\n \t * 'git grep -h', unlike 'git grep -h <pattern>', is a request\n \t * to show usage information and exit.\n@@ -864,10 +861,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION |\n \t\t\t     PARSE_OPT_NO_INTERNAL_HELP);\n \n-\tif (use_index && nongit)\n-\t\t/* die the same way as if we did it at the beginning */\n-\t\tsetup_git_directory();\n-\n \t/* First unrecognized non-option token */\n \tif (argc > 0 && !opt.pattern_list) {\n \t\tappend_grep_pattern(&opt, argv[0], \"command line\", 0,\ndiff --git a/git.c b/git.c\nindex b3e23f1..4c3028c 100644\n--- a/git.c\n+++ b/git.c\n@@ -317,7 +317,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"fsck-objects\", cmd_fsck, RUN_SETUP },\n \t\t{ \"gc\", cmd_gc, RUN_SETUP },\n \t\t{ \"get-tar-commit-id\", cmd_get_tar_commit_id },\n-\t\t{ \"grep\", cmd_grep, USE_PAGER },\n+\t\t{ \"grep\", cmd_grep, RUN_SETUP | USE_PAGER },\n \t\t{ \"hash-object\", cmd_hash_object },\n \t\t{ \"help\", cmd_help },\n \t\t{ \"index-pack\", cmd_index_pack },\n"},{"id":"133714","messageId":"4B6C7FAE.5070104@lsrfire.ath.cx","threadId":"22531","inReplyTo":"7vljf7a901.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove .git auto detection from setup_git_env()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-05T20:29:34Z","receivedAt":"2010-02-05T20:29:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.02.2010 18:27, schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> This topic started from \"We broke 'git grep' in .git directory\" and I\n>> think it is the sanest to revert 3081623 (grep --no-index: allow use of\n>> \"git grep\" outside a git repository, 2010-01-15) which nobody has used so\n>> far in any released version of git, until we sort this out at least.\n> \n> Sorry; spoke too fast.  We'd also need to revert the one to git.c (7e62265\n> (grep: prepare to run outside of a work tree, 2010-01-15) as well.\n\n>  \t\t{ \"fsck-objects\", cmd_fsck, RUN_SETUP },\n>  \t\t{ \"gc\", cmd_gc, RUN_SETUP },\n>  \t\t{ \"get-tar-commit-id\", cmd_get_tar_commit_id },\n> -\t\t{ \"grep\", cmd_grep, USE_PAGER },\n> +\t\t{ \"grep\", cmd_grep, RUN_SETUP | USE_PAGER },\n>  \t\t{ \"hash-object\", cmd_hash_object },\n>  \t\t{ \"help\", cmd_help },\n>  \t\t{ \"index-pack\", cmd_index_pack },\n\nThere is one other command with only the USE_PAGER flag turned on, which\nalso seems to have been broken by the change to remove the RUN_SETUP\nflag: shortlog.  But that was done in March 2008 (abe549e1, \"shortlog:\ndo not require to run from inside a git repository\"), so it's no regression.\n\nRené\n"}]}