7.1.Managing patches with git¶

The use of distributed version control for the kernel began in early 2002,when Linus first started playing with the proprietary BitKeeperapplication. While BitKeeper was controversial, the approach to softwareversion management it embodied most certainly was not. Distributed versioncontrol enabled an immediate acceleration of the kernel developmentproject. In current times, there are several free alternatives toBitKeeper. For better or for worse, the kernel project has settled on gitas its tool of choice.

Managing patches with git can make life much easier for the developer,especially as the volume of those patches grows. Git also has its roughedges and poses certain hazards; it is a young and powerful tool which isstill being civilized by its developers. This document will not attempt toteach the reader how to use git; that would be sufficient material for along document in its own right. Instead, the focus here will be on how gitfits into the kernel development process in particular. Developers whowish to come up to speed with git will find more information at:

https://git-scm.com/

https://www.kernel.org/pub/software/scm/git/docs/user-manual.html

and on various tutorials found on the web.

The first order of business is to read the above sites and get a solidunderstanding of how git works before trying to use it to make patchesavailable to others. A git-using developer should be able to obtain a copyof the mainline repository, explore the revision history, commit changes tothe tree, use branches, etc. An understanding of git’s tools for therewriting of history (such as rebase) is also useful. Git comes with itsown terminology and concepts; a new user of git should know about refs,remote branches, the index, fast-forward merges, pushes and pulls, detachedheads, etc. It can all be a little intimidating at the outset, but theconcepts are not that hard to grasp with a bit of study.

Using git to generate patches for submission by email can be a goodexercise while coming up to speed.

When you are ready to start putting up git trees for others to look at, youwill, of course, need a server that can be pulled from. Setting up such aserver with git-daemon is relatively straightforward if you have a systemwhich is accessible to the Internet. Otherwise, free, public hosting sites(Github, for example) are starting to appear on the net. Establisheddevelopers can get an account on kernel.org, but those are not easy to comeby; seehttps://kernel.org/faq/ for more information.

The normal git workflow involves the use of a lot of branches. Each lineof development can be separated into a separate “topic branch” andmaintained independently. Branches in git are cheap, there is no reason tonot make free use of them. And, in any case, you should not do yourdevelopment in any branch which you intend to ask others to pull from.Publicly-available branches should be created with care; merge in patchesfrom development branches when they are in complete form and ready to go -not before.

Git provides some powerful tools which can allow you to rewrite yourdevelopment history. An inconvenient patch (one which breaks bisection,say, or which has some other sort of obvious bug) can be fixed in place ormade to disappear from the history entirely. A patch series can berewritten as if it had been written on top of today’s mainline, even thoughyou have been working on it for months. Changes can be transparentlyshifted from one branch to another. And so on. Judicious use of git’sability to revise history can help in the creation of clean patch sets withfewer problems.

Excessive use of this capability can lead to other problems, though, beyonda simple obsession for the creation of the perfect project history.Rewriting history will rewrite the changes contained in that history,turning a tested (hopefully) kernel tree into an untested one. But, beyondthat, developers cannot easily collaborate if they do not have a sharedview of the project history; if you rewrite history which other developershave pulled into their repositories, you will make life much more difficultfor those developers. So a simple rule of thumb applies here: historywhich has been exported to others should generally be seen as immutablethereafter.

So, once you push a set of changes to your publicly-available server, thosechanges should not be rewritten. Git will attempt to enforce this rule ifyou try to push changes which do not result in a fast-forward merge(i.e. changes which do not share the same history). It is possible tooverride this check, and there may be times when it is necessary to rewritean exported tree. Moving changesets between trees to avoid conflicts inlinux-next is one example. But such actions should be rare. This is oneof the reasons why development should be done in private branches (whichcan be rewritten if necessary) and only moved into public branches whenit’s in a reasonably advanced state.

