Re: [PATCH 1/8] Documentation: Adding "The Perfect Patch" by Andrew Morton

[Rob Landley <rob@landley.net>]

On 05/23/2013 07:49:53 AM, Ben Minerds wrote:
>  Adding Andrews advice on patch submission and subdirectory for  
> further patch
>  documentstion.

You've seen Documentation/SubmittingPatches right?

>  Signed-off-by: Ben Minerds <puzzleduck@gmail.com>
> ---
>  .../patches/The-Perfect-Patch.txt                  | 167  
> +++++++++++++++++++++
>  1 file changed, 167 insertions(+)
>  create mode 100644  
> Documentation/development-process/patches/The-Perfect-Patch.txt
> 
> diff --git  
> a/Documentation/development-process/patches/The-Perfect-Patch.txt  
> b/Documentation/development-process/patches/The-Perfect-Patch.txt
> new file mode 100644
> index 0000000..b07074e
> --- /dev/null
> +++ b/Documentation/development-process/patches/The-Perfect-Patch.txt
> @@ -0,0 +1,167 @@
> +From: Andrew Morton [email blocked]
> +To: Mukker, Atul [email blocked]
> +Subject: Re: [patch] 2.6.9-rc1-mm1: megaraid_mbox.c compile error  
> with gcc 3.4
> +Date: 	Sat, 28 Aug 2004 13:04:19 -0700
> +

This email is eight years old.

> +"Mukker, Atul" [email blocked] wrote:
> +>
> +> The driver and the patches with the re-ordered functions is  
> available at
> +>  ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.20.3.1/
> +
> +I dunno about James, but I *really* dislike receiving patches by  
> going and
> +getting them from internet servers.  It breaks our commonly-used  
> tools.  It
> +loses authorship info.  It loses Signed-off-by: info.  There is no
> +changelog.  All this means that your patch is more likely to be  
> ignored by
> +busy people.  Please, just email the diffs.
> +
> +I wrote a little guide this week:
> +
> +
> +
> +The perfect patch. [email blocked]
> +
> +The latest version of this document may be found at
> +http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

Could you just grab the actual version from his repo instead of one  
that says "email blocked" and commemorates the URL of a rejected  
submission in a chatty email header?

> +Delivery
> +========
> +
> +- Patches are delivered via email only.  Downloading them from  
> internet
> +  servers is a pain.
> +
> +- One patch per email, with the changelog in the body of the email.
> +
> +Subject:
> +========
> +
> +- The email's Subject: should consisely describe the patch which  
> that email
> +  contains.  The Subject: should not be a filename.  Do not use the  
> same
> +  Subject: for every patch in a whole patch series.
> +
> +  Bear in mind that the Subject: of your email becomes a  
> globally-unique
> +  identifier for that patch.  It propagates all the way into  
> BitKeeper.  The
> +  Subject: may later be used in developer discussions which refer to  
> the
> +  patch.  People will want to google for the patch's Subject: to read
> +  discussion regarding that patch.
> +
> +- When sending a series of patches, the patches should be  
> sequence-numbered
> +  in the Subject:
> +
> +- It is nice if the Subject: includes mention of the subsystem which  
> it
> +  affects.  See example below.
> +
> +- Example Subject:
> +
> +	[patch 2/5] ext2: improve scalability of bitmap searching
> +
> +- Note that various people's patch receiving scripts will strip away
> +  Subject: text which is inside brackets [].  So you should place  
> information
> +  which has no long-term relevance to the patch inside brackets.   
> This
> +  includes the word "patch" and any sequence numbering.  The  
> subsystem
> +  identifier ("ext2:") should hence be outside brackets.
> +
> +
> +Changelog
> +=========
> +
> +- Bear in mind that the Subject: and changelog which you provide will
> +  propagate all the way into the permanent kernel record.  Other  
> developers
> +  will want to read and understand your patch and changelog years in  
> the
> +  future.
> +
> +  So the changelog should describe the patch fully:
> +
> +  - why the kernel needed patching
> +
> +  - the overall design approach in the patch
> +
> +  - implementation details
> +
> +  - testing results
> +
> +- Don't bother mentioning what version of the kernel the patch  
> applies to
> +  ("applies to 2.6.8-rc1").  This is not interesting information -  
> once the
> +  patch is in bitkeeper, of _course_ it applied, and it'll probably  
> be merged
> +  into a later kernel than the one which you wrote it for.
> +

