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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2016, 00:53 UTC
Message-ID: <xmqqr34uchuq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161220232012.15997-4-sbeller@google.com>

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.

Show 9 quoted lines

> -	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.