RE: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)

[<rsbecker@nexbridge.com>]

On Wednesday, July 3, 2024 11:25 AM, Emanuel Czirai wrote:
>This doesn't affect `git rebase` as it's way more robust than simply extracting the
>commits as patches and re-applying them. (I haven't looked into `git merge` though,
>but I doubt it's affected)
>
>It seems to me that no matter which algorithm `git diff` uses  (of the 4), it generates
>the same hunk, because it's really the context length that matters (which by default
>is 3), which is the same one that gnu `diff` generates, and its application via `git
>apply` is the same as gnu `patch`.
>
>side note:
>`diffy` is a simple rust-written library that behaves (at version 0.4.0) the same as
>normal diff and patch apply(with regards to generated diff contents and patch
>application; except it doesn't set the original/modified filenames in the patch), and
>since my limited experience, I've found it simpler to modify it to make it so that it
>generates unambiguous hunks, as a proof of concept that it can be done this way,
>here:
>https://github.com/bmwill/diffy/issues/31 , whereby increasing the context
>length(lines) of the whole patch(though ideally only of the affected hunks) the
>initially ambiguous hunk(s) cannot be applied anymore in more than 1 place inside
>the original file, thus avoiding both the diff creation and the patch application from
>generating and applying ambiguous hunks.
>
>But forgetting about that for a moment, I'm gonna show you about `git diff` and `git
>apply` below:
>1. clone cargo's repo:
>cd /tmp
>git clone https://github.com/rust-lang/cargo.git
>cd cargo
>2. checkout tag 0.76.0, or branch origin/rust-1.75.0 git checkout 0.76.0 3. manually
>edit this file ./src/cargo/core/workspace.rs at line 1118 or manually go to function:
>pub fn emit_warnings(&self) -> CargoResult<()> { right before that function's end
>which looks like:
>Ok(())
>}
>so there at line 1118, insert above that Ok(()) something, doesn't matter what,
>doesn't have to make sense, like:
>if seen_any_warnings {
>  //comment
>  bail!("reasons");
>}
>save the file
>4. try to generate a diff from it:
>git diff > /tmp/foo.patch
>you get this:
>```diff
>diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index
>4667c8029..8f0a74473 100644
>--- a/src/cargo/core/workspace.rs
>+++ b/src/cargo/core/workspace.rs
>@@ -1115,6 +1115,10 @@ impl<'cfg> Workspace<'cfg> {
>                 }
>             }
>         }
>+        if seen_any_warnings {
>+            //comment
>+            bail!("reasons");
>+        }
>         Ok(())
>     }
>
>```
>Now this is an ambiguous patch/hunk because if you try to apply this patch on the
>same original file, cumulatively, it applies successfully in 3 different places, but we
>won't do that now.
>5. now let's discard it(we already have it saved in /tmp/foo.patch) and pretend that
>something changed in the original code:
>git checkout .
>git checkout 0.80.0
>6. reapply that patch on the new changes:
>git apply /tmp/foo.patch
>(this shows no errors)
>7. look at the diff, for no good reason, just to see that it's the same(kind of):
>git diff > /tmp/foo2.patch
>contents:
>```diff
>diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index
>55bfb7a10..92709f224 100644
>--- a/src/cargo/core/workspace.rs
>+++ b/src/cargo/core/workspace.rs
>@@ -1099,6 +1099,10 @@ impl<'gctx> Workspace<'gctx> {
>                 }
>             }
>         }
>+        if seen_any_warnings {
>+            //comment
>+            bail!("reasons");
>+        }
>         Ok(())
>     }
>
>```
>8. now look at the file, where was the patch applied? that's right, it's at the end of
>the wrong function:
>fn validate_manifest(&mut self) -> CargoResult<()> { vim
>src/cargo/core/workspace.rs +1040 jump at the end of it at line 1107, you see it's
>our patch there, applied in the wrong spot!
>
>Hopefully, depending on the change, this kind of patch which applied in the wrong
>place, will be caught at (rust)compile time (in my case, it was this) or worse, at
>(binary)runtime.
>
>With the aforementioned `diffy` patch, the generated diff would actually be with a
>context of 4, to make it unambiguous, so it would've been this:
>```diff
>--- original
>+++ modified
>@@ -1186,8 +1186,12 @@
>                     self.gctx.shell().warn(msg)?
>                 }
>             }
>         }
>+        if seen_any_warnings {
>+            //use anyhow::bail;
>+            bail!("Compilation failed due to cargo warnings! Manually
>+ done
>this(via cargo patch) so that things like the following (ie. dep key packages= and
>using rust pre 1.26.0 which ignores it, downloads squatted
>package) will be avoided in the future:
>https://github.com/rust-lang/rust/security/advisories/GHSA-phjm-8x66-qw4r");
>+        }
>         Ok(())
>     }
>
>     pub fn emit_lints(&self, pkg: &Package, path: &Path) -> CargoResult<()> { ``` this
>hunk is now unambiguous because it cannot be applied in more than 1 place in the
>original file, furthermore patching a modified future file will fail(with that `diffy`
>patch, and ideally, with `git apply` if any changes are implemented to "fix" this issue)
>if any of the hunks can be independently(and during the full patch application too)
>applied in more than 1 place.
>
>I consider that I don't know enough to understand how `git diff`/`git apply` works
>internally (and similarly, gnu `diff`/`patch`) to actually change them and make them
>generate unambiguous hunks where only the hunks that would've been ambiguous
>have increased context size, instead of the whole patch have increased context size
>for all hunks(which is what I did for `diffy` too so far, in that proof of concept patch),
>therefore if a "fix" is deemed necessary(it may not be, as I might've missed
>something and I'm unaware of it, so a fix may be messing other things up, who
>knows?!) then I hope someone much more knowledgeable could implement
>it(maybe even for gnu diff/patch too), and while I don't think that a "please" would
>be enough, I'm still gonna say it: please do so, if so inclined.
>
>Thank you for your time and consideration.

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.
--Randall