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

From: Rob Landley
Date: Fri May 24 2013 - 00:10:46 EST

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@xxxxxxxxx>
---
.../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@xxxxxx>
+ 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--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/