{"thread":{"id":"61728","subject":"`git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","startedAt":"2024-07-03T15:25:09Z","lastAt":"2024-07-06T05:43:37Z","messageCount":8,"participants":["Emanuel Czirai","rsbecker@nexbridge.com","Emanuel Attila Czirai","Johannes Sixt","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"498033","messageId":"CAFjaU5sAVaNHZ0amPXJcbSvsnaijo+3X5Otg_Mntkx2GbikZMA@mail.gmail.com","threadId":"61728","inReplyTo":null,"subject":"`git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Emanuel Czirai","fromEmail":"correabuscar+gitml@gmail.com","sentAt":"2024-07-03T15:24:58Z","receivedAt":"2024-07-03T15:25:09Z","isPatch":false,"sender":{"key":"correabuscar+gitml@gmail.com","avatar":null},"body":"This doesn't affect `git rebase` as it's way more robust than simply\nextracting the commits as patches and re-applying them. (I haven't looked\ninto `git merge` though, but I doubt it's affected)\n\nIt seems to me that no matter which algorithm `git diff` uses  (of the 4),\nit generates the same hunk, because it's really the context length that\nmatters (which by default is 3), which is the same one that gnu `diff`\ngenerates, and its application via `git apply` is the same as gnu `patch`.\n\nside note:\n`diffy` is a simple rust-written library that behaves (at version 0.4.0)\nthe same as normal diff and patch apply(with regards to generated diff\ncontents and patch application; except it doesn't set the original/modified\nfilenames in the patch), and since my limited experience, I've found it\nsimpler to modify it to make it so that it generates unambiguous hunks, as\na proof of concept that it can be done this way, here:\nhttps://github.com/bmwill/diffy/issues/31 , whereby increasing the context\nlength(lines) of the whole patch(though ideally only of the affected hunks)\nthe initially ambiguous hunk(s) cannot be applied anymore in more than 1\nplace inside the original file, thus avoiding both the diff creation and\nthe patch application from generating and applying ambiguous hunks.\n\nBut forgetting about that for a moment, I'm gonna show you about `git diff`\nand `git apply` below:\n1. clone cargo's repo:\ncd /tmp\ngit clone https://github.com/rust-lang/cargo.git\ncd cargo\n2. checkout tag 0.76.0, or branch origin/rust-1.75.0\ngit checkout 0.76.0\n3. manually edit this file ./src/cargo/core/workspace.rs at line 1118\nor manually go to function:\npub fn emit_warnings(&self) -> CargoResult<()> {\nright before that function's end which looks like:\nOk(())\n}\nso there at line 1118, insert above that Ok(()) something, doesn't matter\nwhat, doesn't have to make sense, like:\nif seen_any_warnings {\n  //comment\n  bail!(\"reasons\");\n}\nsave the file\n4. try to generate a diff from it:\ngit diff > /tmp/foo.patch\nyou get this:\n```diff\ndiff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs\nindex 4667c8029..8f0a74473 100644\n--- a/src/cargo/core/workspace.rs\n+++ b/src/cargo/core/workspace.rs\n@@ -1115,6 +1115,10 @@ impl<'cfg> Workspace<'cfg> {\n                 }\n             }\n         }\n+        if seen_any_warnings {\n+            //comment\n+            bail!(\"reasons\");\n+        }\n         Ok(())\n     }\n\n```\nNow this is an ambiguous patch/hunk because if you try to apply this patch\non the same original file, cumulatively, it applies successfully in 3\ndifferent places, but we won't do that now.\n5. now let's discard it(we already have it saved in /tmp/foo.patch) and\npretend that something changed in the original code:\ngit checkout .\ngit checkout 0.80.0\n6. reapply that patch on the new changes:\ngit apply /tmp/foo.patch\n(this shows no errors)\n7. look at the diff, for no good reason, just to see that it's the\nsame(kind of):\ngit diff > /tmp/foo2.patch\ncontents:\n```diff\ndiff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs\nindex 55bfb7a10..92709f224 100644\n--- a/src/cargo/core/workspace.rs\n+++ b/src/cargo/core/workspace.rs\n@@ -1099,6 +1099,10 @@ impl<'gctx> Workspace<'gctx> {\n                 }\n             }\n         }\n+        if seen_any_warnings {\n+            //comment\n+            bail!(\"reasons\");\n+        }\n         Ok(())\n     }\n\n```\n8. now look at the file, where was the patch applied? that's right, it's at\nthe end of the wrong function:\nfn validate_manifest(&mut self) -> CargoResult<()> {\nvim src/cargo/core/workspace.rs +1040\njump at the end of it at line 1107, you see it's our patch there, applied\nin the wrong spot!\n\nHopefully, depending on the change, this kind of patch which applied in the\nwrong place, will be caught at (rust)compile time (in my case, it was this)\nor worse, at (binary)runtime.\n\nWith the aforementioned `diffy` patch, the generated diff would actually be\nwith a context of 4, to make it unambiguous, so it would've been this:\n```diff\n--- original\n+++ modified\n@@ -1186,8 +1186,12 @@\n                     self.gctx.shell().warn(msg)?\n                 }\n             }\n         }\n+        if seen_any_warnings {\n+            //use anyhow::bail;\n+            bail!(\"Compilation failed due to cargo warnings! Manually done\nthis(via cargo patch) so that things like the following (ie. dep key\npackages= and using rust pre 1.26.0 which ignores it, downloads squatted\npackage) will be avoided in the future:\nhttps://github.com/rust-lang/rust/security/advisories/GHSA-phjm-8x66-qw4r\");\n+        }\n         Ok(())\n     }\n\n     pub fn emit_lints(&self, pkg: &Package, path: &Path) ->\nCargoResult<()> {\n```\nthis hunk is now unambiguous because it cannot be applied in more than 1\nplace in the original file, furthermore patching a modified future file\nwill fail(with that `diffy` patch, and ideally, with `git apply` if any\nchanges are implemented to \"fix\" this issue) if any of the hunks can be\nindependently(and during the full patch application too) applied in more\nthan 1 place.\n\nI consider that I don't know enough to understand how `git diff`/`git\napply` works internally (and similarly, gnu `diff`/`patch`) to actually\nchange them and make them generate unambiguous hunks where only the hunks\nthat would've been ambiguous have increased context size, instead of the\nwhole patch have increased context size for all hunks(which is what I did\nfor `diffy` too so far, in that proof of concept patch), therefore if a\n\"fix\" is deemed necessary(it may not be, as I might've missed something and\nI'm unaware of it, so a fix may be messing other things up, who knows?!)\nthen I hope someone much more knowledgeable could implement it(maybe even\nfor gnu diff/patch too), and while I don't think that a \"please\" would be\nenough, I'm still gonna say it: please do so, if so inclined.\n\nThank you for your time and consideration.\n"},{"id":"498039","messageId":"082b01dacd61$81174a80$8345df80$@nexbridge.com","threadId":"61728","inReplyTo":"CAFjaU5sAVaNHZ0amPXJcbSvsnaijo+3X5Otg_Mntkx2GbikZMA@mail.gmail.com","subject":"RE: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-07-03T15:55:56Z","receivedAt":"2024-07-03T15:56:08Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, July 3, 2024 11:25 AM, Emanuel Czirai wrote:\n>This doesn't affect `git rebase` as it's way more robust than simply extracting the\n>commits as patches and re-applying them. (I haven't looked into `git merge` though,\n>but I doubt it's affected)\n>\n>It seems to me that no matter which algorithm `git diff` uses  (of the 4), it generates\n>the same hunk, because it's really the context length that matters (which by default\n>is 3), which is the same one that gnu `diff` generates, and its application via `git\n>apply` is the same as gnu `patch`.\n>\n>side note:\n>`diffy` is a simple rust-written library that behaves (at version 0.4.0) the same as\n>normal diff and patch apply(with regards to generated diff contents and patch\n>application; except it doesn't set the original/modified filenames in the patch), and\n>since my limited experience, I've found it simpler to modify it to make it so that it\n>generates unambiguous hunks, as a proof of concept that it can be done this way,\n>here:\n>https://github.com/bmwill/diffy/issues/31 , whereby increasing the context\n>length(lines) of the whole patch(though ideally only of the affected hunks) the\n>initially ambiguous hunk(s) cannot be applied anymore in more than 1 place inside\n>the original file, thus avoiding both the diff creation and the patch application from\n>generating and applying ambiguous hunks.\n>\n>But forgetting about that for a moment, I'm gonna show you about `git diff` and `git\n>apply` below:\n>1. clone cargo's repo:\n>cd /tmp\n>git clone https://github.com/rust-lang/cargo.git\n>cd cargo\n>2. checkout tag 0.76.0, or branch origin/rust-1.75.0 git checkout 0.76.0 3. manually\n>edit this file ./src/cargo/core/workspace.rs at line 1118 or manually go to function:\n>pub fn emit_warnings(&self) -> CargoResult<()> { right before that function's end\n>which looks like:\n>Ok(())\n>}\n>so there at line 1118, insert above that Ok(()) something, doesn't matter what,\n>doesn't have to make sense, like:\n>if seen_any_warnings {\n>  //comment\n>  bail!(\"reasons\");\n>}\n>save the file\n>4. try to generate a diff from it:\n>git diff > /tmp/foo.patch\n>you get this:\n>```diff\n>diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index\n>4667c8029..8f0a74473 100644\n>--- a/src/cargo/core/workspace.rs\n>+++ b/src/cargo/core/workspace.rs\n>@@ -1115,6 +1115,10 @@ impl<'cfg> Workspace<'cfg> {\n>                 }\n>             }\n>         }\n>+        if seen_any_warnings {\n>+            //comment\n>+            bail!(\"reasons\");\n>+        }\n>         Ok(())\n>     }\n>\n>```\n>Now this is an ambiguous patch/hunk because if you try to apply this patch on the\n>same original file, cumulatively, it applies successfully in 3 different places, but we\n>won't do that now.\n>5. now let's discard it(we already have it saved in /tmp/foo.patch) and pretend that\n>something changed in the original code:\n>git checkout .\n>git checkout 0.80.0\n>6. reapply that patch on the new changes:\n>git apply /tmp/foo.patch\n>(this shows no errors)\n>7. look at the diff, for no good reason, just to see that it's the same(kind of):\n>git diff > /tmp/foo2.patch\n>contents:\n>```diff\n>diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index\n>55bfb7a10..92709f224 100644\n>--- a/src/cargo/core/workspace.rs\n>+++ b/src/cargo/core/workspace.rs\n>@@ -1099,6 +1099,10 @@ impl<'gctx> Workspace<'gctx> {\n>                 }\n>             }\n>         }\n>+        if seen_any_warnings {\n>+            //comment\n>+            bail!(\"reasons\");\n>+        }\n>         Ok(())\n>     }\n>\n>```\n>8. now look at the file, where was the patch applied? that's right, it's at the end of\n>the wrong function:\n>fn validate_manifest(&mut self) -> CargoResult<()> { vim\n>src/cargo/core/workspace.rs +1040 jump at the end of it at line 1107, you see it's\n>our patch there, applied in the wrong spot!\n>\n>Hopefully, depending on the change, this kind of patch which applied in the wrong\n>place, will be caught at (rust)compile time (in my case, it was this) or worse, at\n>(binary)runtime.\n>\n>With the aforementioned `diffy` patch, the generated diff would actually be with a\n>context of 4, to make it unambiguous, so it would've been this:\n>```diff\n>--- original\n>+++ modified\n>@@ -1186,8 +1186,12 @@\n>                     self.gctx.shell().warn(msg)?\n>                 }\n>             }\n>         }\n>+        if seen_any_warnings {\n>+            //use anyhow::bail;\n>+            bail!(\"Compilation failed due to cargo warnings! Manually\n>+ done\n>this(via cargo patch) so that things like the following (ie. dep key packages= and\n>using rust pre 1.26.0 which ignores it, downloads squatted\n>package) will be avoided in the future:\n>https://github.com/rust-lang/rust/security/advisories/GHSA-phjm-8x66-qw4r\");\n>+        }\n>         Ok(())\n>     }\n>\n>     pub fn emit_lints(&self, pkg: &Package, path: &Path) -> CargoResult<()> { ``` this\n>hunk is now unambiguous because it cannot be applied in more than 1 place in the\n>original file, furthermore patching a modified future file will fail(with that `diffy`\n>patch, and ideally, with `git apply` if any changes are implemented to \"fix\" this issue)\n>if any of the hunks can be independently(and during the full patch application too)\n>applied in more than 1 place.\n>\n>I consider that I don't know enough to understand how `git diff`/`git apply` works\n>internally (and similarly, gnu `diff`/`patch`) to actually change them and make them\n>generate unambiguous hunks where only the hunks that would've been ambiguous\n>have increased context size, instead of the whole patch have increased context size\n>for all hunks(which is what I did for `diffy` too so far, in that proof of concept patch),\n>therefore if a \"fix\" is deemed necessary(it may not be, as I might've missed\n>something and I'm unaware of it, so a fix may be messing other things up, who\n>knows?!) then I hope someone much more knowledgeable could implement\n>it(maybe even for gnu diff/patch too), and while I don't think that a \"please\" would\n>be enough, I'm still gonna say it: please do so, if so inclined.\n>\n>Thank you for your time and consideration.\n\nYou make good points, but Rust code should not be put into the main git code base as it will break many non-GNU platforms. Perhaps rewriting it is C to be compatible with the git code-base.\n--Randall\n\n"},{"id":"498041","messageId":"CAFjaU5vvk-nNLvCyXAgU9C3ScKBNRPFB7=1PXejmLZi+r7EbNQ@mail.gmail.com","threadId":"61728","inReplyTo":"082b01dacd61$81174a80$8345df80$@nexbridge.com","subject":"Re: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Emanuel Attila Czirai","fromEmail":"corre.a.buscar@gmail.com","sentAt":"2024-07-03T16:22:46Z","receivedAt":"2024-07-03T16:22:58Z","isPatch":false,"sender":{"key":"corre.a.buscar@gmail.com","avatar":null},"body":"> >I consider that I don't know enough to understand how `git diff`/`git apply` works\n> >internally (and similarly, gnu `diff`/`patch`) to actually change them and make them\n> >generate unambiguous hunks where only the hunks that would've been ambiguous\n> >have increased context size, instead of the whole patch have increased context size\n> >for all hunks(which is what I did for `diffy` too so far, in that proof of concept patch),\n> >therefore if a \"fix\" is deemed necessary(it may not be, as I might've missed\n> >something and I'm unaware of it, so a fix may be messing other things up, who\n> >knows?!) then I hope someone much more knowledgeable could implement\n> >it(maybe even for gnu diff/patch too), and while I don't think that a \"please\" would\n> >be enough, I'm still gonna say it: please do so, if so inclined.\n> >\n> >Thank you for your time and consideration.\n>\n> You make good points, but Rust code should not be put into the main git code base as it will break many non-GNU platforms. Perhaps rewriting it is C to be compatible with the git code-base.\n> --Randall\n>\nAh, definitely whoever writes the fix would do it in C for the git\ncode base, I didn't mean to imply it would be or should be done in\nrust, therefore please excuse my failure to communicate that clearly.\nThe `diffy` proof-of-concept patch, is just for `diffy`, in rust, and\nit's just to show a way this could be done and that \"it works\" that\nway. It was easier for me to do it for `diffy` in rust, than in C for\ngit diff/apply or gnu diff/patch.\nIf a fix is to be implemented for `git diff/apply`, it would\ndefinitely not be in rust by any means, but C, as you mentioned.\nThank you for your reply.\n\nAlso, I notice that I made a mistake when pasting the patch with the\ncontext length of 4, it was a real patch not the one I used in the\nexamples, here's the corrected unambiguous patch:\n```diff\n--- original\n+++ modified\n@@ -1114,8 +1114,12 @@\n                     self.config.shell().warn(msg)?\n                 }\n             }\n         }\n+        if seen_any_warnings {\n+            //comment\n+            bail!(\"reasons\");\n+        }\n         Ok(())\n     }\n\n     pub fn set_target_dir(&mut self, target_dir: Filesystem) {\n```\n\nCheers, have a great day everyone!\n"},{"id":"498061","messageId":"772a1bd2-dd21-459a-8b95-7605fd7f52dc@kdbg.org","threadId":"61728","inReplyTo":"CAFjaU5sAVaNHZ0amPXJcbSvsnaijo+3X5Otg_Mntkx2GbikZMA@mail.gmail.com","subject":"Re: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-07-03T21:01:47Z","receivedAt":"2024-07-03T21:38:09Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.07.24 um 17:24 schrieb Emanuel Czirai:\n> With the aforementioned `diffy` patch, the generated diff would actually be\n> with a context of 4, to make it unambiguous, so it would've been this:\n> ```diff\n> --- original\n> +++ modified\n> @@ -1186,8 +1186,12 @@\n>                      self.gctx.shell().warn(msg)?\n>                  }\n>              }\n>          }\n> +        if seen_any_warnings {\n> +            //use anyhow::bail;\n> +            bail!(\"reasons\");\n> +        }\n>          Ok(())\n>      }\n> \n>      pub fn emit_lints(&self, pkg: &Package, path: &Path) ->\n> CargoResult<()> {\n> ```\n> this hunk is now unambiguous because it cannot be applied in more than 1\n> place in the original file,\n\nThis assertion is wrong, assuming that the patch is to be applied to a\nmodified version of 'original'. There is nothing that can be done at the\ntime when a patch is generated to make it unambiguous, not even if the\nentire file becomes context. The reason is that the modified 'original'\ncould now have the part duplicated that is the context in the patch, and\nit would be possible to apply the patch any one of the duplicates. Which\none?\n\n-- Hannes\n\n"},{"id":"498077","messageId":"CAFjaU5tmurBrTu_oGu526c362bi9eEgKWV9unwbBGmm9r7Dznw@mail.gmail.com","threadId":"61728","inReplyTo":"772a1bd2-dd21-459a-8b95-7605fd7f52dc@kdbg.org","subject":"Re: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Emanuel Czirai","fromEmail":"correabuscar+gitml@gmail.com","sentAt":"2024-07-04T07:15:35Z","receivedAt":"2024-07-04T07:15:46Z","isPatch":false,"sender":{"key":"correabuscar+gitml@gmail.com","avatar":null},"body":"On Wed, Jul 3, 2024 at 11:01 PM Johannes Sixt <j6t@kdbg.org> wrote:\n>\n> Am 03.07.24 um 17:24 schrieb Emanuel Czirai:\n> > With the aforementioned `diffy` patch, the generated diff would actually be\n> > with a context of 4, to make it unambiguous, so it would've been this:\n> > ```diff\n> > --- original\n> > +++ modified\n> > @@ -1186,8 +1186,12 @@\n> >                      self.gctx.shell().warn(msg)?\n> >                  }\n> >              }\n> >          }\n> > +        if seen_any_warnings {\n> > +            //use anyhow::bail;\n> > +            bail!(\"reasons\");\n> > +        }\n> >          Ok(())\n> >      }\n> >\n> >      pub fn emit_lints(&self, pkg: &Package, path: &Path) ->\n> > CargoResult<()> {\n> > ```\n> > this hunk is now unambiguous because it cannot be applied in more than 1\n> > place in the original file,\n>\n> This assertion is wrong, assuming that the patch is to be applied to a\n> modified version of 'original'. There is nothing that can be done at the\n> time when a patch is generated to make it unambiguous, not even if the\n> entire file becomes context. The reason is that the modified 'original'\n> could now have the part duplicated that is the context in the patch, and\n> it would be possible to apply the patch any one of the duplicates. Which\n> one?\n>\n> -- Hannes\n>\n\ntl;dr: you're correct indeed, a hunk is truly unambiguous only when\n`git apply` determines that, and it thus depends on the target file to\napply to; `git diff` can generate unambiguous hunks only with respect\nto the unchanged original file, but applying that hunk to a changed\noriginal file  only determines if a hunk is relatively unambiguous,\nthat is, unambiguous with respect to that changed original file. Both\ndiff-ing and patch application are needed to ensure unambiguity.\n\nThat is correct, which is why it's necessary to have `git apply` also\nmodified so that it tries to apply each hunk independently to the\nnow-modified original to see if it can be applied in more than 1\nplace, and if so, fail with the proper error message; in addition,\nafter applying it, it would try to apply it again, just in case\napplying it created a new spot for it to be reapplied, and if it\nsucceeds to re-apply it, it would fail again. I kind of mentioned this\nI believe, if not, my bad.\nBoth `git diff` and `git apply` would have to each ensure that the\nhunks are unambiguous for the hunks to be considered unambiguous,\ntherefore you're correct in your statement that my stating that the\nhunk is unambiguous right after generation via a fixed `git diff` is\nwrong. It's merely unambiguous only in the context of applying it to\nthe original file, but as soon as a new modified original file is\npresented, it can be ambiguous as it is, unless `git apply` can\ndetermine that indeed it cannot be applied in more than 1 place in\nthis new modified original file, but both: applied independently(just\nto be sure) and during the application of all previous hunks until\nthen, maybe even more than these 2, because the following hunks can\ncreate new spots for it, so a reapply could make that hunk succeed\neven if all others would fail. Not entirely sure here, how to properly\nensure this, that's why someone more knowledgeable might.\n\nAh, I see that I actually tried to say this(the above) after that\nstatement  but I used wrong wording \"will fail\" instead of \"should\nfail\" or \"would fail\" (if fixed, that is):\n> this hunk is now unambiguous because it cannot be applied in more than 1\n> place in the original file, furthermore patching a modified future file\n> will fail(with that `diffy` patch, and ideally, with `git apply` if any\n> changes are implemented to \"fix\" this issue) if any of the hunks can be\n> independently(and during the full patch application too) applied in more\n> than 1 place.\n\nFor what's worth, barring my lack of explaining, the `diffy` patch\nI've mentioned already does this: makes sure the generated hunks are\nunambiguous at diff generation, and unambiguous at the time of the\napplication of the patch. But definitely both are required.\n\nI guess, I call the hunks unambiguous at diff generation but it\nimplies they're only unambiguous on the original file, and because\npatching is also \"fixed\", patching can ensure the hunks cannot be\napplied in more than in 1 spot and I thus call them unambiguous at\npatching as well. But both are required (diffing+patching) to ensure\nthat indeed a hunk is truly unambiguous, therefore, you'd be right to\nsay that any static diff file even if it had the whole file as context\ncannot or shouldn't be called unambiguous, because that's only half of\nit; because applying that diff as patch(which is the second half of\nit) is necessary to see if it's unambiguous as it depends on the\ntarget that it's applied on. Thus even if the generated hunks(or\npatch) is unambiguous from the point of view of having generated it,\napplying it to a modified original file can cause the patching to fail\nif ambiguity is detected. Seems better to fail(either at diff\ngeneration, if context length is too high, or at patch application if\nambiguity is detected) than to silently succeed and hope that a\ncompilation will catch it, or worse a runtime behavior is eventually\nspotted to be wrong. Rust files appear to be far more prone to this\nambiguous diff generation due to rustfmt which formats them like seen\nabove with 1 brace per line, and having the possibility of consecutive\nsuch braces...\n\nThanks for your reply. Definitely need to think more about calling\nhunks unambiguous, without also specifying that they're truly only so\nonly when applied(which really makes them only as strong as\n_relatively_ unambiguous, since they depend on the target file to be\napplied to). Maybe call them semi-unambiguous, or unambiguous with\nrespect to original file, unclear yet. Perhaps whoever decides to make\na patch for this issue can call them differently, or just be aware of\nthis.\n\nCheers! Have a great day everyone!\n"},{"id":"498094","messageId":"CABPp-BGVdQZCr=0NzY9vpUJqaH+5yxJdpvfUqqhtWB4V=nkwDw@mail.gmail.com","threadId":"61728","inReplyTo":"CAFjaU5sAVaNHZ0amPXJcbSvsnaijo+3X5Otg_Mntkx2GbikZMA@mail.gmail.com","subject":"Re: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-07-04T20:07:52Z","receivedAt":"2024-07-04T20:08:04Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 3, 2024 at 8:25 AM Emanuel Czirai\n<correabuscar+gitML@gmail.com> wrote:\n>\n> Subject: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)\n\nYes, this is already known.  In fact, it was one of the big reasons we\nchanged the default backend in rebase from apply to merge.  From the\ngit-rebase manpage:\n\n```\n   Context\n       The apply backend works by creating a sequence of patches (by calling\n       format-patch internally), and then applying the patches in sequence\n       (calling am internally). Patches are composed of multiple hunks, each\n       with line numbers, a context region, and the actual changes. The line\n       numbers have to be taken with some fuzz, since the other side will\n       likely have inserted or deleted lines earlier in the file. The context\n       region is meant to help find how to adjust the line numbers in order to\n       apply the changes to the right lines. However, if multiple areas of the\n       code have the same surrounding lines of context, the wrong one can be\n       picked. There are real-world cases where this has caused commits to be\n       reapplied incorrectly with no conflicts reported. Setting diff.context\n       to a larger value may prevent such types of problems, but increases the\n       chance of spurious conflicts (since it will require more lines of\n       matching context to apply).\n\n       The merge backend works with a full copy of each relevant file,\n       insulating it from these types of problems.\n```\n\n> This doesn't affect `git rebase` as it's way more robust than simply\n> extracting the commits as patches and re-applying them. (I haven't looked\n> into `git merge` though, but I doubt it's affected)\n\nThis was not always true; and, in fact, rebase is actually still\npartially affected today -- if you pick the `apply` backend or pick\narguments that imply that backend, then you can still run into this\nproblem.  The merge backend (the default) is unaffected, and this\nproblem was one of the big reasons for us switching to make the merge\nbackend the default instead of the apply backend.\n\ngit merge is unaffected.\n"},{"id":"498099","messageId":"CAFjaU5vC--bGWBBPP=6YW43sXR6rDgLA7sTL4G81xVeBKsgFrg@mail.gmail.com","threadId":"61728","inReplyTo":"CABPp-BGVdQZCr=0NzY9vpUJqaH+5yxJdpvfUqqhtWB4V=nkwDw@mail.gmail.com","subject":"Re: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Emanuel Czirai","fromEmail":"correabuscar+gitml@gmail.com","sentAt":"2024-07-04T21:38:41Z","receivedAt":"2024-07-04T21:38:52Z","isPatch":false,"sender":{"key":"correabuscar+gitml@gmail.com","avatar":null},"body":"On Thu, Jul 4, 2024 at 10:08 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> On Wed, Jul 3, 2024 at 8:25 AM Emanuel Czirai\n> <correabuscar+gitML@gmail.com> wrote:\n> >\n> > Subject: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)\n>\n> Yes, this is already known.  In fact, it was one of the big reasons we\n> changed the default backend in rebase from apply to merge.  From the\n> git-rebase manpage:\n>\n> ```\n>    Context\n>        The apply backend works by creating a sequence of patches (by calling\n>        format-patch internally), and then applying the patches in sequence\n>        (calling am internally). Patches are composed of multiple hunks, each\n>        with line numbers, a context region, and the actual changes. The line\n>        numbers have to be taken with some fuzz, since the other side will\n>        likely have inserted or deleted lines earlier in the file. The context\n>        region is meant to help find how to adjust the line numbers in order to\n>        apply the changes to the right lines. However, if multiple areas of the\n>        code have the same surrounding lines of context, the wrong one can be\n>        picked. There are real-world cases where this has caused commits to be\n>        reapplied incorrectly with no conflicts reported. Setting diff.context\n>        to a larger value may prevent such types of problems, but increases the\n>        chance of spurious conflicts (since it will require more lines of\n>        matching context to apply).\n>\n>        The merge backend works with a full copy of each relevant file,\n>        insulating it from these types of problems.\n> ```\n>\n> > This doesn't affect `git rebase` as it's way more robust than simply\n> > extracting the commits as patches and re-applying them. (I haven't looked\n> > into `git merge` though, but I doubt it's affected)\n>\n> This was not always true; and, in fact, rebase is actually still\n> partially affected today -- if you pick the `apply` backend or pick\n> arguments that imply that backend, then you can still run into this\n> problem.  The merge backend (the default) is unaffected, and this\n> problem was one of the big reasons for us switching to make the merge\n> backend the default instead of the apply backend.\n>\n> git merge is unaffected.\n\nThank you very much, I really appreciate knowing this very interesting\ninfo! Cheers!\n\nThis also tells me that I shouldn't expect a solution for `git\ndiff`/`git apply` any time soon, else it would've been done at that\ntime, I suppose.\nIt's all good, I'll find some kind of workaround for my use case.\n(probably something based on `diffy` as it's simpler for me to grasp\nthan the C code)\n\nThanks everyone. All the best!\n"},{"id":"498135","messageId":"xmqq5xtj5b3s.fsf@gitster.g","threadId":"61728","inReplyTo":"CABPp-BGVdQZCr=0NzY9vpUJqaH+5yxJdpvfUqqhtWB4V=nkwDw@mail.gmail.com","subject":"Re: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-06T05:43:35Z","receivedAt":"2024-07-06T05:43:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Yes, this is already known.  In fact, it was one of the big reasons we\n> changed the default backend in rebase from apply to merge.  From the\n> git-rebase manpage:\n\nNot exactly on topic of the discussion, but I see a few things\nproblematic in this part, and as I already invested some time\nreading it, I'd leave #leftoverbits comment here.\n\n> ```\n>    Context\n>        The apply backend works by creating a sequence of patches (by calling\n>        format-patch internally), and then applying the patches in sequence\n>        (calling am internally). Patches are composed of multiple hunks, each\n\n`am` should be some marked-up to stand out.  It would be even better\nto spell it out as `git am`.\n\n>        with line numbers, a context region, and the actual changes. The line\n>        numbers have to be taken with some fuzz, since the other side will\n\n    \"fuzz\" -> \"offset\"\n\nIn the context of discussing patch application, `fuzz` is a term of\nart.  It is the number of context lines you (the patch applicator)\nallow the machinery to allow to be different between the patch and\nthe preimage.  Git allows *absolutely* no fuzz and there is not even\nan option to loosen this (this is philosophical design decision\noriginating back in Linus's days).\n\nThis part is talking about something different.  `offset` is another\nterm of art and refers to the difference between the beginning line\nnumber recorded in the hunk header, and the actual line in the preimage\nthe patch applies to.  Unless you are applying to the same preimage\nas where the patch was taken from, `offset` being non-zero is\nperfectly normal, but Git (and other patch applicators) try to\nminimize the offset.\n"}]}