From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 09 Sep 2025 06:06:50 GMT
Subject: Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring
Message-ID: <aL_D-quAoabKxhCN@pks.im>
In-Reply-To: <20250908161648.GC1308482@coredump.intra.peff.net>

On Mon, Sep 08, 2025 at 12:16:48PM -0400, Jeff King wrote:
> On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:
> 
> > > +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).

Makes sense.

Patrick