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

[Derrick Stolee <stolee@xxxxxxxxx>]

On 9/24/2019 10:01 PM, Alex Henrie wrote:
> Signed-off-by: Alex Henrie <alexhenrie24@xxxxxxxxx>
> ---
>  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;
}