On Tue, Apr 26, 2016 at 10:36 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
>> +enum ws_error_action {
>> + nowarn_ws_error,
>> + warn_on_ws_error,
>> + die_on_ws_error,
>> + correct_ws_error
>> +};
>> +
>> struct apply_state {
>> const char *prefix;
>> int prefix_length;
>> @@ -80,6 +87,8 @@ struct apply_state {
>> int whitespace_error;
>> int squelch_whitespace_errors;
>> int applied_after_fixing_ws;
>> +
>> + enum ws_error_action ws_error_action;
>> };
>>
>> static int newfd = -1;
>> @@ -89,12 +98,6 @@ static const char * const apply_usage[] = {
>> NULL
>> };
>>
>> -static enum ws_error_action {
>> - nowarn_ws_error,
>> - warn_on_ws_error,
>> - die_on_ws_error,
>> - correct_ws_error
>> -} ws_error_action = warn_on_ws_error;
>
> This is a good example of a variable that needs initialization,
> which is turned into a field in apply_state that needs
> initialization. It is done here:
>
>> @@ -4738,6 +4743,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
>> state.p_value = 1;
>> state.p_context = UINT_MAX;
>> state.squelch_whitespace_errors = 5;
>> + state.ws_error_action = warn_on_ws_error;
>> strbuf_init(&state.root, 0);
>
> and we already have these random initial values described here.
>
> As I do not expect that cmd_apply() which is a moral equivalent of
> main() will stay to be the only one who wants to see a reasonably
> initialized apply_state(), I think the patch that introduced the
> very first version of "struct apply_state" should also introduce a
> helper function to initialize it, i.e.
>
> static void init_apply_state(struct apply_state *s,
> const char *prefix)
> {
> memset(s, '\0', sizeof(*s));
> s->prefix = prefix;
> s->prefix_length = s->prefix ? strlen(s->prefix) : 0;
> }
>
> in [PATCH 7/xx].
Yeah, Eric also made nearly the same comment, so in the next reroll
the init_apply_state() is introduced in the patch immediately after
the patch that creates 'struct apply_state' (rather than much later in
the series). I prefer to not introduce it in the same patch as the
patch that creates 'struct apply_state' is already quite big. (But if
you insist, yeah I will squash both patches together.)
--
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