[PATCH] add--interactive: allow diff colors without interactive colors

[PATCH] add--interactive: allow diff colors without interactive colors

[Jeff King]

Users with color.diff set to true/auto will not see color in
"git add -i" unless they also set color.interactive.

However, some users may want just one without the other, so
there's no reason to tie them together.

Note that there is now no way to have color on for "git
diff" but not for diffs from "git add -i"; such a
configuration seems unlikely, though.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 0cdd800..aaa9b24 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -22,16 +22,16 @@ if ($use_color) {
 	$header_color = $repo->get_color("color.interactive.header", "bold");
 	$help_color = $repo->get_color("color.interactive.help", "red bold");
 	$normal_color = $repo->get_color("", "reset");
+}
 
-	# Do we also set diff colors?
-	$diff_use_color = $repo->get_colorbool('color.diff');
-	if ($diff_use_color) {
-		$new_color = $repo->get_color("color.diff.new", "green");
-		$old_color = $repo->get_color("color.diff.old", "red");
-		$fraginfo_color = $repo->get_color("color.diff.frag", "cyan");
-		$metainfo_color = $repo->get_color("color.diff.meta", "bold");
-		$whitespace_color = $repo->get_color("color.diff.whitespace", "normal red");
-	}
+# Do we also set diff colors?
+$diff_use_color = $repo->get_colorbool('color.diff');
+if ($diff_use_color) {
+	$new_color = $repo->get_color("color.diff.new", "green");
+	$old_color = $repo->get_color("color.diff.old", "red");
+	$fraginfo_color = $repo->get_color("color.diff.frag", "cyan");
+	$metainfo_color = $repo->get_color("color.diff.meta", "bold");
+	$whitespace_color = $repo->get_color("color.diff.whitespace", "normal red");
 }
 
 sub colored {
-- 
1.5.4.rc2.1122.g6954-dirty

Re: [PATCH] add--interactive: allow diff colors without interactive colors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Users with color.diff set to true/auto will not see color in
> "git add -i" unless they also set color.interactive.
>
> However, some users may want just one without the other, so
> there's no reason to tie them together.
>
> Note that there is now no way to have color on for "git
> diff" but not for diffs from "git add -i"; such a
> configuration seems unlikely, though.

Although I would agree with what this patch does, I think you
are contradicting with yourself in the above justification.
Some users may want to color "git diff" output but not
interaction with "git add -i", and that's also "just one without
the other", but you just tied them together, only differently,
and "seems unlikely" is a rather weak excuse.

The justification should instead be: having more independent
knobs is not necessarily better.  The user should not have to
tweak too many knobs.

In the longer term, I think we should try reducing the number of
knobs by giving "color.git" that allows you to pretend as if all
of the "color.interactive", "color.diff", "color.status",
"color.someothercolorizedcommand" are all set.  I do not think
being able to control the use of colors per command is giving
much other than confusion to the user.

That may not be so easy with the current structure of the config
reader, though.

Re: [PATCH] add--interactive: allow diff colors without interactive colors

[Jeff King]

On Fri, Jan 04, 2008 at 04:20:09PM -0800, Junio C Hamano wrote:

> Although I would agree with what this patch does, I think you
> are contradicting with yourself in the above justification.
> Some users may want to color "git diff" output but not
> interaction with "git add -i", and that's also "just one without
> the other", but you just tied them together, only differently,
> and "seems unlikely" is a rather weak excuse.

Then let me state it another way that I think makes a better argument.
You have two knobs, color.interactive and color.diff, controlling
menu-coloring of git-add--interactive and controlling colorization of
git-diff output. Now we have a third thing that may be colorized: diff
output of git-add--interactive.

Let's assume that we don't want to add another color.interactive-diff
knob (though that is an option). That means that we have to tie the
colorization either to color.interactive or to color.diff. Right now we
subdivide it by command, so that the coloring of interactive diffs is
tied to color.interactive[1]. What I am proposing is to divide it by
_functionality_, so that by saying color.diff you mean "I like color
diffs, no matter where they are." And by saying color.interactive, you
mean "I like color interactive menus, no matter where they are." I think
it is much more likely that users will find that division useful. And
it's something we already do, since color.diff is respected not just by
git-diff, but by diffs produced by all programs, including the git-log
family.

[1]: Actually, we currently tie interactive diff coloring to "diff &&
interactive" which is even less useful. If I turn on color.interactive,
I still don't get colored diffs. But if I turn on color.diff, then
git-diff starts producing colored diffs. So you really can't represent
all choices, and I think the subdivision I outlined makes more sense (at
least it does to me).

> The justification should instead be: having more independent
> knobs is not necessarily better.  The user should not have to
> tweak too many knobs.
>
> In the longer term, I think we should try reducing the number of
> knobs by giving "color.git" that allows you to pretend as if all
> of the "color.interactive", "color.diff", "color.status",
> "color.someothercolorizedcommand" are all set.  I do not think
> being able to control the use of colors per command is giving
> much other than confusion to the user.

I'm not sure I agree with that; my problem here is that I _want_ to turn
a knob, but the functionality is tied to another knob. IOW, reducing the
number of knobs is going to make it worse.

That being said, all of those knobs _are_ confusing. In my case, I like
color. I just don't like the colors that color.interactive provides, so
I don't want to use them.  However, you can tune that quite a bit by
changing color.interactive.* (and just choosing "plain" for things you
don't want marked). Of course that still doesn't allow you to have
_different_ color settings between the diffs of git-diff and those of
git-add--interactive. But then, my point is that I don't think sane
users want that. They either want diffs colored or they don't, no matter
what command is producing them.

> That may not be so easy with the current structure of the config
> reader, though.

I don't think it's hard; the client code for the colors checks a
"color_foo" knob. It would just check "color_foo || color_all".

-Peff

Re: [PATCH] add--interactive: allow diff colors without interactive colors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Let's assume that we don't want to add another color.interactive-diff
> knob (though that is an option). That means that we have to tie the
> colorization either to color.interactive or to color.diff. Right now we
> subdivide it by command, so that the coloring of interactive diffs is
> tied to color.interactive[1]. What I am proposing is to divide it by
> _functionality_, so that by saying color.diff you mean "I like color
> diffs, no matter where they are." And by saying color.interactive, you
> mean "I like color interactive menus, no matter where they are." I think
> it is much more likely that users will find that division useful. And
> it's something we already do, since color.diff is respected not just by
> git-diff, but by diffs produced by all programs, including the git-log
> family.

I think I understood that part.  What I was saying was that it
is equally valid for other people to say "I like to interact
with 'git add -i' without colours because coloured output
distracts me when I have to think, even though I usually want to
view the whole diff in colours."  So yes, color.interactive-diff
is an option to give more flexibility, but no I do not think
that's a good flexibility.  I'd prefer a single "color.git"
environment that rules all, which is far simpler to explain and
configure.  Either you use colour for all your interaction with
git, or you live in black-and-white world.

> [1]: Actually, we currently tie interactive diff coloring to "diff &&
> interactive" which is even less useful. If I turn on color.interactive,
> I still don't get colored diffs. But if I turn on color.diff, then
> git-diff starts producing colored diffs. So you really can't represent
> all choices, and I think the subdivision I outlined makes more sense (at
> least it does to me).

And my point was that I doubt the change is such a big
improvement, certainly not in the way your justification
claimed, _even though I agree_ that the current "diff &&
interactive" way may not be something many people would want (by
the way, it happens to cover my preference, but that only means
I am a minority).  Neither covers all cases, and as I said, I do
not think more flexibility to cover all cases is necessarily a
good thing to shoot for.

Also there is another confusion factor we haven't discussed.

To an end user, the fact that "git add -p" shows diff using the
underlying "git diff" machinery does not matter.  That's just an
implementation detail.  "git diff" shows the whole diff at once
while "git add -p" shows it hunk by hunk.  It is clear they are
doing different things to the end user.  If he told "git add -p"
to be monochrome, he has every right to expect the part to pick
hunks to also stay monochrome.  To people who know the internal
implementation, it might be natural to expect the color.diff
configuration variable to affect the colouring of the hunk
picker.  To others, it is counterintuitive if color.diff had any
effect to what "git add -i" did.

> That being said, all of those knobs _are_ confusing. In my case, I like
> color. I just don't like the colors that color.interactive provides, so
> I don't want to use them.  However, you can tune that quite a bit by
> changing color.interactive.* (and just choosing "plain" for things you
> don't want marked).

Yes, I 100% agree with you that they are confusing, and being to
able to futz with color.interactive.* palette would probably
alleviate the need to have color.{diff,interactive,status,etc}.

> ... Of course that still doesn't allow you to have
> _different_ color settings between the diffs of git-diff and those of
> git-add--interactive. But then, my point is that I don't think sane
> users want that. They either want diffs colored or they don't, no matter
> what command is producing them.

That point I would agree with you but only 70%.  No sane user
would want deleted lines in red in "git diff" output and in
purple in hunk picker.  But I think it is reasonable to want to
view them in monochrome while in hunk picker (by setting
color.*.old to plain) but in red in normal diff output.

Perhaps a saner alternative would be:

 * When color.interactive tells to use color, all interaction
   with "add -i" will be in color.  There is no need to have
   both color.diff and color.interactive set.

 * When color.interactive tells not to use color, everything
   including the diff output will be monochrome.  What you have
   in color.diff does not matter.

 * We could allow color.interactive.* pallete to have elements
   that are parallel to color.diff.* palette to be used while
   showing the colored diff.  But this would be a low priority
   because (1) a custom setting to anything but "plain" does not
   make much practical sense, as it is just introducing needless
   inconsistency between "git diff" and the hunk picker, and (2)
   custom setting to "plain" would be used by people who like
   colored "git add -i" prompts but not colored hunk picker,
   which would be a minority.

The point of the third item is that you enable color.interactive
and set diff related entries of color.interactive.* palette to
plain, if you want some color while interacting with "add -i"
but do not like colored hunk picker.  This would parallel the
way you can selectively enable coloring in "git diff" output,
where you enable color.diff and set metainfo color to plain if
you want some color in diff output but do not like colored
metainfo.

Admittedly, it's more work.

Re: [PATCH] add--interactive: allow diff colors without interactive colors

[Jeff King]

On Sat, Jan 05, 2008 at 12:01:51AM -0800, Junio C Hamano wrote:

> I think I understood that part.  What I was saying was that it
> is equally valid for other people to say "I like to interact
> with 'git add -i' without colours because coloured output
> distracts me when I have to think, even though I usually want to
> view the whole diff in colours."  So yes, color.interactive-diff

Right. My contention is that I think such people will be in the minority
(though obviously I have no numbers to back it up, and nobody else is
participating in this thread).

> To an end user, the fact that "git add -p" shows diff using the
> underlying "git diff" machinery does not matter.  That's just an
> implementation detail.

But he doesn't have to care about that. He cares only that "color.diff"
means "diffs are displayed in color." And "color.interactive" means
"interactive menus are displayed in color." Period. There is no concern
for the implementation detail of the diff machinery.

> "git diff" shows the whole diff at once while "git add -p" shows it
> hunk by hunk.  It is clear they are doing different things to the end
> user.

I don't see that distinction as relevant. Diffs are diffs, whether you
look at them with a small viewport or the whole thing. Would you expect
"git diff -- file1 file2" to have different display options than "git
diff -- file"?

> If he told "git add -p" to be monochrome, he has every right to expect
> the part to pick hunks to also stay monochrome.  To people who know

But he didn't. He said "git menus should be monochrome."

And yes, that's not exactly what config.txt says that color.interactive
does; but I think that is probably worth fixing.

> picker.  To others, it is counterintuitive if color.diff had any
> effect to what "git add -i" did.

Then why does it affect what "git log" does? Why does it affect what
"git reflog" does? Why does the documentation for color.diff say "use
colors in patch" without any reference to specific programs?

> Perhaps a saner alternative would be:
> 
>  * When color.interactive tells to use color, all interaction
>    with "add -i" will be in color.  There is no need to have
>    both color.diff and color.interactive set.
> 
>  * When color.interactive tells not to use color, everything
>    including the diff output will be monochrome.  What you have
>    in color.diff does not matter.

I think this is equally confusing. A user who sets color.diff will see
color diffs from all porcelains _except_ "add -i".

Moreover, this doesn't allow "I always want color in diffs, but I don't
want menu coloring" which is the very thing I have been trying to
accomplish (but yes, I can do that by individually setting
color.interactive.* to plain).

> The point of the third item is that you enable color.interactive
> and set diff related entries of color.interactive.* palette to
> plain, if you want some color while interacting with "add -i"
> but do not like colored hunk picker.  This would parallel the
> way you can selectively enable coloring in "git diff" output,
> where you enable color.diff and set metainfo color to plain if
> you want some color in diff output but do not like colored
> metainfo.

I fail to see how this is less confusing than just adding a separate
interactive-diff knob, since you are asking them to individually set
each color preference to plain. I.e., "set color.interactive to true and
color.interactive-diff to false" versus "set color.interactive to true,
but then for every type of diff colorization, set the color for it in
color.interactive.* to false".

I think the extra knob is not a problem; it is simply a matter of
defaulting the knobs based on other knobs. The rules for knobs
interacting may seem complex, but I think we can DWIM. E.g., given
config options:

  - color
  - color.diff
  - color.interactive
  - color.interactive.diff

The rules for "git add -i" diff coloring would be:

  (1) if color.interactive.diff is set to TRUE/FALSE, use that
  (2) otherwise, if color.interactive is set to TRUE/FALSE, use that
  (3) otherwise, if color.diff is set to TRUE/FALSE, use that
  (4) otherwise, if color is set to TRUE/FALSE, use that

IOW, even though we _have_ all of those knobs, users which don't want to
fine-tune can just use the higher-level knobs. Those that want to can
negate the higher level with lower level knobs. It's flexible, and it's
easy to tell users "just do 'git config color true'."

> Admittedly, it's more work.

Of course. ;) But I am willing to implement what I said above if you
agree that it is sensible.

-Peff

Re: [PATCH] add--interactive: allow diff colors without interactive colors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But he doesn't have to care about that. He cares only that "color.diff"
> means "diffs are displayed in color." And "color.interactive" means
> "interactive menus are displayed in color."
> ...
>> If he told "git add -p" to be monochrome, he has every right to expect
>> the part to pick hunks to also stay monochrome.  To people who know
>
> But he didn't. He said "git menus should be monochrome."

Who said anything about "interactive is limited to interactive
menus" anywhere?  That is where we differ and what you do not
seem to be getting.  I am talking about color.interactive that
controls the whole user experience of interacting with "add -i".

> Moreover, this doesn't allow "I always want color in diffs,
> but I don't want menu coloring" which is the very thing I have
> been trying to accomplish (but yes, I can do that by
> individually setting color.interactive.* to plain).

As you said earlier you may also be minority, but yes the color
pallette would help you do that.

> I fail to see how this is less confusing than just adding a separate
> interactive-diff knob, since you are asking them to individually set
> each color preference to plain.

What I am aiming at in longer term is to simplify things this way:

 * Users are categorized broadly into two groups.  The ones who
   like colours and the ones who don't want colours at all.
   color.git would control this (with backward compatibility
   options per command such as color.diff and
   color.interactive);

 * Minorities who want to disable colours for particular parts
   of the UI have enough knobs to tweak in the form of palettes.
   By definition this needs to address "particular parts", so
   "color.$command.$context" variables (e.g. color.diff.new,
   color.interactive.new; if somebody really really wants to
   have different settings between diff/show/log, that person
   could add color.{show,log}.new as well) are needed if we want
   to do this.

> E.g., given
> config options:
> ...
>> Admittedly, it's more work.
>
> Of course. ;) But I am willing to implement what I said above if you
> agree that it is sensible.

