| Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 1 | 6: FOLLOWTHROUGH | 
|  | 2 |  | 
|  | 3 | At this point, you have followed the guidelines given so far and, with the | 
|  | 4 | addition of your own engineering skills, have posted a perfect series of | 
|  | 5 | patches.  One of the biggest mistakes that even experienced kernel | 
|  | 6 | developers can make is to conclude that their work is now done.  In truth, | 
|  | 7 | posting patches indicates a transition into the next stage of the process, | 
|  | 8 | with, possibly, quite a bit of work yet to be done. | 
|  | 9 |  | 
|  | 10 | It is a rare patch which is so good at its first posting that there is no | 
|  | 11 | room for improvement.  The kernel development process recognizes this fact, | 
|  | 12 | and, as a result, is heavily oriented toward the improvement of posted | 
|  | 13 | code.  You, as the author of that code, will be expected to work with the | 
|  | 14 | kernel community to ensure that your code is up to the kernel's quality | 
|  | 15 | standards.  A failure to participate in this process is quite likely to | 
|  | 16 | prevent the inclusion of your patches into the mainline. | 
|  | 17 |  | 
|  | 18 |  | 
|  | 19 | 6.1: WORKING WITH REVIEWERS | 
|  | 20 |  | 
|  | 21 | A patch of any significance will result in a number of comments from other | 
|  | 22 | developers as they review the code.  Working with reviewers can be, for | 
|  | 23 | many developers, the most intimidating part of the kernel development | 
|  | 24 | process.  Life can be made much easier, though, if you keep a few things in | 
|  | 25 | mind: | 
|  | 26 |  | 
|  | 27 | - If you have explained your patch well, reviewers will understand its | 
|  | 28 | value and why you went to the trouble of writing it.  But that value | 
|  | 29 | will not keep them from asking a fundamental question: what will it be | 
|  | 30 | like to maintain a kernel with this code in it five or ten years later? | 
|  | 31 | Many of the changes you may be asked to make - from coding style tweaks | 
|  | 32 | to substantial rewrites - come from the understanding that Linux will | 
|  | 33 | still be around and under development a decade from now. | 
|  | 34 |  | 
|  | 35 | - Code review is hard work, and it is a relatively thankless occupation; | 
|  | 36 | people remember who wrote kernel code, but there is little lasting fame | 
|  | 37 | for those who reviewed it.  So reviewers can get grumpy, especially when | 
|  | 38 | they see the same mistakes being made over and over again.  If you get a | 
|  | 39 | review which seems angry, insulting, or outright offensive, resist the | 
|  | 40 | impulse to respond in kind.  Code review is about the code, not about | 
|  | 41 | the people, and code reviewers are not attacking you personally. | 
|  | 42 |  | 
|  | 43 | - Similarly, code reviewers are not trying to promote their employers' | 
|  | 44 | agendas at the expense of your own.  Kernel developers often expect to | 
|  | 45 | be working on the kernel years from now, but they understand that their | 
|  | 46 | employer could change.  They truly are, almost without exception, | 
|  | 47 | working toward the creation of the best kernel they can; they are not | 
|  | 48 | trying to create discomfort for their employers' competitors. | 
|  | 49 |  | 
|  | 50 | What all of this comes down to is that, when reviewers send you comments, | 
|  | 51 | you need to pay attention to the technical observations that they are | 
|  | 52 | making.  Do not let their form of expression or your own pride keep that | 
|  | 53 | from happening.  When you get review comments on a patch, take the time to | 
|  | 54 | understand what the reviewer is trying to say.  If possible, fix the things | 
|  | 55 | that the reviewer is asking you to fix.  And respond back to the reviewer: | 
|  | 56 | thank them, and describe how you will answer their questions. | 
|  | 57 |  | 
|  | 58 | Note that you do not have to agree with every change suggested by | 
|  | 59 | reviewers.  If you believe that the reviewer has misunderstood your code, | 
|  | 60 | explain what is really going on.  If you have a technical objection to a | 
|  | 61 | suggested change, describe it and justify your solution to the problem.  If | 
|  | 62 | your explanations make sense, the reviewer will accept them.  Should your | 
|  | 63 | explanation not prove persuasive, though, especially if others start to | 
|  | 64 | agree with the reviewer, take some time to think things over again.  It can | 
|  | 65 | be easy to become blinded by your own solution to a problem to the point | 
|  | 66 | that you don't realize that something is fundamentally wrong or, perhaps, | 
|  | 67 | you're not even solving the right problem. | 
|  | 68 |  | 
|  | 69 | One fatal mistake is to ignore review comments in the hope that they will | 
|  | 70 | go away.  They will not go away.  If you repost code without having | 
|  | 71 | responded to the comments you got the time before, you're likely to find | 
|  | 72 | that your patches go nowhere. | 
|  | 73 |  | 
|  | 74 | Speaking of reposting code: please bear in mind that reviewers are not | 
|  | 75 | going to remember all the details of the code you posted the last time | 
|  | 76 | around.  So it is always a good idea to remind reviewers of previously | 
|  | 77 | raised issues and how you dealt with them; the patch changelog is a good | 
|  | 78 | place for this kind of information.  Reviewers should not have to search | 
|  | 79 | through list archives to familiarize themselves with what was said last | 
|  | 80 | time; if you help them get a running start, they will be in a better mood | 
|  | 81 | when they revisit your code. | 
|  | 82 |  | 
|  | 83 | What if you've tried to do everything right and things still aren't going | 
|  | 84 | anywhere?  Most technical disagreements can be resolved through discussion, | 
|  | 85 | but there are times when somebody simply has to make a decision.  If you | 
|  | 86 | honestly believe that this decision is going against you wrongly, you can | 
|  | 87 | always try appealing to a higher power.  As of this writing, that higher | 
|  | 88 | power tends to be Andrew Morton.  Andrew has a great deal of respect in the | 
|  | 89 | kernel development community; he can often unjam a situation which seems to | 
|  | 90 | be hopelessly blocked.  Appealing to Andrew should not be done lightly, | 
|  | 91 | though, and not before all other alternatives have been explored.  And bear | 
|  | 92 | in mind, of course, that he may not agree with you either. | 
|  | 93 |  | 
|  | 94 |  | 
|  | 95 | 6.2: WHAT HAPPENS NEXT | 
|  | 96 |  | 
|  | 97 | If a patch is considered to be a good thing to add to the kernel, and once | 
|  | 98 | most of the review issues have been resolved, the next step is usually | 
|  | 99 | entry into a subsystem maintainer's tree.  How that works varies from one | 
|  | 100 | subsystem to the next; each maintainer has his or her own way of doing | 
|  | 101 | things.  In particular, there may be more than one tree - one, perhaps, | 
|  | 102 | dedicated to patches planned for the next merge window, and another for | 
|  | 103 | longer-term work. | 
|  | 104 |  | 
|  | 105 | For patches applying to areas for which there is no obvious subsystem tree | 
|  | 106 | (memory management patches, for example), the default tree often ends up | 
|  | 107 | being -mm.  Patches which affect multiple subsystems can also end up going | 
|  | 108 | through the -mm tree. | 
|  | 109 |  | 
|  | 110 | Inclusion into a subsystem tree can bring a higher level of visibility to a | 
|  | 111 | patch.  Now other developers working with that tree will get the patch by | 
|  | 112 | default.  Subsystem trees typically feed into -mm and linux-next as well, | 
|  | 113 | making their contents visible to the development community as a whole.  At | 
|  | 114 | this point, there's a good chance that you will get more comments from a | 
|  | 115 | new set of reviewers; these comments need to be answered as in the previous | 
|  | 116 | round. | 
|  | 117 |  | 
|  | 118 | What may also happen at this point, depending on the nature of your patch, | 
|  | 119 | is that conflicts with work being done by others turn up.  In the worst | 
|  | 120 | case, heavy patch conflicts can result in some work being put on the back | 
|  | 121 | burner so that the remaining patches can be worked into shape and merged. | 
|  | 122 | Other times, conflict resolution will involve working with the other | 
|  | 123 | developers and, possibly, moving some patches between trees to ensure that | 
|  | 124 | everything applies cleanly.  This work can be a pain, but count your | 
|  | 125 | blessings: before the advent of the linux-next tree, these conflicts often | 
|  | 126 | only turned up during the merge window and had to be addressed in a hurry. | 
|  | 127 | Now they can be resolved at leisure, before the merge window opens. | 
|  | 128 |  | 
|  | 129 | Some day, if all goes well, you'll log on and see that your patch has been | 
|  | 130 | merged into the mainline kernel.  Congratulations!  Once the celebration is | 
|  | 131 | complete (and you have added yourself to the MAINTAINERS file), though, it | 
|  | 132 | is worth remembering an important little fact: the job still is not done. | 
|  | 133 | Merging into the mainline brings its own challenges. | 
|  | 134 |  | 
|  | 135 | To begin with, the visibility of your patch has increased yet again.  There | 
|  | 136 | may be a new round of comments from developers who had not been aware of | 
|  | 137 | the patch before.  It may be tempting to ignore them, since there is no | 
|  | 138 | longer any question of your code being merged.  Resist that temptation, | 
|  | 139 | though; you still need to be responsive to developers who have questions or | 
|  | 140 | suggestions. | 
|  | 141 |  | 
|  | 142 | More importantly, though: inclusion into the mainline puts your code into | 
|  | 143 | the hands of a much larger group of testers.  Even if you have contributed | 
|  | 144 | a driver for hardware which is not yet available, you will be surprised by | 
|  | 145 | how many people will build your code into their kernels.  And, of course, | 
|  | 146 | where there are testers, there will be bug reports. | 
|  | 147 |  | 
|  | 148 | The worst sort of bug reports are regressions.  If your patch causes a | 
|  | 149 | regression, you'll find an uncomfortable number of eyes upon you; | 
|  | 150 | regressions need to be fixed as soon as possible.  If you are unwilling or | 
|  | 151 | unable to fix the regression (and nobody else does it for you), your patch | 
|  | 152 | will almost certainly be removed during the stabilization period.  Beyond | 
|  | 153 | negating all of the work you have done to get your patch into the mainline, | 
|  | 154 | having a patch pulled as the result of a failure to fix a regression could | 
|  | 155 | well make it harder for you to get work merged in the future. | 
|  | 156 |  | 
|  | 157 | After any regressions have been dealt with, there may be other, ordinary | 
|  | 158 | bugs to deal with.  The stabilization period is your best opportunity to | 
|  | 159 | fix these bugs and ensure that your code's debut in a mainline kernel | 
|  | 160 | release is as solid as possible.  So, please, answer bug reports, and fix | 
|  | 161 | the problems if at all possible.  That's what the stabilization period is | 
|  | 162 | for; you can start creating cool new patches once any problems with the old | 
|  | 163 | ones have been taken care of. | 
|  | 164 |  | 
|  | 165 | And don't forget that there are other milestones which may also create bug | 
|  | 166 | reports: the next mainline stable release, when prominent distributors pick | 
|  | 167 | up a version of the kernel containing your patch, etc.  Continuing to | 
|  | 168 | respond to these reports is a matter of basic pride in your work.  If that | 
|  | 169 | is insufficient motivation, though, it's also worth considering that the | 
|  | 170 | development community remembers developers who lose interest in their code | 
|  | 171 | after it's merged.  The next time you post a patch, they will be evaluating | 
|  | 172 | it with the assumption that you will not be around to maintain it | 
|  | 173 | afterward. | 
|  | 174 |  | 
|  | 175 |  | 
|  | 176 | 6.3: OTHER THINGS THAT CAN HAPPEN | 
|  | 177 |  | 
|  | 178 | One day, you may open your mail client and see that somebody has mailed you | 
|  | 179 | a patch to your code.  That is one of the advantages of having your code | 
|  | 180 | out there in the open, after all.  If you agree with the patch, you can | 
|  | 181 | either forward it on to the subsystem maintainer (be sure to include a | 
|  | 182 | proper From: line so that the attribution is correct, and add a signoff of | 
|  | 183 | your own), or send an Acked-by: response back and let the original poster | 
|  | 184 | send it upward. | 
|  | 185 |  | 
|  | 186 | If you disagree with the patch, send a polite response explaining why.  If | 
|  | 187 | possible, tell the author what changes need to be made to make the patch | 
|  | 188 | acceptable to you.  There is a certain resistance to merging patches which | 
|  | 189 | are opposed by the author and maintainer of the code, but it only goes so | 
|  | 190 | far.  If you are seen as needlessly blocking good work, those patches will | 
|  | 191 | eventually flow around you and get into the mainline anyway.  In the Linux | 
|  | 192 | kernel, nobody has absolute veto power over any code.  Except maybe Linus. | 
|  | 193 |  | 
|  | 194 | On very rare occasion, you may see something completely different: another | 
|  | 195 | developer posts a different solution to your problem.  At that point, | 
|  | 196 | chances are that one of the two patches will not be merged, and "mine was | 
|  | 197 | here first" is not considered to be a compelling technical argument.  If | 
|  | 198 | somebody else's patch displaces yours and gets into the mainline, there is | 
|  | 199 | really only one way to respond: be pleased that your problem got solved and | 
|  | 200 | get on with your work.  Having one's work shoved aside in this manner can | 
|  | 201 | be hurtful and discouraging, but the community will remember your reaction | 
|  | 202 | long after they have forgotten whose patch actually got merged. |