New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #148: Incompatible with Git 2.14+. #165

Merged
merged 2 commits into from Nov 22, 2017

Conversation

Projects
None yet
6 participants
@danepowell
Contributor

danepowell commented Nov 8, 2017

No description provided.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2017

Coverage Status

Coverage increased (+0.05%) to 2.492% when pulling d759a72 on danepowell:issue-148 into 4b1de2a on cweagans:master.

coveralls commented Nov 8, 2017

Coverage Status

Coverage increased (+0.05%) to 2.492% when pulling d759a72 on danepowell:issue-148 into 4b1de2a on cweagans:master.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Nov 8, 2017

Contributor

Ironically, while this does solve the errors thrown by Git 2.14, it actually triggers another bug, and patches don't get applied correctly.

Back when git apply was failing, composer-patches would fall back to using the patch command, i.e.:
patch '-p2' --no-backup-if-mismatch -d 'docroot/core' < '/tmp/5a0340574777b.patch'

This worked great. But for some reason, now that it's using git apply, it fails silently.

Contributor

danepowell commented Nov 8, 2017

Ironically, while this does solve the errors thrown by Git 2.14, it actually triggers another bug, and patches don't get applied correctly.

Back when git apply was failing, composer-patches would fall back to using the patch command, i.e.:
patch '-p2' --no-backup-if-mismatch -d 'docroot/core' < '/tmp/5a0340574777b.patch'

This worked great. But for some reason, now that it's using git apply, it fails silently.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Nov 8, 2017

Contributor

Debugging with this command:
set -x; GIT_TRACE=2 GIT_CURL_VERBOSE=2 GIT_TRACE_PERFORMANCE=2 GIT_TRACE_PACK_ACCESS=2 GIT_TRACE_PACKET=2 GIT_TRACE_PACKFILE=2 GIT_TRACE_SETUP=2 GIT_TRACE_SHALLOW=2 git -C 'docroot/core' apply '-p2' /home/dane/sites/blted89/2883813-27.patch -v -v; set +x

I see this:

+ GIT_TRACE=2
+ GIT_CURL_VERBOSE=2
+ GIT_TRACE_PERFORMANCE=2
+ GIT_TRACE_PACK_ACCESS=2
+ GIT_TRACE_PACKET=2
+ GIT_TRACE_PACKFILE=2
+ GIT_TRACE_SETUP=2
+ GIT_TRACE_SHALLOW=2
+ git -C docroot/core apply -p2 /home/dane/sites/blted89/2883813-27.patch -v -v
09:58:15.310932 trace.c:333             setup: git_dir: .git
09:58:15.310973 trace.c:334             setup: git_common_dir: .git
09:58:15.310979 trace.c:335             setup: worktree: /home/dane/sites/blted89
09:58:15.310984 trace.c:336             setup: cwd: /home/dane/sites/blted89
09:58:15.310988 trace.c:337             setup: prefix: docroot/core/
09:58:15.310993 git.c:344               trace: built-in: git 'apply' '-p2' '/home/dane/sites/blted89/2883813-27.patch' '-v' '-v'
Skipped patch 'modules/hal/tests/src/Functional/EntityResource/Media/MediaHalJsonAnonTest.php'.
Skipped patch 'modules/media/tests/modules/media_test_type/media_test_type.info.yml'.
Skipped patch 'modules/media/tests/src/Functional/MediaFunctionalTestCreateMediaTypeTrait.php'.
Skipped patch 'modules/media/tests/src/Functional/MediaRevisionTest.php'.
Skipped patch 'modules/media/tests/src/Functional/MediaSourceFileTest.php'.
Skipped patch 'modules/media/tests/src/Functional/MediaUiFunctionalTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaViewsWizardTest.php'.
Skipped patch 'modules/media/tests/src/Kernel/MediaSourceFileTest.php'.
Skipped patch 'modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php'.
Skipped patch 'modules/media/config/install/core.entity_form_display.media.file.default.yml => profiles/standard/config/optional/core.entity_form_display.media.file.default.yml'.
Skipped patch 'modules/media/config/install/core.entity_form_display.media.image.default.yml => profiles/standard/config/optional/core.entity_form_display.media.image.default.yml'.
Skipped patch 'modules/media/config/install/core.entity_view_display.media.file.default.yml => profiles/standard/config/optional/core.entity_view_display.media.file.default.yml'.
Skipped patch 'modules/media/config/install/core.entity_view_display.media.image.default.yml => profiles/standard/config/optional/core.entity_view_display.media.image.default.yml'.
Skipped patch 'modules/media/config/install/field.field.media.file.field_media_file.yml => profiles/standard/config/optional/field.field.media.file.field_media_file.yml'.
Skipped patch 'modules/media/config/install/field.field.media.image.field_media_image.yml => profiles/standard/config/optional/field.field.media.image.field_media_image.yml'.
Skipped patch 'modules/media/config/install/field.storage.media.field_media_file.yml => profiles/standard/config/optional/field.storage.media.field_media_file.yml'.
Skipped patch 'modules/media/config/install/field.storage.media.field_media_image.yml => profiles/standard/config/optional/field.storage.media.field_media_image.yml'.
Skipped patch 'modules/media/config/install/media.type.file.yml => profiles/standard/config/optional/media.type.file.yml'.
Skipped patch 'modules/media/config/install/media.type.image.yml => profiles/standard/config/optional/media.type.image.yml'.
09:58:15.311544 trace.c:435             performance: 0.000773097 s: git command: 'git' '-C' 'docroot/core' 'apply' '-p2' '/home/dane/sites/blted89/2883813-27.patch' '-v' '-v'
+ set +x

