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

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 26, 2019, 13:12 UTC
Message-ID: <5382f6cc-d699-0c99-6d61-60cae45f27c2@gmail.com>
In-Reply-To: <20190925020158.751492-3-alexhenrie24@gmail.com>

On 9/24/2019 10:01 PM, Alex Henrie wrote:

Show 34 quoted lines

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  diffcore-break.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/diffcore-break.c b/diffcore-break.c
> index 875aefd3fe..f6ab74141b 100644
> --- a/diffcore-break.c
> +++ b/diffcore-break.c
> @@ -286,18 +286,17 @@ void diffcore_merge_broken(void)
>  					/* Peer survived.  Merge them */
>  					merge_broken(p, pp, &outq);
>  					q->queue[j] = NULL;
> -					break;
> +					goto done;
>  				}
>  			}
> -			if (q->nr <= j)
> -				/* The peer did not survive, so we keep
> -				 * it in the output.
> -				 */
> -				diff_q(&outq, p);
> +			/* The peer did not survive, so we keep
> +			 * it in the output.
> +			 */
>  		}
> -		else
> -			diff_q(&outq, p);
> +		diff_q(&outq, p);
>  	}
> +
> +done:
>  	free(q->queue);
>  	*q = outq;

This took a little more context to check.

The "if (q->nr <= j)" condition essentially means "did the for loop above end via the sentinel, not a break?" This means that the diff_q() call is run if the for loop finishes normally OR if we hit the visible "else" call.

There is some subtlety with the if / else if / else conditions. The 'if' condition has a 'continue' which avoids the diff_q in its new position.

Something like the two paragraphs above would be good to add to the commit message so we can more easily read this patch in the future.

Here is the full method context (in master) to help others:

void diffcore_merge_broken(void)
{
        struct diff_queue_struct *q = &diff_queued_diff;
        struct diff_queue_struct outq;
        int i, j;

        DIFF_QUEUE_CLEAR(&outq);

        for (i = 0; i < q->nr; i++) {
                struct diff_filepair *p = q->queue[i];
                if (!p)
                        /* we already merged this with its peer */
                        continue;
                else if (p->broken_pair &&
                         !strcmp(p->one->path, p->two->path)) {
                        /* If the peer also survived rename/copy, then
                         * we merge them back together.
                         */
                        for (j = i + 1; j < q->nr; j++) {
                                struct diff_filepair *pp = q->queue[j];
                                if (pp->broken_pair &&
                                    !strcmp(pp->one->path, pp->two->path) &&
                                    !strcmp(p->one->path, pp->two->path)) {
                                        /* Peer survived.  Merge them */
                                        merge_broken(p, pp, &outq);
                                        q->queue[j] = NULL;
                                        break;
                                }
                        }
                        if (q->nr <= j)
                                /* The peer did not survive, so we keep
                                 * it in the output.
                                 */
                                diff_q(&outq, p);
                }
                else
                        diff_q(&outq, p);
        }
        free(q->queue);
        *q = outq;

        return;
}