{"thread":{"id":"41626","subject":"[PATCH 0/6] avoid touching ref code in non-repositories","startedAt":"2016-03-05T22:08:29Z","lastAt":"2016-03-07T15:51:21Z","messageCount":10,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"280263","messageId":"20160305220829.GA31316@sigill.intra.peff.net","threadId":"41626","inReplyTo":null,"subject":"[PATCH 0/6] avoid touching ref code in non-repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T22:08:29Z","receivedAt":"2016-03-05T22:08:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's the series I mentioned earlier, to avoid avoid accessing the ref\ncode if we know we aren't in a repository. For those just joining us,\nthe end goal is to be able to detect and complain when code tries to\nopen a ref without having run check_repository_format() or similar. Once\nwe have pluggable ref backends, doing so would be wrong, since we won't\nknow which backend we're supposed to be using.\n\nIt's already _kind of_ wrong, in the sense that we should not be looking\nfor file-backend refs if we know we don't have a repository. But nobody\nreally noticed in practice, because you do not generally have\n\".git/HEAD\" lying around in non-repository directories. So it was a de\nfacto noop, though you could construct pathological cases where it did\nthe wrong thing.\n\nThe patches are:\n\n [1/6]: setup: make startup_info available everywhere\n [2/6]: setup: set startup_info->have_repository more reliably\n [3/6]: remote: don't resolve HEAD in non-repository\n [4/6]: mailmap: do not resolve blobs in a non-repository\n [5/6]: grep: turn off gitlink detection for --no-index\n [6/6]: use setup_git_directory() in test-* programs\n\nThere's a sort-of \"7/6\" I used to find these spots:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 81f68f8..22c1b6d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -947,6 +947,8 @@ static struct ref_cache *lookup_ref_cache(const char *submodule)\n {\n \tstruct ref_cache *refs;\n \n+\tassert(startup_info->have_repository);\n+\n \tif (!submodule || !*submodule)\n \t\treturn &ref_cache;\n \n@@ -1414,6 +1416,8 @@ static const char *resolve_ref_1(const char *refname,\n \tint depth = MAXDEPTH;\n \tint bad_name = 0;\n \n+\tassert(startup_info->have_repository);\n+\n \tif (flags)\n \t\t*flags = 0;\n \n\nRunning the test suite with that patch and 1/6 turned up the cases I've\nfixed here. But I'm not including it here for a few reasons:\n\n 1. I'm not sure that asserting is the best behavior. It's great for\n finding problems (and that's why I used assert() -- to get a\n coredump for the backtrace), but in practice it might be friendlier\n to turn it into a silent noop (\"no, I don't even have a ref\n backend, so your ref definitely does not exist\").\n\n 2. In a pluggable ref-backend world, this isn't where we'd put the\n asserts, anyway. We'd do it when accessing the ref_backend vtable\n (or just starting with a dummy vtable that asserts, or returns\n \"nope, no refs\", or whatever).\n\nSo it was useful for debugging/analysis now, and we may want something\nlike it later, but I think that should come on top the ref-backend\nseries.\n\n-Peff\n"},{"id":"280264","messageId":"20160305221027.GA31508@sigill.intra.peff.net","threadId":"41626","inReplyTo":"20160305220829.GA31316@sigill.intra.peff.net","subject":"[PATCH 1/6] setup: make startup_info available everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T22:10:27Z","receivedAt":"2016-03-05T22:10:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit a60645f (setup: remember whether repository was\nfound, 2010-08-05) introduced the startup_info structure,\nwhich records some parts of the setup_git_directory()\nprocess (notably, whether we actually found a repository or\nnot).\n\nOne of the uses of this data is for functions to behave\nappropriately based on whether we are in a repo. But the\nstartup_info struct is just a pointer to storage provided by\nthe main program, and the only program that sets it up is\nthe git.c wrapper. Thus builtins have access to\nstartup_info, but externally linked programs do not.\n\nWorse, library code which is accessible from both has to be\ncareful about accessing startup_info. This can be used to\ntrigger a die(\"BUG\") via get_sha1():\n\n\t$ git fast-import <<-\\EOF\n\ttag foo\n\tfrom HEAD:./whatever\n\tEOF\n\n\tfatal: BUG: startup_info struct is not initialized.\n\nObviously that's fairly nonsensical input to feed to\nfast-import, but we should never hit a die(\"BUG\"). And there\nmay be other ways to trigger it if other non-builtins\nresolve sha1s.\n\nSo let's point the storage for startup_info to a static\nvariable in setup.c, making it available to all users of the\nlibrary code. We _could_ turn startup_info into a regular\nextern struct, but doing so would mean tweaking all of the\nexisting use sites. So let's leave the pointer indirection\nin place. We can, however, drop any checks for NULL, as\nthey will always be false (and likewise, we can drop the\ntest covering this case, which was a rather artificial\nsituation using one of the test-* programs).\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n environment.c | 1 -\n git.c | 3 ---\n setup.c | 10 ++++++----\n sha1_name.c | 3 ---\n t/t1506-rev-parse-diagnosis.sh | 5 -----\n 6 files changed, 7 insertions(+), 17 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b829410..9f09540 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1771,7 +1771,7 @@ int split_cmdline(char *cmdline, const char ***argv);\n /* Takes a negative value returned by split_cmdline */\n const char *split_cmdline_strerror(int cmdline_errno);\n \n-/* git.c */\n+/* setup.c */\n struct startup_info {\n \tint have_repository;\n \tconst char *prefix;\ndiff --git a/environment.c b/environment.c\nindex 6dec9d0..6cc0a77 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -64,7 +64,6 @@ int grafts_replace_parents = 1;\n int core_apply_sparse_checkout;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n-struct startup_info *startup_info;\n unsigned long pack_size_limit_cfg;\n \n #ifndef PROTECT_HFS_DEFAULT\ndiff --git a/git.c b/git.c\nindex 6cc0c07..968a8a4 100644\n--- a/git.c\n+++ b/git.c\n@@ -15,7 +15,6 @@ const char git_more_info_string[] =\n \t \"concept guides. See 'git help ' or 'git help '\\n\"\n \t \"to read about a specific subcommand or concept.\");\n \n-static struct startup_info git_startup_info;\n static int use_pager = -1;\n static char *orig_cwd;\n static const char *env_names[] = {\n@@ -637,8 +636,6 @@ int main(int argc, char **av)\n \tconst char *cmd;\n \tint done_help = 0;\n \n-\tstartup_info = &git_startup_info;\n-\n \tcmd = git_extract_argv0_path(argv[0]);\n \tif (!cmd)\n \t\tcmd = \"git-help\";\ndiff --git a/setup.c b/setup.c\nindex de1a2a7..fa7a306 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -7,6 +7,9 @@ static int inside_work_tree = -1;\n static int work_tree_config_is_bogus;\n static struct string_list unknown_extensions = STRING_LIST_INIT_DUP;\n \n+static struct startup_info the_startup_info;\n+struct startup_info *startup_info = &the_startup_info;\n+\n /*\n * The input parameter must contain an absolute path, and it must already be\n * normalized.\n@@ -905,10 +908,9 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \telse\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n \n-\tif (startup_info) {\n-\t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n-\t\tstartup_info->prefix = prefix;\n-\t}\n+\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n+\tstartup_info->prefix = prefix;\n+\n \treturn prefix;\n }\n \ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3acf221..776101e 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1353,9 +1353,6 @@ static char *resolve_relative_path(const char *rel)\n \tif (!starts_with(rel, \"./\") && !starts_with(rel, \"../\"))\n \t\treturn NULL;\n \n-\tif (!startup_info)\n-\t\tdie(\"BUG: startup_info struct is not initialized.\");\n-\n \tif (!is_inside_work_tree())\n \t\tdie(\"relative path syntax can't be used outside working tree.\");\n \ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 613d9bf..86c2ff2 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -166,11 +166,6 @@ test_expect_success 'relative path when cwd is outside worktree' '\n \tgrep \"relative path syntax can.t be used outside working tree.\" error\n '\n \n-test_expect_success 'relative path when startup_info is NULL' '\n-\ttest_must_fail test-match-trees HEAD:./file.txt HEAD:./file.txt 2>error &&\n-\tgrep \"BUG: startup_info struct is not initialized.\" error\n-'\n-\n test_expect_success ':file correctly diagnosed after a pathname' '\n \ttest_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&\n \ttest_i18ngrep ! \"exists on disk\" error &&\n-- \n2.8.0.rc1.318.g2193183\n"},{"id":"280265","messageId":"20160305221134.GB31508@sigill.intra.peff.net","threadId":"41626","inReplyTo":"20160305220829.GA31316@sigill.intra.peff.net","subject":"[PATCH 2/6] setup: set startup_info->have_repository more reliably","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T22:11:34Z","receivedAt":"2016-03-05T22:11:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When setup_git_directory() is called, we set a flag in\nstartup_info to indicate we have a repository. But there are\na few other mechanisms by which we might set up a repo:\n\n 1. When creating a new repository via init_db(), we\n transition from no-repo to being in a repo. We should\n tweak this flag at that moment.\n\n 2. In enter_repo(), a stricter form of\n setup_git_directory() used by server-side programs, we\n check the repository format config. After doing so, we\n know we're in a repository, and can set the flag.\n\nWith these changes, library code can now reliably tell\nwhether we are in a repository and act accordingly. We'll\nleave the \"prefix\" field as NULL, which is what happens when\nsetup_git_directory() finds there is no prefix.\n\nSigned-off-by: Jeff King \n---\nI did these patches directly on master. When applied on top of the\nother check_repository_format series I posted a few days ago, this one\nwill have a minor textual conflict (that other series dropped the\npointless return from this function).\n\n builtin/init-db.c | 1 +\n setup.c | 4 +++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 6223b7d..da531f6 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -322,6 +322,7 @@ int set_git_dir_init(const char *git_dir, const char *real_git_dir,\n \t\tset_git_dir(real_path(git_dir));\n \t\tgit_link = NULL;\n \t}\n+\tstartup_info->have_repository = 1;\n \treturn 0;\n }\n \ndiff --git a/setup.c b/setup.c\nindex fa7a306..3439ec6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -986,7 +986,9 @@ int check_repository_format_version(const char *var, const char *value, void *cb\n \n int check_repository_format(void)\n {\n-\treturn check_repository_format_gently(get_git_dir(), NULL);\n+\tcheck_repository_format_gently(get_git_dir(), NULL);\n+\tstartup_info->have_repository = 1;\n+\treturn 0;\n }\n \n /*\n-- \n2.8.0.rc1.318.g2193183\n"},{"id":"280266","messageId":"20160305221157.GC31508@sigill.intra.peff.net","threadId":"41626","inReplyTo":"20160305220829.GA31316@sigill.intra.peff.net","subject":"[PATCH 3/6] remote: don't resolve HEAD in non-repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T22:11:57Z","receivedAt":"2016-03-05T22:11:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The remote-config code wants to look at HEAD to mark the\ncurrent branch specially. But if we are not in a repository\n(e.g., running \"git archive --remote\"), this makes no sense;\nthere is no HEAD to look at, and we have no current branch.\n\nThis doesn't really cause any bugs in practice (if you are\nnot in a repo, you probably don't have a .git/HEAD file),\nbut we should be more careful about triggering the refs code\nat all in a non-repo. As we grow new ref backends, we would\nnot even know which backend to use.\n\nSigned-off-by: Jeff King \n---\n remote.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex fc02698..28fd676 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -455,7 +455,6 @@ static void read_config(void)\n {\n \tstatic int loaded;\n \tstruct object_id oid;\n-\tconst char *head_ref;\n \tint flag;\n \n \tif (loaded)\n@@ -463,10 +462,12 @@ static void read_config(void)\n \tloaded = 1;\n \n \tcurrent_branch = NULL;\n-\thead_ref = resolve_ref_unsafe(\"HEAD\", 0, oid.hash, &flag);\n-\tif (head_ref && (flag & REF_ISSYMREF) &&\n-\t skip_prefix(head_ref, \"refs/heads/\", &head_ref)) {\n-\t\tcurrent_branch = make_branch(head_ref, 0);\n+\tif (startup_info->have_repository) {\n+\t\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\", 0, oid.hash, &flag);\n+\t\tif (head_ref && (flag & REF_ISSYMREF) &&\n+\t\t skip_prefix(head_ref, \"refs/heads/\", &head_ref)) {\n+\t\t\tcurrent_branch = make_branch(head_ref, 0);\n+\t\t}\n \t}\n \tgit_config(handle_config, NULL);\n \talias_all_urls();\n-- \n2.8.0.rc1.318.g2193183\n"},{"id":"280267","messageId":"20160305221329.GD31508@sigill.intra.peff.net","threadId":"41626","inReplyTo":"20160305220829.GA31316@sigill.intra.peff.net","subject":"[PATCH 4/6] mailmap: do not resolve blobs in a non-repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T22:13:29Z","receivedAt":"2016-03-05T22:13:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The mailmap code may be triggered outside of a repository by\ngit-shortlog. There is no point in looking up a name like\n\"HEAD:.mailmap\" there; without a repository, we have no\nrefs.\n\nThis is unlikely to matter much in practice for the current\ncode, as we would simply fail to find the ref. But as the\nrefs code learns about new backends, this is more important;\nwithout a repository, we do not even know which backend to\nlook at.\n\nSigned-off-by: Jeff King \n---\nI debated whether \".mailmap\" in the $PWD should be respected in a\nnon-repository. If it were 2005, I think I could go either way. But it's\nnot, and changing it now seems like a pointless and unnecessary\nregression, even if there is some grounds to argue for it.\n\n mailmap.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex f4a0f1c..9726237 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -250,7 +250,8 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)\n \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n \n \terr |= read_mailmap_file(map, \".mailmap\", repo_abbrev);\n-\terr |= read_mailmap_blob(map, git_mailmap_blob, repo_abbrev);\n+\tif (startup_info->have_repository)\n+\t\terr |= read_mailmap_blob(map, git_mailmap_blob, repo_abbrev);\n \terr |= read_mailmap_file(map, git_mailmap_file, repo_abbrev);\n \treturn err;\n }\n-- \n2.8.0.rc1.318.g2193183\n"},{"id":"280268","messageId":"20160305221551.GE31508@sigill.intra.peff.net","threadId":"41626","inReplyTo":"20160305220829.GA31316@sigill.intra.peff.net","subject":"[PATCH 5/6] grep: turn off gitlink detection for --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T22:15:51Z","receivedAt":"2016-03-05T22:15:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are running \"git grep --no-index\" outside of a git\nrepository, we behave roughly like \"grep -r\", examining all\nfiles in the current directory and its subdirectories.\nHowever, because we use fill_directory() to do the\nrecursion, it will skip over any directories which look like\nsub-repositories.\n\nFor a normal git operation (like \"git grep\" in a repository)\nthis makes sense; we do not want to cross the boundary out\nof our current repository into a submodule. But for\n\"--no-index\" without a repository, we should look at all\nfiles, including embedded repositories.\n\nThere is one exception, though: we probably should _not_\ndescend into \".git\" directories. Doing so is inefficient and\nunlikely to turn up useful hits.\n\nThis patch drops our use of dir.c's gitlink-detection, but\nwe do still avoid \".git\". That makes us more like tools such\nas \"ack\" or \"ag\", which also know to avoid cruft in .git.\n\nAs a bonus, this also drops our usage of the ref code\nwhen we are outside of a repository, making the transition\nto pluggable ref backends cleaner.\n\nBased-on-a-patch-by: David Turner \nSigned-off-by: Jeff King \n---\nI hope the reasoning above makes sense. My ulterior motive is the\n\"bonus\", but I really think the new behavior is what people would expect\n(i.e., that \"git grep --no-index\" is basically a replacement for \"ack\",\netc).\n\n builtin/grep.c | 1 +\n t/t7810-grep.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex aa7435f..0636cd7 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -528,6 +528,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \tint i, hit = 0;\n \n \tmemset(&dir, 0, sizeof(dir));\n+\tdir.flags |= DIR_NO_GITLINKS;\n \tif (exc_std)\n \t\tsetup_standard_excludes(&dir);\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex b540944..1e72971 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -905,6 +905,33 @@ test_expect_success 'inside git repository but with --no-index' '\n \t)\n '\n \n+test_expect_success 'grep --no-index descends into repos, but not .git' '\n+\trm -fr non &&\n+\tmkdir -p non/git &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\n+\t\techo magic >file &&\n+\t\tgit init repo &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\techo magic >file &&\n+\t\t\tgit add file &&\n+\t\t\tgit commit -m foo &&\n+\t\t\techo magic >.git/file\n+\t\t) &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfile\n+\t\trepo/file\n+\t\tEOF\n+\t\tgit grep -l --no-index magic >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'setup double-dash tests' '\n cat >double-dash <\n---\nAs discussed elsewhere, test-match-trees isn't actually used in the test\nsuite (except for the weird use which was removed in patch 1 here). So\nwe could also just get rid of it, but maybe somebody uses it for\ndebugging or something.\n\n test-match-trees.c | 2 ++\n test-revision-walking.c | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/test-match-trees.c b/test-match-trees.c\nindex 109f03e..4dad709 100644\n--- a/test-match-trees.c\n+++ b/test-match-trees.c\n@@ -6,6 +6,8 @@ int main(int ac, char **av)\n \tunsigned char hash1[20], hash2[20], shifted[20];\n \tstruct tree *one, *two;\n \n+\tsetup_git_directory();\n+\n \tif (get_sha1(av[1], hash1))\n \t\tdie(\"cannot parse %s as an object name\", av[1]);\n \tif (get_sha1(av[2], hash2))\ndiff --git a/test-revision-walking.c b/test-revision-walking.c\nindex 285f06b..3d03133 100644\n--- a/test-revision-walking.c\n+++ b/test-revision-walking.c\n@@ -50,6 +50,8 @@ int main(int argc, char **argv)\n \tif (argc < 2)\n \t\treturn 1;\n \n+\tsetup_git_directory();\n+\n \tif (!strcmp(argv[1], \"run-twice\")) {\n \t\tprintf(\"1st\\n\");\n \t\tif (!run_revision_walk())\n-- \n2.8.0.rc1.318.g2193183\n"},{"id":"280311","messageId":"xmqqoaarrs9o.fsf@gitster.mtv.corp.google.com","threadId":"41626","inReplyTo":"20160305221650.GF31508@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] use setup_git_directory() in test-* programs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T01:28:03Z","receivedAt":"2016-03-07T01:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Some of the test-* programs rely on examining refs, but did\n> not bother to make sure we are actually in a git repository.\n> Let's have them call setup_git_directory() to do so.\n>\n> Signed-off-by: Jeff King \n> ---\n> As discussed elsewhere, test-match-trees isn't actually used in the test\n> suite (except for the weird use which was removed in patch 1 here). So\n> we could also just get rid of it, but maybe somebody uses it for\n> debugging or something.\n\nI tend to agree that it is very tempting to remove that one. It was\na throw-away program I wrote early while inventing the subtree merge\nstrategy.\n\nIt would be useful only to those who want to extend the heuristics\nof figuring out how to shift two trees to make them match, without\nhaving to exercise the whole merge-recursive machinery.\n"},{"id":"280312","messageId":"xmqqk2lfrs82.fsf@gitster.mtv.corp.google.com","threadId":"41626","inReplyTo":"20160305221551.GE31508@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] grep: turn off gitlink detection for --no-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T01:29:01Z","receivedAt":"2016-03-07T01:29:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we are running \"git grep --no-index\" outside of a git\n> repository, we behave roughly like \"grep -r\", examining all\n> files in the current directory and its subdirectories.\n> However, because we use fill_directory() to do the\n> recursion, it will skip over any directories which look like\n> sub-repositories.\n>\n> For a normal git operation (like \"git grep\" in a repository)\n> this makes sense; we do not want to cross the boundary out\n> of our current repository into a submodule. But for\n> \"--no-index\" without a repository, we should look at all\n> files, including embedded repositories.\n>\n> There is one exception, though: we probably should _not_\n> descend into \".git\" directories. Doing so is inefficient and\n> unlikely to turn up useful hits.\n>\n> This patch drops our use of dir.c's gitlink-detection, but\n> we do still avoid \".git\". That makes us more like tools such\n> as \"ack\" or \"ag\", which also know to avoid cruft in .git.\n>\n> As a bonus, this also drops our usage of the ref code\n> when we are outside of a repository, making the transition\n> to pluggable ref backends cleaner.\n>\n> Based-on-a-patch-by: David Turner \n> Signed-off-by: Jeff King \n> ---\n> I hope the reasoning above makes sense. My ulterior motive is the\n> \"bonus\", but I really think the new behavior is what people would expect\n> (i.e., that \"git grep --no-index\" is basically a replacement for \"ack\",\n> etc).\n\nI agree with --no-index part, but the caller of grep_directory() is\n\"either no-index, or untracked\". I am not sure if the latter wants\nthis new behaviour.\n\n>\n> builtin/grep.c | 1 +\n> t/t7810-grep.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 28 insertions(+)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index aa7435f..0636cd7 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -528,6 +528,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n> \tint i, hit = 0;\n> \n> \tmemset(&dir, 0, sizeof(dir));\n> +\tdir.flags |= DIR_NO_GITLINKS;\n> \tif (exc_std)\n> \t\tsetup_standard_excludes(&dir);\n> \n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index b540944..1e72971 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -905,6 +905,33 @@ test_expect_success 'inside git repository but with --no-index' '\n> \t)\n> '\n> \n> +test_expect_success 'grep --no-index descends into repos, but not .git' '\n> +\trm -fr non &&\n> +\tmkdir -p non/git &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd non/git &&\n> +\n> +\t\techo magic >file &&\n> +\t\tgit init repo &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\techo magic >file &&\n> +\t\t\tgit add file &&\n> +\t\t\tgit commit -m foo &&\n> +\t\t\techo magic >.git/file\n> +\t\t) &&\n> +\n> +\t\tcat >expect <<-\\EOF &&\n> +\t\tfile\n> +\t\trepo/file\n> +\t\tEOF\n> +\t\tgit grep -l --no-index magic >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_expect_success 'setup double-dash tests' '\n> cat >double-dash < --\n"},{"id":"280328","messageId":"20160307155121.GB23010@sigill.intra.peff.net","threadId":"41626","inReplyTo":"xmqqk2lfrs82.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 5/6] grep: turn off gitlink detection for --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-07T15:51:21Z","receivedAt":"2016-03-07T15:51:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 06, 2016 at 05:29:01PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If we are running \"git grep --no-index\" outside of a git\n> > repository, we behave roughly like \"grep -r\", examining all\n> > files in the current directory and its subdirectories.\n> > However, because we use fill_directory() to do the\n> > recursion, it will skip over any directories which look like\n> > sub-repositories.\n> >\n> > For a normal git operation (like \"git grep\" in a repository)\n> > this makes sense; we do not want to cross the boundary out\n> > of our current repository into a submodule. But for\n> > \"--no-index\" without a repository, we should look at all\n> > files, including embedded repositories.\n> >\n> > There is one exception, though: we probably should _not_\n> > descend into \".git\" directories. Doing so is inefficient and\n> > unlikely to turn up useful hits.\n> >\n> > This patch drops our use of dir.c's gitlink-detection, but\n> > we do still avoid \".git\". That makes us more like tools such\n> > as \"ack\" or \"ag\", which also know to avoid cruft in .git.\n> >\n> > As a bonus, this also drops our usage of the ref code\n> > when we are outside of a repository, making the transition\n> > to pluggable ref backends cleaner.\n> >\n> > Based-on-a-patch-by: David Turner \n> > Signed-off-by: Jeff King \n> > ---\n> > I hope the reasoning above makes sense. My ulterior motive is the\n> > \"bonus\", but I really think the new behavior is what people would expect\n> > (i.e., that \"git grep --no-index\" is basically a replacement for \"ack\",\n> > etc).\n> \n> I agree with --no-index part, but the caller of grep_directory() is\n> \"either no-index, or untracked\". I am not sure if the latter wants\n> this new behaviour.\n\nI didn't consider that. I could see arguments either way for\n--untracked, but I think I'd lean towards it doing the \"usual\" thing\nwith embedded untracked repos (i.e., ignoring them).\n\nWe could do that by checking startup_info->have_repository. Which makes\nme wonder if that check should actually go deep into dir.c, and all\ncallers (if there are any) who don't have a repository should not treat\ngitlinks specially.\n\nBut I guess one can actually do \"--no-index\" inside a repository, so we\nreally do need to predicate it on that option, not \"are we in a repo\".\n\nSo here's a replacement patch:\n\n-- >8 --\nSubject: grep: turn off gitlink detection for --no-index\n\nIf we are running \"git grep --no-index\" outside of a git\nrepository, we behave roughly like \"grep -r\", examining all\nfiles in the current directory and its subdirectories.\nHowever, because we use fill_directory() to do the\nrecursion, it will skip over any directories which look like\nsub-repositories.\n\nFor a normal git operation (like \"git grep\" in a repository)\nthis makes sense; we do not want to cross the boundary out\nof our current repository into a submodule. But for\n\"--no-index\" without a repository, we should look at all\nfiles, including embedded repositories.\n\nThere is one exception, though: we probably should _not_\ndescend into \".git\" directories. Doing so is inefficient and\nunlikely to turn up useful hits.\n\nThis patch drops our use of dir.c's gitlink-detection, but\nwe do still avoid \".git\". That makes us more like tools such\nas \"ack\" or \"ag\", which also know to avoid cruft in .git.\n\nAs a bonus, this also drops our usage of the ref code\nwhen we are outside of a repository, making the transition\nto pluggable ref backends cleaner.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 6 ++++--\n t/t7810-grep.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex aa7435f..111b6f6 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -522,12 +522,14 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n }\n \n static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t\t int exc_std)\n+\t\t\t int exc_std, int use_index)\n {\n \tstruct dir_struct dir;\n \tint i, hit = 0;\n \n \tmemset(&dir, 0, sizeof(dir));\n+\tif (!use_index)\n+\t\tdir.flags |= DIR_NO_GITLINKS;\n \tif (exc_std)\n \t\tsetup_standard_excludes(&dir);\n \n@@ -902,7 +904,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n \t\tif (list.nr)\n \t\t\tdie(_(\"--no-index or --untracked cannot be used with revs.\"));\n-\t\thit = grep_directory(&opt, &pathspec, use_exclude);\n+\t\thit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n \t} else if (0 <= opt_exclude) {\n \t\tdie(_(\"--[no-]exclude-standard cannot be used for tracked contents.\"));\n \t} else if (!list.nr) {\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex b540944..1e72971 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -905,6 +905,33 @@ test_expect_success 'inside git repository but with --no-index' '\n \t)\n '\n \n+test_expect_success 'grep --no-index descends into repos, but not .git' '\n+\trm -fr non &&\n+\tmkdir -p non/git &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\n+\t\techo magic >file &&\n+\t\tgit init repo &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\techo magic >file &&\n+\t\t\tgit add file &&\n+\t\t\tgit commit -m foo &&\n+\t\t\techo magic >.git/file\n+\t\t) &&\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfile\n+\t\trepo/file\n+\t\tEOF\n+\t\tgit grep -l --no-index magic >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'setup double-dash tests' '\n cat >double-dash <