Git is silently skipping those file renames. No idea why. I've tried that command with different patch levels (p0, p1...) as well, none of them work.

Could it be because docroot/core was checked out as a distributed package, rather than from source? So it's not a git repo, and git apply just ignores it.

Contributor

danepowell commented Nov 8, 2017

Debugging with this command:
set -x; GIT_TRACE=2 GIT_CURL_VERBOSE=2 GIT_TRACE_PERFORMANCE=2 GIT_TRACE_PACK_ACCESS=2 GIT_TRACE_PACKET=2 GIT_TRACE_PACKFILE=2 GIT_TRACE_SETUP=2 GIT_TRACE_SHALLOW=2 git -C 'docroot/core' apply '-p2' /home/dane/sites/blted89/2883813-27.patch -v -v; set +x

I see this:

+ GIT_TRACE=2
+ GIT_CURL_VERBOSE=2
+ GIT_TRACE_PERFORMANCE=2
+ GIT_TRACE_PACK_ACCESS=2
+ GIT_TRACE_PACKET=2
+ GIT_TRACE_PACKFILE=2
+ GIT_TRACE_SETUP=2
+ GIT_TRACE_SHALLOW=2
+ git -C docroot/core apply -p2 /home/dane/sites/blted89/2883813-27.patch -v -v
09:58:15.310932 trace.c:333             setup: git_dir: .git
09:58:15.310973 trace.c:334             setup: git_common_dir: .git
09:58:15.310979 trace.c:335             setup: worktree: /home/dane/sites/blted89
09:58:15.310984 trace.c:336             setup: cwd: /home/dane/sites/blted89
09:58:15.310988 trace.c:337             setup: prefix: docroot/core/
09:58:15.310993 git.c:344               trace: built-in: git 'apply' '-p2' '/home/dane/sites/blted89/2883813-27.patch' '-v' '-v'
Skipped patch 'modules/hal/tests/src/Functional/EntityResource/Media/MediaHalJsonAnonTest.php'.
Skipped patch 'modules/media/tests/modules/media_test_type/media_test_type.info.yml'.
Skipped patch 'modules/media/tests/src/Functional/MediaFunctionalTestCreateMediaTypeTrait.php'.
Skipped patch 'modules/media/tests/src/Functional/MediaRevisionTest.php'.
Skipped patch 'modules/media/tests/src/Functional/MediaSourceFileTest.php'.
Skipped patch 'modules/media/tests/src/Functional/MediaUiFunctionalTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php'.
Skipped patch 'modules/media/tests/src/FunctionalJavascript/MediaViewsWizardTest.php'.
Skipped patch 'modules/media/tests/src/Kernel/MediaSourceFileTest.php'.
Skipped patch 'modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php'.
Skipped patch 'modules/media/config/install/core.entity_form_display.media.file.default.yml => profiles/standard/config/optional/core.entity_form_display.media.file.default.yml'.
Skipped patch 'modules/media/config/install/core.entity_form_display.media.image.default.yml => profiles/standard/config/optional/core.entity_form_display.media.image.default.yml'.
Skipped patch 'modules/media/config/install/core.entity_view_display.media.file.default.yml => profiles/standard/config/optional/core.entity_view_display.media.file.default.yml'.
Skipped patch 'modules/media/config/install/core.entity_view_display.media.image.default.yml => profiles/standard/config/optional/core.entity_view_display.media.image.default.yml'.
Skipped patch 'modules/media/config/install/field.field.media.file.field_media_file.yml => profiles/standard/config/optional/field.field.media.file.field_media_file.yml'.
Skipped patch 'modules/media/config/install/field.field.media.image.field_media_image.yml => profiles/standard/config/optional/field.field.media.image.field_media_image.yml'.
Skipped patch 'modules/media/config/install/field.storage.media.field_media_file.yml => profiles/standard/config/optional/field.storage.media.field_media_file.yml'.
Skipped patch 'modules/media/config/install/field.storage.media.field_media_image.yml => profiles/standard/config/optional/field.storage.media.field_media_image.yml'.
Skipped patch 'modules/media/config/install/media.type.file.yml => profiles/standard/config/optional/media.type.file.yml'.
Skipped patch 'modules/media/config/install/media.type.image.yml => profiles/standard/config/optional/media.type.image.yml'.
09:58:15.311544 trace.c:435             performance: 0.000773097 s: git command: 'git' '-C' 'docroot/core' 'apply' '-p2' '/home/dane/sites/blted89/2883813-27.patch' '-v' '-v'
+ set +x

