Re: [PATCH v2 13/18] color: provide inverted colors, too

From: Jeff King <peff@peff.net>
Date: May 6, 2018, 06:35 UTC
Message-ID: <20180506063543.GA3418@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1805060001230.77@tvgsbejvaqbjf.bet>

On Sun, May 06, 2018 at 12:03:50AM +0200, Johannes Schindelin wrote:

Show 7 quoted lines

> > There's a "reverse" attribute (which we already parse and support) that
> > can do this without having to repeat the colors. AFAIK it's well
> > supported everywhere, but I could be wrong.
> 
> How would I use that here, though? I need to get the thing via
> diff_get_color_opt() which takes a parameter of type `enum color_diff`.
> There is no way I can specify `reverse` here, can I?

My thinking was that the code would know that coloring the initial "+" should combine color.diff.new, along with a new tbdiff-specific config option. So the C equivalent of something like this:

  new=$(git config --get-color color.diff.new green)
  tbdiff=$(git config --get-color color.tbdiff.new reverse)
  reset=$(git config --get-color color.diff.reset reset)

  echo "${new}${tbdiff}+${reset}${new}+actual diff content${reset}"

Then if you set color.diff.new to blue, you'll get a reverse-blue "+" without having to configure anything else.

You can still override the tbdiff coloring with a totally unrelated color, since it comes after ${new} (so you could set it to purple or something if you wanted, though obviously a background or attribute from ${new} can still leak through if you have one set). The only downside in such a case is that the color sequence is slightly longer ("green, no blue!").

You could also have tbdiff.new and tbdiff.old to allow setting them independently (but they'd both default to "reverse").

Show 7 quoted lines

> > I wonder if that would make configuring this slightly more pleasant,
> > since it saves the user having to define "oldinv" whenever they change
> > "old".
> 
> I am all for making the configuration more pleasant. So I hope I can make
> use of the `reverse` thing here, without having to introduce a new enum
> value.

I think the new enum (and matching config) has some value in case people want to override it. But if you don't want to, diff_get_color() is really just checking want_color() as a convenience. You could do that, too:

  const char *reverse = want_color(opt->use_color) ? GIT_COLOR_REVERSE : "";

You'd have to introduce GIT_COLOR_REVERSE. I don't think we have a constant for it yet, but it's \x[7m.

-Peff