[cairo] A gentle request for better commit messages

[Carl Worth]

I'm enjoying the first Linux Plumbers Conference[*] right now. Last
night I had the privilege of attending (part of) a git tutorial
presented by Linus Torvalds. Some of the things I found most interesting
from the talk were not about git features or capabilities, but specific,
and simple recommendations about things to do, (or avoid doing), with
git.

For example, Linus recommends never using "git commit -m" and instead
recommends using "git commit" and letting git pop up the editor. The
reasoning here is that the lists of files to be committed, (or not
committed), provides an important reminder to the user if the user
forgot to "git add" a new file. So that gives an idea of the kind of
thing he talked about.

But the point I really want to share, and am requesting we all follow
better with cairo was on the topic of commit messages. Linus' suggestion
is to always have a one-line summary to provide the "what" of the
commit, and then to also have subsequent text to provide the "why". Of
course, we do follow that convention in cairo. But Linus went on to
emphasize that the "why" part should never be omitted---even for a
one-line change, it's important to explain the motivation.

I was totally convinced, and I'll need to change my habits. For example,
3 out of my last 5 commits have the one-line "what" and no "why".

And here's a demonstration of why this matters. (Chris, please forgive
me for singling out a commit of yours. Like I said above, I'm guilty of
this myself, but I needed an example here where I don't actually know
the "why" myself.) I'm currently going through the commit history since
1.7.4 and writing up relevant things in NEWS. Here's a recent commit I
found:

	commit eb39151fdc4e72f0836d6fbb5df54885352b3b87
	Author: Chris Wilson <chris at chris-wilson.co.uk>
	Date:   Wed Sep 17 21:19:13 2008 +0100

	    [scaled-fonts] Correct the order of scaled/user arguments.

	diff --git a/src/cairo-scaled-font-subsets.c 	b/src/cairo-scaled-font-subsets.c
	index 9d44ed5..365c74c 100644
	--- a/src/cairo-scaled-font-subsets.c
	+++ b/src/cairo-scaled-font-subsets.c
	@@ -870,8 +870,8 @@ _cairo_scaled_font_subsets_foreach_scaled (cairo_scaled_font
	     return _cairo_scaled_font_subsets_foreach_internal (font_subsets,
	                                                         font_subset_callback,
	                                                         closure,
	-                                                        FALSE,
	-                                                        TRUE);
	+                                                        TRUE,
	+                                                        FALSE);
	 }

So, the "what" part is clear enough. Two arguments were switched, and
now they're fixed. And I don't doubt that the fix is correct[**]. I
trust Chris and his careful attention to detail just fine.

But there's still a problem with the missing "why". I'm writing the
release notes here---is this a fix worth mentioning there? What was the
impact of the switched parameters? Is the just a fragility cleanup that
prevents a real problem happening later? Or is this fixing a bug where
people could have seen disastrous output in some cases?

I think a single-sentence "why" would easily answer any of those
questions.

Is everyone willing to commit with me to always write good commit
messages, even for tiny changes? I don't think it's a lot of extra work,
but I think it will help a lot.

Thanks, and have fun!

-Carl

[*] http://linuxplumbersconf.org/

[**] It's worth noting that we've got a nice demonstration here of the
problems that happen with Boolean parameters to functions. It's far too
easy to make a mistake like this and far too hard to notice it by just
examining the code. Note also that even if these two Booleans were
replaced with some symbolic constants, (our usual solution for Boolean
parameters), the compiler would still silently let the swap go by. So
that wouldn't have prevented this bug. But then we would at least have a
chance of some static analysis tool catching the problem and alerting us
of it. And, of course, the symbolic constants would make the intent of
the code much more obvious from just reading it, (perhaps making it
easier for me to guess the impact of this bug from just reading this
diff).