Re: [PATCH 1/9] submodule-config: "goto" removal in parse_config()

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 27, 2015, 21:26 UTC
Message-ID: <20151027212645.GF7881@google.com>
In-Reply-To: <1445969753-418-2-git-send-email-sbeller@google.com>

Hi,

Stefan Beller wrote:

Show 5 quoted lines

> Subject: submodule-config: "goto" removal in parse_config()
>
> Many components in if/else if/... cascade jumped to a shared
> clean-up with "goto release_return", but we can restructure the
> function a bit and make them disappear,

Not having read the patch yet, the above makes me suspect this is going to make the code worse. A 'goto' for exception handling can be a clean way to ensure everything allocated gets released, and restructuring to avoid that can end up making the code more error prone and harder to read.

In other words, the "goto" removal should be a side effect and not the motivation.

>                                         which reduces the line count
> as well.  Also reformat overlong lines and poorly indented ones
> while at it.

These sound like good things. Hopefully this will make the code structure easier to understand, too.

> The order of rules to verify the value for "ignore" used to be to
> complain on multiple values first and then complain to boolean, but
> swap the order to match how the values for "path" and "url" are
> verified.

I don't understand this.  Hopefully the patch will make it clearer.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule-config.c | 74 +++++++++++++++++++++---------------------------------
>  1 file changed, 29 insertions(+), 45 deletions(-)

What patch does this apply against? A similar patch appears to already be part of "master".

[...]

Show 11 quoted lines

> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)
>  	if (!name_and_item_from_var(var, &name, &item))
>  		return 0;
>  
> -	submodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,
> -			name.buf);
> +	submodule = lookup_or_create_by_name(me->cache,
> +					     me->gitmodules_sha1,
> +					     name.buf);

Ok.

Show 7 quoted lines

>  	if (!strcmp(item.buf, "path")) {
> -		struct strbuf path = STRBUF_INIT;
> -		if (!value) {
> +		if (!value)
>  			ret = config_error_nonbool(var);
> -			goto release_return;
> -		}

In the preimage, I can see at this line already that nothing more is going to happen in this case. In the postimage, I need to scroll down to find that everything else is "else"s.

More generally, the patch seems to be about changing from a code structure of

	if (condition) {
		handle it;
		goto done;
	}
	if (other condition) {
		handle it;
		goto done;
	}
	handle misc;
	goto done;

to

	if (condition) {
		handle it;
	} else if (other condition) {
		handle it;
	} else {
		handle misc;
	}

In this example the postimage is concise and simple enough that it's probably worth it, but it is not obvious in the general case that this is always a good thing to do.

Now that I see the patch is already merged, I don't think it needs tweaks. Just a little concerned about the possibility of people judging from the commit message and emulating the pattern in the rest of git.

Thanks and hope that helps, Jonathan