DIYEFI.org Forum

This post is a list (that I intend to maintain) of people who I'd like to see reviewing code on a regular basis, and what they're suited to reviewing:

• Me - firmware and OLV (I could do EMStudio too, but don't have enough time)
• Sean0 - firmware and EMStudio
• MrOnion - firmware, OLV and EMStudio
• johntramp - firmware and EMStudio
• malcom2073 - firmware and EMStudio
• MerpCore - firmware and EMStudio
• BenFenner - OLV

To help ensure high quality in every project each commit should be reviewed. It doesn't take long to do (unless the commit is huge or complex), so it's very worth while to avoid mistakes slipping in.

This is not only really good for the quality of the project, it's also a solid way to become familiar with the code bases and that's gotta be good for both you and the project.

A guide on how to do a basic review is available here: viewtopic.php?f=41&t=1683

Please post up links/issues/commit hashes of stuff that you actively want reviewed!

Fred.

Right now I'd like to see 3 commit-sets of mine reviewed and three associated issues closed. Two are a pair currently assigned to MrOnion, and the other one to Sean0.

http://issues.freeems.org/view.php?id=243 seank-efi
http://issues.freeems.org/view.php?id=213 & http://issues.freeems.org/view.php?id=844 MrOnion

Others can dive in and grab these any time, they're not owned by the person they're assigned to (in this case).

I'd like to see any commit of Mike's that makes it into dev/master branches reviewed too. I hate regressions, let's help him eliminate those with good process.

Fred.

Are you using the github "pull request" inteface for code review?

No, but that's not a bad idea, I guess. I'm considering setting up Gerrit, but it's fairly rigid in the way it works. For now the volume is low enough that the issue tracker is enough. The hash goes in there anyway.

Is there a clean way to 'sign off' a commit as being reviewed?

On github you just click "merge", or comment +1

I'll probably require commits to have a signed off by field in them in future. Gerrit takes care of the reviewed/not thing nicely, but as mentioned, it's pretty serious/heavy infrastructure.

Fred wrote:I'll probably require commits to have a signed off by field in them in future. Gerrit takes care of the reviewed/not thing nicely, but as mentioned, it's pretty serious/heavy infrastructure.

Overkill for this project right now IMHO. Especially since you use github. I've setup a few Gerrit's now, but only because we needed our own infrastructure. Also, Gerrit is hard on servers!

sry_not4sale wrote: Overkill for this project right now IMHO. Especially since you use github. I've setup a few Gerrit's now, but only because we needed our own infrastructure. Also, Gerrit is hard on servers!

Could not agree more.
Git am, has a sign option, you can sign merges too (In no-ff mode)