I think we share the ultimate goal of introducing higher level
knobs and our difference is just about minor details of how to
get there and what the intermediate levels look like.

I am trying to avoid introducing new intermediate level knobs
(e.g. color.log vs color.diff), as it is enough to disable or in
general change the way particular parts of the UI is coloured by
palette setting that specifically states which part of the UI is
tweaked (e.g. color.interactive.prompt).

Re: [PATCH] add--interactive: allow diff colors without interactive colors

[Jeff King]

On Sat, Jan 05, 2008 at 01:28:25AM -0800, Junio C Hamano wrote:

> Who said anything about "interactive is limited to interactive
> menus" anywhere?  That is where we differ and what you do not
> seem to be getting.  I am talking about color.interactive that
> controls the whole user experience of interacting with "add -i".

I did, just now. I am "getting" it just fine, but am trying to propose
an equally valid, and IMHO more useful, semantic for
"color.interactive." (which btw, is a horrible name if you truly mean
"the behavior of git add -i" since there are _other_ interactive
commands. Why doesn't it turn on color for git-rebase -i?).

> What I am aiming at in longer term is to simplify things this way:
> 
>  * Users are categorized broadly into two groups.  The ones who
>    like colours and the ones who don't want colours at all.
>    color.git would control this (with backward compatibility
>    options per command such as color.diff and
>    color.interactive);

OK, that makes sense. But I disagree somewhat with the phrase "backward
compatibility."

>  * Minorities who want to disable colours for particular parts
>    of the UI have enough knobs to tweak in the form of palettes.

They do, but those knobs are a pain to use. Why are we making it so hard
for them when we _already_ have the nice knobs for them to use. IOW, for
these users, git after this change will be _more_ annoying to use. Not
to mention that we must keep those other knobs around for historical
reasons for some amount of time.

Why not just add the higher-level knob and leave the existing ones
(using the priority scheme I mentioned in my last mail)?

>    By definition this needs to address "particular parts", so
>    "color.$command.$context" variables (e.g. color.diff.new,
>    color.interactive.new; if somebody really really wants to
>    have different settings between diff/show/log, that person
>    could add color.{show,log}.new as well) are needed if we want
>    to do this.

Sure. Though like you said previously, I think in 99% of cases, nobody
is going to set these to something exotic; they're going to set them all
ot their custom colors, or all to "plain". In which case, we are just
making work for them by not providing "color.show = false".

> I am trying to avoid introducing new intermediate level knobs
> (e.g. color.log vs color.diff), as it is enough to disable or in
> general change the way particular parts of the UI is coloured by
> palette setting that specifically states which part of the UI is
> tweaked (e.g. color.interactive.prompt).

It is "enough" but I don't think it is the sensible goal. I respect your
desire to keep knobs to a minimum. But I think the fact that we can
split these color knobs into "dead simple, color = true" and "advanced,
super-picky color options" in the documentation makes it less of an
issue.

Anyway, almost none of this is pre-1.5.4. The one thing which is, IMHO,
is the semantics of color.interactive (since it has not yet been
released, this is the best time to set it). I will summarize my stance,
and you can do what you will. Beyond that, this discussion has already
taken way more time than this change is worth.

My opinion is that color.diff should be respected in "git-add -i"
regardless of color.interactive because:
  - the color.diff setting controls diffs in all porcelain parts of git
    (diff, log, show, reflog)
  - parts of diffs are the same as diffs. Thus, hunks should in
    general be treated like diffs for presentation.
  - splitting color by functionality (i.e., diff versus menus) is
    therefore more consistent with the rest of git than splitting it by
    program ("git add -i" versus "git diff")
  - I also happen to think that users are more likely to find "split by
    functionality" useful, but neither of us has relevant data
    (except that you seemed to indicate that's what you would find more
    useful, and that's what I find more useful. So it's 2-0, and clearly
    statistically significant :) ).

