Re: [PATCH 09/16] update submodules: add scheduling to update submodules
On Tue, Nov 15, 2016 at 4:02 PM, Brandon Williams <bmwill@google.com> wrote:
Show 9 quoted lines
> On 11/15, Stefan Beller wrote:
>> +
>> + child_process_clear(&cp);
>> + return 0;
>> +}
>
> If run command is successful then it handles the clearing of the child
> process struct, correct? Is there a negative to having all the explicit
> clears when the child was successful?
void child_process_clear(struct child_process *child)
{
argv_array_clear(&child->args);
argv_array_clear(&child->env_array);
}I don't think so, as clearing empty arg arrays is a no op.
Show 5 quoted lines
>> +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}
>
> I may not know enough about these types of initializors but that Init
> macro only has 2 entries while there are three entries in the struct
> itself.Filled up to 3 to be explicit.
Show 6 quoted lines
>
>> +
>> +int scheduled_submodules_nr, scheduled_submodules_alloc;
>
> Should these globals be static since they should be scoped to only this
> file?
Show 5 quoted lines
>
> nit: organization wise it makes more sense to me to have the
> 'update_submodule' helper function be located more closely to the
> 'update_submodules' function.
>
> In fact, only the first NULL is necessary; unspecified initializer entries in C default to zero.
as said above, I explicitly init all of them now.
Stefan Beller <sbeller@google.com>