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

[Stefan Beller <sbeller@google.com>]

On Mon, Dec 26, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I took the check "len > 2" from the other occurrence in submodule.c.
When thinking about it (and inspecting the man page for
porcelain status), I think just checking != 0 is sufficient.

So in a reroll I'll change that to just

    if (len)
        ...

I'll look at the other occurrences and see if we can reuse code
as well.

Thanks,
Stefan