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)

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

Sven Manguard, thanks for pointing this out. I think this is a flaw in Gerrit, not a decision made by Wikimedia's developers. I'd like for this to be changed. I filed a bug: https://bugzilla.wikimedia.org/show_bug.cgi?id=60041

Gerrit user logins are available for anyone to get, but right now it's a different system than the regular Wikimedia/Wikipedia login system. I would love to have true single sign-on, that is, unification among SUL, Bugzilla, Gerrit/Labs (a.k.a. Developer Access), and anything else I've forgotten. But that's some ways off in the future right now.

I figured I could answer your question right now (although of course this may change): the membership in that group is now:

• Wikimedia Foundation engineers, including those in Operations
• Alex Monk
• Aude
• Bartosz Dziewoński
• Brian Wolff
• Daniel Kinzler
• Hoo man
• IAlex
• MarkAHershberger
• Parent5446
• Platonides
• Raimond Spekking
• SPQRobin
• TheDJ
• Victor Vasiliev

Of course, this may change in the future.

I hope that's helpful. Sharihareswara (WMF) (talk) 16:19, 14 January 2014 (UTC)

Thanks. I was asking out of a curiosity to see if some of the people I've worked with had it, rather than any concern. Sven Manguard (talk) 20:32, 14 January 2014 (UTC)

Whither WMF staff +/-2?

Not that I need it. But I've just noticed that the list of people who can merge in core has dropped to a small fraction. If that's a glitch, please disregard. If it was a decision, it would be nice to link on this page, and clarify some of the language that says WMF-employed developers have merge privileges. Thanks! Adamw (talk) 04:54, 18 May 2014 (UTC)

@Adamw: Do you mean that https://gerrit.wikimedia.org/r/#/admin/groups/11,members doesn't list all WMF staff? Most of us are caught by the "ldap/wmf" group; the individuals who are added directly generally are there in case LDAP fails, I think. Jdforrester (WMF) (talk) 06:23, 18 May 2014 (UTC)

@Jdforrester (WMF): Exactly—I don't see a ldap/wmf group, but I imagine other people would be complaining if that were the case? Maybe it is invisible to non-members? Adamw (talk) 04:23, 27 May 2014 (UTC)

@Adamw: Yep, having checked with Chad it looks like groups in access lists are invisible unless you're in them. Yay for gerrit confusion. We should add you back into that list. Jdforrester (WMF) (talk) 15:56, 27 May 2014 (UTC)

Just core and extensions?

Should this not also cover other things? For example, it would be a terrible idea for me to merge my own commits to VisualEditor core, but User:Greg (WMF) points out this page says "So, you got +2 rights to MediaWiki core or a WMF-deployed extension." at the top. And WMF has a ton of other stuff deployed that isn't MediaWiki core/extensions (skins, among many other things). (He also points out that the current reality is that operations team basically merges all their own patches to operations/puppet.git, which may need to be taken into account - or fixed - if/when making changes to this policy). --Krenair ^{(talk • contribs)} 00:05, 20 December 2014 (UTC)

Given the lack of response for 8 months I went ahead and added skins where we currently mention extensions. Haven't done anything about cases like VisualEditor core. --Krenair ^{(talk • contribs)} 01:03, 18 August 2015 (UTC)

Moving this page to "plus2" (not under Gerrit)

This is largely a policy page, not about Gerrit the software. I would like to move this to plus2, which is a redirect I've been using for a while. It used to be housed at +2, which seems better, but ends up with readability issues due to URL escaping issues.

If someone else agrees with me, feel free to make the change. I'll consider silence assent, and do this in a week or so if no one brings up something I haven't thought of. -- RobLa-WMF (talk) 19:05, 14 April 2016 (UTC)

@RobLa-WMF: I think the "readability concerns" were over-blown; moving back to +2 works for me, but plus2 is ugly. That said, we're going to have to come up with a proper name for this very soon given the switch to Differential (where there's no such state as "+2"). Jdforrester (WMF) (talk) 18:05, 18 April 2016 (UTC)

@Jdforrester (WMF): hmm, you prefer <https://www.mediawiki.org/wiki/%2B2>? I was hoping to have a more readable URL to copy/paste around, since I refer to the page frequently. I suppose I can wait until after the Differential move to make the change. -- RobLa-WMF (talk) 19:45, 22 April 2016 (UTC)

@RobLa-WMF: I agree it's not that readable, but it is the name by which it's known. :-( When we pick a new, Differential-compatible name, let's make this a criterion. Jdforrester (WMF) (talk) 20:10, 22 April 2016 (UTC)

Does archiving an extension qualify for self+2?

I was wondering if changes like <https://gerrit.wikimedia.org/r/#/c/437941/> or <https://gerrit.wikimedia.org/r/#/c/437929/> do qualify for self-merge. Purely speaking, those extensions are unmaintained/abandoned/no longer working and are not deployed to the Wikimedia cluster either. I don't feel those are particularly dangerous. Thoughts? MarcoAurelio (talk) 10:32, 7 June 2018 (UTC)

I think there should at least be some kind of review by someone else besides the proposer on the task probably, but the individual patches are routine and can safely be self-merged. Semi-relatedly, I think we should document clear guidelines on when repositories should be archived. Legoktm (talk) 21:39, 7 June 2018 (UTC)