On Mon, Dec 26, 2016 at 4:53 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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