Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> writes: > Junio C Hamano <gitster@xxxxxxxxx> writes: > >> Matthieu Moy <Matthieu.Moy@xxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html