Re: [PATCH 45/83] builtin/apply: move 'state' init into init_apply_state()

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 27, 2016, 12:00 UTC
Message-ID: <CAP8UFD03d7DVnKDmzv0JbAv1j7=nPv=qCJgkKqNOt5JPL5+hMg@mail.gmail.com>
In-Reply-To: <CAPig+cTx6jGwS12c8j_O+kT-f06o3E_yxrWZXmeU-XQ7fUY_9w@mail.gmail.com>

On Mon, Apr 25, 2016 at 9:32 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 34 quoted lines

> On Sun, Apr 24, 2016 at 9:33 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> @@ -4670,6 +4670,28 @@ static int option_parse_directory(const struct option *opt,
>> +static void init_apply_state(struct apply_state *state, const char *prefix_)
>> +{
>> +       memset(state, 0, sizeof(*state));
>> +       state->prefix = prefix_;
>> +       state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
>> +       state->apply = 1;
>> +       state->line_termination = '\n';
>> +       state->p_value = 1;
>> +       state->p_context = UINT_MAX;
>> +       state->squelch_whitespace_errors = 5;
>> +       state->ws_error_action = warn_on_ws_error;
>> +       state->ws_ignore_action = ignore_ws_none;
>> +       state->linenr = 1;
>> +       strbuf_init(&state->root, 0);
>> +
>> +       git_apply_config();
>> +       if (apply_default_whitespace)
>> +               parse_whitespace_option(state, apply_default_whitespace);
>> +       if (apply_default_ignorewhitespace)
>> +               parse_ignorewhitespace_option(state, apply_default_ignorewhitespace);
>> +}
>
> Minor:
>
> If factoring out this code from cmd_apply() into init_apply_state()
> was done as a preparatory patch before introduction of 'apply_state',
> then each new 'state->foo=...' line would already be at its final
> location when added by its respective patch.

Yeah I agree. So I did something like that. And by the way while doing it I saw that Junio also suggested a similar change.

The little difference with what you and Junio suggest is that the patch that creates init_apply_state() is just after the patch that introduces 'struct apply_state'. I think it is a bit cleaner this way, as if I create init_apply_state() first, then I would have to change its arguments in the next patch that introduces 'struct apply_state'.

> Doing so would also provide an obvious opportunity to name the
> 'prefix' argument to init_apply_state() "prefix" rather than the odd
> "prefix_".

Yeah, I did that too.

Thanks, Christian.