So if you are swayed by that, then please apply my previous patch, and
consider the documentation patch below. And if not, let me know and I
will begin changing my color.interactive.* config. :)

-- >8 --
Clarify color.interactive documentation

There are two possible confusions with the color.interactive
description:

  1. the short name "interactive" implies that it covers all
     interactive commands; let's explicitly make it so, even
     though there are no other interactive commands which
     currently use it

  2. Not all parts of "git add --interactive" are controlled
     by color.interactive (specifically, the diffs require
     tweaking color.diff). So let's clarify that it applies
     only to displays and prompts.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e1eaee9..8907e16 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -406,7 +406,8 @@ color.diff.<slot>::
 	in color.branch.<slot>.
 
 color.interactive::
-	When set to `always`, always use colors in `git add --interactive`.
+	When set to `always`, always use colors for interactive prompts
+	and displays (such as those used by "git add --interactive").
 	When false (or `never`), never.  When set to `true` or `auto`, use
 	colors only when the output is to the terminal. Defaults to false.
 
-- 
1.5.4.rc2.1125.ga305e-dirty

Re: [PATCH/resend] add--interactive: allow diff colors without interactive colors

[Matthias Kestenholz]

(trying to resend because vger.kernel.org would not let the previous
email pass to the list. I am very sorry. The Evolution MUA is making
trouble.)

