Per warning given to the community, we should remove the acceptance of -p0 patches.

See http://groups.drupal.org/node/140204

CommentFileSizeAuthor
#1 pifr.accept_only_p0.patch5.94 KBrfay

Comments

rfay’s picture

Status: Active » Needs review
StatusFileSize
new5.94 KB

This switches to using only -p1 patches, but gives notice if the patch seems to be a -p0 patch.

rfay’s picture

Status: Needs review » Fixed

Committed: bc06f94fdb3daa953c4b214c40a64860ef176140

I made a mistake in the commit and wrote "accept only -p0 patches" when we're actually only accepting -p1 patches.

Status: Fixed » Closed (fixed)

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

pwolanin’s picture

Status: Closed (fixed) » Active

I think we should revisit this. When I was a new contributor, I created patches with just the diff utility. AFAICT, there is no way with that to easily add a prefix to the testbot will take the patch?

It can be hard enough for people to figure that out - yet I think it's probably much easier than learning git enough to roll patches.

rfay’s picture

The reason for the decision was the other side of this: If we have both p1 and p0 patches in the queue, there's no possible way to explain to people how to apply them. And applying may be more important.

Also... Using git to create a patch is way easier than using diff... And people seem to be figuring it out OK.

rfay’s picture

Status: Active » Fixed

No response, so back to fixed.

Status: Fixed » Closed (fixed)

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

salvis’s picture

Status: Closed (fixed) » Active

The reason for the decision was the other side of this: If we have both p1 and p0 patches in the queue, there's no possible way to explain to people how to apply them. And applying may be more important.

In #1345584-7: -p0 vs. -p1 and associated error messages I took a real-world patch from a first-time patch contributor who prepared his patch with Eclipse/SVN. That patch has the following header:

Index: devel.module
===================================================================
--- devel.module	(revision 1)
+++ devel.module	(working copy)
@@ -854,8 +854,12 @@

It's a -p0 header and Git will accept it with git apply -p0 or git apply (without -p!), but it failed in the testbot, because the testbot insisted on git apply -p1.

I can prepare the same patch with diff -up (as pwolanin mentions in #4) and get a header like...

--- devel.module.old	Sat Nov 19 12:48:48 2011
+++ devel.module	Sat Nov 19 12:48:36 2011
@@ -854,8 +854,12 @@ function devel_shutdown_real() {

... and this, too, applies just fine with git apply -p0 or git apply (without -p!).

Alternatively, I can use diff -up with the original file in a different directory:

--- ../devel.ori/devel.module   Sat Nov 19 12:56:30 2011
+++ devel.module        Sat Nov 19 16:51:10 2011
@@ -854,8 +854,12 @@ function devel_shutdown_real() {

Again, it applies with git apply -p0 or git apply (without -p!).

If I prepare the patch with git diff then I get...

diff --git a/devel.module b/devel.module
index 6af57d0..6300c92 100644
--- a/devel.module
+++ b/devel.module
@@ -854,8 +854,12 @@ function devel_shutdown_real() {

... and this applies with git apply -p1 or git apply (without -p!).

Finally, if I prepare the patch with git diff --no-prefix then I get...

diff --git devel.module devel.module
index 6af57d0..6300c92 100644
--- devel.module
+++ devel.module
@@ -854,8 +854,12 @@ function devel_shutdown_real() {

... and this applies with git apply -p0 only! This is the big exception here, but we don't want to support -p0 anyway, and patch creators would have to go out of their way and specify --no-prefix to get this type of patch.

To sum it up, all the patch formats that we've seen (except the deliberate --no-prefix) are acceptable to git apply. If we let Git do what it does by default (i.e. we don't force it into -p1-only mode by specifying -p1), then we may not need to worry about -p1 vs. -p0 at all. This would remove a barrier for newcomers who may be using other tools and who may not be quite ready to embrace Git yet.

Can anyone come up with a common patch format that git apply (without -p!) will not accept? and that git apply -p1 will?

If not, then I propose that we drop the unnecessary -p1 constraint and be as flexible as Git is.

rfay’s picture

Status: Active » Closed (fixed)

If you want to take this up as a policy debate, please don't do it in the issue queue. Go over to http://groups.drupal.org/node/140204 and take it up there.