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

[Aaron Durbin]

On Tue, Dec 22, 2015 at 7:12 PM, Alex G. <mr.nuke.me at 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:
> 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

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 at coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot