{"thread":{"id":"29381","subject":"git-grep while excluding files in a blacklist","startedAt":"2012-01-17T09:14:59Z","lastAt":"2012-02-04T23:18:25Z","messageCount":43,"participants":["Dov Grobgeld","Nguyen Thai Ngoc Duy","Junio C Hamano","conrad.irwin@gmail.com","Conrad Irwin","Jeff King","Stephen Bash","Michael Haggerty","Thomas Rast","Pete Wyckoff"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182680","messageId":"CA++fsGHGrNQzR-schP0yTXnD4jkYJjHHVk6QoJvfxPX9mguJPQ@mail.gmail.com","threadId":"29381","inReplyTo":null,"subject":"git-grep while excluding files in a blacklist","fromName":"Dov Grobgeld","fromEmail":"dov.grobgeld@gmail.com","sentAt":"2012-01-17T09:14:59Z","receivedAt":"2012-01-17T09:14:59Z","isPatch":false,"sender":{"key":"dov.grobgeld@gmail.com","avatar":"https://gravatar.com/avatar/b74b57a9ce04ef5f356927856e125f1e70e7ce38772c5b8793b854b830f0fe6c?d=mp&s=160"},"body":"Does git-grep allow searching for a pattern in all files *except*\nfiles matching a pattern. E.g. in our project we have multiple DLL's\nin git, but when searching I would like to exclude these for speed. Is\nthat possible with git-grep?\n\nThanks,\nDov\n"},{"id":"182681","messageId":"CACsJy8A8eWt_wcxWrdjgmkHZpS1bBet7DTT-bRf9zrxfszUtjw@mail.gmail.com","threadId":"29381","inReplyTo":"CA++fsGHGrNQzR-schP0yTXnD4jkYJjHHVk6QoJvfxPX9mguJPQ@mail.gmail.com","subject":"Re: git-grep while excluding files in a blacklist","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-17T09:19:49Z","receivedAt":"2012-01-17T09:19:49Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 17, 2012 at 4:14 PM, Dov Grobgeld wrote:\n> Does git-grep allow searching for a pattern in all files *except*\n> files matching a pattern. E.g. in our project we have multiple DLL's\n> in git, but when searching I would like to exclude these for speed. Is\n> that possible with git-grep?\n\nNot from command line, no. You can put \"*.dll\" to .gitignore file then\n\"git grep --exclude-standard\".\n-- \nDuy\n"},{"id":"182695","messageId":"7v4nvurszj.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"CACsJy8A8eWt_wcxWrdjgmkHZpS1bBet7DTT-bRf9zrxfszUtjw@mail.gmail.com","subject":"Re: git-grep while excluding files in a blacklist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-17T20:09:36Z","receivedAt":"2012-01-17T20:09:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Tue, Jan 17, 2012 at 4:14 PM, Dov Grobgeld wrote:\n>> Does git-grep allow searching for a pattern in all files *except*\n>> files matching a pattern. E.g. in our project we have multiple DLL's\n>> in git, but when searching I would like to exclude these for speed. Is\n>> that possible with git-grep?\n>\n> Not from command line, no. You can put \"*.dll\" to .gitignore file then\n> \"git grep --exclude-standard\".\n\nNo rush, but is this something we would eventually want to handle with the\nnegative pathspec?\n"},{"id":"182712","messageId":"CACsJy8C0aXgecCWHrCK3yzNLWnWX4g81x-OzZCY0xtonbspzXw@mail.gmail.com","threadId":"29381","inReplyTo":"7v4nvurszj.fsf@alter.siamese.dyndns.org","subject":"Re: git-grep while excluding files in a blacklist","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-18T01:24:03Z","receivedAt":"2012-01-18T01:24:03Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 18, 2012 at 3:09 AM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>> On Tue, Jan 17, 2012 at 4:14 PM, Dov Grobgeld wrote:\n>>> Does git-grep allow searching for a pattern in all files *except*\n>>> files matching a pattern. E.g. in our project we have multiple DLL's\n>>> in git, but when searching I would like to exclude these for speed. Is\n>>> that possible with git-grep?\n>>\n>> Not from command line, no. You can put \"*.dll\" to .gitignore file then\n>> \"git grep --exclude-standard\".\n>\n> No rush, but is this something we would eventually want to handle with the\n> negative pathspec?\n\nDefinitely. But because I'm stuck at adding \"seen\" feature from\nmatch_pathspec_depth to tree_entry_interesting, that probably won't\nhappen this year. Adding \"--exclude=\" to git-grep is a more\nplausible option.\n-- \nDuy\n"},{"id":"182955","messageId":"4f1d2a8b.a2d8320a.50ec.576d@mx.google.com","threadId":"29381","inReplyTo":"CACsJy8C0aXgecCWHrCK3yzNLWnWX4g81x-OzZCY0xtonbspzXw@mail.gmail.com","subject":"[PATCH] Don't search files with an unset \"grep\" attribute","fromName":"","fromEmail":"conrad.irwin@gmail.com","sentAt":"2012-01-23T09:37:36Z","receivedAt":"2012-01-23T09:37:36Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"From: Conrad Irwin \n\nNguyen Thai Ngoc Duy gmail.com> writes:\n> Definitely. But because I'm stuck at adding \"seen\" feature from\n> match_pathspec_depth to tree_entry_interesting, that probably won't happen\n> this year. Adding \"--exclude=\" to git-grep is a more plausible\n> option.\n\nI think the .gitattributes mechanism is a better place to configure this, as the\nfiles I with to exclude from grep are always the same files (test fixtures\nmainly, minified library code also). I often want to make git-diff be quieter\nabout them too, so I'll be setting the -diff attribute already.\n\nI've attached a patch, which adds the \"grep\" attribute, which can (for now) only\nbe unset. This has the same effect for me as my original patch [1], but should\nalso improve life for people with large blobs as described above.\n\nIn future we could extend the attribute to give a meaning to set values, but I'm\nnot yet sure what that would look like. We could also add an --exclude=\nflag to grep for people who want to configure grep on a per-run basis, but I\nthink that is a much less common desire.\n\nThe failing test attached to this patch is a symptom of a larger issue with the\nway that git-grep handles objects that are not at the root of the repository. A\nmore obvious symptom can be revealed by comparing the output of:\n\n git grep int HEAD:./builtin\n\n cd builtin; git grep int HEAD:./\n\nThe problem is that grep doesn't correctly separate the path from the revision\npart of the spec. It's currently unobvious to me how to fix this but I hope\nsomeone more familiar with the code (Nguyen or Junio) might be able to see a\nway.\n\nYours,\nConrad\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/179299\n\n---8<---\n\n\nTo set -grep on an file in .gitattributes will cause that file to be\nskipped completely while grepping. This can be used to reduce the number\nof false positives when your repository contains files that are\nuninteresting to you, for example test fixtures, dlls or minified source\ncode.\n\nThe other approach considered was to allow an --exclude flag to grep at runtime,\nhowever that better serves the less common use-case of wanting to customise the\nlist of files per-invocation rather than statically.\n\nSigned-off-by: Conrad Irwin \n---\n Documentation/git-grep.txt | 7 +++++++\n Documentation/gitattributes.txt | 9 +++++++++\n builtin/grep.c | 8 ++++++++\n grep.c | 21 +++++++++++++++++++++\n grep.h | 1 +\n t/t7810-grep.sh | 30 ++++++++++++++++++++++++++++++\n 6 files changed, 76 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 6a8b1e3..7c74165 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -242,6 +242,13 @@ OPTIONS\n \tIf given, limit the search to paths matching at least one pattern.\n \tBoth leading paths match and glob(7) patterns are supported.\n \n+ATTRIBUTES\n+----------\n+\n+grep::\n+\tIf the grep attribute is unset on a file using the linkgit:gitattributes[1]\n+\tmechanism, then that file will not be searched.\n+\n Examples\n --------\n \ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex a85b187..3ffcec7 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -869,6 +869,15 @@ If this attribute is not set or has an invalid value, the value of the\n `gui.encoding` configuration variable is used instead\n (See linkgit:git-config[1]).\n \n+Configuring files to search\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+`grep`\n+^^^^^^\n+\n+If the attribute `grep` is unset for a file then linkgit:git-grep[1]\n+will ignore that file while searching for matches.\n+\n \n USING MACRO ATTRIBUTES\n ----------------------\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..9f8dfc0 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -398,6 +398,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \tchar *name;\n \n+\tif (!should_grep_path(opt, filename + tree_name_len)) {\n+\t\treturn 0;\n+\t}\n+\n \tif (opt->relative && opt->prefix_length) {\n \t\tquote_path_relative(filename + tree_name_len, -1, &pathbuf,\n \t\t\t\t opt->prefix);\n@@ -464,6 +468,10 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *name;\n \n+\tif (!should_grep_path(opt, filename)) {\n+\t\treturn 0;\n+\t}\n+\n \tif (opt->relative && opt->prefix_length)\n \t\tquote_path_relative(filename, -1, &buf, opt->prefix);\n \telse\ndiff --git a/grep.c b/grep.c\nindex 486230b..e948576 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"grep.h\"\n+#include \"attr.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n \n@@ -829,6 +830,26 @@ static inline void grep_attr_unlock(struct grep_opt *opt)\n #define grep_attr_unlock(opt)\n #endif\n \n+static struct git_attr_check attr_check[1];\n+static void setup_attr_check(void)\n+{\n+\tif (attr_check[0].attr)\n+\t\treturn; /* already done */\n+\tattr_check[0].attr = git_attr(\"grep\");\n+}\n+int should_grep_path(struct grep_opt *opt, const char *name) {\n+\tint ret = 1;\n+\n+\tgrep_attr_lock(opt);\n+\tsetup_attr_check();\n+\tgit_check_attr(name, 1, attr_check);\n+\tif (ATTR_FALSE(attr_check[0].value))\n+\t\tret = 0;\n+\tgrep_attr_unlock(opt);\n+\n+\treturn ret;\n+}\n+\n static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\ndiff --git a/grep.h b/grep.h\nindex fb205f3..266002d 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -129,6 +129,7 @@ extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field\n extern void compile_grep_patterns(struct grep_opt *opt);\n extern void free_grep_patterns(struct grep_opt *opt);\n extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);\n+extern int should_grep_path(struct grep_opt *opt, const char *name);\n \n extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n extern int grep_threads_ok(const struct grep_opt *opt);\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 7ba5b16..c991518 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -871,4 +871,34 @@ test_expect_success 'mimic ack-grep --group' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with -grep attribute' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'with -grep attribute on specific file' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf hello.c &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'with -grep attribute on specific revision' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf HEAD &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'with -grep attribute on specific file/revision' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf HEAD hello.c &&\n+\trm .gitattributes\n+'\n+\n+test_expect_failure 'with -grep attribute on specific tree' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf HEAD:hello.c &&\n+\trm .gitattributes\n+'\n+\n test_done\n-- \n1.7.9.rc2.1.g1fdd3\n"},{"id":"182978","messageId":"7vy5sy8e0y.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"4f1d2a8b.a2d8320a.50ec.576d@mx.google.com","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-23T18:33:33Z","receivedAt":"2012-01-23T18:33:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"conrad.irwin@gmail.com writes:\n\n> ---8<---\n>\n> To set -grep on an file in .gitattributes will cause that file to be\n> skipped completely while grepping. This can be used to reduce the number\n> of false positives when your repository contains files that are\n> uninteresting to you, for example test fixtures, dlls or minified source\n> code.\n\nPlease reword this to describe the problem being solved first (why it\nneeds to be solved, in what situation you cannot live without the\nfeature), and then describe the approach you used to solve it.\n\nPlain \"grep\" does this:\n\n\t$ grep world hello.*\n\thello.c: printf(\"Hello, world.\\n\");\n Binary file hello.o matches\n\nin order to avoid uselessly spewing garbage at you while reminding you\nthat the command is not showing the whole truth and you _might_ want to\nlook into the elided file if you wanted to with \"grep -a world hello.o\".\nCompared to this, it feels that the result your patch goes too far and\nhides the truth a bit too much to my taste. Maybe it is just me.\n\nPerhaps you, or all participants of your particular project, usually do\nnot want to see any grep hits from minified.js, but you may still want to\nbe able to say \"I want to dig deeper and make sure I have copyright\nstrings in all files\", for example. It is unclear how you envision to\nsupport such a use case building on top of this patch.\n\nYour \"attributes only\" is not an acceptable solution in the longer run,\neven though it is a good first step (i.e. \"attributes first and other\nenhancement later\"). There should be an easy way to get the best of both\nworlds.\n\n> The other approach considered was to allow an --exclude flag to grep at\n> runtime, however that better serves the less common use-case of wanting\n> to customise the list of files per-invocation rather than statically.\n\nI doubt that it is justifiable to call per-invocation \"the less common\".\n\nBy the way, if the uninteresting ones are dll and minified.js, I wonder\nwhy it is insufficient to mark them binary, i.e. uninteresting for the\npurpose of all textual operations not just grep but also for diff.\n\nI am *not* going to ask why they are treated as source and tracked by git\nto begin with.\n"},{"id":"182994","messageId":"1327359555-29457-1-git-send-email-conrad.irwin@gmail.com","threadId":"29381","inReplyTo":"7vy5sy8e0y.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2012-01-23T22:59:15Z","receivedAt":"2012-01-23T22:59:15Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"Junio C Hamano pobox.com> writes:\n> Please reword this to describe the problem being solved first (why it\n> needs to be solved, in what situation you cannot live without the\n> feature), and then describe the approach you used to solve it.\n> \n\nDone — I also removed the extraneous braces from the patch.\n\n> \n> in order to avoid uselessly spewing garbage at you while reminding you\n> that the command is not showing the whole truth and you _might_ want to\n> look into the elided file if you wanted to with \"grep -a world hello.o\".\n> Compared to this, it feels that the result your patch goes too far and\n> hides the truth a bit too much to my taste. Maybe it is just me.\n\nI used to use this approach, hooking into the \"diff\" attribute directly to mark\na file as binary, however that was clearly a hack. When developing this patch I\nwent through a few iterations, one in which -grep meant \"treat this file as\nbinary\", however explaining that in the man page is subtle and ugly: \"HINT: you\nmight want to set a file as binary because you don't want to see results from it\nwhen grepping\". It's much more obvious to have -grep mean \"don't show me\nresults\".\n\nA nicer alternative could be to allow \"grep=binary\" (and for completeness\n\"grep=text\") in addition to \"-grep\". Then people who want to see matches but not\nthe contents of the matches can tell grep that their files are \"binary\". It\nwould also make sense to add \"grep=binary\" to the binary macro-attribute. We\ncould even extend the system arbitrarily to allow something like the textconv\nattributes of git-diff... one step at a time is probably better though.\n\n> Perhaps you, or all participants of your particular project, usually do\n> not want to see any grep hits from minified.js, but you may still want to\n> be able to say \"I want to dig deeper and make sure I have copyright\n> strings in all files\", for example. It is unclear how you envision to\n> support such a use case building on top of this patch.\n\nI think that it would be very reasonable to add a flag to grep to tell it to\nignore the attribute temporarily (like --no-textconv on git-diff) and update the\n\"-a\" shorthand to imply \"--text --no-exclude-attribute\".\n\nYours,\nConrad\n\n---8<---\n\nGit grep is used by developers to search the code stored in their repositories,\nhowever it can give noisy results when the repository contains files that are\nnot of direct interest to development. Examples of such files include test\nfixtures, dlls, or minified source code.\n\nTo help these developers search efficiently, git grep will now use the\ngitattributes mechanism to ignore all files with an unset \"grep\" attribute.\n\nAnother approach considered was to allow an --exclude flag to grep at runtime,\nhowever this is more clunky to use when the set of files to be excluded is\nfixed.\n\nSigned-off-by: Conrad Irwin \n---\n Documentation/git-grep.txt | 7 +++++++\n Documentation/gitattributes.txt | 9 +++++++++\n builtin/grep.c | 6 ++++++\n grep.c | 21 +++++++++++++++++++++\n grep.h | 1 +\n t/t7810-grep.sh | 30 ++++++++++++++++++++++++++++++\n 6 files changed, 74 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 6a8b1e3..7c74165 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -242,6 +242,13 @@ OPTIONS\n \tIf given, limit the search to paths matching at least one pattern.\n \tBoth leading paths match and glob(7) patterns are supported.\n \n+ATTRIBUTES\n+----------\n+\n+grep::\n+\tIf the grep attribute is unset on a file using the linkgit:gitattributes[1]\n+\tmechanism, then that file will not be searched.\n+\n Examples\n --------\n \ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex a85b187..3ffcec7 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -869,6 +869,15 @@ If this attribute is not set or has an invalid value, the value of the\n `gui.encoding` configuration variable is used instead\n (See linkgit:git-config[1]).\n \n+Configuring files to search\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+`grep`\n+^^^^^^\n+\n+If the attribute `grep` is unset for a file then linkgit:git-grep[1]\n+will ignore that file while searching for matches.\n+\n \n USING MACRO ATTRIBUTES\n ----------------------\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..a7817fe 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -398,6 +398,9 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \tchar *name;\n \n+\tif (!should_grep_path(opt, filename + tree_name_len))\n+\t\treturn 0;\n+\n \tif (opt->relative && opt->prefix_length) {\n \t\tquote_path_relative(filename + tree_name_len, -1, &pathbuf,\n \t\t\t\t opt->prefix);\n@@ -464,6 +467,9 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *name;\n \n+\tif (!should_grep_path(opt, filename))\n+\t\treturn 0;\n+\n \tif (opt->relative && opt->prefix_length)\n \t\tquote_path_relative(filename, -1, &buf, opt->prefix);\n \telse\ndiff --git a/grep.c b/grep.c\nindex 486230b..e948576 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"grep.h\"\n+#include \"attr.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n \n@@ -829,6 +830,26 @@ static inline void grep_attr_unlock(struct grep_opt *opt)\n #define grep_attr_unlock(opt)\n #endif\n \n+static struct git_attr_check attr_check[1];\n+static void setup_attr_check(void)\n+{\n+\tif (attr_check[0].attr)\n+\t\treturn; /* already done */\n+\tattr_check[0].attr = git_attr(\"grep\");\n+}\n+int should_grep_path(struct grep_opt *opt, const char *name) {\n+\tint ret = 1;\n+\n+\tgrep_attr_lock(opt);\n+\tsetup_attr_check();\n+\tgit_check_attr(name, 1, attr_check);\n+\tif (ATTR_FALSE(attr_check[0].value))\n+\t\tret = 0;\n+\tgrep_attr_unlock(opt);\n+\n+\treturn ret;\n+}\n+\n static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\ndiff --git a/grep.h b/grep.h\nindex fb205f3..266002d 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -129,6 +129,7 @@ extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field\n extern void compile_grep_patterns(struct grep_opt *opt);\n extern void free_grep_patterns(struct grep_opt *opt);\n extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);\n+extern int should_grep_path(struct grep_opt *opt, const char *name);\n \n extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n extern int grep_threads_ok(const struct grep_opt *opt);\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 7ba5b16..c991518 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -871,4 +871,34 @@ test_expect_success 'mimic ack-grep --group' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with -grep attribute' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'with -grep attribute on specific file' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf hello.c &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'with -grep attribute on specific revision' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf HEAD &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'with -grep attribute on specific file/revision' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf HEAD hello.c &&\n+\trm .gitattributes\n+'\n+\n+test_expect_failure 'with -grep attribute on specific tree' '\n+\techo \"hello.c -grep\" >.gitattributes &&\n+\ttest_must_fail git grep printf HEAD:hello.c &&\n+\trm .gitattributes\n+'\n+\n test_done\n-- \n1.7.9.rc2.1.g1fdd3\n"},{"id":"183009","messageId":"7vaa5d4mce.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"1327359555-29457-1-git-send-email-conrad.irwin@gmail.com","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-24T06:59:45Z","receivedAt":"2012-01-24T06:59:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Conrad Irwin writes:\n\n>> in order to avoid uselessly spewing garbage at you while reminding you\n>> that the command is not showing the whole truth and you _might_ want to\n>> look into the elided file if you wanted to with \"grep -a world hello.o\".\n>> Compared to this, it feels that the result your patch goes too far and\n>> hides the truth a bit too much to my taste. Maybe it is just me.\n>\n> I used to use this approach, hooking into the \"diff\" attribute directly to mark\n> a file as binary, however that was clearly a hack.\n\nAfter thinking about this a bit more, I have to say I disagree that it is\na hack.\n\nI agree that the issue you are trying to address is real, but I think the\napproach taken by the patch is flawed on three counts:\n\n - You cannot do \"grep -a\" equivalent once you set your \"-grep\" attribute;\n\n - The user has flexibility to set \"diff\" and \"grep\" independently, which\n is an unnecessary complication [*1*]; and\n\n - The user does not get any hint that potential hits are elided, like\n normal \"grep\" would do.\n\nI also think we can fix the above problems, while simplifying the external\ninterface visible to the end users.\n\nSo let's step back a bit and take a look at the handling of files for\nwhich you do not want to see patch output and/or you do not want to see\ngrep hits, in a fictional but realisitic use scenario.\n\nI am building a small embedded applicance that links with a binary blob\nfirmware, xyzzy.so, supplied by an upstream vendor, and this blob is\nupdated from time to time. In order to keep track of what binary blob went\ninto which product release, this binary blob is stored in the repository\ntogether with the source files. Because it is useless to view binary\nchanges in \"git log -p\" output, I would naturally mark this as \"binary\".\n\nNow, is it realistic to expect that I might want to see \"grep\" hits from\nthis binary blob? I do not think so [*2*].\n\nWhen I say \"xyzzy.so is a binary\" in my .gitattributes file, according to\nthe current documentation, I am saying that \"do not run diff, and also do\nnot run EOL conversion on this file\" [*3*], but from the end user's point\nof view, it is natural that I am entitled to expect much more than that.\nWithout having to know how many different kinds of textual operations the\nSCM I happen to be using supports, I am telling it that I do not want to\nsee _any_ textual operations done on it. If a new version of \"git grep\"\nlearns to honor the attributes system, I want my \"this is binary\" to be\nconsidered by the improved \"git grep\".\n\nIf you think about it this way from the very high level description of the\nproblem, aka, end user's point of view, it is fairly clear that tying the\n\"binary\" attribute to \"git grep\" to allow us to override the built-in\nbuffer_is_binary() call you see in grep.c gives the most intuitive result,\nwithout forcing the user to do anything more than they are already doing.\n\nBecause \"binary\" decomposes to \"-diff\" and \"-text\", we have two choices.\nWe _could_ say \"-text\" should be the one that overrides buffer_is_binary()\nautodetection, but using \"-diff\" instead for that purpose opens the door\nfor us to give our users even more intuitive and consistent experience.\n\nSuppose that this binary blob firmware came with an API manual formatted\nin PDF, xyzzy.pdf, also supplied by the vendor. It is also kept in the\nrepository, but again, running textual diff between updated versions of\nthe PDF documentation would not be very useful. I however may have a\ntextconv filter defined for it to grab the text by running pdf2ascii.\n\nNow if my \"git show --textconv xyzzy.pdf\" has an output line that says a\nstring \"XYZZY API 2.0\" was added to the current version, wouldn't it be\nnatural for me to expect that \"git grep --textconv 'XYZZY API' xyzzy.pdf\"\nto find it [*4*]?\n\nAs an added bonus, if you truly want to omit _any_ hit from \"git grep\" for\nyour minified.js files, you can easily do so. Just define a textconv\nfilter that yields an empty string for them, and \"grep --textconv\" won't\nproduce any matches, not even \"Binary file ... matches\".\n\nSo I am inclined to think that reusing the infrastructure we already have\nfor the `diff` attribute as much as possible would be a better design for\nthis new feature you are proposing.\n\nThe name of the attribute `diff` is somewhat unfortunate, but the end user\ninterface to the feature at the highest level is already called \"binary\"\nattribute (macro), and our low-level documentation can give precise\nmeaning of the attribute while alluding to the origin of the name so that\nintelligent readers would understand it pretty easily (see attached\npatch).\n\nBesides, we already tell the users in \"Marking files as binary\" section to\nmark them with \"-diff\" in the attributes file if they want their contents\ntreated as binary.\n\n\n[Footnotes]\n\n*1* An unused flexibility like this does nothing useful, other than\nforcing the user to flip two attributes always as a pair, when one should\nsuffice.\n\n*2* The essence of my previous message was \"why it is insufficient to mark\nthem binary, i.e. uninteresting for the purpose of all textual operations\nnot just grep but also for diff.\" which was unanswered.\n\n*3* This is because \"binary\" is defined as an attribute macro that expands\nto \"-diff -text\".\n\n*4* This also suggests that the infrastructure we already have for driving\n\"git diff\" is a good match for \"git grep\". The \"funcname\" patterns, which\n\"git grep -p\" already can borrow and use from \"diff\" infrastructure, is\nanother indication that using `diff` is a good match for \"git grep\".\n\n\n Documentation/gitattributes.txt | 15 ++++++++++++---\n 1 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex a85b187..63844c4 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -388,9 +388,18 @@ Generating diff text\n `diff`\n ^^^^^^\n \n-The attribute `diff` affects how 'git' generates diffs for particular\n-files. It can tell git whether to generate a textual patch for the path\n-or to treat the path as a binary file. It can also affect what line is\n+The attribute `diff` affects if `git` treats the contents of file as text\n+or binary. Historically, `git diff` and its friends were the first to use\n+this attribute, hence its name, but the attribute governs textual\n+operations in general, including `git grep`.\n+\n+For `git diff` and its friends, differences in contents marked as text\n+produces a textual patch, while differences in non-text are reported only\n+as \"Binary files differ\". For `git grep`, matches in contents marked as\n+text are reported by showing lines that contain them, while matches in\n+non-text are reported only as \"Binary file ... matches\".\n+\n+This attribute can also affect what line is\n shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an\n external command to generate the diff, or ask git to convert binary\n files to a text format before generating the diff.\n"},{"id":"183096","messageId":"20120125214625.GA4666@sigill.intra.peff.net","threadId":"29381","inReplyTo":"7vaa5d4mce.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-25T21:46:26Z","receivedAt":"2012-01-25T21:46:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 23, 2012 at 10:59:45PM -0800, Junio C Hamano wrote:\n\n> Conrad Irwin writes:\n> > I used to use this approach, hooking into the \"diff\" attribute directly to mark\n> > a file as binary, however that was clearly a hack.\n> \n> After thinking about this a bit more, I have to say I disagree that it is\n> a hack.\n\nI kind of agree.\n\nThe biggest problem is that the name is wrong. The \"diff.*.command\"\noption really is about generating a diff between two blobs of a certain\ntype. But \"diff.*.textconv\" and \"diff.*.binary\" are really just\nattributes of the file, and may or may not have to do with generating a\ndiff. Ditto for diff.*.funcname, I think.\n\nYou argue, and I agree, that if we are talking about attributes of the\nfiles and not diff-specific things, then other parts of git can and\nshould make use of that information.\n\nSo if this was all spelled:\n\n $ cat .gitattributes\n *.pdf filetype=pdf\n $ cat .git/config\n [filetype \"pdf\"]\n binary = true\n textconv = pdf2txt\n\nI think it would be a no-brainer that those type attributes should apply\nto \"git grep\".\n\nSo maybe the first step on this path would be to introduce something\nlike \"filetype\" as a new attribute, have \"diff\" respect its settings,\nand recommend people set up filetypes as appropriate. Or maybe that just\nmakes things more confusing in the long run, and we are better off\nsimply accepting that the name is slightly misleading. But either way,\nit seems clear that git should be respecting gitattributes at the very\nleast to mark files as binary (and I think we already use funcname\npatterns; textconv is a slightly stickier subject, so I'll address that\nbelow).\n\nBut what I'm not sure I agree with is that the idea of \"I don't want to\ninclude path X in my grep\" maps to \"just mark the file as binary\". For\nexample, in git we carry around a lot of code in compat/ that comes from\nother places. I generally don't want to see grep results from\ncompat/nedmalloc/, because that isn't git code.\n\nBut should I mark everything in compat/nedmalloc as binary? I don't\nthink so. I _do_ want to see changes in nedmalloc in \"git log\" or \"git\ndiff\". They don't bother me because they're infrequent, and we still\nwant to produce regular text patches for the list when they come up. But\nbecause nedmalloc contains a lot of lines of code (even though they\ndon't change a lot), it happens to produce a lot of uninteresting\nmatches when grepping.\n\n> - The user has flexibility to set \"diff\" and \"grep\" independently, which\n> is an unnecessary complication [*1*]; and\n\nIn the nedmalloc case, if we are to have \"grep\" and \"diff\" attributes\nthat behave similarly, you potentially do want to set them differently.\nIt would be nice to be able to treat them differently in the cases you\nwanted to, but not _have_ to do so. Attribute macros can almost\nimplement this. You could add \"-grep\" to binary. But you can't (as far\nas I know) do \"macro=foo\" to handle arbitrary diff drivers. I suspect we\ncould extend the rules to allow macros that take an argument and pass it\nto their constituent attributes.\n\nHowever, I think this is the wrong road to go down. You would want\nmacros like this _if_ you have grep and diff attributes that basically\ndo the same thing (e.g., marking a file as binary for diff versus binary\nfor grep). But I think that's a wrong road to go down. More likely a\nfile is binary or it is not binary, and the problem is conflating \"file\nis binary\" with \"I do not usually want to grep this file\".\n\nI'd much rather see grep inherit diff's file attributes unconditionally\n(whether we still call them \"diff\" or not), and add a grep attribute\nthat is about \"usually this is worth grepping\". And then have a\ntri-state command-line option for grep to either include uninteresting\nthings, exclude them, or to give terse output for them (mention that\nthere were matches in foo.c, but not each one). Probably defaulting to\nterse output.\n\nThat makes the case you presented work out of the box: things marked as\nbinary for diffing look binary to grep, and we give the usual terse\n\"binary file matches\" output. The user doesn't have to do anything. For\nmore complex cases like nedmalloc, you can still achieve the \"this is\ntext, but it's usually boring\" behavior. And if you really want to do a\nthorough grep, you can just \"git grep --exclude=none\".\n\n> So let's step back a bit and take a look at the handling of files for\n> which you do not want to see patch output and/or you do not want to see\n> grep hits, in a fictional but realisitic use scenario.\n> [...]\n\nI think this is a nice user story, and it fits in with your suggested\ngit behavior. But I think there are other stories, too, like the\nnedmalloc one. And it would be nice to make them work without hurting\nthe simplicity of the case you mentioned.\n\n> If you think about it this way from the very high level description of the\n> problem, aka, end user's point of view, it is fairly clear that tying the\n> \"binary\" attribute to \"git grep\" to allow us to override the built-in\n> buffer_is_binary() call you see in grep.c gives the most intuitive result,\n> without forcing the user to do anything more than they are already doing.\n\nThis is not a complaint about the core of your point, but rather an\naside that should be considered: how many people are really using the\nbinary macro attribute? Personally, I never use it, because when I mark\nsomething with a \"diff\" attribute, it is because I am telling git about\na specific diff driver (usually textconv). Otherwise I don't bother\nsetting attributes at all, because git's binary detection tends to be\ngood. This leaves me without setting \"-text\", of course, but I don't\ngenerally care because I don't do CRLF conversion at all.\n\nSo I think it is worth considering not just people setting \"binary\", but\nhow users of just \"diff\" (both \"-diff\" and \"diff=foo\") will want things\nto work.\n\n> Suppose that this binary blob firmware came with an API manual formatted\n> in PDF, xyzzy.pdf, also supplied by the vendor. It is also kept in the\n> repository, but again, running textual diff between updated versions of\n> the PDF documentation would not be very useful. I however may have a\n> textconv filter defined for it to grab the text by running pdf2ascii.\n> \n> Now if my \"git show --textconv xyzzy.pdf\" has an output line that says a\n> string \"XYZZY API 2.0\" was added to the current version, wouldn't it be\n> natural for me to expect that \"git grep --textconv 'XYZZY API' xyzzy.pdf\"\n> to find it [*4*]?\n\nThis is an interesting concept. As a user of textconv, I already have\nsome specialized grep tools for matching inside binary files (e.g., I\nhave a tool that greps within exif tags of images). But being able to do\nso with \"git grep\", and even at an arbitrary revision, is kind of neat.\n\nI would worry about turning it on by default, since the results could be\nmisleading. In particular, your pattern \"foo\" might be in the binary\nfile but not in the textconv'd version, leading you to think you had\nfound all instances of \"foo\" but had not (or much more subtle, things\nlike line breaks really matter during the conversion if you are going to\nbe using \"grep -C\").\n\nMaking it available by \"--textconv\" seems reasonable to me, though. The\nonly inconsistency is that it's on by default for \"git show\", but would\nnot be for \"git grep\".\n\nPerhaps I am being overly paranoid on the \"misleading\" bit above. It\nseems to me that grep has the room to be a lot more subtle, because an\nomission from the output is considered \"did not match\". But you could\nconstruct equally weird scenarios for \"git show\" (e.g., you changed\n\"foo\" to \"bar\" but that part did not appear in the textconv portion.\nWhich is really a quality-of-implementation issue for your textconv\nfilter).\n\n> As an added bonus, if you truly want to omit _any_ hit from \"git grep\" for\n> your minified.js files, you can easily do so. Just define a textconv\n> filter that yields an empty string for them, and \"grep --textconv\" won't\n> produce any matches, not even \"Binary file ... matches\".\n\nClever. But then you will never ever see a diff for that file, either,\nbecause we will consider all changes to be empty (actually, I didn't\ncheck, but you may get the diff header without any content, similar to\nthe stat-dirty entries).\n\n-Peff\n"},{"id":"183117","messageId":"1af46e50-fdc5-47b8-af36-d070d91dd954@mail","threadId":"29381","inReplyTo":"20120125214625.GA4666@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Stephen Bash","fromEmail":"bash@genarts.com","sentAt":"2012-01-26T13:51:52Z","receivedAt":"2012-01-26T13:51:52Z","isPatch":true,"sender":{"key":"bash@genarts.com","avatar":null},"body":"----- Original Message -----\n> From: \"Jeff King\" \n> Sent: Wednesday, January 25, 2012 4:46:26 PM\n> Subject: Re: [PATCH] Don't search files with an unset \"grep\" attribute\n>\n> ... snip ...\n> \n> So if this was all spelled:\n> \n> $ cat .gitattributes\n> *.pdf filetype=pdf\n> $ cat .git/config\n> [filetype \"pdf\"]\n> binary = true\n> textconv = pdf2txt\n> \n> I think it would be a no-brainer that those type attributes should\n> apply to \"git grep\".\n\nLooking at this purely as a user, what difference/advantage would that bring versus\n\n $ cat .gitattributes\n *.pdf binary=true textconv=pdf2text\n\nor\n\n $ cat .gitattributes\n [attr]pdf binary=true textconv=pdf2text\n *.pdf pdf\n\n(admittedly I have no clue if gitattributes actually supports anything like this)\n\nI guess my point is as a user, I've gravitated to \"gitattributes is about files in my repo, gitconfig is about Git's behavior\" (though this is a grey area).\n\nTo partially answer my own question: one advantage of putting the filetype information in a config file is it allows system- and user-wide filetype settings. In my personal experience I've always handled that information on a per-repository basis, but that doesn't mean everyone would want to.\n\nThanks,\nStephen\n"},{"id":"183121","messageId":"4F21831C.7060609@alum.mit.edu","threadId":"29381","inReplyTo":"20120125214625.GA4666@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-26T16:45:16Z","receivedAt":"2012-01-26T16:45:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/25/2012 10:46 PM, Jeff King wrote:\n> But what I'm not sure I agree with is that the idea of \"I don't want to\n> include path X in my grep\" maps to \"just mark the file as binary\".\n\nAnybody who wants this policy can simply set\n\n [attr]binary -diff -text -grep\n\nIf they want finer granularity, they can adjust the settings for\nparticular file types or for particular files.\n\n> But should I mark everything in compat/nedmalloc as binary? I don't\n> think so. I _do_ want to see changes in nedmalloc in \"git log\" or \"git\n> diff\". They don't bother me because they're infrequent, and we still\n> want to produce regular text patches for the list when they come up. But\n> because nedmalloc contains a lot of lines of code (even though they\n> don't change a lot), it happens to produce a lot of uninteresting\n> matches when grepping.\n\nI think decisions such as whether to include an imported module in \"git\ndiff\" output is a personal preference and should not be decided at the\nlevel of the git project. The in-tree .gitattributes files should, by\nand large, just *describe* the files and leave it to users to associate\npolicies with the tags (or at least make it possible for users to\noverride the policies) via .git/info/attributes. For example, the\nrepository could set an \"external=nedmalloc\" attribute on all files\nunder compat/nedmalloc, and users could themselves configure a macro\n\"[attr]external -diff -grep\" (or maybe something like\n\"[attr]external=nedmalloc -diff -grep\") if that is their preference.\n\n> It would be nice to be able to treat them differently in the cases you\n> wanted to, but not _have_ to do so. Attribute macros can almost\n> implement this. You could add \"-grep\" to binary. But you can't (as far\n> as I know) do \"macro=foo\" to handle arbitrary diff drivers. I suspect we\n> could extend the rules to allow macros that take an argument and pass it\n> to their constituent attributes.\n\nIs it really common to want to use the same argument on multiple macros\nwithout also wanting to set other things specifically? If not, then\nthere is not much reason to complicate macros with argument support.\n\nFor example, I do something like\n\n [attr]type-python type=python text diff=python check-ws\n *.py type-python\n\n [attr]type-makefile type=makefile text diff check-ws -check-tab\n Makefile.* type-makefile\n\nfor the main file types in my repository, and it is not very cumbersome.\n\n\"type-python\" and \"type=python\" seem redundant but they are not.\n\"type-python\" is needed so that it can be used as a macro.\n\"type=python\" makes it easier to inquire about the type of a file using\nsomething like \"git check-attr type -- PATH\" rather than having to\ninquire about each possible type-* attribute. It might be nice to\nsupport a slightly extended macro definition syntax like\n\n [attr]type=python text diff=python check-ws\n *.py type=python\n\n [attr]type=makefile text diff check-ws -check-tab\n Makefile.* type=makefile\n\n(i.e., macros that are only triggered for particular values of an\nattribute).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"183125","messageId":"20120126172948.GC5278@sigill.intra.peff.net","threadId":"29381","inReplyTo":"1af46e50-fdc5-47b8-af36-d070d91dd954@mail","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-26T17:29:48Z","receivedAt":"2012-01-26T17:29:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2012 at 08:51:52AM -0500, Stephen Bash wrote:\n\n> > $ cat .gitattributes\n> > *.pdf filetype=pdf\n> > $ cat .git/config\n> > [filetype \"pdf\"]\n> > binary = true\n> > textconv = pdf2txt\n> \n> Looking at this purely as a user, what difference/advantage would that bring versus\n> \n> $ cat .gitattributes\n> *.pdf binary=true textconv=pdf2text\n\nFor \"binary\", probably not much. But for textconv, it is all about the\nsplit between attributes and config, as mentioned below:\n\n> To partially answer my own question: one advantage of putting the\n> filetype information in a config file is it allows system- and\n> user-wide filetype settings. In my personal experience I've always\n> handled that information on a per-repository basis, but that doesn't\n> mean everyone would want to.\n\nRight. Setting things system-wide instead of per-repo is one advantage.\nBut more important is that attributes are not per-repo, but rather\n\"per-project\". They get committed, and everybody who works on the\nproject shares them.\n\nIn your example, the gitattributes get committed, and the project is\nmandating \"you _will_ use pdf2text to view diffs of these files\". But\nthat may not be appropriate for everybody who clones. Somebody may have\na different pdf-to-text converter. Somebody may simply have pdf2txt at a\ndifferent path, or need different options. Or somebody may want to skip\nit altogether and use an external diff command, or even just see the\nfiles as binary.\n\nBy splitting the information across the two files, the project gets to\nsay \"this file is of type pdf\", and then each user gets to decide \"how\ndo I want to diff pdf files?\"\n\n-Peff\n"},{"id":"183168","messageId":"20120127063503.GA23934@sigill.intra.peff.net","threadId":"29381","inReplyTo":"4F21831C.7060609@alum.mit.edu","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-27T06:35:03Z","receivedAt":"2012-01-27T06:35:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2012 at 05:45:16PM +0100, Michael Haggerty wrote:\n\n> I think decisions such as whether to include an imported module in \"git\n> diff\" output is a personal preference and should not be decided at the\n> level of the git project.\n\nYou're right. I thought of it as an annotation that the project could\nmark via .gitattributes, or the user could mark via .git/info/attributes.\nBut that is not following the right split of responsibility for\nattributes and config. The attributes should annotate \"this isn't really\npart of the regular git code base\" or \"this is really part of the\nnedmalloc codebase\". And then the _config_ should say \"when I am\ngrepping, I am not interested in nedmalloc\". I.e.:\n\n # mark a set of paths with an attribute\n echo \"compat/nedmalloc external\" >>.gitattributes\n\n # and then ignore that attribute for this grep\n git grep --exclude-attr=external \n\n # or for all greps\n git config --add grep.exclude external\n\nand git doesn't even have to care about what the attribute is called.\nIt's between the project and the user how they want to annotate their\nfiles, and how they want to feed them to grep.\n\nOr any other program, for that matter. I wonder if this could also be a\nmore powerful way of grouping files to be included or excluded from diff\npathspecs. Something like (and I'm just talking off the top of my head,\nso there may be some syntactic conflicts here):\n\n # annotate some files\n cat >>.gitattributes <<-\\EOF\n t/t????-*.sh test-script\n t/lib-*.sh test-script\n t/test-lib.sh test-script\n EOF\n\n # and then consider the tagged files to be a group, and look only at\n # that group\n git log :attr:test-script\n\n # ditto, but imagine we had the negative pathspecs Duy has proposed\n git log :~attr:test-script\n\nThat seems kind of cool to me. But maybe it is getting into crazy\nover-engineering. I like the idea that we don't need a new option to\ngrep or diff; rather it is simply a new syntax for mentioning paths.\n\n> The in-tree .gitattributes files should, by and large, just *describe*\n> the files and leave it to users to associate policies with the tags\n> (or at least make it possible for users to override the policies) via\n> .git/info/attributes. For example, the repository could set an\n> \"external=nedmalloc\" attribute on all files under compat/nedmalloc,\n> and users could themselves configure a macro \"[attr]external -diff\n> -grep\" (or maybe something like \"[attr]external=nedmalloc -diff\n> -grep\") if that is their preference.\n\nSo obviously I took what you were saying here and ran with it above. But\nI do disagree with one thing here: the attributes should be giving some\ntag to the paths, but the actual decision about whether to grep should\nbe part of the _config_. That's the usual split we have for all of the\nother attributes, and I think it makes sense and has worked well.\n\n> Is it really common to want to use the same argument on multiple macros\n> without also wanting to set other things specifically? If not, then\n> there is not much reason to complicate macros with argument support.\n\nI dunno. I admit my attribute usage tends to just match by extension,\nand I generally only have one or two such lines.\n\n> For example, I do something like\n> \n> [attr]type-python type=python text diff=python check-ws\n> *.py type-python\n> \n> [attr]type-makefile type=makefile text diff check-ws -check-tab\n> Makefile.* type-makefile\n> \n> for the main file types in my repository, and it is not very cumbersome.\n\nI think it's not a big deal if you are making your own macros. I was\nmore concerned that people would want to use the \"binary\" macro to get\nthe \"-grep\" automagic, but could not do so because they don't want\n\"-diff\", but rather \"diff=foo\".\n\nAnyway, after reading your response and thinking on it more, I think\n\"-grep\" is totally the wrong way to go. If the files are marked binary,\nthen grep should be respecting \"-diff\" or the \"diff.*.binary\" config. If\nwe want to do more advanced exclusion, then the right place for that is\nthe config file (or the weird :attr pathspec thing I mentioned above).\n\n> \"type-python\" and \"type=python\" seem redundant but they are not.\n> \"type-python\" is needed so that it can be used as a macro.\n> \"type=python\" makes it easier to inquire about the type of a file using\n> something like \"git check-attr type -- PATH\" rather than having to\n> inquire about each possible type-* attribute. It might be nice to\n> support a slightly extended macro definition syntax like\n> \n> [attr]type=python text diff=python check-ws\n> *.py type=python\n> \n> [attr]type=makefile text diff check-ws -check-tab\n> Makefile.* type=makefile\n> \n> (i.e., macros that are only triggered for particular values of an\n> attribute).\n\nI don't think there's any semantic reason why that is not workable. It's\nsimply not syntactically allowed at this point.\n\n-Peff\n"},{"id":"183461","messageId":"7vhazb3rtm.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"20120125214625.GA4666@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-01T08:01:41Z","receivedAt":"2012-02-01T08:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jan 23, 2012 at 10:59:45PM -0800, Junio C Hamano wrote:\n>\n>> Conrad Irwin writes:\n>> > I used to use this approach, hooking into the \"diff\" attribute directly to mark\n>> > a file as binary, however that was clearly a hack.\n>> \n>> After thinking about this a bit more, I have to say I disagree that it is\n>> a hack.\n>\n> I kind of agree.\n>\n> The biggest problem is that the name is wrong. The \"diff.*.command\"\n> option really is about generating a diff between two blobs of a certain\n> type. But \"diff.*.textconv\" and \"diff.*.binary\" are really just\n> attributes of the file, and may or may not have to do with generating a\n> diff. Ditto for diff.*.funcname, I think.\n>\n> You argue, and I agree, that if we are talking about attributes of the\n> files and not diff-specific things, then other parts of git can and\n> should make use of that information.\n>\n> So if this was all spelled:\n>\n> $ cat .gitattributes\n> *.pdf filetype=pdf\n> $ cat .git/config\n> [filetype \"pdf\"]\n> binary = true\n> textconv = pdf2txt\n>\n> I think it would be a no-brainer that those type attributes should apply\n> to \"git grep\".\n\nI think this discussion has, instead of forking into two equally\ninteresting subthreads, veered to a more intellectually stimulating\ntangent and we ended up losing focus.\n\nRegardless of what to do with \"I do not want to grep in these types of\nfiles\" and \"I want textconv applied when grepping in these types\", which\nwould be new attributes to implement two new features, I would like to see\nus first concentrate on fixing the \"binary\" issue. When somebody tells us\n\"Your autodetection may screw it up, but this file is binary; just show\n'Binary files differ.' when comparing.\" with \"-diff\" (or \"binary\"), we\nshould honor that when \"git grep\" decides if it should take the 'Binary\nfile matches' codepath. We currently do not, and it clearly is a bug.\n\nThis is especially made somewhat urgent because I do not want a half-baked\n\"two pathspecs\" approach that only \"git grep\" knows about when we add the\nsupport for \"git grep --exclude-path=...\".\n\nWe should have to teach the underlying machinery that matches pathspec\nabout negative pathspec entries only once. After we have done so, all the\ncallers, not just \"git grep\", should be able to take advantage of the\nchange by just learning to place negative pathspec entries in the \"struct\npathspec\" they pass to the machinery. Doing anything else will lead to\nmadness of adding ad-hoc \"here we should further filter with the other\nnegative 'struct pathspec'\" in each and every application.\n\nBut I suspect that it would not materialize anytime soon. And I also\nsuspect that the correct handling of 'Binary file matches', which is a\npure bugfix, should solve the original issue started these threads 90% in\npractice.\n"},{"id":"183462","messageId":"20120201082005.GA32348@sigill.intra.peff.net","threadId":"29381","inReplyTo":"7vhazb3rtm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-01T08:20:05Z","receivedAt":"2012-02-01T08:20:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2012 at 12:01:41AM -0800, Junio C Hamano wrote:\n\n> > So if this was all spelled:\n> >\n> > $ cat .gitattributes\n> > *.pdf filetype=pdf\n> > $ cat .git/config\n> > [filetype \"pdf\"]\n> > binary = true\n> > textconv = pdf2txt\n> >\n> > I think it would be a no-brainer that those type attributes should apply\n> > to \"git grep\".\n> \n> I think this discussion has, instead of forking into two equally\n> interesting subthreads, veered to a more intellectually stimulating\n> tangent and we ended up losing focus.\n\nThat's what I'm here for.\n\n> Regardless of what to do with \"I do not want to grep in these types of\n> files\" and \"I want textconv applied when grepping in these types\", which\n> would be new attributes to implement two new features, I would like to see\n> us first concentrate on fixing the \"binary\" issue. When somebody tells us\n> \"Your autodetection may screw it up, but this file is binary; just show\n> 'Binary files differ.' when comparing.\" with \"-diff\" (or \"binary\"), we\n> should honor that when \"git grep\" decides if it should take the 'Binary\n> file matches' codepath. We currently do not, and it clearly is a bug.\n\nRight. It may have been lost in the verbosity of what I wrote in my\nprevious email, but I completely agree. With the caveat that one should\nalso respect \"diff=foo\" coupled with \"diff.foo.binary = true\" as making\nsomething binary. But that is already handled transparently by the\nuserdiff.[ch] code (which seems like the obvious entry point for\ngrep to use for attribute lookup, and which we already use there for\nfuncname lookup).\n\nThe trivial-ish patch is below.\n\n> We should have to teach the underlying machinery that matches pathspec\n> about negative pathspec entries only once. After we have done so, all the\n> callers, not just \"git grep\", should be able to take advantage of the\n> change by just learning to place negative pathspec entries in the \"struct\n> pathspec\" they pass to the machinery. Doing anything else will lead to\n> madness of adding ad-hoc \"here we should further filter with the other\n> negative 'struct pathspec'\" in each and every application.\n\nYes, I agree.\n\n> But I suspect that it would not materialize anytime soon. And I also\n> suspect that the correct handling of 'Binary file matches', which is a\n> pure bugfix, should solve the original issue started these threads 90% in\n> practice.\n\nAlso agree. Let's fix the bug and then give it some time to see whether\npeople really want more explicit exclusions.\n\nHere's the bug-fix patch. Not quite ready for inclusion, as it obviously\nneeds tests and a commit message. Also, we can cache the result of the\nuserdiff lookup so the funcname code doesn't have to look it up again.\n\n---\ndiff --git a/grep.c b/grep.c\nindex b29d09c..d7ab054 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -960,6 +960,15 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\n }\n \n+static int grep_buffer_is_binary(const char *path,\n+\t\t\t\t char *buf, unsigned long size)\n+{\n+\tstruct userdiff_driver *drv = userdiff_find_by_path(path);\n+\tif (drv && drv->binary != -1)\n+\t\treturn drv->binary;\n+\treturn buffer_is_binary(buf, size);\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@@ -994,11 +1003,11 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (grep_buffer_is_binary(name, buf, size))\n \t\t\tbinary_match_only = 1;\n \t\tbreak;\n \tcase GREP_BINARY_NOMATCH:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (grep_buffer_is_binary(name, buf, size))\n \t\t\treturn 0; /* Assume unmatch */\n \t\tbreak;\n \tcase GREP_BINARY_TEXT:\n"},{"id":"183463","messageId":"20120201091009.GA20984@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120201082005.GA32348@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-01T09:10:09Z","receivedAt":"2012-02-01T09:10:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2012 at 03:20:05AM -0500, Jeff King wrote:\n\n> Here's the bug-fix patch. Not quite ready for inclusion, as it obviously\n> needs tests and a commit message. Also, we can cache the result of the\n> userdiff lookup so the funcname code doesn't have to look it up again.\n\nActually, it's a little bit more complicated. I was looking at a\nslightly old version of grep.c. Since 0579f91 (grep: enable threading\nwith -p and -W using lazy attribute lookup, 2011-12-12), the lookup\nhappens in lots of sub-functions, and locking is required.\n\nSo this is what the patch looks like with proper locking and caching of\nthe looked-up driver. It's quite messy because the cached driver pointer\nhas to get passed around quite a bit. And I'm not sure it buys that much\nin practice. The cost of attribute lookup _is_ noticeable (which I'll\ndiscuss below), but funcname lookup only happens when we get a grep hit.\nSo unless you are searching for something extremely common, you're only\ngoing to do a lookup very occasionally (compared to the load of actually\nsearching through the files). So all of the messiness and caching may\nnot be worth the effort, as I wasn't able to measure a performance gain.\n\nBut there's more. Respecting binary attributes does mean looking up\nattributes for _every_ file. And that has a noticeable impact. My\nbest-of-five for \"git grep foo\" on linux-2.6 went from 0.302s to 0.392s.\nYuck.\n\nPart of the problem, I suspect, is that the attribute lookup code is\noptimized for locality. We only unwind as much of the stack as we need,\nso looking at \"foo/bar/baz.c\" after \"foo/bar/bleep.c\" is much cheaper\nthan looking at \"some/other/directory.c\". But with threaded grep, that\nlocality is likely lost, as we are mixing up attribute requests from\ndifferent threads.\n\nGiven that binary lookup means we need every file's gitattribute, it\nmight be better to look them up serially at the beginning of the\nprogram, and then pass the resulting userdiff driver to grep_buffer\nalong with each path.\n\n---\ndiff --git a/grep.c b/grep.c\nindex 486230b..3ca840a 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -829,15 +829,28 @@ static inline void grep_attr_unlock(struct grep_opt *opt)\n #define grep_attr_unlock(opt)\n #endif\n \n-static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n+static struct userdiff_driver *get_cached_userdiff(struct grep_opt *opt,\n+\t\t\t\t\t\t const char *path,\n+\t\t\t\t\t\t struct userdiff_driver **drv)\n {\n-\txdemitconf_t *xecfg = opt->priv;\n-\tif (xecfg && !xecfg->find_func) {\n-\t\tstruct userdiff_driver *drv;\n+\tif (!*drv) {\n \t\tgrep_attr_lock(opt);\n-\t\tdrv = userdiff_find_by_path(name);\n+\t\t*drv = userdiff_find_by_path(path);\n+\t\tif (!*drv)\n+\t\t\t*drv = userdiff_find_by_name(\"default\");\n \t\tgrep_attr_unlock(opt);\n-\t\tif (drv && drv->funcname.pattern) {\n+\t}\n+\treturn *drv;\n+}\n+\n+static int match_funcname(struct grep_opt *opt, const char *name,\n+\t\t\t struct userdiff_driver **drv_p,\n+\t\t\t char *bol, char *eol)\n+{\n+\txdemitconf_t *xecfg = opt->priv;\n+\tif (xecfg && !xecfg->find_func) {\n+\t\tstruct userdiff_driver *drv = get_cached_userdiff(opt, name, drv_p);\n+\t\tif (drv->funcname.pattern) {\n \t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n \t\t\txdiff_set_find_func(xecfg, pe->pattern, pe->cflags);\n \t\t} else {\n@@ -859,6 +872,7 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha\n }\n \n static void show_funcname_line(struct grep_opt *opt, const char *name,\n+\t\t\t struct userdiff_driver **drv_p,\n \t\t\t char *buf, char *bol, unsigned lno)\n {\n \twhile (bol > buf) {\n@@ -871,20 +885,21 @@ static void show_funcname_line(struct grep_opt *opt, const char *name,\n \t\tif (lno <= opt->last_shown)\n \t\t\tbreak;\n \n-\t\tif (match_funcname(opt, name, bol, eol)) {\n+\t\tif (match_funcname(opt, name, drv_p, bol, eol)) {\n \t\t\tshow_line(opt, bol, eol, name, lno, '=');\n \t\t\tbreak;\n \t\t}\n \t}\n }\n \n-static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n-\t\t\t char *bol, char *end, unsigned lno)\n+static void show_pre_context(struct grep_opt *opt, const char *name,\n+\t\t\t struct userdiff_driver **drv_p,\n+\t\t\t char *buf, char *bol, char *end, unsigned lno)\n {\n \tunsigned cur = lno, from = 1, funcname_lno = 0;\n \tint funcname_needed = !!opt->funcname;\n \n-\tif (opt->funcbody && !match_funcname(opt, name, bol, end))\n+\tif (opt->funcbody && !match_funcname(opt, name, drv_p, bol, end))\n \t\tfuncname_needed = 2;\n \n \tif (opt->pre_context < lno)\n@@ -900,7 +915,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \t\twhile (bol > buf && bol[-1] != '\\n')\n \t\t\tbol--;\n \t\tcur--;\n-\t\tif (funcname_needed && match_funcname(opt, name, bol, eol)) {\n+\t\tif (funcname_needed && match_funcname(opt, name, drv_p, bol, eol)) {\n \t\t\tfuncname_lno = cur;\n \t\t\tfuncname_needed = 0;\n \t\t}\n@@ -908,7 +923,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \n \t/* We need to look even further back to find a function signature. */\n \tif (opt->funcname && funcname_needed)\n-\t\tshow_funcname_line(opt, name, buf, bol, cur);\n+\t\tshow_funcname_line(opt, name, drv_p, buf, bol, cur);\n \n \t/* Back forward. */\n \twhile (cur < lno) {\n@@ -983,6 +998,17 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\n }\n \n+static int grep_buffer_is_binary(struct grep_opt *opt,\n+\t\t\t\t const char *path,\n+\t\t\t\t char *buf, unsigned long size,\n+\t\t\t\t struct userdiff_driver **drv_p)\n+{\n+\tstruct userdiff_driver *drv = get_cached_userdiff(opt, path, drv_p);\n+\tif (drv && drv->binary != -1)\n+\t\treturn drv->binary;\n+\treturn buffer_is_binary(buf, size);\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@@ -996,6 +1022,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \tint show_function = 0;\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n+\tstruct userdiff_driver *drv = NULL;\n \n \tif (!opt->output)\n \t\topt->output = std_output;\n@@ -1017,11 +1044,11 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (grep_buffer_is_binary(opt, name, buf, size, &drv))\n \t\t\tbinary_match_only = 1;\n \t\tbreak;\n \tcase GREP_BINARY_NOMATCH:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (grep_buffer_is_binary(opt, name, buf, size, &drv))\n \t\t\treturn 0; /* Assume unmatch */\n \t\tbreak;\n \tcase GREP_BINARY_TEXT:\n@@ -1099,16 +1126,16 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\t\t * pre-context lines, we would need to show them.\n \t\t\t */\n \t\t\tif (opt->pre_context || opt->funcbody)\n-\t\t\t\tshow_pre_context(opt, name, buf, bol, eol, lno);\n+\t\t\t\tshow_pre_context(opt, name, &drv, buf, bol, eol, lno);\n \t\t\telse if (opt->funcname)\n-\t\t\t\tshow_funcname_line(opt, name, buf, bol, lno);\n+\t\t\t\tshow_funcname_line(opt, name, &drv, buf, bol, lno);\n \t\t\tshow_line(opt, bol, eol, name, lno, ':');\n \t\t\tlast_hit = lno;\n \t\t\tif (opt->funcbody)\n \t\t\t\tshow_function = 1;\n \t\t\tgoto next_line;\n \t\t}\n-\t\tif (show_function && match_funcname(opt, name, bol, eol))\n+\t\tif (show_function && match_funcname(opt, name, &drv, bol, eol))\n \t\t\tshow_function = 0;\n \t\tif (show_function ||\n \t\t (last_hit && lno <= last_hit + opt->post_context)) {\n"},{"id":"183464","messageId":"CAOTq_ptj06aNGsQRjV0fVRxnQFBHmU2FFSXwWDUUk9MM77k2LQ@mail.gmail.com","threadId":"29381","inReplyTo":"20120201091009.GA20984@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2012-02-01T09:28:47Z","receivedAt":"2012-02-01T09:28:47Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"On Wed, Feb 1, 2012 at 1:10 AM, Jeff King wrote:\n> On Wed, Feb 01, 2012 at 03:20:05AM -0500, Jeff King wrote:\n>\n> Actually, it's a little bit more complicated. I was looking at a\n> slightly old version of grep.c. Since 0579f91 (grep: enable threading\n> with -p and -W using lazy attribute lookup, 2011-12-12), the lookup\n> happens in lots of sub-functions, and locking is required.\n\nHeh, you just beat me to it.\n\n> But there's more. Respecting binary attributes does mean looking up\n> attributes for _every_ file. And that has a noticeable impact. My\n> best-of-five for \"git grep foo\" on linux-2.6 went from 0.302s to 0.392s.\n> Yuck.\n\nThe first time I introduced this behaviour[1], I made it conditional\non a preference — those who wanted \"good\" grep could set the\npreference, while those who wanted \"fast\" grep could not. I think\nthat's not a good idea, though if the performance issues are\nshow-stoppers, I'd suggest the opposite preference (so speed-freaks\ncan disable the checks).\n\nTests from [1] included below in case they're still useful (they pass\nwith your change)\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/179299/match=grep\n---\n\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 917a264..4d94461 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -99,4 +99,23 @@ test_expect_success 'git grep yx a' \"\n test_must_fail git grep -f f a\n \"\n\n+test_expect_success 'git -c grep.binaryFiles=1 grep ina a' \"\n+ echo 'a diff' > .gitattributes &&\n+ printf 'binaryQfile' | q_to_nul >a &&\n+ echo 'a:binaryQfile' | q_to_nul >expect &&\n+ git -c grep.binaryFiles=1 grep ina a > actual &&\n+ rm .gitattributes &&\n+ test_cmp expect actual\n+\"\n+test_expect_success 'git -c grep.binaryFiles=1 grep tex t' \"\n+ echo 'text' > t &&\n+ git add t &&\n+ echo 't -diff' > .gitattributes &&\n+ echo Binary file t matches >expect &&\n+ git -c grep.binaryFiles=1 grep tex t >actual &&\n+ rm .gitattributes &&\n+ test_cmp expect actual\n+\"\n+\n+\n test_done\n"},{"id":"183481","messageId":"7vd39y4iwx.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"20120201091009.GA20984@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-01T16:28:46Z","receivedAt":"2012-02-01T16:28:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Part of the problem, I suspect, is that the attribute lookup code is\n> optimized for locality. We only unwind as much of the stack as we need,\n> so looking at \"foo/bar/baz.c\" after \"foo/bar/bleep.c\" is much cheaper\n> than looking at \"some/other/directory.c\". But with threaded grep, that\n> locality is likely lost, as we are mixing up attribute requests from\n> different threads.\n>\n> Given that binary lookup means we need every file's gitattribute, it\n> might be better to look them up serially at the beginning of the\n> program, and then pass the resulting userdiff driver to grep_buffer\n> along with each path.\n\nYeah, that was my impression when the performance of threaded grep was\ndiscussed, which was before this \"let's honor binary attribute\".\n"},{"id":"183505","messageId":"20120201221437.GA19044@sigill.intra.peff.net","threadId":"29381","inReplyTo":"CAOTq_ptj06aNGsQRjV0fVRxnQFBHmU2FFSXwWDUUk9MM77k2LQ@mail.gmail.com","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-01T22:14:37Z","receivedAt":"2012-02-01T22:14:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2012 at 01:28:47AM -0800, Conrad Irwin wrote:\n\n> > But there's more. Respecting binary attributes does mean looking up\n> > attributes for _every_ file. And that has a noticeable impact. My\n> > best-of-five for \"git grep foo\" on linux-2.6 went from 0.302s to 0.392s.\n> > Yuck.\n> \n> The first time I introduced this behaviour[1], I made it conditional\n> on a preference — those who wanted \"good\" grep could set the\n> preference, while those who wanted \"fast\" grep could not. I think\n> that's not a good idea, though if the performance issues are\n> show-stoppers, I'd suggest the opposite preference (so speed-freaks\n> can disable the checks).\n\nI've been able to get somewhat better performance by hoisting the\nattribute lookup into the parent thread. That means it happens in order\n(which lets the attr code's stack optimizations work), and there's no\nlock contention.\n\nI'll post finished patches with numbers in a few minutes.\n\n> Tests from [1] included below in case they're still useful (they pass\n> with your change)\n\nThanks, I'll include them.\n\n-Peff\n"},{"id":"183512","messageId":"20120201232027.GA32119@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120201221437.GA19044@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-01T23:20:27Z","receivedAt":"2012-02-01T23:20:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2012 at 05:14:37PM -0500, Jeff King wrote:\n\n> > The first time I introduced this behaviour[1], I made it conditional\n> > on a preference — those who wanted \"good\" grep could set the\n> > preference, while those who wanted \"fast\" grep could not. I think\n> > that's not a good idea, though if the performance issues are\n> > show-stoppers, I'd suggest the opposite preference (so speed-freaks\n> > can disable the checks).\n> \n> I've been able to get somewhat better performance by hoisting the\n> attribute lookup into the parent thread. That means it happens in order\n> (which lets the attr code's stack optimizations work), and there's no\n> lock contention.\n> \n> I'll post finished patches with numbers in a few minutes.\n\nOK, here they are. After playing with some options, I'm satisfied this\nis a sane way to do it. I don't think it's worth having a config option.\nThere is a measurable slowdown, but it's simply not that big.\n\n [1/2]: grep: let grep_buffer callers specify a binary flag\n [2/2]: grep: respect diff attributes for binary-ness\n\nThere are a few optimizations I didn't do that you could put on top:\n\n 1. When \"-a\" is given, we can avoid the attribute lookup altogether.\n\n 2. When \"-I\" is given, we can actually check attributes _before_\n loading the file or blob into memory. This can help with very large\n binaries.\n\n 3. When \"-I\" is given but we have no attribute, we can stream the\n beginning of the file or blob to check for binary-ness, and then\n avoid loading the whole thing if it turns out to be binary.\n\nI think (1) and (2) should be easy. Doing (3) is a little messier,\nbecause binary detection happens inside grep_buffer, but we can hoist it\nout. However, for large files, it might be nice to have a streaming grep\ninterface anyway, and (3) could be part of that.\n\n-Peff\n"},{"id":"183513","messageId":"20120201232109.GA2652@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120201221437.GA19044@sigill.intra.peff.net","subject":"[PATCH 1/2] grep: let grep_buffer callers specify a binary flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-01T23:21:09Z","receivedAt":"2012-02-01T23:21:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The caller of grep_buffer may have extra information about\nwhether a buffer is binary or not (e.g., from configuration).\nLet's give them a chance to pass along that information and\noverride our binary auto-detection.\n\nCallers can still pass \"-1\" to get the regular\nauto-detection (and all callers are converted to do this,\nmeaning there should be no behavior change yet).\n\nWe could maintain source compatibility for callers by adding\na new \"grep_buffer_with_flags\" and leaving \"grep_buffer\" as\na wrapper that always passes \"-1\". But there are only 5\ncallers of grep_buffer, and only 1 of those (grepping commit\nbuffers) will not be converted to pass something useful in\nthe next patch. So it's simpler to just add a \"-1\" there.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 8 ++++----\n grep.c | 23 ++++++++++++++++-------\n grep.h | 2 +-\n revision.c | 1 +\n 4 files changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..e328316 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -221,14 +221,14 @@ static void *run(void *arg)\n \t\t\tvoid* data = load_sha1(w->identifier, &sz, w->name);\n \n \t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, data, sz);\n+\t\t\t\thit |= grep_buffer(opt, w->name, -1, data, sz);\n \t\t\t\tfree(data);\n \t\t\t}\n \t\t} else if (w->type == WORK_FILE) {\n \t\t\tsize_t sz;\n \t\t\tvoid* data = load_file(w->identifier, &sz);\n \t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, data, sz);\n+\t\t\t\thit |= grep_buffer(opt, w->name, -1, data, sz);\n \t\t\t\tfree(data);\n \t\t\t}\n \t\t} else {\n@@ -421,7 +421,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tif (!data)\n \t\t\thit = 0;\n \t\telse\n-\t\t\thit = grep_buffer(opt, name, data, sz);\n+\t\t\thit = grep_buffer(opt, name, -1, data, sz);\n \n \t\tfree(data);\n \t\tfree(name);\n@@ -483,7 +483,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tif (!data)\n \t\t\thit = 0;\n \t\telse\n-\t\t\thit = grep_buffer(opt, name, data, sz);\n+\t\t\thit = grep_buffer(opt, name, -1, data, sz);\n \n \t\tfree(data);\n \t\tfree(name);\ndiff --git a/grep.c b/grep.c\nindex 486230b..e547db2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -983,8 +983,16 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\n }\n \n+static int grep_buffer_is_binary(char *buf, unsigned long size, int flag)\n+{\n+\tif (flag == -1)\n+\t\tflag = buffer_is_binary(buf, size);\n+\treturn flag;\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+\t\t\t int is_binary, char *buf, unsigned long size,\n+\t\t\t int collect_hits)\n {\n \tchar *bol = buf;\n \tunsigned long left = size;\n@@ -1017,11 +1025,11 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (grep_buffer_is_binary(buf, size, is_binary))\n \t\t\tbinary_match_only = 1;\n \t\tbreak;\n \tcase GREP_BINARY_NOMATCH:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (grep_buffer_is_binary(buf, size, is_binary))\n \t\t\treturn 0; /* Assume unmatch */\n \t\tbreak;\n \tcase GREP_BINARY_TEXT:\n@@ -1182,23 +1190,24 @@ static int chk_hit_marker(struct grep_expr *x)\n \t}\n }\n \n-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)\n+int grep_buffer(struct grep_opt *opt, const char *name, int is_binary,\n+\t\tchar *buf, unsigned long size)\n {\n \t/*\n \t * we do not have to do the two-pass grep when we do not check\n \t * buffer-wide \"all-match\".\n \t */\n \tif (!opt->all_match)\n-\t\treturn grep_buffer_1(opt, name, buf, size, 0);\n+\t\treturn grep_buffer_1(opt, name, is_binary, buf, size, 0);\n \n \t/* Otherwise the toplevel \"or\" terms hit a bit differently.\n \t * We first clear hit markers from them.\n \t */\n \tclr_hit_marker(opt->pattern_expression);\n-\tgrep_buffer_1(opt, name, buf, size, 1);\n+\tgrep_buffer_1(opt, name, is_binary, buf, size, 1);\n \n \tif (!chk_hit_marker(opt->pattern_expression))\n \t\treturn 0;\n \n-\treturn grep_buffer_1(opt, name, buf, size, 0);\n+\treturn grep_buffer_1(opt, name, is_binary, buf, size, 0);\n }\ndiff --git a/grep.h b/grep.h\nindex fb205f3..8447e4c 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -128,7 +128,7 @@ extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const cha\n extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);\n extern void compile_grep_patterns(struct grep_opt *opt);\n extern void free_grep_patterns(struct grep_opt *opt);\n-extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);\n+extern int grep_buffer(struct grep_opt *opt, const char *name, int is_binary, char *buf, unsigned long size);\n \n extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n extern int grep_threads_ok(const struct grep_opt *opt);\ndiff --git a/revision.c b/revision.c\nindex c97d834..3dcd968 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2150,6 +2150,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\treturn 1;\n \treturn grep_buffer(&opt->grep_filter,\n \t\t\t NULL, /* we say nothing, not even filename */\n+\t\t\t -1,\n \t\t\t commit->buffer, strlen(commit->buffer));\n }\n \n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183514","messageId":"20120201232129.GB2652@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120201221437.GA19044@sigill.intra.peff.net","subject":"[PATCH 2/2] grep: respect diff attributes for binary-ness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-01T23:21:29Z","receivedAt":"2012-02-01T23:21:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is currently no way for users to tell git-grep that a\nparticular path is or is not a binary file; instead, grep\nalways relies on its auto-detection (or the user specifying\n\"-a\" to treat all binary-looking files like text).\n\nThis patch teaches git-grep to use the same attribute lookup\nthat is used by git-diff. We could add a new \"grep\" flag,\nbut that is unnecessarily complex and unlikely to be useful.\nDespite the name, the \"-diff\" attribute (or \"diff=foo\" and\nthe associated diff.foo.binary config option) are really\nabout describing the contents of the path. It's simply\nhistorical that diff was the only thing that cared about\nthese attributes in the past.\n\nAnd if this simple approach turns out to be insufficient, we\nstill have a backwards-compatible path forward: we can add a\nseparate \"grep\" attribute, and fall back to respecting\n\"diff\" if it is unset.\n\nThere are a few things worth nothing about the\nimplementation.\n\nOne is that the attribute lookup happens outside of the\ngrep.[ch] interface (i.e., outside of grep_buffer). We could\ndo it at a lower level, which would be slightly more\nconvenient for callers. However, this interacts badly with\nthreading. The attribute-lookup code performs best when\nlookup order matches the filesystem order (so looking up\n\"a/b/c\" is cheaper if we just looked up \"a/b/d\" than if we\njust did \"x/y/z\"). Because we issue many simultaneous\nrequests to grep_buffer, performing the attribute lookup at\nthat level will cause requests from unrelated paths to\ninterleave, and we lose the locality that makes the lookup\noptimization work.\n\nInstead, in the threaded case we check the attributes as\nthey are added to the work queue, meaning they are looked up\nin the optimal order (in the single threaded case, this is a\nnon-issue, as we process the files serially in the optimal\norder).\n\nHere are a few numbers showing the difference. The first is\na best-of-five time for \"git grep foo\" on the linux-2.6 repo\nbefore this patch (on a 4-core HT processor, using 8\nthreads):\n\n real 0m0.306s\n user 0m1.512s\n sys 0m0.412s\n\nNow here's the time for the same operation with a trial\nimplementation looking up attributes in grep_buffer,\nshowing a 31% slowdown:\n\n real 0m0.401s\n user 0m1.760s\n sys 0m0.636s\n\nAnd here's the same operation with this patch, with only an\n11% slowdown:\n\n real 0m0.339s\n user 0m1.444s\n sys 0m0.584s\n\nNote that while the percentages are big, the absolute\nnumbers are pretty small. In particular, this is a very\ninexpensive grep to do. A more complex regex should have the\nsame absolute slowdown from the attribute lookup, but it\nwould be a much smaller percentage of the total processing\ntime. So doing it this way is not a huge win, but it does\nhelp on small greps.\n\nThe second issue worth noting is that while we do a full\nattribute lookup, we pass along only the binary flag to\ngrep_buffer. When the \"-p\" flag is given to grep, we will\nactually look up the same attributes to find funcname\npatterns of matches. We could pass along the pointer to the\nuserdiff driver for reuse.\n\nHowever, it's not worth doing this. It clutters the code, as\nthe driver has to be passed through a large number of helper\nfunctions (and pollutes the grep_buffer interface with\nuserdiff code). And in my tests, it didn't actually improve\nperformance. Because we only have to look up the attribute\nfor a grep hit, in most cases we will only do the funcname\nlookup for a small subset of files. The cost of the extra\nlookups turns out to be negligible.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 26 ++++++++++++++++++++++----\n t/t7008-grep-binary.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e328316..bb38804 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -42,6 +42,7 @@ enum work_type {WORK_SHA1, WORK_FILE};\n struct work_item {\n \tenum work_type type;\n \tchar *name;\n+\tint is_binary;\n \n \t/* if type == WORK_SHA1, then 'identifier' is a SHA1,\n \t * otherwise type == WORK_FILE, and 'identifier' is a NUL\n@@ -113,6 +114,14 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n+static int path_is_binary(const char *path)\n+{\n+\tstruct userdiff_driver *drv = userdiff_find_by_path(path);\n+\tif (drv)\n+\t\treturn drv->binary;\n+\treturn -1;\n+}\n+\n static void add_work(enum work_type type, char *name, void *id)\n {\n \tgrep_lock();\n@@ -123,6 +132,11 @@ static void add_work(enum work_type type, char *name, void *id)\n \n \ttodo[todo_end].type = type;\n \ttodo[todo_end].name = name;\n+\n+\tpthread_mutex_lock(&grep_attr_mutex);\n+\ttodo[todo_end].is_binary = path_is_binary(name);\n+\tpthread_mutex_unlock(&grep_attr_mutex);\n+\n \ttodo[todo_end].identifier = id;\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n@@ -221,14 +235,16 @@ static void *run(void *arg)\n \t\t\tvoid* data = load_sha1(w->identifier, &sz, w->name);\n \n \t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, -1, data, sz);\n+\t\t\t\thit |= grep_buffer(opt, w->name, w->is_binary,\n+\t\t\t\t\t\t data, sz);\n \t\t\t\tfree(data);\n \t\t\t}\n \t\t} else if (w->type == WORK_FILE) {\n \t\t\tsize_t sz;\n \t\t\tvoid* data = load_file(w->identifier, &sz);\n \t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, -1, data, sz);\n+\t\t\t\thit |= grep_buffer(opt, w->name, w->is_binary,\n+\t\t\t\t\t\t data, sz);\n \t\t\t\tfree(data);\n \t\t\t}\n \t\t} else {\n@@ -421,7 +437,8 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tif (!data)\n \t\t\thit = 0;\n \t\telse\n-\t\t\thit = grep_buffer(opt, name, -1, data, sz);\n+\t\t\thit = grep_buffer(opt, name, path_is_binary(name),\n+\t\t\t\t\t data, sz);\n \n \t\tfree(data);\n \t\tfree(name);\n@@ -483,7 +500,8 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tif (!data)\n \t\t\thit = 0;\n \t\telse\n-\t\t\thit = grep_buffer(opt, name, -1, data, sz);\n+\t\t\thit = grep_buffer(opt, name, path_is_binary(name),\n+\t\t\t\t\t data, sz);\n \n \t\tfree(data);\n \t\tfree(name);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 917a264..fd6410f 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -99,4 +99,28 @@ test_expect_success 'git grep yx a' \"\n \ttest_must_fail git grep -f f a\n \"\n \n+test_expect_success 'grep respects binary diff attribute' '\n+\techo text >t &&\n+\tgit add t &&\n+\techo t:text >expect &&\n+\tgit grep text t >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"t -diff\" >.gitattributes &&\n+\techo \"Binary file t matches\" >expect &&\n+\tgit grep text t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep respects not-binary diff attribute' '\n+\techo binQary | q_to_nul >b &&\n+\tgit add b &&\n+\techo \"Binary file b matches\" >expect &&\n+\tgit grep bin b >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"b diff\" >.gitattributes &&\n+\techo \"b:binQary\" >expect &&\n+\tgit grep bin b | nul_to_q >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183524","messageId":"7vhaza12ol.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"20120201232109.GA2652@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] grep: let grep_buffer callers specify a binary flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T00:47:38Z","receivedAt":"2012-02-02T00:47:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The caller of grep_buffer may have extra information about\n> whether a buffer is binary or not (e.g., from configuration).\n> Let's give them a chance to pass along that information and\n> override our binary auto-detection.\n\nHrm, I would have expected a patch that turns \"const char *name\" into a\nstructure that has name and drv as its members, so that later we can tell\nthe function more about the nature of the contents. Or a separate pointer\nto drv in place of your \"binary\" flag word.\n\nI am not saying that your patch is wrong. It was just somewhat unexpected\nthat \"binary\" is the only additional thing we want to tell the function.\n"},{"id":"183525","messageId":"20120202005209.GA6883@sigill.intra.peff.net","threadId":"29381","inReplyTo":"7vhaza12ol.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] grep: let grep_buffer callers specify a binary flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T00:52:09Z","receivedAt":"2012-02-02T00:52:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2012 at 04:47:38PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The caller of grep_buffer may have extra information about\n> > whether a buffer is binary or not (e.g., from configuration).\n> > Let's give them a chance to pass along that information and\n> > override our binary auto-detection.\n> \n> Hrm, I would have expected a patch that turns \"const char *name\" into a\n> structure that has name and drv as its members, so that later we can tell\n> the function more about the nature of the contents. Or a separate pointer\n> to drv in place of your \"binary\" flag word.\n\nHmm. Yeah, I would be OK with that, as it's really encapsulating the\nidea of \"stuff we want to tell grep about this buffer\".\n\nWhat I really didn't want to do was pass the userdiff driver directly,\nas that feels way too much like an implementation detail (and while it\ncan be used to avoid further lookups, it doesn't seem to make a\ndifference in practice -- see the following patch). And passing it\naround became a big messy chore.\n\nBut if it were a \"struct grep_context\" that encapsulated those things, I\nthink it would be much nicer (it could even carry a pointer to \"struct\ngrep_opt\" in it, making the code _more_ pleasant to read, not less).\n\nI'll take a look at re-working it that way.\n\n-Peff\n"},{"id":"183535","messageId":"7vmx92yos8.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"20120201232027.GA32119@sigill.intra.peff.net","subject":"Re: [PATCH] Don't search files with an unset \"grep\" attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T02:03:51Z","receivedAt":"2012-02-02T02:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There are a few optimizations I didn't do that you could put on top:\n>\n> 1. When \"-a\" is given, we can avoid the attribute lookup altogether.\n\nCorrect.\n\n> 2. When \"-I\" is given, we can actually check attributes _before_\n> loading the file or blob into memory. This can help with very large\n> binaries.\n\nNice.\n\n> ... However, for large files, it might be nice to have a streaming grep\n> interface anyway, and (3) could be part of that.\n\nEven nicer ;-)\n"},{"id":"183551","messageId":"20120202081747.GA10271@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202005209.GA6883@sigill.intra.peff.net","subject":"[PATCH 0/9] respect binary attribute in grep","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:17:47Z","receivedAt":"2012-02-02T08:17:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Thomas, as I am mangling some of his recent work with my\n refactoring]\n\nOn Wed, Feb 01, 2012 at 07:52:09PM -0500, Jeff King wrote:\n\n> > Hrm, I would have expected a patch that turns \"const char *name\" into a\n> > structure that has name and drv as its members, so that later we can tell\n> > the function more about the nature of the contents. Or a separate pointer\n> > to drv in place of your \"binary\" flag word.\n> [...]\n> I'll take a look at re-working it that way.\n\nThanks for a dose of sanity. The result turned out much easier to read\n(and explain in the commit messages, as it was simple to break into\nsmaller commits). In particular, the \"don't read binary-marked files at\nall with -I\" optimization became very natural.\n\nI implemented all of the other optimizations I mentioned except the\n\"only stream the first few bytes when auto-detecting binary-ness\" one.\nHowever, it should be easy to do on top of these changes. I need to\nre-visit the similar change to diff_filespec_is_binary, and I'll do both\nat the same time.\n\nThe patches are:\n\n [1/9]: grep: make locking flag global\n [2/9]: grep: move sha1-reading mutex into low-level code\n [3/9]: grep: refactor the concept of \"grep source\" into an object\n [4/9]: convert git-grep to use grep_source interface\n [5/9]: grep: drop grep_buffer's \"name\" parameter\n [6/9]: grep: cache userdiff_driver in grep_source\n\n These are all refactoring that should have no behavior change.\n\n [7/9]: grep: respect diff attributes for binary-ness\n\n This is the point of the series. :)\n\n [8/9]: grep: load file data after checking binary-ness\n [9/9]: grep: pre-load userdiff drivers when threaded\n\n And these two are simple optimizations.\n\n-Peff\n"},{"id":"183552","messageId":"20120202081829.GA6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 1/9] grep: make locking flag global","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:18:29Z","receivedAt":"2012-02-02T08:18:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The low-level grep code traditionally didn't care about\nthreading, as it doesn't do any threading itself and didn't\ncall out to other non-thread-safe code. That changed with\n0579f91 (grep: enable threading with -p and -W using lazy\nattribute lookup, 2011-12-12), which pushed the lookup of\nfuncname attributes (which is not thread-safe) into the\nlow-level grep code.\n\nAs a result, the low-level code learned about a new global\n\"grep_attr_mutex\" to serialize access to the attribute code.\nA multi-threaded caller (e.g., builtin/grep.c) is expected\nto initialize the mutex and set \"use_threads\" in the\ngrep_opt structure. The low-level code only uses the lock if\nuse_threads is set.\n\nHowever, putting the use_threads flag into the grep_opt\nstruct is not the most logical place. Whether threading is\nin use is not something that matters for each call to\ngrep_buffer, but is instead global to the whole program\n(i.e., if any thread is doing multi-threaded grep, every\nother thread, even if it thinks it is doing its own\nsingle-threaded grep, would need to use the locking). In\npractice, this distinction isn't a problem for us, because\nthe only user of multi-threaded grep is \"git-grep\", which\ndoes nothing except call grep.\n\nThis patch turns the opt->use_threads flag into a global\nflag. More important than the nit-picking semantic argument\nabove is that this means that the locking functions don't\nneed to actually have access to a grep_opt to know whether\nto lock. Which in turn can make adding new locks simpler, as\nwe don't need to pass around a grep_opt.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 4 ++--\n grep.c | 18 ++++++++++--------\n grep.h | 2 +-\n 3 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5c2ae94..06983f9 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -259,6 +259,7 @@ static void start_threads(struct grep_opt *opt)\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n+\tgrep_use_locks = 1;\n \n \tfor (i = 0; i < ARRAY_SIZE(todo); i++) {\n \t\tstrbuf_init(&todo[i].out, 0);\n@@ -307,6 +308,7 @@ static int wait_all(void)\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n+\tgrep_use_locks = 0;\n \n \treturn hit;\n }\n@@ -1030,8 +1032,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tuse_threads = 0;\n #endif\n \n-\topt.use_threads = use_threads;\n-\n #ifndef NO_PTHREADS\n \tif (use_threads) {\n \t\tif (!(opt.name_only || opt.unmatch_name_only || opt.count)\ndiff --git a/grep.c b/grep.c\nindex 486230b..7a67c2f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -807,26 +807,28 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n }\n \n #ifndef NO_PTHREADS\n+int grep_use_locks;\n+\n /*\n * This lock protects access to the gitattributes machinery, which is\n * not thread-safe.\n */\n pthread_mutex_t grep_attr_mutex;\n \n-static inline void grep_attr_lock(struct grep_opt *opt)\n+static inline void grep_attr_lock(void)\n {\n-\tif (opt->use_threads)\n+\tif (grep_use_locks)\n \t\tpthread_mutex_lock(&grep_attr_mutex);\n }\n \n-static inline void grep_attr_unlock(struct grep_opt *opt)\n+static inline void grep_attr_unlock(void)\n {\n-\tif (opt->use_threads)\n+\tif (grep_use_locks)\n \t\tpthread_mutex_unlock(&grep_attr_mutex);\n }\n #else\n-#define grep_attr_lock(opt)\n-#define grep_attr_unlock(opt)\n+#define grep_attr_lock()\n+#define grep_attr_unlock()\n #endif\n \n static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n@@ -834,9 +836,9 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha\n \txdemitconf_t *xecfg = opt->priv;\n \tif (xecfg && !xecfg->find_func) {\n \t\tstruct userdiff_driver *drv;\n-\t\tgrep_attr_lock(opt);\n+\t\tgrep_attr_lock();\n \t\tdrv = userdiff_find_by_path(name);\n-\t\tgrep_attr_unlock(opt);\n+\t\tgrep_attr_unlock();\n \t\tif (drv && drv->funcname.pattern) {\n \t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n \t\t\txdiff_set_find_func(xecfg, pe->pattern, pe->cflags);\ndiff --git a/grep.h b/grep.h\nindex fb205f3..3653bb3 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -116,7 +116,6 @@ struct grep_opt {\n \tint show_hunk_mark;\n \tint file_break;\n \tint heading;\n-\tint use_threads;\n \tvoid *priv;\n \n \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n@@ -138,6 +137,7 @@ extern int grep_threads_ok(const struct grep_opt *opt);\n * Mutex used around access to the attributes machinery if\n * opt->use_threads. Must be initialized/destroyed by callers!\n */\n+extern int grep_use_locks;\n extern pthread_mutex_t grep_attr_mutex;\n #endif\n \n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183553","messageId":"20120202081841.GB6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 2/9] grep: move sha1-reading mutex into low-level code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:18:41Z","receivedAt":"2012-02-02T08:18:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The multi-threaded git-grep code needs to serialize access\nto the thread-unsafe read_sha1_file call. It does this with\na mutex that is local to builtin/grep.c.\n\nLet's instead push this down into grep.c, where it can be\nused by both builtin/grep.c and grep.c. This will let us\nsafely teach the low-level grep.c code tricks that involve\nreading from the object db.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 29 ++++++-----------------------\n grep.c | 6 ++++++\n grep.h | 17 +++++++++++++++++\n 3 files changed, 29 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 06983f9..f4402fa 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -85,21 +85,6 @@ static inline void grep_unlock(void)\n \t\tpthread_mutex_unlock(&grep_mutex);\n }\n \n-/* Used to serialize calls to read_sha1_file. */\n-static pthread_mutex_t read_sha1_mutex;\n-\n-static inline void read_sha1_lock(void)\n-{\n-\tif (use_threads)\n-\t\tpthread_mutex_lock(&read_sha1_mutex);\n-}\n-\n-static inline void read_sha1_unlock(void)\n-{\n-\tif (use_threads)\n-\t\tpthread_mutex_unlock(&read_sha1_mutex);\n-}\n-\n /* Signalled when a new work_item is added to todo. */\n static pthread_cond_t cond_add;\n \n@@ -254,7 +239,7 @@ static void start_threads(struct grep_opt *opt)\n \tint i;\n \n \tpthread_mutex_init(&grep_mutex, NULL);\n-\tpthread_mutex_init(&read_sha1_mutex, NULL);\n+\tpthread_mutex_init(&grep_read_mutex, NULL);\n \tpthread_mutex_init(&grep_attr_mutex, NULL);\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n@@ -303,7 +288,7 @@ static int wait_all(void)\n \t}\n \n \tpthread_mutex_destroy(&grep_mutex);\n-\tpthread_mutex_destroy(&read_sha1_mutex);\n+\tpthread_mutex_destroy(&grep_read_mutex);\n \tpthread_mutex_destroy(&grep_attr_mutex);\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n@@ -313,8 +298,6 @@ static int wait_all(void)\n \treturn hit;\n }\n #else /* !NO_PTHREADS */\n-#define read_sha1_lock()\n-#define read_sha1_unlock()\n \n static int wait_all(void)\n {\n@@ -376,9 +359,9 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type\n {\n \tvoid *data;\n \n-\tread_sha1_lock();\n+\tgrep_read_lock();\n \tdata = read_sha1_file(sha1, type, size);\n-\tread_sha1_unlock();\n+\tgrep_read_unlock();\n \treturn data;\n }\n \n@@ -617,10 +600,10 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tstruct strbuf base;\n \t\tint hit, len;\n \n-\t\tread_sha1_lock();\n+\t\tgrep_read_lock();\n \t\tdata = read_object_with_reference(obj->sha1, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-\t\tread_sha1_unlock();\n+\t\tgrep_read_unlock();\n \n \t\tif (!data)\n \t\t\tdie(_(\"unable to read tree (%s)\"), sha1_to_hex(obj->sha1));\ndiff --git a/grep.c b/grep.c\nindex 7a67c2f..db58a29 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -826,6 +826,12 @@ static inline void grep_attr_unlock(void)\n \tif (grep_use_locks)\n \t\tpthread_mutex_unlock(&grep_attr_mutex);\n }\n+\n+/*\n+ * Same as git_attr_mutex, but protecting the thread-unsafe object db access.\n+ */\n+pthread_mutex_t grep_read_mutex;\n+\n #else\n #define grep_attr_lock()\n #define grep_attr_unlock()\ndiff --git a/grep.h b/grep.h\nindex 3653bb3..4f1b025 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -139,6 +139,23 @@ extern int grep_threads_ok(const struct grep_opt *opt);\n */\n extern int grep_use_locks;\n extern pthread_mutex_t grep_attr_mutex;\n+extern pthread_mutex_t grep_read_mutex;\n+\n+static inline void grep_read_lock(void)\n+{\n+\tif (grep_use_locks)\n+\t\tpthread_mutex_lock(&grep_read_mutex);\n+}\n+\n+static inline void grep_read_unlock(void)\n+{\n+\tif (grep_use_locks)\n+\t\tpthread_mutex_unlock(&grep_read_mutex);\n+}\n+\n+#else\n+#define grep_read_lock()\n+#define grep_read_unlock()\n #endif\n \n #endif\n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183554","messageId":"20120202081928.GC6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 3/9] grep: refactor the concept of \"grep source\" into an object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:19:28Z","receivedAt":"2012-02-02T08:19:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The main interface to the low-level grep code is\ngrep_buffer, which takes a pointer to a buffer and a size.\nThis is convenient and flexible (we use it to grep commit\nbodies, files on disk, and blobs by sha1), but it makes it\nhard to pass extra information about what we are grepping\n(either for correctness, like overriding binary\nauto-detection, or for optimizations, like lazily loading\nblob contents).\n\nInstead, let's encapsulate the idea of a \"grep source\",\nincluding the buffer, its size, and where the data is coming\nfrom. This is similar to the diff_filespec structure used by\nthe diff code (unsurprising, since future patches will\nimplement some of the same optimizations found there).\n\nThe diffstat is slightly scarier than the actual patch\ncontent. Most of the modified lines are simply replacing\naccess to raw variables with their counterparts that are now\nin a \"struct grep_source\". Most of the added lines were\ntaken from builtin/grep.c, which partially abstracted the\nidea of grep sources (for file vs sha1 sources).\n\nInstead of dropping the now-redundant code, this patch\nleaves builtin/grep.c using the traditional grep_buffer\ninterface (which now wraps the grep_source interface). That\nmakes it easy to test that there is no change of behavior\n(yet).\n\nSigned-off-by: Jeff King \n---\n grep.c | 198 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------\n grep.h | 22 +++++++\n 2 files changed, 186 insertions(+), 34 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex db58a29..8204ca2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -837,13 +837,13 @@ pthread_mutex_t grep_read_mutex;\n #define grep_attr_unlock()\n #endif\n \n-static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n+static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\n \tif (xecfg && !xecfg->find_func) {\n \t\tstruct userdiff_driver *drv;\n \t\tgrep_attr_lock();\n-\t\tdrv = userdiff_find_by_path(name);\n+\t\tdrv = userdiff_find_by_path(gs->name);\n \t\tgrep_attr_unlock();\n \t\tif (drv && drv->funcname.pattern) {\n \t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n@@ -866,33 +866,33 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha\n \treturn 0;\n }\n \n-static void show_funcname_line(struct grep_opt *opt, const char *name,\n-\t\t\t char *buf, char *bol, unsigned lno)\n+static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n+\t\t\t char *bol, unsigned lno)\n {\n-\twhile (bol > buf) {\n+\twhile (bol > gs->buf) {\n \t\tchar *eol = --bol;\n \n-\t\twhile (bol > buf && bol[-1] != '\\n')\n+\t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n \t\tlno--;\n \n \t\tif (lno <= opt->last_shown)\n \t\t\tbreak;\n \n-\t\tif (match_funcname(opt, name, bol, eol)) {\n-\t\t\tshow_line(opt, bol, eol, name, lno, '=');\n+\t\tif (match_funcname(opt, gs, bol, eol)) {\n+\t\t\tshow_line(opt, bol, eol, gs->name, lno, '=');\n \t\t\tbreak;\n \t\t}\n \t}\n }\n \n-static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n+static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,\n \t\t\t char *bol, char *end, unsigned lno)\n {\n \tunsigned cur = lno, from = 1, funcname_lno = 0;\n \tint funcname_needed = !!opt->funcname;\n \n-\tif (opt->funcbody && !match_funcname(opt, name, bol, end))\n+\tif (opt->funcbody && !match_funcname(opt, gs, bol, end))\n \t\tfuncname_needed = 2;\n \n \tif (opt->pre_context < lno)\n@@ -901,14 +901,14 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \t\tfrom = opt->last_shown + 1;\n \n \t/* Rewind. */\n-\twhile (bol > buf &&\n+\twhile (bol > gs->buf &&\n \t cur > (funcname_needed == 2 ? opt->last_shown + 1 : from)) {\n \t\tchar *eol = --bol;\n \n-\t\twhile (bol > buf && bol[-1] != '\\n')\n+\t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n \t\tcur--;\n-\t\tif (funcname_needed && match_funcname(opt, name, bol, eol)) {\n+\t\tif (funcname_needed && match_funcname(opt, gs, bol, eol)) {\n \t\t\tfuncname_lno = cur;\n \t\t\tfuncname_needed = 0;\n \t\t}\n@@ -916,7 +916,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \n \t/* We need to look even further back to find a function signature. */\n \tif (opt->funcname && funcname_needed)\n-\t\tshow_funcname_line(opt, name, buf, bol, cur);\n+\t\tshow_funcname_line(opt, gs, bol, cur);\n \n \t/* Back forward. */\n \twhile (cur < lno) {\n@@ -924,7 +924,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \n \t\twhile (*eol != '\\n')\n \t\t\teol++;\n-\t\tshow_line(opt, bol, eol, name, cur, sign);\n+\t\tshow_line(opt, bol, eol, gs->name, cur, sign);\n \t\tbol = eol + 1;\n \t\tcur++;\n \t}\n@@ -991,11 +991,10 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n \tfwrite(buf, size, 1, stdout);\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+static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n-\tchar *bol = buf;\n-\tunsigned long left = size;\n+\tchar *bol;\n+\tunsigned long left;\n \tunsigned lno = 1;\n \tunsigned last_hit = 0;\n \tint binary_match_only = 0;\n@@ -1023,13 +1022,16 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t}\n \topt->last_shown = 0;\n \n+\tif (grep_source_load(gs) < 0)\n+\t\treturn 0;\n+\n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (buffer_is_binary(gs->buf, gs->size))\n \t\t\tbinary_match_only = 1;\n \t\tbreak;\n \tcase GREP_BINARY_NOMATCH:\n-\t\tif (buffer_is_binary(buf, size))\n+\t\tif (buffer_is_binary(gs->buf, gs->size))\n \t\t\treturn 0; /* Assume unmatch */\n \t\tbreak;\n \tcase GREP_BINARY_TEXT:\n@@ -1043,6 +1045,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \n \ttry_lookahead = should_lookahead(opt);\n \n+\tbol = gs->buf;\n+\tleft = gs->size;\n \twhile (left) {\n \t\tchar *eol, ch;\n \t\tint hit;\n@@ -1091,14 +1095,14 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\t\tif (opt->status_only)\n \t\t\t\treturn 1;\n \t\t\tif (opt->name_only) {\n-\t\t\t\tshow_name(opt, name);\n+\t\t\t\tshow_name(opt, gs->name);\n \t\t\t\treturn 1;\n \t\t\t}\n \t\t\tif (opt->count)\n \t\t\t\tgoto next_line;\n \t\t\tif (binary_match_only) {\n \t\t\t\topt->output(opt, \"Binary file \", 12);\n-\t\t\t\toutput_color(opt, name, strlen(name),\n+\t\t\t\toutput_color(opt, gs->name, strlen(gs->name),\n \t\t\t\t\t opt->color_filename);\n \t\t\t\topt->output(opt, \" matches\\n\", 9);\n \t\t\t\treturn 1;\n@@ -1107,23 +1111,23 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\t\t * pre-context lines, we would need to show them.\n \t\t\t */\n \t\t\tif (opt->pre_context || opt->funcbody)\n-\t\t\t\tshow_pre_context(opt, name, buf, bol, eol, lno);\n+\t\t\t\tshow_pre_context(opt, gs, bol, eol, lno);\n \t\t\telse if (opt->funcname)\n-\t\t\t\tshow_funcname_line(opt, name, buf, bol, lno);\n-\t\t\tshow_line(opt, bol, eol, name, lno, ':');\n+\t\t\t\tshow_funcname_line(opt, gs, bol, lno);\n+\t\t\tshow_line(opt, bol, eol, gs->name, lno, ':');\n \t\t\tlast_hit = lno;\n \t\t\tif (opt->funcbody)\n \t\t\t\tshow_function = 1;\n \t\t\tgoto next_line;\n \t\t}\n-\t\tif (show_function && match_funcname(opt, name, bol, eol))\n+\t\tif (show_function && match_funcname(opt, gs, bol, eol))\n \t\t\tshow_function = 0;\n \t\tif (show_function ||\n \t\t (last_hit && lno <= last_hit + opt->post_context)) {\n \t\t\t/* If the last hit is within the post context,\n \t\t\t * we need to show this line.\n \t\t\t */\n-\t\t\tshow_line(opt, bol, eol, name, lno, '-');\n+\t\t\tshow_line(opt, bol, eol, gs->name, lno, '-');\n \t\t}\n \n \tnext_line:\n@@ -1141,7 +1145,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\treturn 0;\n \tif (opt->unmatch_name_only) {\n \t\t/* We did not see any hit, so we want to show this */\n-\t\tshow_name(opt, name);\n+\t\tshow_name(opt, gs->name);\n \t\treturn 1;\n \t}\n \n@@ -1155,7 +1159,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t */\n \tif (opt->count && count) {\n \t\tchar buf[32];\n-\t\toutput_color(opt, name, strlen(name), opt->color_filename);\n+\t\toutput_color(opt, gs->name, strlen(gs->name), opt->color_filename);\n \t\toutput_sep(opt, ':');\n \t\tsnprintf(buf, sizeof(buf), \"%u\\n\", count);\n \t\topt->output(opt, buf, strlen(buf));\n@@ -1190,23 +1194,149 @@ static int chk_hit_marker(struct grep_expr *x)\n \t}\n }\n \n-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)\n+int grep_source(struct grep_opt *opt, struct grep_source *gs)\n {\n \t/*\n \t * we do not have to do the two-pass grep when we do not check\n \t * buffer-wide \"all-match\".\n \t */\n \tif (!opt->all_match)\n-\t\treturn grep_buffer_1(opt, name, buf, size, 0);\n+\t\treturn grep_source_1(opt, gs, 0);\n \n \t/* Otherwise the toplevel \"or\" terms hit a bit differently.\n \t * We first clear hit markers from them.\n \t */\n \tclr_hit_marker(opt->pattern_expression);\n-\tgrep_buffer_1(opt, name, buf, size, 1);\n+\tgrep_source_1(opt, gs, 1);\n \n \tif (!chk_hit_marker(opt->pattern_expression))\n \t\treturn 0;\n \n-\treturn grep_buffer_1(opt, name, buf, size, 0);\n+\treturn grep_source_1(opt, gs, 0);\n+}\n+\n+int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)\n+{\n+\tstruct grep_source gs;\n+\tint r;\n+\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, name, NULL);\n+\tgs.buf = buf;\n+\tgs.size = size;\n+\n+\tr = grep_source(opt, &gs);\n+\n+\tgrep_source_clear(&gs);\n+\treturn r;\n+}\n+\n+void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n+\t\t const char *name, const void *identifier)\n+{\n+\tgs->type = type;\n+\tgs->name = name ? xstrdup(name) : NULL;\n+\tgs->buf = NULL;\n+\tgs->size = 0;\n+\n+\tswitch (type) {\n+\tcase GREP_SOURCE_FILE:\n+\t\tgs->identifier = xstrdup(identifier);\n+\t\tbreak;\n+\tcase GREP_SOURCE_SHA1:\n+\t\tgs->identifier = xmalloc(20);\n+\t\tmemcpy(gs->identifier, identifier, 20);\n+\t\tbreak;\n+\tcase GREP_SOURCE_BUF:\n+\t\tgs->identifier = NULL;\n+\t}\n+}\n+\n+void grep_source_clear(struct grep_source *gs)\n+{\n+\tfree(gs->name);\n+\tgs->name = NULL;\n+\tfree(gs->identifier);\n+\tgs->identifier = NULL;\n+\tgrep_source_clear_data(gs);\n+}\n+\n+void grep_source_clear_data(struct grep_source *gs)\n+{\n+\tswitch (gs->type) {\n+\tcase GREP_SOURCE_FILE:\n+\tcase GREP_SOURCE_SHA1:\n+\t\tfree(gs->buf);\n+\t\tgs->buf = NULL;\n+\t\tgs->size = 0;\n+\t\tbreak;\n+\tcase GREP_SOURCE_BUF:\n+\t\t/* leave user-provided buf intact */\n+\t\tbreak;\n+\t}\n+}\n+\n+static int grep_source_load_sha1(struct grep_source *gs)\n+{\n+\tenum object_type type;\n+\n+\tgrep_read_lock();\n+\tgs->buf = read_sha1_file(gs->identifier, &type, &gs->size);\n+\tgrep_read_unlock();\n+\n+\tif (!gs->buf)\n+\t\treturn error(_(\"'%s': unable to read %s\"),\n+\t\t\t gs->name,\n+\t\t\t sha1_to_hex(gs->identifier));\n+\treturn 0;\n+}\n+\n+static int grep_source_load_file(struct grep_source *gs)\n+{\n+\tconst char *filename = gs->identifier;\n+\tstruct stat st;\n+\tchar *data;\n+\tsize_t size;\n+\tint i;\n+\n+\tif (lstat(filename, &st) < 0) {\n+\terr_ret:\n+\t\tif (errno != ENOENT)\n+\t\t\terror(_(\"'%s': %s\"), filename, strerror(errno));\n+\t\treturn -1;\n+\t}\n+\tif (!S_ISREG(st.st_mode))\n+\t\treturn -1;\n+\tsize = xsize_t(st.st_size);\n+\ti = open(filename, O_RDONLY);\n+\tif (i < 0)\n+\t\tgoto err_ret;\n+\tdata = xmalloc(size + 1);\n+\tif (st.st_size != read_in_full(i, data, size)) {\n+\t\terror(_(\"'%s': short read %s\"), filename, strerror(errno));\n+\t\tclose(i);\n+\t\tfree(data);\n+\t\treturn -1;\n+\t}\n+\tclose(i);\n+\tdata[size] = 0;\n+\n+\tgs->buf = data;\n+\tgs->size = size;\n+\treturn 0;\n+}\n+\n+int grep_source_load(struct grep_source *gs)\n+{\n+\tif (gs->buf)\n+\t\treturn 0;\n+\n+\tswitch (gs->type) {\n+\tcase GREP_SOURCE_FILE:\n+\t\treturn grep_source_load_file(gs);\n+\tcase GREP_SOURCE_SHA1:\n+\t\treturn grep_source_load_sha1(gs);\n+\tcase GREP_SOURCE_BUF:\n+\t\treturn gs->buf ? 0 : -1;\n+\t}\n+\tdie(\"BUG: invalid grep_source type\");\n }\ndiff --git a/grep.h b/grep.h\nindex 4f1b025..e386ca4 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -129,6 +129,28 @@ extern void compile_grep_patterns(struct grep_opt *opt);\n extern void free_grep_patterns(struct grep_opt *opt);\n extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);\n \n+struct grep_source {\n+\tchar *name;\n+\n+\tenum grep_source_type {\n+\t\tGREP_SOURCE_SHA1,\n+\t\tGREP_SOURCE_FILE,\n+\t\tGREP_SOURCE_BUF,\n+\t} type;\n+\tvoid *identifier;\n+\n+\tchar *buf;\n+\tunsigned long size;\n+};\n+\n+void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n+\t\t const char *name, const void *identifier);\n+int grep_source_load(struct grep_source *gs);\n+void grep_source_clear_data(struct grep_source *gs);\n+void grep_source_clear(struct grep_source *gs);\n+\n+int grep_source(struct grep_opt *opt, struct grep_source *gs);\n+\n extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n extern int grep_threads_ok(const struct grep_opt *opt);\n \n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183555","messageId":"20120202081937.GD6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 4/9] convert git-grep to use grep_source interface","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:19:37Z","receivedAt":"2012-02-02T08:19:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The grep_source interface (as opposed to grep_buffer) will\neventually gives us a richer interface for telling the\nlow-level grep code about our buffers. Eventually this will\nlead to things like better binary-file handling. For now, it\nlets us drop a lot of now-redundant code.\n\nThe conversion is mostly straight-forward. One thing to note\nis that the memory ownership rules for \"struct grep_source\"\nare different than the \"struct work_item\" found here (the\nformer will copy things like the filename, rather than\ntaking ownership). Therefore you will also see some slight\ntweaking of when filename buffers are released.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 142 +++++++++-----------------------------------------------\n 1 files changed, 23 insertions(+), 119 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex f4402fa..bc85a20 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -29,25 +29,12 @@ static int use_threads = 1;\n #define THREADS 8\n static pthread_t threads[THREADS];\n \n-static void *load_sha1(const unsigned char *sha1, unsigned long *size,\n-\t\t const char *name);\n-static void *load_file(const char *filename, size_t *sz);\n-\n-enum work_type {WORK_SHA1, WORK_FILE};\n-\n /* We use one producer thread and THREADS consumer\n * threads. The producer adds struct work_items to 'todo' and the\n * consumers pick work items from the same array.\n */\n struct work_item {\n-\tenum work_type type;\n-\tchar *name;\n-\n-\t/* if type == WORK_SHA1, then 'identifier' is a SHA1,\n-\t * otherwise type == WORK_FILE, and 'identifier' is a NUL\n-\t * terminated filename.\n-\t */\n-\tvoid *identifier;\n+\tstruct grep_source source;\n \tchar done;\n \tstruct strbuf out;\n };\n@@ -98,7 +85,8 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(enum work_type type, char *name, void *id)\n+static void add_work(enum grep_source_type type, const char *name,\n+\t\t const void *id)\n {\n \tgrep_lock();\n \n@@ -106,9 +94,7 @@ static void add_work(enum work_type type, char *name, void *id)\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\ttodo[todo_end].type = type;\n-\ttodo[todo_end].name = name;\n-\ttodo[todo_end].identifier = id;\n+\tgrep_source_init(&todo[todo_end].source, type, name, id);\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n \ttodo_end = (todo_end + 1) % ARRAY_SIZE(todo);\n@@ -136,21 +122,6 @@ static struct work_item *get_work(void)\n \treturn ret;\n }\n \n-static void grep_sha1_async(struct grep_opt *opt, char *name,\n-\t\t\t const unsigned char *sha1)\n-{\n-\tunsigned char *s;\n-\ts = xmalloc(20);\n-\tmemcpy(s, sha1, 20);\n-\tadd_work(WORK_SHA1, name, s);\n-}\n-\n-static void grep_file_async(struct grep_opt *opt, char *name,\n-\t\t\t const char *filename)\n-{\n-\tadd_work(WORK_FILE, name, xstrdup(filename));\n-}\n-\n static void work_done(struct work_item *w)\n {\n \tint old_done;\n@@ -177,8 +148,7 @@ static void work_done(struct work_item *w)\n \n \t\t\twrite_or_die(1, p, len);\n \t\t}\n-\t\tfree(w->name);\n-\t\tfree(w->identifier);\n+\t\tgrep_source_clear(&w->source);\n \t}\n \n \tif (old_done != todo_done)\n@@ -201,25 +171,8 @@ static void *run(void *arg)\n \t\t\tbreak;\n \n \t\topt->output_priv = w;\n-\t\tif (w->type == WORK_SHA1) {\n-\t\t\tunsigned long sz;\n-\t\t\tvoid* data = load_sha1(w->identifier, &sz, w->name);\n-\n-\t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, data, sz);\n-\t\t\t\tfree(data);\n-\t\t\t}\n-\t\t} else if (w->type == WORK_FILE) {\n-\t\t\tsize_t sz;\n-\t\t\tvoid* data = load_file(w->identifier, &sz);\n-\t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, data, sz);\n-\t\t\t\tfree(data);\n-\t\t\t}\n-\t\t} else {\n-\t\t\tassert(0);\n-\t\t}\n-\n+\t\thit |= grep_source(opt, &w->source);\n+\t\tgrep_source_clear_data(&w->source);\n \t\twork_done(w);\n \t}\n \tfree_grep_patterns(arg);\n@@ -365,23 +318,10 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type\n \treturn data;\n }\n \n-static void *load_sha1(const unsigned char *sha1, unsigned long *size,\n-\t\t const char *name)\n-{\n-\tenum object_type type;\n-\tvoid *data = lock_and_read_sha1_file(sha1, &type, size);\n-\n-\tif (!data)\n-\t\terror(_(\"'%s': unable to read %s\"), name, sha1_to_hex(sha1));\n-\n-\treturn data;\n-}\n-\n static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\t const char *filename, int tree_name_len)\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n-\tchar *name;\n \n \tif (opt->relative && opt->prefix_length) {\n \t\tquote_path_relative(filename + tree_name_len, -1, &pathbuf,\n@@ -391,87 +331,51 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n-\tname = strbuf_detach(&pathbuf, NULL);\n-\n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tgrep_sha1_async(opt, name, sha1);\n+\t\tadd_work(GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n+\t\tstruct grep_source gs;\n \t\tint hit;\n-\t\tunsigned long sz;\n-\t\tvoid *data = load_sha1(sha1, &sz, name);\n-\t\tif (!data)\n-\t\t\thit = 0;\n-\t\telse\n-\t\t\thit = grep_buffer(opt, name, data, sz);\n \n-\t\tfree(data);\n-\t\tfree(name);\n-\t\treturn hit;\n-\t}\n-}\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tstrbuf_release(&pathbuf);\n+\t\thit = grep_source(opt, &gs);\n \n-static void *load_file(const char *filename, size_t *sz)\n-{\n-\tstruct stat st;\n-\tchar *data;\n-\tint i;\n-\n-\tif (lstat(filename, &st) < 0) {\n-\terr_ret:\n-\t\tif (errno != ENOENT)\n-\t\t\terror(_(\"'%s': %s\"), filename, strerror(errno));\n-\t\treturn NULL;\n-\t}\n-\tif (!S_ISREG(st.st_mode))\n-\t\treturn NULL;\n-\t*sz = xsize_t(st.st_size);\n-\ti = open(filename, O_RDONLY);\n-\tif (i < 0)\n-\t\tgoto err_ret;\n-\tdata = xmalloc(*sz + 1);\n-\tif (st.st_size != read_in_full(i, data, *sz)) {\n-\t\terror(_(\"'%s': short read %s\"), filename, strerror(errno));\n-\t\tclose(i);\n-\t\tfree(data);\n-\t\treturn NULL;\n+\t\tgrep_source_clear(&gs);\n+\t\treturn hit;\n \t}\n-\tclose(i);\n-\tdata[*sz] = 0;\n-\treturn data;\n }\n \n static int grep_file(struct grep_opt *opt, const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tchar *name;\n \n \tif (opt->relative && opt->prefix_length)\n \t\tquote_path_relative(filename, -1, &buf, opt->prefix);\n \telse\n \t\tstrbuf_addstr(&buf, filename);\n-\tname = strbuf_detach(&buf, NULL);\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tgrep_file_async(opt, name, filename);\n+\t\tadd_work(GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n+\t\tstruct grep_source gs;\n \t\tint hit;\n-\t\tsize_t sz;\n-\t\tvoid *data = load_file(filename, &sz);\n-\t\tif (!data)\n-\t\t\thit = 0;\n-\t\telse\n-\t\t\thit = grep_buffer(opt, name, data, sz);\n \n-\t\tfree(data);\n-\t\tfree(name);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tstrbuf_release(&buf);\n+\t\thit = grep_source(opt, &gs);\n+\n+\t\tgrep_source_clear(&gs);\n \t\treturn hit;\n \t}\n }\n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183556","messageId":"20120202082010.GE6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 5/9] grep: drop grep_buffer's \"name\" parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:20:10Z","receivedAt":"2012-02-02T08:20:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Before the grep_source interface existed, grep_buffer was\nused by two types of callers:\n\n 1. Ones which pulled a file into a buffer, and then wanted\n to supply the file's name for the output (i.e.,\n git grep).\n\n 2. Ones which really just wanted to grep a buffer (i.e.,\n git log --grep).\n\nCallers in set (1) should now be using grep_source. Callers\nin set (2) always pass NULL for the \"name\" parameter of\ngrep_buffer. We can therefore get rid of this now-useless\nparameter.\n\nSigned-off-by: Jeff King \n---\nThis one isn't necessary, obviously, but I think it's a nice clean-up\nafter the last two patches.\n\n grep.c | 4 ++--\n grep.h | 2 +-\n revision.c | 1 -\n 3 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 8204ca2..2a3fe7c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1215,12 +1215,12 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)\n \treturn grep_source_1(opt, gs, 0);\n }\n \n-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)\n+int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n {\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, name, NULL);\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \ndiff --git a/grep.h b/grep.h\nindex e386ca4..8bf3001 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -127,7 +127,7 @@ extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const cha\n extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);\n extern void compile_grep_patterns(struct grep_opt *opt);\n extern void free_grep_patterns(struct grep_opt *opt);\n-extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);\n+extern int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size);\n \n struct grep_source {\n \tchar *name;\ndiff --git a/revision.c b/revision.c\nindex c97d834..819ff01 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2149,7 +2149,6 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n \t\treturn 1;\n \treturn grep_buffer(&opt->grep_filter,\n-\t\t\t NULL, /* we say nothing, not even filename */\n \t\t\t commit->buffer, strlen(commit->buffer));\n }\n \n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183557","messageId":"20120202082043.GF6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 6/9] grep: cache userdiff_driver in grep_source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:20:43Z","receivedAt":"2012-02-02T08:20:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Right now, grep only uses the userdiff_driver for one thing:\nlooking up funcname patterns for \"-p\" and \"-W\". As new uses\nfor userdiff drivers are added to the grep code, we want to\nminimize attribute lookups, which can be expensive.\n\nIt might seem at first that this would also optimize multiple\nlookups when the funcname pattern for a file is needed\nmultiple times. However, the compiled funcname pattern is\nalready cached in struct grep_opt's \"priv\" member, so\nmultiple lookups are already suppressed.\n\nSigned-off-by: Jeff King \n---\n grep.c | 22 ++++++++++++++++------\n grep.h | 4 ++++\n 2 files changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 2a3fe7c..bb18569 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -841,12 +841,9 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo\n {\n \txdemitconf_t *xecfg = opt->priv;\n \tif (xecfg && !xecfg->find_func) {\n-\t\tstruct userdiff_driver *drv;\n-\t\tgrep_attr_lock();\n-\t\tdrv = userdiff_find_by_path(gs->name);\n-\t\tgrep_attr_unlock();\n-\t\tif (drv && drv->funcname.pattern) {\n-\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n+\t\tgrep_source_load_driver(gs);\n+\t\tif (gs->driver->funcname.pattern) {\n+\t\t\tconst struct userdiff_funcname *pe = &gs->driver->funcname;\n \t\t\txdiff_set_find_func(xecfg, pe->pattern, pe->cflags);\n \t\t} else {\n \t\t\txecfg = opt->priv = NULL;\n@@ -1237,6 +1234,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n \tgs->name = name ? xstrdup(name) : NULL;\n \tgs->buf = NULL;\n \tgs->size = 0;\n+\tgs->driver = NULL;\n \n \tswitch (type) {\n \tcase GREP_SOURCE_FILE:\n@@ -1340,3 +1338,15 @@ int grep_source_load(struct grep_source *gs)\n \t}\n \tdie(\"BUG: invalid grep_source type\");\n }\n+\n+void grep_source_load_driver(struct grep_source *gs)\n+{\n+\tif (gs->driver)\n+\t\treturn;\n+\n+\tgrep_attr_lock();\n+\tgs->driver = userdiff_find_by_path(gs->name);\n+\tif (!gs->driver)\n+\t\tgs->driver = userdiff_find_by_name(\"default\");\n+\tgrep_attr_unlock();\n+}\ndiff --git a/grep.h b/grep.h\nindex 8bf3001..73b28c2 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -9,6 +9,7 @@ typedef int pcre_extra;\n #endif\n #include \"kwset.h\"\n #include \"thread-utils.h\"\n+#include \"userdiff.h\"\n \n enum grep_pat_token {\n \tGREP_PATTERN,\n@@ -141,6 +142,8 @@ struct grep_source {\n \n \tchar *buf;\n \tunsigned long size;\n+\n+\tstruct userdiff_driver *driver;\n };\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n@@ -148,6 +151,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n int grep_source_load(struct grep_source *gs);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n+void grep_source_load_driver(struct grep_source *gs);\n \n int grep_source(struct grep_opt *opt, struct grep_source *gs);\n \n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183558","messageId":"20120202082102.GG6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 7/9] grep: respect diff attributes for binary-ness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:21:02Z","receivedAt":"2012-02-02T08:21:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is currently no way for users to tell git-grep that a\nparticular path is or is not a binary file; instead, grep\nalways relies on its auto-detection (or the user specifying\n\"-a\" to treat all binary-looking files like text).\n\nThis patch teaches git-grep to use the same attribute lookup\nthat is used by git-diff. We could add a new \"grep\" flag,\nbut that is unnecessarily complex and unlikely to be useful.\nDespite the name, the \"-diff\" attribute (or \"diff=foo\" and\nthe associated diff.foo.binary config option) are really\nabout describing the contents of the path. It's simply\nhistorical that diff was the only thing that cared about\nthese attributes in the past.\n\nAnd if this simple approach turns out to be insufficient, we\nstill have a backwards-compatible path forward: we can add a\nseparate \"grep\" attribute, and fall back to respecting\n\"diff\" if it is unset.\n\nSigned-off-by: Jeff King \n---\n grep.c | 16 ++++++++++++++--\n grep.h | 1 +\n t/t7008-grep-binary.sh | 24 ++++++++++++++++++++++++\n 3 files changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex bb18569..a50d161 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1024,11 +1024,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n-\t\tif (buffer_is_binary(gs->buf, gs->size))\n+\t\tif (grep_source_is_binary(gs))\n \t\t\tbinary_match_only = 1;\n \t\tbreak;\n \tcase GREP_BINARY_NOMATCH:\n-\t\tif (buffer_is_binary(gs->buf, gs->size))\n+\t\tif (grep_source_is_binary(gs))\n \t\t\treturn 0; /* Assume unmatch */\n \t\tbreak;\n \tcase GREP_BINARY_TEXT:\n@@ -1350,3 +1350,15 @@ void grep_source_load_driver(struct grep_source *gs)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\n }\n+\n+int grep_source_is_binary(struct grep_source *gs)\n+{\n+\tgrep_source_load_driver(gs);\n+\tif (gs->driver->binary != -1)\n+\t\treturn gs->driver->binary;\n+\n+\tif (!grep_source_load(gs))\n+\t\treturn buffer_is_binary(gs->buf, gs->size);\n+\n+\treturn 0;\n+}\ndiff --git a/grep.h b/grep.h\nindex 73b28c2..36e49d8 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -152,6 +152,7 @@ int grep_source_load(struct grep_source *gs);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs);\n+int grep_source_is_binary(struct grep_source *gs);\n \n int grep_source(struct grep_opt *opt, struct grep_source *gs);\n \ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex 917a264..fd6410f 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -99,4 +99,28 @@ test_expect_success 'git grep yx a' \"\n \ttest_must_fail git grep -f f a\n \"\n \n+test_expect_success 'grep respects binary diff attribute' '\n+\techo text >t &&\n+\tgit add t &&\n+\techo t:text >expect &&\n+\tgit grep text t >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"t -diff\" >.gitattributes &&\n+\techo \"Binary file t matches\" >expect &&\n+\tgit grep text t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep respects not-binary diff attribute' '\n+\techo binQary | q_to_nul >b &&\n+\tgit add b &&\n+\techo \"Binary file b matches\" >expect &&\n+\tgit grep bin b >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"b diff\" >.gitattributes &&\n+\techo \"b:binQary\" >expect &&\n+\tgit grep bin b | nul_to_q >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183559","messageId":"20120202082111.GH6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 8/9] grep: load file data after checking binary-ness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:21:11Z","receivedAt":"2012-02-02T08:21:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Usually we load each file to grep into memory, check whether\nit's binary, and then either grep it (the default) or not\n(if \"-I\" was given).\n\nIn the \"-I\" case, we can skip loading the file entirely if\nit is marked as binary via gitattributes. On my giant\n3-gigabyte media repository, doing \"git grep -I foo\" went\nfrom:\n\n real 0m0.712s\n user 0m0.044s\n sys 0m4.780s\n\nto:\n\n real 0m0.026s\n user 0m0.016s\n sys 0m0.020s\n\nObviously this is an extreme example. The repo is almost\nentirely binary files, and you can see that we spent all of\nour time asking the kernel to read() the data. However, with\na cold disk cache, even avoiding a few binary files can have\nan impact.\n\nSigned-off-by: Jeff King \n---\n grep.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex a50d161..3821400 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1019,9 +1019,6 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t}\n \topt->last_shown = 0;\n \n-\tif (grep_source_load(gs) < 0)\n-\t\treturn 0;\n-\n \tswitch (opt->binary) {\n \tcase GREP_BINARY_DEFAULT:\n \t\tif (grep_source_is_binary(gs))\n@@ -1042,6 +1039,9 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \ttry_lookahead = should_lookahead(opt);\n \n+\tif (grep_source_load(gs) < 0)\n+\t\treturn 0;\n+\n \tbol = gs->buf;\n \tleft = gs->size;\n \twhile (left) {\n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183560","messageId":"20120202082428.GI6786@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"[PATCH 9/9] grep: pre-load userdiff drivers when threaded","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:24:28Z","receivedAt":"2012-02-02T08:24:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The low-level grep_source code will automatically load the\nuserdiff driver to see whether a file is binary. However,\nwhen we are threaded, it will load the drivers in a\nnon-deterministic order, handling each one as its assigned\nthread happens to be scheduled.\n\nMeanwhile, the attribute lookup code (which underlies the\nuserdiff driver lookup) is optimized to handle paths in\nsequential order (because they tend to share the same\ngitattributes files). Multi-threading the lookups destroys\nthe locality and makes this optimization less effective.\n\nWe can fix this by pre-loading the userdiff driver in the\nmain thread, before we hand off the file to a worker thread.\nMy best-of-five for \"git grep foo\" on the linux-2.6\nrepository went from:\n\n real 0m0.391s\n user 0m1.708s\n sys 0m0.584s\n\nto:\n\n real 0m0.360s\n user 0m1.576s\n sys 0m0.572s\n\nNot a huge speedup, but it's quite easy to do. The only\ntrick is that we shouldn't perform this optimization if \"-a\"\nwas used, in which case we won't bother checking whether\nthe files are binary at all.\n\nSigned-off-by: Jeff King \n---\nThe speedup is especially unimpressive when you consider that it won't\ngrow as the grep load grows. This is a pretty fast grep. If you used a\nreal regex, the whole thing would take even longer, and you will still\nonly be shaving off a few tens of milliseconds. So I wouldn't be\nheart-broken if this patch was dropped. I included it because it's easy\nto do, and maybe somebody with a slower machine would find the absolute\ntime difference more noticeable.\n\n builtin/grep.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex bc85a20..9fc3e95 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -85,8 +85,8 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(enum grep_source_type type, const char *name,\n-\t\t const void *id)\n+static void add_work(struct grep_opt *opt, enum grep_source_type type,\n+\t\t const char *name, const void *id)\n {\n \tgrep_lock();\n \n@@ -95,6 +95,8 @@ static void add_work(enum grep_source_type type, const char *name,\n \t}\n \n \tgrep_source_init(&todo[todo_end].source, type, name, id);\n+\tif (opt->binary != GREP_BINARY_TEXT)\n+\t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n \ttodo_end = (todo_end + 1) % ARRAY_SIZE(todo);\n@@ -333,7 +335,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n@@ -362,7 +364,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n-- \n1.7.9.3.gc3fce1.dirty\n"},{"id":"183561","messageId":"20120202083009.GA6933@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] respect binary attribute in grep","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T08:30:09Z","receivedAt":"2012-02-02T08:30:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2012 at 03:17:47AM -0500, Jeff King wrote:\n\n> I implemented all of the other optimizations I mentioned except the\n> \"only stream the first few bytes when auto-detecting binary-ness\" one.\n> However, it should be easy to do on top of these changes. I need to\n> re-visit the similar change to diff_filespec_is_binary, and I'll do both\n> at the same time.\n\nOh, and I didn't even think about implementing streaming grep. The\ncontext-finding code relies on being able to backtrack through the file\nin memory. We _could_ implement streaming only for binary files (i.e.,\nwhen we will just print \"Binary file foo matches\"). However, I suspect\npeople with big binary files will want to be using \"-I\" anyway, so as to\navoid even pulling the data from disk at all.\n\nWe might eventually want to add a config-option version of \"-I\" for\npeople who have repositories of mixed source code and large binary\nassets.\n\n-Peff\n"},{"id":"183582","messageId":"87vcnp5wkg.fsf@thomas.inf.ethz.ch","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] respect binary attribute in grep","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-02T11:00:47Z","receivedAt":"2012-02-02T11:00:47Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King writes:\n\n> [+cc Thomas, as I am mangling some of his recent work with my\n> refactoring]\n\nMangling? I think it all looks very good.\n\nMy original plan was to make use_threads git-global, instead of\ngrep-global (and shift responsibility to the subsystems instead of their\nusers), but that's just me and the patches aren't ready yet.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"183586","messageId":"20120202110719.GA29870@sigill.intra.peff.net","threadId":"29381","inReplyTo":"87vcnp5wkg.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 0/9] respect binary attribute in grep","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T11:07:19Z","receivedAt":"2012-02-02T11:07:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2012 at 12:00:47PM +0100, Thomas Rast wrote:\n\n> My original plan was to make use_threads git-global, instead of\n> grep-global (and shift responsibility to the subsystems instead of their\n> users), but that's just me and the patches aren't ready yet.\n\nYeah, having just dug into the threading code in grep a bit, I agree\nthat would be a saner approach. The locking is all bolted-on, so you end\nup with these weird contracts between code, like the low-level grep code\nasking anybody who might be multi-threading it to initialize the mutexes\nto cover access to a totally different subsystem. I'd much rather each\nsubsystem just take care of itself.\n\n-Peff\n"},{"id":"183621","messageId":"7v4nv9xexs.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"20120202082043.GF6786@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] grep: cache userdiff_driver in grep_source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T18:34:07Z","receivedAt":"2012-02-02T18:34:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> -\t\tgrep_attr_lock();\n> -\t\tdrv = userdiff_find_by_path(gs->name);\n> -\t\tgrep_attr_unlock();\n> -\t\tif (drv && drv->funcname.pattern) {\n> -\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n> +\t\tgrep_source_load_driver(gs);\n> +\t\tif (gs->driver->funcname.pattern) {\n> +\t\t\tconst struct userdiff_funcname *pe = &gs->driver->funcname;\n\nWhen we load driver, gs->driver gets at least \"default\" driver, so we no\nlonger need to check for drv != NULL as we used to? Is that the reason\nfor the slight difference here?\n\n> @@ -1237,6 +1234,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n> \tgs->name = name ? xstrdup(name) : NULL;\n> \tgs->buf = NULL;\n> \tgs->size = 0;\n> +\tgs->driver = NULL;\n> \n> \tswitch (type) {\n> \tcase GREP_SOURCE_FILE:\n"},{"id":"183626","messageId":"7vzkd1w04j.fsf@alter.siamese.dyndns.org","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] respect binary attribute in grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T18:39:24Z","receivedAt":"2012-02-02T18:39:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... The result turned out much easier to read\n> (and explain in the commit messages, as it was simple to break into\n> smaller commits)....\n\nIndeed the series is very nicely done ;-)\n\nThanks.\n"},{"id":"183639","messageId":"20120202193756.GA9246@sigill.intra.peff.net","threadId":"29381","inReplyTo":"7v4nv9xexs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/9] grep: cache userdiff_driver in grep_source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T19:37:56Z","receivedAt":"2012-02-02T19:37:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2012 at 10:34:07AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > -\t\tgrep_attr_lock();\n> > -\t\tdrv = userdiff_find_by_path(gs->name);\n> > -\t\tgrep_attr_unlock();\n> > -\t\tif (drv && drv->funcname.pattern) {\n> > -\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n> > +\t\tgrep_source_load_driver(gs);\n> > +\t\tif (gs->driver->funcname.pattern) {\n> > +\t\t\tconst struct userdiff_funcname *pe = &gs->driver->funcname;\n> \n> When we load driver, gs->driver gets at least \"default\" driver, so we no\n> longer need to check for drv != NULL as we used to? Is that the reason\n> for the slight difference here?\n\nYes, exactly.\n\nWe could just leave gs->driver NULL instead of looking up \"default\", and\nthen use NULL to signal to the calling code that defaults should be\nused. But NULL is interpreted by grep_source_load_driver as \"we did not\nlook up the driver yet\", so the common case of \"no driver\" would mean we\naccidentally do the lookup multiple times. The diff_filespec code uses\nthe same convention to solve the same problem.\n\nSpeaking of which, there was some notion in my mind that a \"grep_source\"\nand a \"diff_filespec\" were very similar objects, and that we could\npossibly unify the implementations. I decided against that route with\nthis series, as it would have involved pretty heavy refactoring of the\ndiff code to prevent a fairly small amount of code duplication.\n\n-Peff\n"},{"id":"183850","messageId":"20120204192252.GA15319@padd.com","threadId":"29381","inReplyTo":"20120202081747.GA10271@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] respect binary attribute in grep","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-02-04T19:22:52Z","receivedAt":"2012-02-04T19:22:52Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"I took a look at this series. It's nice. My worry was that the\nextra open() of non-existent .gitattributes files in all the\ndirectories would cause performance problems across networked\nfilesystems like NFS.\n\nMy usual (non-public) repository has order:\n\n 100k files\n 10k directories\n\nand no files marked as binary. The grep string is such that it\nis disk-bound, and not expected to match in any file (or binary):\n\"time ~/src/git/bin-wrappers/git grep unfindable-string\".\n\nWith your change, there are 10k new open() calls looking for\n.gitattributes in each directory, all of which return ENOENT.\nThis turns out to have an insignificant impact on performance due\nto the much bigger time sink of stat()-ing all the files.\n\nI think this happens to be true because the gitattributes lookups\nrun in parallel to all the file stat work, as the main thread\ndispatches file work while doing its own gitattributes lookups.\n\nIt could be plausible that deep directory structures with few\ngrep-able files will suffer with this change. For example, many\nbig binary blobs in deep directory hierarchies, but also some\nuseful files here and there.\n\nOne could argue that with the use of .gitattributes to specify\nwhich blobs should not be searched, this series makes this faster\nby not having to to read the binary blobs at all. And I'd be\nokay with that.\n\nJust FYI that there may be a performance impact on certain\nrepositories.\n\n\t\t-- Pete\n"},{"id":"183875","messageId":"20120204231825.GA1170@sigill.intra.peff.net","threadId":"29381","inReplyTo":"20120204192252.GA15319@padd.com","subject":"Re: [PATCH 0/9] respect binary attribute in grep","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T23:18:25Z","receivedAt":"2012-02-04T23:18:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 04, 2012 at 02:22:52PM -0500, Pete Wyckoff wrote:\n\n> I took a look at this series. It's nice. My worry was that the\n> extra open() of non-existent .gitattributes files in all the\n> directories would cause performance problems across networked\n> filesystems like NFS.\n\nYeah, I was able to measure a small slow-down on a quick grep even with\na warm cache. So it does take some extra effort, but I think the\ncorrectness is worth it (and note that the slow down is in the tens of\nmilliseconds if you have a reasonable stat()).\n\nIf people have big trees on NFS (or some other slow-stat system) where\nthese lookups are actually a problem, I'd rather see a global option to\ndisable .gitattributes lookups for both diff and grep (i.e., a \"trust\nme, I'm not using gitattributes, and don't bother with stat\" flag). In\npractice, though, I think such a thing is not necessary because the\nstat() is local to the file being examined (e.g., for \"foo/bar/baz\", we\nlook only at \"foo/bar/.gitattributes\", \"foo/.gitattributes\", and\n\".gitattributes\", without having to touch other parts of the tree).\n\nAnyway, thanks for doing some performance testing. More data is always\ngood.\n\n> It could be plausible that deep directory structures with few\n> grep-able files will suffer with this change. For example, many\n> big binary blobs in deep directory hierarchies, but also some\n> useful files here and there.\n>\n> One could argue that with the use of .gitattributes to specify\n> which blobs should not be searched, this series makes this faster\n> by not having to to read the binary blobs at all. And I'd be\n> okay with that.\n\nYes, exactly. I think this will end up being a big win for such cases,\nbecause the cost of loading even one large binary file from disk will\ndwarf all of the stats. But it does depend on people marking their\nbinaries and using \"-I\".\n\n-Peff\n"}]}