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

Gerrit: self-review restriction is unintuitive when cherry-picking #43812

Open
rolandshoemaker opened this issue Jan 20, 2021 · 2 comments
Open

Gerrit: self-review restriction is unintuitive when cherry-picking #43812

rolandshoemaker opened this issue Jan 20, 2021 · 2 comments

Comments

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jan 20, 2021

When cherry-picking someone else's change and creating a CL (i.e. https://go-review.googlesource.com/c/go/+/284779/1) the imposed self-review restriction is somewhat confusing. In this case the author of the commit is able to +2 the change, but the owner who uploaded it is it.

This seems reversed, the "owner" did not actually author the commit so they aren't self-reviewing, whereas the "author" did. I think it would make more sense if the owner was able to +2, but the author was not. The person who uploads the CL still needs to get a Trust+1 before they can submit the change, so they would still be prevented from submitting something without any oversight.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 20, 2021

I think your mental model is just not quite right, because an uploader +2ing the change they uploaded is absolutely "self-review".

If you are uploading a change, then you are asserting it is correct and appropriate and understood. For a cherry-pick, you may not review it too carefully because you know it was already reviewed, and that's fine. But as the uploader you are still asserting that you cherry-picked it correctly, didn't modify the code after the cherry-pick, resolved conflicts properly, and so on. After you upload any change, with that implicit assertion it is good to go, someone else has to verify (review) it and add their own assertion that it is OK (the +2).

The trust bits are separate from the review bits. They just count the number of trusted computers involved. They're not a substitute for the actual review counted by the +2 bits.

@dmitshur dmitshur added this to the Unreleased milestone Jan 20, 2021
@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented Jan 20, 2021

If you are uploading a change, then you are asserting it is correct and appropriate and understood. For a cherry-pick, you may not review it too carefully because you know it was already reviewed, and that's fine. But as the uploader you are still asserting that you cherry-picked it correctly, didn't modify the code after the cherry-pick, resolved conflicts properly, and so on. After you upload any change, with that implicit assertion it is good to go, someone else has to verify (review) it and add their own assertion that it is OK (the +2).

Ah yup, that is reasonable, especially given the uploader can alter the change during/after the cherry-pick (surprising I forgot this given how many times I did that recently). Given this though it still seems like we should be preventing the original author from +2ing? Or in that case are we asserting that the author is essentially verifying that the uploader properly cherry-picked their original change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants