{"thread":{"id":"51938","subject":"[PATCH v3] diffcore-break: use a goto instead of a redundant if statement","startedAt":"2019-09-29T00:56:54Z","lastAt":"2019-09-30T01:19:23Z","messageCount":4,"participants":["Alex Henrie","CB Bailey","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"383093","messageId":"20190929005646.734046-1-alexhenrie24@gmail.com","threadId":"51938","inReplyTo":null,"subject":"[PATCH v3] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-29T00:56:46Z","receivedAt":"2019-09-29T00:56:54Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The condition \"if (q->nr <= j)\" checks whether the loop exited normally\nor via a break statement. This check can be avoided by replacing the\njump to the end of the loop with a jump to the end of the function.\n\nWith the break replaced by a goto, the two diff_q calls then can be\nreplaced with a single diff_q call outside of the outer if statement.\n\nReviewed-by: Derrick Stolee \nSigned-off-by: Alex Henrie \n---\n diffcore-break.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 875aefd3fe..f6ab74141b 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -286,18 +286,17 @@ void diffcore_merge_broken(void)\n \t\t\t\t\t/* Peer survived. Merge them */\n \t\t\t\t\tmerge_broken(p, pp, &outq);\n \t\t\t\t\tq->queue[j] = NULL;\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tgoto done;\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (q->nr <= j)\n-\t\t\t\t/* The peer did not survive, so we keep\n-\t\t\t\t * it in the output.\n-\t\t\t\t */\n-\t\t\t\tdiff_q(&outq, p);\n+\t\t\t/* The peer did not survive, so we keep\n+\t\t\t * it in the output.\n+\t\t\t */\n \t\t}\n-\t\telse\n-\t\t\tdiff_q(&outq, p);\n+\t\tdiff_q(&outq, p);\n \t}\n+\n+done:\n \tfree(q->queue);\n \t*q = outq;\n \n-- \n2.23.0\n\n"},{"id":"383099","messageId":"20190929093706.ylm5dsftwl2y2nnz@hashpling.org","threadId":"51938","inReplyTo":"20190929005646.734046-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v3] diffcore-break: use a goto instead of a redundant if statement","fromName":"CB Bailey","fromEmail":"cb@hashpling.org","sentAt":"2019-09-29T09:37:06Z","receivedAt":"2019-09-29T09:44:52Z","isPatch":true,"sender":{"key":"cb@hashpling.org","avatar":null},"body":"On Sat, Sep 28, 2019 at 06:56:46PM -0600, Alex Henrie wrote:\n> The condition \"if (q->nr <= j)\" checks whether the loop exited normally\n> or via a break statement. This check can be avoided by replacing the\n> jump to the end of the loop with a jump to the end of the function.\n> \n> With the break replaced by a goto, the two diff_q calls then can be\n> replaced with a single diff_q call outside of the outer if statement.\n> \n> Reviewed-by: Derrick Stolee \n> Signed-off-by: Alex Henrie \n> ---\n> diffcore-break.c | 15 +++++++--------\n> 1 file changed, 7 insertions(+), 8 deletions(-)\n\nFor easier discussion, I've snipped the original patch and replaced with\none with enough context to show the entire function.\n\nI was reviewing this patch and it appeared to introduce a change in\nbehaviour.\n\n> diff --git a/diffcore-break.c b/diffcore-break.c\n> index 875aefd3fe..f6ab74141b 100644\n> --- a/diffcore-break.c\n> +++ b/diffcore-break.c\n> @@ -262,44 +262,43 @@ static void merge_broken(struct diff_filepair *p,\n> \n> void diffcore_merge_broken(void)\n> {\n> \tstruct diff_queue_struct *q = &diff_queued_diff;\n> \tstruct diff_queue_struct outq;\n> \tint i, j;\n> \n> \tDIFF_QUEUE_CLEAR(&outq);\n> \n> \tfor (i = 0; i < q->nr; i++) {\n> \t\tstruct diff_filepair *p = q->queue[i];\n> \t\tif (!p)\n> \t\t\t/* we already merged this with its peer */\n> \t\t\tcontinue;\n> \t\telse if (p->broken_pair &&\n> \t\t\t !strcmp(p->one->path, p->two->path)) {\n> \t\t\t/* If the peer also survived rename/copy, then\n> \t\t\t * we merge them back together.\n> \t\t\t */\n> \t\t\tfor (j = i + 1; j < q->nr; j++) {\n> \t\t\t\tstruct diff_filepair *pp = q->queue[j];\n> \t\t\t\tif (pp->broken_pair &&\n> \t\t\t\t !strcmp(pp->one->path, pp->two->path) &&\n> \t\t\t\t !strcmp(p->one->path, pp->two->path)) {\n> \t\t\t\t\t/* Peer survived. Merge them */\n> \t\t\t\t\tmerge_broken(p, pp, &outq);\n> \t\t\t\t\tq->queue[j] = NULL;\n> -\t\t\t\t\tbreak;\n> +\t\t\t\t\tgoto done;\n\nPreviously, if the condition matched in the inner loop, the function\nwould null out the entry in the queue that that inner loop had reached\n(q->queue[j] = NULL) and then break out of the inner loop. This meant\nthat the outer loop would skip over this entry (if (!p)).\n\nThe change introduced seems to break out of both loops as soon as we\nreach one match, whereas before other subsequent matches would be\nconsidered and merged. Not only this, but the outer 'else' case for all\nsubsequent entries is skipped so the rest of the entries the original\nqueue are missing from 'outq'.\n\n> \t\t\t\t}\n> \t\t\t}\n> -\t\t\tif (q->nr <= j)\n> -\t\t\t\t/* The peer did not survive, so we keep\n> -\t\t\t\t * it in the output.\n> -\t\t\t\t */\n> -\t\t\t\tdiff_q(&outq, p);\n> +\t\t\t/* The peer did not survive, so we keep\n> +\t\t\t * it in the output.\n> +\t\t\t */\n> \t\t}\n> -\t\telse\n> -\t\t\tdiff_q(&outq, p);\n> +\t\tdiff_q(&outq, p);\n> \t}\n> +\n> +done:\n> \tfree(q->queue);\n> \t*q = outq;\n> \n> \treturn;\n> }\n\nI spent a bit of time trying to see if this change was user visible\nwhich turned out to be unneeded as t4008-diff-break-rewrite.sh already\nfails with this change for me in my environment, initially with this\ntest but also 3 other tests in this file.\n\n> expecting success of 4008.6 'run diff with -B (#3)':\n> \tgit diff-index -B reference >current &&\n> \tcat >expect <<-EOF &&\n> \t:100644 100644 $blob0_id $blob1_id M100\tfile0\n> \t:100644 100644 $blob1_id $blob0_id M100\tfile1\n> \tEOF\n> \tcompare_diff_raw expect current\n> \n> --- .tmp-1\t2019-09-29 09:21:07.089070076 +0000\n> +++ .tmp-2\t2019-09-29 09:21:07.093070086 +0000\n> @@ -1,2 +1 @@\n> :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 6ff87c4664981e4397625791c8ea3bbb5f2279a3 M#\tfile0\n> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 548142c327a6790ff8821d67c2ee1eff7a656b52 M#\tfile1\n> not ok 6 - run diff with -B (#3)\n"},{"id":"383106","messageId":"CAMMLpeQnVb1M_Wy_GDeH44YVweXvNQ2jpyJqtvJ4zS_fGfxKuw@mail.gmail.com","threadId":"51938","inReplyTo":"20190929093706.ylm5dsftwl2y2nnz@hashpling.org","subject":"Re: [PATCH v3] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-29T20:10:33Z","receivedAt":"2019-09-29T20:10:48Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Sun, Sep 29, 2019 at 3:37 AM CB Bailey wrote:\n>\n> Previously, if the condition matched in the inner loop, the function\n> would null out the entry in the queue that that inner loop had reached\n> (q->queue[j] = NULL) and then break out of the inner loop. This meant\n> that the outer loop would skip over this entry (if (!p)).\n>\n> The change introduced seems to break out of both loops as soon as we\n> reach one match, whereas before other subsequent matches would be\n> considered and merged. Not only this, but the outer 'else' case for all\n> subsequent entries is skipped so the rest of the entries the original\n> queue are missing from 'outq'.\n\n> I spent a bit of time trying to see if this change was user visible\n> which turned out to be unneeded as t4008-diff-break-rewrite.sh already\n> fails with this change for me in my environment, initially with this\n> test but also 3 other tests in this file.\n\nThank you for reviewing this. I should have run `make test` myself\nbefore sending the patch; I do indeed see the same test failure that\nyou saw. I will send a v4 of this patch with the label in the right\nplace.\n\n-Alex\n"},{"id":"383108","messageId":"xmqqpnjiio08.fsf@gitster-ct.c.googlers.com","threadId":"51938","inReplyTo":"20190929093706.ylm5dsftwl2y2nnz@hashpling.org","subject":"Re: [PATCH v3] diffcore-break: use a goto instead of a redundant if statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-30T01:14:31Z","receivedAt":"2019-09-30T01:19:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"CB Bailey writes:\n\n> For easier discussion, I've snipped the original patch and replaced with\n> one with enough context to show the entire function.\n>\n> I was reviewing this patch and it appeared to introduce a change in\n> behaviour.\n>\n>> diff --git a/diffcore-break.c b/diffcore-break.c\n>> index 875aefd3fe..f6ab74141b 100644\n>> --- a/diffcore-break.c\n>> +++ b/diffcore-break.c\n>> @@ -262,44 +262,43 @@ static void merge_broken(struct diff_filepair *p,\n>> \n>> void diffcore_merge_broken(void)\n>> {\n>> \tstruct diff_queue_struct *q = &diff_queued_diff;\n>> \tstruct diff_queue_struct outq;\n>> \tint i, j;\n>> \n>> \tDIFF_QUEUE_CLEAR(&outq);\n>> \n>> \tfor (i = 0; i < q->nr; i++) {\n>> \t\tstruct diff_filepair *p = q->queue[i];\n>> \t\tif (!p)\n>> \t\t\t/* we already merged this with its peer */\n>> \t\t\tcontinue;\n>> \t\telse if (p->broken_pair &&\n>> \t\t\t !strcmp(p->one->path, p->two->path)) {\n>> \t\t\t/* If the peer also survived rename/copy, then\n>> \t\t\t * we merge them back together.\n>> \t\t\t */\n>> \t\t\tfor (j = i + 1; j < q->nr; j++) {\n>> \t\t\t\tstruct diff_filepair *pp = q->queue[j];\n>> \t\t\t\tif (pp->broken_pair &&\n>> \t\t\t\t !strcmp(pp->one->path, pp->two->path) &&\n>> \t\t\t\t !strcmp(p->one->path, pp->two->path)) {\n>> \t\t\t\t\t/* Peer survived. Merge them */\n>> \t\t\t\t\tmerge_broken(p, pp, &outq);\n>> \t\t\t\t\tq->queue[j] = NULL;\n>> -\t\t\t\t\tbreak;\n>> +\t\t\t\t\tgoto done;\n>\n> Previously, if the condition matched in the inner loop, the function\n> would null out the entry in the queue that that inner loop had reached\n> (q->queue[j] = NULL) and then break out of the inner loop. This meant\n> that the outer loop would skip over this entry (if (!p)).\n>\n> The change introduced seems to break out of both loops as soon as we\n> reach one match, whereas before other subsequent matches would be\n> considered and merged. Not only this, but the outer 'else' case for all\n> subsequent entries is skipped so the rest of the entries the original\n> queue are missing from 'outq'.\n\nThanks.\n\nSometimes judicious use of 'goto' makes the resulting code easier to\nfollow, but quite honestly, I do not see it happening with this\nchange. The original makes it much more clear that there are three\ncases to worry about:\n\n A. an earlier round handled this one already;\n\n B. we have a broken pair and need to find the other one,\n B-1. if there is, we process it;\n B-2. otherwise we keep it in the outq.\n\n C. a normal one that does not need the complication of B is\n sent to the outq.\n\nand I find it much easier to follow without any goto.\n\n>\n>> \t\t\t\t}\n>> \t\t\t}\n>> -\t\t\tif (q->nr <= j)\n>> -\t\t\t\t/* The peer did not survive, so we keep\n>> -\t\t\t\t * it in the output.\n>> -\t\t\t\t */\n>> -\t\t\t\tdiff_q(&outq, p);\n>> +\t\t\t/* The peer did not survive, so we keep\n>> +\t\t\t * it in the output.\n>> +\t\t\t */\n>> \t\t}\n>> -\t\telse\n>> -\t\t\tdiff_q(&outq, p);\n>> +\t\tdiff_q(&outq, p);\n>> \t}\n>> +\n>> +done:\n>> \tfree(q->queue);\n>> \t*q = outq;\n>> \n>> \treturn;\n>> }\n>\n> I spent a bit of time trying to see if this change was user visible\n> which turned out to be unneeded as t4008-diff-break-rewrite.sh already\n> fails with this change for me in my environment, initially with this\n> test but also 3 other tests in this file.\n>\n>> expecting success of 4008.6 'run diff with -B (#3)':\n>> \tgit diff-index -B reference >current &&\n>> \tcat >expect <<-EOF &&\n>> \t:100644 100644 $blob0_id $blob1_id M100\tfile0\n>> \t:100644 100644 $blob1_id $blob0_id M100\tfile1\n>> \tEOF\n>> \tcompare_diff_raw expect current\n>> \n>> --- .tmp-1\t2019-09-29 09:21:07.089070076 +0000\n>> +++ .tmp-2\t2019-09-29 09:21:07.093070086 +0000\n>> @@ -1,2 +1 @@\n>> :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 6ff87c4664981e4397625791c8ea3bbb5f2279a3 M#\tfile0\n>> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 548142c327a6790ff8821d67c2ee1eff7a656b52 M#\tfile1\n>> not ok 6 - run diff with -B (#3)\n"}]}