On Sat, Sep 28, 2019 at 06:56:46PM -0600, Alex Henrie wrote:
> The condition "if (q->nr <= j)" checks whether the loop exited normally
> or via a break statement. This check can be avoided by replacing the
> jump to the end of the loop with a jump to the end of the function.
>
> With the break replaced by a goto, the two diff_q calls then can be
> replaced with a single diff_q call outside of the outer if statement.
>
> Reviewed-by: Derrick Stolee <stolee@xxxxxxxxx>
> Signed-off-by: Alex Henrie <alexhenrie24@xxxxxxxxx>
> ---
> diffcore-break.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
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'.
> }
> }
> - 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)
