Subject: Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement

From: Junio C Hamano <[email-masked]>
Date: Wed, 02 Oct 2019 14:55:43 +0900

Alex Henrie <[email-masked]> writes:

> On Mon, Sep 30, 2019 at 3:48 AM Junio C Hamano <[email-masked]> wrote:
>>
>> Alex Henrie <[email-masked]> writes:
>>
>> > Well, I admit that code clarity is somewhat subjective. To me it's not
>> > obvious that "if (q->nr <= j)" means "if the loop exited normally",

I agree that it is subjective, but "if counter hasn't run beyond the
limit" immediately after a loop is a pretty-well established idiom
to see if we broke out prematurely.

> To me at least it was not clear what code is executed if the peer
> survives. The fact that I put the goto label in the wrong place the
> first time only underscores the difficulty of understanding this
> function. But with a goto (and with its label in the correct place),
> the execution path is obvious.

Well, the fact that the version with goto jumping to a wrong place
was so easy to spot as buggy might be an indication that goto made
it obvious to CB, but clearly use of goto did not make logic obvious
at least to the older version of you who wrote the buggy code.

Well I am being a bit mean in the above ;-) A more fair statement
would be that your bug did not come from goto; I think what
contributed more to the logic flaw in your earlier round was the
merging of two diff_q() calls from unrelated codepath.  Without that
change, and with the knowledge of "did the loop terminate early"
idiom, the version without 'goto' is obvious, and with only 'goto'
change, that original obviousness would not diminish.