On Fri, 2008-01-04 at 16:20 -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Users with color.diff set to true/auto will not see color in
> > "git add -i" unless they also set color.interactive.
> >
> > However, some users may want just one without the other, so
> > there's no reason to tie them together.
> >
> > Note that there is now no way to have color on for "git
> > diff" but not for diffs from "git add -i"; such a
> > configuration seems unlikely, though.
> 
> Although I would agree with what this patch does, I think you
> are contradicting with yourself in the above justification.
> Some users may want to color "git diff" output but not
> interaction with "git add -i", and that's also "just one without
> the other", but you just tied them together, only differently,
> and "seems unlikely" is a rather weak excuse.
> 
> The justification should instead be: having more independent
> knobs is not necessarily better.  The user should not have to
> tweak too many knobs.
> 
> In the longer term, I think we should try reducing the number of
> knobs by giving "color.git" that allows you to pretend as if all
> of the "color.interactive", "color.diff", "color.status",
> "color.someothercolorizedcommand" are all set.  I do not think
> being able to control the use of colors per command is giving
> much other than confusion to the user.
> 
> That may not be so easy with the current structure of the config
> reader, though.

I managed to throw something together which works and passes all
the tests. Documentation included. :-)

I would be happy for feedback and suggestions.

-- 8< --

>From 90663fa5a7ac1a59d74aab3aa4d2bd8397340bd4 Mon Sep 17 00:00:00 2001
From: Matthias Kestenholz <matthias@spinlock.ch>
Date: Sat, 5 Jan 2008 11:20:08 +0100
Subject: [PATCH] Add global color config switch color.git

Signed-off-by: Matthias Kestenholz <matthias@spinlock.ch>
---
 Documentation/config.txt  |    6 ++++++
 builtin-branch.c          |    4 ++--
 builtin-log.c             |    3 ++-
 cache.h                   |    1 +
 color.c                   |   12 ++++++++++++
 color.h                   |    3 +++
 diff.c                    |    4 ++--
 git-add--interactive.perl |    4 ++--
 wt-status.c               |    4 ++--
 9 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ee08845..0a40102 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -405,6 +405,12 @@ color.diff.<slot>::
 	whitespace errors). The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.git::
