Re: [PATCH v2] make color.ui default to 'auto'
Matthieu Moy writes: > Junio C Hamano writes: > >> Matthieu Moy writes: >> >>> diff --git a/builtin/config.c b/builtin/config.c >>> index 000d27c..ecfceca 100644 >>> --- a/builtin/config.c >>> +++ b/builtin/config.c >>> @@ -316,7 +316,7 @@ static void get_color(const char *def_color) >>> >>> static int get_colorbool_found; >>> static int get_diff_color_found; >>> -static int get_color_ui_found; >>> +static int get_color_ui_found = GIT_COLOR_AUTO; >> >> It is curious to notice that we have these three and only one is >> initialized to the new default value, while the other two get -1 >> at the beginning of get_colorbool(). > > Right. The meaning of the _found suffix is clear for the first two, but > not the last. > >> I wonder if it would be cleaner to statically initialize all three >> to -1 here, drop the assignment of -1 to two of them from the >> beginning of get_colorbool(), and then have a final fallback inside >> the want_color() call itself, i.e. > > I've left the assignments within the function (I like the initialisation > right before usage, I don't have to worry about how many times the > function is called then), but I've added a patch that initializes > get_color_ui_found to -1 like the others, and does essentially this: > >> get_colorbool_found = want_color(get_colorbool_found < 0 >> ? GIT_COLOR_AUTO >> : get_colorbool_found); > > Except I've made it a separate if statement. Then PATCH 2/2 is really > crystal clear. Yeah, sounds good. > Reroll comming, with an improved commit message that should adress the > points in the other message. Hmm, I don't see much improvement in the message, though. It seems to talk about "may not discover", "live with", "a few people", and "they can easily", none of which should be there. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] make color.ui default to 'auto'
Junio C Hamano writes: > Matthieu Moy writes: > >> diff --git a/builtin/config.c b/builtin/config.c >> index 000d27c..ecfceca 100644 >> --- a/builtin/config.c >> +++ b/builtin/config.c >> @@ -316,7 +316,7 @@ static void get_color(const char *def_color) >> >> static int get_colorbool_found; >> static int get_diff_color_found; >> -static int get_color_ui_found; >> +static int get_color_ui_found = GIT_COLOR_AUTO; > > It is curious to notice that we have these three and only one is > initialized to the new default value, while the other two get -1 > at the beginning of get_colorbool(). Right. The meaning of the _found suffix is clear for the first two, but not the last. > I wonder if it would be cleaner to statically initialize all three > to -1 here, drop the assignment of -1 to two of them from the > beginning of get_colorbool(), and then have a final fallback inside > the want_color() call itself, i.e. I've left the assignments within the function (I like the initialisation right before usage, I don't have to worry about how many times the function is called then), but I've added a patch that initializes get_color_ui_found to -1 like the others, and does essentially this: > get_colorbool_found = want_color(get_colorbool_found < 0 > ? GIT_COLOR_AUTO > : get_colorbool_found); Except I've made it a separate if statement. Then PATCH 2/2 is really crystal clear. Reroll comming, with an improved commit message that should adress the points in the other message. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] make color.ui default to 'auto'
Matthieu Moy writes: > diff --git a/builtin/config.c b/builtin/config.c > index 000d27c..ecfceca 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -316,7 +316,7 @@ static void get_color(const char *def_color) > > static int get_colorbool_found; > static int get_diff_color_found; > -static int get_color_ui_found; > +static int get_color_ui_found = GIT_COLOR_AUTO; It is curious to notice that we have these three and only one is initialized to the new default value, while the other two get -1 at the beginning of get_colorbool(). I wonder if it would be cleaner to statically initialize all three to -1 here, drop the assignment of -1 to two of them from the beginning of get_colorbool(), and then have a final fallback inside the want_color() call itself, i.e. get_colorbool_found = want_color(get_colorbool_found < 0 ? GIT_COLOR_AUTO : get_colorbool_found); so that it is clear that -1 consistently mean "We haven't read any value from the configuration file for this variable", instead of making get_color_ui_found mean slightly different thing (the value read from the configuration; GIT_COLOR_AUTO means we cannot tell if we saw this variable or the user specified auto) from the other two (the value read from the configuration; -1 means we did not find any). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] make color.ui default to 'auto'
Most users seem to like having colors enabled, and colors can help
beginners to understand the output of some commands (e.g. notice
immediately the boundary between commits in the output of "git log").
Many tutorials tell the users to set color.ui=auto as a very first step.
These tutorials would benefit from skiping this step and starting the
real Git manipualtions earlier. Other beginners do not know about
color.ui=auto, and may not discover it by themselves, hence live with
black&white outputs while they may have prefered colors.
A few people (e.g. color-blind) prefer having no colors, but they can
easily set color.ui=never for this (and googling "disable colors in git"
already tells them how to do so).
A transition period with Git emitting a warning when color.ui is unset
would be possible, but the discomfort of having the warning seems
superior to the benefit: users may be surprised by the change, but not
harmed by it.
The default value is changed, and the documentation is reworded to
mention "color.ui=false" first, since the primary use of color.ui after
this change is to disable colors, not to enable it.
Signed-off-by: Matthieu Moy
---
> Reviewed and supported-by: Johan Herland
Apparently not well enough ;-).
In v1, "git config --get-colorbool" was not affected, hence "git add
-p" wasn't colored. v2 fixes this.
Documentation/config.txt | 11 ++-
builtin/config.c | 2 +-
color.c | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1009bfc..97550be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -913,11 +913,12 @@ color.ui::
as `color.diff` and `color.grep` that control the use of color
per command family. Its scope will expand as more commands learn
configuration to set a default for the `--color` option. Set it
- to `always` if you want all output not intended for machine
- consumption to use color, to `true` or `auto` if you want such
- output to use color when written to the terminal, or to `false` or
- `never` if you prefer Git commands not to use color unless enabled
- explicitly with some other configuration or the `--color` option.
+ to `false` or `never` if you prefer Git commands not to use
+ color unless enabled explicitly with some other configuration
+ or the `--color` option. Set it to `always` if you want all
+ output not intended for machine consumption to use color, to
+ `true` or `auto` (this is the default since Git 2.0) if you
+ want such output to use color when written to the terminal.
column.ui::
Specify whether supported commands should output in columns.
diff --git a/builtin/config.c b/builtin/config.c
index 000d27c..ecfceca 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -316,7 +316,7 @@ static void get_color(const char *def_color)
static int get_colorbool_found;
static int get_diff_color_found;
-static int get_color_ui_found;
+static int get_color_ui_found = GIT_COLOR_AUTO;
static int git_get_colorbool_config(const char *var, const char *value,
void *cb)
{
diff --git a/color.c b/color.c
index e8e2681..f672885 100644
--- a/color.c
+++ b/color.c
@@ -1,7 +1,7 @@
#include "cache.h"
#include "color.h"
-static int git_use_color_default = 0;
+static int git_use_color_default = GIT_COLOR_AUTO;
int color_stdout_is_tty = -1;
/*
--
1.8.3.rc1.314.g2261e40.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
