Working with reviewers

A patch of any significance will result in a number of comments from other developers as they review the code. Working with reviewers can be, for many developers, the most intimidating part of the kernel development process. Life can be made much easier, though, if you keep a few things in mind: 

  • If you have explained your patch well, reviewers will understand its value and why you went to the trouble of writing it. But that value will not keep them from asking a fundamental question: what will it be like to maintain a kernel with this code in it five or ten years later? Many of the changes you may be asked to make – from coding style tweaks to substantial rewrites – come from the understanding that Linux will still be around and under development a decade from now.
  • Code review is hard work, and it is a relatively thankless occupation; people remember who wrote kernel code, but there is little lasting fame for those who reviewed it. So reviewers can get grumpy, especially when they see the same mistakes being made over and over again. If you get a review which seems angry, insulting, or outright offensive, resist the impulse to respond in kind. Code review is about the code, not about the people, and code reviewers are not attacking you personally.
  • Similarly, code reviewers are not trying to promote their employers‘ agendas at the expense of your own. Kernel developers often expect to be working on the kernel years from now, but they understand that their employer could change. They truly are, almost without exception, working toward the creation of the best kernel they can; they are not trying to create discomfort for their employers‘ competitors.

What all of this comes down to is that, when reviewers send you comments, you need to pay attention to the technical observations that they are making. Do not let their form of expression or your own pride keep that from happening. When you get review comments on a patch, take the time to understand what the reviewer is trying to say. If possible, fix the things that the reviewer is asking you to fix. And respond back to the reviewer: thank them, and describe how you will answer their questions.

Note that you do not have to agree with every change suggested by reviewers. If you believe that the reviewer has misunderstood your code, explain what is really going on. If you have a technical objection to a suggested change, describe it and justify your solution to the problem. If your explanations make sense, the reviewer will accept them. Should your explanation not prove persuasive, though, especially if others start to agree with the reviewer, take some time to think things over again. It can be easy to become blinded by your own solution to a problem to the point that you don’t realize that something is fundamentally wrong or, perhaps, you’re not even solving the right problem.

One fatal mistake is to ignore review comments in the hope that they will go away. They will not go away. If you repost code without having responded to the comments you got the time before, you’re likely to find that your patches go nowhere.

Speaking of reposting code: please bear in mind that reviewers are not going to remember all the details of the code you posted the last time around. So it is always a good idea to remind reviewers of previously raised issues and how you dealt with them; the patch changelog is a good place for this kind of information. Reviewers should not have to search through list archives to familiarize themselves with what was said last time; if you help them get a running start, they will be in a better mood when they revisit your code.

What if you’ve tried to do everything right and things still aren’t going anywhere? Most technical disagreements can be resolved through discussion, but there are times when somebody simply has to make a decision. If you honestly believe that this decision is going against you wrongly, you can always try appealing to a higher power. As of this writing, that higher power tends to be Andrew Morton. Andrew has a great deal of respect in the kernel development community; he can often unjam a situation which seems to be hopelessly blocked. Appealing to Andrew should not be done lightly, though, and not before all other alternatives have been explored. And bear in mind, of course, that he may not agree with you either.