+	When set to `always`, always use colors in all git commands which
+	are capable of colored output. When false (or `never`), never. When
+	set to `true` or `auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.interactive::
 	When set to `always`, always use colors in `git add --interactive`.
 	When false (or `never`), never.  When set to `true` or `auto`, use
diff --git a/builtin-branch.c b/builtin-branch.c
index 089cae5..8179938 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -76,12 +76,12 @@ static int git_branch_config(const char *var, const char *value)
 	if (!strcmp(var, "branch.autosetupmerge"))
 			branch_track = git_config_bool(var, value);
 
-	return git_default_config(var, value);
+	return git_use_color_config(var, value) || git_default_config(var, value);
 }
 
 static const char *branch_get_color(enum color_branch ix)
 {
-	if (branch_use_color)
+	if (branch_use_color || git_use_color)
 		return branch_colors[ix];
 	return "";
 }
diff --git a/builtin-log.c b/builtin-log.c
index dcc9f81..03ba732 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -436,7 +436,8 @@ static int git_format_config(const char *var, const char *value)
 		fmt_patch_suffix = xstrdup(value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
+	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")
+			|| !strcmp(var, "color.git")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/cache.h b/cache.h
index 39331c2..0968551 100644
--- a/cache.h
+++ b/cache.h
@@ -329,6 +329,7 @@ extern size_t packed_git_window_size;
 extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int auto_crlf;
+extern int git_use_color;
 
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
diff --git a/color.c b/color.c
index 7f66c29..c891fd8 100644
--- a/color.c
+++ b/color.c
@@ -3,6 +3,8 @@
 
 #define COLOR_RESET "\033[m"
 
+int git_use_color = 0;
+
 static int parse_color(const char *name, int len)
 {
 	static const char * const color_names[] = {
@@ -143,6 +145,16 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 	return 0;
 }
 
+int git_use_color_config(const char *var, const char *value)
+{
+	if(!strcmp(var, "color.git")) {
+		git_use_color = git_config_colorbool(var, value, -1);
+		return 1;
+	}
+
+	return 0;
+}
+
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 		va_list args, const char *trail)
 {
diff --git a/color.h b/color.h
index ff63513..929ebf2 100644
--- a/color.h
+++ b/color.h
@@ -4,7 +4,10 @@
 /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
 #define COLOR_MAXLEN 24
 
+extern int git_use_color;
+
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
+int git_use_color_config(const char *var, const char *value);
 void color_parse(const char *var, const char *value, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
diff --git a/diff.c b/diff.c
index 5bdc111..5fa6d14 100644
--- a/diff.c
+++ b/diff.c
@@ -184,7 +184,7 @@ int git_diff_ui_config(const char *var, const char *value)
 		return 0;
 	}
 
-	return git_default_config(var, value);
+	return git_use_color_config(var, value) || git_default_config(var, value);
 }
 
 static char *quote_two(const char *one, const char *two)
@@ -2021,7 +2021,7 @@ void diff_setup(struct diff_options *options)
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
-	if (diff_use_color_default)
+	if (diff_use_color_default || git_use_color)
 		DIFF_OPT_SET(options, COLOR_DIFF);
 	else
 		DIFF_OPT_CLR(options, COLOR_DIFF);
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 0cdd800..eaae888 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -11,7 +11,7 @@ my ($new_color, $old_color, $fraginfo_color, $metainfo_color, $whitespace_color)
 my ($use_color, $diff_use_color);
 my $repo = Git->repository();
 
-$use_color = $repo->get_colorbool('color.interactive');
+$use_color = $repo->get_colorbool('color.interactive') || $repo->get_colorbool('color.git');
 
 if ($use_color) {
 	# Set interactive colors:
@@ -24,7 +24,7 @@ if ($use_color) {
 	$normal_color = $repo->get_color("", "reset");
 
 	# Do we also set diff colors?
-	$diff_use_color = $repo->get_colorbool('color.diff');
+	$diff_use_color = $repo->get_colorbool('color.diff') || $repo->get_colorbool('color.git');
 	if ($diff_use_color) {
 		$new_color = $repo->get_color("color.diff.new", "green");
 		$old_color = $repo->get_color("color.diff.old", "red");
diff --git a/wt-status.c b/wt-status.c
index c0c2472..1c4169c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -40,7 +40,7 @@ static int parse_status_slot(const char *var, int offset)
 
 static const char* color(int slot)
 {
-	return wt_status_use_color ? wt_status_colors[slot] : "";
+	return wt_status_use_color || git_use_color ? wt_status_colors[slot] : "";
 }
 
 void wt_status_prepare(struct wt_status *s)
@@ -409,5 +409,5 @@ int git_status_config(const char *k, const char *v)
 		wt_status_relative_paths = git_config_bool(k, v);
 		return 0;
 	}
-	return git_default_config(k, v);
+	return git_use_color_config(k, v) || git_default_config(k, v);
 }
-- 
1.5.4.rc2.1105.g90663f

Re: [PATCH/resend] add--interactive: allow diff colors without interactive colors

[Junio C Hamano]

Matthias Kestenholz <mk@spinlock.ch> writes:

> I managed to throw something together which works and passes all
> the tests. Documentation included. :-)

Is it because we do not usually test colours and the tests run
without terminals to make sure "color.* = auto" does not kick
in?

> I would be happy for feedback and suggestions.

 * Shouldn't "color.git = true" with "color.diff = false" mean
   "I want colour for everything by default but I do not want to
   see coloured diff"?

 * git_foo_config() callback from git_config() returns 0 on
   success; the API change needs to be documented to warn
   others.

I haven't studied your patch very deeply so I may have misread
what you tried to do, regarding the first point, though.

Re: [PATCH/resend] add--interactive: allow diff colors without interactive colors

[Matthias Kestenholz]

On Sat, 2008-01-05 at 03:11 -0800, Junio C Hamano wrote:
> Matthias Kestenholz <mk@spinlock.ch> writes:
> 
> > I managed to throw something together which works and passes all
> > the tests. Documentation included. :-)
> 
> Is it because we do not usually test colours and the tests run
> without terminals to make sure "color.* = auto" does not kick
> in?
> 

Probably, yes. I've to confess that I have not thought too much about
how color output could be tested. 

> > I would be happy for feedback and suggestions.
> 
>  * Shouldn't "color.git = true" with "color.diff = false" mean
>    "I want colour for everything by default but I do not want to
>    see coloured diff"?
> 

This works with the new patch below.

>  * git_foo_config() callback from git_config() returns 0 on
>    success; the API change needs to be documented to warn
>    others.
> 

I've changed the config reader. It behaves like the others now.

> I haven't studied your patch very deeply so I may have misread
> what you tried to do, regarding the first point, though.

Still missing are the updates for git add -i and git svn.


I have added a new function git_color_config() which should be called
after git_config() has finished its work. The function needs to run when
all config reading has been done, because otherwise the color setup
would be sensitive to the order in which the variables are placed in the
config file, which I'd rather avoid.

I've also unified the colorbool variables, because otherwise the
individual colorbool variables would need to be exported into other C
files, and I don't see the point of multiple colorbools anyway.

The patch got somewhat big, therefore I've splitted it up into several
pieces:

[PATCH 1/4] Add infrastructure for a single color config variable
[PATCH 2/4] git branch: Use color configuration infrastructure
[PATCH 3/4] status and commit: Use color configuration infrastructure
[PATCH 4/4] diff and log: Use color configuration infrastructure

[PATCH 1/4] Add infrastructure for a single color config variable

[Matthias Kestenholz]

From: Matthias Kestenholz <matthias@spinlock.ch>

Signed-off-by: Matthias Kestenholz <matthias@spinlock.ch>
---
 Documentation/config.txt |    6 ++++++
 color.c                  |   19 +++++++++++++++++++
 color.h                  |   17 +++++++++++++++++
 3 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ee08845..4bebd47 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -438,6 +438,12 @@ color.status.<slot>::
 commit.template::
 	Specify a file to use as the template for new commit messages.
 
+color.ui::
+	When set to `always`, always use colors in all git commands which
+	are capable of colored output. When false (or `never`), never. When
+	set to `true` or `auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 diff.autorefreshindex::
 	When using `git diff` to compare with work tree
 	files, do not consider stat-only change as changed.
diff --git a/color.c b/color.c
index 7f66c29..0792571 100644
--- a/color.c
+++ b/color.c
@@ -3,6 +3,9 @@
 
 #define COLOR_RESET "\033[m"
 
+int git_use_color = -1;
+int git_use_color_default = -1;
+
 static int parse_color(const char *name, int len)
 {
 	static const char * const color_names[] = {
@@ -143,6 +146,22 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 	return 0;
 }
 
+int git_color_default_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "color.ui")) {
+		git_use_color_default = git_config_colorbool(var, value, -1);
+		return 0;
+	}
+
+	return git_default_config(var, value);
+}
+
+void git_color_config()
+{
+	if (git_use_color == -1 && git_use_color_default != -1)
+		git_use_color = git_use_color_default;
+}
+
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 		va_list args, const char *trail)
 {
diff --git a/color.h b/color.h
index ff63513..bcb845d 100644
--- a/color.h
+++ b/color.h
@@ -4,7 +4,24 @@
 /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
 #define COLOR_MAXLEN 24
 
+/*
+ * Use this variable to store the colorbool when reading from the config file.
+ */
+extern int git_use_color;
+
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
+
+/*
+ * Use this instead of git_default_config if you need the value of color.ui.
+ */
+int git_color_default_config(const char *var, const char *value);
+
+/*
+ * Call this function after git_config(config_fn_t fn) to set git_use_color,
+ * respecting the value of color.ui.
+ */
+void git_color_config();
+
 void color_parse(const char *var, const char *value, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-- 
1.5.4.rc2.1104.gec8ae5-dirty

[PATCH 2/4] git branch: Use color configuration infrastructure

[Matthias Kestenholz]

From: Matthias Kestenholz <matthias@spinlock.ch>

Signed-off-by: Matthias Kestenholz <matthias@spinlock.ch>
---
 builtin-branch.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 089cae5..448144f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -31,7 +31,6 @@ static unsigned char head_sha1[20];
 
 static int branch_track = 1;
 
-static int branch_use_color;
 static char branch_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
 	"",		/* PLAIN (normal) */
@@ -65,7 +64,7 @@ static int parse_branch_color_slot(const char *var, int ofs)
 static int git_branch_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "color.branch")) {
-		branch_use_color = git_config_colorbool(var, value, -1);
+		git_use_color = git_config_colorbool(var, value, -1);
 		return 0;
 	}
 	if (!prefixcmp(var, "color.branch.")) {
@@ -76,12 +75,12 @@ static int git_branch_config(const char *var, const char *value)
 	if (!strcmp(var, "branch.autosetupmerge"))
 			branch_track = git_config_bool(var, value);
 
-	return git_default_config(var, value);
+	return git_color_default_config(var, value);
 }
 
 static const char *branch_get_color(enum color_branch ix)
 {
-	if (branch_use_color)
+	if (git_use_color > 0)
 		return branch_colors[ix];
 	return "";
 }
@@ -559,7 +558,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_GROUP("Generic options"),
 		OPT__VERBOSE(&verbose),
 		OPT_BOOLEAN( 0 , "track",  &track, "set up tracking mode (see git-pull(1))"),
-		OPT_BOOLEAN( 0 , "color",  &branch_use_color, "use colored output"),
+		OPT_BOOLEAN( 0 , "color",  &git_use_color, "use colored output"),
 		OPT_SET_INT('r', NULL,     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
 		OPT_CALLBACK(0, "contains", &with_commit, "commit",
@@ -585,6 +584,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_branch_config);
+	git_color_config();
+
 	track = branch_track;
 	argc = parse_options(argc, argv, options, builtin_branch_usage, 0);
 	if (!!delete + !!rename + !!force_create > 1)
-- 
1.5.4.rc2.1104.gec8ae5-dirty

[PATCH 3/4] status and commit: Use color configuration infrastructure

[Matthias Kestenholz]

From: Matthias Kestenholz <matthias@spinlock.ch>

Signed-off-by: Matthias Kestenholz <matthias@spinlock.ch>
---
 builtin-commit.c |    8 +++++---
 wt-status.c      |    7 +++----
 wt-status.h      |    1 -
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 73f1e35..e442b5f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "cache-tree.h"
+#include "color.h"
 #include "dir.h"
 #include "builtin.h"
 #include "diff.h"
@@ -435,10 +436,10 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	if (only_include_assumed)
 		fprintf(fp, "# %s\n", only_include_assumed);
 
-	saved_color_setting = wt_status_use_color;
-	wt_status_use_color = 0;
+	saved_color_setting = git_use_color;
+	git_use_color = 0;
 	commitable = run_status(fp, index_file, prefix, 1);
-	wt_status_use_color = saved_color_setting;
+	git_use_color = saved_color_setting;
 
 	fclose(fp);
 
@@ -639,6 +640,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	int commitable;
 
 	git_config(git_status_config);
+	git_color_config();
 
 	argc = parse_and_validate_options(argc, argv, builtin_status_usage);
 
diff --git a/wt-status.c b/wt-status.c
index c0c2472..ebcbe5a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -9,7 +9,6 @@
 #include "diffcore.h"
 
 int wt_status_relative_paths = 1;
-int wt_status_use_color = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
 	"\033[32m", /* WT_STATUS_UPDATED: green */
@@ -40,7 +39,7 @@ static int parse_status_slot(const char *var, int offset)
 
 static const char* color(int slot)
 {
-	return wt_status_use_color ? wt_status_colors[slot] : "";
+	return git_use_color > 0 ? wt_status_colors[slot] : "";
 }
 
 void wt_status_prepare(struct wt_status *s)
@@ -397,7 +396,7 @@ void wt_status_print(struct wt_status *s)
 int git_status_config(const char *k, const char *v)
 {
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
-		wt_status_use_color = git_config_colorbool(k, v, -1);
+		git_use_color = git_config_colorbool(k, v, -1);
 		return 0;
 	}
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
@@ -409,5 +408,5 @@ int git_status_config(const char *k, const char *v)
 		wt_status_relative_paths = git_config_bool(k, v);
 		return 0;
 	}
-	return git_default_config(k, v);
+	return git_color_default_config(k, v);
 }
diff --git a/wt-status.h b/wt-status.h
index 02afaa6..05b05ba 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -28,7 +28,6 @@ struct wt_status {
 };
 
 int git_status_config(const char *var, const char *value);
-int wt_status_use_color;
 int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
-- 
1.5.4.rc2.1104.gec8ae5-dirty

[PATCH 4/4] diff and log: Use color configuration infrastructure

[Matthias Kestenholz]

From: Matthias Kestenholz <matthias@spinlock.ch>

Signed-off-by: Matthias Kestenholz <matthias@spinlock.ch>
---
 builtin-diff.c |    2 ++
 builtin-log.c  |    9 ++++++++-
 diff.c         |    8 ++++----
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 29365a0..66c5896 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2006 Junio C Hamano
  */
 #include "cache.h"
+#include "color.h"
 #include "commit.h"
 #include "blob.h"
 #include "tag.h"
