Fix #294 - Make "git apply" working in non-root-git dirs

[mvorisek]

Fixes #294

[mvorisek]

Hi @cweagans, can we merge this?

[cweagans]

I appreciate the work and research that you've put into this, but I'm probably not going to merge this. If it's not a git directory in the first place, there's not a good reason to use Git to apply patches to that directory. patch should be more than adequate. I'm not willing to complicate this plugin to make it do something it really doesn't need to do - it's already fragile enough.

[mvorisek]

Hi @cweagans, this fix is very important - it solves one very strong usecase - when patch (for ex. any standard WIndows) is not avaialble. Can you reconsider this? Thanks in advance!

Hi @samdark @Berdir, can you review this PR please as well?

[samdark]

@mvorisek can't help in this since I don't get the use case.

[mvorisek]

@mvorisek can't help in this since I don't get the use case.

@samdark Any system, where patch is not within PATH, but git is. On linux, patch is typically available, but on Windows it is not. Patchin with git is already supported, but if the main package does not use .git, the git patch command will fail as it requires to be run in root git directory. This PR solves this issue by faking the root directory and git patch then works like patch.

[samdark]

Ah, so it is backwards compatible platform compatibility fix?

[mvorisek]

It is forward and backward compatible. I spent quite a lot of time to address this issue properly.

In past the git patch issue (ie. requirement of the dir .git dir to be present) was addressed by fallback to patch command - see https://github.com/mvorisek/composer-patches/blame/2e6f72a2ad8d59cd7e2b729f218bf42adb14f590/src/Patches.php#L395-L396 . But this approach is more hotfix than a real solution as patch is typically not available on non-unix systems. Several issues for this were already opened.

With this PR, the git patch issue is fully addressed.

[samdark]

I see. Well, it makes sense as a fallback but it's @cweagans to decide.

[cweagans]

Why not just install ‘patch’? https://chocolatey.org/packages/patch

There’s only so far we can go to work around missing host dependencies, and this seems like overstepping that for a very rare thing.

In 2.x, patch application will be a capability, so you can extend it in a separate plugin. I’m still not convinced that this needs to happen at all though.

[mvorisek]

Why not just install ‘patch’? https://chocolatey.org/packages/patch

this tools should work in any enviroment where composer does, right?

Currently, any users that:
a) use Windows,
b) do not have patch in PATH,
b) and do not have .git dir in composer root
are affected.

This affected group is large, it was already reported here many times and we can fix this issue very robustly with a few lines of code without any side effect.

In code, the current behaviour is described as:

// In some rare cases, git will fail to apply a patch, fallback to using
// the 'patch' command.

very likely because of this issue.

I hope I explained the current situation well.

Now, back to your question - if I distribute my app patched using this package, is convincing users to install any executable really a better solution than a few lines of fixes in plugin that can work of the box?

@cweagans, please, review this PR. It would really help me and I hope others as well. Thank you.

[cweagans]

this tools should work in any enviroment where composer does, right?

Yes, as long as the system dependencies are satisfied. You didn't answer my question though. Why can't/won't you install patch for Windows?

without any side effect.

We don't have any way of verifying this assertion in 1.x. I've made this assumption before and ended up causing myself a lot of work to get the plugin working again quickly because I broke a workflow for somebody. I'm not thrilled about the idea of making this assumption for something that can be easily worked around by installing a common tool on your workstation.

Now, back to your question - if I distribute my app patched using this package, is convincing users to install any executable really a better solution than a few lines of fixes in plugin that can work of the box?

I don't think it's unreasonable to expect that a developer has (or can have) both git and patch installed. This plugin has been happily operating under that assumption since 2014.

Again, what you want to do will be possible by extending this plugin in 2.x, but I just haven't had time to get it done.

[samdark]

@cweagans you have to know that you need to install it. As far as I understand the issue, it currently fails silently without giving any clue.

[cweagans]

Okay. That is something that I could see fixing in 1.x.

[mvorisek]

Okay. That is something that I could see fixing in 1.x.

@cweagans Can you please merge then?

I hit this issue again and the patch is highly welcomed also for linux - Alpine has no patch command installed by default, thus support with git is highly appreciated.

[andypost]

@mvorisek in alpinelinux you need anyway to run apk add patch or install git because busybox's one is unable to apply binary patches

[cweagans]

@mvorisek My apologies. I may not have communicated clearly enough. I'm willing to merge a PR that gives an explicit error message about patch not being installed. I'm not interested in merging this particular PR.

[andypost]

Moreover it's just 100KB vs 20MB of git

[mvorisek]

@cweagans thanks for info - this PR solves however the git apply issue completely with very low LOC it fixes support for two major usecases:

• patch not available, very common on Windows
• patch not avaiable, very common on lite linux distros like Alpine

I am not sure if I explained the issues well, but this PR:

• does not drop support of patch in any way
• it does not even add git patch support. It only fixes it, so patch can be applied by git when the project is not withing a git repo

Please reconsider this fix once again.

[cweagans]

@mvorisek I did take the time to thoroughly understand the PR. The problem is not a lack of understanding. It's that I disagree with the premise. patch is available for every platform that I can think of. The fact that it's not installed isn't a problem that this plugin needs to solve.

In 2.x (if I can ever get it finished and out the door), you'll be able to write a separate plugin that extends this one. Via Composer Capabilities, you'll be able to define whatever method you like for applying patches (including creating a fake .git dir and using git apply if that's what you want to do for whatever reason).

In either case though, I'm really not willing to bring this into the core plugin. The solution is to simply install patch and call it a day.