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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:

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

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

>  	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