On Tue, Dec 22, 2015 at 7:12 PM, Alex G. mr.nuke.me@gmail.com wrote:
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:
I'll bite since this is largely directed at me.
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:
- potential cbfs header at offset 0
- potential cbfs header retlative offset at cbfs size - 4
- 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
I didn't know summaries were complete sentences.
- 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"
That's excellent code review feedback. Too bad it wasn't on gerrit.
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.
Then why bring it up?
### 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.
That creates more work on my part for juggling. I opted to create patches against coreboot.org instead of just landing things in our internal repository. If I stripped everything across that boundary I'd be doing cumbersome clerical work. In the face of that it's actually easier to not engage w/ coreboot.org first. That'd be an unfortunate outcome in my opinion.
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.
You may also optionally ignore the content. I don't see the issue. If you are doing development against internal bug/feature databases that tracking is very helpful. Demanding that such references not be put in commit descriptions seems like a silly request when the developers need that for their own work.
### 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.
Again, this can be ignored but it also doesn't hurt anything.
### 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.
Indeed good feedback on the code review, but it's not there. Actually, it looks like I didn't backfill what I did. Thanks for the pointer.
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.
But it's not booted so in that sense it's actually a positive signal. Jenkins building also doesn't cover all the combinations of options that something could be impacted by.
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.
If you juggle that many definitions of 'booted' in your head I can see why the pedant comes out. It is my understanding that booted means booting through the OS to userspace. Is there really other definitions? And if there are wouldn't those be less progressing?
Again, feel free to ignore it as well. While it may not be beneficial to you it may be beneficial to others.
### <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.
That's a waste of line length which would impede on your full sentence summary and --oneline.
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).
mcp509 lives in all those directories? Do we have subdirectories with the same name under the higher level component directories?
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
There's only one glados directory in the tree.
### 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.
Alex-deemed 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'm curious how you can't determine if something is relevant to you or not. Either you are familiar with a board, subsystem, etc or not. If you aren't why wouldn't you just ignore it?
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.
You keep using the term 'mistake', but I don't think that's the right term. You have an opinion on things and are expressing it. Did I miss somewhere on https://www.coreboot.org/Development_Guidelines where your rules are stated? Many of things I see you commenting on here aren't mentioned. Much of the requests seem like "make me happy because I don't like this". When, in fact, you could easily ignore the extraneous-to-you info while not putting upon others who are trying to contribute back.
Taking the 65 characters available limit I actually think prefixing the higher level component to be a net-loss when attempting to form a clear summary.
$ find . -maxdepth 3 -type d | sort | grep -v include | sed -e 's#./##' -e 's#$#:#' | while read l; do echo $(echo $l | wc -c) "'$l'"; done | sort -n -r | less
Look at the distribution there. That's a lot of wasted characters. Ignoring anything w/ mainboard in it still yields 20 characters or more that disappear. That would suggest to me that the request is for terse summaries and duplicating the directory structure in the summary line.
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.
That's the crux of the issue. You are requesting changes that make that harder. To some it may seem all patches are standalone w/o any other dependencies. That's definitely not the case when creating a product that utilizes coreboot. As such, that metadata is needed for tracking the dependencies across repositories. In the face of stripping that metadata away to appease people that are annoyed by some minor content in the description then the path of least resistance is to not participate early.
Alex
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot