Stefan Beller <sbel...@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.
