In T114419#1863829, @Qgil wrote:

The investigation needed is to know what is the expected outcome of this session, and how @Aklapper's T101686 and T78768 fit here. Ideally, these efforts would converge in a document published before the event where problems and potential solutions are proposed and discussed online.

I am working on T101686: Goal: Define potential actions to reduce code review queues and waiting times as this has been one of my quarterly goals for 2015-Q4.
(T78768: Agree on and implement actions to prioritize code review of patches submitted by volunteers is post-summit).
I'm currently structuring and summarizing my own thoughts and I'm going to prepare a list of CR problems and potential actions/"solutions" still in 2015.
Thanks everybody for sharing your thoughts in this very task so far!

In T114419#1872365, @Bawolff wrote:

As for T78768: Agree on and implement actions to prioritize code review of patches submitted by volunteers
While originally my intended focus for this meeting was volunteers, after discussions with various people (esp. WMF staff) I no longer think that's the best way to approach the code review problem. Of course, given this is intended as a brain storming meeting primarily, all ideas related to fixing code review are arguably in scope. That said, I'd like to frame this meeting as "How to fix Code review", not "How to fix code review for volunteers" (or any other group).

@Bawolff: I agree with this, and also with your "ideal outcome" of the WikiDev session.
Volunteer-contributed patches are a subset of the CR problem.

FWIW, my personal opinions is that this is entirely a social problem, and that technology changes will have little effect

So far I see 3 aspects (social, technical, organizational), 2 roles (contributor, reviewer), and 3 factors ("time-to-review/-merge", "patch acceptance likeliness/probability", and "contributor onboarding" which is way less relevant in the context of CR as it happens before, but interesting in a Developer-Advocacy context).

In T114419#1876179, @ori wrote:

I'd like to make a counterproposal, and that is that we abolish mandatory code-review, and instead allow people to exercise discretion and self-merge patches when they see fit. This is how operations/puppet works (in practice if not in theory), and how MediaWiki development used to work.

I argue that our code review process --

• Reduces personal accountability by blurring and distributing responsibility for good outcomes.
• Discourages innovation by implicitly selecting for boring changes.
• Wastes enormous amounts of developer time and attention.
• Demotivates and demoralizes contributors.
• Slows development.
• Fosters code rot.
• Makes software development joyless.

I conclude that it degrades rather than improves the quality of the software.

Given the lack of substantiation and other flaws, I find this argument weak.

Is there a similar profession to code development in which there are no reviewers? If a magazine author writes a piece, she has an editor look over and review her work. Sometimes there's also a dedicated fact-checker and others involved. Does this slow development? Of course, but that's an accepted cost for creating a better product.

Like @Krenair, I think your proposal would only possibly help a few people. With exceptions, it seems that typically people with +2 are able to find others to review and merge their changes. It's the people who don't have +2 whose changes lag for months and years.

In T114419#1749653, @Legoktm wrote:

Technology really isn't the problem here. It's mostly a social one.

Agreed. Having better visibility into the code review bottlenecks and backlogs might be helpful.

In many professional fields, and quite often in commercial software engineering, 'Senior' in a job title means the person no longer creates stuff , and only does reviews all week long, and in effect is constantly training the juniors.

This is part of differences wrt XEmacs, with their 'Reviewers' expected to avoid submitting patches, especially when the review queue is not empty. The prestige comes from prioritising review of other peoples work before your own pet projects.

In T114419#1876179, @ori wrote:

I think we can retain most of the benefits provided by code review by making it optional rather than obligatory.

This seems to rest on two assumptions:

• code review mainly sucks for people with +2 (or at least they are a large enough part of the sucker population that special-casing them takes us significantly closer to solving the whole issue, or we could distribute +2 liberally enough to make a +2-requiring solution work for most people)
• most of the patches which do not get timely review are suitable for self-review

