[PATCHv2] Document from line in patch format

[PATCHv2] Document from line in patch format

[Paul Jackson]

Document more details of patch format such as the "from" line
used to specify the patch author, and provide more references
for patch guidelines.

Signed-off-by: Paul Jackson <pj@sgi.com>

Index: 2.6.14-rc2-mm2/Documentation/SubmittingPatches
===================================================================
--- 2.6.14-rc2-mm2.orig/Documentation/SubmittingPatches
+++ 2.6.14-rc2-mm2/Documentation/SubmittingPatches
@@ -301,8 +301,46 @@ now, but you can do this to mark interna
 point out some special detail about the sign-off. 
 
 
+12) The canonical patch format
 
-12) More references for submitting patches
+The canonical patch subject line is:
+
+    Subject: [PATCH 001/123] [<area>:] <explanation>
+
+The canonical patch message body contains the following:
+
+    The first line of the body contains a "from" line specifying
+    the author of the patch:
+
+        From: Original Author <author@email.com>
+
+    If the "from" line is missing, then the author of the patch will
+    be recorded in the source code revision history as whomever sent
+    Linus the email (the email message "From:" header.)
+
+    The "from" line is followed by an empty line and then the body
+    of the explanation.
+
+    After the body of the explanation comes the "Signed-off-by:"
+    lines, and then a simple "---" line, and below that comes the
+    diffstat of the patch and then the patch itself.  The "---" line
+    and diffstat are optional, but helpful to readers of non-trivial
+    patches.
+
+The Subject line format makes it very easy to sort the emails
+alphabetically by subject line - pretty much any email reader will
+support that - since because the sequence number is zero-padded,
+the numerical and alphabetic sort is the same.
+
+See further details on how to phrase the "<explanation>" in
+the "Subject:" line in Andrew Morton's "The perfect patch",
+referenced below.
+
+See more details on the proper patch format in the following
+references.
+
+
+13) More references for submitting patches
 
 Andrew Morton, "The perfect patch" (tpp).
   <http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt>
@@ -310,6 +348,14 @@ Andrew Morton, "The perfect patch" (tpp)
 Jeff Garzik, "Linux kernel patch submission format."
   <http://linux.yyz.us/patch-format.html>
 
+Greg KH, "How to piss off a kernel subsystem maintainer"
+  <http://www.kroah.com/log/2005/03/31/>
+
+Kernel Documentation/CodingStyle
+  <http://sosdg.org/~coywolf/lxr/source/>
+
+Linus Torvald's mail on the canonical patch format:
+  <http://lkml.org/lkml/2005/4/7/183>
 
 
 -----------------------------------

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCHv2] Document from line in patch format

[Linus Torvalds]

On Sun, 2 Oct 2005, Paul Jackson wrote:
>
> Document more details of patch format such as the "from" line
> used to specify the patch author, and provide more references
> for patch guidelines.

One more issue: I'd really prefer that the "---" not be documented as 
"optional".

Yes, my tools will also notice "diff -" and "Index: " at the start of the 
line as being markers for where the real patch starts, but that's a hack 
because people haven't been following the "---" rule. I'd much rather make 
it clear that the "---" is supposed to be there, to mark where the end of 
the comments are.

Note that after the "---" you can have any amount of explanations that are 
not to be committed as comments on why. Not just a "diffstat", but in 
general it's an area for explaining why I should commit it or random notes 
on the patch. Things that don't necessarily make sense once the patch _is_ 
committed.

For example, a submitter might want to note that he's working on a better 
patch, but that this patch is the minimal one before a release. Things he 
wants to tell the maintainer, but that don't necessarily make sense to 
anybody else.

So the _diffstat_ is optional (although much preferred, especially for 
bigger patches), but the marker of "this is the end of the comments for 
the changelog" is not.

		Linus

Re: [PATCHv2] Document from line in patch format

[Paul Jackson]

Linus wrote:
> One more issue: I'd really prefer that the "---" not be documented as 
> "optional".

Makes sense.  Thanks for taking the time to spell it out

PATCHv3 coming up soon.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [PATCHv2] Document from line in patch format

[Magnus Damm]

On 10/3/05, Linus Torvalds <torvalds@osdl.org> wrote:
> On Sun, 2 Oct 2005, Paul Jackson wrote:
> >
> > Document more details of patch format such as the "from" line
> > used to specify the patch author, and provide more references
> > for patch guidelines.
>
> One more issue: I'd really prefer that the "---" not be documented as
> "optional".
>
> Yes, my tools will also notice "diff -" and "Index: " at the start of the
> line as being markers for where the real patch starts, but that's a hack
> because people haven't been following the "---" rule. I'd much rather make
> it clear that the "---" is supposed to be there, to mark where the end of
> the comments are.

