| 1 | .. _development_advancedtopics:
|
| 2 |
|
| 3 | Advanced topics
|
| 4 | ===============
|
| 5 |
|
| 6 | At this point, hopefully, you have a handle on how the development process
|
| 7 | works. There is still more to learn, however! This section will cover a
|
| 8 | number of topics which can be helpful for developers wanting to become a
|
| 9 | regular part of the Linux kernel development process.
|
| 10 |
|
| 11 | Managing patches with git
|
| 12 | -------------------------
|
| 13 |
|
| 14 | The use of distributed version control for the kernel began in early 2002,
|
| 15 | when Linus first started playing with the proprietary BitKeeper
|
| 16 | application. While BitKeeper was controversial, the approach to software
|
| 17 | version management it embodied most certainly was not. Distributed version
|
| 18 | control enabled an immediate acceleration of the kernel development
|
| 19 | project. In current times, there are several free alternatives to
|
| 20 | BitKeeper. For better or for worse, the kernel project has settled on git
|
| 21 | as its tool of choice.
|
| 22 |
|
| 23 | Managing patches with git can make life much easier for the developer,
|
| 24 | especially as the volume of those patches grows. Git also has its rough
|
| 25 | edges and poses certain hazards; it is a young and powerful tool which is
|
| 26 | still being civilized by its developers. This document will not attempt to
|
| 27 | teach the reader how to use git; that would be sufficient material for a
|
| 28 | long document in its own right. Instead, the focus here will be on how git
|
| 29 | fits into the kernel development process in particular. Developers who
|
| 30 | wish to come up to speed with git will find more information at:
|
| 31 |
|
| 32 | https://git-scm.com/
|
| 33 |
|
| 34 | https://www.kernel.org/pub/software/scm/git/docs/user-manual.html
|
| 35 |
|
| 36 | and on various tutorials found on the web.
|
| 37 |
|
| 38 | The first order of business is to read the above sites and get a solid
|
| 39 | understanding of how git works before trying to use it to make patches
|
| 40 | available to others. A git-using developer should be able to obtain a copy
|
| 41 | of the mainline repository, explore the revision history, commit changes to
|
| 42 | the tree, use branches, etc. An understanding of git's tools for the
|
| 43 | rewriting of history (such as rebase) is also useful. Git comes with its
|
| 44 | own terminology and concepts; a new user of git should know about refs,
|
| 45 | remote branches, the index, fast-forward merges, pushes and pulls, detached
|
| 46 | heads, etc. It can all be a little intimidating at the outset, but the
|
| 47 | concepts are not that hard to grasp with a bit of study.
|
| 48 |
|
| 49 | Using git to generate patches for submission by email can be a good
|
| 50 | exercise while coming up to speed.
|
| 51 |
|
| 52 | When you are ready to start putting up git trees for others to look at, you
|
| 53 | will, of course, need a server that can be pulled from. Setting up such a
|
| 54 | server with git-daemon is relatively straightforward if you have a system
|
| 55 | which is accessible to the Internet. Otherwise, free, public hosting sites
|
| 56 | (GitHub, for example) are starting to appear on the net. Established
|
| 57 | developers can get an account on kernel.org, but those are not easy to come
|
| 58 | by; see https://kernel.org/faq/ for more information.
|
| 59 |
|
| 60 | The normal git workflow involves the use of a lot of branches. Each line
|
| 61 | of development can be separated into a separate "topic branch" and
|
| 62 | maintained independently. Branches in git are cheap, there is no reason to
|
| 63 | not make free use of them. And, in any case, you should not do your
|
| 64 | development in any branch which you intend to ask others to pull from.
|
| 65 | Publicly-available branches should be created with care; merge in patches
|
| 66 | from development branches when they are in complete form and ready to go -
|
| 67 | not before.
|
| 68 |
|
| 69 | Git provides some powerful tools which can allow you to rewrite your
|
| 70 | development history. An inconvenient patch (one which breaks bisection,
|
| 71 | say, or which has some other sort of obvious bug) can be fixed in place or
|
| 72 | made to disappear from the history entirely. A patch series can be
|
| 73 | rewritten as if it had been written on top of today's mainline, even though
|
| 74 | you have been working on it for months. Changes can be transparently
|
| 75 | shifted from one branch to another. And so on. Judicious use of git's
|
| 76 | ability to revise history can help in the creation of clean patch sets with
|
| 77 | fewer problems.
|
| 78 |
|
| 79 | Excessive use of this capability can lead to other problems, though, beyond
|
| 80 | a simple obsession for the creation of the perfect project history.
|
| 81 | Rewriting history will rewrite the changes contained in that history,
|
| 82 | turning a tested (hopefully) kernel tree into an untested one. But, beyond
|
| 83 | that, developers cannot easily collaborate if they do not have a shared
|
| 84 | view of the project history; if you rewrite history which other developers
|
| 85 | have pulled into their repositories, you will make life much more difficult
|
| 86 | for those developers. So a simple rule of thumb applies here: history
|
| 87 | which has been exported to others should generally be seen as immutable
|
| 88 | thereafter.
|
| 89 |
|
| 90 | So, once you push a set of changes to your publicly-available server, those
|
| 91 | changes should not be rewritten. Git will attempt to enforce this rule if
|
| 92 | you try to push changes which do not result in a fast-forward merge
|
| 93 | (i.e. changes which do not share the same history). It is possible to
|
| 94 | override this check, and there may be times when it is necessary to rewrite
|
| 95 | an exported tree. Moving changesets between trees to avoid conflicts in
|
| 96 | linux-next is one example. But such actions should be rare. This is one
|
| 97 | of the reasons why development should be done in private branches (which
|
| 98 | can be rewritten if necessary) and only moved into public branches when
|
| 99 | it's in a reasonably advanced state.
|
| 100 |
|
| 101 | As the mainline (or other tree upon which a set of changes is based)
|
| 102 | advances, it is tempting to merge with that tree to stay on the leading
|
| 103 | edge. For a private branch, rebasing can be an easy way to keep up with
|
| 104 | another tree, but rebasing is not an option once a tree is exported to the
|
| 105 | world. Once that happens, a full merge must be done. Merging occasionally
|
| 106 | makes good sense, but overly frequent merges can clutter the history
|
| 107 | needlessly. Suggested technique in this case is to merge infrequently, and
|
| 108 | generally only at specific release points (such as a mainline -rc
|
| 109 | release). If you are nervous about specific changes, you can always
|
| 110 | perform test merges in a private branch. The git "rerere" tool can be
|
| 111 | useful in such situations; it remembers how merge conflicts were resolved
|
| 112 | so that you don't have to do the same work twice.
|
| 113 |
|
| 114 | One of the biggest recurring complaints about tools like git is this: the
|
| 115 | mass movement of patches from one repository to another makes it easy to
|
| 116 | slip in ill-advised changes which go into the mainline below the review
|
| 117 | radar. Kernel developers tend to get unhappy when they see that kind of
|
| 118 | thing happening; putting up a git tree with unreviewed or off-topic patches
|
| 119 | can affect your ability to get trees pulled in the future. Quoting Linus:
|
| 120 |
|
| 121 | ::
|
| 122 |
|
| 123 | You can send me patches, but for me to pull a git patch from you, I
|
| 124 | need to know that you know what you're doing, and I need to be able
|
| 125 | to trust things *without* then having to go and check every
|
| 126 | individual change by hand.
|
| 127 |
|
| 128 | (https://lwn.net/Articles/224135/).
|
| 129 |
|
| 130 | To avoid this kind of situation, ensure that all patches within a given
|
| 131 | branch stick closely to the associated topic; a "driver fixes" branch
|
| 132 | should not be making changes to the core memory management code. And, most
|
| 133 | importantly, do not use a git tree to bypass the review process. Post an
|
| 134 | occasional summary of the tree to the relevant list, and, when the time is
|
| 135 | right, request that the tree be included in linux-next.
|
| 136 |
|
| 137 | If and when others start to send patches for inclusion into your tree,
|
| 138 | don't forget to review them. Also ensure that you maintain the correct
|
| 139 | authorship information; the git "am" tool does its best in this regard, but
|
| 140 | you may have to add a "From:" line to the patch if it has been relayed to
|
| 141 | you via a third party.
|
| 142 |
|
| 143 | When requesting a pull, be sure to give all the relevant information: where
|
| 144 | your tree is, what branch to pull, and what changes will result from the
|
| 145 | pull. The git request-pull command can be helpful in this regard; it will
|
| 146 | format the request as other developers expect, and will also check to be
|
| 147 | sure that you have remembered to push those changes to the public server.
|
| 148 |
|
| 149 | .. _development_advancedtopics_reviews:
|
| 150 |
|
| 151 | Reviewing patches
|
| 152 | -----------------
|
| 153 |
|
| 154 | Some readers will certainly object to putting this section with "advanced
|
| 155 | topics" on the grounds that even beginning kernel developers should be
|
| 156 | reviewing patches. It is certainly true that there is no better way to
|
| 157 | learn how to program in the kernel environment than by looking at code
|
| 158 | posted by others. In addition, reviewers are forever in short supply; by
|
| 159 | looking at code you can make a significant contribution to the process as a
|
| 160 | whole.
|
| 161 |
|
| 162 | Reviewing code can be an intimidating prospect, especially for a new kernel
|
| 163 | developer who may well feel nervous about questioning code - in public -
|
| 164 | which has been posted by those with more experience. Even code written by
|
| 165 | the most experienced developers can be improved, though. Perhaps the best
|
| 166 | piece of advice for reviewers (all reviewers) is this: phrase review
|
| 167 | comments as questions rather than criticisms. Asking "how does the lock
|
| 168 | get released in this path?" will always work better than stating "the
|
| 169 | locking here is wrong."
|
| 170 |
|
| 171 | Another technique that is useful in case of a disagreement is to ask for others
|
| 172 | to chime in. If a discussion reaches a stalemate after a few exchanges,
|
| 173 | then call for opinions of other reviewers or maintainers. Often those in
|
| 174 | agreement with a reviewer remain silent unless called upon.
|
| 175 | The opinion of multiple people carries exponentially more weight.
|
| 176 |
|
| 177 | Different developers will review code from different points of view. Some
|
| 178 | are mostly concerned with coding style and whether code lines have trailing
|
| 179 | white space. Others will focus primarily on whether the change implemented
|
| 180 | by the patch as a whole is a good thing for the kernel or not. Yet others
|
| 181 | will check for problematic locking, excessive stack usage, possible
|
| 182 | security issues, duplication of code found elsewhere, adequate
|
| 183 | documentation, adverse effects on performance, user-space ABI changes, etc.
|
| 184 | All types of review, if they lead to better code going into the kernel, are
|
| 185 | welcome and worthwhile.
|
| 186 |
|
| 187 | There is no strict requirement to use specific tags like ``Reviewed-by``.
|
| 188 | In fact reviews in plain English are more informative and encouraged
|
| 189 | even when a tag is provided, e.g. "I looked at aspects A, B and C of this
|
| 190 | submission and it looks good to me."
|
| 191 | Some form of a review message or reply is obviously necessary otherwise
|
| 192 | maintainers will not know that the reviewer has looked at the patch at all!
|
| 193 |
|
| 194 | Last but not least patch review may become a negative process, focused
|
| 195 | on pointing out problems. Please throw in a compliment once in a while,
|
| 196 | particularly for newbies!
|