I don't know the numbers for the first one but I would not bet money on it. And in general solving a problem only for the people who can do something about it is dangerous - the problem remains but the people who can do something about it don't perceive it anymore. (You cannot expect fresh volunteers to organize a summit discussion about the state of code review.)

I'm pretty sure the second assumption is false - it is usually easy to get code review on small, trivial changes, it's the big complex changes that take forever, and those are exactly the ones where code review is important. Also, punishing people who are cautious with huge delays could cause some unhealthy incentives.

One thing I miss a lot in gerrit is an "intent to merge" option (basically, +2 without merging). Often I think something is ready to merge but has been uploaded recently and I want to give other interested parties time to review; but I want to be able to find it later as something that just needs a merge button press (or worst case I want the patch owner to be able to find me and poke me about merging). With gerrit I have to use +1 for that, which is not ideal because I also use that for "looks good but I don't have the expertise to judge".

As I understand this is something that will be possible with Differential.

One thing I miss a lot in gerrit is an "intent to merge" option (basically, +2 without merging)

Does C+2 V-2, C+1 V+2 or something similar work?

I went ahead pre-scheduling T114419: Event on "Make code review not suck" in the main slot for the Collaboration area, on Tuesday at 2pm: https://www.mediawiki.org/wiki/Wikimedia_Developer_Summit_2016#Tuesday.2C_January_5

Feel free to edit the description of the session. I extracted it from T114419#1872365

About the current discussion, I think progress can be done in two key aspects:

• The definition of the problem: what is objectively and measurably wrong, frustrating, not sustainable in the current situation. Let's focus on the data we have today.
• The desired scenario: what is reasonable to expect from code contributors, reviewers, and maintainers?

Agreeing on what really matters about the current situation and the desired destination looks like a good step before digging too deep into solutions.

In T114419#1883882, @Nemo_bis wrote:

One thing I miss a lot in gerrit is an "intent to merge" option (basically, +2 without merging)

Does C+2 V-2, C+1 V+2 or something similar work?

No, V+-2 means something different and is set/overridden automatically by the CI bot.

Let's focus on the data we have today.

I'm not sure if we should necessarily make currently available data the focus. There's a danger there in bending desires to fit what we can currently measure. I think its better to go from a place of problems, and then ask how to measure it. If we are lucky, its what we are already measuring. If we are unlucky, well its better to know what we need to measure instead, then to make decesions based on the wrong data.

@Qgil: you implicitly put @Bawolff in charge of the entire 80 minutes. I put T119030: WikiDev 16 working area: Collaboration back on the mw.org version of the Tuesday schedule, since my conversations this week have indicated some confusion over the "disappearance" of this area. Was your intention for @Bawolff to be responsible for the entire 80 minutes? Do any of the other sessions in T119030 merit any mention in this session? If you are planning to hand the entire 80 minutes over to @Bawolff, then @Bawolff, are you ready to take this on?

In T114419#1876179, @ori wrote:

I'd like to make a counterproposal, and that is that we abolish mandatory code-review, and instead allow people to exercise discretion and self-merge patches when they see fit. This is how operations/puppet works (in practice if not in theory), and how MediaWiki development used to work.

I argue that our code review process --

• Reduces personal accountability by blurring and distributing responsibility for good outcomes.
• Discourages innovation by implicitly selecting for boring changes.
• Wastes enormous amounts of developer time and attention.
• Demotivates and demoralizes contributors.
• Slows development.
• Fosters code rot.
• Makes software development joyless.

I conclude that it degrades rather than improves the quality of the software.

I think we can retain most of the benefits provided by code review by making it optional rather than obligatory.

Most of E130: Informal WikiDev '16 agenda bashing session (2015-12-23) ended up being about this topic. In particular, see 22:17 through 22:54 in https://tools.wmflabs.org/meetbot/wikimedia-office/2015/wikimedia-office.2015-12-23-22.01.log.html

In T114419#1882811, @Tgr wrote:

One thing I miss a lot in gerrit is an "intent to merge" option (basically, +2 without merging).

I miss that when I would merge something but don't have permission, to distinguish it from "I reviewed this part, but the rest still needs review". CR+1 also sometimes gets used for I want this change that its commit message promises, but no review happened. I get by with the existing vote options, but maybe a more differentiated voting table would encourage being clearer about the meaning.

All this is configurable in gerrit. One e.g. can add other rows to the votes table and change their range.

I miss that when I would merge something but don't have permission, to distinguish it from "I reviewed this part, but the rest still needs review".

To indicate that they tested the code etc., some use CR+1 V+1.

My experience of certain self merges that were done are: Deployments on weekends that take down the site. Changes that were not socialized beforehand. Changes that broke stuff that was not user visible and thus then where then not cleaned up. A commit deleting all unit test with the comment that someone just wanted to maximize the lines they wrote. Is this the way of working we want to encourage?

In T114419#1904419, @JanZerebecki wrote:

My experience of certain self merges that were done are: Deployments on weekends that take down the site. Changes that were not socialized beforehand. Changes that broke stuff that was not user visible and thus then where then not cleaned up. A commit deleting all unit test with the comment that someone just wanted to maximize the lines they wrote. Is this the way of working we want to encourage?

• Each such example could be matched by examples of similarly shoddy code-reviewed commits.
• All these occurred under the current code-review regime, showing that its application is inconsistent and enforcement uneven.

I just self merged two things this weekend, that were minor and rather self contained. I don't do that often, usually I either just give up, or wait another 2 months....

@TheDJ Lucky you, at least you can do it yourself; and a prolonged code review is merely a nuisance when you have +2

In T114419#1904419, @JanZerebecki wrote:

My experience of certain self merges that were done are: Deployments on weekends that take down the site. Changes that were not socialized beforehand. Changes that broke stuff that was not user visible and thus then where then not cleaned up. A commit deleting all unit test with the comment that someone just wanted to maximize the lines they wrote. Is this the way of working we want to encourage?

Both of these categories strike me as retrospective report material:

• Deployments on weekends that take down the site
• Changes that broke stuff that was not user visible and thus then where then not cleaned up

This one is rather vague, though I don't think I would have to try too hard to come up with an example:

• Changes that were not socialized beforehand.

Then there is this one, which I kinda wanna know what the heck you're talking about:

• A commit deleting all unit test with the comment that someone just wanted to maximize the lines they wrote.

For retrospective report material, we should reinforce the norm of writing up incident documentation when we introduce a failure this way. For changes making sure changes get socialized well, we have the MediaWiki RFC process.

As I said in the E130 discussion, I think the biggest catch is dealing with security and privacy. Privacy and security mistakes often can't be undone. It may not be sufficient to allow for sincere contrition, but rather, we need to be prepared to deal with the case where a committer has demonstrated their inability to be trusted. Peer review is one longstanding proxy for trust. It seems reckless to abolish it without a sensible replacement. I'm particularly interested in hearing from @csteipp, @tstarling, Security, and SRE on this.

Thoughts?

For changes making sure changes get socialized well, we have the MediaWiki RFC process.

Not really, that works for developers only (and only really for those who like to attend IRC meetings during the European night).

In T114419#1905777, @Nemo_bis wrote:

For changes making sure changes get socialized well, we have the MediaWiki RFC process.

Not really, that works for developers only (and only really for those who like to attend IRC meetings during the European night).

Let me clarify what I was trying to say: we have an Architecture Committee to help ensure big changes get a proper amount of socialization. There is nothing that says that an IRC meeting is required for a MediaWiki RFC to move along; that's one way to document that an idea is well-socialized. In fact, we don't technically require MediaWiki RFCs for large architectural changes. I don't think we should make this a requirement, but rather, we should make this an increasingly appealing social norm that anyone reviewing code can suggest as part of their review (e.g. "This looks like it has a lot of ripple effects. I think you should probably move an RFC through the process for this before merging it")

