Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/website: reaffirm "not obviously wrong" for +1 or otherwise clarify its meaning #61364

Open
thepudds opened this issue Jul 14, 2023 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@thepudds
Copy link
Contributor

Issue

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.

Details

Gardening page
The Gardening wiki page currently includes this (added by Brad in early 2017):

Pending CLs

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.

GerritAccess page
The GerritAccess wiki page currently includes this (introduced by Russ in 2022 (as part of the compliance and supply chain security code review requirement changes):

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).

Most of the second sentence was taken directly from the older and now replaced language describing Trust+1:

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.

Contribution Guide
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.

Suggestion

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.

Rationale

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).

@gopherbot gopherbot added this to the Proposal milestone Jul 14, 2023
@ianlancetaylor
Copy link
Contributor

I'm not sure this needs to be a proposal.

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.

@bcmills
Copy link
Contributor

bcmills commented Jul 17, 2023

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.

@ianlancetaylor
Copy link
Contributor

I'm taking this out of the proposal process, I don't think it needs that kind of review. Thanks.

@ianlancetaylor ianlancetaylor changed the title proposal: all: reaffirm "not obviously wrong" for +1 or otherwise clarify its meaning website: reaffirm "not obviously wrong" for +1 or otherwise clarify its meaning Jul 19, 2023
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Jul 19, 2023
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Jul 19, 2023
@seankhliao seankhliao changed the title website: reaffirm "not obviously wrong" for +1 or otherwise clarify its meaning x/website: reaffirm "not obviously wrong" for +1 or otherwise clarify its meaning Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants