Topic on Talk:Gerrit/Privilege policy

Three things, which are tangential to the actual point of this proposal, so feel free to ignore:

• When picking up (stale) patches from others, I often seek at least +1 from someone else for my additional changes before I self-merge. Assuming this is acceptable, maybe it can be documented under self-review.
• It would be kind for code bases with clear maintainers to give the maintainers a chance for review. I have seen patches created and merged by non-maintainers while maintainers were asleep. Sometimes these do have issues that could have been resolved before merging if the maintainers had the possibility to give a look at them. Can we encourage this in the policy?
• A non-criticizing observation: due to these rules, I often ask others to create patches I could do quickly myself, because getting someone to review my patch would take even longer than having the other person figure out how to create the patch, and I sometimes basically tell how it needs to be written, which is actually a self-review if one thinks about it very strictly.

I said something about the first point, and I also renamed the section and changed some language to make it clear that the problem is merging changes that have not been reviewed by someone else, not "self-review", whatever that is. You might naively think that self-review means reading your own code before you upload it to Gerrit, which seems OK to me.

I've also encountered all three of these situations, particularly the first and third. Along these lines, I'm reminded of a few more edge cases:

• The "virtual +2": If I'd be willing to merge the patch from another +2er as-is, but I also want to leave a suggestion for minor improvement, I'll leave the comment and tell the author to feel free to self-merge the patch if they don't want to make further changes, instead of having to wait for me to come back to do it.
• Transient Jenkins errors: I've sometimes self-merged a patch when a previous non-self +2 failed to merge because of some transient Jenkins error, for example when npm occasionally fails to download its packages.
  • I know I've been tempted to do the same if Jenkins failed only because of a conflict in the RELEASE-NOTES file, but I don't recall whether I've ever actually done it. I have sometimes +2ed after doing the needed manual rebase of someone else's patch for RELEASE-NOTES or blatantly obvious documentation typos.

I've hopefully covered this now. Rebasing RELEASE-NOTES could count as a trivial change not needing review.

• puppet and mediawiki-config are mentioned under the heading "Deployment branches", is that a mistake? I imagine for deployment branches (and more generally backports) we'd want to say that self-merging is fine because these patches already passed code review elsewhere.
• I think self-merge after a virtual +2 or real +2 and simple rebase / CI failure is normal and does not contradict the policy as currently phrased (note it talks about self-review, not self-merge).
• To generalize Nikerabbit's last point, what's the difference between someone with +2 self-merging a patch that has been +1-ed by someone without +2, and someone with +2 merging a patch from written by someone without +2? In both cases the code is reviewed by exactly one person who is trusted with and responsible for enforcing conventions etc.etc. and another person who is less trusted. So the arguments given in the self-review section don't fully make sense to me.
  • And then sometimes that leads to merging code written by a less experienced developer that's OK but not great, because directing them through all the necessary changes would take forever and would not be a great experience for either of us, and changing the patch would mean I'd have to find another person with +2.
• "Inline comments can be used to indicate issues with the code that should be addressed before it is merged. Exercising -1 or -2 is just as important as +2." - this should probably be in the previous section, not self-review?

I removed the bit about -1 being as important as +2, because it seems a bit glib and confusing. Hopefully my change to the language to emphasize code review over merging covers this anyway. That change also hopefully answers your first point.

puppet and mediawiki-config are mentioned under the heading "Deployment branches", is that a mistake? I imagine for deployment branches (and more generally backports) we'd want to say that self-merging is fine because these patches already passed code review elsewhere.

I was given pause in reading the same bulletpoint. I made a note to myself that perhaps the wording, "In deployment branches and the Puppet repository, changes are merged by the person doing the deployment" could be modified to indicate that self-+2 is common in any repository (or branch within a repository) where +2 indicates deployment rather than review; e.g., Deployment branches, integration changes, puppet repositories.

I don't know how to improve that wording, or whether the wording necessarily needs changes. I will say I did not understand the current wording quickly.

> In both cases the code is reviewed by exactly one person who is trusted with and responsible for enforcing conventions etc.etc. and another person who is less trusted

Not really - in both cases the code has been viewed by one who is trusted with and responsible for conventions. But spotting one's own mistakes is a lot harder than spotting other people's mistakes. So I do think it makes sense to require review by another person with +2 rights who is not the author.

For some smaller projects, we can be in a situation where only one person has enough knowledge on the project to sensibly do +2, or, even more frequently, only one such person is available (e.g. there may be two but one is on a month-long leave, or buried under urgent workloads, otherwise inaccessible). In this case, if the person who has +2 authors a change, there would be no way to merge it without self-merging.

@Smalyshev, I think that's covered by "For extensions (and other projects) not deployed to the Wikimedia cluster, the code review policy is up to the maintainer or author of the extension".