Let me clarify something. I'm not against mandatory code review. I have a slight problem with the "someone else needs to merge" policy. Sometimes you are stuck looking for a +2 merge person that is comfortable merging in the area you are working on. And I don't have the time to continuously be prodding people to motivate them to look into and learn about the thing that I'm doing, just for them to be comfortable enough to merge. And the further outside the beaten path you operate, the worse that problem is.

In T114419#1903658, @RobLa-WMF wrote:

Was your intention for @Bawolff to be responsible for the entire 80 minutes?

Yes, this is clearly the Collaboration proposal that is most relevant and is attracting more discussion.

Do any of the other sessions in T119030 merit any mention in this session?

It is not a matter of merit but of focus on one topic for one slot. Besides, being the last slot on Tuesday, the other sessions will be completed by then.

In T114419#1905752, @RobLa-WMF wrote:

Both of these categories strike me as retrospective report material:

Do you mean https://wikitech.wikimedia.org/wiki/Incident_documentation which is for "site outages"?

Then there is this one, which I kinda wanna know what the heck you're talking about:

• A commit deleting all unit test with the comment that someone just wanted to maximize the lines they wrote.

The person in question expressed that the only reason for writing unit tests (it was not mentioned if that was somehow specific to the language or task or meant as a general truth) is to increase the line count, possibly implying that the engineer who wrote the unit tests was being assessed by metrics looking at line count. Then self merged the commit deleting the unit tests. But I should not have brought that up here, it is mostly off topic.

In T114419#1905845, @TheDJ wrote:

I'm not against mandatory code review. I have a slight problem with the "someone else needs to merge" policy.

Is there an important difference? If someone with +2 rights says "good to be merged, +1" it is equivalent to "+2". This is important to retain for things that may only get a +2 for deployment directly afterwards. (Though one shouldn't deploy without having others available that are capable of incident response.) Also, one typical failure mode with the word "mandatory" here is that it is easy to undermine the intent via meat-proxy-merge, all this relies on trusting the people involved.

Sometimes you are stuck looking for a +2 merge person that is comfortable merging in the area you are working on.
And I don't have the time to continuously be prodding people to motivate them to look into and learn about the thing that I'm doing, just for them to be comfortable enough to merge.
And the further outside the beaten path you operate, the worse that problem is.

This is one of the things that I think are the most important aspect of this task, especially so because its effect is worse for people who don't have +2 rights.

One reason for the need to "continuously be prodding people" is for some people that a) they have more to review than time and b) they get so many notifications that they discard all of them because there is no way to only look at the high priority ones.

As I said in the E130 discussion, I think the biggest catch is dealing with security and privacy. Privacy and security mistakes often can't be undone. It may not be sufficient to allow for sincere contrition, but rather, we need to be prepared to deal with the case where a committer has demonstrated their inability to be trusted. Peer review is one longstanding proxy for trust. It seems reckless to abolish it without a sensible replacement. I'm particularly interested in hearing from @csteipp, @tstarling, Security, and operations on this.

I definitely see security and privacy issues addressed in code review-- unfortunately we don't have comment indexing in gerrit, so I can't search for instances of "xss" or "injection" in gerrit comments. But I would guess it's >1/mo. I would love to be able to actually run that search to see how often patches by users with +2 have security issues caught in code review. If the answer to that is near zero, then it doesn't make sense as a control.

That said, anyone who +2's, whether for their own patch a patch by another developer, should be following a set of reasonable standards, including checking for security and privacy issues.

In T114419#1903658, @RobLa-WMF wrote:

@Qgil: you implicitly put @Bawolff in charge of the entire 80 minutes. I put T119030: WikiDev 16 working area: Collaboration back on the mw.org version of the Tuesday schedule, since my conversations this week have indicated some confusion over the "disappearance" of this area. Was your intention for @Bawolff to be responsible for the entire 80 minutes? Do any of the other sessions in T119030 merit any mention in this session? If you are planning to hand the entire 80 minutes over to @Bawolff, then @Bawolff, are you ready to take this on?

That's slightly more time then I was expecting, but there seems to be many people with differing opinions, so I think this topic can fill that size of a time slot. So yes, I'm ready to take that on.

unfortunately we don't have comment indexing in gerrit, so I can't search for instances of "xss" or "injection" in gerrit comments

As https://www.mediawiki.org/wiki/Gerrit/Navigation#Full-text_search suggests, you can search the mailing lists:

• http://markmail.org/search/?q=xss+%22Gerrit-Project%3A+mediawiki%22+list%3Aorg.wikimedia.lists.mediawiki-cvs+order%3Adate-backward
• http://markmail.org/search/?q=injection+%22Gerrit-Project%3A+mediawiki%22+list%3Aorg.wikimedia.lists.mediawiki-cvs+order%3Adate-backward

That said, anyone who +2's, whether for their own patch a patch by another developer, should be following a set of reasonable standards, including checking for security and privacy issues.

And this is documented as part of https://www.mediawiki.org/wiki/Category:MediaWiki_development_policies , at https://www.mediawiki.org/wiki/Security_for_developers and https://www.mediawiki.org/wiki/Security_checklist_for_developers .

In my experience the current pre-merge code review we have is great, even if it sometimes results in simple patches waiting a long time. People caught so many embarassing mistakes in my code, and I have caught so many in theirs. As someone who joined just after the big Git migration, it's amazing to me how the whole project hasn't collapsed in SVN times. :)

It's true though that at least some of these caught mistakes resulted from people (including me) simply not testing the changes before they submit them for review, on the assumption that whoever bonks the C+2 button will do it. We could maybe start using the V+1 vote to mean "Yes, I actually checked whether this works". One thing I like about Differential is that it has a mandatory "Test plan" field when submitting code for review.

If we hypothetically were to remove mandatory code review, I think we should make reverting problematic changes less of a big deal. I feel really bad reverting anything that's not my own change currently. I don't necessarily want to fix everything myself, and leaving a comment on the merged changeset is hit-or-miss (many people simply don't read emails from Gerrit), often resulting in the newly introduced bugs not getting fixed at all.

In T114419#1906664, @JanZerebecki wrote:

One reason for the need to "continuously be prodding people" is for some people that a) they have more to review than time and b) they get so many notifications that they discard all of them because there is no way to only look at the high priority ones.

I think these are two good points. The WMF development teams are too wedded to the quarterly schedules, and non-trivial changes submitted by volunteers often have to wait until whichever quarter when the team's goals align. Gerrit also spews emails like crazy and I know that many people simply filter them all out of their inbox. (I mostly manage to keep up only thanks to a bunch of filters that sift through the junk for me.)

And this is documented as part of https://www.mediawiki.org/wiki/Category:MediaWiki_development_policies , at https://www.mediawiki.org/wiki/Security_for_developers and https://www.mediawiki.org/wiki/Security_checklist_for_developers .

I don't think those capture what I mean. One of @Bawolff's points was that code reviews vary widely, depending on who is doing the review. The closest we have to documenting a standard process is https://www.mediawiki.org/wiki/Gerrit/Code_review, but (imo) those points can be interpreted in fairly diverse ways. I think it would be beneficial to tighten up the recommendations (with examples), so that code reviews are more predictable.

I think we should make reverting problematic changes less of a big deal

I think reverting becoming a big deal is one of the sadder outcomes of the current code review situation. I think reverting should be no big deal regardless.

As an outcome of T101686: Goal: Define potential actions to reduce code review queues and waiting times I have collected a bunch of potential Code Review factors and potential {social, technical, organizational} actions on https://www.mediawiki.org/wiki/User:AKlapper_%28WMF%29/Code_Review

That list is long but is hopefully an interesting read.
For this WikiDev Summit 2016 session, I would prefer to focus on aspects with unclear solutions and no consensus (instead of items that everybody agrees on anyway) and making the discussion go around these points.

I would love to focus on

• whether we should have a three-steps structured review approach for patch reviewers
• if we have enough skillful and available confident reviewers and how to increase that number
• how to decrease the lack of repository owners and or under-resourced or unclear responsibilities in certain code repositories,
• and only if we have time left, how to make it easier for patch contributors to add the 'right' potential reviewers to their proposed patches.

In T114419#1912376, @Aklapper wrote:

• and only if we have time left, how to make it easier for patch contributors to add the 'right' potential reviewers to their proposed patches.

FWIW, Phabricator has an 'owners' tool that would allow us to automatically add appropriate reviewers which can be teams, not just individuals.

From the task description:

I propose that every single patch (to MW core) has a single person responsible for it. Which person that is should be deterministic and known at all times. That person is responsible for either merging that patch, rejecting the patch outright, -1'ing the patch for something fixable, or finding somebody else to assign the patch for review (If they're not comfortable reviewing it). Of course people should be able to reject patches on the basis of this is three thousand lines without a code comment, or you've submitted 10 patches already where you tested none of them and there are fatal errors in all of them, I'm not reviewing anymore of your patches. Furthermore, the review-owner person should do one of these actions in no more then 1 week time since the patch is submitted. People who don't review things in a timely manner are removed as a reviewer.

