{"thread":{"id":"18834","subject":"[PATCH] builtin-apply: keep information about files to be deleted","startedAt":"2009-04-11T19:31:00Z","lastAt":"2009-04-18T22:05:38Z","messageCount":12,"participants":["Michał Kiedrowicz","Junio C Hamano","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111083","messageId":"1239478260-7420-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"18834","inReplyTo":null,"subject":"[PATCH] builtin-apply: keep information about files to be deleted","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2009-04-11T19:31:00Z","receivedAt":"2009-04-11T19:31:00Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Example correct diff generated by `diff -M -B' might look like this:\n\n\tdiff --git a/file1 b/file2\n\tsimilarity index 100%\n\trename from file1\n\trename to file2\n\tdiff --git a/file2 b/file1\n\tsimilarity index 100%\n\trename from file2\n\trename to file1\n\nInformation about removing `file2' comes after information about creation\nof new `file2' (renamed from `file1'). Existing implementation isn't able to\napply such patch, because it has to know in advance which files will be\nremoved.\n\nThis patch populates fn_table with information about removal of files\nbefore calling check_patch() for each patch to be applied.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n builtin-apply.c |   44 +++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 1926cd8..6f6bf85 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2271,6 +2271,16 @@ static struct patch *in_fn_table(const char *name)\n \treturn NULL;\n }\n \n+static int to_be_deleted(struct patch *patch)\n+{\n+\treturn patch == (struct patch *) -2;\n+}\n+\n+static int was_deleted(struct patch *patch)\n+{\n+\treturn patch == (struct patch *) -1;\n+}\n+\n static void add_to_fn_table(struct patch *patch)\n {\n \tstruct string_list_item *item;\n@@ -2295,6 +2305,24 @@ static void add_to_fn_table(struct patch *patch)\n \t}\n }\n \n+static void prepare_fn_table(struct patch *patch)\n+{\n+\t/*\n+\t * store information about incoming file deletion\n+\t */\n+\n+\twhile (patch) {\n+\n+\t\tif ((patch->new_name == NULL) || (patch->is_rename)) {\n+\t\t\tstruct string_list_item *item =\n+\t\t\t\tstring_list_insert(patch->old_name, &fn_table);\n+\t\t\titem->util = (struct patch *) -2;\n+\t\t}\n+\n+\t\tpatch = patch->next;\n+\t}\n+}\n+\n static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -2304,8 +2332,8 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \tstruct patch *tpatch;\n \n \tif (!(patch->is_copy || patch->is_rename) &&\n-\t    ((tpatch = in_fn_table(patch->old_name)) != NULL)) {\n-\t\tif (tpatch == (struct patch *) -1) {\n+\t    (tpatch = in_fn_table(patch->old_name)) != NULL && !to_be_deleted(tpatch)) {\n+\t\tif (was_deleted(tpatch)) {\n \t\t\treturn error(\"patch %s has been renamed/deleted\",\n \t\t\t\tpatch->old_name);\n \t\t}\n@@ -2399,8 +2427,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \tassert(patch->is_new <= 0);\n \n \tif (!(patch->is_copy || patch->is_rename) &&\n-\t    (tpatch = in_fn_table(old_name)) != NULL) {\n-\t\tif (tpatch == (struct patch *) -1) {\n+\t    (tpatch = in_fn_table(old_name)) != NULL && !to_be_deleted(tpatch)) {\n+\t\tif (was_deleted(tpatch)) {\n \t\t\treturn error(\"%s: has been deleted/renamed\", old_name);\n \t\t}\n \t\tst_mode = tpatch->new_mode;\n@@ -2410,6 +2438,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n \t}\n \n+\tif(to_be_deleted(tpatch)) tpatch = NULL;\n+\n \tif (check_index && !tpatch) {\n \t\tint pos = cache_name_pos(old_name, strlen(old_name));\n \t\tif (pos < 0) {\n@@ -2471,6 +2501,7 @@ static int check_patch(struct patch *patch)\n \tconst char *new_name = patch->new_name;\n \tconst char *name = old_name ? old_name : new_name;\n \tstruct cache_entry *ce = NULL;\n+\tstruct patch *tpatch;\n \tint ok_if_exists;\n \tint status;\n \n@@ -2481,7 +2512,8 @@ static int check_patch(struct patch *patch)\n \t\treturn status;\n \told_name = patch->old_name;\n \n-\tif (in_fn_table(new_name) == (struct patch *) -1)\n+\tif ((tpatch = in_fn_table(new_name)) &&\n+\t\t\t(was_deleted(tpatch) || to_be_deleted(tpatch)))\n \t\t/*\n \t\t * A type-change diff is always split into a patch to\n \t\t * delete old, immediately followed by a patch to\n@@ -2533,6 +2565,8 @@ static int check_patch_list(struct patch *patch)\n {\n \tint err = 0;\n \n+\tprepare_fn_table(patch);\n+\n \twhile (patch) {\n \t\tif (apply_verbosely)\n \t\t\tsay_patch_name(stderr,\n-- \n1.6.0.6\n"},{"id":"111189","messageId":"20090413155152.726647d8@gmail.com","threadId":"18834","inReplyTo":"1239478260-7420-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2009-04-13T13:51:52Z","receivedAt":"2009-04-13T13:51:52Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Dnia 2009-04-11, o godz. 21:31:00\nMichał Kiedrowicz <michal.kiedrowicz@gmail.com> napisał(a):\n\n> Example correct diff generated by `diff -M -B' might look like this:\n> \n> \tdiff --git a/file1 b/file2\n> \tsimilarity index 100%\n> \trename from file1\n> \trename to file2\n> \tdiff --git a/file2 b/file1\n> \tsimilarity index 100%\n> \trename from file2\n> \trename to file1\n> \n> Information about removing `file2' comes after information about\n> creation of new `file2' (renamed from `file1'). Existing\n> implementation isn't able to apply such patch, because it has to know\n> in advance which files will be removed.\n> \n> This patch populates fn_table with information about removal of files\n> before calling check_patch() for each patch to be applied.\n> \n> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n\nCan anyone comment on this patch? It should fix bug mentioned at\n\nhttp://www.spinics.net/lists/git/msg100481.html\n\n-- \nMichał Kiedrowicz\n"},{"id":"111226","messageId":"7v4owsfktw.fsf@gitster.siamese.dyndns.org","threadId":"18834","inReplyTo":"1239478260-7420-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-13T18:51:55Z","receivedAt":"2009-04-13T18:51:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index 1926cd8..6f6bf85 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -2271,6 +2271,16 @@ static struct patch *in_fn_table(const char *name)\n>  \treturn NULL;\n>  }\n>  \n> +static int to_be_deleted(struct patch *patch)\n> +{\n> +\treturn patch == (struct patch *) -2;\n> +}\n> +\n> +static int was_deleted(struct patch *patch)\n> +{\n> +\treturn patch == (struct patch *) -1;\n> +}\n\nPlease use more descriptive symbolic constants, and add a comment.\nPerhaps:\n\n    /*\n     * item->util in the filename table records the status of the path.\n     * Usually it points at a patch (whose result records the contents\n     * of it after applying it), but it could be PATH_WAS_DELETED for a\n     * path that a previously applied patch has already removed.\n     */\n    #define PATH_TO_BE_DELETED ((struct patch *) -2)\n    #define PATH_WAS_DELETED ((struct patch *) -1)\n\n> @@ -2295,6 +2305,24 @@ static void add_to_fn_table(struct patch *patch)\n> ...\n> +static void prepare_fn_table(struct patch *patch)\n> +{\n> +\t/*\n> +\t * store information about incoming file deletion\n> +\t */\n> +\twhile (patch) {\n> +\t\tif ((patch->new_name == NULL) || (patch->is_rename)) {\n> +\t\t\tstruct string_list_item *item =\n> +\t\t\t\tstring_list_insert(patch->old_name, &fn_table);\n> +\t\t\titem->util = (struct patch *) -2;\n> +\t\t}\n> +\t\tpatch = patch->next;\n> +\t}\n> +}\n\nThis PATH_TO_BE_DELETED logic should be Ok for the normal case, but it\nseems a bit fragile.  In a sequence of patches, if you have even one patch\nthat makes the path disappear, you initialize it as PATH_TO_BE_DELETED,\nand special case the \"creation should not clobber existing path\" rule to\nallow it to be present in the tree.\n\nThat may make this sequence work, I presume, with your change:\n\n\tpatch #1\trenames frotz.c to hello.c\n        patch #2\trenames hello.c to frotz.c\n\nbecause of patch #2, hello.c is marked as PATH_TO_BE_DELETED initially and\nthen when patch #1 is handled, frotz.c is allowed to replace it.\n\nBut if you have further patches that do the following (the \"file table\"\nmechanism was added to handle concatenated patches that affect the same\npath more than once), I thing PATH_TO_BE_DELETED logic would break down:\n\n        patch #3\trenames alpha.c to hello.c\n\tpatch #4\trenames hello.c to alpha.c\n\nWhen patch #3 is handled, the PATH_TO_BE_DELETED mark is long gone from\nhello.c, and we will see the same failure you addressed in your patch,\nwon't we?\n\nThe prepare_fn_table() may be a good place to diagnose such a situation\nand warn or error out if the user feeds such an input we cannot handle\nsanely.\n\n> @@ -2410,6 +2438,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n>  \t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n>  \t}\n>  \n> +\tif(to_be_deleted(tpatch)) tpatch = NULL;\n> +\n\nStyle;\n\n\tif (to_be_deleted(tpatch))\n        \ttpatch = NULL;\n\nOther than that, I think it is a sensible approach.\n"},{"id":"111236","messageId":"20090413230351.7cbb01f5@gmail.com","threadId":"18834","inReplyTo":"7v4owsfktw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2009-04-13T21:03:51Z","receivedAt":"2009-04-13T21:03:51Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> This PATH_TO_BE_DELETED logic should be Ok for the normal case, but it\n> seems a bit fragile.  In a sequence of patches, if you have even one\n> patch that makes the path disappear, you initialize it as\n> PATH_TO_BE_DELETED, and special case the \"creation should not clobber\n> existing path\" rule to allow it to be present in the tree.\n> \n> That may make this sequence work, I presume, with your change:\n> \n> \tpatch #1\trenames frotz.c to hello.c\n>       patch #2\trenames hello.c to frotz.c\n> \n> because of patch #2, hello.c is marked as PATH_TO_BE_DELETED\n> initially and then when patch #1 is handled, frotz.c is allowed to\n> replace it.\n> \n> But if you have further patches that do the following (the \"file\n> table\" mechanism was added to handle concatenated patches that affect\n> the same path more than once), I thing PATH_TO_BE_DELETED logic would\n> break down:\n> \n>       patch #3\trenames alpha.c to hello.c\n> \tpatch #4\trenames hello.c to alpha.c\n> \n> When patch #3 is handled, the PATH_TO_BE_DELETED mark is long gone\n> from hello.c, and we will see the same failure you addressed in your\n> patch, won't we?\n\nAs far as I understand the code, diffs are applied independently\n(for every file apply_patch() is called) and for every apply_patch()\ncall fn_table is cleared. So situation you described in only possible\nin a *single* diff and I don't think it is possible to happen. \n\nPerforming two criss-cross renames results in following diff:\n\n\tmv file1 tmp\n\tmv file2 file1\n\tmv tmp file2\n\n\tmv file1 tmp\n\tmv file3 file1\n\tmv tmp file3\n\n\tgit diff -M -B\n\ndiff --git a/file3 b/file1\nsimilarity index 100%\nrename from file3\nrename to file1\ndiff --git a/file1 b/file2\nsimilarity index 100%\nrename from file1\nrename to file2\ndiff --git a/file2 b/file3\nsimilarity index 100%\nrename from file2\nrename to file3\n\nHowever, sanity checking still may be performed and error printed on\nsituations which cannot be resolved.\n\n> \n> The prepare_fn_table() may be a good place to diagnose such a\n> situation and warn or error out if the user feeds such an input we\n> cannot handle sanely.\n> \n\nI'll add some checks to this function as you suggest.\n\n> \n> Style;\n> \n> \tif (to_be_deleted(tpatch))\n>         \ttpatch = NULL;\n> \n> Other than that, I think it is a sensible approach.\n\nThanks for feedback.\n\n-- \nMichał Kiedrowicz\n"},{"id":"111238","messageId":"7v1vrwdyxx.fsf@gitster.siamese.dyndns.org","threadId":"18834","inReplyTo":"20090413230351.7cbb01f5@gmail.com","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-13T21:30:02Z","receivedAt":"2009-04-13T21:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> But if you have further patches that do the following (the \"file\n>> table\" mechanism was added to handle concatenated patches that affect\n>> the same path more than once), I thing PATH_TO_BE_DELETED logic would\n>> break down:\n>> \n>>      patch #3\trenames alpha.c to hello.c\n>> \tpatch #4\trenames hello.c to alpha.c\n>> \n>> When patch #3 is handled, the PATH_TO_BE_DELETED mark is long gone\n>> from hello.c, and we will see the same failure you addressed in your\n>> patch, won't we?\n>\n> As far as I understand the code, diffs are applied independently\n> (for every file apply_patch() is called) and for every apply_patch()\n> call fn_table is cleared. So situation you described in only possible\n> in a *single* diff and I don't think it is possible to happen. \n\nYes, one invocation of \"git format-patch -1\" will not produce such a\nsituation.\n\nA single diff file that is concatenation of two \"git format-patch -1\"\noutput (or just a plain-old \"diff -ru\" output from outside git, perhaps\nmanaged in quilt) was what introduced fn_table mechanism.  Apparently\npeople use \"git apply\" to apply such a patch.\n"},{"id":"111511","messageId":"20090417192324.3a888abf@gmail.com","threadId":"18834","inReplyTo":"7v1vrwdyxx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2009-04-17T17:23:24Z","receivedAt":"2009-04-17T17:23:24Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"\nW dniu 13 kwietnia 2009 23:30 użytkownik Junio C Hamano\n<gitster@pobox.com> napisał:\n>  \n> >\n> > As far as I understand the code, diffs are applied independently\n> > (for every file apply_patch() is called) and for every apply_patch()\n> > call fn_table is cleared. So situation you described in only\n> > possible in a *single* diff and I don't think it is possible to\n> > happen.  \n>\n> Yes, one invocation of \"git format-patch -1\" will not produce such a\n> situation.\n>\n> A single diff file that is concatenation of two \"git format-patch -1\"\n> output (or just a plain-old \"diff -ru\" output from outside git,\n> perhaps managed in quilt) was what introduced fn_table mechanism.\n>  Apparently people use \"git apply\" to apply such a patch.\n>  \n\nI have been thinking about that and IMO something is not right in\nhandling multiple patches. I'm still new to git, so I may be wrong.\nLook:\n\nSuppose I have 3 patches:\n\npatch #1: modify A\npatch #2: rename A to B\npatch #3: modify B\n\nThese patches will be applied correctly.\n\nBut, if I swap patches #1 and #3, none of them will be applied. This\nis because of 2 rules, implemented in add_to_fn_table():\n\n1. If a file was renamed/deleted, applying a patch is not possible.\n2. If a file is new/modified, applying a patch is possible.\n\nThey seem reasonable. In previous example, file A comes under rule #1\nand file B under rule #2. However, there are some cases when these two\nrules may cause problems:\n\npatch #1: rename A to B\npatch #2: rename C to A\npatch #3: modify A\n\nShould patch #3 modify B (which was A) or A (which was C)?\n\npatch #1: rename A to B\npatch #2: rename B to A\npatch #3: modify A\npatch #4: modify B\n\nWhich files should be patched by #3 and #4?\n\nIn my opinion both #3 and #4 should fail (or both should succeed) --\nwith my patch only #3 will work and #4 will be rejected, because in #2\nB was marked as deleted.\n"},{"id":"111559","messageId":"7vskk6y2tl.fsf@gitster.siamese.dyndns.org","threadId":"18834","inReplyTo":"20090417192324.3a888abf@gmail.com","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-18T04:59:34Z","receivedAt":"2009-04-18T04:59:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> ... However, there are some cases when these two\n> rules may cause problems:\n>\n> patch #1: rename A to B\n> patch #2: rename C to A\n> patch #3: modify A\n>\n> Should patch #3 modify B (which was A) or A (which was C)?\n>\n> patch #1: rename A to B\n> patch #2: rename B to A\n> patch #3: modify A\n> patch #4: modify B\n>\n> Which files should be patched by #3 and #4?\n>\n> In my opinion both #3 and #4 should fail (or both should succeed) --\n> with my patch only #3 will work and #4 will be rejected, because in #2\n> B was marked as deleted.\n\nBoth of the examples above cannot be emitted as a single commit by\nformat-patch; the user is feeding a combined patch.  Perhaps renames\nin each example sequence were came from one git commit but modifications\nare from separate commit or handcrafted \"follow-up\" patch.\n\nThere are two stances we can take:\n\n (1) The user knows what he is doing.\n\n     In the first example, if he wanted the change in #3 to end up in B,\n     he would have arranged the patches in a different order, namely, 3 1\n     2, but he didn't.  We should modify A (that came from C).\n\n (2) In situations like these when it is unusual and there is no clear and\n     unambiguous answer, the rule has always been \"fail and ask the user\n     to clean up\", because silently doing a wrong thing in an unusual\n     situation that happens only once in a while is far worse than\n     interrupting the user and forcing a manual intervention.\n\n     In the first example, there is no clear answer.  Perhaps all three\n     patches were independent patches (the first two obviously came from\n     git because only we can do renames, but they may have been separate\n     commits), and the user may have reordered them (or just picked a\n     random order because he was linearizing a history with a merge).\n\nThe second one is even iffier.  If we _know_ that originally patch #1 and\n#2 came from the same commit, then they represent swapping between A and\nB, but if they came from different git commits, and if the user picked\npatches in a random order, it may mean something completely different.\n\nI am somewhat tempted to say that we should fail all of them, including\nthe original \"single patch swapping files\" brought up by Linus.\n\nBUT\n\nCan we make use simple rule to detect problematic cases?\n\n - An input to git-apply can contain more than one patch that affects a\n   path; however\n\n   - you cannot create a path that still exists, except for a path that\n     _will_ be renamed away or removed (your patch fixes this by adding\n     this \"except for...\" part to loosen the existing rule);\n\n   - you cannot modify a path in a separate patch if it is involved in an\n     either side of a rename (this will catch the ambiguity of patch #3 in\n     your first example and #3 and #4 in your second example);\n\n - In addition:\n\n   - the same path cannot be renamed from more than once (this will catch\n     concatenation of two git generated patches);\n\nWith such a change, I think we can keep the safety of \"when there are more\nthan one plausible outcomes, the tool shouldn't silently decide, nor make\nprogress that the user later needs to undo and redo\", while allowing a\nsane use of rename patches generated out of a git commit.\n"},{"id":"111572","messageId":"49E9B90F.8070204@op5.com","threadId":"18834","inReplyTo":"7vskk6y2tl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Andreas Ericsson","fromEmail":"exon@op5.com","sentAt":"2009-04-18T11:27:11Z","receivedAt":"2009-04-18T11:27:11Z","isPatch":true,"sender":{"key":"exon@op5.com","avatar":null},"body":"Junio C Hamano wrote:\n> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n> \n>> ... However, there are some cases when these two\n>> rules may cause problems:\n>>\n>> patch #1: rename A to B\n>> patch #2: rename C to A\n>> patch #3: modify A\n>>\n>> Should patch #3 modify B (which was A) or A (which was C)?\n>>\n>> patch #1: rename A to B\n>> patch #2: rename B to A\n>> patch #3: modify A\n>> patch #4: modify B\n>>\n>> Which files should be patched by #3 and #4?\n>>\n>> In my opinion both #3 and #4 should fail (or both should succeed) --\n>> with my patch only #3 will work and #4 will be rejected, because in #2\n>> B was marked as deleted.\n> \n> Both of the examples above cannot be emitted as a single commit by\n> format-patch; the user is feeding a combined patch.  Perhaps renames\n> in each example sequence were came from one git commit but modifications\n> are from separate commit or handcrafted \"follow-up\" patch.\n> \n> There are two stances we can take:\n> \n>  (1) The user knows what he is doing.\n> \n>      In the first example, if he wanted the change in #3 to end up in B,\n>      he would have arranged the patches in a different order, namely, 3 1\n>      2, but he didn't.  We should modify A (that came from C).\n> \n\nThis gets my vote. Standard \"diff -u\" patches have always had to be\nnumbered properly if they have even the slightest chance of interfering\nwith each other, so developers are already used to it.\n\n/Andreas\n"},{"id":"111610","messageId":"7vws9hviqk.fsf@gitster.siamese.dyndns.org","threadId":"18834","inReplyTo":"49E9B90F.8070204@op5.com","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-18T19:56:19Z","receivedAt":"2009-04-18T19:56:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <exon@op5.com> writes:\n\n>> There are two stances we can take:\n>>\n>>  (1) The user knows what he is doing.\n>>\n>>      In the first example, if he wanted the change in #3 to end up in B,\n>>      he would have arranged the patches in a different order, namely, 3 1\n>>      2, but he didn't.  We should modify A (that came from C).\n>>\n>\n> This gets my vote. Standard \"diff -u\" patches have always had to be\n> numbered properly if they have even the slightest chance of interfering\n> with each other, so developers are already used to it.\n\nYou stripped the more important part from the quote, where I describe why\nthis would not work well for the second situation.  Without addressing it,\nhow could you possibly vote?\n"},{"id":"111614","messageId":"20090418225847.54862bdf@gmail.com","threadId":"18834","inReplyTo":"7vskk6y2tl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2009-04-18T20:58:47Z","receivedAt":"2009-04-18T20:58:47Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n>   \n>> ... However, there are some cases when these two\n>> rules may cause problems:\n>>\n>> patch #1: rename A to B\n>> patch #2: rename C to A\n>> patch #3: modify A\n>>\n>> Should patch #3 modify B (which was A) or A (which was C)?\n>>\n>> patch #1: rename A to B\n>> patch #2: rename B to A\n>> patch #3: modify A\n>> patch #4: modify B\n>>\n>> Which files should be patched by #3 and #4?\n>>\n>> In my opinion both #3 and #4 should fail (or both should succeed) --\n>> with my patch only #3 will work and #4 will be rejected, because in\n>> #2 B was marked as deleted.  \n> \n> Both of the examples above cannot be emitted as a single commit by\n> format-patch; the user is feeding a combined patch.  Perhaps renames\n> in each example sequence were came from one git commit but\n> modifications are from separate commit or handcrafted \"follow-up\"\n> patch.\n\nYes, that's true. In \"normal\" case, renames and modifications should be\nhandled properly and (generally) aren't subject of this discussion.\n\n>\n> There are two stances we can take:\n> \n>  (1) The user knows what he is doing.\n> \n>      In the first example, if he wanted the change in #3 to end up in\n> B, he would have arranged the patches in a different order, namely, 3\n> 1 2, but he didn't.  We should modify A (that came from C).\n> \n>  (2) In situations like these when it is unusual and there is no\n> clear and unambiguous answer, the rule has always been \"fail and ask\n> the user to clean up\", because silently doing a wrong thing in an\n> unusual situation that happens only once in a while is far worse than\n>      interrupting the user and forcing a manual intervention.\n> \n>      In the first example, there is no clear answer.  Perhaps all\n> three patches were independent patches (the first two obviously came\n> from git because only we can do renames, but they may have been\n> separate commits), and the user may have reordered them (or just\n> picked a random order because he was linearizing a history with a\n> merge).\n> \n> The second one is even iffier.  If we _know_ that originally patch #1\n> and #2 came from the same commit, then they represent swapping\n> between A and B, but if they came from different git commits, and if\n> the user picked patches in a random order, it may mean something\n> completely different.\n\nThe problem here is that there are at least two patches which touch the\nsame file(s) and it is impossible to say which patches should be handled\natomically. However, there is no easy way to specify renames as a\nsingle patch. A diff containing swapping of three files looks like this:\n\n\tdiff --git a/file2 b/file1\n\tsimilarity index 100%\n\trename from file2\n\trename to file1\n\tdiff --git a/file3 b/file2\n\tsimilarity index 100%\n\trename from file3\n\trename to file2\n\tdiff --git a/file1 b/file3\n\tsimilarity index 100%\n\trename from file1\n\trename to file3\n\nBTW: it applies correctly :).\n\n> \n> I am somewhat tempted to say that we should fail all of them,\n> including the original \"single patch swapping files\" brought up by\n> Linus.\n\nI may agree that difficult scenarios should be rejected, but I will\nalso say that git-apply should always accept git-diff output.\n\n> \n> BUT\n> \n> Can we make use simple rule to detect problematic cases?\n> \n>  - An input to git-apply can contain more than one patch that affects\n> a path; however\n> \n>    - you cannot create a path that still exists, except for a path\n> that _will_ be renamed away or removed (your patch fixes this by\n> adding this \"except for...\" part to loosen the existing rule);\n> \n>    - you cannot modify a path in a separate patch if it is involved\n> in an either side of a rename (this will catch the ambiguity of patch\n> #3 in your first example and #3 and #4 in your second example);\n\nWhat should happen in following situation:\n\npatch #1: modify A\npatch #2: rename A to B\n\n#2 should fail? Now it creates new B which is a copy of A before\napplying any patches and modifies A according to #1.\n\nAFAIC, copies and renames are handled differently from normal\nmodifications (in_fn_table() is not used for them, but\nadd_to_fn_table() is, so \"rename patches don't look in the past, but\nhave influence upon the future\").\n\n> \n>  - In addition:\n> \n>    - the same path cannot be renamed from more than once (this will\n> catch concatenation of two git generated patches);\n> \n> With such a change, I think we can keep the safety of \"when there are\n> more than one plausible outcomes, the tool shouldn't silently decide,\n> nor make progress that the user later needs to undo and redo\", while\n> allowing a sane use of rename patches generated out of a git commit.\n> \n> \n\nDo you mean that patches which break above rules should be\nskipped when \"--reject\" is set, as other failures? Or that\nwhole git-apply should fail regardless of \"--reject\"?\n\n\nMichal Kiedrowicz\n"},{"id":"111616","messageId":"20090418230357.47b7c6c2@gmail.com","threadId":"18834","inReplyTo":"20090418225847.54862bdf@gmail.com","subject":"[PATCH] tests: make test-apply-criss-cross-rename more robust","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2009-04-18T21:03:57Z","receivedAt":"2009-04-18T21:03:57Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"I realized that this test does check if git-apply succeeds, but doesn't\ntell if it applies patches correctly. So I added test_cmp to check it.\n\nI also added a test which checks swapping three files.\n---\n t/t4130-apply-criss-cross-rename.sh |   34 +++++++++++++++++++++++++++++++---\n 1 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4130-apply-criss-cross-rename.sh b/t/t4130-apply-criss-cross-rename.sh\nindex 8623dbe..7cfa2d6 100755\n--- a/t/t4130-apply-criss-cross-rename.sh\n+++ b/t/t4130-apply-criss-cross-rename.sh\n@@ -15,14 +15,17 @@ create_file() {\n test_expect_success 'setup' '\n \tcreate_file file1 \"File1 contents\" &&\n \tcreate_file file2 \"File2 contents\" &&\n-\tgit add file1 file2 &&\n+\tcreate_file file3 \"File3 contents\" &&\n+\tgit add file1 file2 file3 &&\n \tgit commit -m 1\n '\n \n test_expect_success 'criss-cross rename' '\n \tmv file1 tmp &&\n \tmv file2 file1 &&\n-\tmv tmp file2\n+\tmv tmp file2 &&\n+\tcp file1 file1-swapped &&\n+\tcp file2 file2-swapped\n '\n \n test_expect_success 'diff -M -B' '\n@@ -32,7 +35,32 @@ test_expect_success 'diff -M -B' '\n '\n \n test_expect_success 'apply' '\n-\tgit apply diff\n+\tgit apply diff &&\n+\ttest_cmp file1 file1-swapped &&\n+\ttest_cmp file2 file2-swapped\n+'\n+\n+test_expect_success 'criss-cross rename' '\n+\tgit reset --hard &&\n+\tmv file1 tmp &&\n+\tmv file2 file1 &&\n+\tmv file3 file2\n+\tmv tmp file3 &&\n+\tcp file1 file1-swapped &&\n+\tcp file2 file2-swapped &&\n+\tcp file3 file3-swapped\n+'\n+\n+test_expect_success 'diff -M -B' '\n+\tgit diff -M -B > diff &&\n+\tgit reset --hard\n+'\n+\n+test_expect_success 'apply' '\n+\tgit apply diff &&\n+\ttest_cmp file1 file1-swapped &&\n+\ttest_cmp file2 file2-swapped &&\n+\ttest_cmp file3 file3-swapped\n '\n \n test_done\n-- \n1.6.0.6\n"},{"id":"111618","messageId":"7viql1ty6l.fsf@gitster.siamese.dyndns.org","threadId":"18834","inReplyTo":"20090418225847.54862bdf@gmail.com","subject":"Re: [PATCH] builtin-apply: keep information about files to be deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-18T22:05:38Z","receivedAt":"2009-04-18T22:05:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n>>   \n>>> ... However, there are some cases when these two\n>>> rules may cause problems:\n>>>\n>>> patch #1: rename A to B\n>>> patch #2: rename C to A\n>>> patch #3: modify A\n>>>\n>>> Should patch #3 modify B (which was A) or A (which was C)?\n>>>\n>>> patch #1: rename A to B\n>>> patch #2: rename B to A\n>>> patch #3: modify A\n>>> patch #4: modify B\n>>>\n>>> Which files should be patched by #3 and #4?\n>>>\n>>> In my opinion both #3 and #4 should fail (or both should succeed) --\n>>> with my patch only #3 will work and #4 will be rejected, because in\n>>> #2 B was marked as deleted.  \n>> \n>> Both of the examples above cannot be emitted as a single commit by\n>> format-patch; the user is feeding a combined patch.  Perhaps renames\n>> in each example sequence were came from one git commit but\n>> modifications are from separate commit or handcrafted \"follow-up\"\n>> patch.\n>\n> Yes, that's true. In \"normal\" case, renames and modifications should be\n> handled properly and (generally) aren't subject of this discussion.\n>\n>>\n>> There are two stances we can take:\n>> \n>>  (1) The user knows what he is doing.\n>> \n>>      In the first example, if he wanted the change in #3 to end up in\n>> B, he would have arranged the patches in a different order, namely, 3\n>> 1 2, but he didn't.  We should modify A (that came from C).\n>> \n>>  (2) In situations like these when it is unusual and there is no\n>> clear and unambiguous answer, the rule has always been \"fail and ask\n>> the user to clean up\", because silently doing a wrong thing in an\n>> unusual situation that happens only once in a while is far worse than\n>>      interrupting the user and forcing a manual intervention.\n>> \n>>      In the first example, there is no clear answer.  Perhaps all\n>> three patches were independent patches (the first two obviously came\n>> from git because only we can do renames, but they may have been\n>> separate commits), and the user may have reordered them (or just\n>> picked a random order because he was linearizing a history with a\n>> merge).\n>> \n>> The second one is even iffier.  If we _know_ that originally patch #1\n>> and #2 came from the same commit, then they represent swapping\n>> between A and B, but if they came from different git commits, and if\n>> the user picked patches in a random order, it may mean something\n>> completely different.\n>\n> The problem here is that there are at least two patches which touch the\n> same file(s) and it is impossible to say which patches should be handled\n> atomically. However, there is no easy way to specify renames as a\n> single patch. A diff containing swapping of three files looks like this:\n>\n> \tdiff --git a/file2 b/file1\n> \tsimilarity index 100%\n> \trename from file2\n> \trename to file1\n> \tdiff --git a/file3 b/file2\n> \tsimilarity index 100%\n> \trename from file3\n> \trename to file2\n> \tdiff --git a/file1 b/file3\n> \tsimilarity index 100%\n> \trename from file1\n> \trename to file3\n>\n> BTW: it applies correctly :).\n>\n>> \n>> I am somewhat tempted to say that we should fail all of them,\n>> including the original \"single patch swapping files\" brought up by\n>> Linus.\n>\n> I may agree that difficult scenarios should be rejected, but I will\n> also say that git-apply should always accept git-diff output.\n>\n>> \n>> BUT\n>> \n>> Can we make use simple rule to detect problematic cases?\n>> \n>>  - An input to git-apply can contain more than one patch that affects\n>> a path; however\n>> \n>>    - you cannot create a path that still exists, except for a path\n>> that _will_ be renamed away or removed (your patch fixes this by\n>> adding this \"except for...\" part to loosen the existing rule);\n>> \n>>    - you cannot modify a path in a separate patch if it is involved\n>> in an either side of a rename (this will catch the ambiguity of patch\n>> #3 in your first example and #3 and #4 in your second example);\n>\n> What should happen in following situation:\n>\n> patch #1: modify A\n> patch #2: rename A to B\n>\n> #2 should fail? Now it creates new B which is a copy of A before\n> applying any patches and modifies A according to #1.\n\nYes.  It is obviously a handcrafted sequence, and it could even have been\nmechanically created.\n\nImagine a merge of two branches like this:\n\n               2----HEAD\n              /    /\n\tcommon----1\n\nand somebody fed \"common..HEAD\" to his script that internally runs\nformat-patch and squashes the patch output into one, perhaps:\n\n\t#!/bin/sh\n\t# Create a single patch e-mail, squashed.\n        tmp=/var/tmp/my-squash$$\n\trm -rf \"$tmp\" && mkdir -p \"$tmp/out\" || exit\n        trap 'rm -rf \"$tmp\"' 0 1 2 3 15\n\tgit format-patch -o \"$tmp/out\" \"$@\"\n\t>\"$tmp/all.messages\"\n        >\"$tmp/all.patches\"\n        for mail in \"$tmp\"/out/0*\n        do\n        \tgit mailinfo \"$tmp/msg\" \"$tmp/patch\" >\"$tmp/info\" <\"$mail\"\n                echo \"$mail\" >>.messages\n                cat \"$tmp/msg\" >>\"$tmp/all.messages\"\n                cat \"$tmp/patch\" >>\"$tmp/all.patches\"\n\tdone\n\t(\n\t        cat \"$tmp/info\"; echo\n                cat \"$tmp/all.messages\"; echo\n\t        cat \"$tmp/all.patches\"\n\t)\n\nDepending on the sort order between #1 and #2, you cannot tell \"modify A\nand then rename it to B\" is the order we will see such a patch at all.  I\nthink it is safer to reject such a patch with the \"when in doubt, do not\nact too clever and risk making a silent mistake\" principle.\n\nIn this particular case, the reverse order of \"renaming A to B\" and then\n\"modifing A\" would fail anyway, but if you have another patch that renames\nC to A in the mix of patches whose order cannot be determined, I think you\ncan come up with a sequence that results in an \"applicable in order but is\nthe order really what the author intended?\" situation.\n\n> Do you mean that patches which break above rules should be\n> skipped when \"--reject\" is set, as other failures? Or that\n> whole git-apply should fail regardless of \"--reject\"?\n\nI meant the latter.\n"}]}