wiki:org/meetings/2016WinterDevMeeting/Notes/CodeReviews

BACK TO 2016 WINTER DEV MEETING NOTES PAGE

Tor Code Review

The biggest use of Nick's time is code review, and people are still dissatisfied

Reviewing things is time-consuming
Re-reviewing things is even harder, it is difficult to tell whether the new changes address the comments made on the review

How can we avoid big patches?

Multiple commits - difficult, dependencies, fixups

Write code
Submit commits
Review
Add fixup commits
More review
More fixups
Eventual merge

Line of code + Comment + Fixup Commit

Git extension that rejects fixup commits if they don't autosquash

A common tool for code review
- Web interface or command-line
- Many tools want to control the git repository
- Many tools want to control the bug tracker
- Many tools want people who submit patches to install command-line tools

Email works about as well as trac

Identify each issue with a patch:
- Small issues with reviewer initials and number: NM01, NM02, \u2026
- Split off tickets (child or otherwise) for larger issues

Hard to get patches reviewed quickly

We leave code reviews for a long time

We use different ways of finding code to review
- #tor-bots
- tor-bugs@
- query on trac

But we don't review at a rate that keeps up with our code writing

We need to dedicate a certain amount of time to code review
- 20% of our time

needs-review for more than a week
needs-revision for more than a week

review little things on a routine basis
schedule reviews for big things

a view of how many reviews people need or people are doing

Nick is reviewing for 10-15 hours per week
teor has peaked at 10-15 hours in a few weeks

A two-stage review process would help Nick to do faster reviews
- initial review
- Nick's review

We could tag tickets with reviewers and months (like Tor Browser does)

Does everyone need to do code review every day or every week?

Check how many tickets are in need-review at every core tor meeting

Add a status for final review to trac

Add a reviewer field to trac

Code Review Tools
- GitHub pull requests
- gerrit

Features
- inline commenting
- ask people to review
- who has reviewed it
- what they thought

We want to try gerritt
- special will set it up
- the coder or reviewer will submit the code to gerrit
- gerrit sends emails to potential reviewers

Gerrit requires a commit id for every commit in the commit message
It wants you to modify individual commits rather than using fixups
We can use fixups and autosquash before submitting to gerrit

git resquash in git hacks repository helps with getting fixups right

Last modified 2 years ago Last modified on Mar 18, 2016, 10:04:43 PM