Re: [PATCH 1/3] color.c: Refactor color_output to use enums

From: Jeff King <peff@peff.net>
Date: Jan 16, 2020, 18:23 UTC
Message-ID: <20200116182331.GA2946050@coredump.intra.peff.net>
In-Reply-To: <xmqq5zhbi8l3.fsf@gitster-ct.c.googlers.com>

On Thu, Jan 16, 2020 at 10:01:44AM -0800, Junio C Hamano wrote:

Show 7 quoted lines

> Not that I agree with the (untold) reasoning why we chose to use
> 30-37 instead of 0-7, though.  If this were up to me, I would have
> rather defined COLOR_BACKGROUND_ANSI = 40, kept .value to 0-7 and
> passed COLOR_{FORE,BACK}GROUPD_ANSI to callers of color_output().
> 
> Since I haven't seen 2/3 and 3/3, perhaps there is a good reason why
> this step was done this way instead, though, I guess.

Yeah, it becomes more clear in patch 2, where the value can be either "31" or "91", for the bright or non-bright variant, and adding "30" is wrong. (But certainly I agree this needs to be explained here).

Another way to write it would be to store 0-7 in the value as before, and then add a separate "bright" flag to "struct color". And then the output becomes:

  COLOR_FOREGROUND_OFFSET + c->value + (c->bright ? COLOR_BRIGHT_OFFSET : 0)

or similar. One minor confusion there is that COLOR_256 and COLOR_RGB would ignore the "bright" field.

-Peff