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@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@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@example.com Signed-off-by: Jane Doe bar@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@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@example.com Signed-off-by: Jane Doe bar@example.com Acked-by: John Reviewer baz@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@example.com Acked-by: John Doe foo@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.