Right now all you can do is something like:

projects[htmlpurifier][patch][] = "http://drupal.org/files/issues/htmlpurifier-library_0.patch"

which will download the patch and apply it with 'patch -p0 -d $project_directory < patch'

but many patches require a different strip level or a specific origin file.

I propose adding additional functionality (while keeping the existing one as well) that would allow:

projects[htmlpurifier][patch][0][url] = "http://drupal.org/files/issues/htmlpurifier-library_0.patch"
projects[htmlpurifier][patch][0][strip] = 0
projects[htmlpurifier][patch][0][target] = ""

I'm attaching a patch that enables this

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

medlefsen’s picture

Just a quick note, both strip and target are optional and will default to 0 and "" respectively.

dmitrig01’s picture

how about changing the notation to projects[x][patch][url][strip] = 0

dmitrig01’s picture

Status: Active » Needs review
yhahn’s picture

Status: Needs review » Closed (works as designed)

I don't think we should support badly created patches. As described by the docs on Drupal.org, patches should be created from the root directory of the specified project and follow various other conventions. It's simple enough to re-roll a patch from the correct location and it helps everyone who is collaborating on a project for the patch to be in good shape.

Obviously we could spend our time on drush_make supporting everything under the sun but I think it's worth making decisions that encourage best practices as well.

makara’s picture

Status: Closed (works as designed) » Needs work

Git adds "a/" and "b/" to the file paths, when creating with either "git diff" or "git format-patch".

adamdicarlo’s picture

@makara,

You're supposed to roll patches with this command:

git diff --no-prefix --relative

I use a bash alias for the purpose:

alias make-patch='git diff --no-prefix --relative'
adamdicarlo’s picture

On the other hand, I happened to just look at patch formatting instructions at http://drupal.org/node/1054616 -- apparently patches are supposed to be in the format-patch format these days (since git migration). Huh.

So it looks a bit like drush make will need to support old style (-p0) and new style (-p1) patches.

patcon’s picture

Posted a comment to relevant git handbook page:
http://drupal.org/node/1054616#comment-4152234

makara’s picture

I think it's even better if it can support "git am" or similar.

joshuajabbour’s picture

Yes, as far as I can tell, drush_make can't handle any of the newer patches on d.o

mrf’s picture

When I follow the new git patch directions that are now available on every single project page the patch fails to apply with drush make.

It's too much to ask users to reformat all patches just to work with make. Being able to specify the type of patch would be a nice temporary fix, but ideally the newer format would just work.

patcon’s picture

@makara Correct me if I'm wrong, but I think the catch of git am exclusively (which may not be the suggestion :) is that it would require posters of working patches to reveal their email addresses to google bots.. (or fudge it, which doesn't seem right)

Steven Jones’s picture

Subscribe.

Even a stop-gap of trying -p1 if -p0 fails would be good here.

makara’s picture

Yes I think git am is too much now. It is not for the purpose of applying some changes locally. I think git apply is better. But patch -p1 is enough here.

tim.plunkett’s picture

The convention for drupal.org is to use -p1.

JeremyFrench’s picture

Just to add something to the mix here. I wanted to download a stable version of a module and to apply a patch from the projects repository. This patch was not submitted via the issue queue so the natural choice was to use this

http://drupalcode.org/project/flot.git/patch/0b9d38d28e9f66cee239e7f92cc...

However this won't work with drush make and other than rolling my own patch for this diff. I can't do much about it.

Cyberwolf’s picture

Subscribing.

glennpratt’s picture

Title: better patch support » Apply patches from git diff and git format-patch
Version: 6.x-2.x-dev » 6.x-3.x-dev
Status: Needs work » Needs review
FileSize
1.23 KB

Following the input from Berdir -> http://drupal.org/node/1054616#comment-4179940

This appears to correctly apply patches from http://drupal.org/node/1054616 and older patches without configuration.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Perfect.

dasjo’s picture

looks good

boombatower’s picture

subscribe

rjmackay’s picture

subscribe. Will test this out soon.

rickvug’s picture

I can also confirm that this works with new and old style patches. RTBC.

christianchristensen’s picture

This is awesome! I want to go to there!

jhedstrom’s picture

The patch in #18 fixes this for 3.x.

