Skip to content

Commit 8ee2476

Browse files
peffgitster
authored andcommitted
grep: don't treat grep_opt.color as a strict bool
In show_line(), we check to see if colors are desired with just: if (opt->color) ...we want colors... But this is incorrect. The color field here is really a git_colorbool, so it may be "true" for GIT_COLOR_UNKNOWN or GIT_COLOR_AUTO. Either of those _might_ end up true eventually (once we apply default fallbacks and check stdout's tty), but they may not. E.g.: git grep foo | cat will enter the conditional even though we're not going to show colors. We should collapse it into a true boolean by calling want_color(). It turns out that this does not produce a user-visible bug. We do some extra processing to isolate the matched portion of the line in order to colorize it, but ultimately we pass it to our output_color() helper, which does correctly check want_color(). So we end up with no colors. But dropping the extra processing saves a measurable amount of time. For example, running under hyperfine (which redirects to /dev/null, and thus does not colorize): Benchmark 1: ./git.old grep a Time (mean ± σ): 58.7 ms ± 3.5 ms [User: 580.6 ms, System: 74.3 ms] Range (min … max): 53.5 ms … 67.1 ms 48 runs Benchmark 2: ./git.new grep a Time (mean ± σ): 35.5 ms ± 0.9 ms [User: 276.8 ms, System: 73.8 ms] Range (min … max): 34.3 ms … 39.3 ms 79 runs Summary ./git.new grep a ran 1.65 ± 0.11 times faster than ./git.old grep a That's a fairly extreme benchmark, just because it will come up with a ton of small matches, but it shows that this really does matter. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 53e8a43 commit 8ee2476

File tree

1 file changed

+2
-2
lines changed

1 file changed

+2
-2
lines changed

grep.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1263,12 +1263,12 @@ static void show_line(struct grep_opt *opt,
12631263
*/
12641264
show_line_header(opt, name, lno, cno, sign);
12651265
}
1266-
if (opt->color || opt->only_matching) {
1266+
if (want_color(opt->color) || opt->only_matching) {
12671267
regmatch_t match;
12681268
enum grep_context ctx = GREP_CONTEXT_BODY;
12691269
int eflags = 0;
12701270

1271-
if (opt->color) {
1271+
if (want_color(opt->color)) {
12721272
if (sign == ':')
12731273
match_color = opt->colors[GREP_COLOR_MATCH_SELECTED];
12741274
else

0 commit comments

Comments
 (0)