fetchpatch: ignores file renames

[domenkozar]

If the patch renames a file, and contains:

rename from libraries/base/GHC/Event/Clock.hsc
rename to libraries/base/GHC/Clock.hsc

Then using patch -p1 < foobar.patch works as expected, while using fetchpatch no rename happens.

cc @vcunat @aszlig @copumpkin @fpletz @globin

[domenkozar]

Proof that patch consideres that input: https://git.savannah.gnu.org/cgit/patch.git/commit/?h=v2.7&id=9e39d9c9950076e319d1dbbeeb7b8bd268b465f3

[nh2]

Example:

diff --git a/libraries/base/GHC/Event/Clock.hsc b/libraries/base/GHC/Clock.hsc
similarity index 53%
rename from libraries/base/GHC/Event/Clock.hsc
rename to libraries/base/GHC/Clock.hsc
index 5dbdb674d31..6339dc0a52b 100644
--- a/libraries/base/GHC/Event/Clock.hsc
+++ b/libraries/base/GHC/Clock.hsc
@@ -1,17 +1,25 @@
 {-# LANGUAGE Trustworthy #-}
 {-# LANGUAGE NoImplicitPrelude #-}
...

If in this patch, you strip everything before the ---, then patch will not execute the rename.

In other words, it interprets the

rename from libraries/base/GHC/Event/Clock.hsc
rename to libraries/base/GHC/Clock.hsc

and potentially other stuff; fetchpatch stripping that off breaks it.

(I personally also thought that everything before --- was a comment and would be ignored for the purpose of applying the patch; turns out this is wrong.)

[dezgeg]

(I personally also thought that everything before --- was a comment and would be ignored for the purpose of applying the patch; turns out this is wrong.)

The rename lines (like index ...) are a Git extension to the patch format.

[aszlig]

This should already be fixed upstream (twaugh/patchutils@14261ad), we just need to bump patchutils.

[dezgeg]

Last time that was attempted, those new git-specific lines caused most existing fetchpatch hashes to change.

[aszlig]

@dezgeg: Okay, which is probably because of that very commit, because it left in other git-specific extensions. Regardless of that, I just checked to verify whether this is problem persists in 0.3.4 and even in master and it's still the case, because lsdiff is simply dropping renames because they're essentially non-existing hunks.

[aszlig]

Okay, while trying to patch patchutils it turns out that fixing the renames is a bit more involved, because the way patchutils does its parsing it's not very easy to do lookahead. Another obstacle is what @dezgeg mentioned with the changed hashes.

So what we practically need is just to turn the patch into a form that is deterministic in its results, which patchutils only does to a certain extent, for example it doesn't sort the file names.

Maybe it makes sense to implement what we need for fetchpatch ourselves and not use patchutils?

[dezgeg]

Yeah, I suppose writing a custom implementation is the best. After all, I think all it is doing now is stripping out any comments from the .patch files.

[globin]

We also use it to exclude files from patches and add prefixes/suffixes(?)

[aszlig]

@globin: Sure, that's no problem once we have a parser that uses proper data structures for the patches. On the other hand, however I was thinking about writing such an implementation in Haskell and parser combinators, but that would increase the closure size of fetchpatch a lot and also probably won't work that well because we probably have fetchpatch in the dependency graph of GHC (haven't checked).

So I'd like to amend my comment in a way that it's still probably better to patch patchutils to use proper data structures instead of that painful getline-based state-all-over-the-place parser that it's currently using and also add options to it for our main use case, which is bringing the patch into a form which deterministic in its output.

edit: Rewriting the parser of patchutils also has the advantage that the project already has a comprehensive test suite, which should make it a bit easier to implement.

[orivej]

I consider updating patchutils and automatically recomputing all fetchpatch hashes.

[aszlig]

@orivej: Just updating patchutils won't be enough, as mentioned previously because it won't handle file renames.

[stale]

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
2. Ask on the NixOS Discourse.
3. Ask on the #nixos channel on irc.freenode.net.

[arapov]

I hit this issue today. It would be nice to document this behaviour somehow .... I replaced fetchpatch() with fetchurl() and it did the trick for me.

[jtojnar]

@arapov Using fetchurl for dynamically generated patches (e.g. those from GitHub) is not safe since the file can change in the future. You might be better just inlining the patch into the source tree (or re-adding the renames in fetchpatch’s postFetch attribute).

[arapov]

@arapov Using fetchurl for dynamically generated patches (e.g. those from GitHub) is not safe since the file can change in the future. You might be better just inlining the patch into the source tree (or re-adding the renames in fetchpatch’s postFetch attribute).

@jtojnar, why fetchpatch is not fixed the way it respects moves/renames?

[jtojnar]

@arapov We would need a fetchpatch2 in order not to break the old hashes. And we would also need to change patchutils to support renames, 0.3.4 does not support it according to #32084 (comment), even though it is supposed to preserve some git metadata. Looking at the changelogs of the newer version they did not add that either.

Edit: There is a long-standing issue open twaugh/patchutils#22, no-one probably found time to implement it yet. Nor implement a custom, more fitting tool as proposed above.

[stale]

I marked this as stale due to inactivity. → More info

[bobby285271]

Still important (I think I just hit this issue, #140429 (comment)).

[eyJhb]

Also just ran into this, while applying a patch from Github using applyPatches. :)

[eyJhb]

Still important

[Artturin]

#182250
1st path is by fetchpatch and 2nd is by fetchpatch2

$ diff /nix/store/namp0v9k05pjcax761h7nacpvh77205b-abc52460201bc5c7603505bb187138b0c59291aa.diff /nix/store/d2vkicv4b3xghlmaw4bnwnp01zhk7x6f-abc52460201bc5c7603505bb187138b0c59291aa.diff
0a1,2
> diff --git a/configure b/configure
> index b6c9b462f24..a86f2ceaa5b 100755
18a21,22
> diff --git a/mkspecs/common/macx.conf b/mkspecs/common/macx.conf
> index d16b77acb8e..4ba0a8eaa36 100644
28a33,37
> diff --git a/mkspecs/macx-clang/qmake.conf b/mkspecs/common/clang-macx-desktop.conf
> similarity index 83%
> rename from mkspecs/macx-clang/qmake.conf
> rename to mkspecs/common/clang-macx-desktop.conf
> index 0cf1f31b60d..042319a2aa3 100644

[Luflosi]

Is fetchpatch2 supposed to fix this issue? I'm trying to apply teeworlds/teeworlds@86d6687, which only renames files and nothing else and I get the error message error: Normalized patch '/build/patch' is empty (while the fetched file was not)!.

[Artturin]

Is fetchpatch2 supposed to fix this issue?

Yep

I'm trying to apply teeworlds/teeworlds@86d6687, which only renames files and nothing else and I get the error message error: Normalized patch '/build/patch' is empty (while the fetched file was not)!.

If youre already using it then experiment with https://github.com/nixos/nixpkgs/blob/41877098f3ac00295a2a9d5e5846201b969a689b/pkgs/build-support/fetchpatch/default.nix#L39 and see if you can get it working

If not, then it's a upstream issue

[Luflosi]

I executed

nix-shell -p patchutils_0_4_2

then inside the shell:

wget 'https://github.com/teeworlds/teeworlds/commit/86d6687ef07f7f05457a7f67537b650656b13820.patch'
lsdiff e03553605b45c88f0b4b2980adfbbb8f6fca2fd6.patch

lsdiff outputs nothing, so this looks like an upstream issue, right? Did I invoke it correctly?

[Luflosi]

For reference, #208826 is where I wanted to use this.

[wegank]

#208918 also has fetchpatch2 failing on the first patch.

[vcunat]

fetchpatch2 still drops the rename part of https://gitlab.gnome.org/GNOME/NetworkManager-fortisslvpn/-/commit/084ef529c5fb816927ca54866f66b340265aa9f6.diff