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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Yes, I shared the same general feeling (cf. $gmane/279405).

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

Generally, a large piece of code is _easier_ to read with forward
"goto"s that jump to the shared clean-up code, as they serve as
visual cues that tell the reader "you can stop reading here and
ignore the remainder of this if/else if/... cascade".

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

Yes, we shouldn't let people blindly imitate this change.  I merged
it primarily because I wanted the change get out of my hair, as
other changes in flight started conflicting with it.

This kind of change can be good one only in a narrowly defined case
(like this one) but I agree that in general, as you said at the
beginning, it is an easy way to make the resulting code less
maintainable and harder to read.

Thanks.