From: Jeff King <peff@peff•net>
To: git@vger•kernel.org
Cc: Patrick Steinhardt <ps@pks•im>
Subject: [PATCH 03/13] grep: don't treat grep_opt.color as a strict bool
Date: Tue, 16 Sep 2025 16:16:00 -0400 [thread overview]
Message-ID: <20250916201600.GC612873@coredump.intra.peff.net> (raw)
In-Reply-To: <20250916201036.GA612463@coredump.intra.peff.net>
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>
---
If you really want to cheat on the benchmark, grep for a single space,
which is even more common.
grep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/grep.c b/grep.c
index 932647e4a6..c7e1dc1e0e 100644
--- a/grep.c
+++ b/grep.c
@@ -1263,12 +1263,12 @@ static void show_line(struct grep_opt *opt,
*/
show_line_header(opt, name, lno, cno, sign);
}
- if (opt->color || opt->only_matching) {
+ if (want_color(opt->color) || opt->only_matching) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
int eflags = 0;
- if (opt->color) {
+ if (want_color(opt->color)) {
if (sign == ':')
match_color = opt->colors[GREP_COLOR_MATCH_SELECTED];
else
--
2.51.0.527.g34bc42dacd
next prev parent reply other threads:[~2025-09-16 20:16 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-16 20:10 [PATCH 0/13] unraveling the mysteries of color variables Jeff King
2025-09-16 20:12 ` Jeff King
2025-09-16 20:13 ` [PATCH 01/13] color: use GIT_COLOR_* instead of numeric constants Jeff King
2025-09-16 20:14 ` [PATCH 02/13] color: return enum from git_config_colorbool() Jeff King
2025-09-16 20:16 ` Jeff King [this message]
2025-09-16 20:17 ` [PATCH 04/13] diff: simplify color_moved check when flushing Jeff King
2025-09-16 20:19 ` [PATCH 05/13] diff: don't use diff_options.use_color as a strict bool Jeff King
2025-09-16 20:20 ` [PATCH 06/13] diff: pass o->use_color directly to fill_metainfo() Jeff King
2025-09-16 20:21 ` [PATCH 07/13] diff: stop passing ecbdata->use_color as boolean Jeff King
2025-09-16 20:22 ` [PATCH 08/13] pretty: use format_commit_context.auto_color as colorbool Jeff King
2025-09-16 20:24 ` [PATCH 09/13] color: use git_colorbool enum to type to store colorbools Jeff King
2025-09-16 23:13 ` [PATCH v1.5 9/13] color: use git_colorbool enum " Jeff King
2025-09-16 20:25 ` [PATCH 10/13] color: return bool from want_color() Jeff King
2025-09-16 20:26 ` [PATCH 11/13] add-interactive: retain colorbool values longer Jeff King
2025-09-16 20:26 ` [PATCH 12/13] config: store want_color() result in a separate bool Jeff King
2025-09-16 20:27 ` [PATCH 13/13 DO NOT APPLY] color: convert git_colorbool into a struct Jeff King
2025-09-16 20:35 ` Junio C Hamano
2025-09-16 20:28 ` [PATCH 0/13] unraveling the mysteries of color variables Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250916201600.GC612873@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox