Patch review

[denise]

So, right now our major bottleneck in the dev process is code review! We're doing really well with patches being coded, and really well with bringing in new developers and getting them interested and working on things, but we've been having real problems finding people to do code review, and almost all the time it winds up being the people who are committing patches who do all the reviews.

This is a bad thing, for a few reasons: it means the committers burn out more quickly; everyone has different review styles, and notices different things, so having only one or two people doing the bulk of reviews leads to more things being missed; it slows down the turnaround time on commits; it means the committers are mostly working on reviewing and committing so they don't get as much time to code themselves; it means the committers feel rushed and pressured to get through the queue, because they know that nobody else is doing it, and that doesn't make for good work; and it means the rest of the dev team is losing out on the chance to learn from the process of reviewing someone else's code. I'd really like to see more peer review going on, with people reviewing each other, so that the committers have more of a chance to commit things that have already been reviewed by others instead of doing the bulk of the review themselves.

Ideally, I think we should be at around 60-70% peer review, with the remaining bits being committer review, and right now we're pretty much at 0%.

There are a few things I'd like to try to encourage people to do more peer reviewing, but before I do any of that, I figured I'd ask you guys some questions about doing reviews. Behind the cut is a poll, and I'd also love to hear any of your thoughts in comments. Poll answers are viewable in specific to nobody but maintainers of the comm, and if you have a comment that you'd like to leave, but don't feel comfortable doing so logged-in, anon commenting is enabled.

Poll #3965

Open to: Registered Users, detailed results viewable to: Just the Poll Creator, participants: 23

I don't do patch reviews because:

I don't like doing it
2 (9.5%)

I don't feel qualified
18 (85.7%)

I'm scared I might miss something and cause problems
12 (57.1%)

The instructions aren't clear enough
0 (0.0%)

I didn't think I was allowed to
8 (38.1%)

I don't have the time
6 (28.6%)

I'd rather be coding
4 (19.0%)

It's too hard to find things that need review
2 (9.5%)

I just generally feel intimidated by the thought
12 (57.1%)

I don't know what to look for
14 (66.7%)

I feel bad about being critical
3 (14.3%)

When I spot flaws in a patch, I want to fix them myself
3 (14.3%)

I find it too difficult to explain what's wrong with a patch
1 (4.8%)

I just keep forgetting
5 (23.8%)

I don't feel like there's enough social/emotional reward
1 (4.8%)

(Something else I will explain in comments)
1 (4.8%)

I would do more code review if:

I knew I wasn't going to be the only person to review the patch before commit
15 (65.2%)

There were more regular reminders to do reviews
6 (26.1%)

There were more clear instructions about what to look for when reviewing
11 (47.8%)

I could do partial reviews (review for style vs functionality vs efficiency/security)
13 (56.5%)

There were more public thanks/praise for reviewers
2 (8.7%)

There were tangible reward for reviewing (ie, DW points)
1 (4.3%)

There were classes/tutorials on how to do a review
10 (43.5%)

It were easier to apply and test patches
5 (21.7%)

It were easier to find patches that need review
4 (17.4%)

There were regularly scheduled group/peer "review queue whacks"
9 (39.1%)

I had a "review partner" to go over patches together
12 (52.2%)

Patches in the queue were more easy to identify as major vs minor
5 (21.7%)

I got better feedback on the quality of my reviews
3 (13.0%)

(Something else I will explain in comments)
3 (13.0%)