<vendor> commit messages and their suitability for coreboot.org - coreboot

23 Dec 2015


      There's a trend I've been noticing about commit messages which is to
follow google's format for everything coreboot. Please stop this.
Here's why google's format is not suitable in many respects. For the
sake of example, I will pick on a RABDOM commit message which follows
the format:
commonlib: add cbfs_vb2_hash_contents()
Provide a common routine to hash the contents of a cbfs
region. The cbfs region is hashed in the following order:
1. potential cbfs header at offset 0
2. potential cbfs header retlative offset at cbfs size - 4
3. For each file the metadata of the file and it is not
   an empty file then the data as well.
BUG=chrome-os-partner:48412
BUG=chromium:445938
BRANCH=None
TEST=TBD
## Summary line ##
So what's wrong with this? Let's look at the first line.
* indicates component, check
* sentence not capitalized, let not pick on this point here
* Sentence content:
And this is where I want to get. What does cbfs_vb2_hash_contents() do?
Does it hash an entire imagine? multiple cbfses? only one file? These
are things that someone looking over a git shortlog must not ask -- it
just wastes time. A much better description is:
"commonlib: Add function to hash contents of a CBFS region"
Here's what I did different:
* I used english, not C to describe the change
* I used wording that makes sense for people less technical than me
* I didn't just throw C names in the summarry
This ties into the "document what, not how" idea. Stretching that a
little, I documented what the function does, not how it's named.
## Commit message body ##
The body does a decent job of explaining the content in more detail. We
could go into whether a certain piece belongs in a source comment, or
commit message, but that's besides the point I'm trying to make here.
### BUG tags ###
Now we get to the tags. Why are tags considered harmful? First off,
"BUG=chrome-os-partner:48412" is not publicly accessible. As a result,
that information is completely useless to coreboot.org.
So how would one go about this? Instead of a pointer to effectively
unusable data, say "This fixes a bug where ...".
Now we come to the "BUG=chromium:445938" line. That is publicly
accessible. When referring to external bug trackers, please include a
short description of the bug, such as:
"This fixes a bug where ...(chromium bug #445938)".
If the "bug report" in question is a feature request, then just leave
out references to it. It's not a bug in the correct sense of the word,
and in the coreboot sense of the word.
### BRANCH tags ###
coreboot.org does not use branches. Even if it did, any reference to a
"BRANCH" does not make sense, as git is responsible for handling
branches. Please leave these lines out entirely.
### TEST tags ###
The general attitude towards a patch is "if people have to ask how you
tested it, then you probably need to include it in the commit message".
There's no hard rule towards the format of the commit message body, so
TEST tags are perfectly fine.
What is not fine is TEST tags that have no useful information. In this
case, a feature is introduced, and some later patch, the feature is
used. Where should the feature get tested? The patch that adds the
feature? The patch that uses the feature? There are cases where patches
have to be tested in bulk, and that's fine. Please describe the testing
methodology only on the last patch of the series.
A very common TEST tag says "built and booted <board>". That's lazy.
First, every coreboot commit is build-tested build jenkins for each
board. Restating that the patch builds for a specific board is redundant.
What does "booted" mean? Does it mean boot to ramstage? Does it mean
boot to payload? Which payload? Does it mean boot to kernel? Which
kernel? Does kernel crash or reach shell? Does it bluescreen? Does it
reach a shell? Graphical shell? Console shell? Is the console over
serial? Is it over network? Does USB work? Can you access SATA drives?
...
From what media was your kernel loaded?
You get the point. "booted" provides no meaningful information. If your
testing involved "building and booting" a specific board, then please
leave out the TEST tag from your commit message, or remove it before
pushing the commit to coreboot.org.
### <vendor>-centrism ###
Many times, commits look like:
funnychip: Enable flux capacitor calibration
funnyboard: Enable flux capacitor in devicetree
When they should actually look like:
soc/funnychip: Enable flux capacitor calibration
funnyvendor/funnyboard: Enable flux capacitor in devicetree
It's understandable that a vendor works only on a very limited part of
the codebase, and for them referring by codename alone is obvious.
However, if your internal policy is not to follow coreboot.org
namespacing policy, when those patches are pushed to coreboot.org,
please make sure they are properly namespace.
For example, if you tell me that you enable clock gating on an mcp509,
how will someone know if that is a southbridge, soc, or SPI chip? By
prefixing, and that's why you should do it too (TM).
And my favorite "built and booted glados":
* You build glados? I didn't know you worked for Aperture, Inc.
* Oooh, you meant "built and booted google/glados". NVM then
### Tags in general ###
Tags are meant for machine-parsable content. coreboot commit message
body are inherently meant to be read by a human, and thus are by design,
human-readable. This fundamental contradiction means that tags will
always be second-class to clear sentences and well-structured
paragraphs. If you can avoid tags in the first place, please do so.
## Closing remarks ##
google "owns" most of the brand-name, respectable coreboot developers.
When those individuals make the mistakes I pointed here, it's very easy
for an observing new contributor to mistake those as the official policy
of the project. Because if all the respected names do it, it must be
right, right? People make mistakes.
I have the luck of doing coreboot for a company which does coreboot (and
is not google). I've shown people how to think about coreboot and write
upstreamable patches. Those very same people have corrected me on code
reviews. I make mistakes. If you're reading this, and are from planet
Earth, you make mistakes. If you're one of the very respected names I
mentioned earlier, you make mistakes... because nobody is perfect.
There's much more work going on in coreboot than the patches in google's
little world, or intel's little world, or raptor's little world, or
minifree's little world. When I write patches, I do my best to be
courteous to those outside my little world. I do my best to write short
patches that people not familiar with the hardware can review. I do my
best to properly namespace and describe the patch in the first line, so
that someone looking over 100+ patches can have a better idea of whether
or not that patch is relevant to him (or her).
I always think in terms of "would this be acceptable to coreboot.org" ?
Most developers think in those terms, and are held to that standard.
It's easy to do things like not properly prefix a title because you only
have 65 characters available, and it's inconvenient to spend time
thinking of a better title. It's easy to add nonsensical tags because
your employers tools will automootically check for such tags, what you
do when you submit your work to your employer for a paycheck is up to
you, but please make sure that work ending up on coreboot.org is free of
these easy to fix mistakes.
And if you're someone else observing this discussion, please keep in
mind that the people I mentioned earlier are well-intentioned. They try
to keep the chromium and coreboot.org branches in sync, and that's
neither an easy task, nor something they are asked to do.
Before someone decides to take my side, and start accusations, please
keep in mind these people are well-intentioned.
Alex