Interim Code Review and Issue Tracker Conventions

Hi golang-dev,

The Go team is working through the backlog of change lists (CLs) and issues that we've let pile up. We're also working on putting more mechanism in place to make sure things are not forgotten. And we're working on some clear docs about expectations for the issue and the CL process, to help both the people filing those and the people fielding them.

As a stopgap for real documentation, this mail has a few interim requests to help enable some of the planned mechanisms. Please read it and help us by following these. This is not a complete doc in that it only gives deltas from what we’re already doing. Following these conventions will help make future tooling more useful.

Thanks very much.

Russ, on behalf of the Go team

Code Reviews

Reviewer Assignment

Rietveld (codereview.appspot.com) had a feature that simply replying to an issue put you in the “Reviewer” line, making it unclear who exactly was responsible for the actual review. To cope, we introduced a convention that saying R=name in a review message meant name was the reviewer who was responsible for doing the actual review of the CL.

Gerrit has the same feature of automatically adding “Reviewers”, and thus the same lack of clarity about who is responsible for the review. It is possible that Gerrit might be fixed directly, but until then, we’ll continue to use the R= convention.

The 'name' in R=name is anything that you can pass to git mail -r, so any full email address is suitable or, for the common reviewers, just the part before the @ sign.

CL scores

Gerrit distinguishes between +1 and +2 scores, but we haven't agreed what they mean beyond the hover text Gerrit has. We want to establish a workflow where receiving a +2 is an unambiguous signal that the CL can be submitted without further review, but to do that reviewers will need to be more careful about using +1 versus +2 as appropriate.

Please use the following meanings for the scores:

+2 means “this CL can be submitted with no further review from me or anyone else.”

If the CL author is one of the Go project's committers, that may include situations when the author should make minor changes first, since (in our configuration) Gerrit will let the +2 enable submit for the changes in that case.

+1 means “this is okay with me, but more review is needed.”

Reasons for more review might include: the CL needs large changes that should be re-reviewed; the CL needs minor changes, but the author is not a committer; or the CL needs a different reviewer for the final sign-off. In the last case, the review comment should also include an R= reviewer assignment (see above).

This policy is different from what we've done so far: there is no “+2 but wait for Rob.”

If the author should wait for someone else, it's by definition only a +1.

Similarly, there is (usually) no “+2 but make these changes.” If changes are needed, it’s a +1.
Exception: if the CL author is a committer, “+2 but make these trivial changes” is still allowed, because the +2 will propagate forward to the updated CL to avoid a review cycle.

Again, we are trying to eliminate uncertainty about whether it is okay to click Submit when Gerrit offers that button. Because +2 is what enables the Submit button, it should be used only when the CL author should feel free to submit without further review.

If problems are found in a CL after it is submitted, additional reviewers may post comments after the submission. It is preferable for the original author to address the comments when possible.

We expect that there will be uncertainty while reviewers adjusts to the new scoring. If you think you might need additional review despite having a +2, it is always fine to ask for additional reviews.

We are asking reviewers to build a collective sense of how much review a particular CL needs, rather than trying to write and enforce some kind of machine-readable rule set. We do expect that an occasional CL will slip through without enough review, and when that happens we will reply explaining why the CL should have had more review, to try to adjust that collective “review sense.”

DO NOT REVIEW / DO NOT SUBMIT

For Gerrit, please write the text DO NOT REVIEW or DO NOT SUBMIT in your Git commit messages when appropriate. The spelling matters but case doesn't; however, the all caps form is conventional.

DO NOT REVIEW means no reviewers should look at the CL at all. It's a draft that the author posted for private perusal or to sync between different machines. Eventually Gerrit will stop mailing golang-codereviews about the upload of these commits. To make sure that every submitted change is mailed to golang-codereviews before being submitted, Gerrit will refuse to submit a patch with DO NOT REVIEW in the commit message. We can't stop people from replying anyway, but please don't.

