is this a bug of git-diff?

is this a bug of git-diff?

[eric liou]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

The output of git-diff is different from my expectation.
It may skip some lines of context.
For the case of the diff result attached here, a blank line and a line
with a leading slash is skipped.

Please check out the attached files for details.

Thanks.

[-- Attachment #2: ab.patch --]
[-- Type: application/octet-stream, Size: 110 bytes --]

index 1ef2bf5..ef2206b 100644
--- a/t.c
+++ b/t.c
@@ -4,5 +4,6 @@ int a = 1;
  * 1
  * 2
  * 3
+ * added
  */

[-- Attachment #3: b.c --]
[-- Type: text/x-csrc, Size: 44 bytes --]

int a = 1;

/*
 * 1
 * 2
 * 3
 * added
 */
 

[-- Attachment #4: a.c --]
[-- Type: text/x-csrc, Size: 35 bytes --]

int a = 1;

/*
 * 1
 * 2
 * 3
 */
 

Re: is this a bug of git-diff?

[Antoine Pelisse]

On Wed, May 15, 2013 at 8:23 AM, eric liou <accwuya@gmail.com> wrote:
> The output of git-diff is different from my expectation.
> It may skip some lines of context.

git-diff is using a default of 3 lines of context above and below the changes.
In your example, there is only two lines of context below the change,
so only two lines are displayed.
Above the change, three lines are displayed, as expected. That's why
the blank line and leading slash line are not displayed.
You can change the number of context lines by invoking git-diff with -U<n>.

Hope that helps,
Antoine

Re: is this a bug of git-diff?

[Antoine Pelisse]

On Wed, May 15, 2013 at 8:52 AM, eric liou <accwuya@gmail.com> wrote:
> Thank you for the quick reply.
> But this line is not correct: "@@ -4,5 +4,6 @@ int a = 1;"

Oh OK, I see.
Git tries to name the function where the changes take place. This is
purely informative.
In your example, you don't have any function so of course the
information is not very helpful.

Typically it will look like the following, helping the reader by
giving the function name:

@@ -591,6 +609,14 @@ int cmd_grep(int argc, const char **argv, const
char *prefix)
                paths[1] = NULL;
        }

+       if (!use_index) {
+               if (cached)
+                       die("--cached cannot be used with --no-index.");
+               if (list.nr)
+                       die("--no-index cannot be used with revs.");
+               return !grep_directory(&opt, paths);
+       }
+
        if (!list.nr) {
                if (!cached)
                        setup_work_tree();

Re: is this a bug of git-diff?

[Matthieu Moy]

Antoine Pelisse <apelisse@gmail.com> writes:

> On Wed, May 15, 2013 at 8:52 AM, eric liou <accwuya@gmail.com> wrote:
>> Thank you for the quick reply.
>> But this line is not correct: "@@ -4,5 +4,6 @@ int a = 1;"

Antoine's answer is correct. In addition, I'd say that you may want to
enable color in the output to make it clearer (the @@ ... @@ part would
be colored, but not the function name):

  git config --global color.ui auto

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: is this a bug of git-diff?

[John Keeping]

On Wed, May 15, 2013 at 11:34:41AM +0200, Matthieu Moy wrote:
> Antoine's answer is correct. In addition, I'd say that you may want to
> enable color in the output to make it clearer (the @@ ... @@ part would
> be colored, but not the function name):
> 
>   git config --global color.ui auto

I wonder if that should be the default.  I've advised a lot of people to
turn it on and it seems to me that a user is much more likely to go
looking for a "turn color off" option than realise that color is an
option at all.

Default for color.ui (was Re: is this a bug of git-diff?)

[Matthieu Moy]

John Keeping <john@keeping.me.uk> writes:

> I wonder if that should be the default.  I've advised a lot of people to
> turn it on and it seems to me that a user is much more likely to go
> looking for a "turn color off" option than realise that color is an
> option at all.

I'd love to see this by default, yes. Maybe a 2.0 change?

If people agree that this is a good change, would we need a transition
plan? I'd say no, as there is no real backward incompatibility involved.
People who dislike colors can already set color.ui=false, and seeing
colors can hardly harm them, just temporarily reduce the comfort for
them.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Default for color.ui (was Re: is this a bug of git-diff?)

[Felipe Contreras]

On Wed, May 15, 2013 at 5:03 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> I wonder if that should be the default.  I've advised a lot of people to
>> turn it on and it seems to me that a user is much more likely to go
>> looking for a "turn color off" option than realise that color is an
>> option at all.
>
> I'd love to see this by default, yes. Maybe a 2.0 change?
>
> If people agree that this is a good change, would we need a transition
> plan? I'd say no, as there is no real backward incompatibility involved.
> People who dislike colors can already set color.ui=false, and seeing
> colors can hardly harm them, just temporarily reduce the comfort for
> them.

I vote for this. It's the first thing I do in any setup, even the ones
that are note mine. I've also seen it in basically all the tutorials,
even before setting user.name/email.

I also don't see the point of a transition plan.

-- 
Felipe Contreras

[PATCH] make color.ui default to 'auto'

[Matthieu Moy]

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 <Matthieu.Moy@imag.fr>
---
> > I'd love to see this by default, yes. Maybe a 2.0 change?
> >
> > If people agree that this is a good change, would we need a transition
> > plan? I'd say no, as there is no real backward incompatibility involved.
> > People who dislike colors can already set color.ui=false, and seeing
> > colors can hardly harm them, just temporarily reduce the comfort for
> > them.
> 
> I vote for this. It's the first thing I do in any setup, even the ones
> that are note mine. I've also seen it in basically all the tutorials,
> even before setting user.name/email.
> 
> I also don't see the point of a transition plan.

OK, then let's try turning the discussion into code.

I'm starting to wonder why we didn't do this earlier ;-).

 Documentation/config.txt | 11 ++++++-----
 color.c                  |  2 +-
 2 files changed, 7 insertions(+), 6 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/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.313.geb32591.dirty

Re: [PATCH] make color.ui default to 'auto'

[Johan Herland]

On Wed, May 15, 2013 at 2:09 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>

Reviewed and supported-by: Johan Herland <johan@herland.net>


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH v2] make color.ui default to 'auto'

[Matthieu Moy]

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 <Matthieu.Moy@imag.fr>
---
> Reviewed and supported-by: Johan Herland <johan@herland.net>

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

Re: [PATCH v2] make color.ui default to 'auto'

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> 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).

