Review

Why do reviews at all?

1. As a practice of analytic quality assurance, reviews hinder defects from being introduced into the product before it's released to the end-user. The later a defect is noticed, found, and eliminated the costlier it gets. Finding bugs as early as it's (often) achieved by reviews also prevents other developers from building new patches on top of faulty code, which can be a huge pain saver.
2. Furthermore, reviews can be regarded as constructive quality assurance as well. That's because of the implicit knowledge transfer that takes place from the authoring developer to the reviewers, and vice versa. The author is encouraged to explain his thoughts to the reviewers, to justify the design decisions he made, etc. thus increasing the "truck factor" of the project -- because there are simply more people who possess knowledge of this particular part of the product. The other way around, the reviewers might be more skilled in certain areas (or simply more attentive) and may provide valuable feedback, helping the author to write better code -- now and in the future.

Content

1. Author's perspective: How to prepare your changes for a review
2. Reviewer's perspective: How to perform a review

How to prepare your changes for a review

The basic process is as follows (details follow below):

1. You create one local commit containing your changes.
2. You create a review request by pushing your commit to Gerrit.
3. Wait for Jenkins to build and test your changes.
   • If Jenkins complains: Figure out what went wrong, amend your commit and push it to Gerrit again. Don't proceed with the next step until you have a successful compilation and passed tests.
4. Invite other developers to review your changes. (In the meantime: Perform two reviews of your own.)
5. Make sure you understand the issues reported by your reviewers and discuss the possible solutions.
   • Otherwise you will probably end up reworking your rework.
6. Integrate the suggestions from the reviewers in a new patch set and repeat at step 3.
7. You are allowed to submit your patch if you have positive votes and no negative votes.
   • At least two +1 votes OR one single +1 vote that is two working days old.
   • Any negative vote is a hard veto. You have to address the issues pointed out by your reviewers first.

1. Creating your commit

Nitty-gritty Git aspects

• Checkout the current master before you start working (either through a git fetch/checkout combination or git pull). If your basis is too old, the review system won't complain immediately and the patch might look fine. But if relevant parts of the source did change in the meantime, your patch won't be applicable and you'll need to rebase it.
• Don't push merge commits: Saros follows a single-branch policy, meaning we allow branches only for special occasions. Don't be seduced by GitHub's Pull Request model -- that's not how Gerrit works. A Google search leads you to several good explanations of the differences.

Scope of your patch

• Smaller patches are always better. If it takes more than 15 minutes to review your patch, it is very unlikely that anyone is going to review it and your changes won't get in the code base. Split larger patches into independent smaller ones. (To facilitate this, we allow patches that add code that is unused at the time of addition. The commit message has to clearly state which larger change this patch contributes to and announce follow-up patches).
• The same goes for independent changes. E.g. don't mix up (automatic) refactorings and functional changes in one patch, but provide two separate ones -- both of them will be easier to review.

Format of your patch

• Write a concise commit message following our guidelines. If you have trouble finding one single descriptive type tag for your commit, you probably mixed up different concerns and should address them in separate patches instead.

2./3./6. Pushing and Pushing again

In case you don't know yet:

• Learn how to push your first change.
• Learn how to amend an existing patch.
• If you want to learn more: Read the Gerrit documentation.

Don't rebase

When you create a new version (= patch set) of your change, please refrain from changing its parent. This happens when you rebase your commit onto a more recent state of the master, or when you're working with a chain of commits and are tempted to use git rebase -i.

Gerrit offers the possibility to compare different patch sets (to see the differences between set 2 and 3, for example). This feature breaks when you change the parent commit (it then includes all differences of the respective parents).

So don't rebase until you really need to. You are allowed to rebase in these cases:

1. There were functional changes on the master branch and you need to adapt your patch to them.
2. There were major changes in other pending patches that your patch relies on.
3. You wanted to submit your patch, but Gerrit tells you that you need to rebase first (see below, "7. Submitting your patch").

4. Invite Reviewers

• You can invite individual reviewers by their names.
  • Take a look into the commit history to find some developers who might be knowledgeable about the source code area you touched.
  • Send a short notice on the mailing list containing the URL to your patch if you're unsure who to invite (see here for an example).
• You can invite a group of reviewers (how to find the groups you are in).
• If you find that your patch is complex and not easy to understand by other developers, you should offer to conduct a pair review session -- either in a co-located fashion or using Saros itself (eat your own dogfood, right?).
  • Let the reviewer set the pace and directions. If you -- as the patch's author -- would drive, things would probably go too fast.
  • Even if you perform a review in such a pair session, the reviewer should still post his remarks in Gerrit, so others can learn from the mistakes and defects you two found.
• Since we require two reviewers for each patch, everytime you submit a new patch (or new version of a patch) you need to do at least two reviews yourself to maintain that ratio. Having a hard time getting others to review your patch? Be a role model and do a few reviews yourself.

6. While integrating the reviewers' suggestions: Avoid perfectionism

• Ever heard of the "Good Enough Principle"? Resist the temptation to correct even the last typo in every class your patch touched. Finish your actual patch and consider follow-up patches for any additional things.
• Also resist your reviewers: If you start working on things that could as easily be done in another commit (or even should, e.g. if a refactoring commit gradually becomes more than that), stand your ground and focus the discussion on the actual patch, not some imaginary perfect patch-to-be.

7. Submitting your patch

• Normally, submitting an approved patch is the duty of the patch author, i.e. you.
  • Vice versa, don't just simply submit foreign patches even if they meet the submission criteria. Talk to the patch author (usually the owner) first.
• See above for the rules to when you are allowed to do so. However, if you are new to the project, you'll probably not have the permission to submit a patch.
  • If this is one of your very first patches, ask someone else to submit your patch.
  • If you think you gained enough experience (this includes understanding and accepting the consequences, see next section), you can ask for "Submit" permission on the mailing list.
• Technically, a patch in Gerrit needs a "+2" before the "Submit" button appears. In case no reviewer gave you a "+2", you are allowed to give your own patch a "+2" if the contraints for submitting patches are satisfied.
• If Gerrit asks you to rebase your change when you tried to submit it, please make sure you rebase on the current master.

What happens next?

As soon as your patch hits the master branch, multiple Jenkins jobs will perform different tasks at different times. The fastet jobs run every hour, the slowest runs daily, roughly 6 o'clock in the morning -- so it can take up to 24 hours until integration problems pop up. If any come up, you and all other developers will be notified on the mailing list. Any commit that breaks the CI will be reverted.

How to perform a review

To get an equal distribution of work load in the Saros community, everyone who submits patches needs to do twice as many reviews of foreign patches. Every time you submit a new patch or patch set for review, you owe the team two reviews of your own. This section describes the most important things about doing a review.

• See here for an explanation of how to use the Gerrit web interface for reviewing.

How to look for issues

A review process becomes more effective if the reviewers take different perspectives on the code. Here are just two of them:

Attention: Not all problems can be found by just looking at the difference between old and new code. So as a rule of thumb, whenever a patch introduces new structural elements (new methods, interfaces, classes) checkout the patch and take a look at the bigger picture: Are there already other methods, interfaces, classes for this or a similar job? Can the same goal be achieved using existing components of Saros?

How to report the issues found

• Explain the problem: Don't just point to a problem and expect the author to understand what you are talking about. If it really was so obvious, the author probably wouldn't have made the mistake in the first place. So, make sure it's really clear what the problem is (and expect follow-up questions of the author).
• Propose a solution: Don't stop with the analysis. Instead, give advice on how to fix the problem and describe well what you would expect of a solution. Otherwise the author is likely to solve the problem in a way you didn't expect, leading to avoidable rework.
  • If you can't think of a solution yourself, at least do the second part and formulate your requirements.
• Provide background information: If suitable (and possible), provide some background information by URL -- either for the analytical part (explaining why something is a problem) or for the constructive part (explaining how to solve such problems).

How to handle recurring problems

• Explain the schema, not all instances: Some types of errors occur in multiple places and it's certainly not the reviewer's job to find all instances. Just find enough instances to make the pattern clear to the author. One reported instance might be enough; just make sure to tell the author to look out for more of that kind.

How to cope with a lack of understanding

• Don't be afraid to ask if you don't understand some parts of the code. Nobody requires the reviewer to be smarter than the author -- just different.

How to hook into the ongoing review process

• If you are not the first reviewer, there are probably already some comments of others. There are different kinds:
  • The ones you totally agree with: If you think the point is relevant, make clear you agree with your predecessor.
  • The ones you don't agree with: Make clear whether you don't agree with the other reviewer's (or author's) analysis of the problem or construction of the solution (or the requirements thereof).
  • The ones you don't understand: Some review comments are hard to understand (even if the reviewers put effort into it). Help clarify open issues when you see them. Even if you're not the author, it might be helpful to ask what a specific issue is about. Chances are that the author does not get the reviewer's point either.
• In general: If you review a patch, let the author know what you think of it, even if you don't have anything substantial to add to the things previous reviewers already said. Otherwise the author has no way of telling how many reviewers already looked and didn't find anything suspicious: one or four?