Git is silently skipping those file renames. No idea why. I've tried that command with different patch levels (p0, p1...) as well, none of them work.

Could it be because docroot/core was checked out as a distributed package, rather than from source? So it's not a git repo, and git apply just ignores it.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Nov 8, 2017

Contributor

This is the root cause: https://stackoverflow.com/a/27283285

tl;dr: you can't run git apply within a git repo that's not the project being patched

@cweagans how do you want to address this? It seems like we need to stop using git apply altogether, I don't know how this could ever work (the vast majority of projects are going to be a Git repo)

Contributor

danepowell commented Nov 8, 2017

This is the root cause: https://stackoverflow.com/a/27283285

tl;dr: you can't run git apply within a git repo that's not the project being patched

@cweagans how do you want to address this? It seems like we need to stop using git apply altogether, I don't know how this could ever work (the vast majority of projects are going to be a Git repo)

@danepowell danepowell referenced this pull request Nov 8, 2017

Closed

Breaks with Git 2.14.0 #148

@cweagans

This comment has been minimized.

Show comment
Hide comment
@cweagans

cweagans Nov 9, 2017

Owner

Could it be because docroot/core was checked out as a distributed package, rather than from source? So it's not a git repo, and git apply just ignores it.

I think git apply doesn't work on non git repos, which is why we have the patch fallback.

I think we should probably solve one thing at a time. If this change resolves things for the new version of Git (ideally without breaking earlier versions), we should move forward here, and then address the other issue (patches sporadically not getting applied) separately.

Owner

cweagans commented Nov 9, 2017

Could it be because docroot/core was checked out as a distributed package, rather than from source? So it's not a git repo, and git apply just ignores it.

I think git apply doesn't work on non git repos, which is why we have the patch fallback.

I think we should probably solve one thing at a time. If this change resolves things for the new version of Git (ideally without breaking earlier versions), we should move forward here, and then address the other issue (patches sporadically not getting applied) separately.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Nov 9, 2017

Contributor

Okay, I added a conditional that attempts to catch instances where git apply silently fails (such as if you are running it against a distribution of a package, but your application as a whole is under version control).

I think this is the best way to preserve the current business logic while fixing the incompatibilities with newer versions of Git.

Long-term, I think we should stop using git apply altogether, or apply the patches in a temporary directory outside of version control and then move them into place.

Contributor

danepowell commented Nov 9, 2017

Okay, I added a conditional that attempts to catch instances where git apply silently fails (such as if you are running it against a distribution of a package, but your application as a whole is under version control).

I think this is the best way to preserve the current business logic while fixing the incompatibilities with newer versions of Git.

Long-term, I think we should stop using git apply altogether, or apply the patches in a temporary directory outside of version control and then move them into place.

@andypost

This comment has been minimized.

