From 955000d91718eee5abd005dd43ed035a5115d870 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:21:20 -0400 Subject: [PATCH] diff: stop passing ecbdata->use_color as boolean In emit_hunk_header(), we evaluate ecbdata->color_diff both as a git_colorbool, passing it to diff_get_color(): const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET); and as a strict boolean: const char *reverse = ecbdata->color_diff ? GIT_COLOR_REVERSE : ""; At first glance this seems wrong. Usually we store the color decision as a git_colorbool, so the second line would get confused by GIT_COLOR_AUTO (which is boolean true, but may still mean we do not produce color). However, the second line is correct because our caller sets color_diff using want_color(), which collapses the colorbool to a strict true/false boolean. The first line is _also_ correct because of the idempotence of want_color(). Even though diff_get_color() will pass our true/false value through want_color() again, the result will be left untouched. But let's pass through the colorbool itself, which makes it more consistent with the rest of the diff code. We'll need to then call want_color() whenever we treat it as a boolean, but there is only such spot (the one quoted above). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index 505819c6c6..3544be2318 100644 --- a/diff.c +++ b/diff.c @@ -1678,7 +1678,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata, const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO); const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO); const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET); - const char *reverse = ecbdata->color_diff ? GIT_COLOR_REVERSE : ""; + const char *reverse = want_color(ecbdata->color_diff) ? GIT_COLOR_REVERSE : ""; static const char atat[2] = { '@', '@' }; const char *cp, *ep; struct strbuf msgbuf = STRBUF_INIT; @@ -1832,7 +1832,7 @@ static void emit_rewrite_diff(const char *name_a, size_two = fill_textconv(o->repo, textconv_two, two, &data_two); memset(&ecbdata, 0, sizeof(ecbdata)); - ecbdata.color_diff = want_color(o->use_color); + ecbdata.color_diff = o->use_color; ecbdata.ws_rule = whitespace_rule(o->repo->index, name_b); ecbdata.opt = o; if (ecbdata.ws_rule & WS_BLANK_AT_EOF) { @@ -3729,7 +3729,7 @@ static void builtin_diff(const char *name_a, if (o->flags.suppress_diff_headers) lbl[0] = NULL; ecbdata.label_path = lbl; - ecbdata.color_diff = want_color(o->use_color); + ecbdata.color_diff = o->use_color; ecbdata.ws_rule = whitespace_rule(o->repo->index, name_b); if (ecbdata.ws_rule & WS_BLANK_AT_EOF) check_blank_at_eof(&mf1, &mf2, &ecbdata); -- 2.47.3