Re: [PATCH v2] make color.ui default to 'auto'

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> 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/

[PATCH 1/2] config: refactor management of color.ui's default value

[Matthieu Moy]

The meaning of get_colorbool_found and get_diff_color_found is "the
config value if found, and -1 otherwise", but get_color_ui_found had a
slightly different meaning, as it has the value 0 (which corresponds to
the default value from the user point of view) when color.ui is unset.

Make get_color_ui_found default to -1, and make it explicit that 0 is the
default value when nothing else is found.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
So, this is new, as suggested by Junio.

 builtin/config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/config.c b/builtin/config.c
index 000d27c..171bad7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -333,6 +333,7 @@ static int get_colorbool(int print)
 {
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
+	get_color_ui_found = -1;
 	git_config_with_options(git_get_colorbool_config, NULL,
 				given_config_file, given_config_blob,
 				respect_includes);
@@ -344,6 +345,10 @@ static int get_colorbool(int print)
 			get_colorbool_found = get_color_ui_found;
 	}
 
+	if (get_colorbool_found < 0)
+		/* default value if none found in config */
+		get_colorbool_found = 0;
+
 	get_colorbool_found = want_color(get_colorbool_found);
 
 	if (print) {
-- 
1.8.3.rc1.315.g4602f33

[PATCH 2/2 v4] make color.ui default to 'auto'

[Matthieu Moy]

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,
which tend to indicate that color.ui=none is not the recommanded value,
hence should not be the default.

These tutorials would benefit from skipping this step and starting the
real Git manipulations 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 preferred 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), but this needs not occupy space in
beginner-oriented documentations.

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 <Matthieu.Moy@imag.fr>
---
Adapted after PATCH 1/2, and commit message updated.

 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 171bad7..4010c43 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -347,7 +347,7 @@ static int get_colorbool(int print)
 
 	if (get_colorbool_found < 0)
 		/* default value if none found in config */
-		get_colorbool_found = 0;
+		get_colorbool_found = GIT_COLOR_AUTO;
 
 	get_colorbool_found = want_color(get_colorbool_found);
 
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.315.g4602f33

Re: [PATCH v2] make color.ui default to 'auto'

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@imag.fr> 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.

Re: [PATCH] make color.ui default to 'auto'

[Stefano Lattarini]

On 05/15/2013 02:09 PM, Matthieu Moy wrote:
> 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
>
s/skiping/skipping/

> this step and starting the
> real Git manipualtions earlier.
>
s/manipualtions/manipulations/

> 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.
>
s/prefered/preferred/

