Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring

From: Jeff King <peff@peff.net>
Date: Sep 8, 2025, 16:16 UTC
Message-ID: <20250908161648.GC1308482@coredump.intra.peff.net>
In-Reply-To: <aLfs7wuFpMhg8fK_@pks.im>

On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:

Show 9 quoted lines

> > +static int check_color_config(struct repository *r, const char *var)
> >  {
> >  	const char *value;
> > +	int ret;
> > +
> > +	if (repo_config_get_value(r, var, &value))
> > +		ret = -1;
> 
> Not an old issue, but should we use `GIT_COLOR_UNKNOWN` here?

My initial reaction was: yeah, we could probably fix this up in a preparatory patch. But the problem is much deeper than the add-interactive code. Nobody uses GIT_COLOR_UNKNOWN at all! Even git_config_colorbool() just returns -1.

Moreover, it does not even use the ALWAYS/NEVER defines, but just 1 and
0. Making things even more complicated, we sometimes want to consider
"do we want color" as this always/never/auto/unknown set, and then
sometimes we collapse that (using the same variable!) into a single
true/false value.

So using that consistently and possibly switching to an enum is a much bigger topic. It may be worth cleaning up, but I don't think it's worth derailing this regression fix. In the meantime, I'd rather keep this code matching the rest of the color code (it's not even really adding new instances of "-1", but just shuffling them around).

Show 9 quoted lines

> > -	if (want_color_fd(1, -1)) {
> > +	if (want_color_fd(1, s->s.use_color_diff)) {
> >  		struct child_process colored_cp = CHILD_PROCESS_INIT;
> >  		const char *diff_filter = s->s.interactive_diff_filter;
> >  
> 
> We're printing the diff here, and this change is the whole point of this
> commit as far as I understand as we now properly respect configured diff
> colors.

Yes. I would have liked to split it up more to make this hunk stand out, but there's some chicken-and-egg dependencies.

Show 19 quoted lines

> > +test_expect_success 're-coloring diff without color.interactive' '
> > +	git reset --hard &&
> > +
> > +	test_write_lines 1 2 3 >test &&
> > +	git add test &&
> > +	test_write_lines one 2 three >test &&
> > +
> > +	test_write_lines s n n |
> > +	force_color git \
> > +		-c color.interactive=false \
> > +		-c color.diff=true \
> > +		-c color.diff.frag="bold magenta" \
> > +		add -p >output.raw 2>&1 &&
> > +	test_decode_color <output.raw >output &&
> > +	test_grep "<BOLD;MAGENTA>@@" output
> > +'
> > +
> 
> Should we also verify that the interactive prompts aren't colored here?

Seems reasonable. Knowing that the patch is splitting the diff coloring off of the interactive, it would be pretty hard to introduce such a bug. But from a black box perspective, that is probably a good thing to test.

Ultimately the best test would be for every item that _could_ be colored by each type to be individually checked in each scenario. But I didn't want the test to depend on enumerating those very specific details of the code.

-Peff