{"thread":{"id":"27235","subject":"[PATCH] grep: add known breakage of coloring when using extended patterns","startedAt":"2011-05-02T11:35:40Z","lastAt":"2011-05-03T17:36:39Z","messageCount":8,"participants":["Bert Wesarg","Johannes Sixt","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"166846","messageId":"328649cbd9fe7f2ee5f43d1e860d712f4204bdc5.1304333975.git.bert.wesarg@googlemail.com","threadId":"27235","inReplyTo":null,"subject":"[PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2011-05-02T11:35:40Z","receivedAt":"2011-05-02T11:35:40Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"The coloring code does not respect the extended pattern format, expose\nthis with an known breakage test.\n\nSigned-off-by: Bert Wesarg \n---\n t/t7812-grep-color.sh | 25 +++++++++++++++++++++++++\n 1 files changed, 25 insertions(+), 0 deletions(-)\n create mode 100755 t/t7812-grep-color.sh\n\ndiff --git a/t/t7812-grep-color.sh b/t/t7812-grep-color.sh\nnew file mode 100755\nindex 0000000..2cffff3\n--- /dev/null\n+++ b/t/t7812-grep-color.sh\n@@ -0,0 +1,25 @@\n+#!/bin/sh\n+\n+test_description='git grep --color\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial input \"foo bar baz\n+bar baz\n+\"\n+'\n+\n+cat >expected <:foo bar baz\n+EOF\n+\n+\n+test_expect_failure 'grep coloring does not honor boolean algebra' '\n+\tgit grep --color -e foo --or \\( -e bar --and --not -e baz \\) |\n+\t\ttest_decode_color >actual\n+ test_cmp expected actual\n+'\n+\n+test_done\n-- \n1.7.5.349.gfeb1a\n"},{"id":"166852","messageId":"4DBE9859.8030408@viscovery.net","threadId":"27235","inReplyTo":"328649cbd9fe7f2ee5f43d1e860d712f4204bdc5.1304333975.git.bert.wesarg@googlemail.com","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-05-02T11:41:13Z","receivedAt":"2011-05-02T11:41:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/2/2011 13:35, schrieb Bert Wesarg:\n> +test_expect_failure 'grep coloring does not honor boolean algebra' '\n\nI'm sure you meant to say 'grep coloring honors boolean algebra', no?\n\n-- Hannes\n"},{"id":"166853","messageId":"BANLkTik+ARHyjbJLq-nfVh6gOSr1i=ANEw@mail.gmail.com","threadId":"27235","inReplyTo":"4DBE9859.8030408@viscovery.net","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2011-05-02T11:48:45Z","receivedAt":"2011-05-02T11:48:45Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Mon, May 2, 2011 at 13:41, Johannes Sixt wrote:\n> Am 5/2/2011 13:35, schrieb Bert Wesarg:\n>> +test_expect_failure 'grep coloring does not honor boolean algebra' '\n>\n> I'm sure you meant to say 'grep coloring honors boolean algebra', no?\n\nYou're right, I'm not so good in double-negations.\n\nBert\n\n>\n> -- Hannes\n>\n"},{"id":"166880","messageId":"4DBEE672.5070707@lsrfire.ath.cx","threadId":"27235","inReplyTo":"328649cbd9fe7f2ee5f43d1e860d712f4204bdc5.1304333975.git.bert.wesarg@googlemail.com","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-05-02T17:14:26Z","receivedAt":"2011-05-02T17:14:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.05.2011 13:35, schrieb Bert Wesarg:\n> The coloring code does not respect the extended pattern format, expose\n> this with an known breakage test.\n>\n> Signed-off-by: Bert Wesarg\n> ---\n> t/t7812-grep-color.sh | 25 +++++++++++++++++++++++++\n> 1 files changed, 25 insertions(+), 0 deletions(-)\n> create mode 100755 t/t7812-grep-color.sh\n>\n> diff --git a/t/t7812-grep-color.sh b/t/t7812-grep-color.sh\n> new file mode 100755\n> index 0000000..2cffff3\n> --- /dev/null\n> +++ b/t/t7812-grep-color.sh\n> @@ -0,0 +1,25 @@\n> +#!/bin/sh\n> +\n> +test_description='git grep --color\n> +'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit initial input \"foo bar baz\n> +bar baz\n> +\"\n> +'\n> +\n> +cat>expected< +input:foo bar baz\n> +EOF\n> +\n> +\n> +test_expect_failure 'grep coloring does not honor boolean algebra' '\n> +\tgit grep --color -e foo --or \\( -e bar --and --not -e baz \\) |\n> +\t\ttest_decode_color>actual\n> + test_cmp expected actual\n> +'\n> +\n> +test_done\n\nThe current code highlights the given search terms (\"atoms\"). You \nexpect the matching part of a line to be highlighted instead. Another \nexample with differing results between the two ways is this:\n\n\t$ git grep -v -e foo --and -e bar\n\nThe first one was easy to implement and useful already. I wonder how to \nimplement the second one (I couldn't think of an easy way back then) and \nalso if we really prefer it to the current behaviour in the first place.\n\nThe twisted logic that convinced me to forget about the issue goes like \nthis: Even by giving negative search terms, the user is expressing a \ncertain value of interest in them; highlighting these atoms even in \nlines which were not matched by them is a courtesy.\n\nRené\n"},{"id":"166882","messageId":"7vd3k1ngun.fsf@alter.siamese.dyndns.org","threadId":"27235","inReplyTo":"4DBEE672.5070707@lsrfire.ath.cx","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T17:37:52Z","receivedAt":"2011-05-02T17:37:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> +\ttest_commit initial input \"foo bar baz\n...\n>> +\tgit grep --color -e foo --or \\( -e bar --and --not -e baz \\) |\n...\n> The current code highlights the given search terms (\"atoms\").\n\nHmm, I was probably not paying attention to \"coloring the parts that\nmatched\" topic at all, but wouldn't it be easier and more efficient to\npaint only \"foo\" without painting \"bar baz\"? We know the first term \"foo\"\nmatches, and the rest \\(...\\) that is --or'ed does not have to even be\nevaluated, no?\n"},{"id":"166909","messageId":"4DBF1D2A.6030807@lsrfire.ath.cx","threadId":"27235","inReplyTo":"7vd3k1ngun.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-05-02T21:07:54Z","receivedAt":"2011-05-02T21:07:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.05.2011 19:37, schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>> +\ttest_commit initial input \"foo bar baz\n> ...\n>>> +\tgit grep --color -e foo --or \\( -e bar --and --not -e baz \\) |\n> ...\n>> The current code highlights the given search terms (\"atoms\").\n>\n> Hmm, I was probably not paying attention to \"coloring the parts that\n> matched\" topic at all, but wouldn't it be easier and more efficient to\n> paint only \"foo\" without painting \"bar baz\"? We know the first term \"foo\"\n> matches, and the rest \\(...\\) that is --or'ed does not have to even be\n> evaluated, no?\n\nAggregating the set of matching characters and passing them back during \nexpression evaluation can be more efficient, yes, as it would avoid \ncalling regexec() on the printed lines again only to find out what to \ncolor. But I wouldn't call it easy. E.g. how to return the set of \nmatching characters in the following case?\n\n\t$ git grep --color -e foo --and -e bar\n\nI can only think of using lists or, even uglier, perhaps a bitmap to \nremember which characters to highlight.\n\nAlso GNU grep doesn't only color the first sufficient match. E.g. this \nwill color both b and d:\n\n\t$ echo abcde | grep --color -e b -e d\n\nAnd I think this makes sense and meets my expectations.\n\nBut the other mode, which only colors the exact characters that were \nused to determine that the shown line matches (or doesn't match, if -v \nis given) may be interesting as well. I'm curious to see an \nimplementation and wonder if the results are really better than the ones \nof the current (cheating) approach. :)\n\nRené\n"},{"id":"166911","messageId":"7vwri8kbcy.fsf@alter.siamese.dyndns.org","threadId":"27235","inReplyTo":"4DBF1D2A.6030807@lsrfire.ath.cx","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-02T22:04:45Z","receivedAt":"2011-05-02T22:04:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Aggregating the set of matching characters and passing them back\n> during expression evaluation can be more efficient, yes, as it would\n> avoid calling regexec() on the printed lines again only to find out\n> what to color. But I wouldn't call it easy. E.g. how to return the\n> set of matching characters in the following case?\n>\n> \t$ git grep --color -e foo --and -e bar\n\nI would naïvely expect each of the \"grep_expr\" in the parsed grep tree to\nhave not just a one-bit \"hit\", but rm_so/rm_eo pair, but as I said, I was\nasleep while \"coloring the parts that matched\" discussion was in progress\n;-)\n\nBut\n\n> Also GNU grep doesn't only color the first sufficient match.\n> E.g. this will color both b and d:\n>\n> \t$ echo abcde | grep --color -e b -e d\n\nthat is probably a useful thing to have. Obviously, my suggestion should\noptimize away the match with 'd' to begin with, so keeping rm_so/rm_eo\npair would not help painting d at all. Worse yet, I would expect that\n\n\t$ echo abcdeabcde | grep --color -e b -e d\n\nwould need to paint all occurrences for such a purpose, so obviously you\nwould need an extra pass to find all potential hits. Ugly and makes me\npersonally wonder if it is really worth it, but the user is explicitly\nasking for --color and is prepared to pay the price, so it probably is Ok.\n"},{"id":"166950","messageId":"4DC03D27.80803@lsrfire.ath.cx","threadId":"27235","inReplyTo":"7vwri8kbcy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: add known breakage of coloring when using extended patterns","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-05-03T17:36:39Z","receivedAt":"2011-05-03T17:36:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.05.2011 00:04, schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Aggregating the set of matching characters and passing them back\n>> during expression evaluation can be more efficient, yes, as it would\n>> avoid calling regexec() on the printed lines again only to find out\n>> what to color. But I wouldn't call it easy. E.g. how to return the\n>> set of matching characters in the following case?\n>>\n>> \t$ git grep --color -e foo --and -e bar\n> \n> I would naïvely expect each of the \"grep_expr\" in the parsed grep tree to\n> have not just a one-bit \"hit\", but rm_so/rm_eo pair, but as I said, I was\n> asleep while \"coloring the parts that matched\" discussion was in progress\n> ;-)\n\nThat's true, but each one represents a single contiguous run of\ncharacters to color. But I meant to say that returning non-contiguous\nranges, as you can get in the --and example above, is not that easy. To me.\n\nHmm, if one just wants to color the parts that triggered a match it gets\na lot easier. Each pattern just needs to remember the last matching\noffsets and then show_line can walk the expression tree to find out what\nneeds coloring. Not sure about the details, though. :)\n\n> But\n> \n>> Also GNU grep doesn't only color the first sufficient match.\n>> E.g. this will color both b and d:\n>>\n>> \t$ echo abcde | grep --color -e b -e d\n> \n> that is probably a useful thing to have. Obviously, my suggestion should\n> optimize away the match with 'd' to begin with, so keeping rm_so/rm_eo\n> pair would not help painting d at all. Worse yet, I would expect that\n> \n> \t$ echo abcdeabcde | grep --color -e b -e d\n> \n> would need to paint all occurrences for such a purpose, so obviously you\n> would need an extra pass to find all potential hits. Ugly and makes me\n> personally wonder if it is really worth it, but the user is explicitly\n> asking for --color and is prepared to pay the price, so it probably is Ok.\n\nThe performance impact is low as long as there are lots more lines that\ndon't match than matching ones. Which should be a given for most uses\nof grep.\n\nI always miss hit coloring if I have to use an environment that doesn't\nprovide , similarly to syntax highlighting, so I do think it's worth it,\nfor me at least.\n\nRené\n"}]}