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 · 3 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@rolandshoemaker
Copy link
Member

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 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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 20, 2021
@dmitshur dmitshur added this to the Unreleased milestone Jan 20, 2021
@rolandshoemaker
Copy link
Member Author

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?

@dmitshur
Copy link
Contributor

Or in that case are we asserting that the author is essentially verifying that the uploader properly cherry-picked their original change?

I think that is indeed what we're doing.

At least when I review cherry-pick CLs, I review that they are a faithful recreation of the original CL (and that the original CL was already reviewed and submitted), that the merge conflict resolution if any was good. The original change author makes for a good cherry-pick reviewer since they're already familiar with the original CL.

Note that at least in main repo's release branches, CL review is about making sure the cherry-pick went well, but CL submission is done by release managers who need to also confirm that the CL went through the backport process and was approved (i.e., has the CherryPickApproved label).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants