CB Bailey <c...@hashpling.org> writes:
> 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.
>
>> }
>> }
>> - 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)
