[PATCH] Bugfix: grep: Do not colorize output when -O is set

When color.ui is set to auto, "git grep -Ovi foo" breaks due to the
presence of color escape sequences.

Signed-off-by: Nazri Ramliy <redacted>
---
Breakage aside, 'git grep -Ovi' really rocks!

 builtin/grep.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 232cd1c..597f76b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -1001,6 +1001,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (show_in_pager == default_pager)
 		show_in_pager = git_pager(1);
 	if (show_in_pager) {
+		opt.color = 0;
 		opt.name_only = 1;
 		opt.null_following_name = 1;
 		opt.output_priv = &path_list;

-- 
1.7.1.245.g7c42e.dirty

Am 02.07.2010 12:02, schrieb Nazri Ramliy:

When color.ui is set to auto, "git grep -Ovi foo" breaks due to the
presence of color escape sequences.

Hmm, but with --open-files-in-pager without argument or -Oless colours
may be handled correctly and desirable.  Turning colouring off with -O
is probably the most sensible default, but is it possible to allow
turning it back on explicitly (--color -O)?

René

Hi Nazri,

Nazri Ramliy wrote:

When color.ui is set to auto, "git grep -Ovi foo" breaks due to the
presence of color escape sequences.

I tried the following test without your patch, and it seemed to pass
without trouble.  What am I doing wrong?

diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
index c110441..d47c054 100755
--- a/t/t7811-grep-open.sh
+++ b/t/t7811-grep-open.sh

@@ -125,6 +125,24 @@ test_expect_success 'modified file' '
 	test_cmp empty out
 '
 
+test_expect_success 'copes with color.ui' '
+	rm -f actual &&
+	echo grep.h >expect &&
+	git config color.ui always &&
+	test_when_finished "git config --unset color.ui" &&
+	git grep -O'\''printf "%s\n" >actual'\'' GREP_AND &&
+	test_cmp expect actual
+'
+
+test_expect_success 'copes with color.grep' '
+	rm -f actual &&
+	echo grep.h >expect &&
+	git config color.grep always &&
+	test_when_finished "git config --unset color.grep" &&
+	git grep -O'\''printf "%s\n" >actual'\'' GREP_AND &&
+	test_cmp expect actual
+'
+
 test_expect_success 'run from subdir' '
 	rm -f actual &&
 	echo grep.c >expect &&

On Sat, Jul 3, 2010 at 3:21 AM, Jonathan Nieder [off-list ref] wrote:

Hi Nazri,

Nazri Ramliy wrote:

quoted

When color.ui is set to auto, "git grep -Ovi foo" breaks due to the
presence of color escape sequences.

I tried the following test without your patch, and it seemed to pass
without trouble.  What am I doing wrong?

Sorry for not being more specific about the breakage. "color.ui" is not
enough to trigger the breakage.

You'll have to set "color.grep.filename" too in order to break the two
test cases.

Something like the following will do (I'm doing this in gmail so sorry
for any tabs<->space conversion):

test_expect_success 'copes with color.ui' '
       rm -f actual &&
       echo grep.h >expect &&
       git config color.ui always &&
       git config color.grep.filename yellow &&
       test_when_finished "git config --unset color.ui" &&
       test_when_finished "git config --unset color.grep.filename" &&
       git grep -O'\''printf "%s\n" >actual'\'' GREP_AND &&
       test_cmp expect actual
'

test_expect_success 'copes with color.grep' '
       rm -f actual &&
       echo grep.h >expect &&
       git config color.grep always &&
       git config color.grep.filename yellow &&
       test_when_finished "git config --unset color.grep" &&
       test_when_finished "git config --unset color.grep.filename" &&
       git grep -O'\''printf "%s\n" >actual'\'' GREP_AND &&
       test_cmp expect actual
'

nazri.

From: Nazri Ramliy <redacted>

With a .gitconfig like this:

 [color]
	ui = auto
 [color "grep"]
	filename = magenta

if stdout is a terminal, the grep machinery will output the color
sequence \e[36m before each filename in its output.

In the case of "git grep -O foo", output is argv for the pager.
Disable color when calling the grep machinery in this case.

Signed-off-by: Nazri Ramliy <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Nazri Ramliy wrote:

You'll have to set "color.grep.filename" too in order to break the two
test cases.

Thanks.

 builtin/grep.c       |    1 +
 t/t7811-grep-open.sh |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 232cd1c..597f76b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -1001,6 +1001,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (show_in_pager == default_pager)
 		show_in_pager = git_pager(1);
 	if (show_in_pager) {
+		opt.color = 0;
 		opt.name_only = 1;
 		opt.null_following_name = 1;
 		opt.output_priv = &path_list;

diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
index c110441..568a6f2 100755
--- a/t/t7811-grep-open.sh
+++ b/t/t7811-grep-open.sh

@@ -125,6 +125,21 @@ test_expect_success 'modified file' '
 	test_cmp empty out
 '
 
+test_config() {
+	git config "$1" "$2" &&
+	test_when_finished "git config --unset $1"
+}
+
+test_expect_success 'copes with color settings' '
+	rm -f actual &&
+	echo grep.h >expect &&
+	test_config color.grep always &&
+	test_config color.grep.filename yellow &&
+	test_config color.grep.separator green &&
+	git grep -O'\''printf "%s\n" >actual'\'' GREP_AND &&
+	test_cmp expect actual
+'
+
 test_expect_success 'run from subdir' '
 	rm -f actual &&
 	echo grep.c >expect &&

-- 
1.7.1.1

Nazri Ramliy [off-list ref] writes:
 

Something like the following will do (I'm doing this in gmail so sorry
for any tabs<->space conversion):

test_expect_success 'copes with color.ui' '
       rm -f actual &&
       echo grep.h >expect &&
       git config color.ui always &&
       git config color.grep.filename yellow &&
       test_when_finished "git config --unset color.ui" &&
       test_when_finished "git config --unset color.grep.filename" &&
       git grep -O'\''printf "%s\n" >actual'\'' GREP_AND &&
       test_cmp expect actual
'

Sidenote: test_when_finished, introduced by Jonathan Nieder in 3bf7886
(test-lib: Let tests specify commands to be run at end of test,
2010-05-02) is not documented in t/README.  Also, shouldn't it be
named 'when_finished_test' rather than 'test_when_finished'?

Currently 'test_when_finished' / 'when_finished_test' is used only in
t0000-basic and t7509-commit.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Hi,

René Scharfe wrote:

Hmm, but with --open-files-in-pager without argument or -Oless colours
may be handled correctly and desirable.

Sorry I missed this before.  Is there really a pager that will accept
\e[36m as a command-line argument and do something reasonable with it?

Turning colouring off with -O
is probably the most sensible default, but is it possible to allow
turning it back on explicitly (--color -O)?

A person trying that might be wanting to highlight matches in the
pager rather than in argv itself. :)  Unfortunately, it is not
completely obvious how to comply.

‘less’ already highlights matches by default, though not in the color
configured for git.  grep -O will tell ‘less’ what to look for if
there was just one pattern.

editors like vim tend to use syntax highlighting in addition to
optionally highlighting search matches.

Probably a better solution is to recommend -C option, possibly
implementing -C infinity so people don’t have to use -C 1000000.

But your point is well taken that the current behavior is confusing.
How about the following?

diff --git a/builtin/grep.c b/builtin/grep.c
index 7a9427d..921f554 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -835,6 +835,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct string_list path_list = { NULL, 0, 0, 0 };
 	int i;
 	int dummy;
+	int use_color = -1;
 	int nongit = 0, use_index = 1;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,

@@ -881,7 +882,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			"print NUL after filenames"),
 		OPT_BOOLEAN('c', "count", &opt.count,
 			"show the number of matches instead of matching lines"),
-		OPT__COLOR(&opt.color, "highlight matches"),
+		OPT__COLOR(&use_color, "highlight matches"),
 		OPT_GROUP(""),
 		OPT_CALLBACK('C', NULL, &opt, "n",
 			"show <n> context lines before and after matches",

@@ -994,6 +995,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		argc--;
 	}
 
+	if (use_color != -1)
+		opt.color = use_color;
+
 	if (show_in_pager == default_pager)
 		show_in_pager = git_pager(1);
 	if (show_in_pager) {

@@ -1006,6 +1010,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		use_threads = 0;
 	}
 
+	if (show_in_pager && use_color)
+		die("cannot mix -O and --color");
 	if (!opt.pattern_list)
 		die("no pattern given.");
 	if (!opt.fixed && opt.ignore_case)

There is no documentation in t/README for test_must_fail,
test_might_fail, test_cmp, or test_when_finished.

Reported-by: Jakub Narebski <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Jakub Narebski wrote:

Sidenote: test_when_finished, introduced by Jonathan Nieder in 3bf7886
(test-lib: Let tests specify commands to be run at end of test,
2010-05-02) is not documented in t/README.

Good catch.

Also, shouldn't it be
named 'when_finished_test' rather than 'test_when_finished'?

It uses the test_* name to avoid a land-grab by test-lib.sh for other
namespaces.

Currently 'test_when_finished' / 'when_finished_test' is used only in
t0000-basic and t7509-commit.

Right, I have some ideas for using this but it is hard to find time.

Thanks for the comments.  Patch is on top of 6fd4529 (t/README:
proposed rewording..., 2010-07-05) from 'next'.

 t/README |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 271f868..9df0ae9 100644
--- a/t/README
+++ b/t/README

@@ -448,6 +448,37 @@ library for your script to use.
 	    'Perl API' \
 	    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl
 
+ - test_must_fail <git-command>
+
+   Run a git command and ensure it fails in a controlled way.  Use
+   this instead of "! <git-command>" to fail when git commands
+   segfault.
+
+ - test_might_fail <git-command>
+
+   Similar to test_must_fail, but tolerate success, too.  Use this
+   instead of "<git-command> || :" to catch failures due to segv.
+
+ - test_cmp <expected> <actual>
+
+   Check whether the content of the <actual> file matches the
+   <expected> file.  This behaves like "cmp" but produces more
+   helpful output.
+
+ - test_when_finished <script>
+
+   Prepend <script> to a list of commands to run to clean up
+   at the end of the current test.  If some clean-up command
+   fails, the test will not pass.
+
+   Example:
+
+	test_expect_success 'branch pointing to non-commit' '
+		git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&
+		test_when_finished "git update-ref -d refs/heads/invalid" &&
+		...
+	'
+
 
 Tips for Writing Tests
 ----------------------

-- 
1.7.2.rc1

Am 06.07.2010 21:38, schrieb Jonathan Nieder:

Hi,

René Scharfe wrote:

quoted

Hmm, but with --open-files-in-pager without argument or -Oless colours
may be handled correctly and desirable.

Sorry I missed this before.  Is there really a pager that will accept
\e[36m as a command-line argument and do something reasonable with it?

I was missing that -O enforces -l, and that it makes the pager open all
files directly from the worktree, one by one.  Somehow I assumed that it
would pipe something like the output of "grep -h -C inf" to the pager,
colour marks and all -- similar to what is done without -O, except that
it would start a new pager for each file.

I think the "pager" part of the long option name confused me, but that's
a weak excuse.  Just ignore me, your original patch was fine.

[snip]

Probably a better solution is to recommend -C option, possibly
implementing -C infinity so people don’t have to use -C 1000000.

Hmm, that could be useful, also with -A and -B.

René

On Tue, Jul 6, 2010 at 20:04, Jonathan Nieder [off-list ref] wrote:

There is no documentation in t/README for test_must_fail,
test_might_fail, test_cmp, or test_when_finished.

Excellent, looks good.

Acked-by: Ævar Arnfjörð Bjarmason <redacted>