Re: [PATCH] diff-tree: obey the color.ui configuration

[Jeff King <peff@peff.net>]

On Fri, Dec 29, 2017 at 06:16:31PM -0500, Todd Zullinger wrote:

> Ævar Arnfjörð Bjarmason wrote:
> > No idea how to test this, in particular trying to pipe the output of
> > color.ui=never v.s. color.ui=auto to a file as "auto" will disable
> > coloring when it detects a pipe, but this fixes the issue.
> 
> You might be able to use similar methods as those Jeff used
> in the series merged from jk/ui-color-always-to-auto:
> 
> https://github.com/gitster/git/tree/jk/ui-color-always-to-auto

Yeah, test_terminal is the solution to testing. But...

> He may also have some ideas about this issue in general.
> (Or they could be tramatic memories, depending on how
> painful it was to dig into the color code.)

Yep. If we make diff-tree support color.ui, it's going to break a bunch
of other stuff (like add--interactive) for people who set color.ui=always.
I know this empirically, because we did that in v2.13, and a bunch of
people complained. ;)

The root of the problem is that the plumbing diff-tree defaults its
internal color variable to "auto" in the first place. In theory the best
way forward is fixing that, but it's likely to have a bunch of fallouts
itself (scripts which use plumbing and where the user _does_ want color
will stop showing it). This bug has been around since v1.8.4, I think,
so it's hard to say how many people are depending on it at this point.

A hackier option which would probably make most people happy would be to
have plumbing respect "color.ui=never", but not any other values.

I think the history of the back and forth is:

  - 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10) introduced
    the problem of plumbing defaulting to "auto". This was in v1.8.4.

  - we did something similar to Ævar's patch in 136c8c8b8f (color: check
    color.ui in git_default_config(), 2017-07-13). That shipped in
    v2.14.2, and people with color.ui=always complained, because things
    like add--interactive broke for them.

  - we tried fixing it with 6be4595edb (color: make "always" the same as
    "auto" in config, 2017-10-03), but that broke people doing "git -c
    color.ui=always" as an equivalent of "--color". We talked about
    making the "-c" config behave differently from on-disk config, but
    got pretty disgusted at the weird hacks. And so...

  - we ended up with 33c643bb08 (Revert "color: check color.ui in
    git_default_config()", 2017-10-13), which just reverts the whole
    mess back to the pre-v2.14 state. This shipped in v2.15.

So I don't think we want to go down that road again. If anything, we
want to either fix the original sin from 4c7f1819b3, or we want to do
the "respect only never" hack.

-Peff