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

On Sat, Dec 30 2017, Jeff King jotted:

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

quoted

Ævar Arnfjörð Bjarmason wrote:

quoted

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...

quoted

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.

Thanks. What a mess.

I haven't tried that add-interactive case you mentioned, an earlier
version of this patch where I tried adding the color detection in
git_diff_basic_config() did break one of its tests, but not my ptch, but
it's probably still broken with =always (haven't tested.

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.

Getting back to the bug report that prompted this whole thing, wouldn't
the easiest solution just to run "git show --stat $commit" instead of
"git diff-tree --pretty $commit" when bisect wants to report the commit
it found?

I've always thought the output was a bit ugly, it's plumbing command, so
why wouldn't we just show the commit as the user usually prefers to see
commits?