Currently, git patches are blindly applied with -p1 and then -p0 if that fails. There are scenarios such as core:
/README.txt
/modules/README.txt

and more commonly addition of files that could cause this approach to behave inappropriately.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

There are two areas that need improvement:
* Patch application
* Determining what files were changed

We can improve patch application by doing a git apply --check or a patch -p1 --dry-run and seeing whether it will apply before actually trying it.

We can improve the determination of what files were changed rather dramatically by using git apply -v --check and parsing the result rather than doing what we're currently doing.

There is still some risk, particularly when just adding files, that this will make a mistake. But for the time being I think it's worth the risk. I'd like to phase out -p0 patches within the next year or so and then remove the code that handles this.

rfay’s picture

Title: Make git patch application more robust » Make git patch application more robust and use git for patch application

We definitely need to use git apply for actual application. See #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

Also, need to see what will happen with binary patches.

rfay’s picture

Priority: Normal » Critical
rfay’s picture

Here are the basics of what needs to happen with this patch:

1. Test patch application instead of just applying the patch.
2. Use git apply to apply the patch
3. Change to using the results of git apply --dry-run -v to determine what files are changed; much simpler.
4. Determine the consequences or implications of binary patches.

scor’s picture

Priority: Critical » Normal

subscribe. this should help to make the infrastructure more consistent, and once we figure out if #1116900: git format-patch displays file moves as whole verbose diffs is possible, make patches smaller :)

scor’s picture

Priority: Normal » Critical

oops

rfay’s picture

Assigned: Unassigned » rfay

I'm going to see if I can take a look at this tonight.

If it can be done and tested, it's easy to deploy because it just means deploying it on the testbots.

rfay’s picture

Status: Active » Needs review
FileSize
6.43 KB

I believe this is going to work. I'm testing it now, and will try it out on the scratch servers.

Status: Needs review » Needs work

The last submitted patch, pifr.apply_patches_with_git_1107552_08.patch, failed testing.

pillarsdotnet’s picture

+1

rfay’s picture

Status: Needs work » Fixed

I guess we'll have to fix the tests someday.

I committed this to 6.x-2.x: 5e8508

I tested it with binary patches and with moves. I also tested it with a number of -p0 and -p1 patches. It seems OK, although I wouldn't be surprised if there are a few patches that applied with patch that don't apply.

To create a binary patch: git format-patch --binary --full-index --stdout

To create a moves-and-copies patch (one that magically moves and copies): git format-patch -C -M --stdout

pillarsdotnet’s picture

Yay! (rfay++)

Question for rfay:

What do I need to put in my .gitconfig file so that the "--binary --full-index -C -C -M" options are the default?

I tried reading the git docs and couldn't figure it out. Or if there's a tutorial anywhere that you think answers the question, a link would be awesome.

For now I'm just using a bash alias...

rfay’s picture

Created 6.x-2.4 release containing this and requested deployment in #1122890: Deploy 6.x-2.4 of PIFR on qa.drupal.org

rfay’s picture

@pillarsdotnet, git help format-patch is the fastest way to the options. git help config is the fastest way to "how to set up .gitconfig".

It looks to me like git config --global diff.renames copy is what you want for automatic rename and copy detection.

I don't see instructions in there about using binary. I haven't experimented with it, but it may not be something you want on all the time. Seems relatively unusual that we want binary handling.

pillarsdotnet’s picture

Hmm.. maybe git config --global format.attach true allows binary?

Thanks anyway. I was hoping/expecting that "git config" could add any arbitrary command-line options, but I guess it can't. Back to bash aliases again.

chx’s picture

pillarsdotnet’s picture

chx++

Status: Fixed » Closed (fixed)

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