You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
There are at least two published definitions in the Go project for the meaning of +1 on a Gerrit review, with the more recent definition on the GerritAccess wiki page seeming to set a higher bar than the earlier and still published definition on the Gardening wiki page.
The Gardening wiki page currently includes this (added by Brad in early 2017):
Review the format of commit messages and presence of tests and formatting of code and typos/grammar in incoming pending CLs. All of that can be done without determining the correctness of the change itself. [...]
Once it has a +1, the owner of that area can give it a +2.
Read a +1 as meaning "triaged", or "not obviously wrong". If it has tests, is formatted properly (references a bug number, probably), and is ready for more review, give it a +1.
A Code-Review+1 vote means that you have read the change and believe it seems reasonable but aren’t making the definitive judgement that Code-Review+2 indicates. It also means you are confident the change does not introduce any sort of security vulnerability or other clearly inappropriate code change.
The second sentence in that quote (especially the use of "confident" and "any sort of") reads to me as a substantially higher bar than "not obviously wrong". (One example is a non-obvious performance problem that could turn into a denial-of-service security issue, which can be tough for a casual contributor to feel confident is not present).
A Trust+1 vote means that you have read the change and are confident that the change does not introduce any sort of security vulnerability or other clearly inappropriate code change. As long as you are sure about that, it's OK to Trust+1 a change even if you don't fully understand all the details of the change.
There is another mention of +1 in the Contributing Guide (which looks to be purposefully making a higher-level statement):
As they near a decision, reviewers will apply a Code-Review “vote” to your change. There are two possible votes:
+1 The change looks good, but either the reviewer is requesting minor changes before approving it, or they are not a maintainer and cannot approve it, but would like to encourage an approval.
Finally, to be submitted, a change must have the involvement of two Google employees, either as the uploader of the change or as a reviewer voting at least Code-Review +1. This requirement is for compliance and supply chain security reasons.
Ideally, the "not obviously wrong" meaning would be reaffirmed. It could be strengthen with "and does not contain a clearly inappropriate code change", or something similar.
If that sets the bar too too low for compliance and security reasons, then perhaps the definition could be split between what a +1 means for a Googler on the core Go team vs. the meaning for an external contributor giving a +1.
Alternatively, perhaps a new "triaged" vote or similar could be introduced with a meaning closer to "not obviously wrong", or some other solution.
It is already somewhat intimidating to think about contributing to a project as large and successful as Go.
Beginning some form of interaction with Gerrit is a great on-ramp to becoming a more active contributor, whether it starts by lurking, or just subscribing to some interesting CLs via the star icon, or making a drive by comment to another new contributor, or actually reviewing a CL.
The prospect of being able to do something as useful as give a +1 is a draw for someone to register for Gerrit, even if they then only use their Gerrit registration for a while for more passive things like starring CLs. (For me personally, I lurked on Gerrit for a long-ish time before I ever registered there, but a "gardening" pitch I heard from Brad in a talk or podcast is a large part of what got me over the hurdle of registering).
Once someone has interacted with some CLs (comments, or just starring), the ensuing notifications spread out over time then offer multiple chances for something interesting to arrive in the potential contributor's inbox when they are not otherwise busy.
There are material benefits for the potential contributor as well when they engage in Gerrit, including improving their own development skills by seeing the rigorous processes of the Go project for code reviews and testing and its approach to software engineering. A deeper understanding of the internals of things like the runtime, compiler, and tools helps in their day-to-day use of Go. If they do later choose to more actively contribute, that can help career development.
I'm hoping the Go project continues to encourage triage-level reviews from outside contributors. It is a true virtuous circle, where the benefits integrated over time are substantial even if point-in-time benefits are sometimes hard to observe (e.g., the confusing bug report that was never written because someone had deepen their understanding of Go after starring & following a handful of CLs).
The text was updated successfully, but these errors were encountered:
I tend to think that a +1 vote should mean "not obviously wrong" and also "does not introduce a security vulnerability." I agree that that is a higher bar, but it seems to me that it's not a much higher bar. Most changes obviously do not introduce a vulnerability. And if you aren't sure whether it does or not, that seems like a reasonable indication that it's not a good change for you to review.
As a separate comment, we should explicitly discourage a +1 vote after the change has been submitted. Those do happen from time to time, and serve no useful purpose. I'm happy to encourage people to read old changes, and comment on them if they see something, but not to vote on them.
As a separate comment, we should explicitly discourage a +1 vote after the change has been submitted. Those do happen from time to time, and serve no useful purpose.
I agree that drive-by +1 reviews from unexpected reviewers are generally not helpful, but I do occasionally leave those votes to record that I have actually reviewed a CL submitted while I was not available to review it, when I am already listed as a reviewer or might otherwise be expected to have particular context for the change.