diff mbox series

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

Message ID CAFjaU5sAVaNHZ0amPXJcbSvsnaijo+3X5Otg_Mntkx2GbikZMA@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch) | expand

Commit Message

Emanuel Czirai July 3, 2024, 3:24 p.m. UTC
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
         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.

Comments

Randall S. Becker July 3, 2024, 3:55 p.m. UTC | #1
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
Emanuel Attila Czirai July 3, 2024, 4:22 p.m. UTC | #2
> >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
>
Ah, definitely whoever writes the fix would do it in C for the git
code base, I didn't mean to imply it would be or should be done in
rust, therefore please excuse my failure to communicate that clearly.
The `diffy` proof-of-concept patch, is just for `diffy`, in rust, and
it's just to show a way this could be done and that "it works" that
way. It was easier for me to do it for `diffy` in rust, than in C for
git diff/apply or gnu diff/patch.
If a fix is to be implemented for `git diff/apply`, it would
definitely not be in rust by any means, but C, as you mentioned.
Thank you for your reply.

Also, I notice that I made a mistake when pasting the patch with the
context length of 4, it was a real patch not the one I used in the
examples, here's the corrected unambiguous patch:
```diff
--- original
+++ modified
@@ -1114,8 +1114,12 @@
                     self.config.shell().warn(msg)?
                 }
             }
         }
+        if seen_any_warnings {
+            //comment
+            bail!("reasons");
+        }
         Ok(())
     }

     pub fn set_target_dir(&mut self, target_dir: Filesystem) {
```

Cheers, have a great day everyone!
Johannes Sixt July 3, 2024, 9:01 p.m. UTC | #3
Am 03.07.24 um 17:24 schrieb Emanuel Czirai:
> 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!("reasons");
> +        }
>          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,

This assertion is wrong, assuming that the patch is to be applied to a
modified version of 'original'. There is nothing that can be done at the
time when a patch is generated to make it unambiguous, not even if the
entire file becomes context. The reason is that the modified 'original'
could now have the part duplicated that is the context in the patch, and
it would be possible to apply the patch any one of the duplicates. Which
one?

-- Hannes
Emanuel Czirai July 4, 2024, 7:15 a.m. UTC | #4
On Wed, Jul 3, 2024 at 11:01 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 03.07.24 um 17:24 schrieb Emanuel Czirai:
> > 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!("reasons");
> > +        }
> >          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,
>
> This assertion is wrong, assuming that the patch is to be applied to a
> modified version of 'original'. There is nothing that can be done at the
> time when a patch is generated to make it unambiguous, not even if the
> entire file becomes context. The reason is that the modified 'original'
> could now have the part duplicated that is the context in the patch, and
> it would be possible to apply the patch any one of the duplicates. Which
> one?
>
> -- Hannes
>

tl;dr: you're correct indeed, a hunk is truly unambiguous only when
`git apply` determines that, and it thus depends on the target file to
apply to; `git diff` can generate unambiguous hunks only with respect
to the unchanged original file, but applying that hunk to a changed
original file  only determines if a hunk is relatively unambiguous,
that is, unambiguous with respect to that changed original file. Both
diff-ing and patch application are needed to ensure unambiguity.

That is correct, which is why it's necessary to have `git apply` also
modified so that it tries to apply each hunk independently to the
now-modified original to see if it can be applied in more than 1
place, and if so, fail with the proper error message; in addition,
after applying it, it would try to apply it again, just in case
applying it created a new spot for it to be reapplied, and if it
succeeds to re-apply it, it would fail again. I kind of mentioned this
I believe, if not, my bad.
Both `git diff` and `git apply` would have to each ensure that the
hunks are unambiguous for the hunks to be considered unambiguous,
therefore you're correct in your statement that my stating that the
hunk is unambiguous right after generation via a fixed `git diff` is
wrong. It's merely unambiguous only in the context of applying it to
the original file, but as soon as a new modified original file is
presented, it can be ambiguous as it is, unless `git apply` can
determine that indeed it cannot be applied in more than 1 place in
this new modified original file, but both: applied independently(just
to be sure) and during the application of all previous hunks until
then, maybe even more than these 2, because the following hunks can
create new spots for it, so a reapply could make that hunk succeed
even if all others would fail. Not entirely sure here, how to properly
ensure this, that's why someone more knowledgeable might.

Ah, I see that I actually tried to say this(the above) after that
statement  but I used wrong wording "will fail" instead of "should
fail" or "would fail" (if fixed, that is):
> 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.

For what's worth, barring my lack of explaining, the `diffy` patch
I've mentioned already does this: makes sure the generated hunks are
unambiguous at diff generation, and unambiguous at the time of the
application of the patch. But definitely both are required.

I guess, I call the hunks unambiguous at diff generation but it
implies they're only unambiguous on the original file, and because
patching is also "fixed", patching can ensure the hunks cannot be
applied in more than in 1 spot and I thus call them unambiguous at
patching as well. But both are required (diffing+patching) to ensure
that indeed a hunk is truly unambiguous, therefore, you'd be right to
say that any static diff file even if it had the whole file as context
cannot or shouldn't be called unambiguous, because that's only half of
it; because applying that diff as patch(which is the second half of
it) is necessary to see if it's unambiguous as it depends on the
target that it's applied on. Thus even if the generated hunks(or
patch) is unambiguous from the point of view of having generated it,
applying it to a modified original file can cause the patching to fail
if ambiguity is detected. Seems better to fail(either at diff
generation, if context length is too high, or at patch application if
ambiguity is detected) than to silently succeed and hope that a
compilation will catch it, or worse a runtime behavior is eventually
spotted to be wrong. Rust files appear to be far more prone to this
ambiguous diff generation due to rustfmt which formats them like seen
above with 1 brace per line, and having the possibility of consecutive
such braces...

