Re: [PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule

> Stefan Beller <sbeller@google.com> writes:
>
>> As only 0 is understood as false, rename the function to invert the
>> meaning, i.e. the return code of 0 signals the removal of the submodule
>> is fine, and other values can be used to return a more precise answer
>> what went wrong.
>
> Makes sense to rename it as that will catch all the callers that
> depend on the old semantics and name.
>
>> -     if (start_command(&cp))
>> -             die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
>> +     if (start_command(&cp)) {
>> +             if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
>> +                     die(_("could not start 'git status in submodule '%s'"),
>> +                             path);
>> +             ret = -1;
>> +             goto out;
>> +     }
>
> This new codepath that does not die will not leak anything, as
> a failed start_command() should release its argv[] and env[].
>
>>       len = strbuf_read(&buf, cp.out, 1024);
>>       if (len > 2)
>> -             ok_to_remove = 0;
>> +             ret = 1;
>
> Not a new problem but is it obvious why the comparison of "len" is
> against "2"?  This may deserve a one-liner comment.
>
> Otherwise looks good to me.