Re: [LinuxBIOS] commit rules

[Stefan Reinauer]

* Uwe Hermann  [061022 18:07]:
> 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).

I think we can easily reach this. All tracker changes can be sent to the
list, similar to what OLPC does.

I am all for putting all patches into the tracker. For several reasons:

- if I make a patch A and it is superseded by patch B, I need to be able
  to track this.

- Feature enhancements should be handled like bug fixes as well to make
  the change process simpler-

- Every patch therefore is a to-be-tracked bug/issue, and should go to
  the tracker.

If there's no bug, there's no need to change the code, right?

- If there is some issue that requires action on developer side, this
  issue has a distinct status. an issue can be resolved, or open, or 
  in progress, etc. 
  On a mailing list we dont have this status. I have issues in my inbox
  that reflect open threads, though being several weeks old. This is
  not going to change, but it should be handled properly. Just having 
  such stuff in mails _does_ make important issues go down.

I want to point out Uwe's very useful mail of un-checked in patches on 
  the mailing list. I guess some are still open, but I forgot.

Having them in an issue tracker at one point would make life a lot
  easier.

- Last but not least: That is how we've already been doing it since
  quite a while with the roundup issue tracker. But since roundup pretty
  much sucks people stopped using it.
> 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.

Yes, we do want milestones and handle them publically so people can plan
on interactions to get those solved.
> Another important thing for the issue tracker: _Users_ should be able to
> use it to submit bug reports, feature requests etc.

full ack. This is a more modal form of doing support.
> > > As in: No code goes in
> > > that does not serve a (documented) purpose.
> >
> > A good check in comment would state that purpose anyway.

this is the wrong direction. The reason has to be documented 
_before_ the checkin, not after the checkin. I agree though that we
need better checkin comments.
> > 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.

currently we have a couple of people who are long-term contributors and
looked at many different pieces of code to be able to judge whether code
"fits in the structure" of LinuxBIOS, and there are some others that
have 100% indepth hardware knowledge that is required to make things
work. Those are arguing which things should go in until someone feels
determined to check things in. So we dont have a voters system, but we
dont really have a dictator system as well.

The grub project had this discussion recently, and Johan Rydberg had an
interesting suggestion (which did not find too much positive feedback
from the "dictators" though ;):

------- 8< -------
From: Johan Rydberg 
To: [e-mail]
Subject: Let core developers "vote" to accept patches.
[..]
In other projects that I've worked on, we've used a system where the
core developers can either give a +1 or a -1 on a contributed patch.
If a patch receives two +1's or more, it is accepted (if there are no
-1's) and committed by one of the developers.

This lowers the pressure on the maintainers, and hopefully will speed
up the development.

Though, the core developers need to be a bit careful.  They should not
give +1's or -1's on a patch that they either (1) do not fully
understand, or that (2) applies to a section of the code that they are
not knowledgeable about.  It is then better to either state that they
do not know the area well enough, and maybe give the patch a +0, or be
silent.

The downside is that some developers will never feel that they are
knowledgeable enough to comment on a contribution; resulting in that
the maintainer will have to do it.  The same situation we are in now,
more or less.

The upside is that the maintainers can focus on other things than just
reviewing patches; maybe getting some new features in.  Also, trivial
and small patches will get quicker review and get merged into the tree
in a swift manner.

Please note:

This is in no way an attempt to overrun any of our maintainers, it is
just an attempt to make things go a bit smoother and faster in the
times when the maintainers are busy with other things.
[..]
~j
------- 8< -------
> As for the sign-off mechanism: the two main points it should
> achieve, I think, is this:
> Review of _all_ patches.

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

ack!
> 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
>
> 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.

So in addition to one Signed-off-by: we would need at least one other 
Signed-off-by or at least one Acked-by?
> 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.

which argues for putting the patches into the tracker. grabbing this
stuff together in the mailing list archives is fine for stuff that goes 
in after 1-4 days, but will be hell for bigger things..
> 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.

This description should also go to the tracker, so it can be copied to
the commit message from there. There should also be a link to the
tracker. ie. This commit adds the feature described in isse #2342.

This is why we wanted a tracker with VC support after all, right?
> 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.

Maybe a second signed-off-by should be handled like an acked-by, because 
we can assume that someone participating in a patch will acknowledge
its inclusion in the tree.
> > 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.

It still needs to be a process though. We've had enough uber-hackers
destroying the tree because they thought they just know better than all
the rest of us. If people do not want to cooperate, they can do their
own fork, right. So we might very well enforce cooperation in this
project. ;-)

And a set of rules that apply to anyone is 100% to be preferred over
the endless arguing about why one party insists on the other doing this
and that even though its only overhead and it doesnt really matter
because the tree compiles (meaning: for one single board).
> 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.

ack.
> 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.

It should at least be the same email address.

Also: Who should be doing reviews? Only other committers? Or anyone?
> 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:).

ok changed.
> Make a Name and an email address required (already done?)

currently only an email is required. any regex welcome ;)
> 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
>