{"thread":{"id":"36655","subject":"[PATCH] grep -I: do not bother to read known-binary files","startedAt":"2014-05-14T15:44:19Z","lastAt":"2014-05-16T09:16:41Z","messageCount":10,"participants":["Stepan Kasal","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"241506","messageId":"20140514154419.GA4517@camelia.ucw.cz","threadId":"36655","inReplyTo":null,"subject":"[PATCH] grep -I: do not bother to read known-binary files","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2014-05-14T15:44:19Z","receivedAt":"2014-05-14T15:44:19Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate: Mon, 8 Nov 2010 16:10:43 +0100\n\nIncidentally, this makes grep -I respect the \"binary\" attribute (actually,\nthe \"-text\" attribute, but \"binary\" implies that).\n\nSince the attributes are not thread-safe, we now need to switch off\nthreading if -I was passed.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Stepan Kasal <kasal@ucw.cz>\n---\n\nHi,\nthis patch has been in msysgit for 3.5 years.\nStepan\n\n builtin/grep.c | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 43af5b7..8073fbe 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -18,6 +18,7 @@\n #include \"quote.h\"\n #include \"dir.h\"\n #include \"pathspec.h\"\n+#include \"attr.h\"\n \n static char const * const grep_usage[] = {\n \tN_(\"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]\"),\n@@ -163,6 +164,22 @@ static void work_done(struct work_item *w)\n \tgrep_unlock();\n }\n \n+static int skip_binary(struct grep_opt *opt, const char *filename)\n+{\n+\tif ((opt->binary & GREP_BINARY_NOMATCH)) {\n+\t\tstatic struct git_attr *attr_text;\n+\t\tstruct git_attr_check check;\n+\n+\t\tif (!attr_text)\n+\t\t\tattr_text = git_attr(\"text\");\n+\t\tmemset(&check, 0, sizeof(check));\n+\t\tcheck.attr = attr_text;\n+\t\treturn !git_check_attr(filename, 1, &check) &&\n+\t\t\t\tATTR_FALSE(check.value);\n+\t}\n+\treturn 0;\n+}\n+\n static void *run(void *arg)\n {\n \tint hit = 0;\n@@ -173,6 +190,9 @@ static void *run(void *arg)\n \t\tif (!w)\n \t\t\tbreak;\n \n+\t\tif (skip_binary(opt, (const char *)w->source.identifier))\n+\t\t\tcontinue;\n+\n \t\topt->output_priv = w;\n \t\thit |= grep_source(opt, &w->source);\n \t\tgrep_source_clear_data(&w->source);\n@@ -379,6 +399,9 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \t\t\tcontinue;\n \t\tif (!ce_path_match(ce, pathspec, NULL))\n \t\t\tcontinue;\n+\t\tif (skip_binary(opt, ce->name))\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * If CE_VALID is on, we assume worktree file and its cache entry\n \t\t * are identical, even if worktree file has been modified, so use\n@@ -803,6 +826,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tstring_list_append(&path_list, show_in_pager);\n \t\tuse_threads = 0;\n \t}\n+\tif ((opt.binary & GREP_BINARY_NOMATCH))\n+\t\tuse_threads = 0;\n \n \tif (!opt.pattern_list)\n \t\tdie(_(\"no pattern given.\"));\n-- \n1.9.2.msysgit.0.335.gd2a461f\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241514","messageId":"xmqqbnv0l02r.fsf@gitster.dls.corp.google.com","threadId":"36655","inReplyTo":"20140514154419.GA4517@camelia.ucw.cz","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-14T17:52:28Z","receivedAt":"2014-05-14T17:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stepan Kasal <kasal@ucw.cz> writes:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Date: Mon, 8 Nov 2010 16:10:43 +0100\n>\n> Incidentally, this makes grep -I respect the \"binary\" attribute (actually,\n> the \"-text\" attribute, but \"binary\" implies that).\n>\n> Since the attributes are not thread-safe, we now need to switch off\n> threading if -I was passed.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: Stepan Kasal <kasal@ucw.cz>\n> ---\n>\n> Hi,\n> this patch has been in msysgit for 3.5 years.\n> Stepan\n\nI do not think checking 'text' is the right way to do this.  The\nattribute controls the eof normalization, and people sometimes want\nto keep CRLF terminated files in the repository no matter what the\nplatform is (an example I heard is a sample protocol exchange over\ntextual network protocol such as HTTP and SMTP), and the way to do\nso is to unset it.  That would still let them look for patterns in\nand compare the changes to these paths.\n\nLooking for \"Marking files as binary\" in gitattributes(5) gives us a\nmore authoritative alternative, I think.  In short:\n\n - If 'diff' is Unset, or\n - If 'diff' is Set to X and diff.X.binary is true\n\nthen the contents are not suitable for human consumption.  I haven't\nthought things through to declare that \"grep -I\" would want to treat\na PostScript file (which is an example in that section) as a binary\nfile and refrain from trying to find substrings in it, but my gut\nfeeling is that we probably should let it look inside such a file,\nso replacing 'text' with 'diff' in this patch and doing nothing\nabout diff.*.binary would be the way to go.  Given that the posted\npatch was older than 3.5 years, perhaps it needs updating to adhere\nthe advice given in ab435611 (docs: explain diff.*.binary option,\n2011-01-09).\n\nBy the way, I wonder if the patch is about performance, implied by\n\"do not bother\" (to waste time looking inside), though.  Is it an\noverall win to avoid looking inside \"-diff\" files, if that requires\nyou to use only 1 core and leaving the other 7 idle?\n\nI think \"the user tells us it is binary with attribute, and the user\ntells us not to look into binary with -I\" is a lot better rationale\nto do this change , and that would characterise it as a correctness\npatch, not a performance patch.\n\nThanks.\n\n\n>  builtin/grep.c | 25 +++++++++++++++++++++++++\n>  1 file changed, 25 insertions(+)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 43af5b7..8073fbe 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -18,6 +18,7 @@\n>  #include \"quote.h\"\n>  #include \"dir.h\"\n>  #include \"pathspec.h\"\n> +#include \"attr.h\"\n>  \n>  static char const * const grep_usage[] = {\n>  \tN_(\"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]\"),\n> @@ -163,6 +164,22 @@ static void work_done(struct work_item *w)\n>  \tgrep_unlock();\n>  }\n>  \n> +static int skip_binary(struct grep_opt *opt, const char *filename)\n> +{\n> +\tif ((opt->binary & GREP_BINARY_NOMATCH)) {\n> +\t\tstatic struct git_attr *attr_text;\n> +\t\tstruct git_attr_check check;\n> +\n> +\t\tif (!attr_text)\n> +\t\t\tattr_text = git_attr(\"text\");\n> +\t\tmemset(&check, 0, sizeof(check));\n> +\t\tcheck.attr = attr_text;\n> +\t\treturn !git_check_attr(filename, 1, &check) &&\n> +\t\t\t\tATTR_FALSE(check.value);\n> +\t}\n> +\treturn 0;\n> +}\n> +\n>  static void *run(void *arg)\n>  {\n>  \tint hit = 0;\n> @@ -173,6 +190,9 @@ static void *run(void *arg)\n>  \t\tif (!w)\n>  \t\t\tbreak;\n>  \n> +\t\tif (skip_binary(opt, (const char *)w->source.identifier))\n> +\t\t\tcontinue;\n> +\n>  \t\topt->output_priv = w;\n>  \t\thit |= grep_source(opt, &w->source);\n>  \t\tgrep_source_clear_data(&w->source);\n> @@ -379,6 +399,9 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n>  \t\t\tcontinue;\n>  \t\tif (!ce_path_match(ce, pathspec, NULL))\n>  \t\t\tcontinue;\n> +\t\tif (skip_binary(opt, ce->name))\n> +\t\t\tcontinue;\n> +\n>  \t\t/*\n>  \t\t * If CE_VALID is on, we assume worktree file and its cache entry\n>  \t\t * are identical, even if worktree file has been modified, so use\n> @@ -803,6 +826,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\tstring_list_append(&path_list, show_in_pager);\n>  \t\tuse_threads = 0;\n>  \t}\n> +\tif ((opt.binary & GREP_BINARY_NOMATCH))\n> +\t\tuse_threads = 0;\n>  \n>  \tif (!opt.pattern_list)\n>  \t\tdie(_(\"no pattern given.\"));\n> -- \n> 1.9.2.msysgit.0.335.gd2a461f\n>\n> -- \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241526","messageId":"20140514194128.GC2715@sigill.intra.peff.net","threadId":"36655","inReplyTo":"20140514154419.GA4517@camelia.ucw.cz","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-14T19:41:29Z","receivedAt":"2014-05-14T19:41:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 14, 2014 at 05:44:19PM +0200, Stepan Kasal wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Date: Mon, 8 Nov 2010 16:10:43 +0100\n> \n> Incidentally, this makes grep -I respect the \"binary\" attribute (actually,\n> the \"-text\" attribute, but \"binary\" implies that).\n> \n> Since the attributes are not thread-safe, we now need to switch off\n> threading if -I was passed.\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: Stepan Kasal <kasal@ucw.cz>\n> ---\n> \n> Hi,\n> this patch has been in msysgit for 3.5 years.\n> Stepan\n\nHrm. Is this patch still necessary? In the time since this patch was\nwritten, we did 0826579 (grep: load file data after checking\nbinary-ness, 2012-02-02), which should do the same thing. It deals with\nthe threading via a lock, but we later learned in 9dd5245 (grep:\npre-load userdiff drivers when threaded, 2012-02-02) to hoist that bit\nout.\n\nSo I suspect this patch at best is doing nothing, and at worst is\nwasting extra time doing redundant attribute checks.\n\n-Peff\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241527","messageId":"20140514194442.GD2715@sigill.intra.peff.net","threadId":"36655","inReplyTo":"xmqqbnv0l02r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-14T19:44:42Z","receivedAt":"2014-05-14T19:44:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 14, 2014 at 10:52:28AM -0700, Junio C Hamano wrote:\n\n> I do not think checking 'text' is the right way to do this.  The\n> attribute controls the eof normalization, and people sometimes want\n> to keep CRLF terminated files in the repository no matter what the\n> platform is (an example I heard is a sample protocol exchange over\n> textual network protocol such as HTTP and SMTP), and the way to do\n> so is to unset it.  That would still let them look for patterns in\n> and compare the changes to these paths.\n> \n> Looking for \"Marking files as binary\" in gitattributes(5) gives us a\n> more authoritative alternative, I think.  In short:\n> \n>  - If 'diff' is Unset, or\n>  - If 'diff' is Set to X and diff.X.binary is true\n>\n> then the contents are not suitable for human consumption.\n\nI responded elsewhere in the thread that I think the patch under\ndiscussion is redundant at this point, but just to clarify: grep\ncurrently uses the rules you give above, as it builds on the userdiff\ndriver code.\n\n-Peff\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241538","messageId":"xmqqy4y4hxj5.fsf@gitster.dls.corp.google.com","threadId":"36655","inReplyTo":"20140514194128.GC2715@sigill.intra.peff.net","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-14T21:15:42Z","receivedAt":"2014-05-14T21:15:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, May 14, 2014 at 05:44:19PM +0200, Stepan Kasal wrote:\n>\n>> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> Date: Mon, 8 Nov 2010 16:10:43 +0100\n>> \n>> Incidentally, this makes grep -I respect the \"binary\" attribute (actually,\n>> the \"-text\" attribute, but \"binary\" implies that).\n>> \n>> Since the attributes are not thread-safe, we now need to switch off\n>> threading if -I was passed.\n>> \n>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> Signed-off-by: Stepan Kasal <kasal@ucw.cz>\n>> ---\n>> \n>> Hi,\n>> this patch has been in msysgit for 3.5 years.\n>> Stepan\n>\n> Hrm. Is this patch still necessary? In the time since this patch was\n> written, we did 0826579 (grep: load file data after checking\n> binary-ness, 2012-02-02), which should do the same thing. It deals with\n> the threading via a lock, but we later learned in 9dd5245 (grep:\n> pre-load userdiff drivers when threaded, 2012-02-02) to hoist that bit\n> out.\n\nWow, power of Git history ;-)\n\n> So I suspect this patch at best is doing nothing, and at worst is\n> wasting extra time doing redundant attribute checks.\n\nSounds like a sensible conclusion.  Thanks.\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241750","messageId":"alpine.DEB.1.00.1405151940170.14982@s15462909.onlinehome-server.info","threadId":"36655","inReplyTo":"20140514194128.GC2715@sigill.intra.peff.net","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-05-15T17:42:00Z","receivedAt":"2014-05-15T17:42:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 14 May 2014, Jeff King wrote:\n\n> On Wed, May 14, 2014 at 05:44:19PM +0200, Stepan Kasal wrote:\n> \n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > Date: Mon, 8 Nov 2010 16:10:43 +0100\n> > \n> > Incidentally, this makes grep -I respect the \"binary\" attribute (actually,\n> > the \"-text\" attribute, but \"binary\" implies that).\n> \n> Hrm. Is this patch still necessary? In the time since this patch was\n> written, we did 0826579 (grep: load file data after checking\n> binary-ness, 2012-02-02)\n\nI have no time to test this but I trust that you made sure that it works\nas advertised. In my case, there were about 500 gigabytes of image data\nintermixed with code, and waiting for 'git grep' was not funny at all (and\nI did not have time back then to go through a full code submission cycle\non the Git mailing list, either).\n\nSo I guess we can drop my patch.\n\nCiao,\nJohannes\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241770","messageId":"20140515192226.GC29746@sigill.intra.peff.net","threadId":"36655","inReplyTo":"alpine.DEB.1.00.1405151940170.14982@s15462909.onlinehome-server.info","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T19:22:26Z","receivedAt":"2014-05-15T19:22:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2014 at 07:42:00PM +0200, Johannes Schindelin wrote:\n\n> > Hrm. Is this patch still necessary? In the time since this patch was\n> > written, we did 0826579 (grep: load file data after checking\n> > binary-ness, 2012-02-02)\n> \n> I have no time to test this but I trust that you made sure that it works\n> as advertised. In my case, there were about 500 gigabytes of image data\n> intermixed with code, and waiting for 'git grep' was not funny at all (and\n> I did not have time back then to go through a full code submission cycle\n> on the Git mailing list, either).\n> \n> So I guess we can drop my patch.\n\nCertainly I tested it at the time (those commits I referenced contain\ntiming information), and it should have improved the workload you\ndescribe. I did not test before/after the patch in this thread, but only\nread it and noticed that it was trying to do the same thing (that is why\nI said \"I suspect...\").\n\nAs the person who is proposing the patch for git.git, I would hope\nStepan would follow up on such review and confirm whether or not it is\nstill needed.\n\n-Peff\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241857","messageId":"20140516081957.GA19845@camelia.ucw.cz","threadId":"36655","inReplyTo":"20140515192226.GC29746@sigill.intra.peff.net","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2014-05-16T08:19:57Z","receivedAt":"2014-05-16T08:19:57Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hello,\n\nOn Thu, May 15, 2014 at 03:22:26PM -0400, Jeff King wrote:\n> As the person who is proposing the patch for git.git, I would hope\n> Stepan would follow up on such review and confirm whether or not it is\n> still needed.\n\nwell, I try to.  (I verified that \"less -I\" works in msysGit before\nsubmitting the fixup back there, fox example.)\n\nBut I think my role is to moderate the reconciliation.\nIn this case, having read the discussion, I decided to to ask Dscho\nto drop the patch.\n\n(It is always about balancing the risks and the expenses.)\n\nStepan\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241860","messageId":"20140516082958.GA21956@sigill.intra.peff.net","threadId":"36655","inReplyTo":"20140516081957.GA19845@camelia.ucw.cz","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-16T08:29:58Z","receivedAt":"2014-05-16T08:29:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 16, 2014 at 10:19:57AM +0200, Stepan Kasal wrote:\n\n> On Thu, May 15, 2014 at 03:22:26PM -0400, Jeff King wrote:\n> > As the person who is proposing the patch for git.git, I would hope\n> > Stepan would follow up on such review and confirm whether or not it is\n> > still needed.\n> \n> well, I try to.  (I verified that \"less -I\" works in msysGit before\n> submitting the fixup back there, fox example.)\n> \n> But I think my role is to moderate the reconciliation.\n> In this case, having read the discussion, I decided to to ask Dscho\n> to drop the patch.\n> \n> (It is always about balancing the risks and the expenses.)\n\nSure, I think that is reasonable, and I hope I did not sound like \"blame\nStepan, he was screwed up\". After reading Dscho's other message and\nknowing that he has not much time for git, I was trying to communicate\nthat I did not expect _him_ to be dealing with it.\n\nGit.git has existed for a long time without that patch, so from our\nperspective, it is a new patch. And as with any new patch, I would\nexpect a submitter who receives review of \"eh, don't we already do this\"\nto either look into it, or decide that the review is probably right and\nnot bother with it. And you did the latter, and that is totally fine and\nreasonable.\n\n>From msysgit's perspective, they may or may not want to revert the patch\nthat they already have. That is a _separate_ issue, and I think the\nburden of effort goes the other way. The patch should stay unless\nsomebody goes to the work to confirm that it is not necessary (or unless\nthey want to accept my say-so and indication that I did not do fresh\ntesting, but that is up to them).\n\nSorry if that is long and/or obvious. There have been enough bad\nfeelings on the list lately that I didn't want there to be a\nmisunderstanding about what I meant.\n\n-Peff\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"241868","messageId":"20140516091641.GC19845@camelia.ucw.cz","threadId":"36655","inReplyTo":"20140516082958.GA21956@sigill.intra.peff.net","subject":"Re: [PATCH] grep -I: do not bother to read known-binary files","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2014-05-16T09:16:41Z","receivedAt":"2014-05-16T09:16:41Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hi,\n\nOn Fri, May 16, 2014 at 04:29:58AM -0400, Jeff King wrote:\n> [..] I hope I did not sound like \"blame Stepan, he was screwed up\".\n\nno, you did not, it was ok.\n\n> From msysgit's perspective, they may or may not want to revert the patch\n> that they already have. That is a _separate_ issue, and I think the\n\nI hope Dscho will help with the decision: he can say \"keep it until\nwe have evidence or at least time to do a more thorough review,\" for\nexample.\n\nStepan\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"}]}