> 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 <Matthieu.Moy@imag.fr>
> ---
>>> I'd love to see this by default, yes. Maybe a 2.0 change?
>>>
>>> If people agree that this is a good change, would we need a transition
>>> plan? I'd say no, as there is no real backward incompatibility involved.
>>> People who dislike colors can already set color.ui=false, and seeing
>>> colors can hardly harm them, just temporarily reduce the comfort for
>>> them.
>>
>> I vote for this. It's the first thing I do in any setup, even the ones
>> that are note mine. I've also seen it in basically all the tutorials,
>> even before setting user.name/email.
>>
>> I also don't see the point of a transition plan.
> 
> OK, then let's try turning the discussion into code.
> 
> I'm starting to wonder why we didn't do this earlier ;-).
> 
>  Documentation/config.txt | 11 ++++++-----
>  color.c                  |  2 +-
>  2 files changed, 7 insertions(+), 6 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/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;
>  
>  /*
>
With the typos above fixed:

  Reviewed and supported-by: Stefano Lattarini <stefano.lattarini@gmail.com>

Thanks,
  Stefano

[PATCH v3] make color.ui default to 'auto'

[Matthieu Moy]

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 skipping this step and starting the
real Git manipulations 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 preferred 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 <Matthieu.Moy@imag.fr>
---
v2 crossed Stefano's email with typos. v3 just fixes these typos in
the commit message.

 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

Re: [PATCH] make color.ui default to 'auto'

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

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

The above two paragraphs do not make a good justification [*1*].
The former can just as easily websearch for "enable colours in git"
as the latter would for "disable" in order to avoid having to live
with distraction while they may have preferred monochrome.

The train of thought that is a sufficient justification for this
change is "Our document and third-party tutorials often start with
setting color.ui=auto configuration." leading to "Our recommendation
is to enable colour on terminals." which in turn leading to "Why is
our default monochrome, against our own recommendation?".  Saying
anything more, like who are the majority or how easily the default
can be overridden, is unnecessary, I think [*2*].

As this is purely a UI thing, and since daa0c3d97176 (color: delay
auto-color decision until point of use, 2011-08-17), the logic to
decide when "auto colouring" is triggered is centrary controlled
(hence it is much less likely than before that color.ui=auto could
misfire when it shouldn't), I agree that this does not even deserve
a warning. You could even sell it as a pure bugfix ("we recommend
users to use auto colouring but we did not set it up for users").

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

Good.

> I'm starting to wonder why we didn't do this earlier ;-).
>
>  Documentation/config.txt | 11 ++++++-----
>  color.c                  |  2 +-
>  2 files changed, 7 insertions(+), 6 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 `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.

OK, so this is planned for 2.0?


[Footnote]

*1* Unless you have some statistical fact to demonstrate that
beginners who prefer colours are of lessor intelligence than
those who do not, that is.

*2* It unnecessarily muddies the water to bring up "which is
majority?".  A poll might reveal more people prefer monochrome, but
in that case, either we keep the default monochrome *and* fix the
tutorial not to suggest auto, or we stick to the recommendation to
use auto colouring.  In other words, I see this change as merely
making the code in line with the spirit of the documentation.

Re: [PATCH] make color.ui default to 'auto'

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> 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).
>
> The above two paragraphs do not make a good justification [*1*].
> The former can just as easily websearch for "enable colours in git"

I disagree: I do not know anyone who would be really harmed by colors
(and such users would most likely have a terminal configured without
colors I guess), so although I can imagine some people feeling less
comfortable, disabling colors can be deferred to much later in the
learning process.

When I teach Git to students (a relatively short tutorial), I currently
ask them to type a ~/.gitconfig containing color.ui=auto before anything
else. If this was the default, I would skip this completely from the
beginner-oriented doc, and I would mention color.ui=never only to people
complaining about colors. It's really about _skipping_ the color-related
stuff from the newbie docs, not about reverting them.

Also, as my message points out, with "disabled by default", many people
do not know that it is possible to have it, hence won't google for
anything related to colors. There's no symmetry either here: with colors
enabled by default, people will know that Git can use colors.

>> 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 `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.
>
> OK, so this is planned for 2.0?

We've lived without this for years, so I'd say it can wait untill Git
2.0. It may give a "Wow" effect to some users when upgrading ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] make color.ui default to 'auto'

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> The above two paragraphs do not make a good justification [*1*].
>> The former can just as easily websearch for "enable colours in git"
>
> I disagree: I do not know anyone who would be really harmed by colors
> ...

I actually am one of them (light cyan or green on white background
with small font is very hard to read for me), but I think you are
missing the entire point, which is not "is anyone harmed?"

This patch is not even about deciding if colored output should be
the default.  That has already been decided by documentation for us
long time ago; the patch does not have to (and should not) argue for
and justify why color is good.  Our recommendation has been "use
color=auto", and change of the in-code default is merely to make the
default in line with that recommendation.

Re: [PATCH] make color.ui default to 'auto'

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I think you are missing the entire point, which is not "is anyone
> harmed?"

Again, it is. If the new default is really harmful for too many people,
then documentations will have to mention how to fix it.

And really, I do not forsee any newbie-oriented starting with "here's
how to disable colors in case you need it", because of the reasons
mentionned in the message.

> Our recommendation has been "use color=auto"

Not really. Neither Documentation/gittutorial.txt nor
Documentation/user-manual.txt mention colors. Pro Git mentions it, but
more as a possibility than as a recommandation. This is the
recommandation of the rest of the world, not "ours".

It's not "either we update the docs or we update the code", it's "follow
what the rest of the world is doing", and "rest of the world" has to
imply a notion of majority (not all tutorials talk about color.ui).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] make color.ui default to 'auto'

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I think you are missing the entire point, which is not "is anyone
>> harmed?"
>
> Again, it is. If the new default is really harmful for too many people,
> then documentations will have to mention how to fix it.
>
> And really, I do not forsee any newbie-oriented starting with "here's
> how to disable colors in case you need it", because of the reasons
> mentionned in the message.
>
>> Our recommendation has been "use color=auto"
>
> Not really. Neither Documentation/gittutorial.txt nor
> Documentation/user-manual.txt mention colors. Pro Git mentions it, but
> more as a possibility than as a recommandation. This is the
> recommandation of the rest of the world, not "ours".

Do you mean the git users who learn and use Git without being in the
circle of people who updates Documentation/ hierarchy "the rest of
the world"?

I think that is a flawed mentality.  They are part of "us".

> It's not "either we update the docs or we update the code", it's "follow
> what the rest of the world is doing", and "rest of the world" has to
> imply a notion of majority (not all tutorials talk about color.ui).

Yes, exactly.

Read the statement you made again, with the assumption that
everybody ("the rest of the world") already knows (and/or agreed to)
colouring is a good thing.

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

Now, realize that after switching the default, these "few people"
have to live with distracting (or unreadable) output.  Because these
people are minority, their websearch "disable colors in git" will by
definition have smaller number of hits than "enable colors in git"
the above claims people "may not discover it by themselves".  In a
way, you are making things even harder because these minority do not
have many similar others to ask help for.

That is the honest way to express what you said in the second
paragraph.

If we really want to justify the changing of the default, we should
not try to weasel out by using asymmetric wording from the fact that
we are making things less convenient for one kind of people.  We
should be honest and say what we are doing: "it will make things
easier for majority while making it less convenient for minority".

I am however saying that in this case, we are better off not even
trying to come up with such a lame excuse for us to hurt color-blind
people in order to make things easier for majority.  Just saying
"the rest of the world prefer automatic color and that is what we
recommend, so make the code match" should be sufficient.

Re: [PATCH] make color.ui default to 'auto'

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Now, realize that after switching the default, these "few people"
> have to live with distracting (or unreadable) output.  Because these
> people are minority, their websearch "disable colors in git" will by
> definition have smaller number of hits than "enable colors in git"
> the above claims people "may not discover it by themselves".

As my message says, "disable colors in git" already gives you the
answer, today (1st hit in Google). I'm not worried about the difficulty
to find the information in the future.

> We should be honest and say what we are doing: "it will make things
> easier for majority while making it less convenient for minority".

I thought this was what I did, but your first complain was I was
mentionning the majority, and you are now suggesting something about
majority/minority, so I'm lost.

In any case, feel free to change the commit message, what's really
important is the actual change, and it does not seem controversial.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] make color.ui default to 'auto'

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> We should be honest and say what we are doing: "it will make things
>> easier for majority while making it less convenient for minority".
>
> I thought this was what I did, but your first complain was I was
> mentionning the majority, and you are now suggesting something about
> majority/minority, so I'm lost.

Not really.  My main complaint is that you were making it sound as
if the inconvenience for the "majority" is very severe with "many
not discover", "live with", and such phrases, while making the
inconveience you are placing on the "minority" trivial with "easily
set" and "already tells them".  That sounds a lot more like making a
lame excuse than doing a balanced analysis of pros and cons of the
change.

Re: [PATCH] make color.ui default to 'auto'

[Felipe Contreras]

On Wed, May 15, 2013 at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>> We should be honest and say what we are doing: "it will make things
>>> easier for majority while making it less convenient for minority".
>>
>> I thought this was what I did, but your first complain was I was
>> mentionning the majority, and you are now suggesting something about
>> majority/minority, so I'm lost.
>
> Not really.  My main complaint is that you were making it sound as
> if the inconvenience for the "majority" is very severe with "many
> not discover", "live with", and such phrases, while making the
> inconveience you are placing on the "minority" trivial with "easily
> set" and "already tells them".  That sounds a lot more like making a
> lame excuse than doing a balanced analysis of pros and cons of the
> change.

I could barely parse this, but I've found that many colleagues didn't
know about this configuration. And I don't see why anybody would not
want this. The minority that don't want this can search the interwebs
to find out how to disable the unwanted behavior, so the majority that
do want this don't have to enable it all the time (*if* they know
about it).

-- 
Felipe Contreras

Re: [PATCH] make color.ui default to 'auto'

[John Keeping]

On Wed, May 15, 2013 at 08:42:39AM -0700, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> 
> > 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).
> 
> The above two paragraphs do not make a good justification [*1*].
> The former can just as easily websearch for "enable colours in git"
> as the latter would for "disable" in order to avoid having to live
> with distraction while they may have preferred monochrome.
> 
> The train of thought that is a sufficient justification for this
> change is "Our document and third-party tutorials often start with
> setting color.ui=auto configuration." leading to "Our recommendation
> is to enable colour on terminals." which in turn leading to "Why is
> our default monochrome, against our own recommendation?".  Saying
> anything more, like who are the majority or how easily the default
> can be overridden, is unnecessary, I think [*2*].
> 
> As this is purely a UI thing, and since daa0c3d97176 (color: delay
> auto-color decision until point of use, 2011-08-17), the logic to
> decide when "auto colouring" is triggered is centrary controlled
> (hence it is much less likely than before that color.ui=auto could
> misfire when it shouldn't), I agree that this does not even deserve
> a warning. You could even sell it as a pure bugfix ("we recommend
> users to use auto colouring but we did not set it up for users").
> 
> > 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.
> 
> Good.
> 
> > I'm starting to wonder why we didn't do this earlier ;-).
> >
> >  Documentation/config.txt | 11 ++++++-----
> >  color.c                  |  2 +-
> >  2 files changed, 7 insertions(+), 6 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 `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.
> 
> OK, so this is planned for 2.0?

I would vote for just considering this a bugfix as you say above and
therefore not worthy of any special treatment, so it should end up in
whatever the next release is after it hits master.

The changes that are being held back for 2.0 change how commands operate
and we don't provide any overrides for those; this is just a cosmetic
change to the default output format.

Re: is this a bug of git-diff?

[Mike Hommey]

On Wed, May 15, 2013 at 10:50:25AM +0100, John Keeping wrote:
> On Wed, May 15, 2013 at 11:34:41AM +0200, Matthieu Moy wrote:
> > Antoine's answer is correct. In addition, I'd say that you may want to
> > enable color in the output to make it clearer (the @@ ... @@ part would
> > be colored, but not the function name):
> > 
> >   git config --global color.ui auto
> 
> I wonder if that should be the default.  I've advised a lot of people to
> turn it on and it seems to me that a user is much more likely to go
> looking for a "turn color off" option than realise that color is an
> option at all.

+1. My settings have been there for so long that I thought it was the
default.

Mike

[PATCH 1/2] config: refactor management of color.ui's default value

[Matthieu Moy]

The meaning of get_colorbool_found and get_diff_color_found is "the
config value if found, and -1 otherwise", but get_color_ui_found had a
slightly different meaning, as it has the value 0 (which corresponds to
the default value from the user point of view) when color.ui is unset.

Make get_color_ui_found default to -1, and make it explicit that 0 is the
default value when nothing else is found.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
This one is just a resend of what's already in pu.

 builtin/config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..057bb61 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -329,6 +329,7 @@ static int get_colorbool(int print)
 {
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
+	get_color_ui_found = -1;
 	git_config_with_options(git_get_colorbool_config, NULL,
 				given_config_file, respect_includes);
 
@@ -339,6 +340,10 @@ static int get_colorbool(int print)
 			get_colorbool_found = get_color_ui_found;
 	}
 
+	if (get_colorbool_found < 0)
+		/* default value if none found in config */
+		get_colorbool_found = 0;
+
 	get_colorbool_found = want_color(get_colorbool_found);
 
 	if (print) {
-- 
1.8.3.rc3.8.g5e49f30

Re: [PATCH 1/2] config: refactor management of color.ui's default value

[Junio C Hamano]

Thanks.