{"thread":{"id":"24353","subject":"[BUG] 'diff A...B' fails with multiple merge bases","startedAt":"2010-07-10T01:15:45Z","lastAt":"2010-07-13T01:16:55Z","messageCount":10,"participants":["Pickens, James E","Junio C Hamano","Sverre Rabbelier"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"145256","messageId":"3BA20DF9B35F384F8B7395B001EC3FB36CF674AE@azsmsx507.amr.corp.intel.com","threadId":"24353","inReplyTo":null,"subject":"[BUG] 'diff A...B' fails with multiple merge bases","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2010-07-10T01:15:45Z","receivedAt":"2010-07-10T01:15:45Z","isPatch":false,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"Hi,\n\nThe command 'git diff A...B' is supposed to be equivalent to 'git diff $(git\nmerge-base A B) B'. But when there are multiple merge bases between A and B,\nthe former gives no output. Here's a recipe to reproduce the problem:\n\n git init\n git commit --allow-empty -m 1\n git checkout -b A\n touch file1\n git add file1\n git commit -m A\n git checkout master\n touch file2\n git add file2\n git commit -m B\n git checkout -b B\n git merge A\n git checkout A\n git merge master\n git diff A...B\n git diff $(git merge-base A B) B\n\nThe diff commands at the end will give different results. It bisects to:\n\ncommit b75271d93a9e4be960d53fc4f955802530e0e733\nAuthor: Matt McCutchen \nDate: Fri Oct 10 21:56:15 2008 -0400\n\n \"git diff {3,}\": do not reverse order of arguments\n\n According to the message of commit 0fe7c1de16f71312e6adac4b85bddf0d62a47168,\n \"git diff\" with three or more trees expects the merged tree first followed by\n the parents, in order. However, this command reversed the order of its\n arguments, resulting in confusing diffs. A comment /* Again, the revs are all\n reverse */ suggested there was a reason for this, but I can't figure out the\n reason, so I removed the reversal of the arguments. Test case included.\n\n\nI verified that if I revert that commit, 'diff A...B' works as expected, but\ntest t4013-diff-various.sh fails. The failing command is 'git diff master\nmaster^ side'. I don't understand what that command is supposed to do, so I\ndidn't go any further.\n\nAm I right that this is a bug, and if so can someone help to address it?\n\nJames\n"},{"id":"145401","messageId":"7vaapw8ehn.fsf@alter.siamese.dyndns.org","threadId":"24353","inReplyTo":"3BA20DF9B35F384F8B7395B001EC3FB36CF674AE@azsmsx507.amr.corp.intel.com","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-12T23:30:44Z","receivedAt":"2010-07-12T23:30:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Pickens, James E\" writes:\n\n> The command 'git diff A...B' is supposed to be equivalent to 'git diff $(git\n> merge-base A B) B'. But when there are multiple merge bases between A and B,\n> the former gives no output...\n> ...\n> The diff commands at the end will give different results. It bisects to:\n>\n> commit b75271d93a9e4be960d53fc4f955802530e0e733\n> Author: Matt McCutchen \n> Date: Fri Oct 10 21:56:15 2008 -0400\n\nThanks for a report, and for bisecting.\n\nThis unfortunately is somewhat an expected fallout from Matt's patch.\n\nThe low-level diff dispatcher in cmd_diff() function, where \"ents\" are\ntree-ish given from the command line (either using 'diff A', 'diff A B',\n'diff A..B', 'diff A...B' or 'diff A B C D E ...', syntaxes), says this:\n\n\t...\n\telse if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {\n\t\t/* diff A...B where there is one sane merge base between\n\t\t * A and B. We have ent[0] == merge-base, ent[1] == A,\n\t\t * and ent[2] == B. Show diff between the base and B.\n\t\t */\n\t\tent[1] = ent[2];\n\t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n\t}\n\telse\n\t\tresult = builtin_diff_combined(&rev, argc, argv,\n\t\t\t\t\t ent, ents);\n\nI omitted the 1 or 2 trees case where we do naturally diff-index or\ndiff-tree from the above.\n\nWhen the user gives more than two trees (e.g. \"diff A B C\"), we show a\ncombined diff that explains a merge of B and C that produces A, which was\nintroduced by 0fe7c1d (built-in diff: assorted updates., 2006-04-29).\nRemember that the first tree is the merge result, and the user is asking\nus to explain that result relative to its parents.\n\nThe special case with three trees, among which the first one being\nuninteresting, came much later. The revision parser parses A...B into a\nlist of \"--not $(merge-bases A B)\", A (SYMMETRIC_LEFT), and then B. As\nthe purpose of \"diff A...B\" is to show what you did up to B since you\nforked from A, showing the tree diff between the ent[0] (merge base) and\nent[2] (B) is the right thing to do. But the codepath is of course\nprepared to about dealing with a single-base merges, so your criss-cross\nmerge case does not trigger this special case.\n\nSo we fall into the \"combined diff\" case, which does this:\n\n git diff $(git merge-base --all A B) A B\n\nAs defined by 0fe7c1de, this should output the combined diff to explain as\nif one of the merge bases (that happens to be the first one in merge-base\noutput) were the merge result of all the other merge bases, the refs A and\nB you gave from the command line. Which does not make _any_ sense, as it\nis picking one of the criss-cross merge bases at random and forcing the\nhistory to flow backwards.\n\nBefore Matt's patch, I think diff_combined() was giving a _slightly_ more\nreasonable result because it (incorrectly) reversed the arguments to\nexplain as if B (typically yours) is the merge across all the merge bases\nand the other tip A. I say that is a \"slightly\" more reasonable, only\nbecause what is explained is what you are familiar with, i.e. B. I don't\nthink the way it explains it as a pseudo merge across all the merge bases\nand the other tip makes any sense.\n\nIt should not be too hard to add logic to reverse the list of revisions as\nanother special case in the above else-if chain to support the old output\nyou saw before Matt's fix if such an output were useful. You would detect\nif the list begins with a run of UNINTERESTING ones followed by two\ninteresting ones (because that is how A...B parser gives its output to\nus), and in that case feed diff_combined with a reversed list.\n\nBut I do not see how such an pseudo-merge output is useful, so please\nenlighten me with an illustration. Your \"earlier it showed something, now\nit doesn't show anything\" is not good enough here, as I am doubting that\nsomething we used to show in a criss-cross merge case was a useful output.\n\nThanks.\n"},{"id":"145405","messageId":"7vocec6xlt.fsf@alter.siamese.dyndns.org","threadId":"24353","inReplyTo":"7vaapw8ehn.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-13T00:20:46Z","receivedAt":"2010-07-13T00:20:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Pickens, James E\" writes:\n>\n>> The command 'git diff A...B' is supposed to be equivalent to 'git diff $(git\n>> merge-base A B) B'. But when there are multiple merge bases between A and B,\n>> the former gives no output...\n>> ...\n> It should not be too hard to add logic to reverse the list of revisions as\n> another special case in the above else-if chain to support the old output\n> you saw before Matt's fix if such an output were useful. You would detect\n> if the list begins with a run of UNINTERESTING ones followed by two\n> interesting ones (because that is how A...B parser gives its output to\n> us), and in that case feed diff_combined with a reversed list.\n>\n> But I do not see how such an pseudo-merge output is useful, so please\n> enlighten me with an illustration. Your \"earlier it showed something, now\n> it doesn't show anything\" is not good enough here, as I am doubting that\n> something we used to show in a criss-cross merge case was a useful output.\n\nI think I half-misread what you wanted to do. \"git diff A...B\" is not\nequivalent to \"git diff $(git merge-base A B) B\" but is supposed to be\nequivalent to \"git diff $(git merge-base --all A B) B\".\n\nI prepared a patch to reject such a request when there are more than one\nmerge base (see below---it is against 1.6.4 maintenance track). While I\nthink giving _one_ possible explanation of what you did since you forked\nwould be better than rejecting, which I'll try in a separate message, but\nat the same time it may be misleading to give such an output without\ntelling the user that we chose one merge base at random to diff against\nit.\n\n builtin-diff.c | 26 ++++++++++++++++++++++----\n 1 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 2e51f40..f65bc99 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -405,10 +405,28 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n \telse if (ents == 2)\n \t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n-\telse if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {\n-\t\t/* diff A...B where there is one sane merge base between\n-\t\t * A and B. We have ent[0] == merge-base, ent[1] == A,\n-\t\t * and ent[2] == B. Show diff between the base and B.\n+\telse if (ent[0].item->flags & UNINTERESTING) {\n+\t\t/*\n+\t\t * Perhaps the user gave us A...B, which expands\n+\t\t * to a list of negative merge bases followed by\n+\t\t * A (symmetric-left) and B? Let's make sure...\n+\t\t */\n+\t\tfor (i = 1; i < ents; i++)\n+\t\t\tif (!(ent[i].item->flags & UNINTERESTING))\n+\t\t\t\tbreak;\n+\t\tif (ents != i + 2 ||\n+\t\t (ent[i+1].item->flags & UNINTERESTING) ||\n+\t\t (!ent[i].item->flags & SYMMETRIC_LEFT) ||\n+\t\t (ent[i+1].item->flags & SYMMETRIC_LEFT))\n+\t\t\tdie(\"what do you mean by that?\");\n+\t\tif (ents != 3)\n+\t\t\tdie(\"There are more than one merge bases in %s\",\n+\t\t\t ent[ents-2].name);\n+\t\t/*\n+\t\t * diff A...B where there is one sane merge base\n+\t\t * between A and B. We have ent[0] == merge-base,\n+\t\t * ent[1] == A, and ent[2] == B. Show diff between\n+\t\t * the base and B.\n \t\t */\n \t\tent[1] = ent[2];\n \t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n"},{"id":"145406","messageId":"7viq4k6xea.fsf@alter.siamese.dyndns.org","threadId":"24353","inReplyTo":"7vocec6xlt.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-13T00:25:17Z","receivedAt":"2010-07-13T00:25:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I prepared a patch to reject such a request when there are more than one\n> merge base (see below---it is against 1.6.4 maintenance track). While I\n> think giving _one_ possible explanation of what you did since you forked\n> would be better than rejecting, which I'll try in a separate message, but\n> at the same time it may be misleading to give such an output without\n> telling the user that we chose one merge base at random to diff against\n> it.\n\nAnd this is the other one (not relative to the previous patch) that shows\ndiff since one randomly chosen merge base.\n\n builtin-diff.c | 26 +++++++++++++++++++++-----\n 1 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 2e51f40..1f44f5b 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -405,12 +405,28 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n \telse if (ents == 2)\n \t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n-\telse if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {\n-\t\t/* diff A...B where there is one sane merge base between\n-\t\t * A and B. We have ent[0] == merge-base, ent[1] == A,\n-\t\t * and ent[2] == B. Show diff between the base and B.\n+\telse if (ent[0].item->flags & UNINTERESTING) {\n+\t\t/*\n+\t\t * Perhaps the user gave us A...B, which expands\n+\t\t * to a list of negative merge bases followed by\n+\t\t * A (symmetric-left) and B? Let's make sure...\n \t\t */\n-\t\tent[1] = ent[2];\n+\t\tfor (i = 1; i < ents; i++)\n+\t\t\tif (!(ent[i].item->flags & UNINTERESTING))\n+\t\t\t\tbreak;\n+\t\tif (ents != i + 2 ||\n+\t\t (ent[i+1].item->flags & UNINTERESTING) ||\n+\t\t (!ent[i].item->flags & SYMMETRIC_LEFT) ||\n+\t\t (ent[i+1].item->flags & SYMMETRIC_LEFT))\n+\t\t\tdie(\"what do you mean by that?\");\n+\t\t/*\n+\t\t * diff A...B where there is at least one merge base\n+\t\t * between A and B. We have ent[0] == merge-base,\n+\t\t * ent[ents-2] == A, and ent[ents-1] == B. Show diff\n+\t\t * between the base and B. Note that we pick one\n+\t\t * merge base at random if there are more than one.\n+\t\t */\n+\t\tent[1] = ent[ents-1];\n \t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n \t}\n \telse\n"},{"id":"145409","messageId":"AANLkTinfo1QDguIbNgOt9w9V9xuJbhrCmusfe9g8fosm@mail.gmail.com","threadId":"24353","inReplyTo":"7viq4k6xea.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-07-13T00:41:45Z","receivedAt":"2010-07-13T00:41:45Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Jul 12, 2010 at 19:25, Junio C Hamano wrote:\n> +               if (ents != i + 2 ||\n> +                   (ent[i+1].item->flags & UNINTERESTING) ||\n> +                   (!ent[i].item->flags & SYMMETRIC_LEFT) ||\n> +                   (ent[i+1].item->flags & SYMMETRIC_LEFT))\n> +                       die(\"what do you mean by that?\");\n\nThat's about as helpful an error message as \"Your parents must hate\nyou\" or whatever it is. Can we rephrase it to tell the user what they\ndid wrong, and how to fix it?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"145410","messageId":"7vy6dg5hwl.fsf@alter.siamese.dyndns.org","threadId":"24353","inReplyTo":"AANLkTinfo1QDguIbNgOt9w9V9xuJbhrCmusfe9g8fosm@mail.gmail.com","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-13T00:45:14Z","receivedAt":"2010-07-13T00:45:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Heya,\n>\n> On Mon, Jul 12, 2010 at 19:25, Junio C Hamano wrote:\n>> +               if (ents != i + 2 ||\n>> +                   (ent[i+1].item->flags & UNINTERESTING) ||\n>> +                   (!ent[i].item->flags & SYMMETRIC_LEFT) ||\n>> +                   (ent[i+1].item->flags & SYMMETRIC_LEFT))\n>> +                       die(\"what do you mean by that?\");\n>\n> That's about as helpful an error message as \"Your parents must hate\n> you\" or whatever it is. Can we rephrase it to tell the user what they\n> did wrong, and how to fix it?\n\nThis will trigger only when you did something random like this.\n\n $ git diff ^maint master ^next ^pu sr/frotz..\n\nWhat else would you say against such an input?\n"},{"id":"145411","messageId":"AANLkTilnl5sw4M5p8AdO_Z73zBWzlwcZ8hIcdGDXf6de@mail.gmail.com","threadId":"24353","inReplyTo":"7vy6dg5hwl.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-07-13T00:49:46Z","receivedAt":"2010-07-13T00:49:46Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Jul 12, 2010 at 19:45, Junio C Hamano wrote:\n> This will trigger only when you did something random like this.\n>\n>    $ git diff ^maint master ^next ^pu sr/frotz..\n>\n> What else would you say against such an input?\n\nSomething like \"too many arguments, please specify at most two commits\nto diff against\"?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"145412","messageId":"7vpqys5het.fsf@alter.siamese.dyndns.org","threadId":"24353","inReplyTo":"AANLkTilnl5sw4M5p8AdO_Z73zBWzlwcZ8hIcdGDXf6de@mail.gmail.com","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-13T00:55:54Z","receivedAt":"2010-07-13T00:55:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Mon, Jul 12, 2010 at 19:45, Junio C Hamano wrote:\n>> This will trigger only when you did something random like this.\n>>\n>>    $ git diff ^maint master ^next ^pu sr/frotz..\n>>\n>> What else would you say against such an input?\n>\n> Something like \"too many arguments, please specify at most two commits\n> to diff against\"?\n\nThe first part is correct, but the advice is not quite, as you are\nforbidding the combined diff to emulate \"git show --cc pu\" to view a\nmerge at the tip of \"pu\" with\n\n git diff --cc pu pu~1 pu~2\n\nwhich obviously has to allow three commits.\n"},{"id":"145413","messageId":"AANLkTikCiJtnRlKTbhiSRe48aDY27QPvjrEyjiOGGRsl@mail.gmail.com","threadId":"24353","inReplyTo":"7vpqys5het.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-07-13T01:12:54Z","receivedAt":"2010-07-13T01:12:54Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Jul 12, 2010 at 19:55, Junio C Hamano wrote:\n> which obviously has to allow three commits.\n\nWell, then say that. \"too many arguments for a regular diff, please\nspecify at most two commits to diff against\". Anyone trying to use\ndiff to show a merge diff likely knows to look for the --cc switch;\neither way, it'd be better than \"what do you mean?\".\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"145414","messageId":"3BA20DF9B35F384F8B7395B001EC3FB36D000E3D@azsmsx507.amr.corp.intel.com","threadId":"24353","inReplyTo":"7viq4k6xea.fsf@alter.siamese.dyndns.org","subject":"RE: [BUG] 'diff A...B' fails with multiple merge bases","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2010-07-13T01:16:55Z","receivedAt":"2010-07-13T01:16:55Z","isPatch":false,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"Junio C Hamano writes:\n>> I prepared a patch to reject such a request when there are more than one\n>> merge base (see below---it is against 1.6.4 maintenance track). While I\n>> think giving _one_ possible explanation of what you did since you forked\n>> would be better than rejecting, which I'll try in a separate message, but\n>> at the same time it may be misleading to give such an output without\n>> telling the user that we chose one merge base at random to diff against\n>> it.\n>\n>And this is the other one (not relative to the previous patch) that shows\n>diff since one randomly chosen merge base.\n\nThanks for the detailed explanation and patches!\n\nPersonally I like this behavior better than erroring out when there are multiple\nmerge bases, even though the result is unpredictable. We were using 'diff\nA...B' in a script that users run to submit their changes to a continuous\nintegration server, to check whether they had any actual changes to submit. In\nthat context, we don't care whether the output makes sense, we only care whether\nthere is any output.\n\nThe script has been changed to avoid using the A...B syntax now, so it's not a\nbig deal if you prefer to just error out in this situation. But FWIW, this\npatch also has the advantage that it makes the code match the existing\ndocumentation... assuming that the randomly chosen merge base is the same one\nthat 'git merge-base' would print.\n\nJames\n"}]}