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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2015, 21:39 UTC
Message-ID: <xmqq611sng86.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20151027212645.GF7881@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 8 quoted lines

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

Show 27 quoted lines

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