@@ -229,6 +230,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_diff_ui_config);
+	git_color_config();
 	init_revisions(&rev, prefix);
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
diff --git a/builtin-log.c b/builtin-log.c
index dcc9f81..c9a23fb 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -5,6 +5,7 @@
  *		 2006 Junio Hamano
  */
 #include "cache.h"
+#include "color.h"
 #include "commit.h"
 #include "diff.h"
 #include "revision.h"
@@ -235,6 +236,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config);
+	git_color_config();
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
@@ -307,6 +309,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	int i, count, ret = 0;
 
 	git_config(git_log_config);
+	git_color_config();
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.combine_merges = 1;
@@ -367,6 +370,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config);
+	git_color_config();
 	init_revisions(&rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
@@ -395,6 +399,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config);
+	git_color_config();
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, prefix, &rev);
@@ -436,7 +441,8 @@ static int git_format_config(const char *var, const char *value)
 		fmt_patch_suffix = xstrdup(value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
+	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")
+			|| !strcmp(var, "color.ui")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
@@ -630,6 +636,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	char ref_message_id[1024];
 
 	git_config(git_format_config);
+	git_color_config();
 	init_revisions(&rev, prefix);
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
diff --git a/diff.c b/diff.c
index 5bdc111..ccc958e 100644
--- a/diff.c
+++ b/diff.c
@@ -19,7 +19,6 @@
 
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 100;
-static int diff_use_color_default;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
 
@@ -64,6 +63,7 @@ static void read_config_if_needed(void)
 	if (!user_diff_tail) {
 		user_diff_tail = &user_diff;
 		git_config(git_diff_ui_config);
+		git_color_config();
 	}
 }
 
@@ -147,7 +147,7 @@ int git_diff_ui_config(const char *var, const char *value)
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
-		diff_use_color_default = git_config_colorbool(var, value, -1);
+		git_use_color = git_config_colorbool(var, value, -1);
 		return 0;
 	}
 	if (!strcmp(var, "diff.renames")) {
@@ -184,7 +184,7 @@ int git_diff_ui_config(const char *var, const char *value)
 		return 0;
 	}
 
-	return git_default_config(var, value);
+	return git_color_default_config(var, value);
 }
 
 static char *quote_two(const char *one, const char *two)
@@ -2021,7 +2021,7 @@ void diff_setup(struct diff_options *options)
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
-	if (diff_use_color_default)
+	if (git_use_color > 0)
 		DIFF_OPT_SET(options, COLOR_DIFF);
 	else
 		DIFF_OPT_CLR(options, COLOR_DIFF);
-- 
1.5.4.rc2.1104.gec8ae5-dirty

Re: [PATCH 2/4] git branch: Use color configuration infrastructure

[Jeff King]

On Sat, Jan 05, 2008 at 03:11:37PM +0100, Matthias Kestenholz wrote:

> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> [...]
> -static int branch_use_color;
> [...]
>  	if (!strcmp(var, "color.branch")) {
> -		branch_use_color = git_config_colorbool(var, value, -1);
> +		git_use_color = git_config_colorbool(var, value, -1);
>  		return 0;
>  	}

If I read this right, you are getting rid of the individual "use color"
variables with a single static git_use_color. This will break if two
different color "zones" get used in the same program (e.g.,
color.branch and color.diff, but only one is supposed to be set). I
don't think this is a problem currently, but it seems like a step
backwards in terms of libification.

-Peff

Re: [PATCH 2/4] git branch: Use color configuration infrastructure

[Matthias Kestenholz]

On Tue, 2008-01-08 at 06:23 -0500, Jeff King wrote:
> On Sat, Jan 05, 2008 at 03:11:37PM +0100, Matthias Kestenholz wrote:
> 
> > --- a/builtin-branch.c
> > +++ b/builtin-branch.c
> > [...]
> > -static int branch_use_color;
> > [...]
> >  	if (!strcmp(var, "color.branch")) {
> > -		branch_use_color = git_config_colorbool(var, value, -1);
> > +		git_use_color = git_config_colorbool(var, value, -1);
> >  		return 0;
> >  	}
> 
> If I read this right, you are getting rid of the individual "use color"
> variables with a single static git_use_color. This will break if two
> different color "zones" get used in the same program (e.g.,
> color.branch and color.diff, but only one is supposed to be set). I
> don't think this is a problem currently, but it seems like a step
> backwards in terms of libification.

Yes that's right. I am still not convinced that its worth having
multiple color switches, but since they are there now we should probably
preserve them.

I finally made the application of the default config (color.ui)
explicit. It's a little bit more code and I have to export the
colorbools, but appart from that it's not too bad (I hope). I am
initializing all colorbools to -1 to differentiate between `unset` and
`false`/`never`.

I have doubts about the change in diff.c. That git_diff_basic_config()
calls git_color_default_config() seems not too nice, because color.diff
is evaluated one layer higher, in git_diff_ui_config. It does not do no
harm however, because git_color_default_config() has no side effects.

This is the last complete rewrite of the patch without requests from
outside, I promise :-)  I am rather happy with the current state of
affairs. Many thanks for the reviews and comments.

-- 8< --

>From 1ee26ac15628979b480b087f4dba3375ce1efd13 Mon Sep 17 00:00:00 2001
From: Matthias Kestenholz <matthias@spinlock.ch>
Date: Tue, 8 Jan 2008 13:45:32 +0100
Subject: [PATCH] Add color.ui variable which globally enables colorization if set

Signed-off-by: Matthias Kestenholz <matthias@spinlock.ch>
---
 Documentation/config.txt |    7 +++++++
 builtin-branch.c         |   10 +++++++---
 builtin-commit.c         |    4 ++++
 builtin-diff.c           |    5 +++++
 builtin-log.c            |   17 +++++++++++++++++
 color.c                  |   12 ++++++++++++
 color.h                  |   11 +++++++++++
 diff.c                   |    6 +++---
 diff.h                   |    1 +
 wt-status.c              |    6 +++---
 10 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1b6d6d6..b55f3b4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -439,6 +439,13 @@ color.status.<slot>::
 commit.template::
 	Specify a file to use as the template for new commit messages.
 
+color.ui::
+	When set to `always`, always use colors in all git commands which
+	are capable of colored output. When false (or `never`), never. When
+	set to `true` or `auto`, use colors only when the output is to the
+	terminal. When more specific variables of color.* are set, they always
+	take precedence over this setting. Defaults to false.
+
 diff.autorefreshindex::
 	When using `git diff` to compare with work tree
 	files, do not consider stat-only change as changed.