Huh, I thought that the first line in a unified patch started with
"---", and that the lines above were treated as garbage. Relying on
"diff -" or "Index: " seems wrong. Try diffing two files by "diff -u
file1 file2" and look at the output - the first line is "---"... This
extra "---" you are proposing seems like a workaround to me.

/ magnus

Re: [PATCHv2] Document from line in patch format

[Paul Jackson]

Magnus wrote:
> Huh, I thought that the first line in a unified patch started with
> "---",

Yes, it does.  But then the next character is a space, and there
is at least one more space-separated field on the line.

That is, Linus's marker marches

    /^---$/

and the first line of a unified patch matches:

    /^--- .*[^ ].*$/

It ain't fancy, but it works.  These patterns
are easily distinguished.

> Relying on "diff -" or "Index: " seems wrong.

I don't think anyone is relying on those patterns.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [PATCHv2] Document from line in patch format

[Magnus Damm]

On 10/4/05, Paul Jackson <pj@sgi.com> wrote:
> Magnus wrote:
> > Huh, I thought that the first line in a unified patch started with
> > "---",
>
> Yes, it does.  But then the next character is a space, and there
> is at least one more space-separated field on the line.

Nitpicking, but with "diff -L" only one space exists, and that is
between "---" and the label. So it is possible to create patches
without time stamp information.

> > Relying on "diff -" or "Index: " seems wrong.
>
> I don't think anyone is relying on those patterns.

Good. Thanks.

/ magnus

Re: [PATCHv2] Document from line in patch format

[Linus Torvalds]

On Tue, 4 Oct 2005, Magnus Damm wrote:
> 
> Huh, I thought that the first line in a unified patch started with
> "---", and that the lines above were treated as garbage.

Indeed, that's true of traditional unified diffs. That's why my tools 
started using "---" in the first place. Cutting off at the "---" means 
that it's _guaranteed_ that we never include any valid patch in the 
description by mistake rather than applying it.

And I _used_ to just hand-edit emails so that things like "Please apply" 
and "Thanks", and "diff -urN" didn't show up.

However, note that "git diffs" can actually contain renames, so a git diff 
may have relevant stuff before the "---". For example:

	diff --git a/arch/v850/kernel/asm-consts.c b/arch/v850/kernel/asm-offsets.c
	similarity index 100%
	rename from arch/v850/kernel/asm-consts.c
	rename to arch/v850/kernel/asm-offsets.c
	diff --git a/arch/v850/kernel/entry.S b/arch/v850/kernel/entry.S
	--- a/arch/v850/kernel/entry.S
	+++ b/arch/v850/kernel/entry.S
	@@ -22,7 +22,7 @@
	 #include <asm/irq.h>
	 #include <asm/errno.h>
	 
	-#include <asm/asm-consts.h>
	+#include <asm/asm-offsets.h>
	 
	 
	 /* Make a slightly more convenient alias for C_SYMBOL_NAME.  */

is a real diff as far as git is concerned, and is obviously much more 
readable (and much more compact) than the traditional broken "delete file 
and re-create it under another name" kind of diff.

So "^diff --git " is actually the real marker for where a git diff starts.

> Relying on "diff -" or "Index: " seems wrong. Try diffing two files by 
> "diff -u file1 file2" and look at the output - the first line is 
> "---"... This extra "---" you are proposing seems like a workaround to 
> me.

With traditional unified patches nothing _relies_ of "diff -" or "Index:", 
exactly because "---" is _always_ seen as the beginning of the patch. But 
even then the patch applicator often wants to see the "diff -" or "Index:" 
line, because it can use them to disambiguate file names.

So there are real _technical_ reasons so make sure that we know where the 
diff starts, and "---" is a good marker, because it's a marker that is 
guaranteed to be there, regardless.

But even apart from those technical reasons, I don't want the first "diff 
-urN ..." or "Index:" line to show up as a commit message. So the tools 
will _also_ break the commit messages at those markers, but that's a hack.

So the _real_ rule is that we break at "---". No ifs, buts, or maybes. And 
no, it's not "extra" or "optional" or "unnecessary".

And it also allows people to put extra commentary, like a diffstat, or a 
private message to me about why I should apply the patch.

		Linus