Changelist descriptions

[Rob Pike]

An edited version of a rant I posted internally at Google. It applies here too:

Most changelist (CL) descriptions I read are inadequate. The classic case is "Fix bug" (What bug? What fix?) but too many CL descriptions are no better.

There are many forms of "Fix bug": "Fix build." "Add patch." etc.

Here are a few I've seen lately, sanitized to protect the innocent.

"Move code from A to B".

"Phase 1."

"Add convenience functions."

and so on. None of these is helpful in any way. Although their authors may believe they are providing useful information, they are not serving the purpose of the description.

Every CL description should serve as a public record, a summary of what and why and perhaps offer advice to the reviewer. If relevant, it should provide background information such as bug numbers, benchmark results, and links to design documents. If the edits are purely automatic, say so. If the change is purely automatic except for one file, say so. In fact, say everything useful that is important, independent of the code. Do not expect the person reading the CL description to have the code handy, because in the future they might be trying to understand the history and will not have every delta visible on screen.

Even small CLs deserve a little attention to detail. Put the CL in context.

It's likely that a day or month or a year from now someone will search the history log for the change but not be able to find it because all the important information is in the code but not identified in the CL description.

In short, write the CL description for someone in the future who might be looking for your change because of a faint memory of its relevance but without the specifics handy.

I just looked around and found this one by Austin Clements, which is very good because it satisfies these criteria: What, why, and background.

Author: Austin Clements <aus...@google.com>

Date:   Fri Dec 19 16:16:17 2014 -0500

    runtime: run libc SIGSETXID and SIGCANCEL handlers on signal stack

    

    These signals are used by glibc to broadcast setuid/setgid to all

    threads and to send pthread cancellations.  Unlike other signals, the

    Go runtime does not intercept these because they must invoke the libc

    handlers (see issues #3871 and #6997).  However, because 1) these

    signals may be issued asynchronously by a thread running C code to

    another thread running Go code and 2) glibc does not set SA_ONSTACK

    for its handlers, glibc's signal handler may be run on a Go stack.

    Signal frames range from 1.5K on amd64 to many kilobytes on ppc64, so

    this may overflow the Go stack and corrupt heap (or other stack) data.

    

    Fix this by ensuring that these signal handlers have the SA_ONSTACK

    flag (but not otherwise taking over the handler).

    

    This has been a problem since Go 1.1, but it's likely that people

    haven't encountered it because it only affects setuid/setgid and

    pthread_cancel.

    

    Fixes #9600.

What's really sad is how often I find myself telling people this. It should be part of our culture, not something an old fogey needs to beg for regularly.

Please heed my plea and write good CL descriptions for Go—and for any other project you work on.

-rob

[Ingo Oeser]

I have been in the same position (having to read hell a lot useless change descriptions) at multiple companies and teams and feel your pain.

What worked best so far, besides being persistent, is to require a business value in the change description.

I recommended to always start it with "in order to". I observed this tends to start a thought chain which results in better descriptions.

It also adds the most value for the least "process": The answer to "What did you expected to gain from doing this?"

Rationale:

The "what" is usually obvious from the diff and you just save time skimming through lots of changes, if it is repeated. 

Background can be inferred with hard work reading code, related diffs and running tests. It saves a lot of time to have that.

The "Why?" is the only thing that is impossible to guess correctly 100% the time and usually only guessed correctly about 10% of the time.

So I focused on that.

A nice side effect is, that many changes like "adhere to $CURRENTLY_FASHION_PATTERN" get abandoned while trying to come up with a business value for it.

So you have way less such changes to review.

On Tuesday, December 23, 2014 7:43:16 AM UTC+1, Rob Pike wrote:

An edited version of a rant I posted internally at Google. It applies here too:

[Rob Pike]

I would rather not turn this into rules and style guides, just a widespread understanding that a changelist description is worth taking some time to write. If you write one, write it well. If you review a CL and find the description lacking, ask for it to be improved.

Incidentally, I disagree with your claim that the "what" doesn't matter because it's in in the code. One key role of the changelist description is to serve as a reminder of what's in the code for those who don't have the code in front of them, such as someone reading the logs.

-rob

[innn...@gmail.com]

‘’what" doesn't matter because it's in in the code.