Monday, December 28, 2009

In the last few weeks, I've had a surprising number of discussions about commit messages. Many of them were with developers new to a project, trying to get them started. So here's a list of things you should do when committing, and why you should do it. Hint: the linux kernel mailing list gets it right, go there to learn.

Any software project is a collaborative project. It has at least two developers, the original developer and the original developer a few weeks or months later when the train of thought has long left the station. This later self needs to reestablish the context of a particular piece of code each time a new bug occurs or a new feature needs to be implemented.

Re-establishing the context of a piece of code is wasteful. We can't avoid it completely, so our efforts should go to reducing it to as small as possible. Commit messages can do exactly that and as a result, a commit message shows whether a developer is a good collaborator.

A good commit message should answer three questions about a patch:

Why is it necessary? It may fix a bug, it may add a feature, it may improve performance, reliabilty, stability, or just be a change for the sake of correctness.

How does it address the issue? For short obvious patches this part can be omitted, but it should be a high level description of what the approach was.

What effects does the patch have? (In addition to the obvious ones, this may include benchmarks, side effects, etc.)

These three questions establish the context for the actual code changes, put reviewers and others into the frame of mind to look at the diff and check if the approach chosen was correct. A good commit message also helps maintainers to decide if a given patch is suitable for stable branches or inclusion in a distribution.

A patch without these questions answered is mostly useless. The burden for such a patch is on each and every reviewer to find out what the patch does and how it fixes a given issue. Given a large number of reviewers and a sufficiently complex patch, this means many man-hours get wasted just because the original developer did not write a good commit message. Worse, if the maintainers of the project enforce SCM discipline, they will reject the patch and the developer needs to spend time again to rewrite the patch, reviewers spend time reviewing it again, etc. The time wasted quickly multiplies and given that a commit message only takes a few minutes to write, it is simply not economically viable to omit them or do them badly.

Consider this is a hint for proprietary software companies too - not having decent SCM discipline costs money!

How to do it better

There's no strict definition of the ideal commit message, but some general rules have emerged.A commit should contain exactly one logical change. A logical change includes adding a new feature, fixing a specific bug, etc. If it's not possible to describe the high level change in a few words, it is most likely too complex for a single commit. The diff itself should be as concise as reasonably possibly and it's almost always better to err on the side of too many patches than too few. As a rule of thumb, given only the commit message, another developer should be able to implement the same patch in a reasonable amount of time.

If you're using git, get familiar with "git add -p" (or -i) to split up changes into logical commits.

The git commit format

If you're submitting patches for git, the format is mostly standardised. A short one-line summary of the change (the maximum length of the line differs between projects, it's usually somewhere between 50 and 78 characters). This is the line that'll be seen most often, make it count. Many git tools are in one way or another optimised for this format. After that one-line summary, an empty line, then multiple paragraphs explaining the patch in detail (if needed). Don't describe the code, describe the intent and the approach. And keep the log in a present tense.

Learn to love the log

I have used CVS (and SVN to a lesser extent) in the past and log was a tool that was hardly ever used. Mostly because it was pretty useless, both the tool and the information available. These days I look at git logs more often than at code. The git log tool is vastly superior to CVS log and the commit discipline in the projects I'm working on now is a lot better. I grep git logs more often than code files and I use git blame all the time to figure out why a particular piece of code looks the way it does. It's certainly saving me a lot of time and effort. It's come to the point where the most annoying X server bugs are the ones where the git history stops at the original import from XFree86. If you're not using your SCM's log tool yet, I recommend to get more familiar with it.

How not to do it

There's a bunch of common sins that are committed (yay, a pun!) regularly.

SCM is not a backup system! My personal pet hate. Developers who use it as such tend to do end-of-day commits, checking in everything at the end of the day. The result is useless, a random diff across the code with changes that are impossible to understand by anyone including the original author once a few months have passed. (On this note: universities, please stop teaching this crap).

Per-file commit. More often than not a logical change affects more than one file and it should not be split up into two commits.

Lazy commit messages, any commit labelled as "misc fixes and cleanups" or similar. I've seen my fair share of those on non-FOSS projects and they always come back to bite you. Impossible to find when a bug was introduced, hard to bisect and makes it harder for anyone else to keep track of what's happening in the project.

Two changes in one patch. Something like "Fixed bug 2345 and renamed all foo to bar". Unless bug 2345 required the renaming, fixes whould be split it up into multiple patches. Others may have to take one of those bug fixes and apply it to a stable branch but not the other one. Picking bad patches apart into useful chunks is one of the most time-consuming and frustrating things I've done since it doesn't actually add any value to the project.

Whitespace changes together with code changes. Needle in a haystack is a fun game, but not when you're looking at patches. It's a great way to introduce bugs, though because almost no-one will spot the bug hidden in hundreds of lines that got reindented for fun and profit.

The ever-so-lovely code drops. Patches with hundreds of lines of code to dump a new feature into the code while at the same time rewriting half the existing infrastructure to support this feature. As a result, those hundreds of lines of code need to be reviewed every time a bug is discovered that is somehow related to that area of code.It's easier and less time consuming to first rework the infrastructure one piece at a time, then plug the new feature on top. As a side-effect, if a project relies on code dumps too often it's discouraging outside developers. Would you like to contribute to a project where the time spent filtering the signal from the noise outweighs the actual contribution to the code?

Unrelated whitespace changes in patches. A reviewer needs to get the big picture of a patch into their brains. Whitespace-only hunks just confuse, a reviewer has to look extra hard to check if there's a real change or whether it can be ignored. That's not so bad for empty lines added or removed,it's really bad for indentation changes.

There's plenty of excuses for the above, with the favourite one being "but it works!". It may work, but code is not a static thing. In a few weeks time, that code may have moved, been rewritten, may be called in a different manner or may have a bug in it. At the same time, the original developer may have moved on and no-one knows why the code is that way. In the worst case, everyone is afraid of touching it because nobody knows how it actually works.

Another common excuse is the "but I'm the only one working on it". Not true, any software project is a collaborative project (see above). Assuming that there's no-one else is simply short-sighted. In FOSS projects specifically we rely on outside contributors, be it testers, developers, triagers, users, etc. The harder it becomes for them to join, the more likely the project will fail.

Another, less common excuse these days is that the SCM used is too slow. Distributed SCMs avoid this issue, saving time and by inference money.

Thursday, December 24, 2009

Ran into a new bug? Wasn't there with the last version? Want a developer to fix it? Too easy, just bisect it. And here's how:

First of all, we assume you've been running whatever your distribution ships. Find out that version and find the upstream repository. Now clone that repository and check out the matching version. For the example below, let's assume 1.2 is fine and 1.3 is broken.

You can get the configure flags from your distribution's build system. e.g. Fedora's koji. Look up the build of the version, go into the build.log and copy the configure flags from there. After that, the project will be installed in the same location, with the same flags as the rpm/deb/whatever file.Verify it's still broken with this version, if not it might be distribution-specific patches. If it's still broken, check out the working version and verify that one too.

$> git checkout project-1.2 # the working version$> make && make install# test

So, you've just verified that vanilla 1.2 works and 1.3 doesn't? Great, off to bisecting.

git now gives you a version in between those two, simply build and install it, test it and depending on whether it works or not, run "git bisect good" or "git bisect bad". After a number of test runs, git will tell you which commit introduced the bug. And you're done, send this commit info to the developers and they'll have a much easier time fixing the bug.

Now, of course nothing is as easy as it seems so you may see versions that you can't build (git bisect skip) or you'll run into dependency issues between versions so you can't bisect further. I've had some reasonable success with the process above and the smaller the range of commits you can provide to the developers the better.

You can use the same process for distribution-specific patches too. After checking out a a working version, just apply all patches from that distro in-order and then bisect between the working version and HEAD. Works like a treat.

Friday, December 11, 2009

Hello, and welcome to this week's episode of "confusing wordcronyms". This time we'll have a look at letter combinations starting with X, ending in org or 86, and referring to a popular windowing system. Hint: the "X Window System" is an umbrella term for a windowing system based on the X protocol.

Here we go:

X

preferred shorthand for lazy typers or people with most of their keyboard missing. Refers to some incarnation of the X Window System. Is also a symlink to the binary of X servers.

X.Org

Official name of the X.Org project. "The X.Org project provides an open source implementation of the X Window System. [...] The X.Org Foundation is the educational non-profit corporation whose Board serves this effort, and whose Members lead this work." [source]

x.org

URL used by the X.Org project. Sometimes used to refer to X.Org by those with a broken Shift key.

XFree86

"The XFree86 Project, Inc is a global volunteer organization which produces XFree86®, the freely redistributable open-source implementation of the X Window System continuously since 1992." [source] Due to some not-so-exciting political issues, the code produced by XFree86 was forked and most developers shifted to X.Org instead. Today's code is the continuation of what used to be XFree86 code, though XFree86 itself is mostly dead now.

Xorg

The binary name of the X.Org X server implementation.

xfree86

The DDX name for the commonly used X server. A DDX is the device-dependent part of the X server and together with the device-independent bits (DIX) and some other parts forms your binary. The xfree86 DDX is what loads your video drivers and input drivers (amongst other things). Other well-known DDXs are kdrive, Xnest and Xdmx. All patches you see with xfree86 prefixes or pathnames patch this particular DDX.

xf86

Shorthand for drivers that can be loaded by the xfree86 DDX. For example, xf86-input-evdev, xf86-video-ati, etc.

xorg

Firstname of the xorg.conf server configuration file. Lovingly spelled lowercase because we can.

If you're reading this, chances are that you are running Xorg, the xfree86 DDX from the X.Org X server implementation with various xf86-something drivers. How can that possibly be confusing? Pass the mustard, thanks.