threads / patch / 24263

patchBugfix: grep: Do not colorize output when -O is set

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

## tl;dr

11 messages between Jul 2, 2010 and Jul 7, 2010. Diffs are folded; open one to read it.

replies: 10people: 6as markdown or json

Nazri Ramliy· Jul 2, 2010, 10:02 UTC · lore

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 <ayiehere@gmail.com>
---
Breakage aside, 'git grep -Ovi' really rocks!
 builtin/grep.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to builtin/grep.c +1 −0
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
René Scharfe· Jul 2, 2010, 16:19 UTC · re: Nazri Ramliy · lore

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

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é
Jonathan Nieder· Jul 6, 2010, 19:38 UTC · re: René Scharfe · lore

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

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?

Show changes to builtin/grep.c +7 −1
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)
René Scharfe· Jul 6, 2010, 20:19 UTC · re: Jonathan Nieder · lore

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

Am 06.07.2010 21:38, schrieb Jonathan Nieder:
Show 9 quoted lines
> 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?

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é
Jonathan Nieder· Jul 2, 2010, 19:21 UTC · re: Nazri Ramliy · lore

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

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?

Show changes to t/t7811-grep-open.sh +18 −0
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 &&
Nazri Ramliy· Jul 3, 2010, 01:20 UTC · re: Jonathan Nieder · lore

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

On Sat, Jul 3, 2010 at 3:21 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
> 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?

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.
Jonathan Nieder· Jul 3, 2010, 02:55 UTC · re: Nazri Ramliy · lore

[PATCH v2] grep -O: Do not pass color sequences as filenames to pager

From: Nazri Ramliy <ayiehere@gmail.com>
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 <ayiehere@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)
Show changes to 2 files +16 −0

builtin/grep.c, t/t7811-grep-open.sh

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
Jakub Narebski· Jul 3, 2010, 07:59 UTC · re: Nazri Ramliy · lore

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

Nazri Ramliy <ayiehere@gmail.com> writes:
Show 13 quoted lines
> 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
Jonathan Nieder· Jul 6, 2010, 20:04 UTC · re: Jakub Narebski · lore

[PATCH] t/README: document more test helpers

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

Reported-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)
Show changes to t/README +31 −0
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
Ævar Arnfjörð Bjarmason· Jul 6, 2010, 20:23 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t/README: document more test helpers

On Tue, Jul 6, 2010 at 20:04, Jonathan Nieder <jrnieder@gmail.com> 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 <avarab@gmail.com>
Junio C Hamano· Jul 7, 2010, 04:25 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t/README: document more test helpers

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 5 quoted lines
> + - 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.
I'd add '... when the test is run with "-v" option.' at the end.
Otherwise the explanation reads very well.  Thanks.

← back to recent threads