{"thread":{"id":"22062","subject":"[PATCH] grep: do not do external grep on skip-worktree entries","startedAt":"2009-12-30T14:11:44Z","lastAt":"2010-01-13T19:48:38Z","messageCount":60,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy","Miles Bader","Linus Torvalds","Jeff King","Mike Hommey","Johannes Sixt","Fredrik Kuivinen","Sverre Rabbelier","Jay Soffian"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"130557","messageId":"1262182304-19911-1-git-send-email-pclouds@gmail.com","threadId":"22062","inReplyTo":null,"subject":"[PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-12-30T14:11:44Z","receivedAt":"2009-12-30T14:11:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Skip-worktree entries are not on disk. There is no reason to call\nexternal grep in such cases.\n\nA trace message is also added to aid debugging external grep cases.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin-grep.c | 19 ++++++++++++++++++-\n 1 files changed, 18 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex d582232..d49c637 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -488,17 +488,34 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,\n \tread_cache();\n \n #if !NO_EXTERNAL_GREP\n+\tif (cached)\n+\t\texternal_grep_allowed = 0;\n+\tif (external_grep_allowed) {\n+\t\tfor (nr = 0; nr < active_nr; nr++) {\n+\t\t\tstruct cache_entry *ce = active_cache[nr];\n+\t\t\tif (!S_ISREG(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\t\t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n+\t\t\t\tcontinue;\n+\t\t\tif (ce_skip_worktree(ce)) {\n+\t\t\t\texternal_grep_allowed = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n \t/*\n \t * Use the external \"grep\" command for the case where\n \t * we grep through the checked-out files. It tends to\n \t * be a lot more optimized\n \t */\n-\tif (!cached && external_grep_allowed) {\n+\tif (external_grep_allowed) {\n \t\thit = external_grep(opt, paths, cached);\n \t\tif (hit >= 0)\n \t\t\treturn hit;\n \t\thit = 0;\n \t}\n+\telse\n+\t\ttrace_printf(\"grep: external grep not used\\n\");\n #endif\n \n \tfor (nr = 0; nr < active_nr; nr++) {\n-- \n1.6.6.315.g1a406\n"},{"id":"130592","messageId":"7v637nzky0.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"1262182304-19911-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-31T07:01:11Z","receivedAt":"2009-12-31T07:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Skip-worktree entries are not on disk. There is no reason to call\n> external grep in such cases.\n>\n> A trace message is also added to aid debugging external grep cases.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin-grep.c | 19 ++++++++++++++++++-\n> 1 files changed, 18 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-grep.c b/builtin-grep.c\n> index d582232..d49c637 100644\n> --- a/builtin-grep.c\n> +++ b/builtin-grep.c\n> @@ -488,17 +488,34 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,\n> \tread_cache();\n> \n> #if !NO_EXTERNAL_GREP\n> +\tif (cached)\n> +\t\texternal_grep_allowed = 0;\n> +\tif (external_grep_allowed) {\n> +\t\tfor (nr = 0; nr < active_nr; nr++) {\n> +\t\t\tstruct cache_entry *ce = active_cache[nr];\n> +\t\t\tif (!S_ISREG(ce->ce_mode))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\t\texternal_grep_allowed = 0;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> \t/*\n> \t * Use the external \"grep\" command for the case where\n> \t * we grep through the checked-out files. It tends to\n> \t * be a lot more optimized\n> \t */\n> -\tif (!cached && external_grep_allowed) {\n> +\tif (external_grep_allowed) {\n> \t\thit = external_grep(opt, paths, cached);\n> \t\tif (hit >= 0)\n> \t\t\treturn hit;\n> \t\thit = 0;\n> \t}\n\nThis looks a bit wrong for a couple of reasons:\n\n - external_grep() is designed to return negative without running external\n grep when it shouldn't be used (see the beginning of the function for\n how it refuses to run when opt->extended is set and other conditions).\n The new logic seems to belong there, i.e. \"in addition to the existing\n case we decline, if ce_skip_worktree() entry exists in the cache, we\n decline\";\n\n - It should be a separate helper function, has_skip_worktree_entry(paths);\n I wouldn't be surprised if there are some other codepaths that want to\n check for the same condition and do things differently, not just grep.\n\nOriginally I thought S_ISREG() check and pathspec_matches() check were\nalso questionable, but they actually are good things to have, as we skip\nsymbolic links (or submodules) and we do want to use external grep if the\nsubtree we are grepping in do not have skip_worktree entries even when\nsome other parts of the index are marked as skip_worktree.\n\n> +\telse\n> +\t\ttrace_printf(\"grep: external grep not used\\n\");\n> #endif\n> \n> \tfor (nr = 0; nr < active_nr; nr++) {\n> -- \n> 1.6.6.315.g1a406\n"},{"id":"130593","messageId":"7vzl4zy5z3.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7v637nzky0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-31T07:09:52Z","receivedAt":"2009-12-31T07:09:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This looks a bit wrong for a couple of reasons:\n>\n> - external_grep() is designed to return negative without running external\n> grep when it shouldn't be used (see the beginning of the function for\n> how it refuses to run when opt->extended is set and other conditions).\n> The new logic seems to belong there, i.e. \"in addition to the existing\n> case we decline, if ce_skip_worktree() entry exists in the cache, we\n> decline\";\n\nIOW, something like this instead of your patch. You would want to tests\nto demonstrate the original breakage, perhaps?\n\n builtin-grep.c | 18 +++++++++++++++++-\n 1 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 813fe97..25ee75d 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -357,6 +357,21 @@ static void grep_add_color(struct strbuf *sb, const char *escape_seq)\n \t\tstrbuf_setlen(sb, sb->len - 1);\n }\n \n+static int has_skip_worktree_entry(struct grep_opt *opt, const char **paths)\n+{\n+\tint nr;\n+\tfor (nr = 0; nr < active_nr; nr++) {\n+\t\tstruct cache_entry *ce = active_cache[nr];\n+\t\tif (!S_ISREG(ce->ce_mode))\n+\t\t\tcontinue;\n+\t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n+\t\t\tcontinue;\n+\t\tif (ce_skip_worktree(ce))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n {\n \tint i, nr, argc, hit, len, status;\n@@ -365,7 +380,8 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \tchar *argptr = randarg;\n \tstruct grep_pat *p;\n \n-\tif (opt->extended || (opt->relative && opt->prefix_length))\n+\tif (opt->extended || (opt->relative && opt->prefix_length)\n+\t || has_skip_worktree_entry(opt, paths))\n \t\treturn -1;\n \tlen = nr = 0;\n \tpush_arg(\"grep\");\n"},{"id":"130684","messageId":"20100102115041.GA32381@do","threadId":"22062","inReplyTo":"7vzl4zy5z3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-02T11:50:42Z","receivedAt":"2010-01-02T11:50:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 30, 2009 at 11:09:52PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > This looks a bit wrong for a couple of reasons:\n> >\n> > - external_grep() is designed to return negative without running external\n> > grep when it shouldn't be used (see the beginning of the function for\n> > how it refuses to run when opt->extended is set and other conditions).\n> > The new logic seems to belong there, i.e. \"in addition to the existing\n> > case we decline, if ce_skip_worktree() entry exists in the cache, we\n> > decline\";\n> \n> IOW, something like this instead of your patch. You would want to tests\n> to demonstrate the original breakage, perhaps?\n\nYour patch works great. By the way I think we should move \"cached\"\ncheck from grep_cache() into external_grep() too, for consistency.\n\nI thought of tests when I wrote the patch, but it was hard to find a\nreliable way to detect if a git build supports external grep. Perhaps\nthis on top of yours? The way to check for external grep support isn't\nnice, but it works.\n\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex f093b60..bc4500a 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -222,6 +222,7 @@ static int exec_grep(int argc, const char **argv)\n \tint status;\n \n \targv[argc] = NULL;\n+\ttrace_argv_printf(argv, \"trace: grep:\");\n \tpid = fork();\n \tif (pid < 0)\n \t\treturn pid;\n@@ -371,7 +372,7 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \tstruct grep_pat *p;\n \n \tif (opt->extended || (opt->relative && opt->prefix_length)\n-\t || has_skip_worktree_entry(opt, paths))\n+\t || cached || has_skip_worktree_entry(opt, paths))\n \t\treturn -1;\n \tlen = nr = 0;\n \tpush_arg(\"grep\");\n@@ -509,7 +510,7 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,\n \t * we grep through the checked-out files. It tends to\n \t * be a lot more optimized\n \t */\n-\tif (!cached && external_grep_allowed) {\n+\tif (external_grep_allowed) {\n \t\thit = external_grep(opt, paths, cached);\n \t\tif (hit >= 0)\n \t\t\treturn hit;\ndiff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\nindex abd14bf..f77970c 100755\n--- a/t/t7002-grep.sh\n+++ b/t/t7002-grep.sh\n@@ -8,6 +8,14 @@ test_description='git grep various.\n \n . ./test-lib.sh\n \n+support_external_grep() {\n+\tcase \"$(git grep -h 2>&1 >/dev/null|grep -e --ext-grep)\" in\n+\t*\"(default)\"*) return 0;;\n+\t*\"(ignored by this build)\"*) return 1;;\n+\t*) test_expect_success 'External grep check is broken' 'false';;\n+\tesac\n+}\n+\n cat >hello.c <\n int main(int argc, const char **argv)\n@@ -426,4 +434,20 @@ test_expect_success 'grep -Fi' '\n \ttest_cmp expected actual\n '\n \n+if support_external_grep; then\n+\n+test_expect_success 'external grep' '\n+\tGIT_TRACE=2 git grep foo >/dev/null 2>actual &&\n+\tgrep \"trace: grep:.*foo\" actual >/dev/null\n+'\n+test_expect_success 'no external grep when skip-worktree entries exist' '\n+\tgit update-index --skip-worktree file &&\n+\tGIT_TRACE=2 git grep foo >/dev/null 2>actual &&\n+\t! grep \"trace: grep:\" actual >/dev/null &&\n+\tgit update-index --no-skip-worktree file\n+'\n+\n+else\n+\tsay \"External grep tests skipped\"\n+fi\n test_done\n\n-- \nDuy\n"},{"id":"130695","messageId":"7vtyv4cpna.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"20100102115041.GA32381@do","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-02T18:44:57Z","receivedAt":"2010-01-02T18:44:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Wed, Dec 30, 2009 at 11:09:52PM -0800, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> \n>> > This looks a bit wrong for a couple of reasons:\n>> >\n>> > - external_grep() is designed to return negative without running external\n>> > grep when it shouldn't be used (see the beginning of the function for\n>> > how it refuses to run when opt->extended is set and other conditions).\n>> > The new logic seems to belong there, i.e. \"in addition to the existing\n>> > case we decline, if ce_skip_worktree() entry exists in the cache, we\n>> > decline\";\n>> \n>> IOW, something like this instead of your patch. You would want to tests\n>> to demonstrate the original breakage, perhaps?\n>\n> Your patch works great. By the way I think we should move \"cached\"\n> check from grep_cache() into external_grep() too, for consistency.\n\nI think what I gave you is more consistent.\n\n\"cached\" is about \"are we searching in the index or in the work tree?\" and\n\"external_grep()\" is about \"it often is faster to run external grep when\nwe are searching in the work tree, so do that if the other constraints\nallow us to\". An example of such a constraint is that we must be able to\nexpress the operation on the command line of a traditional grep, and use\nof git extended grep synatx makes it unfeasible, hence the function says\n\"I can't\" in such a case.\n\nYou can change the definition of \"external_grep()\" to \"try to use external\ngrep somewhere, if the other constraints allow us to\", and have the caller\nand the function do an \"this time, please grep for this pattern in the\nindex---I can't\" exchange, but I don't see much point. We _know_ no\nexternal grep will ever be able to read from the index.\n\nWe should simply drop \"cached\" argument from external_grep().\n\n> I thought of tests when I wrote the patch, but it was hard to find a\n> reliable way to detect if a git build supports external grep.\n\nAh, you are right. I forgot about that issue.\n\n> diff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\n> index abd14bf..f77970c 100755\n> --- a/t/t7002-grep.sh\n> +++ b/t/t7002-grep.sh\n> @@ -8,6 +8,14 @@ test_description='git grep various.\n> \n> . ./test-lib.sh\n> \n> +support_external_grep() {\n> +\tcase \"$(git grep -h 2>&1 >/dev/null|grep -e --ext-grep)\" in\n> +\t*\"(default)\"*) return 0;;\n> +\t*\"(ignored by this build)\"*) return 1;;\n> +\t*) test_expect_success 'External grep check is broken' 'false';;\n> +\tesac\n> +}\n\nHeh, clever.\n\n\tgit grep -h 2>&1 | grep 'allow calling of grep.*default' >/dev/null\n\nmay be sufficient, though.\n"},{"id":"130696","messageId":"fcaeb9bf1001021115j7b23264n42cfba7855c2253e@mail.gmail.com","threadId":"22062","inReplyTo":"7vtyv4cpna.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-02T19:15:22Z","receivedAt":"2010-01-02T19:15:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 1/3/10, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n> > On Wed, Dec 30, 2009 at 11:09:52PM -0800, Junio C Hamano wrote:\n> >> Junio C Hamano writes:\n> >>\n> >> > This looks a bit wrong for a couple of reasons:\n> >> >\n> >> > - external_grep() is designed to return negative without running external\n> >> > grep when it shouldn't be used (see the beginning of the function for\n> >> > how it refuses to run when opt->extended is set and other conditions).\n> >> > The new logic seems to belong there, i.e. \"in addition to the existing\n> >> > case we decline, if ce_skip_worktree() entry exists in the cache, we\n> >> > decline\";\n> >>\n> >> IOW, something like this instead of your patch. You would want to tests\n> >> to demonstrate the original breakage, perhaps?\n> >\n> > Your patch works great. By the way I think we should move \"cached\"\n> > check from grep_cache() into external_grep() too, for consistency.\n>\n>\n> I think what I gave you is more consistent.\n>\n> \"cached\" is about \"are we searching in the index or in the work tree?\" and\n> \"external_grep()\" is about \"it often is faster to run external grep when\n> we are searching in the work tree, so do that if the other constraints\n> allow us to\". An example of such a constraint is that we must be able to\n> express the operation on the command line of a traditional grep, and use\n> of git extended grep synatx makes it unfeasible, hence the function says\n> \"I can't\" in such a case.\n>\n> You can change the definition of \"external_grep()\" to \"try to use external\n> grep somewhere, if the other constraints allow us to\", and have the caller\n> and the function do an \"this time, please grep for this pattern in the\n> index---I can't\" exchange, but I don't see much point. We _know_ no\n> external grep will ever be able to read from the index.\n>\n> We should simply drop \"cached\" argument from external_grep().\n\nOK.\n\n> > diff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\n> > index abd14bf..f77970c 100755\n> > --- a/t/t7002-grep.sh\n> > +++ b/t/t7002-grep.sh\n> > @@ -8,6 +8,14 @@ test_description='git grep various.\n> >\n> > . ./test-lib.sh\n> >\n> > +support_external_grep() {\n> > + case \"$(git grep -h 2>&1 >/dev/null|grep -e --ext-grep)\" in\n> > + *\"(default)\"*) return 0;;\n> > + *\"(ignored by this build)\"*) return 1;;\n> > + *) test_expect_success 'External grep check is broken' 'false';;\n> > + esac\n> > +}\n>\n>\n> Heh, clever.\n>\n> git grep -h 2>&1 | grep 'allow calling of grep.*default' >/dev/null\n>\n> may be sufficient, though.\n>\n\nYes, until somebody changes help text in builtin-grep.c and all\nexternal grep tests become disable. I wanted to catch that case too.\n\nOr maybe just add another option --show-features to \"git rev-parse\"\n(or extend git version string, is the version format fixed?) to list\nall built-time features that a git build supports. Some comes to mind:\nexternal-grep, iconv, ipv6, http (unlikely), threading... Much less\ntricky to test.\n-- \nDuy\n"},{"id":"130697","messageId":"7v7hs09tpi.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"fcaeb9bf1001021115j7b23264n42cfba7855c2253e@mail.gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-02T19:45:29Z","receivedAt":"2010-01-02T19:45:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n>> > +support_external_grep() {\n>> > + case \"$(git grep -h 2>&1 >/dev/null|grep -e --ext-grep)\" in\n>> > + *\"(default)\"*) return 0;;\n>> > + *\"(ignored by this build)\"*) return 1;;\n>> > + *) test_expect_success 'External grep check is broken' 'false';;\n>> > + esac\n>> > +}\n>>\n>>\n>> Heh, clever.\n>>\n>> git grep -h 2>&1 | grep 'allow calling of grep.*default' >/dev/null\n>>\n>> may be sufficient, though.\n>\n> Yes, until somebody changes help text in builtin-grep.c and all\n> external grep tests become disable. I wanted to catch that case too.\n\nOk, that is a worthwhile thing to do.\n\nThen please at least make the second grep \"grep ext-grep\", droping \"-e --\"\nfrom it. We assume some implementation of external grep to lack \"-e\"\n(e.g. Solaris).\n"},{"id":"130718","messageId":"87ljgfgbl0.fsf@catnip.gol.com","threadId":"22062","inReplyTo":"7v7hs09tpi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2010-01-03T02:35:07Z","receivedAt":"2010-01-03T02:35:07Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Junio C Hamano writes:\n> Then please at least make the second grep \"grep ext-grep\", droping \"-e --\"\n> from it. We assume some implementation of external grep to lack \"-e\"\n> (e.g. Solaris).\n\nIsn't \"-e\" a \"classic\" grep option tho?\n\n-Miles\n\n-- \nConsult, v.i. To seek another's disapproval of a course already decided on.\n"},{"id":"130719","messageId":"fc339e4a1001021847hf1e1a7fq894de7908839ff77@mail.gmail.com","threadId":"22062","inReplyTo":"87ljgfgbl0.fsf@catnip.gol.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2010-01-03T02:47:19Z","receivedAt":"2010-01-03T02:47:19Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"On Sun, Jan 3, 2010 at 11:35 AM, Miles Bader wrote:\n>> Then please at least make the second grep \"grep ext-grep\", droping \"-e --\"\n>> from it.  We assume some implementation of external grep to lack \"-e\"\n>> (e.g. Solaris).\n>\n> Isn't \"-e\" a \"classic\" grep option tho?\n\nHmm, a bit of googling, and it seems that while 7th Edition unix (as\nclassic as I get) had -e, solaris indeed doesn't....\n\n-miles\n\n-- \nDo not taunt Happy Fun Ball.\n"},{"id":"130720","messageId":"877hrzga16.fsf@catnip.gol.com","threadId":"22062","inReplyTo":"fc339e4a1001021847hf1e1a7fq894de7908839ff77@mail.gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2010-01-03T03:08:37Z","receivedAt":"2010-01-03T03:08:37Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Miles Bader writes:\n>> Isn't \"-e\" a \"classic\" grep option tho?\n>\n> Hmm, a bit of googling, and it seems that while 7th Edition unix (as\n> classic as I get) had -e, solaris indeed doesn't....\n\nTho solaris does have it in /usr/xpg4/bin/grep...\n\nSince it's a general attribute of solaris that the default (/usr/bin)\ntools are horrible sysv things and the actual useful tools are in\ne.g. /usr/xpg4/bin, maybe it would be better to just try and add that\ndirectory to the path...?\n\n-miles\n\n-- \nAmericans are broad-minded people. They'll accept the fact that a person can\nbe an alcoholic, a dope fiend, a wife beater, and even a newspaperman, but if\na man doesn't drive, there is something wrong with him. -- Art Buchwald\n"},{"id":"130735","messageId":"alpine.LFD.2.00.1001031124420.3630@localhost.localdomain","threadId":"22062","inReplyTo":"877hrzga16.fsf@catnip.gol.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-03T19:32:24Z","receivedAt":"2010-01-03T19:32:24Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 3 Jan 2010, Miles Bader wrote:\n> \n> Since it's a general attribute of solaris that the default (/usr/bin)\n> tools are horrible sysv things and the actual useful tools are in\n> e.g. /usr/xpg4/bin, maybe it would be better to just try and add that\n> directory to the path...?\n\nThere is no generic way to make solaris sane, I'm afraid.\n\nEverybody agrees that the \"normal\" Solaris tools are so horrible that they \nall set up some alternative. However, qutie often the preferred \nalternative is the GNU versions, mostly installed in /usr/local, and then \nthey put that first in the path.\n\nWhich means that if you put /usr/xpg4/bin before other paths in your PATH, \nyou'll totally break such systems, because now you get the (inferior) \ntools in xpg4 before the preferred tools in /usr/local. Or - this also \nhappens - people end up installing their own versions in $HOME/bin, \nbecause the system admin is uncaring or incompetent.\n\nIn other words, there is no single normal or default Solaris installation \nthat we can work around. The normal/default installation is so horrible \nthat it's virtually never used in any environment where people actually \nhave shell access and do development.\n\nDon't ask me why. EVERYBODY knows that the default /usr/bin is total crap. \nEven Sun people know. But there's apparently some very deep-seated reason \n(probably not technical, but mental/historical) why they can't be fixed or \nreplaced, probably relating to \"make world\" and the whole build system.\n\n\t\t\tLinus\n"},{"id":"130736","messageId":"7v3a2mzzg4.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001031124420.3630@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-03T20:49:15Z","receivedAt":"2010-01-03T20:49:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Which means that if you put /usr/xpg4/bin before other paths in your PATH, \n> you'll totally break such systems, because now you get the (inferior) \n> tools in xpg4 before the preferred tools in /usr/local. Or - this also \n> happens - people end up installing their own versions in $HOME/bin, \n> because the system admin is uncaring or incompetent.\n\nThe build allows you to define SANE_TOOL_PATH (\"the tools found in here\nare saner than the ones in /usr/bin or /bin\" is what it means) and we\ninsert it just in front of /usr/bin or /bin in the original PATH (see\ngit_brokne_path_fix in git-sh-setup.sh).\n\nI would call this the right thing (TM) or the best workaround we could do\nunder the constraints, depending on the mood.\n"},{"id":"130745","messageId":"20100104053125.GA5083@coredump.intra.peff.net","threadId":"22062","inReplyTo":"7v3a2mzzg4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T05:31:25Z","receivedAt":"2010-01-04T05:31:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 03, 2010 at 12:49:15PM -0800, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> \n> > Which means that if you put /usr/xpg4/bin before other paths in your PATH, \n> > you'll totally break such systems, because now you get the (inferior) \n> > tools in xpg4 before the preferred tools in /usr/local. Or - this also \n> > happens - people end up installing their own versions in $HOME/bin, \n> > because the system admin is uncaring or incompetent.\n> \n> The build allows you to define SANE_TOOL_PATH (\"the tools found in here\n> are saner than the ones in /usr/bin or /bin\" is what it means) and we\n> insert it just in front of /usr/bin or /bin in the original PATH (see\n> git_brokne_path_fix in git-sh-setup.sh).\n> \n> I would call this the right thing (TM) or the best workaround we could do\n> under the constraints, depending on the mood.\n\nI agree that Solaris default tools are insane, but is there any reason\nto munge the PATH for a single feature like external grep? Why not\nEXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the\nMakefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?\n\nObviously we still need SANE_TOOL_PATH for systems where the /usr/bin is\nso crappy as to be unusable for our scripts. But surely we can do better\nfor individual tools where the user might have some more specific\npreference about which tool he uses.\n\n-Peff\n"},{"id":"130746","messageId":"7vbphaquwl.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"20100104053125.GA5083@coredump.intra.peff.net","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-04T05:52:10Z","receivedAt":"2010-01-04T05:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jan 03, 2010 at 12:49:15PM -0800, Junio C Hamano wrote:\n>\n>> Linus Torvalds writes:\n>> \n>> > Which means that if you put /usr/xpg4/bin before other paths in your PATH, \n>> > you'll totally break such systems, because now you get the (inferior) \n>> > tools in xpg4 before the preferred tools in /usr/local. Or - this also \n>> > happens - people end up installing their own versions in $HOME/bin, \n>> > because the system admin is uncaring or incompetent.\n>> \n>> The build allows you to define SANE_TOOL_PATH (\"the tools found in here\n>> are saner than the ones in /usr/bin or /bin\" is what it means) and we\n>> insert it just in front of /usr/bin or /bin in the original PATH (see\n>> git_brokne_path_fix in git-sh-setup.sh).\n>> \n>> I would call this the right thing (TM) or the best workaround we could do\n>> under the constraints, depending on the mood.\n>\n> I agree that Solaris default tools are insane, but is there any reason\n> to munge the PATH for a single feature like external grep? Why not\n> EXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the\n> Makefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?\n\nThat git-sh-setup \"fix\" is not for running external grep. It is for our\nscripted Porcelains that rely on working basic tools (sed, tr, who knows\nwhat else is broken).\n\nIn fact, our Makefile by default punts on external grep on Sun's. Run\n\"git grep NO_EXTERNAL_GREP -- Makefile\" to see for yourself --- it would\nwork even on Solaris ;-)\n\n> Obviously we still need SANE_TOOL_PATH for systems where the /usr/bin is\n> so crappy as to be unusable for our scripts. But surely we can do better\n> for individual tools where the user might have some more specific\n> preference about which tool he uses.\n>\n> -Peff\n"},{"id":"130747","messageId":"20100104060646.GA14520@glandium.org","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001031124420.3630@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2010-01-04T06:06:46Z","receivedAt":"2010-01-04T06:06:46Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, Jan 03, 2010 at 11:32:24AM -0800, Linus Torvalds wrote:\n> \n> \n> On Sun, 3 Jan 2010, Miles Bader wrote:\n> > \n> > Since it's a general attribute of solaris that the default (/usr/bin)\n> > tools are horrible sysv things and the actual useful tools are in\n> > e.g. /usr/xpg4/bin, maybe it would be better to just try and add that\n> > directory to the path...?\n> \n> There is no generic way to make solaris sane, I'm afraid.\n> \n> Everybody agrees that the \"normal\" Solaris tools are so horrible that they \n> all set up some alternative. However, qutie often the preferred \n> alternative is the GNU versions, mostly installed in /usr/local, and then \n> they put that first in the path.\n\nOr /usr/sfw, where Sun themselves ship some GNU tools\n\n> Which means that if you put /usr/xpg4/bin before other paths in your PATH, \n> you'll totally break such systems, because now you get the (inferior) \n> tools in xpg4 before the preferred tools in /usr/local. Or - this also \n> happens - people end up installing their own versions in $HOME/bin, \n> because the system admin is uncaring or incompetent.\n> \n> In other words, there is no single normal or default Solaris installation \n> that we can work around. The normal/default installation is so horrible \n> that it's virtually never used in any environment where people actually \n> have shell access and do development.\n> \n> Don't ask me why. EVERYBODY knows that the default /usr/bin is total crap. \n> Even Sun people know. But there's apparently some very deep-seated reason \n> (probably not technical, but mental/historical) why they can't be fixed or \n> replaced, probably relating to \"make world\" and the whole build system.\n\nThere is simply no explanation for these tools to be that crappy at all.\nThe most common reason that is given for these tools to stay as crappy as\nthey always have been is for backwards compatibility. But how does that\nprevent from adding features is beyond me. And on the other hand, they\n*do* add features to their /usr/bin tools, such as -h in (at least) df\nand ls.\n\nAnyways, why not generate the hash-bang lines of the shell scripts from a\nMakefile variable that would be set to /usr/xpg4/bin/sh on Solaris and\n/bin/sh on others ?\n\nMike\n"},{"id":"130748","messageId":"20100104064408.GA7785@coredump.intra.peff.net","threadId":"22062","inReplyTo":"7vbphaquwl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T06:44:08Z","receivedAt":"2010-01-04T06:44:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 03, 2010 at 09:52:10PM -0800, Junio C Hamano wrote:\n\n> > I agree that Solaris default tools are insane, but is there any reason\n> > to munge the PATH for a single feature like external grep? Why not\n> > EXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the\n> > Makefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?\n> \n> That git-sh-setup \"fix\" is not for running external grep. It is for our\n> scripted Porcelains that rely on working basic tools (sed, tr, who knows\n> what else is broken).\n\nRight, but I thought this thread was about external grep, and I thought\nyou were saying \"if you want decent tools, you can use SANE_TOOL_PATH\".\nAnd I think we can do much better for that particular case than\nrecommending SANE_TOOL_PATH (but it seems that is not what you were\nactually recommending).\n\nBut I admit, I have never really wanted to specify my own external grep.\nWanting your own grep for _features_ is probably insane, as some of your\ngreps (on worktree files) will use the external grep, and some (on\ncached files) will not. So it is really just an optimization, and I\nhave never felt it so slow that I cared about messing with an\nalternative grep on Solaris.\n\nI have to wonder, though...did anybody ever actually profile our\ninternal grep to find out _why_ it was so much slower than GNU grep?\nCould we simply ship a better grep engine and obsolete external grep?\n\n> In fact, our Makefile by default punts on external grep on Sun's. Run\n> \"git grep NO_EXTERNAL_GREP -- Makefile\" to see for yourself --- it would\n> work even on Solaris ;-)\n\nYes, I am even mentioned in the commit log of 01ae841c. :)\n\n-Peff\n"},{"id":"130750","messageId":"20100104070405.GE7785@coredump.intra.peff.net","threadId":"22062","inReplyTo":"20100104060646.GA14520@glandium.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T07:04:05Z","receivedAt":"2010-01-04T07:04:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 04, 2010 at 07:06:46AM +0100, Mike Hommey wrote:\n\n> Anyways, why not generate the hash-bang lines of the shell scripts from a\n> Makefile variable that would be set to /usr/xpg4/bin/sh on Solaris and\n> /bin/sh on others ?\n\nWe do that already (though we default it to /bin/bash). SANE_TOOL_PATH\nis about picking up all the _other_ tools for use inside our shell\nscripts, like non-crappy sed, grep, etc.\n\nBrandon did some testing with ksh and sent a patch to default to the\nstock ksh on Solaris and IRIX, which is probably saner than bash for a\ndefault, but it seems to have gotten dropped:\n\n http://article.gmane.org/gmane.comp.version-control.git/129830\n\n-Peff\n"},{"id":"130751","messageId":"7vhbr2pcsh.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"20100104064408.GA7785@coredump.intra.peff.net","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-04T07:08:46Z","receivedAt":"2010-01-04T07:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I have to wonder, though...did anybody ever actually profile our\n> internal grep to find out _why_ it was so much slower than GNU grep?\n\nI vaguely recall that somebody fairly competent mentioned that modern grep\nimplementations are based on DFA engines, but I offhand don't remember if\nthe discussion had concrete numbers.\n\n> Could we simply ship a better grep engine and obsolete external grep?\n\nYes, that is a very constructive longer term solution.\n"},{"id":"130752","messageId":"7v4on2pciw.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7vhbr2pcsh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-04T07:14:31Z","receivedAt":"2010-01-04T07:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I have to wonder, though...did anybody ever actually profile our\n>> internal grep to find out _why_ it was so much slower than GNU grep?\n>\n> I vaguely recall that somebody fairly competent mentioned that modern grep\n> implementations are based on DFA engines, but I offhand don't remember if\n> the discussion had concrete numbers.\n\nI found only this one. The author was indeed somebody fairly competent.\n\n http://thread.gmane.org/gmane.comp.version-control.git/23798\n"},{"id":"130753","messageId":"20100104072659.GA10611@coredump.intra.peff.net","threadId":"22062","inReplyTo":"7vhbr2pcsh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T07:26:59Z","receivedAt":"2010-01-04T07:26:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 03, 2010 at 11:08:46PM -0800, Junio C Hamano wrote:\n\n> > I have to wonder, though...did anybody ever actually profile our\n> > internal grep to find out _why_ it was so much slower than GNU grep?\n> \n> I vaguely recall that somebody fairly competent mentioned that modern grep\n> implementations are based on DFA engines, but I offhand don't remember if\n> the discussion had concrete numbers.\n\nProbably this:\n\n http://article.gmane.org/gmane.comp.version-control.git/41685\n\nAlso of interest is:\n\n http://article.gmane.org/gmane.comp.version-control.git/50174\n\nThe pcre analysis there came from just using the \"pcreposix\" header, I\nthink. From my limited research, modern pcre may have some tuning\noptions (including a DFA engine!) that could do a lot better.\n\n-Peff\n"},{"id":"130754","messageId":"20100104072926.GB10611@coredump.intra.peff.net","threadId":"22062","inReplyTo":"7v4on2pciw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T07:29:26Z","receivedAt":"2010-01-04T07:29:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 03, 2010 at 11:14:31PM -0800, Junio C Hamano wrote:\n\n> > I vaguely recall that somebody fairly competent mentioned that modern grep\n> > implementations are based on DFA engines, but I offhand don't remember if\n> > the discussion had concrete numbers.\n> \n> I found only this one. The author was indeed somebody fairly competent.\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/23798\n\nEr, that link seems to be about internal grep beating external on a cold\ncache because it does less I/O. Did you mean something else (maybe the\nthread I pointed to in another message)?\n\n-Peff\n"},{"id":"130755","messageId":"20100104080940.GA4815@coredump.intra.peff.net","threadId":"22062","inReplyTo":"20100104072659.GA10611@coredump.intra.peff.net","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-04T08:09:40Z","receivedAt":"2010-01-04T08:09:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 04, 2010 at 02:26:59AM -0500, Jeff King wrote:\n\n> The pcre analysis there came from just using the \"pcreposix\" header, I\n> think. From my limited research, modern pcre may have some tuning\n> options (including a DFA engine!) that could do a lot better.\n\nHmm. I was able to get some improvements by using pcre_dfa_exec, but\nstill not as good as external grep. For \"git grep 'foo.*bar'\" in the\nlinux-2.6 repo, I got:\n\n external grep: 0.76s\n pcre_dfa_exec: 1.85s\n pcre_exec: 3.21s\n glibc: 4.00s\n\nHowever, gprof reports that for the pcre dfa case, we spend more time in\ngrep.c:end_of_line than we do actually running the regex. So clearly\nthere are some other micro-optimizations in GNU grep that are making a\ndifference, too.\n\nBy the way, you can see the abysmal performance of our internal code by\ndoing a \"git grep foo\". It uses the \"fixed\" internal engine and weighs\nin at 3.24s on the same machine, _slower_ than pcre doing an actual\nregex.\n\n-Peff\n"},{"id":"130758","messageId":"fcaeb9bf1001040214q678bf7d6ya922923136da0598@mail.gmail.com","threadId":"22062","inReplyTo":"7vbphaquwl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-04T10:14:00Z","receivedAt":"2010-01-04T10:14:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 4, 2010 at 12:52 PM, Junio C Hamano wrote:\n>> I agree that Solaris default tools are insane, but is there any reason\n>> to munge the PATH for a single feature like external grep? Why not\n>> EXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the\n>> Makefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?\n>\n> That git-sh-setup \"fix\" is not for running external grep.  It is for our\n> scripted Porcelains that rely on working basic tools (sed, tr, who knows\n> what else is broken).\n>\n> In fact, our Makefile by default punts on external grep on Sun's.  Run\n> \"git grep NO_EXTERNAL_GREP -- Makefile\" to see for yourself --- it would\n> work even on Solaris ;-)\n\nA bit off-topic. But it seems to me on linux (main development\nplatform?) GNU grep may be tested more than the builtin grep because\nNO_EXTERNAL_GREP would be undefined by default. Should we test both\ngreps in that case?\n-- \nDuy\n"},{"id":"130764","messageId":"1262608455-4045-1-git-send-email-pclouds@gmail.com","threadId":"22062","inReplyTo":"7v7hs09tpi.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] t7002: set test prerequisite \"external-grep\" if supported","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-04T12:34:14Z","receivedAt":"2010-01-04T12:34:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Add another test to set prerequisite \"external-grep\" if the current\nbuild supports external grep. This can be used to skip external grep\nonly tests on builds that do not support this optimization.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t7002-grep.sh | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\nindex abd14bf..ffda0df 100755\n--- a/t/t7002-grep.sh\n+++ b/t/t7002-grep.sh\n@@ -8,6 +8,18 @@ test_description='git grep various.\n \n . ./test-lib.sh\n \n+test_expect_success 'Check for external grep support' '\n+\tcase \"$(git grep -h 2>&1|grep ext-grep)\" in\n+\t*\"(default)\"*)\n+\t\ttest_set_prereq external-grep\n+\t\ttrue;;\n+\t*\"(ignored by this build)\"*)\n+\t\ttrue;;\n+\t*)\n+\t\tfalse;;\n+\tesac\n+'\n+\n cat >hello.c <\n int main(int argc, const char **argv)\n-- \n1.6.6.315.g1a406\n"},{"id":"130763","messageId":"1262608455-4045-2-git-send-email-pclouds@gmail.com","threadId":"22062","inReplyTo":"7v7hs09tpi.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] t7002: add tests for skip-worktree fixes in commit a67e281","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-04T12:34:15Z","receivedAt":"2010-01-04T12:34:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin-grep.c | 1 +\n t/t7002-grep.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex f093b60..59c4b12 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -222,6 +222,7 @@ static int exec_grep(int argc, const char **argv)\n \tint status;\n \n \targv[argc] = NULL;\n+\ttrace_argv_printf(argv, \"trace: grep:\");\n \tpid = fork();\n \tif (pid < 0)\n \t\treturn pid;\ndiff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\nindex ffda0df..ac0a658 100755\n--- a/t/t7002-grep.sh\n+++ b/t/t7002-grep.sh\n@@ -438,4 +438,16 @@ test_expect_success 'grep -Fi' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success external-grep 'external grep is called' '\n+\tGIT_TRACE=2 git grep foo >/dev/null 2>actual &&\n+\tgrep \"trace: grep:.*foo\" actual >/dev/null\n+'\n+\n+test_expect_success external-grep 'no external grep when skip-worktree entries exist' '\n+\tgit update-index --skip-worktree file &&\n+\tGIT_TRACE=2 git grep foo >/dev/null 2>actual &&\n+\t! grep \"trace: grep:\" actual >/dev/null &&\n+\tgit update-index --no-skip-worktree file\n+'\n+\n test_done\n-- \n1.6.6.315.g1a406\n"},{"id":"130772","messageId":"alpine.LFD.2.00.1001040659150.3630@localhost.localdomain","threadId":"22062","inReplyTo":"20100104064408.GA7785@coredump.intra.peff.net","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-04T15:54:24Z","receivedAt":"2010-01-04T15:54:24Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 4 Jan 2010, Jeff King wrote:\n> \n> I have to wonder, though...did anybody ever actually profile our\n> internal grep to find out _why_ it was so much slower than GNU grep?\n> Could we simply ship a better grep engine and obsolete external grep?\n\nThe internal grep is about 2.5 times slower than the external one for me. \nThat's a big deal:\n\n - external grep:\n\n\t[torvalds@nehalem linux]$ time git grep qwerty\n\t...\n\treal\t0m0.412s\n\tuser\t0m0.196s\n\tsys\t0m0.132s\n\n - NO_EXTERNAL_GREP:\n\n\t[torvalds@nehalem linux]$ time ~/git/git grep qwerty\n\t...\n\treal\t0m1.006s\n\tuser\t0m0.900s\n\tsys\t0m0.096s\n\nso that's not even close.\n\nAnd \"perf record\" followed by \"perf report\" on the internal one shows \nthat it's not even regexec() - we use strstr() for the trivial case:\n\n 43.63% git /home/torvalds/git/git [.] grep_buffer_1\n 25.19% git /lib64/libc-2.11.so [.] __strstr_sse42\n 9.16% git /home/torvalds/git/git [.] match_one_pattern\n 4.79% git /lib64/libc-2.11.so [.] __m128i_strloadu\n\nbit it seems to be all that line-per-line crud. If we got rid of that one, \nand could do the match as a _single_ regexec() instead (at least for the \ntrivial cases of just one grep expression), perhaps we'd be better off.\n\n\t\t\tLinus\n"},{"id":"130777","messageId":"fc339e4a1001040757n31298f3h724eacfafb68c63e@mail.gmail.com","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001040659150.3630@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2010-01-04T15:57:49Z","receivedAt":"2010-01-04T15:57:49Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"On Tue, Jan 5, 2010 at 12:54 AM, Linus Torvalds\n wrote:\n> And \"perf record\" followed by \"perf report\" on the internal one shows\n> that it's not even regexec() - we use strstr() for the trivial case:\n\nDoes strstr use e.g. boyer-moore? I imagine grep does...\n\n-miles\n\n-- \nDo not taunt Happy Fun Ball.\n"},{"id":"130773","messageId":"alpine.LFD.2.00.1001040755130.3630@localhost.localdomain","threadId":"22062","inReplyTo":"20100104080940.GA4815@coredump.intra.peff.net","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-04T16:01:15Z","receivedAt":"2010-01-04T16:01:15Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 4 Jan 2010, Jeff King wrote:\n> \n> However, gprof reports that for the pcre dfa case, we spend more time in\n> grep.c:end_of_line than we do actually running the regex. So clearly\n> there are some other micro-optimizations in GNU grep that are making a\n> difference, too.\n\nDon't use gprof. You're _much_ better off using the newish Linux 'perf' \ntool. It's quite competent, and doesn't need the code to be compiled with \n-pg (which totally changes all performance characteristics).\n\nDo something like this:\n\n\tperf record git grep qwerty\n\nfollowed by\n\n\tperf report\n\tperf annotate grep_buffer_1\n\n(that \"perf report\" gives a per-symbol overview, the \"perf annotate\" gives \na disassembly with source annotations and per-instruction costs). It works \nwith inlining too, so you get things like this:\n\n\t...\n : static char *end_of_line(char *cp, unsigned long *left)\n : {\n : unsigned long l = *left;\n : while (l && *cp != '\\n') {\n 24.76 : 476a50: 80 3b 0a cmpb $0xa,(%rbx)\n 10.46 : 476a53: 0f 84 e7 00 00 00 je 476b40 \n : l--;\n : cp++;\n 21.19 : 476a59: 48 83 c3 01 add $0x1,%rbx\n : }\n :\n : static char *end_of_line(char *cp, unsigned long *left)\n : {\n : unsigned long l = *left;\n : while (l && *cp != '\\n') {\n 0.94 : 476a5d: 49 83 ed 01 sub $0x1,%r13\n 4.85 : 476a61: 75 ed jne 476a50 \n :\n\t...\n\nand yes, it's all the per-line crap.\n\nThe perf tools are included with modern kernels in tools/perf (which also \nhas a Documentation subdirectory). I can pretty much guarantee that once \nyou start using it, you'll never use gprof or oprofile again.\n\n\t\tLinus\n"},{"id":"130774","messageId":"alpine.LFD.2.00.1001040801290.3630@localhost.localdomain","threadId":"22062","inReplyTo":"fc339e4a1001040757n31298f3h724eacfafb68c63e@mail.gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-04T16:03:04Z","receivedAt":"2010-01-04T16:03:04Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 5 Jan 2010, Miles Bader wrote:\n\n> On Tue, Jan 5, 2010 at 12:54 AM, Linus Torvalds\n> wrote:\n> > And \"perf record\" followed by \"perf report\" on the internal one shows\n> > that it's not even regexec() - we use strstr() for the trivial case:\n> \n> Does strstr use e.g. boyer-moore? I imagine grep does...\n\nIt doesn't matter. Since we do the line-by-line thing, the input is always \nso short that DFA vs NFA vs BM vs other-clever-search doesn't matter. \nThere is no scaling - the grep buffer tends to be too small for the \nalgorithm to matter.\n\nAnd the reason we do things line-by-line is that we need to then output \nthings line-per-line.\n\n\t\t\tLinus\n"},{"id":"130779","messageId":"alpine.LFD.2.00.1001040817420.3630@localhost.localdomain","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001040659150.3630@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-04T16:24:43Z","receivedAt":"2010-01-04T16:24:43Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 4 Jan 2010, Linus Torvalds wrote:\n> \n> - external grep:\n> \n> \t[torvalds@nehalem linux]$ time git grep qwerty\n> \t...\n> \treal\t0m0.412s\n> \tuser\t0m0.196s\n> \tsys\t0m0.132s\n> \n> - NO_EXTERNAL_GREP:\n> \n> \t[torvalds@nehalem linux]$ time ~/git/git grep qwerty\n> \t...\n> \treal\t0m1.006s\n> \tuser\t0m0.900s\n> \tsys\t0m0.096s\n> \n> so that's not even close.\n\nSide note: at least for me, if we did some auto-parallelization, the \ninternal grep would make up for all its other suckiness. Do four or eight \ngreps in parallel, and buffer the results (you still need to show them in \nthe right order).\n\nThat might be an acceptable way to \"fix\" it. Developers pretty much all \nhave at least two cores these days, some of us have four+HT. We use \nthreads in other places, maybe this could be one more of them.\n\n(Start 'n' threads, do an initial per-thread regex and 'regcomp()' to make \nit thread-safer, and the only interesting issue would be serializing the \noutput. Whenever you get a result, you'd need to make sure that all files \nbefore have been completed, but you could do that all under a specific \nlock that protects completion information).\n\n\t\tLinus\n"},{"id":"130970","messageId":"7v4omyhc7h.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"1262608455-4045-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] t7002: set test prerequisite \"external-grep\" if supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-07T02:37:38Z","receivedAt":"2010-01-07T02:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Add another test to set prerequisite \"external-grep\" if the current\n> build supports external grep. This can be used to skip external grep\n> only tests on builds that do not support this optimization.\n\nThanks. We seem to spell our prerequistes in a single-word, all-caps, so\nI'll change this new one to EXTGREP in both [1/2] and [2/2].\n"},{"id":"130976","messageId":"7vy6ka8rmr.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7v4omyhc7h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t7002: set test prerequisite \"external-grep\" if supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-07T04:29:16Z","receivedAt":"2010-01-07T04:29:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Add another test to set prerequisite \"external-grep\" if the current\n>> build supports external grep. This can be used to skip external grep\n>> only tests on builds that do not support this optimization.\n>\n> Thanks. We seem to spell our prerequistes in a single-word, all-caps, so\n> I'll change this new one to EXTGREP in both [1/2] and [2/2].\n\nSorry, but I had this \"Sheesh, why didn't I think of that earlier before\nwasting Nguyễn's time\" moment.\n\nWhy don't we just test what we _want to_ test? After all what a67e281\n(grep: do not do external grep on skip-worktree entries, 2009-12-30)\nwanted to make sure was this:\n\n \"git grep\" (without --cached) should grep from the index for paths\n that are marked as skip-worktree.\n\nSo how about writing some string that does not appear in the version in\nthe index in the work tree file, and run \"git grep\" to make sure it\ndoesn't find it?\n\nYes, some implementations/builds of \"git grep\" may not even try to cheat\nand run external grep and for them the test _should_ succeed (but your\nlogic to check with ce_skip_worktree() in grep_cache() may be broken by\nlater patch while you are looking the other way), and some will try to\ncheat and the fix was about not letting them.\n\nSo by writing the test to check the desired outcome, instead of writing it\nfor the particular implementation of using external grep optimization, you\nwill catch both kinds of breakages.\n\nPerhaps something like this (untested, of course)?\n\ntest_expect_success 'strings in work tree files are not found for skip-wt paths' '\n\tno=\"no such string in the index\" &&\n\ttest_must_fail git grep -e \"$no\" --cached file &&\n\tgit update-index --skip-worktree file &&\n\techo \"$no\" >file &&\n\ttest_must_fail git grep -e \"$no\" file &&\n\tgit update-index --no-skip-worktree file &&\n\tgit grep -e \"$no\" file\n'\n"},{"id":"130995","messageId":"fcaeb9bf1001070527ra1570a0m12182db9bb029055@mail.gmail.com","threadId":"22062","inReplyTo":"7vy6ka8rmr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t7002: set test prerequisite \"external-grep\" if supported","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-07T13:27:39Z","receivedAt":"2010-01-07T13:27:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 1/7/10, Junio C Hamano wrote:\n> So by writing the test to check the desired outcome, instead of writing it\n> for the particular implementation of using external grep optimization, you\n> will catch both kinds of breakages.\n>\n> Perhaps something like this (untested, of course)?\n>\n> test_expect_success 'strings in work tree files are not found for skip-wt paths' '\n> no=\"no such string in the index\" &&\n> test_must_fail git grep -e \"$no\" --cached file &&\n> git update-index --skip-worktree file &&\n> echo \"$no\" >file &&\n> test_must_fail git grep -e \"$no\" file &&\n> git update-index --no-skip-worktree file &&\n> git grep -e \"$no\" file\n> '\n>\n\nVery well reasoned. I'd say go for it!\n\nTested-by: me\n-- \nDuy\n"},{"id":"130996","messageId":"4B45E9E7.1040905@viscovery.net","threadId":"22062","inReplyTo":"fcaeb9bf1001070527ra1570a0m12182db9bb029055@mail.gmail.com","subject":"Re: [PATCH 1/2] t7002: set test prerequisite \"external-grep\" if supported","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-01-07T14:04:23Z","receivedAt":"2010-01-07T14:04:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nguyen Thai Ngoc Duy schrieb:\n> On 1/7/10, Junio C Hamano wrote:\n>> So by writing the test to check the desired outcome, instead of writing it\n>> for the particular implementation of using external grep optimization, you\n>> will catch both kinds of breakages.\n>>\n>> Perhaps something like this (untested, of course)?\n>>\n>> test_expect_success 'strings in work tree files are not found for skip-wt paths' '\n>> no=\"no such string in the index\" &&\n>> test_must_fail git grep -e \"$no\" --cached file &&\n>> git update-index --skip-worktree file &&\n>> echo \"$no\" >file &&\n>> test_must_fail git grep -e \"$no\" file &&\n>> git update-index --no-skip-worktree file &&\n>> git grep -e \"$no\" file\n>> '\n>>\n> \n> Very well reasoned. I'd say go for it!\n> \n> Tested-by: me\n\nThe test is not quite complete. Not only do you want to test that the\nworktree file is not looked at, but that the index version is used:\n\n\ntest_expect_success 'for skip-wt paths, strings are found in index, not in\nworktree' '\n\tyes=\"this string is in the index\" &&\n\tno=\"no such string in the index\" &&\n\techo \"$yes\" >file &&\n\tgit update-index file &&\n\techo \"$no\" >file &&\n\tgit grep -e \"$yes\" --cached file &&\n\ttest_must_fail git grep -e \"$no\" --cached file &&\n\tgit update-index --skip-worktree file &&\n\tgit grep -e \"$yes\" file &&\n\ttest_must_fail git grep -e \"$no\" file &&\n\tgit update-index --no-skip-worktree file &&\n\ttest_must_fail git grep -e \"$yes\" file &&\n\tgit grep -e \"$no\" file\n'\n\nJust as untested... ;)\n\n-- Hannes\n"},{"id":"130997","messageId":"fcaeb9bf1001070626we21617dt1b76af19a3c4a88d@mail.gmail.com","threadId":"22062","inReplyTo":"4B45E9E7.1040905@viscovery.net","subject":"Re: [PATCH 1/2] t7002: set test prerequisite \"external-grep\" if supported","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-01-07T14:26:27Z","receivedAt":"2010-01-07T14:26:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 1/7/10, Johannes Sixt wrote:\n> Nguyen Thai Ngoc Duy schrieb:\n>\n> > On 1/7/10, Junio C Hamano wrote:\n> >> So by writing the test to check the desired outcome, instead of writing it\n> >> for the particular implementation of using external grep optimization, you\n> >> will catch both kinds of breakages.\n> >>\n> >> Perhaps something like this (untested, of course)?\n> >>\n> >> test_expect_success 'strings in work tree files are not found for skip-wt paths' '\n> >> no=\"no such string in the index\" &&\n> >> test_must_fail git grep -e \"$no\" --cached file &&\n> >> git update-index --skip-worktree file &&\n> >> echo \"$no\" >file &&\n> >> test_must_fail git grep -e \"$no\" file &&\n> >> git update-index --no-skip-worktree file &&\n> >> git grep -e \"$no\" file\n> >> '\n> >>\n> >\n> > Very well reasoned. I'd say go for it!\n> >\n> > Tested-by: me\n>\n>\n> The test is not quite complete. Not only do you want to test that the\n> worktree file is not looked at, but that the index version is used:\n>\n>\n> test_expect_success 'for skip-wt paths, strings are found in index, not in\n> worktree' '\n> yes=\"this string is in the index\" &&\n>\n> no=\"no such string in the index\" &&\n>\n> echo \"$yes\" >file &&\n> git update-index file &&\n> echo \"$no\" >file &&\n> git grep -e \"$yes\" --cached file &&\n>\n> test_must_fail git grep -e \"$no\" --cached file &&\n> git update-index --skip-worktree file &&\n>\n> git grep -e \"$yes\" file &&\n>\n> test_must_fail git grep -e \"$no\" file &&\n> git update-index --no-skip-worktree file &&\n>\n> test_must_fail git grep -e \"$yes\" file &&\n>\n> git grep -e \"$no\" file\n> '\n\nCan we get rid of preparing $yes and do \"grep -e foo file\" instead?\nThere are lots of foo from setup test. It's not as strict as your test\nbecause foo is also in worktree. But we have $no for testing worktree\nalready.\n-- \nDuy\n"},{"id":"131258","messageId":"7vvdf9402f.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001040801290.3630@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T06:39:36Z","receivedAt":"2010-01-11T06:39:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> It doesn't matter. Since we do the line-by-line thing, the input is always \n> so short that DFA vs NFA vs BM vs other-clever-search doesn't matter. \n> There is no scaling - the grep buffer tends to be too small for the \n> algorithm to matter.\n>\n> And the reason we do things line-by-line is that we need to then output \n> things line-per-line.\n\nHere is an experimental patch; first, some numbers (hot cache best of 5 runs).\n\n(baseline -- builtin grep)\n\n $ time git grep --no-ext-grep qwerty\n drivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\"...\n\n real 0m1.521s\n user 0m1.260s\n sys 0m0.256s\n\n (baseline -- external grep)\n\n $ time git grep --ext-grep qwerty\n drivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\"...\n\n real 0m0.773s\n user 0m0.416s\n sys 0m0.376s\n\n (with experimental patch -- builtin grep)\n\n $ time ../git.git/git grep --no-ext-grep qwerty\n drivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\"...\n\n real 0m0.692s\n user 0m0.440s\n sys 0m0.252s\n\nThe idea is to see the earliest location in the entire remainder of the\nbuffer the pattern(s) we are looking for first appears, and skip all the\nlines before that point. For this optimization to be valid, we should not\nbe looking for anything complex (e.g. \"find lines that have both X and Y\nbut not Z\" is out). We cannot use the optimization when \"-v\" is given,\neither, because then we need to find the first line that does _not_ match\ngiven set of patterns.\n\n---\n\n grep.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 64 insertions(+), 0 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex bdadf2c..940e200 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -615,6 +615,65 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \t}\n }\n \n+static int should_lookahead(struct grep_opt *opt)\n+{\n+\tstruct grep_pat *p;\n+\n+\tif (opt->extended)\n+\t\treturn 0; /* punt for too complex stuff */\n+\tif (opt->invert || opt->unmatch_name_only)\n+\t\treturn 0;\n+\tfor (p = opt->pattern_list; p; p = p->next) {\n+\t\tif (p->token != GREP_PATTERN)\n+\t\t\treturn 0; /* punt for \"header only\" and stuff */\n+\t}\n+\treturn 1;\n+}\n+\n+static int look_ahead(struct grep_opt *opt,\n+\t\t unsigned long *left_p,\n+\t\t unsigned *lno_p,\n+\t\t char **bol_p)\n+{\n+\tunsigned lno = *lno_p;\n+\tchar *bol = *bol_p;\n+\tstruct grep_pat *p;\n+\tchar *sp, *last_bol;\n+\tregoff_t earliest = -1;\n+\n+\tfor (p = opt->pattern_list; p; p = p->next) {\n+\t\tint hit;\n+\t\tregmatch_t m;\n+\n+\t\tif (p->fixed)\n+\t\t\thit = !fixmatch(p->pattern, bol, p->ignore_case, &m);\n+\t\telse\n+\t\t\thit = !regexec(&p->regexp, bol, 1, &m, 0);\n+\t\tif (!hit || m.rm_so < 0 || m.rm_eo < 0)\n+\t\t\tcontinue;\n+\t\tif (earliest < 0 || m.rm_so < earliest)\n+\t\t\tearliest = m.rm_so;\n+\t}\n+\n+\tif (earliest < 0) {\n+\t\t*bol_p = bol + *left_p;\n+\t\t*left_p = 0;\n+\t\treturn 1;\n+\t}\n+\tfor (sp = bol + earliest; bol < sp && sp[-1] != '\\n'; sp--)\n+\t\t; /* find the beginning of the line */\n+\tlast_bol = sp;\n+\n+\tfor (sp = bol; sp < last_bol; sp++) {\n+\t\tif (*sp == '\\n')\n+\t\t\tlno++;\n+\t}\n+\t*left_p -= last_bol - bol;\n+\t*bol_p = last_bol;\n+\t*lno_p = lno;\n+\treturn 0;\n+}\n+\n static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\t\t char *buf, unsigned long size, int collect_hits)\n {\n@@ -624,6 +683,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \tunsigned last_hit = 0;\n \tint binary_match_only = 0;\n \tunsigned count = 0;\n+\tint try_lookahead = 0;\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n \n@@ -652,11 +712,15 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\t\topt->priv = &xecfg;\n \t\t}\n \t}\n+\ttry_lookahead = should_lookahead(opt);\n \n \twhile (left) {\n \t\tchar *eol, ch;\n \t\tint hit;\n \n+\t\tif (try_lookahead\n+\t\t && look_ahead(opt, &left, &lno, &bol))\n+\t\t\tbreak;\n \t\teol = end_of_line(bol, &left);\n \t\tch = *eol;\n \t\t*eol = 0;\n"},{"id":"131283","messageId":"alpine.LFD.2.00.1001110739280.13040@localhost.localdomain","threadId":"22062","inReplyTo":"7vvdf9402f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-11T15:43:51Z","receivedAt":"2010-01-11T15:43:51Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Jan 2010, Junio C Hamano wrote:\n> \n> Here is an experimental patch; first, some numbers (hot cache best of 5 runs).\n\nWithout testing it, I can already ACK it. It looks like the \nObviouslyRightThing(tm) to do. But I'll run some numbers too.\n\nOne thing that worries me - but that is independent of this patch - is \nthat I don't think our 'grep' function works correctly (neither the \n'fixmatch()' one or the 'regexec()' one) when there are NUL characters in \na file. Maybe I shouldn't care, but it worries me a bit.\n\n\t\tLinus\n"},{"id":"131285","messageId":"alpine.LFD.2.00.1001110748560.13040@localhost.localdomain","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001110739280.13040@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-11T15:59:18Z","receivedAt":"2010-01-11T15:59:18Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Jan 2010, Linus Torvalds wrote:\n>\n> Without testing it, I can already ACK it. It looks like the \n> ObviouslyRightThing(tm) to do. But I'll run some numbers too.\n\nOk, some good news, some meh news, and some bad news.\n\nThe good news: the trivial numbers look good. It's noticeably faster than \nexternal grep for me when it does the 'fixmatch()' case, quite probably \nbecause fixmatch() on at least Linux/x86-64 (which is the only case I \nreally care about) uses SSE to do the string ops.\n\nSo on my Nehalem:\n\n\t[torvalds@nehalem linux]$ time git grep qwerty > /dev/null \n\n\treal\t0m0.418s\n\tuser\t0m0.204s\n\tsys\t0m0.136s\n\n\t[torvalds@nehalem linux]$ time git grep --no-ext-grep qwerty > /dev/null \n\n\treal\t0m0.309s\n\tuser\t0m0.168s\n\tsys\t0m0.136s\n\nand since that simple fixmatch case is the common one for me, I'm happy.\n\nThe meh news: this shows how grep is faster than regexec() due to being a \nsmarter algorithm. For the non-fixed case (I used \"qwerty.*as\"), the \nnumbers are\n\n - built-in:\n\treal\t0m0.548s\n\tuser\t0m0.384s\n\tsys\t0m0.152s\n\n - external:\n\treal\t0m0.415s\n\tuser\t0m0.176s\n\tsys\t0m0.160s\n\nso it really is just 'strstr()' that is faster. But This is a 'meh', \nbecause I don't really care, and the new code is still way faster than the \nold one. And I'd be personally willing to just drop the external grep if \nthis is the worst problem.\n\n[ I worry a bit that some libc implementations of 'strstr' may suck, but I \n wouldn't lose sleep over it. ]\n\nThe bad news is that you broke multi-line greps:\n\n\tgit grep --no-ext-grep -2 qwerty.*as\n\nresults in:\n\n\tdrivers/char/keyboard.c-unsigned char kbd_sysrq_xlate[KEY_MAX + 1] =\n\tdrivers/char/keyboard.c- \"\\000\\0331234567890-=\\177\\t\" /* 0x00 - 0x0f */\n\tdrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\" /* 0x10 - 0x1f */\n\nwhen the _correct_ result is \n\n\tdrivers/char/keyboard.c-unsigned char kbd_sysrq_xlate[KEY_MAX + 1] =\n\tdrivers/char/keyboard.c- \"\\000\\0331234567890-=\\177\\t\" /* 0x00 - 0x0f */\n\tdrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\" /* 0x10 - 0x1f */\n\tdrivers/char/keyboard.c- \"dfghjkl;'`\\000\\\\zxcv\" /* 0x20 - 0x2f */\n\tdrivers/char/keyboard.c- \"bnm,./\\000*\\000 \\000\\201\\202\\203\\204\\205\" /* 0x30 - 0x3f */\n\nie it didn't do the \"two lines after\" thing.\n\nThat said, the external grep also gets this wrong (a different way), \nbecause it gets all the extra noise due to unnecessary separation lines, \nso for the external grep I actually get\n\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\tdrivers/char/keyboard.c-unsigned char kbd_sysrq_xlate[KEY_MAX + 1] =\n\tdrivers/char/keyboard.c- \"\\000\\0331234567890-=\\177\\t\" /* 0x00 - 0x0f */\n\tdrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\" /* 0x10 - 0x1f */\n\tdrivers/char/keyboard.c- \"dfghjkl;'`\\000\\\\zxcv\" /* 0x20 - 0x2f */\n\tdrivers/char/keyboard.c- \"bnm,./\\000*\\000 \\000\\201\\202\\203\\204\\205\" /* 0x30 - 0x3f */\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\t--\n\nbut that's a long-standing problem, and is more \"ugly\" than \"wrong grep \nresults\".\n\n\t\t\tLinus\n"},{"id":"131289","messageId":"7vtyusr4r7.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001110748560.13040@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T16:22:04Z","receivedAt":"2010-01-11T16:22:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> The bad news is that you broke multi-line greps:\n>\n> \tgit grep --no-ext-grep -2 qwerty.*as\n>\n> results in:\n>\n> \tdrivers/char/keyboard.c-unsigned char kbd_sysrq_xlate[KEY_MAX + 1] =\n> \tdrivers/char/keyboard.c- \"\\000\\0331234567890-=\\177\\t\" /* 0x00 - 0x0f */\n> \tdrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\" /* 0x10 - 0x1f */\n\nMeh. I checked pre-context codepath before sending the patch and was very\nsatisfied that René did the right thing in 49de321 (grep: handle pre\ncontext lines on demand, 2009-07-02), but somehow forgot about the post\ncontext codepath.\n\nAn ObviouslyRightThing fix is this two-liner. We shouldn't lookahead if\nwe want to do something more than just skipping when we see an unmatch for\nthe line we are currently looking at.\n\n grep.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 940e200..ac0ce0b 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -719,6 +719,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\tint hit;\n \n \t\tif (try_lookahead\n+\t\t && !(last_hit\n+\t\t\t && lno <= last_hit + opt->post_context)\n \t\t && look_ahead(opt, &left, &lno, &bol))\n \t\t\tbreak;\n \t\teol = end_of_line(bol, &left);\n"},{"id":"131292","messageId":"7veilwr4ms.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7vtyusr4r7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T16:24:43Z","receivedAt":"2010-01-11T16:24:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Meh. I checked pre-context codepath before sending the patch and was very\n> satisfied that René did the right thing in 49de321 (grep: handle pre\n> context lines on demand, 2009-07-02), but somehow forgot about the post\n> context codepath.\n\nJust to clarify, it was *I* who forgot to check the post context codepath\nwhile adding the lookahead; I didn't mean René forgot anything in 49de321.\n"},{"id":"131293","messageId":"alpine.LFD.2.00.1001110830070.13040@localhost.localdomain","threadId":"22062","inReplyTo":"7vtyusr4r7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-11T16:33:09Z","receivedAt":"2010-01-11T16:33:09Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Jan 2010, Junio C Hamano wrote:\n> \n> An ObviouslyRightThing fix is this two-liner. We shouldn't lookahead if\n> we want to do something more than just skipping when we see an unmatch for\n> the line we are currently looking at.\n\nAck. Works for me. And with that, I'd love for it to go in, and get rid of \nthe external grep. Performance is now a non-issue (it goes both ways), and \nthe internal grep doesn't have the bug with separators between multi-line \ngreps.\n\nAnd dropping the external one gets rid of all the issues with PATHs, crap \n'grep' implementations, and removes actual code. Goodie.\n\n\t\tLinus\n"},{"id":"131307","messageId":"4c8ef71001111126k46626cfas3cb324291cbf44f@mail.gmail.com","threadId":"22062","inReplyTo":"7vvdf9402f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-01-11T19:26:44Z","receivedAt":"2010-01-11T19:26:44Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"[Resending as the first copy didn't reach the list.]\n\nOn Mon, Jan 11, 2010 at 07:39, Junio C Hamano wrote:\n>\n> Linus Torvalds writes:\n>\n> > It doesn't matter. Since we do the line-by-line thing, the input is always\n> > so short that DFA vs NFA vs BM vs other-clever-search doesn't matter.\n> > There is no scaling - the grep buffer tends to be too small for the\n> > algorithm to matter.\n> >\n> > And the reason we do things line-by-line is that we need to then output\n> > things line-per-line.\n>\n> Here is an experimental patch; first, some numbers (hot cache best of 5 runs).\n\nI get some very unexpected results with this patch. (best of 5 runs):\n\nbefore:\n\ntime git grep --no-ext-grep qwerty\ndrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\"\n /* 0x10 - 0x1f */\n\nreal 0m3.531s\nuser 0m3.056s\nsys 0m0.468s\n\nafter:\n\ntime git grep --no-ext-grep qwerty\ndrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\"\n /* 0x10 - 0x1f */\n\nreal 0m4.794s\nuser 0m4.380s\nsys 0m0.404s\n\nwith external grep:\ntime git grep qwerty\ndrivers/char/keyboard.c: \"qwertyuiop[]\\r\\000as\"\n /* 0x10 - 0x1f */\n\nreal 0m1.236s\nuser 0m0.668s\nsys 0m0.544s\n\n\nSo, if I haven't messed up the benchmark, it seems that Junio's patch\nmakes things go _slower_. I don't understand at all why I get these\nresults...\n\nThis is on a laptop with an Intel T2080 processor running Ubuntu 9.10.\n\nAny ideas on how this can be explained?\n\n- Fredrik\n"},{"id":"131308","messageId":"alpine.LFD.2.00.1001111124480.17145@localhost.localdomain","threadId":"22062","inReplyTo":"4c8ef71001111119p253170f8q37bcd3708d894a62@mail.gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-11T19:29:01Z","receivedAt":"2010-01-11T19:29:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Jan 2010, Fredrik Kuivinen wrote:\n> \n> Any ideas on how this can be explained?\n\nCould it be a bad 'strstr()' implementation? \n\nTry a complex pattern (\"qwerty.*as\" finds the same line), and see if that \ntoo is slower than before. If that is faster than it used to be (with \n--no-ext-grep, of course), then it's strstr() that is badly implemented.\n\nFor me, on x86-64 (Fedora-12), strstr() seems to do pretty well. But it's \neasy to do a stupid implementation of strstr that does a 'strlen()' first, \nfor example, and thus always traverses all data _twice_ etc. Depending on \ncache sizes etc, that can end up killing performance (or not mattering \nmuch at all..)\n\n\t\tLinus\n"},{"id":"131309","messageId":"4c8ef71001111140j7e7d0081o7718d956104a2451@mail.gmail.com","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001111124480.17145@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-01-11T19:40:07Z","receivedAt":"2010-01-11T19:40:07Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Mon, Jan 11, 2010 at 20:29, Linus Torvalds\n wrote:\n>\n>\n> On Mon, 11 Jan 2010, Fredrik Kuivinen wrote:\n>>\n>> Any ideas on how this can be explained?\n>\n> Could it be a bad 'strstr()' implementation?\n>\n> Try a complex pattern (\"qwerty.*as\" finds the same line), and see if that\n> too is slower than before. If that is faster than it used to be (with\n> --no-ext-grep, of course), then it's strstr() that is badly implemented.\n\nAh, yes, that's it. With the pattern \"qwerty.*as\" I get 2.5s with the\npatch and 6s without.\n\nThanks.\n\n- Fredrik\n"},{"id":"131313","messageId":"alpine.LFD.2.00.1001111159270.17145@localhost.localdomain","threadId":"22062","inReplyTo":"4c8ef71001111140j7e7d0081o7718d956104a2451@mail.gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-11T20:07:25Z","receivedAt":"2010-01-11T20:07:25Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Jan 2010, Fredrik Kuivinen wrote:\n> >\n> > Try a complex pattern (\"qwerty.*as\" finds the same line), and see if that\n> > too is slower than before. If that is faster than it used to be (with\n> > --no-ext-grep, of course), then it's strstr() that is badly implemented.\n> \n> Ah, yes, that's it. With the pattern \"qwerty.*as\" I get 2.5s with the\n> patch and 6s without.\n\nOk, so on your machine, regcomp() is basically twice as fast as strstr().\n\nWhich is not entirely unexpected: I was actually surprised by strstr() \nbeing apparently so good on my machine. I do not generally expect things \nlike that to be at all optimized for bigger working sets. Most common uses \nof strstr() are in short strings - not \"strings\" that are many kilobytes \nin size (the whole file).\n\nIn fact, I suspect it works so well for me because in my version of glibc \nit's not just SSE-optimized: judging by the naming it's SSE4.2 optimized - \nso the case I see on my machine will _only_ happen on Nehalem-based cores \n(ie the new \"Core i[357]\" cpu's).\n\nIt is entirely possible that strstr in general is a disaster.\n\n\t\tLinus\n"},{"id":"131316","messageId":"4c8ef71001111307q6679039ajbef22f2e1748df56@mail.gmail.com","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001111159270.17145@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-01-11T21:07:00Z","receivedAt":"2010-01-11T21:07:00Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Mon, Jan 11, 2010 at 21:07, Linus Torvalds\n wrote:\n>\n>\n> On Mon, 11 Jan 2010, Fredrik Kuivinen wrote:\n>> >\n>> > Try a complex pattern (\"qwerty.*as\" finds the same line), and see if that\n>> > too is slower than before. If that is faster than it used to be (with\n>> > --no-ext-grep, of course), then it's strstr() that is badly implemented.\n>>\n>> Ah, yes, that's it. With the pattern \"qwerty.*as\" I get 2.5s with the\n>> patch and 6s without.\n>\n> Ok, so on your machine, regcomp() is basically twice as fast as strstr().\n\nYes.\n\n> Which is not entirely unexpected: I was actually surprised by strstr()\n> being apparently so good on my machine. I do not generally expect things\n> like that to be at all optimized for bigger working sets. Most common uses\n> of strstr() are in short strings - not \"strings\" that are many kilobytes\n> in size (the whole file).\n>\n> In fact, I suspect it works so well for me because in my version of glibc\n> it's not just SSE-optimized: judging by the naming it's SSE4.2 optimized -\n> so the case I see on my machine will _only_ happen on Nehalem-based cores\n> (ie the new \"Core i[357]\" cpu's).\n>\n> It is entirely possible that strstr in general is a disaster.\n\nAnother option is to use memmem instead. As we know the length of the\nbuffer already it should be a slight improvement over strstr for\neveryone. memmem may cause some portability problems though as it is a\nGNU extension.\n\nI get these results: (git-grep --no-ext-grep qwerty, best of five)\n\nJunio's patch: 0:04.84\nmemmem (attached patch on top of Junio's): 0:02.91\nregcomp/regexec (I changed is_fixed to always return 0, also on top of\nJunio's): 0:02.02\n\n- Fredrik\n"},{"id":"131318","messageId":"alpine.LFD.2.00.1001111315470.17145@localhost.localdomain","threadId":"22062","inReplyTo":"4c8ef71001111307q6679039ajbef22f2e1748df56@mail.gmail.com","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-11T21:24:57Z","receivedAt":"2010-01-11T21:24:57Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Jan 2010, Fredrik Kuivinen wrote:\n> \n> Another option is to use memmem instead. As we know the length of the\n> buffer already it should be a slight improvement over strstr for\n> everyone. memmem may cause some portability problems though as it is a\n> GNU extension.\n\nI'd almost prefer to just drop the strstr entirely.\n\nIt's not actually all *that* big a win, even on my machine. I get\n\n - strstr:\n\n real 0m0.309s\n user 0m0.168s\n sys 0m0.136s\n\n - regexec:\n\n\treal\t0m0.410s\n\tuser\t0m0.220s\n\tsys\t0m0.116s\n\nso yeah, it's slower, but not by a huge degree. With strstr, \"git grep\" \nactually beats the external grep for me, but I don't really care. It's \nalready way better than it used to be - and clearly strstr has a lot of \npotential problems.\n\nSure memmem() might be better for you than strstr, but on the other hand, \nit might easily be worse than strstr for others - and not just from a \nportability standpoint. Is memmem() optimized to take advantage of SSE4.2? \nI suspect it is not, exactly _because_ it's a GNU extension, so Intel \nhasn't published optimized sample code for people to use.\n\nSo I would argue against even bothering to try memmem. Especially since\tin \nyour case, regexec() is apparently faster than memmem _anyway_. I expect \nthat it is for me too, but I'm too lazy to check.\n\n\t\t\tLinus\n"},{"id":"131354","messageId":"7v63774tfd.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001110830070.13040@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-12T08:29:58Z","receivedAt":"2010-01-12T08:29:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Ack. Works for me. And with that, I'd love for it to go in, and get rid of \n> the external grep. Performance is now a non-issue (it goes both ways), and \n> the internal grep doesn't have the bug with separators between multi-line \n> greps.\n>\n> And dropping the external one gets rid of all the issues with PATHs, crap \n> 'grep' implementations, and removes actual code. Goodie.\n>\n> \t\tLinus\n\nBefore going forward, I found two small nits that should go to maint.\n"},{"id":"131355","messageId":"7v1vhv4tcl.fsf_-_@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7v63774tfd.fsf@alter.siamese.dyndns.org","subject":"[PATCH] grep: lookahead optimization can be used with -L option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-12T08:31:38Z","receivedAt":"2010-01-12T08:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is not an \"inverted logic\" option (\"-v\") that shows lines\nthat do not match given pattern as hits, and skipping lines that\ncannot possibly match wouldn't change its outcome.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Should be squashed in to the \"lookahead\" patch.\n\n grep.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 048b982..03ffcd4 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -614,7 +614,7 @@ static int should_lookahead(struct grep_opt *opt)\n \n \tif (opt->extended)\n \t\treturn 0; /* punt for too complex stuff */\n-\tif (opt->invert || opt->unmatch_name_only)\n+\tif (opt->invert)\n \t\treturn 0;\n \tfor (p = opt->pattern_list; p; p = p->next) {\n \t\tif (p->token != GREP_PATTERN)\n-- \n1.6.6.280.ge295b7.dirty\n"},{"id":"131356","messageId":"7vvdf73eql.fsf_-_@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7v63774tfd.fsf@alter.siamese.dyndns.org","subject":"[PATCH] grep: -L should show empty files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-12T08:32:34Z","receivedAt":"2010-01-12T08:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The -L (--files-without-match) option is supposed to show paths that\nproduced no matches. When running the internal grep on work tree files,\nhowever, we had an optimization to just return on zero-sized files,\nwithout doing anything.\n\nThis optimization doesn't matter too much in practice (a tracked empty\nfile must be rare, or there is something wrong with your project); to\nproduce results consistent with GNU grep, we should stop the optimization\nand show empty files as not having the given pattern.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Fix for a longstanding bug meant for maint.\n\n builtin-grep.c | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex fd450bc..84a5af3 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -159,8 +159,6 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\t\terror(\"'%s': %s\", filename, strerror(errno));\n \t\treturn 0;\n \t}\n-\tif (!st.st_size)\n-\t\treturn 0; /* empty file -- no grep hit */\n \tif (!S_ISREG(st.st_mode))\n \t\treturn 0;\n \tsz = xsize_t(st.st_size);\n-- \n1.6.6.280.ge295b7.dirty\n"},{"id":"131386","messageId":"20100112162154.GA25092@coredump.intra.peff.net","threadId":"22062","inReplyTo":"alpine.LFD.2.00.1001110748560.13040@localhost.localdomain","subject":"Re: [PATCH] grep: do not do external grep on skip-worktree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-12T16:21:54Z","receivedAt":"2010-01-12T16:21:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 11, 2010 at 07:59:18AM -0800, Linus Torvalds wrote:\n\n> The meh news: this shows how grep is faster than regexec() due to being a \n> smarter algorithm. For the non-fixed case (I used \"qwerty.*as\"), the \n> numbers are\n> \n> - built-in:\n> \treal\t0m0.548s\n> \tuser\t0m0.384s\n> \tsys\t0m0.152s\n> \n> - external:\n> \treal\t0m0.415s\n> \tuser\t0m0.176s\n> \tsys\t0m0.160s\n> \n> so it really is just 'strstr()' that is faster. But This is a 'meh', \n> because I don't really care, and the new code is still way faster than the \n> old one. And I'd be personally willing to just drop the external grep if \n> this is the worst problem.\n\nJust for fun, I repeated my pcre tests on what's in pu (which has\nJunio's lookahead patch now). Before they didn't show any improvement\nbecause we wasted all of our time in non-regex code. There is some\nimprovement in just using pcre, but I didn't get any improvement by\ntweaking it:\n\n[pu]\n$ time git grep 'qwerty.*as' >/dev/null\nreal 0m1.007s\nuser 0m0.752s\nsys 0m0.252s\n\n[pu + pcre]\n$ time git grep --no-ext-grep 'qwerty.*as' >/dev/null\nreal 0m0.864s\nuser 0m0.648s\nsys 0m0.212s\n\n[pu + pcre_study]\n$ time git grep --no-ext-grep 'qwerty.*as' >/dev/null\nreal 0m0.866s\nuser 0m0.628s\nsys 0m0.200s\n\n[pu + pcre_dfa_exec]\n$ time git grep --no-ext-grep 'qwerty.*as' >/dev/null\nreal 0m0.868s\nuser 0m0.608s\nsys 0m0.256s\n\nSo pcre seems to buy us about 15%, and tweaking it gets lost in the\nnoise (or I am tweaking it badly, which is entirely possible). I doubt\nit's worth the trouble of supporting pcre for that much.\n\nAnd let me add an additional vote against strstr:\n\n$ time git grep --no-ext-grep qwerty >/dev/null\nreal 0m3.285s\nuser 0m3.032s\nsys 0m0.252s\n\n-Peff\n"},{"id":"131410","messageId":"fabb9a1e1001121327s392f7311t2a7f11081ed70ff8@mail.gmail.com","threadId":"22062","inReplyTo":"7vvdf73eql.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: -L should show empty files","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-12T21:27:37Z","receivedAt":"2010-01-12T21:27:37Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jan 12, 2010 at 09:32, Junio C Hamano wrote:\n> This optimization doesn't matter too much in practice (a tracked empty\n> file must be rare, or there is something wrong with your project);\n\nHow about python projects, where there's an __init__.py file\neverywhere you turn your head? ;)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"131422","messageId":"7v4omqv6tx.fsf_-_@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7v63774tfd.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] grep: rip out support for external grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T06:48:10Z","receivedAt":"2010-01-13T06:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We still allow people to pass --[no-]ext-grep on the command line,\nbut the option is ignored.\n\nSigned-off-by: Junio C Hamano \n---\n Junio C Hamano writes:\n\n > Linus Torvalds writes:\n >\n >> Ack. Works for me. And with that, I'd love for it to go in, and get rid of \n >> the external grep.\n > ...\n > Before going forward, I found two small nits that should go to maint.\n\n These nits out-of-way, we can now start doing this.\n\n Makefile | 10 --\n builtin-grep.c | 305 +------------------------------------------------------\n t/t7002-grep.sh | 6 +-\n 3 files changed, 8 insertions(+), 313 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 4a1e5bc..a4b922e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -185,10 +185,6 @@ all::\n # is a simplified version of the merge sort used in glibc. This is\n # recommended if Git triggers O(n^2) behavior in your platform's qsort().\n #\n-# Define NO_EXTERNAL_GREP if you don't want \"git grep\" to ever call\n-# your external grep (e.g., if your system lacks grep, if its grep is\n-# broken, or spawning external process is slower than built-in grep git has).\n-#\n # Define UNRELIABLE_FSTAT if your system's fstat does not return the same\n # information on a not yet closed file that lstat would return for the same\n # file after it was closed.\n@@ -777,7 +773,6 @@ ifeq ($(uname_S),SunOS)\n \tNO_MKDTEMP = YesPlease\n \tNO_MKSTEMPS = YesPlease\n \tNO_REGEX = YesPlease\n-\tNO_EXTERNAL_GREP = YesPlease\n \tTHREADED_DELTA_SEARCH = YesPlease\n \tifeq ($(uname_R),5.7)\n \t\tNEEDS_RESOLV = YesPlease\n@@ -895,7 +890,6 @@ ifeq ($(uname_S),IRIX)\n \t# NO_MMAP. If you suspect that your compiler is not affected by this\n \t# issue, comment out the NO_MMAP statement.\n \tNO_MMAP = YesPlease\n-\tNO_EXTERNAL_GREP = UnfortunatelyYes\n \tSNPRINTF_RETURNS_BOGUS = YesPlease\n \tSHELL_PATH = /usr/gnu/bin/bash\n \tNEEDS_LIBGEN = YesPlease\n@@ -915,7 +909,6 @@ ifeq ($(uname_S),IRIX64)\n \t# NO_MMAP. If you suspect that your compiler is not affected by this\n \t# issue, comment out the NO_MMAP statement.\n \tNO_MMAP = YesPlease\n-\tNO_EXTERNAL_GREP = UnfortunatelyYes\n \tSNPRINTF_RETURNS_BOGUS = YesPlease\n \tSHELL_PATH=/usr/gnu/bin/bash\n \tNEEDS_LIBGEN = YesPlease\n@@ -1322,9 +1315,6 @@ endif\n ifdef DIR_HAS_BSD_GROUP_SEMANTICS\n \tCOMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS\n endif\n-ifdef NO_EXTERNAL_GREP\n-\tBASIC_CFLAGS += -DNO_EXTERNAL_GREP\n-endif\n ifdef UNRELIABLE_FSTAT\n \tBASIC_CFLAGS += -DUNRELIABLE_FSTAT\n endif\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex a5b6719..4adb971 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -15,14 +15,6 @@\n #include \"grep.h\"\n #include \"quote.h\"\n \n-#ifndef NO_EXTERNAL_GREP\n-#ifdef __unix__\n-#define NO_EXTERNAL_GREP 0\n-#else\n-#define NO_EXTERNAL_GREP 1\n-#endif\n-#endif\n-\n static char const * const grep_usage[] = {\n \t\"git grep [options] [-e] [...] [[--] path...]\",\n \tNULL\n@@ -215,292 +207,12 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \treturn i;\n }\n \n-#if !NO_EXTERNAL_GREP\n-static int exec_grep(int argc, const char **argv)\n-{\n-\tpid_t pid;\n-\tint status;\n-\n-\targv[argc] = NULL;\n-\tpid = fork();\n-\tif (pid < 0)\n-\t\treturn pid;\n-\tif (!pid) {\n-\t\texecvp(\"grep\", (char **) argv);\n-\t\texit(255);\n-\t}\n-\twhile (waitpid(pid, &status, 0) < 0) {\n-\t\tif (errno == EINTR)\n-\t\t\tcontinue;\n-\t\treturn -1;\n-\t}\n-\tif (WIFEXITED(status)) {\n-\t\tif (!WEXITSTATUS(status))\n-\t\t\treturn 1;\n-\t\treturn 0;\n-\t}\n-\treturn -1;\n-}\n-\n-#define MAXARGS 1000\n-#define ARGBUF 4096\n-#define push_arg(a) do { \\\n-\tif (nr < MAXARGS) argv[nr++] = (a); \\\n-\telse die(\"maximum number of args exceeded\"); \\\n-\t} while (0)\n-\n-/*\n- * If you send a singleton filename to grep, it does not give\n- * the name of the file. GNU grep has \"-H\" but we would want\n- * that behaviour in a portable way.\n- *\n- * So we keep two pathnames in argv buffer unsent to grep in\n- * the main loop if we need to do more than one grep.\n- */\n-static int flush_grep(struct grep_opt *opt,\n-\t\t int argc, int arg0, const char **argv, int *kept)\n-{\n-\tint status;\n-\tint count = argc - arg0;\n-\tconst char *kept_0 = NULL;\n-\n-\tif (count <= 2) {\n-\t\t/*\n-\t\t * Because we keep at least 2 paths in the call from\n-\t\t * the main loop (i.e. kept != NULL), and MAXARGS is\n-\t\t * far greater than 2, this usually is a call to\n-\t\t * conclude the grep. However, the user could attempt\n-\t\t * to overflow the argv buffer by giving too many\n-\t\t * options to leave very small number of real\n-\t\t * arguments even for the call in the main loop.\n-\t\t */\n-\t\tif (kept)\n-\t\t\tdie(\"insanely many options to grep\");\n-\n-\t\t/*\n-\t\t * If we have two or more paths, we do not have to do\n-\t\t * anything special, but we need to push /dev/null to\n-\t\t * get \"-H\" behaviour of GNU grep portably but when we\n-\t\t * are not doing \"-l\" nor \"-L\" nor \"-c\".\n-\t\t */\n-\t\tif (count == 1 &&\n-\t\t !opt->name_only &&\n-\t\t !opt->unmatch_name_only &&\n-\t\t !opt->count) {\n-\t\t\targv[argc++] = \"/dev/null\";\n-\t\t\targv[argc] = NULL;\n-\t\t}\n-\t}\n-\n-\telse if (kept) {\n-\t\t/*\n-\t\t * Called because we found many paths and haven't finished\n-\t\t * iterating over the cache yet. We keep two paths\n-\t\t * for the concluding call. argv[argc-2] and argv[argc-1]\n-\t\t * has the last two paths, so save the first one away,\n-\t\t * replace it with NULL while sending the list to grep,\n-\t\t * and recover them after we are done.\n-\t\t */\n-\t\t*kept = 2;\n-\t\tkept_0 = argv[argc-2];\n-\t\targv[argc-2] = NULL;\n-\t\targc -= 2;\n-\t}\n-\n-\tif (opt->pre_context || opt->post_context) {\n-\t\t/*\n-\t\t * grep handles hunk marks between files, but we need to\n-\t\t * do that ourselves between multiple calls.\n-\t\t */\n-\t\tif (opt->show_hunk_mark)\n-\t\t\twrite_or_die(1, \"--\\n\", 3);\n-\t\telse\n-\t\t\topt->show_hunk_mark = 1;\n-\t}\n-\n-\tstatus = exec_grep(argc, argv);\n-\n-\tif (kept_0) {\n-\t\t/*\n-\t\t * Then recover them. Now the last arg is beyond the\n-\t\t * terminating NULL which is at argc, and the second\n-\t\t * from the last is what we saved away in kept_0\n-\t\t */\n-\t\targv[arg0++] = kept_0;\n-\t\targv[arg0] = argv[argc+1];\n-\t}\n-\treturn status;\n-}\n-\n-static void grep_add_color(struct strbuf *sb, const char *escape_seq)\n-{\n-\tsize_t orig_len = sb->len;\n-\n-\twhile (*escape_seq) {\n-\t\tif (*escape_seq == 'm')\n-\t\t\tstrbuf_addch(sb, ';');\n-\t\telse if (*escape_seq != '\\033' && *escape_seq != '[')\n-\t\t\tstrbuf_addch(sb, *escape_seq);\n-\t\tescape_seq++;\n-\t}\n-\tif (sb->len > orig_len && sb->buf[sb->len - 1] == ';')\n-\t\tstrbuf_setlen(sb, sb->len - 1);\n-}\n-\n-static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n-{\n-\tint i, nr, argc, hit, len, status;\n-\tconst char *argv[MAXARGS+1];\n-\tchar randarg[ARGBUF];\n-\tchar *argptr = randarg;\n-\tstruct grep_pat *p;\n-\n-\tif (opt->extended || (opt->relative && opt->prefix_length))\n-\t\treturn -1;\n-\tlen = nr = 0;\n-\tpush_arg(\"grep\");\n-\tif (opt->fixed)\n-\t\tpush_arg(\"-F\");\n-\tif (opt->linenum)\n-\t\tpush_arg(\"-n\");\n-\tif (!opt->pathname)\n-\t\tpush_arg(\"-h\");\n-\tif (opt->regflags & REG_EXTENDED)\n-\t\tpush_arg(\"-E\");\n-\tif (opt->ignore_case)\n-\t\tpush_arg(\"-i\");\n-\tif (opt->binary == GREP_BINARY_NOMATCH)\n-\t\tpush_arg(\"-I\");\n-\tif (opt->word_regexp)\n-\t\tpush_arg(\"-w\");\n-\tif (opt->name_only)\n-\t\tpush_arg(\"-l\");\n-\tif (opt->unmatch_name_only)\n-\t\tpush_arg(\"-L\");\n-\tif (opt->null_following_name)\n-\t\t/* in GNU grep git's \"-z\" translates to \"-Z\" */\n-\t\tpush_arg(\"-Z\");\n-\tif (opt->count)\n-\t\tpush_arg(\"-c\");\n-\tif (opt->post_context || opt->pre_context) {\n-\t\tif (opt->post_context != opt->pre_context) {\n-\t\t\tif (opt->pre_context) {\n-\t\t\t\tpush_arg(\"-B\");\n-\t\t\t\tlen += snprintf(argptr, sizeof(randarg)-len,\n-\t\t\t\t\t\t\"%u\", opt->pre_context) + 1;\n-\t\t\t\tif (sizeof(randarg) <= len)\n-\t\t\t\t\tdie(\"maximum length of args exceeded\");\n-\t\t\t\tpush_arg(argptr);\n-\t\t\t\targptr += len;\n-\t\t\t}\n-\t\t\tif (opt->post_context) {\n-\t\t\t\tpush_arg(\"-A\");\n-\t\t\t\tlen += snprintf(argptr, sizeof(randarg)-len,\n-\t\t\t\t\t\t\"%u\", opt->post_context) + 1;\n-\t\t\t\tif (sizeof(randarg) <= len)\n-\t\t\t\t\tdie(\"maximum length of args exceeded\");\n-\t\t\t\tpush_arg(argptr);\n-\t\t\t\targptr += len;\n-\t\t\t}\n-\t\t}\n-\t\telse {\n-\t\t\tpush_arg(\"-C\");\n-\t\t\tlen += snprintf(argptr, sizeof(randarg)-len,\n-\t\t\t\t\t\"%u\", opt->post_context) + 1;\n-\t\t\tif (sizeof(randarg) <= len)\n-\t\t\t\tdie(\"maximum length of args exceeded\");\n-\t\t\tpush_arg(argptr);\n-\t\t\targptr += len;\n-\t\t}\n-\t}\n-\tfor (p = opt->pattern_list; p; p = p->next) {\n-\t\tpush_arg(\"-e\");\n-\t\tpush_arg(p->pattern);\n-\t}\n-\tif (opt->color) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n-\t\tgrep_add_color(&sb, opt->color_match);\n-\t\tsetenv(\"GREP_COLOR\", sb.buf, 1);\n-\n-\t\tstrbuf_reset(&sb);\n-\t\tstrbuf_addstr(&sb, \"mt=\");\n-\t\tgrep_add_color(&sb, opt->color_match);\n-\t\tstrbuf_addstr(&sb, \":sl=:cx=:fn=:ln=:bn=:se=\");\n-\t\tsetenv(\"GREP_COLORS\", sb.buf, 1);\n-\n-\t\tstrbuf_release(&sb);\n-\n-\t\tif (opt->color_external && strlen(opt->color_external) > 0)\n-\t\t\tpush_arg(opt->color_external);\n-\t} else {\n-\t\tunsetenv(\"GREP_COLOR\");\n-\t\tunsetenv(\"GREP_COLORS\");\n-\t}\n-\tunsetenv(\"GREP_OPTIONS\");\n-\n-\thit = 0;\n-\targc = nr;\n-\tfor (i = 0; i < active_nr; i++) {\n-\t\tstruct cache_entry *ce = active_cache[i];\n-\t\tchar *name;\n-\t\tint kept;\n-\t\tif (!S_ISREG(ce->ce_mode))\n-\t\t\tcontinue;\n-\t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n-\t\t\tcontinue;\n-\t\tname = ce->name;\n-\t\tif (name[0] == '-') {\n-\t\t\tint len = ce_namelen(ce);\n-\t\t\tname = xmalloc(len + 3);\n-\t\t\tmemcpy(name, \"./\", 2);\n-\t\t\tmemcpy(name + 2, ce->name, len + 1);\n-\t\t}\n-\t\targv[argc++] = name;\n-\t\tif (MAXARGS <= argc) {\n-\t\t\tstatus = flush_grep(opt, argc, nr, argv, &kept);\n-\t\t\tif (0 < status)\n-\t\t\t\thit = 1;\n-\t\t\targc = nr + kept;\n-\t\t}\n-\t\tif (ce_stage(ce)) {\n-\t\t\tdo {\n-\t\t\t\ti++;\n-\t\t\t} while (i < active_nr &&\n-\t\t\t\t !strcmp(ce->name, active_cache[i]->name));\n-\t\t\ti--; /* compensate for loop control */\n-\t\t}\n-\t}\n-\tif (argc > nr) {\n-\t\tstatus = flush_grep(opt, argc, nr, argv, NULL);\n-\t\tif (0 < status)\n-\t\t\thit = 1;\n-\t}\n-\treturn hit;\n-}\n-#endif\n-\n-static int grep_cache(struct grep_opt *opt, const char **paths, int cached,\n-\t\t int external_grep_allowed)\n+static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n {\n \tint hit = 0;\n \tint nr;\n \tread_cache();\n \n-#if !NO_EXTERNAL_GREP\n-\t/*\n-\t * Use the external \"grep\" command for the case where\n-\t * we grep through the checked-out files. It tends to\n-\t * be a lot more optimized\n-\t */\n-\tif (!cached && external_grep_allowed) {\n-\t\thit = external_grep(opt, paths, cached);\n-\t\tif (hit >= 0)\n-\t\t\treturn hit;\n-\t\thit = 0;\n-\t}\n-#endif\n-\n \tfor (nr = 0; nr < active_nr; nr++) {\n \t\tstruct cache_entry *ce = active_cache[nr];\n \t\tif (!S_ISREG(ce->ce_mode))\n@@ -697,8 +409,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n {\n \tint hit = 0;\n \tint cached = 0;\n-\tint external_grep_allowed = 1;\n \tint seen_dashdash = 0;\n+\tint external_grep_allowed__ignored;\n \tstruct grep_opt opt;\n \tstruct object_array list = { 0, 0, NULL };\n \tconst char **paths = NULL;\n@@ -780,13 +492,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOLEAN(0, \"all-match\", &opt.all_match,\n \t\t\t\"show only matches from files that match all patterns\"),\n \t\tOPT_GROUP(\"\"),\n-#if NO_EXTERNAL_GREP\n-\t\tOPT_BOOLEAN(0, \"ext-grep\", &external_grep_allowed,\n-\t\t\t\"allow calling of grep(1) (ignored by this build)\"),\n-#else\n-\t\tOPT_BOOLEAN(0, \"ext-grep\", &external_grep_allowed,\n-\t\t\t\"allow calling of grep(1) (default)\"),\n-#endif\n+\t\tOPT_BOOLEAN(0, \"ext-grep\", &external_grep_allowed__ignored,\n+\t\t\t \"allow calling of grep(1) (ignored by this build)\"),\n \t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, \"show usage\",\n \t\t PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n \t\tOPT_END()\n@@ -837,8 +544,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\targc--;\n \t}\n \n-\tif ((opt.color && !opt.color_external) || opt.funcname)\n-\t\texternal_grep_allowed = 0;\n \tif (!opt.pattern_list)\n \t\tdie(\"no pattern given.\");\n \tif (!opt.fixed && opt.ignore_case)\n@@ -884,7 +589,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!list.nr) {\n \t\tif (!cached)\n \t\t\tsetup_work_tree();\n-\t\treturn !grep_cache(&opt, paths, cached, external_grep_allowed);\n+\t\treturn !grep_cache(&opt, paths, cached);\n \t}\n \n \tif (cached)\ndiff --git a/t/t7002-grep.sh b/t/t7002-grep.sh\nindex abd14bf..c369cdb 100755\n--- a/t/t7002-grep.sh\n+++ b/t/t7002-grep.sh\n@@ -302,8 +302,8 @@ test_expect_success 'grep -C1, hunk mark between files' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'grep -C1 --no-ext-grep, hunk mark between files' '\n-\tgit grep -C1 --no-ext-grep \"^[yz]\" >actual &&\n+test_expect_success 'grep -C1 hunk mark between files' '\n+\tgit grep -C1 \"^[yz]\" >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -359,7 +359,7 @@ test_expect_success 'log grep (6)' '\n test_expect_success 'grep with CE_VALID file' '\n \tgit update-index --assume-unchanged t/t &&\n \trm t/t &&\n-\ttest \"$(git grep --no-ext-grep test)\" = \"t/t:test\" &&\n+\ttest \"$(git grep test)\" = \"t/t:test\" &&\n \tgit update-index --no-assume-unchanged t/t &&\n \tgit checkout t/t\n '\n-- \n1.6.6.292.ge84ea.dirty\n"},{"id":"131423","messageId":"7vzl4its43.fsf_-_@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"7v63774tfd.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] grep: rip out pessimization to use fixmatch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T06:51:24Z","receivedAt":"2010-01-13T06:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even when running without the -F (--fixed-strings) option, we checked the\npattern and used fixmatch() codepath when it does not contain any regex\nmagic. Finding fixed strings with strstr() surely must be faster than\nrunning the regular expression crud.\n\nNot so. It turns out that on some libc implementations, using the\nregcomp()/regexec() pair is a lot faster than running strstr() and\nstrcasestr() the fixmatch() codepath uses. Drop the optimization and use\nthe fixmatch() codepath only when the user explicitly asked for it with\nthe -F option.\n\nSigned-off-by: Junio C Hamano \n---\n\n I first thought that this is somewhat questionable and we might want\n an option --[no-]fixed-string-optimization to control it, but there\n already is an option to do so with a shorter name, namely \"-F\".\n\n grep.c | 9 +--------\n 1 files changed, 1 insertions(+), 8 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 62723da..8e1f7de 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -29,13 +29,6 @@ void append_grep_pattern(struct grep_opt *opt, const char *pat,\n \tp->next = NULL;\n }\n \n-static int is_fixed(const char *s)\n-{\n-\twhile (*s && !is_regex_special(*s))\n-\t\ts++;\n-\treturn !*s;\n-}\n-\n static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n {\n \tint err;\n@@ -43,7 +36,7 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \tp->word_regexp = opt->word_regexp;\n \tp->ignore_case = opt->ignore_case;\n \n-\tif (opt->fixed || is_fixed(p->pattern))\n+\tif (opt->fixed)\n \t\tp->fixed = 1;\n \tif (opt->regflags & REG_ICASE)\n \t\tp->fixed = 0;\n-- \n1.6.6.292.ge84ea.dirty\n"},{"id":"131431","messageId":"7vpr5esdbp.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"fabb9a1e1001121327s392f7311t2a7f11081ed70ff8@mail.gmail.com","subject":"Re: [PATCH] grep: -L should show empty files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T06:56:10Z","receivedAt":"2010-01-13T06:56:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Heya,\n>\n> On Tue, Jan 12, 2010 at 09:32, Junio C Hamano wrote:\n>> This optimization doesn't matter too much in practice (a tracked empty\n>> file must be rare, or there is something wrong with your project);\n>\n> How about python projects, where there's an __init__.py file\n> everywhere you turn your head? ;)\n\nIt's Ok as the price we pay for producing correct result is to open those\nempty files, read them, and look for matches which we will never find ;-)\n"},{"id":"131441","messageId":"76718491001130029i24a4edeyf72ce692429dffa6@mail.gmail.com","threadId":"22062","inReplyTo":"7v4omqv6tx.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] grep: rip out support for external grep","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-01-13T08:29:41Z","receivedAt":"2010-01-13T08:29:41Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Jan 13, 2010 at 1:48 AM, Junio C Hamano wrote:\n> We still allow people to pass --[no-]ext-grep on the command line,\n> but the option is ignored.\n\nPerhaps this squashed in on top? (If gmail's web-interface mangles\nthis I can resend.)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex a1e36d7..6a96d9d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -645,14 +645,6 @@ color.grep::\n `never`), never. When set to `true` or `auto`, use color only\n when the output is written to the terminal. Defaults to `false`.\n\n-color.grep.external::\n- The string value of this variable is passed to an external 'grep'\n- command as a command line option if match highlighting is turned\n- on. If set to an empty string, no option is passed at all,\n- turning off coloring for external 'grep' calls; this is the default.\n- For GNU grep, set it to `--color=always` to highlight matches even\n- when a pager is used.\n-\n color.grep.match::\n Use customized color for matches. The value of this variable\n may be specified as in color.branch.. It is passed using\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 4adb971..3d6ebb5 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -34,8 +34,6 @@ static int grep_config(const char *var, const char\n*value, void *cb)\n opt->color = git_config_colorbool(var, value, -1);\n return 0;\n }\n- if (!strcmp(var, \"color.grep.external\"))\n- return git_config_string(&(opt->color_external), var, value);\n if (!strcmp(var, \"color.grep.match\")) {\n if (!value)\n return config_error_nonbool(var);\ndiff --git a/grep.h b/grep.h\nindex 75370f6..0c61b00 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -85,7 +85,6 @@ struct grep_opt {\n int max_depth;\n int funcname;\n char color_match[COLOR_MAXLEN];\n- const char *color_external;\n int regflags;\n unsigned pre_context;\n unsigned post_context;\n"},{"id":"131445","messageId":"7vaawipehn.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"76718491001130029i24a4edeyf72ce692429dffa6@mail.gmail.com","subject":"Re: [PATCH 1/2] grep: rip out support for external grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T08:59:16Z","receivedAt":"2010-01-13T08:59:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; I grepped only for EXTERNAL_GREP and ext-grep and forgot about\nthis one.\n"},{"id":"131482","messageId":"alpine.LFD.2.00.1001130720200.17145@localhost.localdomain","threadId":"22062","inReplyTo":"7v4omqv6tx.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] grep: rip out support for external grep","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-13T15:20:37Z","receivedAt":"2010-01-13T15:20:37Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 12 Jan 2010, Junio C Hamano wrote:\n> \n> These nits out-of-way, we can now start doing this.\n> \n> Makefile | 10 --\n> builtin-grep.c | 305 +------------------------------------------------------\n> t/t7002-grep.sh | 6 +-\n> 3 files changed, 8 insertions(+), 313 deletions(-)\n\nYay!\n\n\t\tLinus\n"},{"id":"131486","messageId":"fabb9a1e1001130804s4716587l57d56884b5d0d068@mail.gmail.com","threadId":"22062","inReplyTo":"7vpr5esdbp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: -L should show empty files","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-13T16:04:18Z","receivedAt":"2010-01-13T16:04:18Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Jan 13, 2010 at 07:56, Junio C Hamano wrote:\n> It's Ok as the price we pay for producing correct result is to open those\n> empty files, read them, and look for matches which we will never find ;-)\n\nI'm not that familiar with the code, but wouldn't it be possible to\nkeep the early abort, but make it dependent on not using the '-L'\nflag?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"131516","messageId":"7viqb5n5ux.fsf@alter.siamese.dyndns.org","threadId":"22062","inReplyTo":"fabb9a1e1001130804s4716587l57d56884b5d0d068@mail.gmail.com","subject":"Re: [PATCH] grep: -L should show empty files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T19:48:38Z","receivedAt":"2010-01-13T19:48:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> I'm not that familiar with the code, but wouldn't it be possible to\n> keep the early abort, but make it dependent on not using the '-L'\n> flag?\n\nAnything codable is possible to code, but my point was I don't think such\nan optimization to avoid reading empty files is worth the time to write\nand maintain extra code necessary for it.\n"}]}