{"thread":{"id":"38475","subject":"[Git BUG] Please do not use \"-B -M\" in \"diff\" family for now","startedAt":"2015-01-31T19:12:34Z","lastAt":"2015-02-02T18:25:39Z","messageCount":5,"participants":["Junio C Hamano","Stefan Beller","Yue Lin Ho"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255461","messageId":"xmqqegqaahnh.fsf@gitster.dls.corp.google.com","threadId":"38475","inReplyTo":null,"subject":"[Git BUG] Please do not use \"-B -M\" in \"diff\" family for now","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-31T19:12:34Z","receivedAt":"2015-01-31T19:12:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Please avoid the combination \"-B -M\" when running \"diff\" family of\ncommands, as it can produce incorrect results [*1*] in corner cases.\nUse of either \"-B\" or \"-M\" by itself is fine, but not both at the\nsame time.\n\nThis problem exists even in Git v1.7.12, and I have no reason to\nsuspect that it worked correctly in any earlier versions, so I do\nnot consider it an urgent issue to fix during the pre-release for\nupcoming Git v2.3 release.\n\nEnd of TL;DR.\n\nFor a simple reproduction, go to your copy of the kernel tree and do\nthis:\n\n    $ git diff -B -M v2.6.13 v2.6.12 -- \\\n        arch/ppc64/kernel/rtas_pci.c arch/ppc64/kernel/pSeries_pci.c >:patch\n\n    $ git reset --hard\n    $ git checkout v2.6.13\n\n    $ git apply --cached --whitespace=nowarn :patch\n    error: arch/ppc64/kernel/pSeries_pci.c: already exists in index\n\nThis is not a bug in \"apply\", but in \"diff\".  The resulting patch\nlooks like this:\n\n    $ git apply --whitespace=nowarn --numstat --summary :patch\n    112     5       arch/ppc64/kernel/pSeries_pci.c\n     rename arch/ppc64/kernel/{rtas_pci.c => pSeries_pci.c} (81%)\n\nThat is, it wants to rename rtas_pci.c to pSeries_pci.c with a bit\nof editing.\n\nHowever, what really happens when going from 2.6.13 to 2.6.12 is\nthis:\n\n    $ git diff v2.6.13 v2.6.12 -- \\\n        arch/ppc64/kernel/rtas_pci.c arch/ppc64/kernel/pSeries_pci.c |\n        git apply --whitespace=nowarn --numstat --summary\n    478     19      arch/ppc64/kernel/pSeries_pci.c\n    0       495     arch/ppc64/kernel/rtas_pci.c\n     delete mode 100644 arch/ppc64/kernel/rtas_pci.c\n\nThat is:\n\n    #1 rtas_pci.c exists in 2.6.13 but not 2.6.12.\n\n    #2 pSeries_pci.c exists in both 2.6.12 and 2.6.13 but is majorly\n       rewritten; in fact, the difference between rtas_pci.c in\n       2.6.13 and pSeries_pci.c in 2.6.12 is much smaller than the\n       difference between pSeries_pci.c from 2.6.13 and 2.6.12.\n\nWhat seems to happen is that \"diff -B\" splits the above #2 into\n\"removal of pSeries_pci.c with contents from 2.6.13\" and \"creation\nof pSeries_pci.c with contents from 2.6.12\" and these gets further\ncombined with the \"removal of rtas_pci.c\" [*2*].  In the end result,\nwe incorrectly get \"rename rtas_pci.c to create pSeries_pci.c with\nsome changes\".  We shouldn't do this because pSeries_pci.c is not\ncreated and is not a new file.\n\n[Footnotes]\n\n*1* \"git apply\" refuses to apply the output affected by the bug, so\n    at least this will not lead to silent corruption.\n\n*2* The \"-B\" option was introduced solely to find a possible\n    rename/copy source to express this sequence:\n\n    $ cp A B\n    $ edit B slightly ;# optional\n    $ edit A heavily\n\nas if it was done like this instead:\n\n    $ mv A B\n    $ edit B slightly ;# optional\n    $ create B from scratch\n\nWithout \"-B\", the change would be expressed as \"Heavily edit A,\ncreate B from scratch\", and with \"-B\", we would say \"Create B by\ncopying from A and then edit, and edit A heavily\", which would\nresult in more readable patch (in fact, we would see in the output\nof \"diff -B -M v2.6.12 v2.6.13\" exactly that).\n"},{"id":"255500","messageId":"xmqqfvapuhkk.fsf@gitster.dls.corp.google.com","threadId":"38475","inReplyTo":"xmqqegqaahnh.fsf@gitster.dls.corp.google.com","subject":"[RFC PATCH] diff: do not use creation-half of -B as a rename target candidate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T03:18:35Z","receivedAt":"2015-02-02T03:18:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When a commit creates new file B by copying the contents of an\nexisting file A and making a small edit and makes large edit to A,\n\"diff -M\" would not see any copying or renaming, because the file A\nappears in both preimage and postimage.  The output ends up showing\ntwo large patches, one that adds almost the entirety of original A\nto the newly created file B, and the other that removes almost the\nentirety of the original contents from A and adds new material to\nit.\n\n\"diff -B -M\" was invented to allow us notice this case and instead\nexpress the change as one patch that copies A to B with small edit,\nand rewrites A with contents unrelated to its original.  The patch\nexpressed this way becomes much easier to read, because the reader\ncan see that most of the contents in B after the change came from\nthe original A (the patch header shows \"copy from A\" for B), and A\nwas completely rewritten by the patch (the patch body shows\neverything removed first and then all new material added).\n\nHowever this logic has a bug to incorrectly produce an unapplicable\npatch in other cases.  Starting from existing files A and B, when a\ncommit removes A and makes the resulting B similar to the original\ncontents of A, it incorrectly expressed it as a change that renames\nA to B and then makes small edits.  Such a patch will not apply to\nthe original state the patch was taken from, as B exists there.\n\nThe root cause of the problem is that after the complete rewrite of\nB is detected and is internally split into deletion of old B and\ncreation of new B, the rename detection machinery matches the old\ncontents of A (which will go away) with the newly created B, because\nthey are similar.  Considering the deletion-half of the change to B\nas possible rename source (i.e. from which a new file is created) is\ngood, but considering the creation-half as possible rename\ndestination (i.e. the file is created by taking whole contents from\nelsewhere) is bad---because we know B being a broken filepair means\nit already existed, and cannot have been newly _created_.\n\nFor a simple reproduction, go to your copy of the kernel tree and do\nthis:\n\n    $ git diff -B -M v2.6.13 v2.6.12 -- \\\n        arch/ppc64/kernel/rtas_pci.c arch/ppc64/kernel/pSeries_pci.c >:patch\n\n    $ git reset --hard\n    $ git checkout v2.6.13\n\n    $ git apply --cached --whitespace=nowarn :patch\n    error: arch/ppc64/kernel/pSeries_pci.c: already exists in index\n\nIn this example, rtas_pci.c and pSeries_pci.c corresponds to files A\nand B, respectively, in the more general description of the problem\ngiven earlier.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Here is what I am at the moment; I cannot quite explain (hence I\n   cannot convince myself) why this is the right solution, but it\n   seems to make the above sample case work without breaking any\n   existing tests.  It is possible that the tests that would break\n   without the \"&& !p->score\" bit are expecting wrong results, but I\n   didn't look at them in detail.\n\n diffcore-rename.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 6c7a72f..f4e8e00 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -516,6 +516,8 @@ void diffcore_rename(struct diff_options *options)\n \t\t\telse if (!DIFF_OPT_TST(options, RENAME_EMPTY) &&\n \t\t\t\t is_empty_blob_sha1(p->two->sha1))\n \t\t\t\tcontinue;\n+\t\t\telse if (p->broken_pair && !p->score)\n+\t\t\t\tcontinue;\n \t\t\telse\n \t\t\t\tlocate_rename_dst(p->two, 1);\n \t\t}\n-- \n2.3.0-rc2-165-gbd2cd9b\n"},{"id":"255501","messageId":"54CF1089.6020804@gmail.com","threadId":"38475","inReplyTo":"xmqqfvapuhkk.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] diff: do not use creation-half of -B as a rename target candidate","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2015-02-02T05:52:09Z","receivedAt":"2015-02-02T05:52:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 01.02.2015 19:18, Junio C Hamano wrote:\n> When a commit creates new file B by copying the contents of an\n> existing file A and making a small edit and makes large edit to A,\n\nThis part is hard to parse\n\"When ... and making a small edit and makes a large edit\"\nSo large or small? It's a bit hard to parse and understand when just\ntrying to read the first sentence. IT becomes clear (somewhat) later.\n"},{"id":"255504","messageId":"1422859667830-7624836.post@n2.nabble.com","threadId":"38475","inReplyTo":"54CF1089.6020804@gmail.com","subject":"Re: [RFC PATCH] diff: do not use creation-half of -B as a rename target candidate","fromName":"Yue Lin Ho","fromEmail":"yuelinho777@gmail.com","sentAt":"2015-02-02T06:47:47Z","receivedAt":"2015-02-02T06:47:47Z","isPatch":true,"sender":{"key":"yuelinho777@gmail.com","avatar":"https://gravatar.com/avatar/dbf9652003664c7518c86149f9e24df4d178c52526eaffab6f3c3d15b61671e2?d=mp&s=160"},"body":"A1 = \"I am file A.\"\nB1 is copied from A1, so B1 = \"I am file A.\"\nB1 changes to B2 = \"I am file B.\"\nA1 changes to A2 = \"file A is changed a lot, a lot, ..., a lot.\"\nAt this moment, commit A2 and B2 files.\n\n\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/Git-BUG-Please-do-not-use-B-M-in-diff-family-for-now-tp7624794p7624836.html\nSent from the git mailing list archive at Nabble.com.\n"},{"id":"255520","messageId":"xmqqbnlcuq58.fsf@gitster.dls.corp.google.com","threadId":"38475","inReplyTo":"xmqqfvapuhkk.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] diff: do not use creation-half of -B as a rename target candidate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T18:25:39Z","receivedAt":"2015-02-02T18:25:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>  * Here is what I am at the moment; I cannot quite explain (hence I\n>    cannot convince myself) why this is the right solution, but it\n>    seems to make the above sample case work without breaking any\n>    existing tests.  It is possible that the tests that would break\n>    without the \"&& !p->score\" bit are expecting wrong results, but I\n>    didn't look at them in detail.\n\nSadly, I think this is garbage.  \"Do not consider creation-half of a\nbroken pair, ever\" is too simple and cripples this case that starts\nwith two files A and B that are quite different:\n\n\t$ git add A B\n\t$ mv A B.new\n        $ mv B A\n        $ mv B.new B\n        $ git diff -B -M\n\nwhere the internal machinery breaks both A and B into these two file\npairs:\n\n\tdelete A(old)\n        create A(new)\n\n\tdelete B(old)\n        create B(new)\n\nand then match them up to produce\n\n\trename A to B\n        rename B to A\n\nThe rule need to be \"creation-half of a broken pair can be used as\nthe destination of a rename, if and only if its corresponding\ndeletion-half is used as the source of another rename elsewhere\".\nUnder that condition, a file A that is completely rewritten to\nbecome similar to another existing file B can be expressed as a\nrename of B, because A is renamed away to make room in the same\nchange.\n\nFixing this is turning out to be more complex than I originally\nhoped X-<.\n"}]}