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

[Junio C Hamano <gitster@pobox.com>]

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.