Use patch instead of git apply

[danepowell]

Following up on the conversation here: #165

Currently, composer-patches tries to use git apply to apply patches, and falls back to GNU patch if that fails. I think we should reverse this logic, or eliminate git apply altogether.

The reason being that you can't run git apply inside of a git repo that's a parent of the project getting patched. This probably describes 90% of the environments where composer patches is used, so git apply would almost never work.

What I mean by this: let's say you have a project foo that depends on package bar. You obviously keep foo in git, so your directory structure looks like this:

foo/.git
foo/composer.json
foo/vendor/bar/bar

And now you want to apply a patch to bar. You can't do this with git apply, because foo/ is the git repo root, but git apply expects foo/vendor/bar/bar to be the repo root.

@cweagans mentioned in https://github.com/cweagans/composer-patches/pull/165#issuecomment-343317202:

git is the default because pretty much everyone has that installed. Fewer people have patch installed

I'd be surprised if that's true. I've never run across a *nix environment that didn't have patch installed. The only place I can imagine this being true is for Windows users running outside of the Ubuntu subsystem (which is not a recipe for success to begin with).

[cweagans]

-1 for these reasons (in no particular order):

1. This is in the readme, and it alleviates part of the issue:

"config": {
    "preferred-install": "source"
  },

2. git apply typically is more tolerant of fuzz (i.e. patch application is successful more often with git apply than patch).

3. The longer term goal here is to use the VCS-appropriate patcher (svn apply, hg import, etc), and then fall back to lesser methods if necessary (patch, a pure PHP patcher, etc).

4. patch is ubiquitous, but not consistent across environments. For instance, https://github.com/cweagans/composer-patches/issues/159 shows an issue where the actual command line flags for patch are different on FreeBSD.

5. The linked stackoverflow comment describes a behavior from 2014. When I wrote the first version of this plugin, applying patches to nested git repos was the only thing it was used for. I'm inclined to think that things have changed a bit since then, but if not, this sounds like a good opportunity to engage with the git community to fix a bug.

[ptmkenny]

I'd like to second the original request by @danepowell. I just got bitten by the bug caused by the silent failure of git apply in #174.

Also, cloning everything from source may allow git apply to work, but it also significantly slows down the site install to clone everything. Even if the logic is kept the same, it would be nice to introduce an additional option for "using patch instead of git apply."

[rudiedirkx]

git apply fails silently for me too, which makes this package completely useless, but very sneakily.

"preferred-install": "source" might fix it, but that changes how composer works, not how this package works. Another package might need "preferred-install": "dist". That's not a solution.

Is there a workaround that doesn't affect all the rest?

it would be nice to introduce an additional option for "using patch instead of git apply"

I'm all for that.

[rudiedirkx]

Ah crap, I missed 1.6.4. That seems to work. Sorry!

[gapple]

I'm currently running into an issue with patching, where patches created with git diff include renames which are not understood by patch. Any file modifications in the patch are applied but renames are silently ignored.

e.g.

diff --git a/core/modules/media/config/install/core.entity_form_display.media.file.default.yml b/core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml
similarity index 100%
rename from core/modules/media/config/install/core.entity_form_display.media.file.default.yml
rename to core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml

From the git format-patch docs:

A renaming patch reduces the amount of text output, and generally makes it easier to review. Note that non-Git "patch" programs won’t understand renaming patches, so use it only when you know the recipient uses Git to apply your patch.

[danepowell]

Does git diff have an option to disable renames? I would argue that if people are generating patches for public consumption, they should not be using a method that is so implementation-specific and precludes users of non-Git "patch" programs (I'm assuming this is a reference to GNU Patch, which predates Git by several decades, so I'm not sure why the Git maintainers felt the need to use scare quotes there)

[gapple]

git format-patch and git diff have the --no-renames option, which sounds like it would produce patch-compatible output.

The patch that I noticed was having the issue was https://www.drupal.org/node/2883813, being applied by acquia/lightning 2.2.7.

The Drupal.org documentation on creating patches only recommends git diff > [patch-name.patch].

[danepowell]

The git docs aren't 100% clear on how renames work, and specifically the --no-renames argument:

Turn off rename detection, even when the configuration file gives the default to do so.

I interpret that as meaning that by default, git diff and git format-patch do not output renames in this incompatible way. It sounds like that would only happen if you passed an explicit flag (-M) or have renames output automatically via a config file.

If that's true, then the patch you found is a fluke and should be corrected.

I will also update the d.o patch docs to call this out.

[mortenson]

Some patches created with Git contain binary diffs, generated using git diff --binary. As far as I know, these kind of patches can only be applied using git apply. The patch command does not support binary files.

[Apteryks]

I'm currently running into an issue with patching, where patches created with git diff include renames which are not understood by patch.

You need GNU Patch >= 2.7. From their News page (https://savannah.gnu.org/forum/forum.php?forum_id=7361) about 2.7:

Support for most features of the "diff --git" format, including renames and copies, permission changes, and symlink diffs. Binary diffs are not supported yet; patch will complain and skip them.

[lawxen]

@Apteryks I spend several hours to find what's going wrong, thanks to you advice, My mac's patch version is 2.5, After I update patch to 2.7.6, every thing works fine with rename.

[navneet0693]

@Apteryks I spend several hours to find what's going wrong, thanks to you advice, My mac's patch version is 2.5, After I update patch to 2.7.6, every thing works fine with rename.

@lawxen I tried searching for the ways to install patch 2.7.6 on macbook, I couldn't find, could please help me out on this?

[cweagans]

@navneet0693 brew install gpatch (you'll need Homebrew installed for that to work (https://brew.sh))

[wimleers]

@cweagans holy shit, the need for brew install gpatch took me many hours to figure out, because for a particular project TravisCI builds were succeeding but local builds were failing. 😬 Can we make cweagans/composer-patches detect the patch version, and explicitly warn the user that any version below 2.7 may result in silent failures?

[srikant-dubey]

@navneet0693 brew install gpatch (you'll need Homebrew installed for that to work (https://brew.sh))

It works like a charm

[soullivaneuh]

I 100% agree with @danepowell, patch is a much more generic way for appliance.

Plus, it would allow us patch appliance for dist installation method, which is great.

If git apply is still needed for some reasons, maybe the method can be chosen as an option?

[mikemadison13]

@navneet0693 brew install gpatch (you'll need Homebrew installed for that to work (https://brew.sh))

this worked for me!

[cweagans]

main now gives you a ton of flexibility with which patchers are enabled. If you don't want the git patcher, you can simply disable it. You can also define your own patchers if you want to...idk...write some cursed shell script that applies patches using awk and sed or something.