{"thread":{"id":"24263","subject":"[PATCH] Bugfix: grep: Do not colorize output when -O is set","startedAt":"2010-07-02T10:02:21Z","lastAt":"2010-07-07T04:25:21Z","messageCount":11,"participants":["Nazri Ramliy","René Scharfe","Jonathan Nieder","Jakub Narebski","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"144676","messageId":"1278064941-30689-1-git-send-email-ayiehere@gmail.com","threadId":"24263","inReplyTo":null,"subject":"[PATCH] Bugfix: grep: Do not colorize output when -O is set","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2010-07-02T10:02:21Z","receivedAt":"2010-07-02T10:02:21Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"When color.ui is set to auto, \"git grep -Ovi foo\" breaks due to the\npresence of color escape sequences.\n\nSigned-off-by: Nazri Ramliy \n---\nBreakage aside, 'git grep -Ovi' really rocks!\n\n builtin/grep.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 232cd1c..597f76b 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1001,6 +1001,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (show_in_pager == default_pager)\n \t\tshow_in_pager = git_pager(1);\n \tif (show_in_pager) {\n+\t\topt.color = 0;\n \t\topt.name_only = 1;\n \t\topt.null_following_name = 1;\n \t\topt.output_priv = &path_list;\n-- \n1.7.1.245.g7c42e.dirty\n"},{"id":"144694","messageId":"4C2E1185.1040406@lsrfire.ath.cx","threadId":"24263","inReplyTo":"1278064941-30689-1-git-send-email-ayiehere@gmail.com","subject":"Re: [PATCH] Bugfix: grep: Do not colorize output when -O is set","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-07-02T16:19:17Z","receivedAt":"2010-07-02T16:19:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.07.2010 12:02, schrieb Nazri Ramliy:\n> When color.ui is set to auto, \"git grep -Ovi foo\" breaks due to the\n> presence of color escape sequences.\n\nHmm, but with --open-files-in-pager without argument or -Oless colours\nmay be handled correctly and desirable. Turning colouring off with -O\nis probably the most sensible default, but is it possible to allow\nturning it back on explicitly (--color -O)?\n\nRené\n"},{"id":"144708","messageId":"20100702192102.GA6585@burratino","threadId":"24263","inReplyTo":"1278064941-30689-1-git-send-email-ayiehere@gmail.com","subject":"Re: [PATCH] Bugfix: grep: Do not colorize output when -O is set","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-02T19:21:02Z","receivedAt":"2010-07-02T19:21:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Nazri,\n\nNazri Ramliy wrote:\n\n> When color.ui is set to auto, \"git grep -Ovi foo\" breaks due to the\n> presence of color escape sequences.\n\nI tried the following test without your patch, and it seemed to pass\nwithout trouble. What am I doing wrong?\n\ndiff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh\nindex c110441..d47c054 100755\n--- a/t/t7811-grep-open.sh\n+++ b/t/t7811-grep-open.sh\n@@ -125,6 +125,24 @@ test_expect_success 'modified file' '\n \ttest_cmp empty out\n '\n \n+test_expect_success 'copes with color.ui' '\n+\trm -f actual &&\n+\techo grep.h >expect &&\n+\tgit config color.ui always &&\n+\ttest_when_finished \"git config --unset color.ui\" &&\n+\tgit grep -O'\\''printf \"%s\\n\" >actual'\\'' GREP_AND &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'copes with color.grep' '\n+\trm -f actual &&\n+\techo grep.h >expect &&\n+\tgit config color.grep always &&\n+\ttest_when_finished \"git config --unset color.grep\" &&\n+\tgit grep -O'\\''printf \"%s\\n\" >actual'\\'' GREP_AND &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'run from subdir' '\n \trm -f actual &&\n \techo grep.c >expect &&\n"},{"id":"144729","messageId":"AANLkTilI0NZiDk3I850x28pr5I0sYRiPLW7HAST9sduU@mail.gmail.com","threadId":"24263","inReplyTo":"20100702192102.GA6585@burratino","subject":"Re: [PATCH] Bugfix: grep: Do not colorize output when -O is set","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2010-07-03T01:20:05Z","receivedAt":"2010-07-03T01:20:05Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"On Sat, Jul 3, 2010 at 3:21 AM, Jonathan Nieder wrote:\n> Hi Nazri,\n>\n> Nazri Ramliy wrote:\n>\n>> When color.ui is set to auto, \"git grep -Ovi foo\" breaks due to the\n>> presence of color escape sequences.\n>\n> I tried the following test without your patch, and it seemed to pass\n> without trouble.  What am I doing wrong?\n\nSorry for not being more specific about the breakage. \"color.ui\" is not\nenough to trigger the breakage.\n\nYou'll have to set \"color.grep.filename\" too in order to break the two\ntest cases.\n\nSomething like the following will do (I'm doing this in gmail so sorry\nfor any tabs<->space conversion):\n\ntest_expect_success 'copes with color.ui' '\n       rm -f actual &&\n       echo grep.h >expect &&\n       git config color.ui always &&\n git config color.grep.filename yellow &&\n       test_when_finished \"git config --unset color.ui\" &&\n test_when_finished \"git config --unset color.grep.filename\" &&\n       git grep -O'\\''printf \"%s\\n\" >actual'\\'' GREP_AND &&\n       test_cmp expect actual\n'\n\ntest_expect_success 'copes with color.grep' '\n       rm -f actual &&\n       echo grep.h >expect &&\n       git config color.grep always &&\n git config color.grep.filename yellow &&\n       test_when_finished \"git config --unset color.grep\" &&\n test_when_finished \"git config --unset color.grep.filename\" &&\n       git grep -O'\\''printf \"%s\\n\" >actual'\\'' GREP_AND &&\n       test_cmp expect actual\n'\n\nnazri.\n"},{"id":"144730","messageId":"20100703025506.GB20980@burratino","threadId":"24263","inReplyTo":"AANLkTilI0NZiDk3I850x28pr5I0sYRiPLW7HAST9sduU@mail.gmail.com","subject":"[PATCH v2] grep -O: Do not pass color sequences as filenames to pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-03T02:55:06Z","receivedAt":"2010-07-03T02:55:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nazri Ramliy \n\nWith a .gitconfig like this:\n\n [color]\n\tui = auto\n [color \"grep\"]\n\tfilename = magenta\n\nif stdout is a terminal, the grep machinery will output the color\nsequence \\e[36m before each filename in its output.\n\nIn the case of \"git grep -O foo\", output is argv for the pager.\nDisable color when calling the grep machinery in this case.\n\nSigned-off-by: Nazri Ramliy \nSigned-off-by: Jonathan Nieder \n---\nNazri Ramliy wrote:\n\n> You'll have to set \"color.grep.filename\" too in order to break the two\n> test cases.\n\nThanks.\n\n builtin/grep.c | 1 +\n t/t7811-grep-open.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 232cd1c..597f76b 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1001,6 +1001,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (show_in_pager == default_pager)\n \t\tshow_in_pager = git_pager(1);\n \tif (show_in_pager) {\n+\t\topt.color = 0;\n \t\topt.name_only = 1;\n \t\topt.null_following_name = 1;\n \t\topt.output_priv = &path_list;\ndiff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh\nindex c110441..568a6f2 100755\n--- a/t/t7811-grep-open.sh\n+++ b/t/t7811-grep-open.sh\n@@ -125,6 +125,21 @@ test_expect_success 'modified file' '\n \ttest_cmp empty out\n '\n \n+test_config() {\n+\tgit config \"$1\" \"$2\" &&\n+\ttest_when_finished \"git config --unset $1\"\n+}\n+\n+test_expect_success 'copes with color settings' '\n+\trm -f actual &&\n+\techo grep.h >expect &&\n+\ttest_config color.grep always &&\n+\ttest_config color.grep.filename yellow &&\n+\ttest_config color.grep.separator green &&\n+\tgit grep -O'\\''printf \"%s\\n\" >actual'\\'' GREP_AND &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'run from subdir' '\n \trm -f actual &&\n \techo grep.c >expect &&\n-- \n1.7.1.1\n"},{"id":"144733","messageId":"m3wrtdm1y9.fsf@localhost.localdomain","threadId":"24263","inReplyTo":"AANLkTilI0NZiDk3I850x28pr5I0sYRiPLW7HAST9sduU@mail.gmail.com","subject":"Re: [PATCH] Bugfix: grep: Do not colorize output when -O is set","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-07-03T07:59:24Z","receivedAt":"2010-07-03T07:59:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Nazri Ramliy writes:\n \n> Something like the following will do (I'm doing this in gmail so sorry\n> for any tabs<->space conversion):\n> \n> test_expect_success 'copes with color.ui' '\n>       rm -f actual &&\n>       echo grep.h >expect &&\n>       git config color.ui always &&\n> git config color.grep.filename yellow &&\n>       test_when_finished \"git config --unset color.ui\" &&\n> test_when_finished \"git config --unset color.grep.filename\" &&\n>       git grep -O'\\''printf \"%s\\n\" >actual'\\'' GREP_AND &&\n>       test_cmp expect actual\n> '\n\nSidenote: test_when_finished, introduced by Jonathan Nieder in 3bf7886\n(test-lib: Let tests specify commands to be run at end of test,\n2010-05-02) is not documented in t/README. Also, shouldn't it be\nnamed 'when_finished_test' rather than 'test_when_finished'?\n\nCurrently 'test_when_finished' / 'when_finished_test' is used only in\nt0000-basic and t7509-commit.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"144956","messageId":"20100706193845.GA7438@burratino","threadId":"24263","inReplyTo":"4C2E1185.1040406@lsrfire.ath.cx","subject":"Re: [PATCH] Bugfix: grep: Do not colorize output when -O is set","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-06T19:38:45Z","receivedAt":"2010-07-06T19:38:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRené Scharfe wrote:\n\n> Hmm, but with --open-files-in-pager without argument or -Oless colours\n> may be handled correctly and desirable.\n\nSorry I missed this before. Is there really a pager that will accept\n\\e[36m as a command-line argument and do something reasonable with it?\n\n> Turning colouring off with -O\n> is probably the most sensible default, but is it possible to allow\n> turning it back on explicitly (--color -O)?\n\nA person trying that might be wanting to highlight matches in the\npager rather than in argv itself. :) Unfortunately, it is not\ncompletely obvious how to comply.\n\n‘less’ already highlights matches by default, though not in the color\nconfigured for git. grep -O will tell ‘less’ what to look for if\nthere was just one pattern.\n\neditors like vim tend to use syntax highlighting in addition to\noptionally highlighting search matches.\n\nProbably a better solution is to recommend -C option, possibly\nimplementing -C infinity so people don’t have to use -C 1000000.\n\nBut your point is well taken that the current behavior is confusing.\nHow about the following?\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7a9427d..921f554 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -835,6 +835,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct string_list path_list = { NULL, 0, 0, 0 };\n \tint i;\n \tint dummy;\n+\tint use_color = -1;\n \tint nongit = 0, use_index = 1;\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n@@ -881,7 +882,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t\"print NUL after filenames\"),\n \t\tOPT_BOOLEAN('c', \"count\", &opt.count,\n \t\t\t\"show the number of matches instead of matching lines\"),\n-\t\tOPT__COLOR(&opt.color, \"highlight matches\"),\n+\t\tOPT__COLOR(&use_color, \"highlight matches\"),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_CALLBACK('C', NULL, &opt, \"n\",\n \t\t\t\"show context lines before and after matches\",\n@@ -994,6 +995,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\targc--;\n \t}\n \n+\tif (use_color != -1)\n+\t\topt.color = use_color;\n+\n \tif (show_in_pager == default_pager)\n \t\tshow_in_pager = git_pager(1);\n \tif (show_in_pager) {\n@@ -1006,6 +1010,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tuse_threads = 0;\n \t}\n \n+\tif (show_in_pager && use_color)\n+\t\tdie(\"cannot mix -O and --color\");\n \tif (!opt.pattern_list)\n \t\tdie(\"no pattern given.\");\n \tif (!opt.fixed && opt.ignore_case)\n"},{"id":"144958","messageId":"20100706200410.GA7606@burratino","threadId":"24263","inReplyTo":"m3wrtdm1y9.fsf@localhost.localdomain","subject":"[PATCH] t/README: document more test helpers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-06T20:04:10Z","receivedAt":"2010-07-06T20:04:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"There is no documentation in t/README for test_must_fail,\ntest_might_fail, test_cmp, or test_when_finished.\n\nReported-by: Jakub Narebski \nSigned-off-by: Jonathan Nieder \n---\nJakub Narebski wrote:\n\n> Sidenote: test_when_finished, introduced by Jonathan Nieder in 3bf7886\n> (test-lib: Let tests specify commands to be run at end of test,\n> 2010-05-02) is not documented in t/README.\n\nGood catch.\n\n> Also, shouldn't it be\n> named 'when_finished_test' rather than 'test_when_finished'?\n\nIt uses the test_* name to avoid a land-grab by test-lib.sh for other\nnamespaces.\n\n> Currently 'test_when_finished' / 'when_finished_test' is used only in\n> t0000-basic and t7509-commit.\n\nRight, I have some ideas for using this but it is hard to find time.\n\nThanks for the comments. Patch is on top of 6fd4529 (t/README:\nproposed rewording..., 2010-07-05) from 'next'.\n\n t/README | 31 +++++++++++++++++++++++++++++++\n 1 files changed, 31 insertions(+), 0 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 271f868..9df0ae9 100644\n--- a/t/README\n+++ b/t/README\n@@ -448,6 +448,37 @@ library for your script to use.\n \t 'Perl API' \\\n \t \"$PERL_PATH\" \"$TEST_DIRECTORY\"/t9700/test.pl\n \n+ - test_must_fail \n+\n+ Run a git command and ensure it fails in a controlled way. Use\n+ this instead of \"! \" to fail when git commands\n+ segfault.\n+\n+ - test_might_fail \n+\n+ Similar to test_must_fail, but tolerate success, too. Use this\n+ instead of \" || :\" to catch failures due to segv.\n+\n+ - test_cmp \n+\n+ Check whether the content of the file matches the\n+ file. This behaves like \"cmp\" but produces more\n+ helpful output.\n+\n+ - test_when_finished