Which person is responsible for a patch could be determined by having a list of paths in MW-core (for bonus points, also wmf-deployed extensions), where each path is owned by 1 or more people. Whichever file the patch touches the most lines of, whomever "owns" that file, is responsible for the patch (If there's multiple people, determine by doing it mod the patch number or something)

Having someone "own" a particular file doesn't mean only they can review stuff for it, anyone should still be able to review stuff, it just means that they have to review it.

Additionally, there would be a gerrit-wrangler, similar to Andre, but for gerrit, to ensure patches are getting reviewed, and nothing falls through cracks.

Cons to this plan: Might require significant resource commitment by WMF, which might be a hard sell. Difficult to enforce that volunteers must follow some deadline.

This is exactly what owners was designed for and the tooling provides for everything proposed here. Additionally, there would not be any need for a "wrangler" because the tools facilitate this with minimal effort.

Adding an owners tag to each patch will only fix the problem if each of these groups (at least when non-volonteers) also has a clear mandate, and the allocated time, to handle any incoming patches.
Otherwise we are back at the current situation albeit with the improvement that it is now clearer who/which team to poke to guilt them into doing a code review.

In T114419#1906664, @JanZerebecki wrote:

In T114419#1905752, @RobLa-WMF wrote:

Both [deployments on weekends that take down the site] and [user-invisible breakage] strike me as retrospective report material:

Do you mean https://wikitech.wikimedia.org/wiki/Incident_documentation which is for "site outages"?

Yup. Incident documentation should be for things we classify as a "big consequence incident", which is an incident involving any mistake, misstep in a process, adherence to a flawed process, or any other action that has big negative consequences. We document these things so that we can study the situation and figure out how to ensure we don't have to keep suffering bad consequences.

In T114419#1907952, @matmarex wrote:

If we hypothetically were to remove mandatory code review, I think we should make reverting problematic changes less of a big deal.

Absolutely. Back in the days when post-commit review was the norm, this was a huge problem, and we sometimes let bad ideas stay in the codebase because we put the responsibility of code quality (and thus the responsibility to revert bad ideas) on too few. Reverting wasn't worth the fight.

Our reverting norm in our code should probably be bold, revert, discuss. Just note that that brings challenges of its own, because while an experienced Wikimedia developer might have the proper "no big deal" attitude about the revert, newcomer developers may get a terrible first impression of us if their change is reverted.

One other point: the bold, revert, discuss (BRD) cycle on code only works well when the "revert" step is done quickly. The dangerous thing behind relying on BRD is if someone is able to sneak a bad idea into the code that "works" (i.e. passes all the tests, provides acceptable functionality, doesn't cause any noticeable regressions), but that unintentionally introduces a long-term design problem. How do we maintain conceptual integrity in this environment?

Can someone copy the notes of this session and post the actions items (as subtasks if needed), please?

In T114419#1927025, @Qgil wrote:

Can someone copy the notes of this session and post the actions items (as subtasks if needed), please?

Im planning to, once im back home (tomorrow)

In T114419#1927025, @Qgil wrote:

Can someone copy the notes of this session

I've done that by pasting them in the task description here.

and post the actions items (as subtasks if needed), please?

Still TODO, action items here will very likely also block T101686 and actions in T78768.

If I recall correctly, @greg, @ori, and... ? talked about the creation of a working group to analyze this problem and bring recommendations for the (WMF?) development teams. Is this correct?

@Aklapper can help with research, proposals, and probably some wrangling, but we will need more muscle to push actual changes in routines within the WMF teams. Also, I wonder whether T113707: Decision on Wikimedia Foundation service-level agreement on code review times is considered as a feasible, desirable goal.

In T114419#1933847, @Qgil wrote:

If I recall correctly, @greg, @ori, and... ? talked about the creation of a working group to analyze this problem and bring recommendations for the (WMF?) development teams. Is this correct?

@Aklapper can help with research, proposals, and probably some wrangling, but we will need more muscle to push actual changes in routines within the WMF teams. Also, I wonder whether T113707: Decision on Wikimedia Foundation service-level agreement on code review times is considered as a feasible, desirable goal.

I don't remember that commitment being made, and it doesn't appear to be captured in the notes.

The last that I had heard, @ori was making the case that we should move to a similar ethic to the operations/puppet repo, where self-merges are allowed, but responsibility lies with the whoever merged the patch. If we do something like this, I believe we also need to have retrospective reports when we slip up. I've filed T123753: Establish retrospective reports for #security and #performance incidents as a proposal to build some good habits in this area.

In T114419#1937053, @RobLa-WMF wrote:

In T114419#1933847, @Qgil wrote:

If I recall correctly, @greg, @ori, and... ? talked about the creation of a working group to analyze this problem and bring recommendations for the (WMF?) development teams. Is this correct?

@Aklapper can help with research, proposals, and probably some wrangling, but we will need more muscle to push actual changes in routines within the WMF teams. Also, I wonder whether T113707: Decision on Wikimedia Foundation service-level agreement on code review times is considered as a feasible, desirable goal.

I don't remember that commitment being made, and it doesn't appear to be captured in the notes.

It wasn't captured in the notes, but yes, that was @ori's suggestion that seemed like a reasonable way forward (iow: no decree yet, get more information, make a recommendation).

In T114419#1937062, @greg wrote:

It wasn't captured in the notes, but yes, that was @ori's suggestion that seemed like a reasonable way forward (iow: no decree yet, get more information, make a recommendation).

To put more words in @ori's mouth, if my memory serves me, his words were approximately

"we need a code review committee with some authority to drive this forward." (paraphrased, roughly)

Now to put my own spin on it:

I think what he was getting at is that we need a code review group with enough authority to actually answer some of the questions that came out of the discussion at Wikimedia-Developer-Summit-2016
We raised a lot of questions without providing many answers. We really need answers in order to move forward. Ideally we can come up with a cohesive plan to improve many of the serious problems we have identified with the existing state of code review on Wikimedia projects.

Wikimedia Developer Summit 2016 ended two weeks ago. This task is still open. If the session in this task took place, please make sure 1) that the session Etherpad notes are linked from this task, 2) that followup tasks for any actions identified have been created and linked from this task, 3) to change the status of this task to "resolved". If this session did not take place, change the task status to "declined". If this task itself has become a well-defined action which is not finished yet, drag and drop this task into the "Work continues after Summit" column on the project workboard. Thank you for your help!

