git bisect colour output contrary to configuration

My ~/.gitconfig sets color.ui=never, which should prevent attempts at colouring output from all git commands. I do not have any git configuration enabling colour in any situation (such as for specific commands). But when a git bisect completes, the output identifying the first bad commit includes escape sequences to colour the "commit 3e6..." line yellow. Excerpt of strace output (with many irrelevant lines omitted):

23851 write(1, "3e6fc602e433dbd76941ac0ef7a438a77fbe9a05 is the first bad commit\n", 65) = 65
23851 open("/home/zefram/.gitconfig", O_RDONLY) = 3
23851 read(3, "[user]\n\tname = Zefram\n\temail = [email protected]\n\tsigningkey = 0x8E1E1EC1\n\n[color]\n\tui = never\n", 4096) = 93 23851 write(1, "\33[33mcommit 3e6fc602e433dbd76941ac0ef7a438a77fbe9a05\33[m\n", 56) = 56

Given the configuration, that line should be free of escape sequences.

I'm mainly using git 2.1.4 via Debian, but I've also reproduced this problem with the latest from git.git (commit 1eaabe34fc6f486367a176207420378f587d3b48, tagged v2.16.0-rc0).

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.

Quoted 3 lines · Ævar Arnfjörð Bjarmason

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

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

Quoted 10 lines

Quoted 3 lines · Ævar Arnfjörð Bjarmason

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 3 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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?

Quoted 5 lines

Quoted 4 lines

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

Quoted 6 lines

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.

It should break a test, since I added one in 33c643bb083. :)

That covers "add -p", though, which only does diff-files under the hood. You can convince it to run "diff-index", too, but I don't think diff-tree. So technically your patch doesn't break add--interactive, but probably does break some other script we don't know about. ;)

Quoted 4 lines

Quoted 3 lines

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.

Quoted 8 lines

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?

I like that solution. I've often found the output ugly, too. And in particular, it doesn't show any output at all for merge commits. Doing "diff-tree --cc --stat" would be the minimal output improvement there.

I do like the idea of using "show", though. We know the point is to show the output to the user, so we don't mind at all if the behavior or output of show changes in future versions (unless we consider the final output of bisect to be machine-readable, but I certainly don't).

Quoted 4 lines

I do like the idea of using "show", though. We know the point is to show the output to the user, so we don't mind at all if the behavior or output of show changes in future versions (unless we consider the final output of bisect to be machine-readable, but I certainly don't).

Not knowing the internal APIs for that well, is this basically a matter of copy/pasting (or factoring out into a function), some of this:

git grep -W cmd_show -- builtin/log.c

I.e. boilerplate + calling cmd_log_walk() to yield a result similar to e22278c0a0 ("bisect: display first bad commit without forking a new process", 2009-05-28).

Or is it preferred to just fake up argc/argv and call cmd_show() directly? I haven't seen many examples of that in the codebase:

    git grep -W '(return|=)\s*cmd.*argc' -- '*.c'

But I don't see why it wouldn't work, the cmd_show() doesn't call exit() itself, and we're right about to call exit anyway when our current diff-tree invocation is called.

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Quoted 4 lines

I do like the idea of using "show", though. We know the point is to show the output to the user, so we don't mind at all if the behavior or output of show changes in future versions (unless we consider the final output of bisect to be machine-readable, but I certainly don't).

Quoted 14 lines

Not knowing the internal APIs for that well, is this basically a matter of copy/pasting (or factoring out into a function), some of this:

git grep -W cmd_show -- builtin/log.c

I.e. boilerplate + calling cmd_log_walk() to yield a result similar to e22278c0a0 ("bisect: display first bad commit without forking a new process", 2009-05-28).

Or is it preferred to just fake up argc/argv and call cmd_show() directly? I haven't seen many examples of that in the codebase:

    git grep -W '(return|=)\s*cmd.*argc' -- '*.c'

Quoted 3 lines

But I don't see why it wouldn't work, the cmd_show() doesn't call exit() itself, and we're right about to call exit anyway when our current diff-tree invocation is called.

Hmm, I just assumed we were actually calling diff-tree. But looking at that code in bisect, it literally is calling log_tree_commit(), which is the same thing that git-show is doing.

So yet another option is to just set up our options similarly:

bisect.c +0 -0

index 0fca17c02b..1eadecd42a 100644
@@ -893,9 +893,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 
        /* diff-tree init */

Though I do kind of like the idea of just delegating to git-show. There's no real need for us to have our own logic.

I think calling cmd_show() from bisect.c is supposed to be forbidden (library code shouldn't call up to builtin code). I was going to suggest just using run_command() to call git-show. After all, we do this only once at the very end of the bisection (which is pretty heavy-weight, as it surely has forked a lot of processes to do the actual testing).

But that would be directly undoing Christian's e22278c0a0 (bisect:
display first bad commit without forking a new process, 2009-05-28). I'm of the opinion that would be OK, but maybe Christian has input. :)