Talk:Gerrit/+2

Revocation
Who's going to make the revocation decision? Sharihareswara (WMF) (talk) 20:38, 19 October 2012 (UTC)


 * Simple policy I'd suggest is that
 * anyone can propose a revocation discussion,
 * any architect (currently Brion, Tim, Asher, Mark) can sign off on a revocation for technical reasons,
 * WMF's most senior engineering manager (that'd be me right now) or his/her delegate can sign off on a revocation for emergency security matters or obvious policy breaches.--Eloquence (talk) 20:47, 19 October 2012 (UTC)
 * Sounds fine to me; I presume "technical reasons" also includes the "failed to socialize" and "not followed guidelines" bullet points. Thanks for specifying. Sharihareswara (WMF) (talk) 20:53, 19 October 2012 (UTC)


 * Yep.--Eloquence (talk) 20:56, 19 October 2012 (UTC)

Probation?
If I'm not mistaken, the standard modern WMF hiring offer stipulates a probationary period for new hires. If granting +2 for core to new hires immediately is too radical a proposition, granting +2 within the standard probationary period for new hires might be a worthy compromise. —The preceding unsigned comment was added by Cmcmahon(WMF) (talk • contribs) 21:48, 19 October 2012

Self-merging
Here are the situations that I currently do self-merges for...
 * 1) Minor documentation/comment fixes - already mentioned in the guidelines
 * 2) Cosmetic CSS changes to extensions I'm working on - no one else on my team has strong CSS knowledge, and I don't want to bug people outside my team for what are essentially trivial changes
 * 3) Updating deployment branches with code that has already been merged to trunk
 * 4) (Rarely) I started a change that was taken over by another team member, i.e. a partial self-merge
 * Are all of these situations appropriate for self-merging? Kaldari (talk) 23:27, 22 October 2012 (UTC)
 * 1) Localisation updates (mostly by L10n-bot, also used with interactively created updates to English language messaging (mostly by Siebrand and Raymond)
 * --siebrand (talk) 00:15, 23 October 2012 (UTC)

I would add the reverse of #4: merging a change that was originally authored by someone else, but that you took over and made trivial modifications to. For instance, I will often review a commit, amend it to change something trivial (typo, whitespace, whatever), then merge it. While technically that's a self-merge, conceptually it's really not. I think that's fine (obviously, because I do it all the time), and I think #4 is fine too if very little of your original code remains and/or if the other author also has +2 (because you can then argue that each individual sub-change was reviewed by someone other than its author). #3 is totally fine provided there isn't some other reason why it shouldn't be merged into that deployment branch. I think #1 is defensible but I avoid it, because even though comment/doc changes won't break things, I think they should still be reviewed, and usually they're not urgent. I also don't like #2 very much, I think it's OK to do on occasion but ideally it wouldn't be a habit.

Also, I'd propose a #5: you're fixing the live site, there's no one around to review your commit and it needs to go live right now, so you merge it into the deployment branch(es). This deploy-first-review-later workflow is OK IMO if the change can't be reviewed quickly enough, and as long as "later" isn't "never" (i.e. you don't self-merge it into master). --Catrope (talk) 00:24, 23 October 2012 (UTC)


 * Another example is if you the person you are deploying with doesn't have +2 privs yet, you can let him/her review with +1 if there is an agreement in place that you'll self-merge anything that they +1. Admittedly with the new policy, this shouldn't happen anymore, but the point is that you can deputize people who have +1 to act as +2. :-) --Tychay (talk) 01:02, 23 October 2012 (UTC)

Is there some way I can assert that a change is "Minor documentation/comment fixes" and git/gerrit/Jenkins can verify my assertion? I come across minor typos and medium mistakes in comments, and I (and reviewers if I don't self-merge) would feel better about patching them if git/gerrit/Jenkins agreed. Maybe independent of a commit having the magic phrase ('doc comments only' ?), git/gerrit/Jenkins could stamp it "COMMENTS ONLY". -- S Page (WMF) (talk) 07:02, 24 October 2012 (UTC) (and tech writer)

Other reasons to self-merge

 * You're reverting something that was broken (this falls under the "code must always be runnable" maxim)
 * You're fixing something that is currently causing downtime (and can't be flat-out reverted for some reason)

Page title
I'm not thrilled with the current page title ("+2"). "+2" is great for a redirect, but not as a destination page, in my opinion. --MZMcBride (talk) 23:59, 22 October 2012 (UTC)


 * Seems reasonable enough. What do you suggest as an alternative title? --Tychay (talk) 01:07, 23 October 2012 (UTC)


 * Not sure. :-) I was thinking that maybe this would be a subpage of Git or Gerrit? Maybe something like Git/Merging code or Git/Tutorial/Merging code or something. The terminology in this area seems kind of weak currently (e.g., what do you call someone with the ability to +2? a +2er?), so it may take time for the language to develop in order to find the "right" title. --MZMcBride (talk) 03:33, 23 October 2012 (UTC)


 * A "merger"? That is, after all, what +2 allows them to do. :-) Of course, the flip side of the coin is -2er - "vetoer" - but that's a less fun thing on which to focus. Jdforrester (WMF) (talk) 17:41, 23 October 2012 (UTC)

Technically the important permission for merging is Submit, not CodeReview+2. In practice though, we hand them out together. So..."Submitters?" ^demon (talk) 13:12, 25 October 2012 (UTC)


 * Now the page is Gerrit/+2. Clearer and in line with the consolidation of Git / Gerrit documentation under Gerrit/.--Qgil (talk) 18:44, 4 March 2013 (UTC)

Overall, page looks good
Overall, this page looks good. I was a little worried about people getting themselves into trouble if it wasn't clearly delineated which specific branches or code areas they could or could not touch, but after reading the page, I'm more confident that the general rule of "use common sense" will work. I hope!

The merging bottleneck will be nice to see reduced (assuming people who get the power to +2 actually do something with it). I think I noticed IAlex and a few others doing some of this merging already and it's great. Combined with a two-week deployment cycle, this will hopefully speed things up overall for bug fixes and feature requests. :-) Then the final step is recruitment.... --MZMcBride (talk) 00:06, 23 October 2012 (UTC)


 * Another step is this "AGF" policy works out well, then hopefully we can extend +2 a little more liberally to the community. There are a lot of extensions out there that no one in the WMF is currently qualified to review. --Tychay (talk) 01:10, 23 October 2012 (UTC)

Tagged as policy
I've gone ahead and boldly tagged this as development policy. I think we all pretty much agree on the general principles. Shouldn't stop anyone from doing further word-smithing. ^demon (talk) 12:59, 25 October 2012 (UTC)

"... or a WMF-deployed extension"
Is this part necessary? I tried to kill the "WMF" part (and make it "Wikimedia Foundation-deployed extension") and then it seemed even sillier. I'm wondering if there's a better way to phrase this or if it can be removed altogether. --MZMcBride (talk) 08:16, 26 February 2013 (UTC)


 * I agree. +2 comes with a lot of responsibility no matter what. Yes, you can break more visible things if your merges end up breaking English Wikipedia but people in a position to get +2 for MW core of Wikimedia deployed extensions know that already.--Qgil (talk) 18:53, 4 March 2013 (UTC)

Seeing who has +2 requires an account
So I clicked on https://gerrit.wikimedia.org/r/#/admin/groups/11,members out of curiosity and I got a message saying that "You are no longer signed in to Gerrit Code Review.", and was unable to view the list. My standard Wikipedia/Wikimedia login didn't work. Is there a reason why this list is access protected? Sven Manguard (talk) 23:51, 12 January 2014 (UTC)