As the mainline (or other tree upon which a set of changes is based)advances, it is tempting to merge with that tree to stay on the leadingedge. For a private branch, rebasing can be an easy way to keep up withanother tree, but rebasing is not an option once a tree is exported to theworld. Once that happens, a full merge must be done. Merging occasionallymakes good sense, but overly frequent merges can clutter the historyneedlessly. Suggested technique in this case is to merge infrequently, andgenerally only at specific release points (such as a mainline -rcrelease). If you are nervous about specific changes, you can alwaysperform test merges in a private branch. The git “rerere” tool can beuseful in such situations; it remembers how merge conflicts were resolvedso that you don’t have to do the same work twice.

One of the biggest recurring complaints about tools like git is this: themass movement of patches from one repository to another makes it easy toslip in ill-advised changes which go into the mainline below the reviewradar. Kernel developers tend to get unhappy when they see that kind ofthing happening; putting up a git tree with unreviewed or off-topic patchescan affect your ability to get trees pulled in the future. Quoting Linus:

You can send me patches, but for me to pull a git patch from you, Ineed to know that you know what you're doing, and I need to be ableto trust things *without* then having to go and check everyindividual change by hand.

(https://lwn.net/Articles/224135/).

To avoid this kind of situation, ensure that all patches within a givenbranch stick closely to the associated topic; a “driver fixes” branchshould not be making changes to the core memory management code. And, mostimportantly, do not use a git tree to bypass the review process. Post anoccasional summary of the tree to the relevant list, and, when the time isright, request that the tree be included in linux-next.

If and when others start to send patches for inclusion into your tree,don’t forget to review them. Also ensure that you maintain the correctauthorship information; the git “am” tool does its best in this regard, butyou may have to add a “From:” line to the patch if it has been relayed toyou via a third party.

When requesting a pull, be sure to give all the relevant information: whereyour tree is, what branch to pull, and what changes will result from thepull. The git request-pull command can be helpful in this regard; it willformat the request as other developers expect, and will also check to besure that you have remembered to push those changes to the public server.

7.2.Reviewing patches¶

Some readers will certainly object to putting this section with “advancedtopics” on the grounds that even beginning kernel developers should bereviewing patches. It is certainly true that there is no better way tolearn how to program in the kernel environment than by looking at codeposted by others. In addition, reviewers are forever in short supply; bylooking at code you can make a significant contribution to the process as awhole.

Reviewing code can be an intimidating prospect, especially for a new kerneldeveloper who may well feel nervous about questioning code - in public -which has been posted by those with more experience. Even code written bythe most experienced developers can be improved, though. Perhaps the bestpiece of advice for reviewers (all reviewers) is this: phrase reviewcomments as questions rather than criticisms. Asking “how does the lockget released in this path?” will always work better than stating “thelocking here is wrong.”

Another technique that is useful in case of a disagreement is to ask for othersto chime in. If a discussion reaches a stalemate after a few exchanges,then call for opinions of other reviewers or maintainers. Often those inagreement with a reviewer remain silent unless called upon.The opinion of multiple people carries exponentially more weight.

Different developers will review code from different points of view. Someare mostly concerned with coding style and whether code lines have trailingwhite space. Others will focus primarily on whether the change implementedby the patch as a whole is a good thing for the kernel or not. Yet otherswill check for problematic locking, excessive stack usage, possiblesecurity issues, duplication of code found elsewhere, adequatedocumentation, adverse effects on performance, user-space ABI changes, etc.All types of review, if they lead to better code going into the kernel, arewelcome and worthwhile.

There is no strict requirement to use specific tags likeReviewed-by.In fact reviews in plain English are more informative and encouragedeven when a tag is provided, e.g. “I looked at aspects A, B and C of thissubmission and it looks good to me.”Some form of a review message or reply is obviously necessary otherwisemaintainers will not know that the reviewer has looked at the patch at all!

Last but not least patch review may become a negative process, focusedon pointing out problems. Please throw in a compliment once in a while,particularly for newbies!