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

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Sep 30, 2019, 17:36 UTC
Message-ID: <CAMMLpeQps8jvgpBXDnzqeeSsH7RA=__XjnDb4p_4SdGB-G2mSw@mail.gmail.com>
In-Reply-To: <xmqq36gei07z.fsf@gitster-ct.c.googlers.com>

On Mon, Sep 30, 2019 at 3:48 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

>
> Alex Henrie <alexhenrie24@gmail.com> 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 actually do not have too much problem with this side of the
> equation.  I however do see problem with squashing the two diff_q
> calls that _happens_ to be textually the same but is made from two
> logically separate codepaths (cases B.1 and C, in the message you
> are responding to but omitted from quote).  After all, you did not
> even realize you introduced a new bug by doing so before CB pointed
> it out, no?  A rewrite like that that easily allows a new bug to
> slip in hardly qualifies as making code "more clear and readable".

Sure, let's keep the two diff_q calls. I'll send a new patch.

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.

Thank you for being willing to look at this code with me, and thank you for your feedback!

-Alex