With the official end of p0 patches on the horizon (http://groups.drupal.org/node/140204), it seems critical to back port this to 2.x.

rfay’s picture

Status: Reviewed & tested by the community » Needs work

I really recommend that you use "git apply" instead of "patch -px".

In the initial round with the testbots, I used patch -px and it meant that binary and copy/move patches wouldn't apply.

Here's the code being used currently on the testbots, see #1107552: Make git patch application more robust and use git for patch application

  public function apply($patch) {
    foreach($this->patch_levels_to_try as $patch_level) {
      $result = pifr_client_review::exec("git apply -p$patch_level --check $patch", TRUE);
      if ($result) {
        $this->patch_level = $patch_level;
        $git_output = "";
        $git_command_result = pifr_client_review::exec("git apply -v -p$patch_level $patch", TRUE, $git_output);
        $this->files_affected_by_patch = $this->determine_files_affected_by_patch($git_output);
        return TRUE;
      }
    }
    return FALSE;
  }
glennpratt’s picture

I tend to agree, but I was trying to make the change minimal.

I assume $this->patch_levels_to_try = array(0, 1); ?

rfay’s picture

patch_levels_to_try = array(1,0). But never mind. I don't think it has to be fancier as fancy as that - it was overkill. IMO the existing structure is fine, I just recommend using git apply to apply. Try -p1 then try -p0.

One nice thing you may want to leverage about git though: If you use git apply --check it doesn't actually change anything. So you can test whether it applies in one easy step, and then apply it if it applies cleanly.

Dane Powell’s picture

subscribing

jhedstrom’s picture

@rfay, wouldn't using git apply only work with git checkouts? drush make currently supports many systems other than git, including non-versioned tar/zip downloads, and not all of these downloads come from d.o.

rfay’s picture

I hadn't thought about that, but I just tried git apply on a non-git directory, and it works fine. Actually, lots better because of the git apply --check which still works fine.

I imagine that binary patches wouldn't work.

Amazingly, copy-move patches do still work in a non-git directory.

So yes, I think git apply will still work fine. It will require that git be available (in the same way that patch must be available).

Dane Powell’s picture

Priority: Normal » Major

I think this is fairly major, considering that -p0 patches will be officially deprecated on May 31 and already most patches I'm seeing in the issue queues are -p1 format. I'm happy to test patches to get this to rtbc, though I'm only using 6.x-2.x right now.

glennpratt’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

New patch with git apply and uses --check before applying.

glennpratt’s picture

Oops, left some debugging in the last git command.

rfay’s picture

I didn't test it, but it looks fine to me.

jhedstrom’s picture

FileSize
1.09 KB

The patch in #34 works for 3.x. Here is the corresponding patch for 2.x.

Dane Powell’s picture

I can confirm that #36 allows patches generated by git to be applied. Thanks!

Dane Powell’s picture

Status: Needs review » Reviewed & tested by the community

Sooo... RTBC?

izmeez’s picture

subscribing

jhedstrom’s picture

Status: Reviewed & tested by the community » Needs work

The patch from #36 (for drush make 2.x) isn't actually applying the patches even though

 $patched = drush_shell_exec('git apply -p%s --directory %s %s', $patch_level, $project_directory, $filename);

is returning true. I haven't tested against 3.x yet, but suspect a similar issue with the patch in #34.

Dane Powell’s picture

@jhedstrom- really? I'm using #36 to great success on 2.x- patches usually apply just fine, and when they fail, it shows up in the drush make log as an error.

jhedstrom’s picture

@Dane Powell, yeah--git apply seems to be silently failing. This seems to only happen under the following conditions: attempt to patch module foo, within a git repo that is ignoring contrib modules (via .gitignore). If module foo is moved completely out of the .git directory structure, say to /tmp, then git apply works as expected.

- master project git repo
-- drupal/sites/all/modules/contrib (ignored by repo)

git apply fails on the above.

jhedstrom’s picture

You can reproduce the git apply behavior I mention in #42 as follows:


jonathan@wonko:/tmp$ mkdir test
jonathan@wonko:/tmp$ cd test
jonathan@wonko:/tmp/test$ touch README.txt
jonathan@wonko:/tmp/test$ git init .
Initialized empty Git repository in /tmp/test/.git/
jonathan@wonko:/tmp/test (master #)$ git add README.txt
jonathan@wonko:/tmp/test (master #)$ git commit -m"Testing git apply issue."
[master (root-commit) d82fc3c] Testing git apply issue.
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 README.txt
jonathan@wonko:/tmp/test (master)$ mv ../drupal-6.20 .
jonathan@wonko:/tmp/test (master)$ cd drupal-6.20/
# This silently fails:
jonathan@wonko:/tmp/test/drupal-6.20 (master)$ git apply  --check -p0 --verbose --stat --directory ./ drupal.locale-import.384794-32.patch
 0 files changed, 0 insertions(+), 0 deletions(-)
jonathan@wonko:/tmp/test/drupal-6.20 (master)$ cd ..
jonathan@wonko:/tmp/test (master)$ mv drupal-6.20/ ..
jonathan@wonko:/tmp/test (master)$ cd ../drupal-6.20/
# Now, outside of the context of a git repo, this command properly throws an error:
jonathan@wonko:/tmp/drupal-6.20$ git apply  --check -p0 --verbose --stat --directory ./ drupal.locale-import.384794-32.patch
Checking patch ./a/includes/locale.inc => ./b/includes/locale.inc...
error: ./a/includes/locale.inc: No such file or directory
jonathan@wonko:/tmp/drupal-6.20$ git apply  --check -p1 --verbose --stat --directory ./ drupal.locale-import.384794-32.patch
Checking patch ./includes/locale.inc...
 ./includes/locale.inc |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)
adamdicarlo’s picture

Looks like there's another way this `git apply` silent-failure bug can be triggered. Check out this post I just found:

http://data.agaric.com/raw/git-apply-does-not-work-from-within-local-che...

So it seems to me that git-apply is unusable in these two conditions:

* your .gitignore indicates ignorance of the to-be-patch file
* your current directory is in an unrelated git repository

The common element apparently is that the to-be-patched file is not committed (and not staged?) in your current-directory git repository.

It seems the solution requires either not using git-apply or changing into a separate directory when running git-apply. Unless there's some git-apply flag that I'm missing that could work around this behavior.

glennpratt’s picture

Status: Needs work » Needs review

I don't think there is a git apply flag, I looked. I think the correct solution is making sure we apply the patches outside any git repo (except perhaps for the current module).

Thing is, it appears that is happening already.

This is a section of drush --verbose --debug make test.make www, working on a -p0 patch, where test.make has both types of patches, run inside a git repo with www/sites/all in .gitignore. Note, we patch in /tmp already.

Executing: curl -LOD '/tmp/drush_make_tmp_1303757528/__header__' 'http://drupal.org/files/issues/og-og_views-member-count-link-558134-22.patch'
    % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                   Dload  Upload   Total   Spent    Left  Speed
103  2884  103  2884    0     0  10572      0 --:--:-- --:--:-- --:--:-- 20309
Calling chdir(/vagrant/public/apci-og6)
Executing: ls '/tmp/drush_make_tmp_1303757528/__download__'
  og-og_views-member-count-link-558134-22.patch
Executing: mv '/tmp/drush_make_tmp_1303757528/__download__/og-og_views-member-count-link-558134-22.patch' '/tmp/drush_make_tmp_1303757528/og-og_views-member-count-link-558134-22.patch'
Executing: rm -f '/tmp/drush_make_tmp_1303757528/__header__'
Executing: rm -rf '/tmp/drush_make_tmp_1303757528/__download__'
Executing: git apply --check -p'1' --directory '/tmp/drush_make_tmp_1303757528/__build__/sites/all/modules/contrib/og' '/tmp/drush_make_tmp_1303757528/og-og_views-member-count-link-558134-22.patch'
  error: /tmp/drush_make_tmp_1303757528/__build__/sites/all/modules/contrib/og/og_views/includes/og_views_handler_field_og_member_count.inc: No such file or directory
Executing: git apply --check -p'0' --directory '/tmp/drush_make_tmp_1303757528/__build__/sites/all/modules/contrib/og' '/tmp/drush_make_tmp_1303757528/og-og_views-member-count-link-558134-22.patch'
Executing: git apply -p'0' --directory '/tmp/drush_make_tmp_1303757528/__build__/sites/all/modules/contrib/og' '/tmp/drush_make_tmp_1303757528/og-og_views-member-count-link-558134-22.patch'
og patched with og-og_views-member-count-link-558134-22.patch. [20.11 sec, 9.4 MB]                                                                                                                                              [ok]

I manually verified the patch is applied correctly in the resulting build.

I put this back to Needs Review, just because I would need more info to be able to fix that issue.

jhedstrom’s picture

@glenpratt, you're right, the build is already taking place outside of any git repo. So, by adding calls to chdir into the build directory, git apply works as expected. The attached patch is for 2.x, I'll reroll the 3.x one shortly.

jhedstrom’s picture

Oops. That last patch had an xdebug call in it.

jhedstrom’s picture

And here is the 3.x patch.

jhedstrom’s picture

And now, a working one for 3.x (the last patch was still using $project_directory instead of $this->project_directory). Sorry for the noise.

glennpratt’s picture

Interesting... I'm wondering what I missed trying to reproduce what you found.

j0nathan’s picture

Subscribing.

ldweeks’s picture

subscribing

Dane Powell’s picture

q0rban’s picture

Here's a different (and more abstract approach) that was on #1110430: Unable to apply patch generated following instructions on drupal.org.

jhedstrom’s picture

I haven't tested #54 yet, but it doesn't appear to include the workaround needed for the issues in #40-#48. It also switches back to using patch instead of git apply. I've been successfully using #48 for a few weeks at this point.

q0rban’s picture

Failed to mention, the problem with using git apply is that when it fails, it still returns 0 (success), so there's no way to detect failure. So, the patch in #54 doesn't use git apply, and instead uses patch and allows you to specify options to that command. It defaults to -p1, but you can specify any options you choose. More flexible IMO.

jhedstrom’s picture

@q0rban, from my testing (see #43), git apply does properly return non 0 on failure when it is outside the context of another git repository, that's why the patch in #47/48 is using chdir to actually go to the tmp directory where the make file is being built.

rfay’s picture

@q0rban, the fundamental reason for using git apply is that it can apply patches containing moves, copies, and binary diffs, things which patch of course cannot do.

q0rban’s picture

"git apply does properly return non 0 on failure when it is outside the context of another git repository, that's why the patch in #47/48 is using chdir to actually go to the tmp directory where the make file is being built."

That was not my experience in testing for writing the patches in #1110430, but I'll give it another go. Thanks!

q0rban’s picture

The patches in #47/49 would not work for non-git downloads. I still think the approach here needs to not assume git. Are binary diffs really an edge case we need to account for?

rfay’s picture

We've already had patches with both binary diffs and copies/moves in the queue.

If we don't support them, we won't be able to apply patches from the issue queue that could normally be applied and that people would expect to work.

q0rban’s picture

Assigned: Unassigned » q0rban
Status: Needs review » Needs work

I'll try to work on a patch for this, but I have my doubts. ;)

jhedstrom’s picture

q0rban, can you post some example patches for testing non-git downloads failing? I've been using it on some fairly old .make files that are presumably still using pre-git patches.

q0rban’s picture

Sorry, i was using git am and that was failing. You are right, git apply works on non git repos. However, git apply still is returning 0 for me when I use patches that don't apply.

q0rban’s picture

It seems like this might be a bug in drush itself, or possibly PHP exec(): if I run the command by hand, I get a sane exit status:

$ git apply ~/Sites/profiler-enable_theme-1149572-1-eojthebrave.patch 
fatal: corrupt patch at line 21
$ echo $?
128
rfay’s picture

FileSize
347 bytes

I guess we have to talk about what kind of patch and what kind of problem. Just for sanity, I just tried a good patch and a bad patch against Examples D7. Note that the return values are correct (I'm using git apply --check here in the first round of tries, which is a wonderful thing). Also note that git doesn't actually apply if a patch application fails. The patches I used are attached.

So here are the things I can think of that are causing the discrepancy:
* Different versions of git?
* Different types of patch failures?
* Different patch application context?

$ git clone git://git.drupal.org/project/examples.git --branch 7.x-1.x
$ cd examples
$ # First try it just using git apply --check
$ git apply --check ../goodpatch.patch
$ echo $?
0
$ git apply --check ../badpatch.patch
error: patch failed: README.txt:1
error: README.txt: patch does not apply
$ echo $?
1
$ # Now try it with actually using git apply without --check
$ git apply ../goodpatch.patch
$ echo $?
0
$ git reset --hard
HEAD is now at 62bf19d Fix syntax error that I introduced in render_example.test in commit 8e2beaa818c9595e233c52734849cbecbe1a412f
$ git apply ../badpatch.patch
error: patch failed: README.txt:1
error: README.txt: patch does not apply
$ echo $?
1
q0rban’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

Okay, figured out my problem. I had a trailing semicolon on the command that was being executed, and drush_shell_exec then appends 2>&1 to that. Patch attached against 6.x-3.x-dev.

q0rban’s picture

Whoops, looks like somehow the patch got stripped to 120 chars wide. New patch attached.

glennpratt’s picture

Status: Needs review » Needs work

@q0rban - Why are we taking a config approach? This means existing drush make files will not work.

Also, you don't have to use git am to apply a patch made with git-format-patch, I can apply them just fine with git apply or patch. I feel like it's a mistake to link the patch command to the command that generated the diff.

I could see a use case for adding 'git am' option with --working-copy, but that seems like scope creep for this issue.

rfay’s picture

git am is probably completely inappropriate for drush make. It actually makes a commit (if a patch was created with git format-patch), and we don't need that. And yes, any patch that applies with git am should apply with git apply.

q0rban’s picture

Why are we taking a config approach? This means existing drush make files will not work.

I think it's more flexible and safer.

Also, you don't have to use git am to apply a patch made with git-format-patch, I can apply them just fine with git apply or patch.

You don't have to use git-format-patch, but I figured some might like the option. I can remove it if it is a problem.

I feel like it's a mistake to link the patch command to the command that generated the diff.

Hmm, Randy has coerced me into using git instead of patch for that very reason (the application used to create the diff should also be the application that applies the patch).

rfay’s picture

I don't agree that "the application used to create the diff should also be the application that applies the patch". I think git apply should be used for every patch in a context like this. I have found no downside to this approach.

jhedstrom’s picture

The patch in #68 will require p0 patches to be explicitly declared in the .make file--is this desired? If so, it will break all .make files out there pointing at p0. The benefit of the approaches earlier in this thread was that they were backwards compatible.

rfay’s picture

I'm not a drush make user, so don't have much of a voice here, but @q0rban, I remain a bit baffled why you started over, when the earlier patches (#47/49) in this issue were (I thought) near-RTBC. I favored them as simple and unobtrusive and backward-compatible.

I guess we should decide at this point which way we're going to go. I vote for the simpler approach.

glennpratt’s picture

I'm +1 for git apply only and determine the patch level automatically, mainly because it's what PIFR is doing.

If you run into a problem, re-roll the offending patch to Drupal standards. That will make everyones life easier.

Convention over configuration or something like that...

rfay’s picture

Remembering, of course, that the testbots will cease to auto-detect patch levels at the end of this month.

q0rban’s picture

I remain a bit baffled why you started over, when the earlier patches (#47/49) in this issue were (I thought) near-RTBC. I favored them as simple and unobtrusive and backward-compatible.

I didn't start over, I picked up from my previous patch that happened to be on a different issue, and added the git-apply functionality like you requested.

The reason the patches in 47/9 don't work is that they require git. We can't assume git. If you're writing an install profile with a drupal-org.make file for d.o you can assume git, but if a developer is writing a make file for themselves and they don't have git, the patches will fail, with no notification of why. The approach in #68 sticks to convention, but adds configuration to allow the developer to customize the way in which the patch is applied in any way they choose.

I agree that existing drush make files will be broken, but what did we expect in changing from p0 to p1? "If you run into a problem, re-roll the offending patch to Drupal standards." Exactly.

I think we need Dmitri to weigh in here before we proceed, but I feel like everyone here is coming at this from a "fix drush_make for d.o" mentality and not a "fix drush_make for developers and d.o" mentality. Flexibility wins, in my book.

jhedstrom’s picture

I think we can assume git. Even if a developer is using drush make for a client project, using svn or bzr, they should still have git on their machine. Back when drupal was on cvs, project[...][download][type] = CVS failed silently and inexplicably if the system didn't have CVS installed--this sort of usability can, and should, be addressed in a separate issue.

but what did we expect in changing from p0 to p1

I expect, that wherever possible, something like drush make retain backwards compatibility--especially, as you mention, if patches are coming from non-drupal.org sources (say, a patch to a jquery library or a WYSIWYG) where p0 is still the standard. Using git apply, in combination with testing for p1 or p0, allows both formats to simply work, without having to write an extra line for every patch in the make file.

q0rban’s picture

Assigned: q0rban » Unassigned
Status: Needs work » Reviewed & tested by the community

I think assuming git is a mistake, for the very reason that you mention, for "patches [that] are coming from non-drupal.org sources (say, a patch to a jquery library or a WYSIWYG)."

Maybe I just joined this issue the wrong way. I didn't intend to ruffle any feathers, I just brought along the patch that had been worked on in a duplicate issue and was trying to accommodate everyone's desire to default to git-apply, as well as add some extra features that I think developers would like, such as the ability to specify custom options and choosing the application that will apply the patch.

But I will stop. Randy says 47/49 are rtbc.

q0rban’s picture

Status: Reviewed & tested by the community » Needs review

Whoops, nevermind, Randy said they were "near RTBC"

q0rban’s picture

Ok, here's an updated version based off of #49. Differences are:

  • it doesn't use cwd(), instead cd-ing into the directory in the command itself
  • it passes the whole p1/0 option in instead of just the number (the command was looking like this otherwise: git apply --check -p'1'). I tried to use %d, but that didn't work (kept outputting 0 there)
  • Proper white spacing and comment formatting
  • Adds debugging info and uses dt instead of concatenating log messages
q0rban’s picture

Whoops, comment got placed in the wrong spot.

jhedstrom’s picture

Category: feature » bug

The patch in #82 works for 3.x for me.

I started on a backport to 2.x, but wasn't sure where the 2.x equivalent of $this->name would be found.

I'm changing this issue from a feature request to a bug, since without this patch, patching via drush make from d.o. no longer works.

univate’s picture

I am using the patch is #82 without any problems, I have it working on a project with both -p0 and -p1 patches and everything is building correctly. I much prefer this patch to other previous ones that require special syntax changes to the *.make files.

Tempted to change this to RTBC

glennpratt’s picture

Status: Needs review » Reviewed & tested by the community

@q0rban - Thank you for the work! I didn't mean to sound argumentative, just sharing my 2 cents.

This has been RTBC before. I'm using #82 with success. univate likes it. RTBC

q0rban’s picture

I didn't mean to sound argumentative, just sharing my 2 cents.

No, not at all, I didn't take it that way. I was just getting frustrated because I was misunderstanding and subsequently put a bunch of time into the other solution. :)

mrfelton’s picture

Patch in #82 works for me. Thanks.

mrfelton’s picture

Sorely in need of a 2.x version of this.

jhedstrom’s picture

@mrfelton, the patch in #47 is what I've been using in 2.x for now--I started to roll the patch in #82 into 2.x, but ran into an issue not knowing what the 2.x equivalent of $this->name would be.

webchick’s picture

Steven Jones’s picture

Assigned: Unassigned » Steven Jones

Will post a 2.x version of #82 in a couple of hours.

Steven Jones’s picture

Assigned: Steven Jones » Unassigned
FileSize
2.51 KB

I started on a backport to 2.x, but wasn't sure where the 2.x equivalent of $this->name would be found.

That would be $this->name, simple!

Attached is basically the patch from #82 ported to Drush make 6.x-2.x.

Steven Jones’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/drush_make.project.inc
@@ -72,14 +72,40 @@ class DrushMakeProject {
+          $checked = drush_shell_exec('cd %s && git apply --check %s %s', $this->project_directory, $patch_level, $filename);

Can we use the drush_shell_cd_and_exec function here, instead of manually calling 'cd' here? I think it's possibly drush 4 only though, not sure how that should be handled in drush make? Do we need to raise the requirements?

Also, it should be noted that this change is going to have a fair bit of impact for patches that include new files, and that need to be applied with '-p0', as there is a bug in most versions of git that prevents such patches being applied, and they fail instead with

fatal: git apply: bad git-diff - inconsistent new filename 

We might need to point this out in the release notes, as some make files will start breaking for this reason I would imagine.

Powered by Dreditor.

Darren Shelley’s picture

#82 fails for me with 3.x

On entry to the following line my current working directory is the drupal root and the project_directory is "custom_search", changing directory to it therefore fails for me:

$checked = drush_shell_exec('cd %s && git apply --check %s %s', $this->project_directory, $patch_level, $filename);

EDIT: Fixed with re-download of drush make, perhaps had old version of make.

jhedstrom’s picture

@Darren Shelley do you get an error message? Can you run the command with --debug and post back any relevant messages here?

Darren Shelley’s picture

@jhedstrom,

I redownloaded drush, make and the patch so I could be confident of the commit numbers and this resolved my issue.
Thankyou.

jhedstrom’s picture

Priority: Major » Critical

@Steven Jones I don't think raising the requirements to Drush 4 should be an issue, since Drush 3 isn't even listed on the module page anymore.

Regarding the p0/new file issue, do you have any more details on which versions of git this happens, or which version it was fixed in? Mentioning that in the release notes seems sufficient.

(Also bumping this to critical, as patching via drush make will not work for p1 patches, and official end of p0 support on d.o. was May 31.)

Steven Jones’s picture

Git don't seem to use a nice issue tracking system, instead using mailing lists so I can't seem to find the place in which it was fixed, but a google search does turn up lots of related posts:

http://www.google.com/search?btnG=1&pws=0&q=fatal%3A+git+apply%3A+bad+gi...

I'm using git from ubuntu 10.10, which http://packages.ubuntu.com/maverick/git seems to suggest means that git has this issue in the 1.7 series, which is a bit scary.

Agileware’s picture

Subscribing

drew reece’s picture

#92 Worked for me with drush make 6.x-2.2

xatoo’s picture

The patch in #92 worked fine for me as well (drush_make 6.x-2.2).

I don't know whether there is consensus yet on whether or not to require Drush 4. I however rerolled Stevens patch from #92 for 3.x and changed drush_shell_exec to drush_shell_cd_and_exec.

Steven Jones’s picture

I really don't want to introduce a regression in drush make, we'll cause an awful lot of pain for people. I wonder if we can detect git failing in this way, and then try patching with the old method, using patch.

Steven Jones’s picture

Okay, so here's #92 plus a fallback to 'patch -p0' if we detect 'git apply' being funny about new files in patches. This should cover all the bases, apart from if git throws the error message in another language, which I'm not sure we can/should do much about.

This is for drush make 6.x-2.x.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

And a patch with the fallback for those that prefer to roll with 6.x-3.x.

Reviews please!

kasperg’s picture

#104 works for me. Thanks!

j0nathan’s picture

Hi,

With patch in #104 I was able to apply patches in both formats in the same makefile using drush make 2.2 and drush 4.4.

Thank you

gboudrias’s picture

Subscribing

justintime’s picture

subscribe

ao2’s picture

FileSize
317 bytes

Hi, in my scenario git-apply does not work, let me explain:

  • I have a distro makefile which gets an installation profile via git (so /profiles/myprofile/.git exists)
  • the installation profile gets some modules (NOT under git control) and some of them I'd like to patch (they are in /profiles/myprofile/modules/contrib/ and their files are all untracked in the .git above from the profile)

normally git-apply should fallback to normal patch when handling files not controlled by git, but in this case it fails to apply changes to untracked files in subdirs, so I think trying patch first is the way to go to be more failsafe, and then possibly give git-apply a try if it that fails, so we can use its features (like git files renaming which patch does not support).

However I think that even relying on patch alone is more than acceptable, so we get rid of the hard dependency on git as well.

Let me know if the description of my scenario is not clear enough, I am also attaching a bash script to recreate what I am experiencing with git apply.

Thanks,
Antonio

lelizondo’s picture

patch in #104 worked for me. thanks.

glennpratt’s picture

@ao2

normally git-apply should fallback to normal patch when handling files not controlled by git, but in this case it fails to apply changes to untracked files in subdirs...

Projects are downloaded and patched in a temp directory, not in your Git repo, exactly because of that concern. My make files are always in Git repos and I have a mix of all sorts of modules sources. If this is indeed a problem, the patch you have needs work.

However I think that even relying on patch alone is more than acceptable, so we get rid of the hard dependency on git as well.

Patch cannot correctly apply all Git patches. Git is what Drupal and all of contrib uses, so I don't see that as an option.

boombatower’s picture

Seems like git apply and fall back to patch -p0 would work, but warn people or maybe make it a setting you have to explicitly override? Otherwise lets just make git a dependency and move forward. Either way we need to at minimum support git apply.

ao2’s picture

Projects are downloaded and patched in a temp directory, not in your Git repo, exactly because of that concern.

@glennpratt Ah, maybe that is what I was missing, I thought the patching was happening at the final destination (/profiles/myprofile/modules/contrib/mymodule in my case), not in the temp dir already, I will check that.

If the patch is applied locally in a temp dir before moving to the destination, then git apply should work in my scenario too indeed.

Thanks for mentioning this.

ao2’s picture

I checked and the problem is still there because patches are applied to projects inside the destination tmpdir, not the project local tempdir, in my case the patch is applied to /tmp/drush_make_tmp_1309939611/__build__/profiles/myprofile/modules/contrib/mymodule, so still in a subir of the git controlled /tmp/drush_make_tmp_1309939611/__build__/profiles/myprofile/.

Maybe I wasn't able to express my problem clearly: only myprofile is fetched with git in my case, not mymodule, and that is what makes git-apply to fail. Is anyone able to replicate what I am experiencing?

If I got it right the current sequence of actions performed by drush_make for a project is:

  1. Download
  2. Copy to __build__ (part of the download task)
  3. Patch

Maybe 2 and 3 should be switched, but for that to happen copying to the destination dir should be decoupled form the download task.

Thanks,
Antonio

jonhattan’s picture

#104 also works for me for a git diff patch. Of course I get the project to be patched from git.

projects[fb][version] = 3.x-dev
projects[fb][download][type] = git
projects[fb][patch][] = http://drupal.org/files/issues/fb-1011912_0.patch
Steven Jones’s picture

@ao2 are you saying that if you have a .gitignore file in the git repo, then git apply will honour that file and won't patch the files specified in it?

ao2’s picture

I am attaching a stripped down example to show what I mean.

Remember, only myprofile if downloaded with git (the fact that I am using the file:// transport does not make any difference at all), not the other modules.

Download the archive and follow the README.

From README.asciidoc

drush_make patched with http://drupal.org/files/issues/drush_make-745224-git-apply-105.patch

Fetch the code

tar xzvf drush_make_git-apply_test.tgz
cd drush_make_git-apply_test
sudo rm -rf ~/public_html/test_site && \
drush make --working-copy distro.make ~/public_html/test_site

Verify that the patch has been applied

In the file myprofile.make there is a patch specified for the rubik theme:

projects[rubik][subdir] = contrib
projects[rubik][version] = 4.0-beta5
projects[rubik][patch][http://drupal.org/files/issues/languageprefs-1140410-2.patch] = http://drupal.org/files/issues/languageprefs-1140410-2.patch

But it looks like it is not applied by git-apply, because myprofile is under git control, while the themes/contrib/rubik/ subdir is not.

cd ~/public_html/test_site/profiles/myprofile/themes/contrib/rubik/
wget http://drupal.org/files/issues/languageprefs-1140410-2.patch 
cat languageprefs-1140410-2.patch | patch -p1 --dry-run

As you can see the patch still applies even if it is supposed to be applied already according to myprofile.make and the drush output.

If I try to run git apply here, it does not work:

git apply languageprefs-1140410-2.patch

Note that it fails silently no indication at all that the patch has not been applied.

Thanks,
Antonio

glennpratt’s picture

Status: Needs review » Needs work

@ ao2 Oh, I see. I'm guessing --working-copy is part of replicating that issue.

I'm going to play around with git apply flags, but you are probably on the right track to patch in project temp dir.

glennpratt’s picture

Poked around, asked in IRC, don't see a way around it, other than patching in temp dirs for projects.

Steven Jones’s picture

Okay, so this is git, so is uber configurable, specifically the GIT_WORK_TREE environment variable can be set to the work tree: $project_directory meaning that it won't get applied to higher level git repos if they exist.

We can also add the --verbose flag to give more feedback.

Drush make has some tests at the moment, we should add some tests too!

ao2’s picture

@Steven Jones thanks, that worked indeed.

This change is enough to make patch in #104/#105 cover my scenario too:

diff --git a/drush_make.project.inc b/drush_make.project.inc
index 399a30c..d92df65 100644
--- a/drush_make.project.inc
+++ b/drush_make.project.inc
@@ -78,10 +78,10 @@ class DrushMakeProject {
         // http://drupal.org/node/1054616
         $patch_levels = array('-p1', '-p0');
         foreach ($patch_levels as $patch_level) {
-          $checked = drush_shell_cd_and_exec($this->project_directory, 'git apply --check %s %s', $patch_level, $filename);
+          $checked = drush_shell_cd_and_exec($this->project_directory, 'GIT_WORK_TREE=. git apply --check %s %s', $patch_level, $filename);
           if ($checked) {
             // Apply the first successful style.
-            $patched = drush_shell_cd_and_exec($this->project_directory, 'git apply %s %s', $patch_level, $filename);
+            $patched = drush_shell_cd_and_exec($this->project_directory, 'GIT_WORK_TREE=. git apply %s %s', $patch_level, $filename);
             break;
           }
         }

I used . because we cd $project_directory already.

Reordering the download, copy and patch tasks would still be nice but that can be done later.

Should I send a fixed up patch or will you do that Steven?

And what about improving the commit messages a little bit? This is the scheme I use:

  • Short commit message: Issue number and a very brief explanation of WHAT is done
  • Long commit message (after a blank line): WHAT, HOW and WHY the changes in the patch are good/necessary
  • Use imperative form like we were telling the patch itself to modify the code to achieve our goal.

Thanks again,
Antonio

Steven Jones’s picture

I'm seeing that git is unable to patch some files that patch used to be able to do, which is a bit strange, I need to investigate more and will roll a patch with the changes in later.

ao2’s picture

About the reordering of the "download", "copy" and "patch" operations, I opened a dedicated issue #1212834: Decouple "download" and "copy" operations..

This one can be solved independently from that reordering problem.

Thanks,
Antonio

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
3.66 KB

Right, so here's a patch for drush make 6.x-2.x that incorporates the GIT_WORK_TREE fix. I've also updated the patch tests so they pass.

Steven Jones’s picture

And a version for 6.x-3.x

mrfelton’s picture

#104 applies to 4.4 and gets things working for me. Thanks.

mrfelton’s picture

EDIT: I take that back. It worked with one exception. So I tried the patch at #125 which also doesn't always work.

With a make file like this:

core = 7.x
api = 2

projects[media_gallery][version] = 1.0-beta6
projects[media_gallery][subdir] = contrib
projects[media_gallery][patch][] = "http://drupal.org/files/issues/1053674-Use_colorbox_module_setting-4_0_0.patch"

drush_make 6.x-2.2 and the patch at #125 applied. Neither of the attempts to patch with git work, and so it resorts to using patch -p0 which doesn't work either since it's a git patch. This revised patch also adds another fallback to try with patch -p1, which gets it working.

thedavidmeister’s picture

patch #128 does seem to make patches work that previously failed when run through aegir.

thanks!

Steven Jones’s picture

git should be able to apply the patch from the makefile in #128: http://drupal.org/files/issues/1053674-Use_colorbox_module_setting-4_0_0... as that's the entire point of using git apply in drush make, any idea as to why it doesn't apply?

ShaunDychko’s picture

#125 is working nicely for me. Thank you very much.
Both http://drupal.org/files/issues/preserve_roles.patch and http://drupal.org/files/issues/securepages-750104-3.patch wouldn't apply before the patch in #125.

jhedstrom’s picture

Steven Jones’s picture

@mrfelton Looking at the patch that isn't applying, it doesn't apply with git, so isn't a 'valid' drupal.org patch, so would need to be re-rolled.

Awesome that we got this in!

dmitrig01’s picture

Oh, sorry for not marking at such. Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jgraham’s picture

GIT_WORK_TREE does not resolve the issue described by ao2 for me in comments 110, 115.

I wasn't sure if this issue should be re-opened so I created a new one;
http://drupal.org/node/1276872

thedavidmeister’s picture

I'm using patch #128.

What I'm seeing when building from a make file that includes patches:

Without --working-copy, or through Aegir
* A module pulled from d.o. as a tarball (ie. not tracked by git) and just identified with a version number will have patches applied correctly
* A module taken from a git repo with a revision set in the make file will have patches applied correctly

Running drush make through the command line with --working-copy
* Modules pulled from d.o. as a tarball will NOT have patches apply at all, but drush will report that the patch was successful and even generate PATCHES.txt
* Modules cloned from a git repo from a set revision will have patches apply correctly and drush will report exactly the same as the previous case where the patch failed

thedavidmeister’s picture

nm, just saw jgraham's post