Gerrit/Code review/Getting reviews
- To learn about reviewing code from others check the tutorial and the Code review guide.
How to get your code changes reviewed faster and make it more likely to get accepted?
Prerequisites
- You have changed some code to fix a bug or add new functionality.
- You have followed the Coding conventions.
- You have followed the Pre-commit checklist.
- You have followed the Security checklist for developers.
- You have a Developer account to use with Gerrit and successfully set up Git/Gerrit on your machine.
Now you plan to create a patch to upload to Gerrit to get your code reviewed and merged (included in the code base).
Your patch
Check beforehand if it is in scope
If your proposed code changes do not fix a bug but introduces a new feature instead, speak first to the maintainers to make sure that your idea is in the project scope and that your proposed technical approach is the optimal solution.[1] This will save you time and potential disappointment.
Test your changes
Test your changes in your development environment to make sure there are no compilation errors or test failures. If you have not tested your patch for some reason, explicitly say so in a comment in Gerrit. Consider going through the Pre-commit checklist.
Avoid providing incomplete fixes or introducing new bugs. If you know that your patch needs more work, explicitly say so in Gerrit by reviewing it as "-2" or adding a "[WIP]" (work in progress) prefix to the commit message.
Write small commits
Small, independent, complete patches are more likely to be accepted.[2][3] The more files there are to review in your patch, the more time and effort a review will take[4] and the more likely several reviews or a larger number of review iterations will be needed.[5]
If your commits are going to be touching separate files and there's not a lot of dependency between them, it's probably best to keep them as smaller discrete commits.
Furthermore, make sure to not include unnecessary changes in your patch (e.g. fixing the coding style).[1]
However, if your commits are going to be touching the same files repeatedly, bundle them up into one large commit (using either --amend or squashing after the fact).
Write a meaningful commit message
Commit message should describe what and why. What was the problem? How does the fix resolve it? How to test that it actually works? See Gerrit/Commit message guidelines for more.
Also, make sure to proofread and use proper spelling and punctuation in your commit message.
Provide documentation
If a feature in your patch is going to be visible to end users or administrators, make sure to update or create related documentation.[1] See Development policy#Documentation policy for more information.
Don't mix rebases with changes
When rebasing, only rebase. If non-rebase changes are made inside a rebase changeset, you have to read through a lot more code to find it and it's non-obvious. When you are making a real change, leave a Gerrit comment explaining your change, and revise your commit summary to make sure it's still accurate.
Your activity
Submit a Phabricator ticket
Create a task in Phabricator explaining the motivation (see Phabricator/Help). Patches without Phabricator tickets are harder to find reviewers for. To associate them, include a line in the commit message that says "Bug:" followed by the Phabricator ticket number.
Respond to test failures and feedback
Check your Gerrit settings and make sure you are getting email notifications. If your code fails automated tests, or you got some review already, respond to it in a comment or resubmission.
(To see why automated tests fail, click on the link in the "failed" comment in Gerrit, hover over the failed test's red dot, wait for the popup to show, and then click "console output.")
Feedback will usually (but not always) come with a C-2, C-1, C+1, or C+2 flag from Gerrit. You can read more about what these mean (from the reviewer's perspective) at Gerrit/Code review#Complete the review. Some reviewers will offer suggestions for improvement without using an explicit C-1 rating in order to reassure new contributors. You should still take their suggestions seriously.
Sometimes you will receive reviews which you will perceive as irrelevant, for instance merely cosmetic. Do not ignore such reviews but amend your patch to satisfy trivial requests: You may disagree, but discussing costs time and is more expensive than conceding a point.
Code review is a matter of building consensus, not winning an election. You are expected to address negative feedback, not "outvote it" by seeking additional positive reviews. Addressing negative feedback does not always require changes to your patch: sometimes all that is required is a better explanation. However, even in those cases it can be useful to incorporate those explanations into the commit message or comments in the code to better inform future maintainers with the same concerns.
In general: Be patient and grow. More experienced patch writers receive faster responses and also more positive ones.[5]
Add reviewers
The choice of reviewers plays an important role on reviewing time. More active reviewers provide faster responses.[5]
Right after you commit, add one or two developers to the changeset as Reviewers. (These are requests – there's no way to assign a review to one specific person in Gerrit.) Experienced developers should help with this: if you notice an unreviewed changeset lingering, then please add reviewers. To find reviewers:
- Check the main maintainers list, or the maintainers listed in the extension's page, to find who's currently maintaining that part of the code, or is in maintainer training.
- On your Gerrit patch's page, click on the project name, or "Repo" (e.g. mediawiki/core). You will see a list of other changesets in that repository: the people who write and review those changesets may be good candidates to add as reviewers.
- In large or very active projects, try filtering this list further. For example, if you are working on the MediaWiki database code, search for "message:rdbms" to find other changes in the rdbms component (this works as long as everyone follows the commit message guidelines and includes the component name in their commit messages).
- You can also filter changes by the files they modify; for example, search for file:mediawiki.page.gallery.styles to find all changes to styles of galleries, or path:resources/src/mediawiki.page.gallery.styles/print.less to narrow your search down to just their print mode styles (you can copy the file paths from your Gerrit patch's page, using the "Copy to clipboard" icon
on the file listing). - See Gerrit search documentation for more advanced options.
- On your Gerrit patch's page, if you view the changes in one of the files by clicking on it in the listing at the bottom, you can click "Show blame" (in top-right corner) to find the previous patches affecting each line of the file. Unlike the previous methods, this often works even if the file has been renamed or if the code has been moved from another file. The authors and reviewers of those patches may be willing to review your work too.
Review more
Many eyes make bugs shallow. Read the Code review guide and help other authors by praising or criticising their commits. Comments are nonbinding, won't cause merges or rejections, and have no formal effect on the code review. But you will learn by reviewing, gain reputation, and get people to return the favor by reviewing your proposed code changes in the future. "How to review code in Gerrit" has the step-by-step explanation.
Dealing with possible obstacles
No timely feedback
Manpower in free and open source software projects is limited, and interests of developers may change. Some code repositories are more active and maintained and you will receive quicker reviews. Other areas have unclear maintainership or are even abandoned and you might have to wait for a long time.
You can check the latest activity in a code repository via git log in your local checkout.
To take over an abandoned project and become its maintainer, follow these steps.
If you think that your patch has been gone unnoticed for a longer time, feel free to bring up the problem on the #wikimedia-tech connect IRC channel.
Further reasons for rework or rejection
Even if you have followed all recommendations, your patch might still require some rework (or in rare cases even get rejected).
Apart from what has been mentioned already, there are more potential reasons for rejection (not all of them are equally decisive), such as a suboptimal solution when there is a more simple or efficient way, performance issues, security issues, improvable naming (e.g. of variables), integration conflicts with existing code, duplication of work, unintended (mis)use of the API, or proposed changes to internal APIs being considered too risky.[1]
Be aware that there is a mismatch of judgement: Patch reviewers often consider test failures, an incomplete fix, introducing new bugs, a suboptimal solution, and inconsistent docs way more decisive for rejecting a patch than patch authors.[1]
See also
- "5 tips to get your code reviewed faster"
- "Code review meeting notes" - from 2012; relevant parts are now included in the present documentation
References
- ↑ 1.0 1.1 1.2 1.3 1.4 Yida Tao; Donggyun Han; Sunghun Kim, "Writing Acceptable Patches: An Empirical Study of Open Source Project Patches," in Software Maintenance and Evolution (ICSME), 2014 IEEE International Conference on , vol., no., pp.271-280, Sept. 29 2014-Oct. 3 2014
- ↑ Peter C. Rigby, Daniel M. German, and Margaret-Anne Storey. 2008. Open source software peer review practices: a case study of the apache server. In Proceedings of the 30th international conference on Software engineering (ICSE '08). ACM, New York, NY, USA, 541-550.
- ↑ Peter Weißgerber, Daniel Neu, and Stephan Diehl. 2008. Small patches get in!. In Proceedings of the 2008 international working conference on Mining software repositories (MSR '08). ACM, New York, NY, USA, 67-76.
- ↑ Amiangshu Bosu, Michaela Greiler, and Christian Bird. 2015. Characteristics of useful code reviews: an empirical study at Microsoft. In Proceedings of the 12th Working Conference on Mining Software Repositories (MSR '15). IEEE Press, Piscataway, NJ, USA, 146-156.
- ↑ 5.0 5.1 5.2 Baysal, O.; Kononenko, O.; Holmes, R.; Godfrey, M.W., "The influence of non-technical factors on code review," in Reverse Engineering (WCRE), 2013 20th Working Conference on , vol., no., pp.122-131, 14-17 Oct. 2013