FYI, T128371 has progress, we got a codereview channel now, and can setup code review hours, so it would be nice if someone of you want's to join too :).

Making code review its own ghetto channel doesn't send a good signal to me.

I think it's a good way, because if we use a channel which is already used, code-review gets mixed up with discussion, but if we got a seperate channel, where we can for example mark people with +2, or where the topic is focussed at code-review, the whole disussion is fixed at that topic, so we can work more focussed.

In T114419#1876178, @ori wrote:

I'd like to make a counterproposal, and that is that we abolish mandatory code-review, and instead allow people to exercise discretion and self-merge patches when they see fit. This is how operations/puppet works (in practice if not in theory), and how MediaWiki development used to work.

Ori elaborates further in that comment, and has held pretty firm on that position. Furthermore, no one has yet made a compelling counterargument.

Over in T123753, @Krenair made a pretty bold statement arguing against this position

In T123753#2373240, @Krenair wrote:

From a MediaWiki perspective, operations/puppet seems to be the opposite of the direction we should be heading in. It won't improve code review, it will make some privileged people (yes, including me) able to bypass it, and it will work against us getting code from people who are not already privileged accepted.

I don't have a strong opinion about which side to take, but @Krenair, I'm hoping you can articulate a more detailed response to Ori's earlier comment.

In T114419#2373286, @RobLa-WMF wrote:

In T123753#2373240, @Krenair wrote:

it will work against us getting code from people who are not already privileged accepted.

The code review requirement does not put everyone on equal footing. Only some people have the right to +2, and this right makes a big difference when it comes to getting your own code reviewed. You are in a position to help people ship code, which people like and appreciate, and they are moved to treat you in kind and reciprocate.

I am not talking about conspiratorial quid-pro-quo arrangements, where I agree to merge your code on condition that you merge mine (although that happens, too); what I'm describing is innocent and largely unconscious. If you have a situation in which everyone can receive presents but only some people can give them, you'll find that over time the people who can give presents will gift each other and ignore the rest.

In T114419#2373286, @RobLa-WMF wrote:

Ori elaborates further in that comment, and has held pretty firm on that position. Furthermore, no one has yet made a compelling counterargument.

• T114419#1876250
• T114419#1876508
• T114419#1876765

Maybe I'm misunderstanding who would have +2 in the proposed future world where mandatory code review is abolished. Would anyone with a Gerrit account have +2 and code review would be optional for every registered user?

In T114419#2373333, @MZMcBride wrote:

In T114419#2373286, @RobLa-WMF wrote:

Ori elaborates further in that comment, and has held pretty firm on that position. Furthermore, no one has yet made a compelling counterargument.

• T114419#1876250
• T114419#1876508
• T114419#1876765

Thank you for collecting up that set of comments. I had admittedly skimmed over them. Those do deserve more careful consideration.

Maybe I'm misunderstanding who would have +2 in the proposed future world where mandatory code review is abolished. Would anyone with a Gerrit account have +2 and code review would be optional for every registered user?

That seems obviously incorrect, but I admittedly don't know what the correction is.

In T114419#2373300, @ori wrote:

The code review requirement [puts those with +2] in a position to help people ship code, which people like and appreciate, and they are moved to treat [those +2 holders] in kind and reciprocate.

This aspect of the social dynamic is what is rough about the current regime; those without +2 aren't in a position to build as much social capital, and furthermore, those with +2 aren't sufficiently motivated to elevate more people into privileged group. Questions for everyone (not just Ori): how can we have more inclusiveness in the +2 group, while still providing reasonable vetting? What constitutes "reasonable vetting"?