Phasing out -p0 patches - May 31?

We encourage users to post events happening in the community to the community events group on https://www.drupal.org.
rfay's picture

When we did the Git migration, we set up the testbots to continue to allow both -p0 (old style Drupal patches) and -p1 (the normal Git style patches), but started encouraging the community to switch. This allowed existing patches on issues to continue to test OK, and avoided irritating everybody enormously.

However, now we have a problem. How do we teach people to successfully apply patches if there can be more than one kind? I think we have to phase out -p0 patches sooner rather than later. This would mean:

  • Disabling -p0 testing on the testbot, meaning that -p0 patches would fail to apply
  • Announcing that -p1 must become the standard in the issue queue.

Would a May 31 phase-out date be OK with everybody? Are you seeing new -p0 patches in the queue?

Comments

I still see quite a lot of p0

jhedstrom's picture

I still see quite a lot of p0 patches in the queues. In fact, on issues where a patch is already in p0 form, I've found myself continuing to post updates to that patch in p0 format so as not to confuse people already on the issue.

It seems that announcing a phase out date, and then rigorously marking p0 patches as 'needs work' with appropriate links to the p1 docs is the only way to completely get rid of p0 style patches.

p1 is git diff default, so

Josh The Geek's picture

p1 is git diff default, so that's what I use.

I go by Josh The Geek.
Drupal.org
GitHub

+1 to setting a date and

tizzo's picture

+1 to setting a date and encouraging people to standardize

I don't care so much when the date is but that sounds fine :-)

This needs to be done so

markdorison's picture

This needs to be done so let's go ahead and rip the band-aid off. +1

How?

sun's picture

It's called "script" and fundamentally works identically everywhere:

Elsewhere (untested):

#!/bin/bash

if [ $('grep -qe "^--- a/" $1') ]; then
  patch -p0 < $1
else
  patch -p1 < $1
fi

Windows:

@echo off

set CYGWIN=nontsec
set patchdir=%~dp1
set patchfile=%~nx1

cd /d "%patchdir%"
rem Determine amount of path prefixes to strip.
rem -p0 needed to cleanly apply own patches and patches from Drupal.org.
rem -p1 needed for git a/b patches.
grep -qe "^--- a/" "%patchfile%"
if not "%errorlevel%"=="0" (
  patch -p0 < "%patchfile%"
) else (
  patch -p1 < "%patchfile%"
)

The latter soon being available in http://drupal.org/sandbox/sun/1117610

Daniel F. Kudwien
netzstrategen

This is what the testbots are currently doing

rfay's picture

This is what the testbots are currently doing.

The problem is that we can't train people to handle patches that might be in multiple formats. So to make the documentation clear, with no disclaimers like (and look inside the patch to see if it has a/ or b/, or use Sun's script) we need to go with one patch format.

Edit: Actually, this isn't what they're currently doing. They actually try p1 and if it fails try p0. It's a more authoritative result. Analyzing the patch had some real gotchas.

FWIW Filterdiff is an awesome

mikey_p's picture

FWIW Filterdiff is an awesome tool for dealing with stripping and adding prefixes to patches.

I support this.

webchick's picture

The Git migration went live at the end of February, after which time -p1 became the official documented standard. If a -p0 patch still exists as the "latest" patch in an issue post-May 31, that means (in most cases) that the solution hasn't been touched in more than 3 months. At that point, it probably could use a second set of eyes anyway, and a re-roll could act as a legitimate way to bump it back up into peoples' consciousness.

However, this won't actually catch and "code needs work" all -p0 patches, because only a small fraction of projects are testbot-enabled. So I don't think there's a way to get around documenting both, or at least adding a "Did this not work for you? Try the patch troubleshooting faq" or similar in the Git instructions tab.

Drush make and p1 compatibility

jhedstrom's picture

One thing worth noting, Drush Make (stable 2.2) does not yet support p1 patches. There is an issue for 3.x, and a working patch, but since Drupal.org uses drush make to build install profiles, it seems critical that this patch be back-ported to 2.x prior to May 31.

The drush make issue is

rfay's picture

The drush make issue is important, but already most patches being used are -p1, so this change really is irrelevant. All we're really going to do is say "The testbot is no longer trying to test -p0 and you should use standard git patches all the time". Neither of these affects drush make, which is already broken.

Or do I misunderstand?

Drush make is not broken, our

mikey_p's picture

Drush make is not broken, our .make file validation code for Install Profiles is broken right now: http://drupal.org/node/751242

Even if that issue were resolved prior to the p1 deadline it wouldn't matter since Drupal.org doesn't allow patches or third party downloads in it's make files anyway. That'll be added someday, but when it is, we'll update the whole mess to a newer drush and drush make version.

What's the rational for

jbrown's picture

What's the rational for standardizing on -p1 instead of -p0?

I really hate seeing all those 'a's and 'b's all the time.

Git uses -p1 by default

rfay's picture

Since we're now using git, it seemed reasonable for people to be able to use "git apply" to apply patches and "git diff" or "git format-patch" to create them.

Fair enough!

jbrown's picture

Fair enough!

Update on testbot code deployment

rfay's picture

I have the code nearly ready to deploy on the testbots and hope to do it next week. -p0 patches are failed in the queue and have a status like "this might be a -p0 patch..."

Why do we have to insist on -p1?

salvis's picture

git apply will accept -p1 and most -p0 format patches. However, the testbot currently does git apply -p1, which makes Git reject all -p0 format patches.

Why do we restrict the acceptable patch formats beyond what Git would do by default?

jonathan1055's picture

Hi,
I had been creating patches for issue queues using

diff -up file1 file2 > foo.patch

because I do not have git installed on my local machine. I'd like to know how I can create patches with the a/ and b/ format as required by the testbots, because my current style of patches are now being rejected.

I've tried to find this info in the documentation on drupal.org but with no luck yet. Hope you can help.

Thanks
Jonathan

Re: -p1 style patches using diff

ohnobinki's picture

You could actually try to recreate the imaginary situation a -p1 patch uses.

  1. Extract the original module and rename the directory to a.
  2. Rename the modified version of the module to b.
  3. diff -rup a b > module.patch

Thanks. Yes I could but

jonathan1055's picture

Thanks. Yes I could but that's a bit more trouble to do, given that I have the files as .module and .module.~1~ in the same folder.

However, I discovered that just editing the original patch and adding --git and a/ and b/ infront of the file names, and the other bits eg change:

--- devel.module    2011-09-16 17:09:11.000000000 +0100
+++ devel.module    2012-05-07 14:30:02.000000000 +0100

into

diff --git a/devel.module b/devel.module
index
--- a/devel.module
+++ b/devel.module

makes the patch file acceptable the testbot :-)