Changes between Initial Version and Version 1 of org/meetings/2016WinterDevMeeting/Notes/CodeReviews


Ignore:
Timestamp:
Mar 18, 2016, 10:04:43 PM (3 years ago)
Author:
isabela
Comment:

--

Legend:

Unmodified
Added
Removed
Modified
  • org/meetings/2016WinterDevMeeting/Notes/CodeReviews

    v1 v1  
     1[https://trac.torproject.org/projects/tor/wiki/org/meetings/2016WinterDevMeeting/Notes BACK TO 2016 WINTER DEV MEETING NOTES PAGE]
     2
     3'''Tor Code Review'''[[BR]][[BR]]The biggest use of Nick's time is code review, and people are still dissatisfied[[BR]][[BR]]Reviewing things is time-consuming[[BR]]Re-reviewing things is even harder, it is difficult to tell whether the new changes address the comments made on the review[[BR]][[BR]]How can we avoid big patches?[[BR]][[BR]]Multiple commits - difficult, dependencies, fixups[[BR]][[BR]]Write code[[BR]]Submit commits[[BR]]Review[[BR]]Add fixup commits[[BR]]More review[[BR]]More fixups[[BR]]Eventual merge[[BR]][[BR]]Line of code + Comment + Fixup Commit[[BR]][[BR]]Git extension that rejects fixup commits if they don't autosquash[[BR]][[BR]]A common tool for code review[[BR]]- Web interface or command-line[[BR]]- Many tools want to control the git repository[[BR]]- Many tools want to control the bug tracker[[BR]]- Many tools want people who submit patches to install command-line tools[[BR]][[BR]]Email works about as well as trac[[BR]][[BR]]Identify each issue with a patch:[[BR]]- Small issues with reviewer initials and number: NM01, NM02, \u2026[[BR]]- Split off tickets (child or otherwise) for larger issues[[BR]][[BR]]Hard to get patches reviewed quickly[[BR]][[BR]]We leave code reviews for a long time[[BR]][[BR]]We use different ways of finding code to review[[BR]]- #tor-bots[[BR]]- tor-bugs@[[BR]]- query on trac[[BR]][[BR]]But we don't review at a rate that keeps up with our code writing[[BR]][[BR]]We need to dedicate a certain amount of time to code review[[BR]]- 20% of our time[[BR]][[BR]]needs-review for more than a week[[BR]]needs-revision for more than a week[[BR]][[BR]]review little things on a routine basis[[BR]]schedule reviews for big things[[BR]][[BR]]a view of how many reviews people need or people are doing[[BR]][[BR]]Nick is reviewing for 10-15 hours per week[[BR]]teor has peaked at 10-15 hours in a few weeks[[BR]][[BR]]A two-stage review process would help Nick to do faster reviews[[BR]]- initial review[[BR]]- Nick's review[[BR]][[BR]]We could tag tickets with reviewers and months (like Tor Browser does)[[BR]][[BR]]Does everyone need to do code review every day or every week?[[BR]][[BR]]Check how many tickets are in need-review at every core tor meeting[[BR]][[BR]]Add a status for final review to trac[[BR]][[BR]]Add a reviewer field to trac[[BR]][[BR]]Code Review Tools[[BR]]- !GitHub pull requests[[BR]]- gerrit[[BR]][[BR]]Features[[BR]]- inline commenting[[BR]]- ask people to review[[BR]]- who has reviewed it[[BR]]- what they thought[[BR]][[BR]]We want to try gerritt[[BR]]- special will set it up[[BR]]- the coder or reviewer will submit the code to gerrit[[BR]]- gerrit sends emails to potential reviewers[[BR]][[BR]]Gerrit requires a commit id for every commit in the commit message[[BR]]It wants you to modify individual commits rather than using fixups[[BR]]We can use fixups and autosquash before submitting to gerrit[[BR]][[BR]]git resquash in git hacks repository helps with getting fixups right