Show comment
Hide comment
@andypost

andypost Nov 9, 2017

I think git apply doesn't work on non git repos, which is why we have the patch fallback

Btw why patch is fallback, it looks it should depend on ”prefer stable" flag or maybe use composer api to detect source of package.

Otoh patch is most universal way so git apply should be fallback

andypost commented Nov 9, 2017

I think git apply doesn't work on non git repos, which is why we have the patch fallback

Btw why patch is fallback, it looks it should depend on ”prefer stable" flag or maybe use composer api to detect source of package.

Otoh patch is most universal way so git apply should be fallback

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 9, 2017

Coverage Status

Coverage decreased (-0.06%) to 2.36% when pulling 8c8c356 on danepowell:issue-148 into 6440b5a on cweagans:master.

coveralls commented Nov 9, 2017

Coverage Status

Coverage decreased (-0.06%) to 2.36% when pulling 8c8c356 on danepowell:issue-148 into 6440b5a on cweagans:master.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Nov 9, 2017

Contributor

This should be good to merge. I'm not really sure how to increase coverage in this instance to get that test to pass.

Contributor

danepowell commented Nov 9, 2017

This should be good to merge. I'm not really sure how to increase coverage in this instance to get that test to pass.

@hostep

This comment has been minimized.

Show comment
Hide comment
@hostep

hostep Nov 9, 2017

There is also the --directory option of git apply which might help here.
(please ignore my comment if irrelevant, I just quickly read through this thread)

hostep commented Nov 9, 2017

There is also the --directory option of git apply which might help here.
(please ignore my comment if irrelevant, I just quickly read through this thread)

@cweagans

This comment has been minimized.

Show comment
Hide comment
@cweagans

cweagans Nov 9, 2017

Owner

Btw why patch is fallback, it looks it should depend on ”prefer stable" flag or maybe use composer api to detect source of package.

@andypost The plugin should use the vcs-appropriate way first (right now, only git is supported, but there's no reason we can't support svn and similar), then patch as the fallback. git is the default because pretty much everyone has that installed. Fewer people have patch installed.

Owner

cweagans commented Nov 9, 2017

Btw why patch is fallback, it looks it should depend on ”prefer stable" flag or maybe use composer api to detect source of package.

@andypost The plugin should use the vcs-appropriate way first (right now, only git is supported, but there's no reason we can't support svn and similar), then patch as the fallback. git is the default because pretty much everyone has that installed. Fewer people have patch installed.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Nov 13, 2017

Contributor

@cweagans I think this is good to merge, is there anything else I can do to improve it?

Contributor

danepowell commented Nov 13, 2017

@cweagans I think this is good to merge, is there anything else I can do to improve it?

@cweagans

This comment has been minimized.

Show comment
Hide comment
@cweagans

cweagans Nov 22, 2017

Owner

This looks good. If you make a PR against 1.x, I'll merge that and tag a 1.x release as well.

Owner

cweagans commented Nov 22, 2017

This looks good. If you make a PR against 1.x, I'll merge that and tag a 1.x release as well.

@cweagans cweagans merged commit 3466175 into cweagans:master Nov 22, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.06%) to 2.36%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RalfEisler

This comment has been minimized.

Show comment
Hide comment
@RalfEisler

RalfEisler Dec 14, 2017

Patches still fail under the following conditions:

"cweagans/composer-patches": "^1.6.4",
git version 2.15.1
patch 2.0-12u10 FreeBSD

Error messages:
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/1356276-408--8.4.3.patch
etc.

RalfEisler commented Dec 14, 2017

Patches still fail under the following conditions:

"cweagans/composer-patches": "^1.6.4",
git version 2.15.1
patch 2.0-12u10 FreeBSD

Error messages:
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/1356276-408--8.4.3.patch
etc.

@danepowell

This comment has been minimized.

Show comment
Hide comment
@danepowell

danepowell Dec 15, 2017

Contributor

@RalfEisler let's not have a discussion in two places, if you can provide a minimum example to reproduce please open a new issue. But as I commented in #148, I suspect there's something with that particular patch, not with Composer.

Contributor

danepowell commented Dec 15, 2017

@RalfEisler let's not have a discussion in two places, if you can provide a minimum example to reproduce please open a new issue. But as I commented in #148, I suspect there's something with that particular patch, not with Composer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment