{"thread":{"id":"38978","subject":"Kernel bug caused by 'git apply' misapplying a patch with ambiguous chunk","startedAt":"2015-04-02T15:25:12Z","lastAt":"2015-04-02T16:13:08Z","messageCount":3,"participants":["Mikko Perttunen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"258859","messageId":"551D5F58.4010201@kapsi.fi","threadId":"38978","inReplyTo":null,"subject":"Kernel bug caused by 'git apply' misapplying a patch with ambiguous chunk","fromName":"Mikko Perttunen","fromEmail":"mikko.perttunen@kapsi.fi","sentAt":"2015-04-02T15:25:12Z","receivedAt":"2015-04-02T15:25:12Z","isPatch":false,"sender":{"key":"mikko.perttunen@kapsi.fi","avatar":null},"body":"Hello everyone,\n\nwe recently ran into a kernel bug caused by git misapplying this patch: \nhttps://lkml.org/lkml/2014/7/3/896 .\n\nThe chunk '@@ -653,6 +655,7 @@' in tegra124.dtsi (the second file in the \npatch) has ambiguous context (there are several almost identical PHY \nnodes in the file). Git applied the chunk to the second PHY node when it \nshould have been applied to the first node.\n\nYou can reproduce this by checking out, for example, version 3.16 of the \nLinux kernel and applying the patch from the above link and then looking at\n\n arch/arm/boot/dts/tegra124.dtsi\n\nand verifying that git has added the 'nvidia,has-utmi-pad-registers'\nproperty to the second, instead of the first 'phy' node.\n\nOf course this is probably rather hard to fix on the applying end; but \nperhaps format-patch could check for ambiguous chunks and either warn \nthe user or increase the context size automatically, or apply could warn \nabout the chunk being ambiguous?\n\nThanks,\nMikko Perttunen\n"},{"id":"258861","messageId":"xmqqy4majzb4.fsf@gitster.dls.corp.google.com","threadId":"38978","inReplyTo":"551D5F58.4010201@kapsi.fi","subject":"Re: Kernel bug caused by 'git apply' misapplying a patch with ambiguous chunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-02T15:59:59Z","receivedAt":"2015-04-02T15:59:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mikko Perttunen writes:\n\n> Of course this is probably rather hard to fix on the applying end; but\n> perhaps format-patch could check for ambiguous chunks and either warn\n> the user or increase the context size automatically, or apply could\n> warn about the chunk being ambiguous?\n\nInteresting thought. Let me rephrase to make sure I got your\nthought process correctly.\n\nImagine you started from an original that had two cut-and-pasted\ncodeblocks A and B in the same file and updated one of them, say A,\nand then sent out the patch that turns A into A1.\n\nMeanwhile, somebody started from the same original and updated the\nsame codeblock A in the upstream already to A2. Your patch applies\ncleanly to codeblock B and turns it to A1, which is a mispatch. And\nyou cannot even detect the problem while applying.\n\nBut if you are starting from the original with idential A and B,\nformat-patch can see that the resulting patch to turn A to A1 can be\nmisunderstood to be a patch to change B to A1 instead. So in that\ncase, you _could_ detect.\n\nBut imagine if you started from an original that had A and C, that\nare clearly different. Your change turns A into A1. In the\nmeantime, the upstream started from the same original, and changed C\ninto B that looks identical to A.\n\nThe same thing would happen to your patch when you try to apply it.\n\"git apply\" could try to diagnose this situation and warn. But you\ncannot check when your format-patch produces a patch that turns \ninto , as there is no ambiguity in the original.\n\nSo,\n\n - format-patch could try to help, but it won't be a complete\n solution.\n\n - apply could try to help, but it won't be a complete solution.\n\nI am not sure if having \"both\" would make it complete, but I doubt\nit.\n"},{"id":"258862","messageId":"551D6A94.3050707@kapsi.fi","threadId":"38978","inReplyTo":"xmqqy4majzb4.fsf@gitster.dls.corp.google.com","subject":"Re: Kernel bug caused by 'git apply' misapplying a patch with ambiguous chunk","fromName":"Mikko Perttunen","fromEmail":"mikko.perttunen@kapsi.fi","sentAt":"2015-04-02T16:13:08Z","receivedAt":"2015-04-02T16:13:08Z","isPatch":false,"sender":{"key":"mikko.perttunen@kapsi.fi","avatar":null},"body":"On 04/02/2015 06:59 PM, Junio C Hamano wrote:\n> Mikko Perttunen writes:\n>\n>> Of course this is probably rather hard to fix on the applying end; but\n>> perhaps format-patch could check for ambiguous chunks and either warn\n>> the user or increase the context size automatically, or apply could\n>> warn about the chunk being ambiguous?\n>\n> Interesting thought. Let me rephrase to make sure I got your\n> thought process correctly.\n>\n> Imagine you started from an original that had two cut-and-pasted\n> codeblocks A and B in the same file and updated one of them, say A,\n> and then sent out the patch that turns A into A1.\n>\n> Meanwhile, somebody started from the same original and updated the\n> same codeblock A in the upstream already to A2. Your patch applies\n> cleanly to codeblock B and turns it to A1, which is a mispatch. And\n> you cannot even detect the problem while applying.\n>\n> But if you are starting from the original with idential A and B,\n> format-patch can see that the resulting patch to turn A to A1 can be\n> misunderstood to be a patch to change B to A1 instead. So in that\n> case, you _could_ detect.\n>\n> But imagine if you started from an original that had A and C, that\n> are clearly different. Your change turns A into A1. In the\n> meantime, the upstream started from the same original, and changed C\n> into B that looks identical to A.\n>\n> The same thing would happen to your patch when you try to apply it.\n> \"git apply\" could try to diagnose this situation and warn. But you\n> cannot check when your format-patch produces a patch that turns \n> into , as there is no ambiguity in the original.\n>\n> So,\n>\n> - format-patch could try to help, but it won't be a complete\n> solution.\n>\n> - apply could try to help, but it won't be a complete solution.\n>\n> I am not sure if having \"both\" would make it complete, but I doubt\n> it.\n>\n>\n\nI agree. I think you can also create a situation where neither would \ndetect the problem, if the upstream changes C such that A->A1 can be \napplied on it and at the same time changes A such that A->A1 no longer \napplies. However, such a situation should be rare, so I think these \nchecks would be useful.\n\nIn this case, the ambiguity existed the whole time both on the \nsubmitter's end and in upstream. Upstream just added more stuff into the \nfile causing the line numbers to shift. So in this case a check in \neither subcommand would have caught it.\n\nMikko\n"}]}