[LinuxBIOS] commit rules

[Uwe Hermann]

Hi,

On Sat, Oct 21, 2006 at 11:35:53AM +0200, Segher Boessenkool wrote:
> There's a mailing list archive, every patch gets sent to the
> mailing list for review --> it's trackable (although a pain
> to find the right threads sometimes, esp. if people break
> the ML threads).

Yeah. I would say every patch _must_ be sent to the list for review,
no exceptions.

Trivial patches (e.g. a new chip for flashrom) can then be checked in
after a few hours or a day if nobody complains, but for more complex
patches the review time should be longer.

I agree that all of the review and patch management should happen on the
list. The issue tracker should only be used for tracking bugs (when a
commit fixes a bug in the tracker, the commit message should say
"Closes: #1234" or some such thing; Trac for example will then
automatically link to the respective issue/bug).

The issue tracker can also be used for long-term issues (so we don't
forget about such things), and maybe release planning/goals etc.

Another important thing for the issue tracker: _Users_ should be able to
use it to submit bug reports, feature requests etc.


> > As in: No code goes in
> > that does not serve a (documented) purpose.
> 
> A good check in comment would state that purpose anyway.

Definately. I like the idea to reject commits with empty log messages.


> > At some point when we are
> > doing LinuxBIOS v4 or v5, we might even go as far as "no code goes in
> > that has no automated test case that the code does what it is  
> > required to do"
> 
> Good :-)

Yeah. Not easy to do for such low-level software, but a good idea.


> How about: only maintainers (of the code under discussion) can approve
> a patch (and they can approve their own).  Patches should _still_ be
> sent to the mailing list of course (and in most cases it would be
> prudent to not check in before it has been out for review for a few  
> days).

I don't think there are clear maintainers of subsystems (as in the Linux
kernel). Neither is there a Benevolent Dictator (Linus) here.
Both is a good thing, IMHO.

As for the sign-off mechanism: the two main points it should
achieve, I think, is this:

 1. Review of _all_ patches.

 2. Tracking of who is the author and copyright owner of the code, and
    making sure that all contributed code is GPL'd.

After reading
http://lxr.linux.no/source/Documentation/SubmittingPatches
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
I think the way the process works in the Linux kernel would be
applicable to LinuxBIOS, too.

Here's my proposal:

 * Every patch _must_ be sent to the list. In the body it should contain
   at least one 'Signed-off-by: John Doe <foo at example.com>' line.

   The Signed-off-by means that the person listed there is the author
   (or co-author) and copyright owner of the code _and_ has made sure
   that it doesn't contain any non-GPL-compatible parts _and_ agrees
   to license the code under the GPL (version 2 or any later version).

   All files in the patch _must_ contain the GPL header with exact
   'Copyright (C) 2006 John Doe <foo at example.com>' lines.
   If someone want to license the code under a license different to
   the 'GPL (version 2 or any later version)' the mail should explicitly
   say so, _and_ the files in the patch should then have the license
   header of that other license (e.g. BSD license, GPLv2, etc.).

   Of course, the license _must_ be GPL-compatible at least, or else
   the patch will be rejected.

 * The patch is reviewed on the list. If someone modifies the code and
   posts a new version to the list, he adds himself to the list
   of Signed-off-by lines, e.g.

     Signed-off-by: John Doe <foo at example.com>
     Signed-off-by: Jane Doe <bar at example.com>

 * If a reviewer merely looked over the patch and thinks it's ok and
   should be applied, he replies to the mail with

     Acked-by: John Reviewer <baz at example.com>

   This only means he reviewed the patch, he does _not_ modify it, and
   thus is not an author/co-author, and thus doesn't own the copyright
   on the code.

 * When the review process is finished and there's consensus that the
   patch is fine, it gets committed (usually by the person who wrote
   it, or by someone else if the person doesn't have an svn account).

   The commit log then _must_ contain _all_ Signed-off-by and Acked-by
   lines the patch gathered during the review process.
   Also, it _must_ contain a good, descriptive explanation what the
   patch does, and why (but that should always be the case anyway).
   If it fixes a bug from the issue tracker, it should be referenced.

   Example commit message:

   Add support for component X of chipset XYZ (Closes: #1234).

     Signed-off-by: John Doe <foo at example.com>
     Signed-off-by: Jane Doe <bar at example.com>
     Acked-by: John Reviewer <baz at example.com>


Optional:

 * We _could_ introduce a rule that every (non-trivial) patch needs at
   least one Signed-off-by and at least one Acked-by line. This ensures
   that at least one other developer looked over the patch and didn't
   spot any glaring obvious breakage.

   This can be enforced by an svn pre-commit-hook. For _trivial_ patches
   the author can then use

     Signed-off-by: John Doe <foo at example.com>
     Acked-by: John Doe <foo at example.com>

   to override the pre-commit-hook. This also allows easy searching for
   patches which probably got no review and for punishing the guilty ;-)


> Please even with a tracker keep it on the mailing list as well.  Mail
> works offline, is distributed, has built-in redundancy -- all things
> that a centralised web-based thing does not.
[...]
> Let's keep the whole "process" thing as flexible and unobtrusive as
> possible?

ACK.

I think using the Linux kernel process is nice, because it's
well-established and well-known, so contributors who know the
kernel-process, feel right at home when contributing to LinuxBIOS.


Also, I suggest to always use the _same_ name, email (and case and spelling)
for the Signed-off-by/Acked-by lines for consistency reasons.
E.g. don't use "Ron G. Minnich", "ron minnich", "Ronald G. minnich",
"ronald g. minnich" etc... Choose one and use it consistently.

Another suggestion for the pre-commit-hook:

 * Make it check for the exact spelling of 'Signed-off-by:' (that's
   what the kernel uses (instead of Signed-Off-By:).
 * Make a Name and an email address required (already done?)


Uwe.
-- 
Uwe Hermann 
http://www.hermann-uwe.de
http://www.it-services-uh.de  | http://www.crazy-hacks.org 
http://www.holsham-traders.de | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20061022/e3f21d4e/attachment.sig>