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

> Matthieu Moy <matthieu....@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
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

Reply via email to