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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 15, 2013, 16:52 UTC
Message-ID: <vpq61ykfang.fsf@grenoble-inp.fr>
In-Reply-To: <7vtxm4cjil.fsf@alter.siamese.dyndns.org>

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

Show 16 quoted lines

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