Thanks for your reply. Definitely need to think more about calling
hunks unambiguous, without also specifying that they're truly only so
only when applied(which really makes them only as strong as
_relatively_ unambiguous, since they depend on the target file to be
applied to). Maybe call them semi-unambiguous, or unambiguous with
respect to original file, unclear yet. Perhaps whoever decides to make
a patch for this issue can call them differently, or just be aware of
this.

Cheers! Have a great day everyone!
Elijah Newren July 4, 2024, 8:07 p.m. UTC | #5
On Wed, Jul 3, 2024 at 8:25 AM Emanuel Czirai
<correabuscar+gitML@gmail.com> wrote:
>
> Subject: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)

Yes, this is already known.  In fact, it was one of the big reasons we
changed the default backend in rebase from apply to merge.  From the
git-rebase manpage:

```
   Context
       The apply backend works by creating a sequence of patches (by calling
       format-patch internally), and then applying the patches in sequence
       (calling am internally). Patches are composed of multiple hunks, each
       with line numbers, a context region, and the actual changes. The line
       numbers have to be taken with some fuzz, since the other side will
       likely have inserted or deleted lines earlier in the file. The context
       region is meant to help find how to adjust the line numbers in order to
       apply the changes to the right lines. However, if multiple areas of the
       code have the same surrounding lines of context, the wrong one can be
       picked. There are real-world cases where this has caused commits to be
       reapplied incorrectly with no conflicts reported. Setting diff.context
       to a larger value may prevent such types of problems, but increases the
       chance of spurious conflicts (since it will require more lines of
       matching context to apply).

       The merge backend works with a full copy of each relevant file,
       insulating it from these types of problems.
```

> 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)

This was not always true; and, in fact, rebase is actually still
partially affected today -- if you pick the `apply` backend or pick
arguments that imply that backend, then you can still run into this
problem.  The merge backend (the default) is unaffected, and this
problem was one of the big reasons for us switching to make the merge
backend the default instead of the apply backend.

git merge is unaffected.
Emanuel Czirai July 4, 2024, 9:38 p.m. UTC | #6
On Thu, Jul 4, 2024 at 10:08 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Jul 3, 2024 at 8:25 AM Emanuel Czirai
> <correabuscar+gitML@gmail.com> wrote:
> >
> > Subject: `git diff`/`git apply` can generate/apply ambiguous hunks (ie. in the wrong place) (just like gnu diff/patch)
>
> Yes, this is already known.  In fact, it was one of the big reasons we
> changed the default backend in rebase from apply to merge.  From the
> git-rebase manpage:
>
> ```
>    Context
>        The apply backend works by creating a sequence of patches (by calling
>        format-patch internally), and then applying the patches in sequence
>        (calling am internally). Patches are composed of multiple hunks, each
>        with line numbers, a context region, and the actual changes. The line
>        numbers have to be taken with some fuzz, since the other side will
>        likely have inserted or deleted lines earlier in the file. The context
>        region is meant to help find how to adjust the line numbers in order to
>        apply the changes to the right lines. However, if multiple areas of the
>        code have the same surrounding lines of context, the wrong one can be
>        picked. There are real-world cases where this has caused commits to be
>        reapplied incorrectly with no conflicts reported. Setting diff.context
>        to a larger value may prevent such types of problems, but increases the
>        chance of spurious conflicts (since it will require more lines of
>        matching context to apply).
>
>        The merge backend works with a full copy of each relevant file,
>        insulating it from these types of problems.
> ```
>
> > 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)
>
> This was not always true; and, in fact, rebase is actually still
> partially affected today -- if you pick the `apply` backend or pick
> arguments that imply that backend, then you can still run into this
> problem.  The merge backend (the default) is unaffected, and this
> problem was one of the big reasons for us switching to make the merge
> backend the default instead of the apply backend.
>
> git merge is unaffected.

Thank you very much, I really appreciate knowing this very interesting
info! Cheers!

This also tells me that I shouldn't expect a solution for `git
diff`/`git apply` any time soon, else it would've been done at that
time, I suppose.
It's all good, I'll find some kind of workaround for my use case.
(probably something based on `diffy` as it's simpler for me to grasp
than the C code)

Thanks everyone. All the best!
Junio C Hamano July 6, 2024, 5:43 a.m. UTC | #7
Elijah Newren <newren@gmail.com> writes:

> Yes, this is already known.  In fact, it was one of the big reasons we
> changed the default backend in rebase from apply to merge.  From the
> git-rebase manpage:

Not exactly on topic of the discussion, but I see a few things
problematic in this part, and as I already invested some time
reading it, I'd leave #leftoverbits comment here.

> ```
>    Context
>        The apply backend works by creating a sequence of patches (by calling
>        format-patch internally), and then applying the patches in sequence
>        (calling am internally). Patches are composed of multiple hunks, each

`am` should be some marked-up to stand out.  It would be even better
to spell it out as `git am`.

>        with line numbers, a context region, and the actual changes. The line
>        numbers have to be taken with some fuzz, since the other side will

    "fuzz" -> "offset"

In the context of discussing patch application, `fuzz` is a term of
art.  It is the number of context lines you (the patch applicator)
allow the machinery to allow to be different between the patch and
the preimage.  Git allows *absolutely* no fuzz and there is not even
an option to loosen this (this is philosophical design decision
originating back in Linus's days).

This part is talking about something different.  `offset` is another
term of art and refers to the difference between the beginning line
number recorded in the hunk header, and the actual line in the preimage
the patch applies to.  Unless you are applying to the same preimage
as where the patch was taken from, `offset` being non-zero is
perfectly normal, but Git (and other patch applicators) try to
minimize the offset.
diff mbox series

Patch

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");
+        }