DO NOT SUBMIT means that while the CL is okay to review, the author wants Gerrit to make sure the CL is not submitted in its current form. Gerrit will refuse to submit a patch with DO NOT SUBMIT in the commit message.

The difference between these two markers is whether other people should look at (and, eventually, receive mail about) the CL.

People have been using a variety of these kinds of strings in the past. To make it easier for people and (especially) computers, please use these exact strings from now on. Also, because Gerrit freezes the email subject for a CL after sending the first mail for that CL (intentionally, for better email threading), it is important to put in a descriptive first line of the commit message even when using these markers.

For example, a first draft of a CL for encoding/xml uploaded to Gerrit only for self-review might use this simple commit message:

encoding/xml: handle decoding of <![...]> form

DO NOT REVIEW

Issue Tracker

Issue Titles

Please edit issue titles to use the standard form:

path/to/dir: <problematic behavior OR action to take>

The path/to/dir: prefix explains where the expected changes will be made. For packages or commands in the main repo, it is the path under $GOROOT/src or else $GOROOT. For example, the prefix should be “encoding/json:” for an issue regarding the code in $GOROOT/src/encoding/json, or “test/bench/go1:” for $GOROOT/test/bench/go1.

For code in subrepos, the path/to/dir: prefix should be an x/<repo> prefix followed by the path inside the subrepo. For example, the prefix should be “x/tools/cmd/benchcmp:” for an issue regarding the code imported as golang.org/x/tools/cmd/benchcmp.

There are also a few conventional non-path prefixes: “all:” for a tree-wide issue, “build:” for a general build problem, “gccgo:” for issues specific to gccgo, and “website:” for general golang.org web site issues.

Issue Milestones

The first goal in processing an issue in the issue tracker should be to set a milestone. It may be necessary to gather more information first, but things should proceed quickly to the milestone assignment. If you know enough to assign a milestone, please do. This way, the issues that have no milestone are the ones that still need direct triage.

The milestone Go1.5 means that action on the issue blocks Go 1.5: it must be fixed, closed without a fix, or moved to a different milestone before Go 1.5. The milestone Go1.5Maybe means that action on the issue would be nice before Go 1.5 but is not a blocker. Early in the freeze the line is a bit fuzzy; do your best. It’s common for issues to change milestones as the cycle progresses.

The milestone Unplanned means that there is no concrete plan to fix the issue in any upcoming Go release. The start of a new release cycle is a good time to look through Unplanned issues for work that should be planned for that cycle.

The milestone Unreleased means that the bug affects code that is not part of a release cycle. Most bugs related to code in subrepos should be in the Unreleased milestone, except for the pieces of x/tools that do get bundled into releases, such as godoc and vet.

Issue Labels

Please use issue labels sparingly, and only for useful concepts beyond the usual categorization present in the issue title and milestones.

For example, we use Documentation and Testing labels to identify issues in any package that are only documentation updates or new tests, because that work can be done later in the release cycle than code changes. Those are useful and not present in the issue title or milestone information. In contrast, there is no need for a “runtime” label, because all the issues related to package runtime should say “runtime:” at the beginning of the title anyway.

As another example, Google Code supported labels that were effectively key-value pairs, like the OS set (OS-Linux, OS-Plan9, ...) the Release set (Release-Go1.3, Release-Go1.4, ...), and the Repo set (Repo-Main, Repo-Tools, ...). The presentation in Google Code made these sets convenient to use, but they transferred to GitHub as many ordinary labels. We kept the OS set, because it is otherwise hard to search for or exclude issues about Windows. We moved release information into milestones, including retroactively to Go 1. We deleted the Repo set in favor of the x/<repo> prefixes, along with the Unreleased or Unplanned milestone as appropriate.

We still have all the old label data from before the cleanup; let us know if information was discarded that you find useful, and we’ll figure out how to reincorporate it.