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