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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 30, 2017, 12:33 UTC
Message-ID: <87tvw875vh.fsf@evledraar.gmail.com>
In-Reply-To: <20171230015533.GA27130@sigill.intra.peff.net>

On Sat, Dec 30 2017, Jeff King jotted:

Show 52 quoted lines

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

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?