Bitkeeper?

Is this really current advice? What part of this has _not_ been put in  
SubmittingPatches yet?

(I'm all for moving SubmittingPatches and friends under  
development-process/ and in fact vaguely plan a general Documentation/  
tree cleanup next time I have some downtime between contracts. But  
adding eight year old duplication: not so much.)

> +- Do not refer to earlier patches when changelogging a new version  
> of a
> +  patch.  It's not very useful to have a bitkeeper changelog which  
> says "OK,
> +  this fixes the things you mentioned yesterday".  Each iteration of  
> the patch
> +  should contain a standalone changelog.  This implies that you need  
> a patch
> +  management system which maintains changelogs.  See below.
> +
> +- Add a Signed-off-by: line.
> +

There's a whole edifice of signed-off-by line advice elsewhere in  
Documentation. (The pointy-haired types descended on this and attempted  
to make a bureaucracy out of it.)

> +- Most people's patch receiving scripts will treat a ^--- string as  
> the
> +  separator between the changelog and the patch itself.  You can use  
> this to
> +  ensure that any diffstat information is discarded when the patch  
> is applied:
> +
> +
> +
> +	Another few #if/#ifdef cleanups, this time for the PPC  
> architecture.
> +
> +	Signed-off-by: <valdis.kletnieks@vt.edu>
> +	Signed-off-by: Andrew Morton [email blocked]
> +	---
> +
> +	 25-akpm/arch/ppc/kernel/process.c                    |    2 +-
> +	 25-akpm/arch/ppc/platforms/85xx/mpc85xx_cds_common.c |    2 +-
> +	 25-akpm/arch/ppc/syslib/ppc85xx_setup.c              |    4  
> ++--
> +	 3 files changed, 4 insertions(+), 4 deletions(-)
> +
> +	--- 25/arch/ppc/kernel/process.c
> +	+++ 25/arch/ppc/kernel/process.c
> +	@@ -667,7 +667,7 @@ void show_stack(struct task_struct *tsk,
> +

SubmittingPatches has recommended diffstat command lines...

> +
> +The diff
> +========
> +
> +- Patches should be in `patch -p1' form:
> +
> +  --- a/kernel/sched.c
> +  +++ b/kernel/sched.c
> +
> +- Make sure that your patches apply to the latest version of the  
> kernel
> +  tree.  Either straight from bitkeeper or from
> +  ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots/
> +
> +- When raising patches for -mm it's generally best to base them on  
> the
> +  latest Linus tree.  I'll work out any rejects/incompatibilities.   
> There are
> +  of course exceptions to this.
> +
> +- Ensure that your patch does not add new trailing whitespace.  The  
> below
> +  script will fix up your patch by stripping off such whitespace.
> +
> +	#!/bin/sh
> +
> +	strip1()
> +	{
> +		TMP=$(mktemp /tmp/XXXXXX)
> +		cp $1 $TMP
> +		sed -e '/^+/s/[ 	]*$//' < $TMP > $1
> +		rm $TMP
> +	}
> +
> +	for i in $*
> +	do
> +		strip1 $i
> +	done

Doesn't scripts/checkpatch.pl check for this?

> +
> +Overall
> +=======
> +
> +- Avoid MIME and attachements if possible.  Make sure that your  
> email client
> +  does not wordwrap your patch.  Make sure that your email client  
> does not
> +  replace tabs with spaces.

We have Docuemntation/email-clients.txt which would probably also go  
under development-process/.

Rob