On Sat, Dec 30 2017, Jeff King jotted: > 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?
