{"thread":{"id":"38518","subject":"[RFH] \"diff -B -M\"","startedAt":"2015-02-06T22:08:27Z","lastAt":"2015-02-06T23:39:46Z","messageCount":4,"participants":["Junio C Hamano","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255683","messageId":"xmqqvbjed76s.fsf@gitster.dls.corp.google.com","threadId":"38518","inReplyTo":null,"subject":"[RFH] \"diff -B -M\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T22:08:27Z","receivedAt":"2015-02-06T22:08:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I've been thinking about \"diff -B -M\" for quite a while, and I am\nfinding a few interesting problems we have in our codebase.  Here is\na snapshot, with a few action items.\n\nIn this write-up of my current thinking, I'll use these terms:\n\npatchset::\n    Output from a single \"git diff\" invocation, which may contain\n    one or more \"patch\".\n\npatch::\n    Part of a \"patchset\" from a line that begins with \"diff --git\"\n    up to (but excluding) the next line that begins with \"diff --git\".\n\ntree::\n    \"diff\" compares two collections of files; each collection is a\n    \"tree\".  Left and right sides of the comparison are called \"old\n    tree\" and \"new tree\", respectively.  The \"tree\" we attempt to\n    apply a patchset is the \"target tree\".  The \"tree\" we would get\n    after successful application of a patchset is the \"resulting\n    tree\".  A \"tree\" does not have to be a tree object--we may be\n    comparing the index and the files in the working tree, for\n    example.\n\npreimage::\npostimage::\n    Lines in a \"patch\" that are prefixed by \"-\" or \" \" but not \"+\"\n    that denote what the patched file ought to have for the patchset\n    to apply (\"preimage\").  Lines with \"+\" or \" \" denote what the\n    patch result ought to look like (\"postimage\").\n\n\n1. Basics\n\nFirst the basics.  Let's think about a patchset with a single patch.\nWhat does this patchset tell us?\n\n    diff --git a/major-08.txt b/major-08.txt\n    index 680c5f6..5de90cb 100644\n    --- a/major-08.txt\n    +++ b/major-08.txt\n    @@ -1,3 +1,3 @@\n    -8. Fortitude.\n    +8. Strength.\n\n     This is one of the cardinal virtues, of which I shall speak later.\n\nIt obviously tells us that the new tree changed \"Fortitude\", that\nused to be in the old tree, to \"Strength\", but it actually tells us\na bit more about the old tree.  For this patch to apply, the target\ntree must have a file named \"major-08.txt\" that begins with lines we\nsee as the preimage in the patch.\n\n\n2. Renaming a file\n\nNow let's get a bit fancier and study a patchset with a rename\npatch, which I invented very early in the Git's life (it was done\nwhile Linus was still running the project).\n\nWhat does this patchset tell us?\n\n    diff --git rws/major-08.txt marseille/major-11.txt\n    similarity index 97%\n    rename from major-08.txt\n    rename to major-11.txt\n    index 680c5f6..2ab22a0 100644\n    --- rws/major-08.txt\n    +++ marseille/major-11.txt\n    @@ -1,3 +1,3 @@\n    -8. Fortitude.\n    +11. Fortitude.\n\n     This is one of the cardinal virtues, of which I shall speak later.\n\nWe can see that this is going from the same old tree as the previous\none's old tree, renames major-08.txt to major-11.txt with slight\nmodification.  It tells us a lot more about the trees, compared to\nthe previous example.  For this patchset to apply, the target tree\nmust satisfy the same preconditon as the previous one about\n\"major-08.txt\", and in addition it must lack \"major-11.txt\";\notherwise we wouldn't be renaming a new file to it.  Also, it tells\nus that the resulting tree must lack \"major-08.txt\".\n\nSo far, these are straight-forward.\n\nIn summary:\n\n rule 1. a patch from file A to file A means file A exists in the\n         target tree with contents that match the preimage of the\n         patch.  file A will be in the resulting tree.\n\n rule 2. a patch renaming file A to file B requires that file A\n         exists the target tree with contents that match the\n         preimage of the patch, and file B does not exist in the\n         target tree. file A will not exist in the resulting tree.\n\n rule 3. a patch that creates file A requires that file A does not exist\n         in the target tree.\n\n rule 4. a patch that deletes file A requires that file A exists in\n         the target tree with contents that match the preimage of\n         the patch.\n\nThe latter two I didn't illustrate with examples, but they should be\nobvious.\n\n\n3. First twist: cross renaming\n\nNow, here is the first twist.  What does this patchset mean?\n\n    diff --git rws/major-11.txt marseille/major-08.txt\n    similarity index 99%\n    rename from major-11.txt\n    rename to major-08.txt\n    index 517d9f8..44e8d3a 100644\n    --- rws/major-11.txt\n    +++ marseille/major-08.txt\n    @@ -1,3 +1,3 @@\n    -11. Justice.\n    +8. La Justice\n\n     That the Tarot, though it is of all reasonable antiquity, is not of\n    diff --git rws/major-08.txt marseille/major-11.txt\n    similarity index 97%\n    rename from major-08.txt\n    rename to major-11.txt\n    index 5de90cb..a101d5f 100644\n    --- rws/major-08.txt\n    +++ marseille/major-11.txt\n    @@ -1,3 +1,3 @@\n    -8. Strength.\n    +11. La Force\n\n     This is one of the cardinal virtues, of which I shall speak later.\n\nThis is a \"swap\" patchset, that swaps major-08.txt and major-11.txt\nwith small edit.  You would have done something like this to prepare\nit:\n\n    $ mv major-11.txt tmp\n    $ mv major-08.txt major-11.txt\n    $ mv tmp major-11.txt\n    $ edit major-08.txt major-11.txt ;# just a little\n    $ git commit -m swap major-08.txt major-11.txt\n    $ git diff -B -M HEAD^\n\nA patch renaming major-11.txt to major-08.txt (i.e. the first one in\nthis two-patch patchset) still requires that the major-11.txt must\nexist in the target tree for the patchset to apply.  But the other\nhalf of the \"rename\" rule we saw in the summary no longer holds.\nThe target of the rename, major-08.txt, has to exist in the target\npatchset; otherwise we cannot rename it to major-11.txt in the\nsecond patch in the patchset.\n\nSo the \"renaming\" rule needs to be tweaked, like this:\n\n rule 2. a patch renaming file A to file B requires that file A\n         exists and file B does not exist in the target tree, unless\n         another patch in the patchset renames file B to some other\n         file (possibly but not necessarily file A), in which case\n         file B must appear in the target tree.\n\nIt is important to have that \"unless\" part.  We need to make sure\nthat we do not allow a patchset in \"2. Renaming a file\" to overwrite\nan existing file major-11.txt in the target tree blindly.\n\n\n4. Second twist: rewriting and copying\n\nThe previous one showed how \"diff -B -M\" can be used to detect cross\nrenaming files and apply the resulting patchset (you can circularly\nrename more than two, i.e. A -> tmp, B -> A, ..., Z -> Y, tmp -> Z).\nIt can also detect when you did:\n\n    $ cp major-08.txt major-11.txt\n    $ edit major-08.txt ;# extensively\n    $ git add major-08.txt major-11.txt\n    $ git commit -m 'create 11 out of 08, rewrite 08'\n    $ git diff -B -M HEAD^\n\nAnd you would see:\n\n    diff --git a/major-08.txt b/major-08.txt\n    dissimilarity index 99%\n    index 5de90cb..44e8d3a 100644\n    --- a/major-08.txt\n    +++ b/major-08.txt\n    @@ -1,10 +1,31 @@\n    -8. Strength.\n    -\n    -This is one of the cardinal virtues, of which I shall speak later.\n    -...\n    -the principle of all force.\n    +8. La Justice\n    +\n    +That the Tarot, though it is of all reasonable antiquity, is not of\n    +...\n    +via prudentiæ.\n    diff --git a/major-08.txt b/major-11.txt\n    similarity index 97%\n    copy from major-08.txt\n    copy to major-11.txt\n    index 5de90cb..a101d5f 100644\n    --- a/major-08.txt\n    +++ b/major-11.txt\n    @@ -1,3 +1,3 @@\n    -8. Strength.\n    +11. La Force\n\n     This is one of the cardinal virtues, of which I shall speak later.\n\nThe first patch in the patchset is \"a patch from file A to file A\",\neven though it is an extensive rewrite.  The target tree is required\nto have \"major-08.txt\" whose contents match the preimage of the\npatch (rule 1).  The second patch copies from major-08.txt to create\na new file major-11.txt.  The target tree is required to lack\n\"major-11.txt\" (rule 3; copying into A is creation of A).  It also\nmust have \"major-08.txt\" that begins with the preimage of the patch.\n\nSo far, all the examples shown above will work correctly with\ntoday's code, and when you use the old tree as the target tree,\n\"git apply\" will apply the patch and recreates \"new tree\" correctly.\n\nBut if you use the new tree of this example as the target tree and\ntry to \"git apply -R\" the patchset, it will _NOT_ work correctly.\n\nCurrently \"git apply -R\" does a nonsense.  It just swaps the\npreimage and the postimage, and then swaps the names of the files in\nthe old tree and in the new tree.\n\nBut the reverse of \"edit major-08 by changing Strength... to La\nJustice...; create major-11 by copying major-08 into it and then\nchange Strength to La Force\" is *NOT* \"edit major-08 by changing La\nJustice... to Strength...; create major-08 by copying major-11 into\nit and then change La Force to Strength\".  The first part of the\nreversion is correct, but the second part is not.\n\nWhat should we do to \"reverse\" a patchset that has copies?\n\nReverse of \"create major-11 by copying major-08\" should at least be\n\"remove major-11\", and preferrably accompanied by \"while making sure\nthat major-11 matches the postimage of the patch\".\n\nThe \"preferrably\" part is a modereatly strong preference.  When the\ncopying was done without any modification, we would not see any\npreimage or postimage to sanity check that the target tree of the\nreverse application is similar enough to the new tree the patchset\nwas taken from (we would end up just checking \"major-11 exists\" and\nthen removing it).  Admittedly, the same \"it feels unsafe\" factor\nexists when applying a bog-standard pure rename patch (imagine that\nthe example in \"2. Renaming a file\" was done without editing the\nfirst line and kept the original \"8. Fortutide.\" without renumbering\nit), but as long as we have patch text that we can use for sanity\nchecking, we should use it, I would think.\n\n\tAction item: somebody look into fixing \"apply -R\" on a patch\n\tthat creates a new file by copying.  The reverse of \"create\n\tB by copying A\" is not \"create A by copying B\".  It is\n\t\"remove B but make sure B matches the postimage\".\n\n        Action item: warn users against using \"apply -R\" on a\n        patchset with such a patch while this is fixed.\n\n\n5. Third twist: rewriting by copying\n\nIf you started from two vastly different files, and did this:\n\n    $ cp major-08.txt major-11.txt\n    $ edit major-11.txt\n    $ rm major-08.txt\n    $ git commit -m 'rewrite 11 by copying 08' major-08.txt major-11.txt\n    $ git diff -B -M HEAD^\n\nYou would see this patchset:\n\n    diff --git a/major-08.txt b/major-11.txt\n    similarity index 97%\n    rename from major-08.txt\n    rename to major-11.txt\n    index 5de90cb..a101d5f 100644\n    --- a/major-08.txt\n    +++ b/major-11.txt\n    @@ -1,3 +1,3 @@\n    -8. Strength.\n    +11. La Force\n\n     This is one of the cardinal virtues, of which I shall speak later.\n\nThis is the potentially buggy behaviour that led me to raise a\nwarning last week and to look into \"diff -B -M\".\n\nThe revised \"rule 2.\" from \"3. First twist\" tells us that\n\"major-08.txt\" must exist in the target tree, which is OK, but also\n\"major-11.txt\", the target of the rename, must not exist.  This\nmakes the resulting patchset unapplicable to the old tree the\npatchset was taken from, which is not, eh, ideal.  Also, the reverse\nof this patchset would apply to the new tree to produce the old tree\ncorrectly, which is a more serious bug (it can apply the patch in\nreverse and recreates the original major-08.txt just fine, but the\nother file major-11.txt is lost.  The patchset does not have any\ninformation for us to recreate its original contents of we had in\nthe old tree.\n\nOne possible way to fix this is to include another patch in the same\npatchset that shows the deletion of major-11.txt.  The rule 2. would\nbe further revised to something like:\n\n rule 2. a patch renaming file A to file B requires that file A\n         exists and file B does not exist in the target tree, unless\n         another patch in the patchset renames file B to some other\n         file (possibly but not necessarily file A) or removed file\n         B, in which case file B must appear in the target tree.\n\nSuch a patchset would look like this:\n\n    diff --git a/major-08.txt b/major-11.txt\n    similarity index 97%\n    rename from major-08.txt\n    rename to major-11.txt\n    index 5de90cb..a101d5f 100644\n    --- a/major-08.txt\n    +++ b/major-11.txt\n    @@ -1,3 +1,3 @@\n    -8. Strength.\n    +11. La Force\n\n     This is one of the cardinal virtues, of which I shall speak later.\n    diff --git a/major-11.txt b/major-11.txt\n    deleted file mode 100644\n    index 517d9f8..0000000\n    --- a/major-11.txt\n    +++ /dev/null\n    @@ -1,31 +0,0 @@\n    -11. Justice.\n    -\n    -That the Tarot, though it is of all reasonable antiquity, is not of\n    -...\n    -via prudentiæ.\n\nAnd these patches, under the re-revised rule 2. and rule 4., would\napply cleanly to the old tree.\n\nWhat about the reverse application?  It would be a patchset that\ncreates major-11.txt from nothingness, and creates major-08.txt by\nrenaming major-11.txt and editing.  Is the rule 2. re-revised above\nsufficient?\n\n    renaming major-11.txt to major-08.txt requires that major-08.txt\n    does not exist in the target tree, unless...\n\nand the new tree (which is the target of the reverse application)\nonly has major-11.txt and not major-08.txt, so this rename should go\nthrough.  The reverse of the deletion of major-11.txt is a creation\nof it with the contents fully given as the pre-image of the\n(original) patch before reversing it, so that should also be OK with\nrule 3.\n\nOne disturbing thing about the above plan is that we have this\ncomment at the end of diffcore-rename.c:\n\n\t * We would output this delete record if:\n\t *\n\t * (1) this is a broken delete and the counterpart\n\t *     broken create remains in the output; or\n\t * (2) this is not a broken delete, and rename_dst\n\t *     does not have a rename/copy to move p->one->path\n\t *     out of existence.\n\t *\n\t * Otherwise, the counterpart broken create\n\t * has been turned into a rename-edit; or\n\t * delete did not have a matching create to\n\t * begin with.\n\nThat is, we have an explicit logic to omit the missing \"delete\nmajor-11.txt\" patch from the patchset.  This comes from the very\nfirst commit that introduced \"diff -B\" support (f345b0a0 ([PATCH]\nAdd -B flag to diff-* brothers., 2005-05-30), iow, it is plausible\nthat it was the lack of thinking in the original code and not\nsomething we did to fix some bugs that would be introduced if we\n\"fix\" the current issue under discussion.\n\nSo tentatively, I would say...\n\n\tAction item: do not filter delete-half of the broken pair\n\tout, even when the other create-half of the pair no longer\n\tis in the output.\n\n\tAction item: update \"git apply\" to honor the rule 2.\n\tre-revised above.\n"},{"id":"255686","messageId":"xmqqegq2d5dv.fsf@gitster.dls.corp.google.com","threadId":"38518","inReplyTo":"xmqqvbjed76s.fsf@gitster.dls.corp.google.com","subject":"Re: [RFH] \"diff -B -M\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T22:47:24Z","receivedAt":"2015-02-06T22:47:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> 5. Third twist: rewriting by copying\n> ...\n> One possible way to fix this is to include another patch in the same\n> patchset that shows the deletion of major-11.txt.  The rule 2. would\n> be further revised to something like:\n>\n>  rule 2. a patch renaming file A to file B requires that file A\n>          exists and file B does not exist in the target tree, unless\n>          another patch in the patchset renames file B to some other\n>          file (possibly but not necessarily file A) or removed file\n\nTypofix: s/removed/removes/\n\n>          B, in which case file B must appear in the target tree.\n>\n> Such a patchset would look like this:\n>\n>     diff --git a/major-08.txt b/major-11.txt\n>     similarity index 97%\n>     rename from major-08.txt\n>     rename to major-11.txt\n> ...\n>     diff --git a/major-11.txt b/major-11.txt\n>     deleted file mode 100644\n> ...\n>\n> And these patches, under the re-revised rule 2. and rule 4., would\n> apply cleanly to the old tree.\n>\n> What about the reverse application?  It would be a patchset that\n> creates major-11.txt from nothingness, and creates major-08.txt by\n> renaming major-11.txt and editing.  Is the rule 2. re-revised above\n> sufficient?\n>\n>     renaming major-11.txt to major-08.txt requires that major-08.txt\n>     does not exist in the target tree, unless...\n>\n> and the new tree (which is the target of the reverse application)\n> only has major-11.txt and not major-08.txt, so this rename should go\n> through.  The reverse of the deletion of major-11.txt is a creation\n> of it with the contents fully given as the pre-image of the\n> (original) patch before reversing it, so that should also be OK with\n> rule 3.\n\n... But rule 3. needs tweaking.  The second patch would be creating\nmajor-11.txt which requires major-11.txt to be missing in the target\ntree.  But obviously, the new tree the patch we are applying in\nreverse was taken from has major-11.txt.  So we would need a\nrevision to it as well.\n\n rule 3. a patch that creates file A requires that file A does not exist\n         in the target tree, unless another patch in the same patchset\n         renames file A away or removes it.\n\n> So tentatively, I would say...\n>\n> \tAction item: do not filter delete-half of the broken pair\n> \tout, even when the other create-half of the pair no longer\n> \tis in the output.\n>\n> \tAction item: update \"git apply\" to honor the rule 2.\n> \tre-revised above.\n\nThe latter should read:\n\n\tupdate \"git apply\" to honor the revised rule 2. and 3.\n"},{"id":"255687","messageId":"xmqqa90qd4iu.fsf@gitster.dls.corp.google.com","threadId":"38518","inReplyTo":"xmqqvbjed76s.fsf@gitster.dls.corp.google.com","subject":"[PATCH] apply: do not allow reversing a 'copy' patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T23:06:01Z","receivedAt":"2015-02-06T23:06:01Z","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>         Action item: warn users against using \"apply -R\" on a\n>         patchset with such a patch while this is fixed.\n\nThis needs to be replaced with the logic to properly reverse a patch\nthat creates a new file by copying from somewhere else, but for now,\nthis would be a good idea to prevent lossages from happening.\n\n builtin/apply.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 0aad912..b33b403 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2069,6 +2069,9 @@ static void reverse_patches(struct patch *p)\n \tfor (; p; p = p->next) {\n \t\tstruct fragment *frag = p->fragments;\n \n+\t\tif (p->is_copy)\n+\t\t\tdie(_(\"sorry, cannot apply a 'copying' patch in reverse (yet)\"));\n+\n \t\tswap(p->new_name, p->old_name);\n \t\tswap(p->new_mode, p->old_mode);\n \t\tswap(p->is_new, p->is_delete);\n"},{"id":"255688","messageId":"CAGZ79kbLGEmVWvDki9UefzX9zj8m+O9_nWH07WLxqTym4mptGg@mail.gmail.com","threadId":"38518","inReplyTo":"xmqqa90qd4iu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: do not allow reversing a 'copy' patch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-06T23:39:46Z","receivedAt":"2015-02-06T23:39:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 6, 2015 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> +                       die(_(\"sorry, cannot apply a 'copying' patch in reverse (yet)\"));\n\nIs it wise to give the reader of the output hope that this is not\nimplemented (yet)\nbut may be in the future?\n\nI'd drop the \"sorry\" as well as the \"(yet)\" as the style of this error\nmessage seems\nto differ from other error messages in git.\n"}]}