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

Matthieu Moy [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

Matthieu Moy [off-list ref] writes:

quoted

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.

quoted

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:

quoted

	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.