{"thread":{"id":"54319","subject":"[PATCH] apply: when -R, also reverse list of sections","startedAt":"2020-09-28T21:20:46Z","lastAt":"2020-10-20T22:04:57Z","messageCount":8,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406583","messageId":"20200928212038.1625698-1-jonathantanmy@google.com","threadId":"54319","inReplyTo":null,"subject":"[PATCH] apply: when -R, also reverse list of sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-28T21:20:38Z","receivedAt":"2020-09-28T21:20:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A patch changing a symlink into a file is written with 2 sections (in\nthe code, represented as \"struct patch\"): firstly, the deletion of the\nsymlink, and secondly, the creation of the file. When applying that\npatch with -R, the sections are reversed, so we get:\n\n (1) creation of a symlink, then\n (2) deletion of a file.\n\nThis causes an issue when the \"deletion of a file\" section is checked,\nbecause Git observes that the so-called file is not a file but a\nsymlink, resulting in a \"wrong type\" error message.\n\nWhat we want is:\n\n (1) deletion of a file, then\n (2) creation of a symlink.\n\nIn the code, this is reflected in the behavior of previous_patch() when\ninvoked from check_preimage() when the deletion is checked. Creation\nthen deletion means that when the deletion is checked, previous_patch()\nreturns the creation section, triggering a mode conflict resulting in\nthe \"wrong type\" error message. But deletion then creation means that\nwhen the deletion is checked, previous_patch() returns NULL, so the\ndeletion mode is checked against lstat, which is what we want.\n\nTherefore, when building the list of sections, build them in reverse\norder (by adding to the front of the list instead of the back) when -R\nis passed.\n\nSigned-off-by: Jonathan Tan \n---\nI traced the different behavior in previous_patch() to what the\ninvocation of to_be_deleted() returns, but I couldn't figure out why it\nreturns false in the current-but-wrong-order case but true in the\nfuture-and-correct-order case. I see that prepare_fn_table() sets\nPATH_TO_BE_DELETED for deletions, but couldn't figure out where and when\nit is set to something else. Further compounding my confusion,\nconceptually, at the point of checking the deletion, both patches are\n(in theory) \"to be deleted\". Any help in this is appreciated.\n---\n apply.c | 9 +++++++--\n t/t4114-apply-typechange.sh | 7 +++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 76dba93c97..359ceb632c 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4699,8 +4699,13 @@ static int apply_patch(struct apply_state *state,\n \t\t\treverse_patches(patch);\n \t\tif (use_patch(state, patch)) {\n \t\t\tpatch_stats(state, patch);\n-\t\t\t*listp = patch;\n-\t\t\tlistp = &patch->next;\n+\t\t\tif (!list || !state->apply_in_reverse) {\n+\t\t\t\t*listp = patch;\n+\t\t\t\tlistp = &patch->next;\n+\t\t\t} else {\n+\t\t\t\tpatch->next = list;\n+\t\t\t\tlist = patch;\n+\t\t\t}\n \n \t\t\tif ((patch->new_name &&\n \t\t\t ends_with_path_components(patch->new_name,\ndiff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh\nindex ebadbc347f..da3e64f811 100755\n--- a/t/t4114-apply-typechange.sh\n+++ b/t/t4114-apply-typechange.sh\n@@ -88,6 +88,13 @@ test_expect_success 'symlink becomes file' '\n \t'\n test_debug 'cat patch'\n \n+test_expect_success 'symlink becomes file, in reverse' '\n+\tgit checkout -f foo-symlinked-to-bar &&\n+\tgit diff-tree -p HEAD foo-back-to-file > patch &&\n+\tgit checkout foo-back-to-file &&\n+\tgit apply -R --index < patch\n+\t'\n+\n test_expect_success 'binary file becomes symlink' '\n \tgit checkout -f foo-becomes-binary &&\n \tgit diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&\n-- \n2.28.0.709.gb0816b6eb0-goog\n\n"},{"id":"406587","messageId":"xmqqy2ktk11t.fsf@gitster.c.googlers.com","threadId":"54319","inReplyTo":"20200928212038.1625698-1-jonathantanmy@google.com","subject":"Re: [PATCH] apply: when -R, also reverse list of sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T22:07:42Z","receivedAt":"2020-09-28T23:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> A patch changing a symlink into a file is written with 2 sections (in\n> the code, represented as \"struct patch\"): firstly, the deletion of the\n> symlink, and secondly, the creation of the file. When applying that\n> patch with -R, the sections are reversed, so we get:\n>\n> (1) creation of a symlink, then\n> (2) deletion of a file.\n\nGood observation.\n\nBut I have to wonder if it breaks the support for (arguably outside\nthe Git usecase) input that has more than one patch that touches the\nsame path to blindly reverse the order of all patches (instead of\nthe obvious implementation of the fix for the above stated problem\n--- i.e. make sure the first patch is a deletion of a symlink and\nwhat immediately follows is a creation of a regular file, and swap\nthem only in such a case).\n"},{"id":"407997","messageId":"20201020191206.104924-1-jonathantanmy@google.com","threadId":"54319","inReplyTo":"xmqqy2ktk11t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] apply: when -R, also reverse list of sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-10-20T19:12:06Z","receivedAt":"2020-10-20T19:12:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > A patch changing a symlink into a file is written with 2 sections (in\n> > the code, represented as \"struct patch\"): firstly, the deletion of the\n> > symlink, and secondly, the creation of the file. When applying that\n> > patch with -R, the sections are reversed, so we get:\n> >\n> > (1) creation of a symlink, then\n> > (2) deletion of a file.\n> \n> Good observation.\n> \n> But I have to wonder if it breaks the support for (arguably outside\n> the Git usecase) input that has more than one patch that touches the\n> same path to blindly reverse the order of all patches\n\nSorry for getting back to this so late.\n\nThe only other case I can think of (besides symlink<->file) is\ndirectory<->file, and even in that case, I think blindly reversing the\norder still works.\n\nIf a more sophisticated rearrangement was needed, I would think that\neven applying the patches in the forward direction (that is, without\n\"-R\") wouldn't work, since Git is sensitive to the order of the patches.\nSo I don't think we need to support such input (since they wouldn't work\nin the forward direction anyway).\n\n> (instead of\n> the obvious implementation of the fix for the above stated problem\n> --- i.e. make sure the first patch is a deletion of a symlink and\n> what immediately follows is a creation of a regular file, and swap\n> them only in such a case).\n\nThis would make patch application more robust, but I still appreciate\nthe relative simplicity of the existing approach - the patches you see\nin the input are all applied in order (or reverse order, in the case of\n\"-R\"). You also wouldn't end up in the situation where you think that\nyour input works because it works with your version of Git, but it\nactually doesn't work with an older version of Git or another\nimplementation. (I haven't researched how other implementation do it,\nthough.)\n"},{"id":"408005","messageId":"xmqqeelsbr2c.fsf@gitster.c.googlers.com","threadId":"54319","inReplyTo":"20201020191206.104924-1-jonathantanmy@google.com","subject":"Re: [PATCH] apply: when -R, also reverse list of sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-20T20:06:19Z","receivedAt":"2020-10-20T20:06:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> But I have to wonder if it breaks the support for (arguably outside\n>> the Git usecase) input that has more than one patch that touches the\n>> same path to blindly reverse the order of all patches\n>\n> Sorry for getting back to this so late.\n>\n> The only other case I can think of (besides symlink<->file) is\n> directory<->file, and even in that case, I think blindly reversing the\n> order still works.\n>\n> If a more sophisticated rearrangement was needed, I would think that\n> even applying the patches in the forward direction (that is, without\n> \"-R\") wouldn't work, since Git is sensitive to the order of the patches.\n> So I don't think we need to support such input (since they wouldn't work\n> in the forward direction anyway).\n\nI wish you told that to those who added fn_table kludge to apply.c\nback when they did so. They apparently wanted to have a patch that\nhas more than one \"diff --git a/hello.c b/hello.c\" that talks about\nthe same file applied with a single invocation of \"git apply\".\nPerhaps what they did is already broken with \"apply -R\", and blind\nreversal of everything magically makes it work? Or what they did\nalready works with \"apply -R\" and your blind reversal would break,\nunless you undo what they did?\n\n>> (instead of\n>> the obvious implementation of the fix for the above stated problem\n>> --- i.e. make sure the first patch is a deletion of a symlink and\n>> what immediately follows is a creation of a regular file, and swap\n>> them only in such a case).\n>\n> This would make patch application more robust, but I still appreciate\n> the relative simplicity of the existing approach\n\nI'd rather want to see that we keep the normal cases simple,\ni.e. majority parts of a patch with \"apply -R\" that did *not* have\nto futz with the application order will keep what we do, and if\nthere are tricky cases like typechange diff, only special case them.\n\nThanks.\n"},{"id":"408015","messageId":"xmqqo8kwaagx.fsf@gitster.c.googlers.com","threadId":"54319","inReplyTo":"xmqqeelsbr2c.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] apply: when -R, also reverse list of sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-20T20:50:06Z","receivedAt":"2020-10-20T20:50:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I wish you told that to those who added fn_table kludge to apply.c\n> back when they did so. They apparently wanted to have a patch that\n> has more than one \"diff --git a/hello.c b/hello.c\" that talks about\n> the same file applied with a single invocation of \"git apply\".\n> Perhaps what they did is already broken with \"apply -R\", and blind\n> reversal of everything magically makes it work? Or what they did\n> already works with \"apply -R\" and your blind reversal would break,\n> unless you undo what they did?\n\n;-) It turns out that it was the former.\n\nWithout your \"blindly reverse everything\" patch, the attached patch\nillustrates how the \"touch the same path more than once\" support\nintroduced in 7a07841c (git-apply: handle a patch that touches the\nsame path more than once better, 2008-06-27) is broken with respect\nto \"apply -R\".\n\nSo, you should be able to sell the change to fix _two_ bugs ;-)\n\nThanks.\n\ndiff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh\nindex 972946c174..fa824ac09f 100755\n--- a/t/t4127-apply-same-fn.sh\n+++ b/t/t4127-apply-same-fn.sh\n@@ -30,7 +30,7 @@ test_expect_success 'apply same filename with independent changes' '\n \ttest_cmp same_fn same_fn2\n '\n \n-test_expect_success 'apply same filename with overlapping changes' '\n+test_expect_failure 'apply same filename with overlapping changes' '\n \tgit reset --hard &&\n \tmodify \"s/^d/z/\" same_fn &&\n \tgit diff > patch0 &&\n@@ -39,8 +39,13 @@ test_expect_success 'apply same filename with overlapping changes' '\n \tgit diff >> patch0 &&\n \tcp same_fn same_fn2 &&\n \tgit reset --hard &&\n+\tcp same_fn same_fn1 &&\n+\n \tgit apply patch0 &&\n-\ttest_cmp same_fn same_fn2\n+\ttest_cmp same_fn same_fn2 &&\n+\n+\tgit apply -R patch0 &&\n+\ttest_cmp same_fn same_fn1\n '\n \n test_expect_success 'apply same new filename after rename' '\n"},{"id":"408020","messageId":"20201020213651.691101-1-jonathantanmy@google.com","threadId":"54319","inReplyTo":"xmqqo8kwaagx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] apply: when -R, also reverse list of sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-10-20T21:36:51Z","receivedAt":"2020-10-20T21:36:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Junio C Hamano writes:\n> \n> > I wish you told that to those who added fn_table kludge to apply.c\n> > back when they did so. They apparently wanted to have a patch that\n> > has more than one \"diff --git a/hello.c b/hello.c\" that talks about\n> > the same file applied with a single invocation of \"git apply\".\n> > Perhaps what they did is already broken with \"apply -R\", and blind\n> > reversal of everything magically makes it work? Or what they did\n> > already works with \"apply -R\" and your blind reversal would break,\n> > unless you undo what they did?\n> \n> ;-) It turns out that it was the former.\n> \n> Without your \"blindly reverse everything\" patch, the attached patch\n> illustrates how the \"touch the same path more than once\" support\n> introduced in 7a07841c (git-apply: handle a patch that touches the\n> same path more than once better, 2008-06-27) is broken with respect\n> to \"apply -R\".\n\nAh, thanks for checking.\n\n> So, you should be able to sell the change to fix _two_ bugs ;-)\n> \n> Thanks.\n> \n> diff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh\n> index 972946c174..fa824ac09f 100755\n> --- a/t/t4127-apply-same-fn.sh\n> +++ b/t/t4127-apply-same-fn.sh\n> @@ -30,7 +30,7 @@ test_expect_success 'apply same filename with independent changes' '\n> \ttest_cmp same_fn same_fn2\n> '\n> \n> -test_expect_success 'apply same filename with overlapping changes' '\n> +test_expect_failure 'apply same filename with overlapping changes' '\n> \tgit reset --hard &&\n> \tmodify \"s/^d/z/\" same_fn &&\n> \tgit diff > patch0 &&\n> @@ -39,8 +39,13 @@ test_expect_success 'apply same filename with overlapping changes' '\n> \tgit diff >> patch0 &&\n> \tcp same_fn same_fn2 &&\n> \tgit reset --hard &&\n> +\tcp same_fn same_fn1 &&\n> +\n> \tgit apply patch0 &&\n> -\ttest_cmp same_fn same_fn2\n> +\ttest_cmp same_fn same_fn2 &&\n> +\n> +\tgit apply -R patch0 &&\n> +\ttest_cmp same_fn same_fn1\n> '\n> \n> test_expect_success 'apply same new filename after rename' '\n\nIndeed, with my patch, this test passes instead of fails.\n\nShould I resend a version 2 that includes this test or will you apply\nthis to your local copy?\n"},{"id":"408022","messageId":"xmqqblgwa7s5.fsf@gitster.c.googlers.com","threadId":"54319","inReplyTo":"20201020213651.691101-1-jonathantanmy@google.com","subject":"Re: [PATCH] apply: when -R, also reverse list of sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-20T21:48:10Z","receivedAt":"2020-10-20T21:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Should I resend a version 2 that includes this test or will you apply\n> this to your local copy?\n\nThe former, as I do not want the final version to be not shown to\nthe list---we'll miss a good opportunity for other people to spot\nissues we didn't think about.\n\nThanks.\n"},{"id":"408025","messageId":"20201020220452.697760-1-jonathantanmy@google.com","threadId":"54319","inReplyTo":"xmqqblgwa7s5.fsf@gitster.c.googlers.com","subject":"[PATCH v2] apply: when -R, also reverse list of sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-10-20T22:04:52Z","receivedAt":"2020-10-20T22:04:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A patch changing a symlink into a file is written with 2 sections (in\nthe code, represented as \"struct patch\"): firstly, the deletion of the\nsymlink, and secondly, the creation of the file. When applying that\npatch with -R, the sections are reversed, so we get:\n\n (1) creation of a symlink, then\n (2) deletion of a file.\n\nThis causes an issue when the \"deletion of a file\" section is checked,\nbecause Git observes that the so-called file is not a file but a\nsymlink, resulting in a \"wrong type\" error message.\n\nWhat we want is:\n\n (1) deletion of a file, then\n (2) creation of a symlink.\n\nIn the code, this is reflected in the behavior of previous_patch() when\ninvoked from check_preimage() when the deletion is checked. Creation\nthen deletion means that when the deletion is checked, previous_patch()\nreturns the creation section, triggering a mode conflict resulting in\nthe \"wrong type\" error message. But deletion then creation means that\nwhen the deletion is checked, previous_patch() returns NULL, so the\ndeletion mode is checked against lstat, which is what we want.\n\nThere are also other ways a patch can contain 2 sections referencing the\nsame file, for example, in 7a07841c0b (\"git-apply: handle a patch that\ntouches the same path more than once better\", 2008-06-27). \"git apply\n-R\" fails in the same way, and this commit makes this case succeed.\n\nTherefore, when building the list of sections, build them in reverse\norder (by adding to the front of the list instead of the back) when -R\nis passed.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Jonathan Tan \n---\nOK - updated the commit message and added a test for the use case Junio\ndescribed.\n---\n apply.c | 9 +++++++--\n t/t4114-apply-typechange.sh | 7 +++++++\n t/t4127-apply-same-fn.sh | 9 +++++++++\n 3 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 76dba93c97..359ceb632c 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4699,8 +4699,13 @@ static int apply_patch(struct apply_state *state,\n \t\t\treverse_patches(patch);\n \t\tif (use_patch(state, patch)) {\n \t\t\tpatch_stats(state, patch);\n-\t\t\t*listp = patch;\n-\t\t\tlistp = &patch->next;\n+\t\t\tif (!list || !state->apply_in_reverse) {\n+\t\t\t\t*listp = patch;\n+\t\t\t\tlistp = &patch->next;\n+\t\t\t} else {\n+\t\t\t\tpatch->next = list;\n+\t\t\t\tlist = patch;\n+\t\t\t}\n \n \t\t\tif ((patch->new_name &&\n \t\t\t ends_with_path_components(patch->new_name,\ndiff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh\nindex ebadbc347f..da3e64f811 100755\n--- a/t/t4114-apply-typechange.sh\n+++ b/t/t4114-apply-typechange.sh\n@@ -88,6 +88,13 @@ test_expect_success 'symlink becomes file' '\n \t'\n test_debug 'cat patch'\n \n+test_expect_success 'symlink becomes file, in reverse' '\n+\tgit checkout -f foo-symlinked-to-bar &&\n+\tgit diff-tree -p HEAD foo-back-to-file > patch &&\n+\tgit checkout foo-back-to-file &&\n+\tgit apply -R --index < patch\n+\t'\n+\n test_expect_success 'binary file becomes symlink' '\n \tgit checkout -f foo-becomes-binary &&\n \tgit diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&\ndiff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh\nindex 972946c174..305b7e649e 100755\n--- a/t/t4127-apply-same-fn.sh\n+++ b/t/t4127-apply-same-fn.sh\n@@ -32,6 +32,10 @@ test_expect_success 'apply same filename with independent changes' '\n \n test_expect_success 'apply same filename with overlapping changes' '\n \tgit reset --hard &&\n+\n+\t# Store same_fn so that we can check apply -R in next test\n+\tcp same_fn same_fn1 &&\n+\n \tmodify \"s/^d/z/\" same_fn &&\n \tgit diff > patch0 &&\n \tgit add same_fn &&\n@@ -43,6 +47,11 @@ test_expect_success 'apply same filename with overlapping changes' '\n \ttest_cmp same_fn same_fn2\n '\n \n+test_expect_success 'apply same filename with overlapping changes, in reverse' '\n+\tgit apply -R patch0 &&\n+\ttest_cmp same_fn same_fn1\n+'\n+\n test_expect_success 'apply same new filename after rename' '\n \tgit reset --hard &&\n \tgit mv same_fn new_fn &&\n-- \n2.29.0.rc1.297.gfa9743e501-goog\n\n"}]}