diff --git a/builtin-branch.c b/builtin-branch.c
index 089cae5..9a1eb21 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -31,7 +31,7 @@ static unsigned char head_sha1[20];
 
 static int branch_track = 1;
 
-static int branch_use_color;
+static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
 	"",		/* PLAIN (normal) */
@@ -76,12 +76,12 @@ static int git_branch_config(const char *var, const char *value)
 	if (!strcmp(var, "branch.autosetupmerge"))
 			branch_track = git_config_bool(var, value);
 
-	return git_default_config(var, value);
+	return git_color_default_config(var, value);
 }
 
 static const char *branch_get_color(enum color_branch ix)
 {
-	if (branch_use_color)
+	if (branch_use_color > 0)
 		return branch_colors[ix];
 	return "";
 }
@@ -585,6 +585,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_branch_config);
+
+	if (branch_use_color == -1)
+		branch_use_color = git_use_color_default;
+
 	track = branch_track;
 	argc = parse_options(argc, argv, options, builtin_branch_usage, 0);
 	if (!!delete + !!rename + !!force_create > 1)
diff --git a/builtin-commit.c b/builtin-commit.c
index 73f1e35..ba60cfa 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "cache-tree.h"
+#include "color.h"
 #include "dir.h"
 #include "builtin.h"
 #include "diff.h"
@@ -640,6 +641,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	git_config(git_status_config);
 
+	if (wt_status_use_color == -1)
+		wt_status_use_color = git_use_color_default;
+
 	argc = parse_and_validate_options(argc, argv, builtin_status_usage);
 
 	index_file = prepare_index(argc, argv, prefix);
diff --git a/builtin-diff.c b/builtin-diff.c
index 29365a0..77a9c9a 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2006 Junio C Hamano
  */
 #include "cache.h"
+#include "color.h"
 #include "commit.h"
 #include "blob.h"
 #include "tag.h"
@@ -229,6 +230,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_diff_ui_config);
+
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	init_revisions(&rev, prefix);
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
diff --git a/builtin-log.c b/builtin-log.c
index dcc9f81..880da94 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -5,6 +5,7 @@
  *		 2006 Junio Hamano
  */
 #include "cache.h"
+#include "color.h"
 #include "commit.h"
 #include "diff.h"
 #include "revision.h"
@@ -235,6 +236,10 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config);
+
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
@@ -307,6 +312,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	int i, count, ret = 0;
 
 	git_config(git_log_config);
+
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.combine_merges = 1;
@@ -367,6 +376,10 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config);
+
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	init_revisions(&rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
@@ -395,6 +408,10 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config);
+
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, prefix, &rev);
diff --git a/color.c b/color.c
index 7f66c29..09b81fe 100644
--- a/color.c
+++ b/color.c
@@ -3,6 +3,8 @@
 
 #define COLOR_RESET "\033[m"
 
+int git_use_color_default = 0;
+
 static int parse_color(const char *name, int len)
 {
 	static const char * const color_names[] = {
@@ -143,6 +145,16 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 	return 0;
 }
 
+int git_color_default_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "color.ui")) {
+		git_use_color_default = git_config_colorbool(var, value, -1);
+		return 0;
+	}
+
+	return git_default_config(var, value);
+}
+
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 		va_list args, const char *trail)
 {
diff --git a/color.h b/color.h
index ff63513..ecda556 100644
--- a/color.h
+++ b/color.h
@@ -4,6 +4,17 @@
 /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
 #define COLOR_MAXLEN 24
 
+/*
+ * This variable stores the value of color.ui
+ */
+extern int git_use_color_default;
+
+
+/*
+ * Use this instead of git_default_config if you need the value of color.ui.
+ */
+int git_color_default_config(const char *var, const char *value);
+
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
 void color_parse(const char *var, const char *value, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
diff --git a/diff.c b/diff.c
index b18c140..d6f23c7 100644
--- a/diff.c
+++ b/diff.c
@@ -20,7 +20,7 @@
 
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 100;
-static int diff_use_color_default;
+int diff_use_color_default = -1;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
 
@@ -189,7 +189,7 @@ int git_diff_basic_config(const char *var, const char *value)
 		}
 	}
 
-	return git_default_config(var, value);
+	return git_color_default_config(var, value);
 }
 
 static char *quote_two(const char *one, const char *two)
@@ -2048,7 +2048,7 @@ void diff_setup(struct diff_options *options)
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
-	if (diff_use_color_default)
+	if (diff_use_color_default > 0)
 		DIFF_OPT_SET(options, COLOR_DIFF);
 	else
 		DIFF_OPT_CLR(options, COLOR_DIFF);
diff --git a/diff.h b/diff.h
index 073d5cb..8e73f07 100644
--- a/diff.h
+++ b/diff.h
@@ -174,6 +174,7 @@ extern void diff_unmerge(struct diff_options *,
 
 extern int git_diff_basic_config(const char *var, const char *value);
 extern int git_diff_ui_config(const char *var, const char *value);
+extern int diff_use_color_default;
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
 extern int diff_setup_done(struct diff_options *);
diff --git a/wt-status.c b/wt-status.c
index c0c2472..0dfc909 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -9,7 +9,7 @@
 #include "diffcore.h"
 
 int wt_status_relative_paths = 1;
-int wt_status_use_color = 0;
+int wt_status_use_color = -1;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
 	"\033[32m", /* WT_STATUS_UPDATED: green */
@@ -40,7 +40,7 @@ static int parse_status_slot(const char *var, int offset)
 
 static const char* color(int slot)
 {
-	return wt_status_use_color ? wt_status_colors[slot] : "";
+	return wt_status_use_color > 0 ? wt_status_colors[slot] : "";
 }
 
 void wt_status_prepare(struct wt_status *s)
@@ -409,5 +409,5 @@ int git_status_config(const char *k, const char *v)
 		wt_status_relative_paths = git_config_bool(k, v);
 		return 0;
 	}
-	return git_default_config(k, v);
+	return git_color_default_config(k, v);
 }
-- 
1.5.4.rc2.68.ge708a-dirty

Re: [PATCH/resend] add--interactive: allow diff colors without interactive colors

[Jakub Narebski]

Matthias Kestenholz wrote:

> +color.git::
> +       When set to `always`, always use colors in all git commands which
> +       are capable of colored output. When false (or `never`), never. When
> +       set to `true` or `auto`, use colors only when the output is to the
> +       terminal. Defaults to false.
> +

Not "color.ui"?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git