[BUG] git bisect colour output contrary to configuration

[BUG] git bisect colour output contrary to configuration

[Zefram]

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 = zefram@fysh.org\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).

-zefram

Re: [BUG] git bisect colour output contrary to configuration

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 29 2017, zefram@fysh.org jotted:

> 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 = zefram@fysh.org\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).

This issue is a bug, but has nothing do do with bisect per-se, but is a
bug in diff-tree, compare these two:

    git -c color.ui=never diff-tree --pretty --stat HEAD
    git -c color.ui=never show --pretty --stat HEAD

diff-tree will incorrectly show colored output here despite
ui.color=never.

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

[Ævar Arnfjörð Bjarmason]

Before git-bisect exits it calls `diff-tree --pretty --stat $commit`
on the bad commit. This would always print the "commit" line with
coloring despite color.ui being set to "never".

Teach diff-tree to look at the git_color_config() configuration. I
initially tried to add this to git_diff_basic_config itself, but it
makes other unrelated things fail, and this is a more isolated change
that solves the issue.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

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.

 builtin/diff-tree.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index b775a75647..0311c01a87 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -97,6 +97,15 @@ static void diff_tree_tweak_rev(struct rev_info *rev, struct setup_revision_opt
 	}
 }
 
+
+static int diff_tree_config(const char *var, const char *value, void *cb)
+{
+	if (git_color_config(var, value, cb) < 0)
+		return -1;
+
+	return git_diff_basic_config(var, value, cb);
+}
+
 int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 {
 	char line[1000];
@@ -108,7 +117,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(diff_tree_usage);
 
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+	git_config(diff_tree_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
-- 
2.15.1.424.g9478a66081

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

[Todd Zullinger]

Æ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

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

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Subtlety is the art of saying what you think and getting out of the
way before it is understood.
    -- Anonymous

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

[Jeff King]

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

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

[Ævar Arnfjörð Bjarmason]

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?

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

[Jeff King]

On Sat, Dec 30, 2017 at 01:33:06PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

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

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

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

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 30 2017, Jeff King jotted:

> On Sat, Dec 30, 2017 at 01:33:06PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >   - 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.
>
> It should break a test, since I added one in 33c643bb083. :)

Right, the one I tried first broke that, but not this version....

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

...Yeah, for sure.

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

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.

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

[Jeff King]

On Sat, Dec 30, 2017 at 04:04:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

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:

diff --git a/bisect.c b/bisect.c
index 0fca17c02b..1eadecd42a 100644
--- a/bisect.c
+++ b/bisect.c
@@ -893,9 +893,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 
 	/* diff-tree init */
 	init_revisions(&opt, prefix);
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+	git_config(git_diff_ui_config, NULL);
 	opt.abbrev = 0;
 	opt.diff = 1;
+	opt.combine_merges = 1;
+	opt.dense_combined_merges = 1;
 
 	/* This is what "--pretty" does */
 	opt.verbose_header = 1;

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

-Peff

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

[Christian Couder]

On Sat, Dec 30, 2017 at 7:15 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Dec 30, 2017 at 04:04:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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).

I think that the first line that gives the sha1 of the first bad
commit (XXX is the first bad commit) should be machine-readable,
because there have been people writing scripts on top of git bisect.
Below that I am ok if it is more fancy, especially because it looks
like it already used some coloring.

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

Nice. This means that we should be able to make small changes to move
from a diff-tree like output to a show like output.

> So yet another option is to just set up our options similarly:
>
> diff --git a/bisect.c b/bisect.c
> index 0fca17c02b..1eadecd42a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -893,9 +893,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>
>         /* diff-tree init */
>         init_revisions(&opt, prefix);
> -       git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> +       git_config(git_diff_ui_config, NULL);
>         opt.abbrev = 0;
>         opt.diff = 1;
> +       opt.combine_merges = 1;
> +       opt.dense_combined_merges = 1;
>
>         /* This is what "--pretty" does */
>         opt.verbose_header = 1;

I am ok with that kind of changes.

> 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 don't think there is a lot of logic in the above. We are mostly just
setting parameters before calling setup_revisions() and
log_tree_commit().

If we think that the above is too much "logic", then we should
probably try to refactor the revision walking interface to make it
simpler, so that not as much "logic" is needed to call it. If this
succeeds, then this could help simplify a lot of things throughout the
code base.

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

In general I am against adding forks that are not necessary and not
specially useful, as they don't perform well on some platforms or some
machines (like big servers where new processes tend to be allocated to
a different CPU).

I know that in this case it happens only once after usually a lot of
processing and forking, but I don't think it gives a good example and
goes in the right direction.

Yes, it looks ugly to have 10 or 20 lines of code to just set
parameters for setup_revisions() and log_tree_commit(), and yes I
don't like the revision walking interface, but I think this is a
different problem that we should take care of separately.

Thanks for working on this.