Re: [PATCH v3] diffcore-break: use a goto instead of a redundant if statement
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2019, 01:14 UTC
- Message-ID
- <xmqqpnjiio08.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190929093706.ylm5dsftwl2y2nnz@hashpling.org>
CB Bailey <cb@hashpling.org> writes:
Show 51 quoted lines
> For easier discussion, I've snipped the original patch and replaced with
> one with enough context to show the entire function.
>
> I was reviewing this patch and it appeared to introduce a change in
> behaviour.
>
>> diff --git a/diffcore-break.c b/diffcore-break.c
>> index 875aefd3fe..f6ab74141b 100644
>> --- a/diffcore-break.c
>> +++ b/diffcore-break.c
>> @@ -262,44 +262,43 @@ static void merge_broken(struct diff_filepair *p,
>>
>> 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;
>> + goto done;
>
> Previously, if the condition matched in the inner loop, the function
> would null out the entry in the queue that that inner loop had reached
> (q->queue[j] = NULL) and then break out of the inner loop. This meant
> that the outer loop would skip over this entry (if (!p)).
>
> The change introduced seems to break out of both loops as soon as we
> reach one match, whereas before other subsequent matches would be
> considered and merged. Not only this, but the outer 'else' case for all
> subsequent entries is skipped so the rest of the entries the original
> queue are missing from 'outq'.Thanks.
Sometimes judicious use of 'goto' makes the resulting code easier to follow, but quite honestly, I do not see it happening with this change. The original makes it much more clear that there are three cases to worry about:
A. an earlier round handled this one already;
B. we have a broken pair and need to find the other one,
B-1. if there is, we process it;
B-2. otherwise we keep it in the outq. C. a normal one that does not need the complication of B is
sent to the outq.and I find it much easier to follow without any goto.
Show 43 quoted lines
> >> } >> } >> - 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; >> >> return; >> } > > I spent a bit of time trying to see if this change was user visible > which turned out to be unneeded as t4008-diff-break-rewrite.sh already > fails with this change for me in my environment, initially with this > test but also 3 other tests in this file. > >> expecting success of 4008.6 'run diff with -B (#3)': >> git diff-index -B reference >current && >> cat >expect <<-EOF && >> :100644 100644 $blob0_id $blob1_id M100 file0 >> :100644 100644 $blob1_id $blob0_id M100 file1 >> EOF >> compare_diff_raw expect current >> >> --- .tmp-1 2019-09-29 09:21:07.089070076 +0000 >> +++ .tmp-2 2019-09-29 09:21:07.093070086 +0000 >> @@ -1,2 +1 @@ >> :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 6ff87c4664981e4397625791c8ea3bbb5f2279a3 M# file0 >> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 548142c327a6790ff8821d67c2ee1eff7a656b52 M# file1 >> not ok 6 - run diff with -B (#3)