Event on "Make code review not suck"
Closed, ResolvedPublic

Description

Problem definition

Recently I've been finding the code review system frustrating and demotivating. In some cases, this has probably reduced the amount I've contributed to MediaWiki. Primarily I'm frustrated by:

  • Lack of standardization over what is looked for in code review
  • Long delays (measured in months) before getting any sort of review
  • Seeming randomness in the code review process (One day my code is reviewed in an hour, next patch takes 3 months. Different patches seem to have different standards applied to them, in a fashion that seems totally arbitrary)

In more detail:

Sometimes it seems like what will get you a -1, is rather random, and varies a lot depending on who is reviewing your code. To use unit tests as an example, Sometimes a small change in legacy code, where it is rather difficult to add unit tests due to code structure, and which seems unlikely to have future regressions, will be -1'd unless it has unit tests to the max. Other times entirely new code which would be trivial to add unit tests to, get merged without any at all.

I would like code review to be more deterministic. I would like -1's to be something that would be easy to predict before hand, and when they happen, be more like d'oh moments, rather then feeling random. This is particularly true when combined with long delays with code review, which I'll get to next:

Delays:

Sometimes it takes months (in the plural) before convincing anyone to look at your patch. This can be sped up by nagging people, but I do not want to spend all my time begging people for code review over and over. Other times it can be unclear who to add to a patch. Having code review take such a long time can be rather demotivating, as what's the point if nobody looks at your patch for six months, followed by a -1 over something silly, followed by another long waiting period. Additionally, once so much time has passed, usually I don't care about the bug the patch is trying to fix anywhere near as much anymore. If the patch is non-trivial, I've probably totally forgotten by this point what the patch was even trying to do. When someone -1's a patch a submitted a couple days ago, I'm likely to fix it. When someone -1's a patch I submitted several months ago, I'm more likely to say screw it, why did I care about this issue in the first place.

In many ways, code review epitomizes the saying that when everyone is responsible for something, nobody is responsible. (Sumana would probably say tyranny of structurelessness - http://www.jofreeman.com/joreen/tyranny.htm )

Code review tends to be fastest in very active areas of MediaWiki, or for people who work in teams (and review each other's code). Social connections can also play a key role, as often code review is done in the form of asking for favours. As such WMF employees are at a significant advantage here as usually WMF teams tend to work in one area of the code base in-force, and are almost always in teams (However, many WMF employees have complained that the CR situation is very frustrating when they have to touch code that's not directly their team's).

Expected outcome at summit

But, its easier to complain then to come up with solutions. With that said, I imagine this as primarily a brain-storming session. People have been talking about the Code review problem for ages - I believe we need radical and out of the box social changes to truly fix it.

Thus the goals of this meeting are:

  • Is this just me? Do other people feel the same way? Should I just get over it? How does this issue affect different groups to different degrees?
  • What are the root causes of frustrations
    • And especially, try to distinguish the root causes from mere symptoms.
  • Come up with plans to fix the code review system once and for all.
    • Ideally for various ideas, we would suss out which root causes we expect the idea would fix
  • Work out a draft policy for how code review should be conducted (Like code conventions document, but for quality instead of spacing). What issues should trigger a -1. Are there things that the author should be encouraged to do. While I think this is something fairly uncontroversial that could help the situation, I would rather keep this meeting focused on brainstorming. If we do do a CR guideline, it should be separate from this meeting. Given how late in the game this is, perhaps it should be during an unconference spot.

Non-goals:

  • Agree on a final solution

Ideal outcome:

  • We have a number of ideas on how to fix the problem
  • While we may not have decided which idea to implement, people come away from the meeting feeling that at least one of the ideas would fix things, and that there is hope for a brighter tomorrow.
  • A summary of the ideas is presented to wikitech-l for futher evaluation, in the hopes that the wikitech-l discussion will narrow down the choices to the best ones.

Proposed solution to reduce delays

This is a bit radical, but I'm presenting it here as a concrete proposal to at the very least move discussion forward. And at least have something to talk about other then, yeah the status-quo is kind of sucky

The main problem is that nobody specifically is responsible for code review, so things fall through the cracks, and its hard to resuce things from the cracks, when there's nobody specifically responsible.

So lets change that.

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.

Related tickets, background information

  • T101686: Goal: Reduce code review queues and waiting times (There's some good identification of problems on this one)
  • T78768: Goal: Plan to prioritize code review of patches submitted by volunteers
  • T101099 : Developer-Relations roadmap. dev-relations team wants to work on this issue. They have some interesting discussions about it over there
  • T113378 Lessons learned from the first Gerrit Cleanup Day - Some interesting stuff here. And well clean-up day was a nice effort, imho, it is at best a band-aid solution, and really not even that, unless we start having monthly clean-up days. Its not like there is just a big back-log that needs to be cleaned and we'll be fine going forward, Code review is an on-going problem.
  • http://korma.wmflabs.org/browser/gerrit_review_queue.html : Some stats
  • T114320: Code-review migration to Differential status/discussion

Developer-Relations Does have some plans to try to alleviate the issue. I haven't really engaged much with them on this issue before filing this (Which WMDS page suggests I maybe should have), but writing this session proposal was kind of a spur of the moment thing I decided to, when I work up this morning and realized the deadline to write this was basically today.


Slightly edited Etherpad session notes:

Goals

  • What are the areas where people are frustrated. What are the root causes
  • What are potential social changes we could do to reduce these frustrations
    • In particular, I want to emphasize that I want to focus on social proble ms first, not technical solutions
    • This meeting is meant to brainstorm root causes and solutions. We are not agreeing on a final solution

The ideal outcome is a summary of the ideas generated is presented to wikitech-l for futher evaluation, in the hopes that the wikitech-l discussion will narrow down the choices to the best ones.

Agenda

  • 10 minutes: session introduction
  • 25 minutes: Discussion of root problems of frustration. Or if there even is frustration
  • 30 minutes: Discussion of potential solutions to problems
  • 15 minutes: Conclusion. How do people feel about what's been discussed. Reflections on stuff discussed. Are there solutions that specificly excite people. Next steps.

Summary

Brian introduced the session. (note: the scribe isn't responsible for the summary)

  • What do other people find frustrating about code review?
    • How does a reviewer find reviewable patches?
    • How does a contributor find the person responsible for the code? Especially for code where no one is "responsible"? What about patches that add/change responsibilities?
    • Are we too conservative about what we accept? Why do things get stalled?
    • Is operations/puppet more lively than the MediaWiki codebase? (one person claimed code Review makes development un-fun)
    • How do you learn how to take responsibility for sections of code? Is CR a good learning opportunity for developers?
    • How do you budget time to do reviews for what you are responsible for?
    • Is code review necessary? How do we catch security bugs without CR? How should we treat people who make mistakes? Does our current process ensure secure code? How do we focus review on the important stuff, rather than nitpicking? Are there restrictions that force us to have CR?
    • Is a governance model necessary for CR?
  • What should we do differently?
    • Should we require a maintainer be identified for each patch? Should we assign patch review to specific individuals on a per patch basis? Should we ask for people to agree to be assigned maintainers? Do we need a more structured approach? What is the reward for being a maintainer? Can we make reviewers more prominent? Rewarding metrics? What if the assigned reviewer is too busy?
    • how can we make non-blocking comments on a patch?
    • Training sessions where main contributors explain the broader background and their plans for areas of code?
    • ANSWERED: Can we have office hours for code review? Is there value in synchronous review? +2 Yes!
    • What about post-commit discussion/improvement? Do github-style pull requests and feature branches work? Does Phab's model work as a middle ground between Github and Gerrit?
    • Are +1s useful at all? Is it a signal for if the reviewer read/understood the code? Is a +1 useful without a comment? If someone doesn't have +2, is it still useful? Does a +1 signal risk?
    • Does Gerrit embody/imply the policy we want? +2/+1/0/-1/-2 work?
    • How can we invite outside experts into CR in a patch? (e.g. localization?)
    • Is automerge a good idea? Does auto-merge demotivate reviewers?
    • Should we migrate to Phabricator?
    • Should we assign owners to areas of code?
    • Can our CR process scale to early Linux level? In CR, is being a jerk better than being silent? Is our code modular enough for this?
    • Have we got/do we need Code review guidelines?
    • Should we have a CR committee? Does a CR committee have sufficient ROI?
    • Are there good examples of WMF teams we can emulate?

Action items:

Detailed notes

[bawolff] Talk about problems people have, what we can do differently. Focus on social conventions, less on technology. What do we ideally want, brianstorm ideas
[bawolff] CR is random, from time to the depth of reviews, like playing roulette. Would like it to be consistent.
[legoktm] Most frustrating is that when I have free time, it's hard to find reviewable patches. And when I need review myself, I ping people on IRC, because they don't respond to just Gerrit requests.
[cindy] For me, hard to figure out who should review the code, unsure who the one person is to review. "when everyone is responsible, no one is responsible", differing opionions on how to implement features, no one to arbitrate that
[bawolff] CR responsibility is not shared enough
(etherpad) [legoktm points to http://koti.kapsi.fi/~federico/crstats/core.txt / http://koti.kapsi.fi/~federico/crstats/extensions.txt ]
[ariel] bunch of MW code that is no longer owned because there is no MW Core team, no one is responsible (literally)
[bawolff] that's bad
[ori] I'm not a good code reviewer, ready to help with debugging in prod/handling fallout, wants to greenlight a patch even if they are good patches from trusted contributors; no socially acceptable way to take risks with patches.
[ori] Compare ori's history in ops/puppet vs mediawiki/core, way more changes in puppet even though more interested in MW. In core CR can be "a drag", people don't take chances or deviate from patterns, doesn't let you build momentum or energy. (ops/puppet allows self-merging). Be more open to risk.

  • (etherpad) It's easy to size up the cost and risk that comes with modifying code, less easy to size up the cost and risk that comes with erecting barriers to contributions, letting code rot, etc. Our practices are geared toward preventing the former, but do nothing to prevent the latter.

[bawolff] summarizes as CR saps creativity
[andre_] reviewers vs. contributors. Should we have a more structured process when it comes to reviewing code: blog post by Sarah Sharp? need more skillfull/confident reviewers? reduce areas where no one is responsible. Go through rotting patches and assigning someone who is responsible? (Long version: https://www.mediawiki.org/wiki/User:AKlapper_%28WMF%29/Code_Review )
[isarra] I have +2 in mediawiki/skins/*, but don't know how to review things
[bawolff] there are no instructions for doing CR except for "don't screw up", no guidelines for what's okay or not okay. what's acceptable/minor/etc. You don't want to be the one who +2's something that wasn't supposed to be +2'd.
[isarra] If people don't have time to review themselves, then definitely don't have time to teach others
[marxarelli] Maintains mediawiki-vagrant, have event in calendar to just review code. considered doing office hours to do 1:1 reviews with people? (notes that vagrant reviews can be slow, initial provisions can take 15min). Dedicated office hours type time
[csteipp] CR is necessary. From security perspective, non-zero number of security issues caught in CR. Standardize CR, make it better. We're too hard on people when they make mistakes.
[bawolff] CR is the job that is thankless, no one gets appreciation for doing a lot of CR, compared to writing patches/fixing bugs. CR'ers take on the risk for a patch, but no reward
[adamw] CR is one of favorite parts of this job. Hallway conversations...? Actual training would be effective, people interested in certain areas of code could teach others about that, problems they see, etc.
[bawolff] we don't have anything like that
[jrobson] I enjoy code review too. Feel good factor about it, asked volunteer why they contribute to MF, its because [jon] reviews their code.
[bawolff] chances of someone sticking around reduces significantly if no review by 72 hours. Wikipedia is based on instant edits, etc.
[jrobson] repos of code that are unmaintained. do we have guidelines for what is mergable? We should have CI set up for pointing out trivial errors. A fear that as soon as you merge code it runs on the train, .. possible alternative is development branches.
[mah] need governance model, http://mwstake.org/mwstake/wiki/Blog_Post:37. quote. if you don't review people's code, it isn't being used and we're failing. Need a MW governance model.
[bawolff] don't think we need governance model for this, that's more for high level decisionmaking
[robla] we have architecture docs, kind of drafty, but we can improve those
[bawolff] ...
[ori] I want to challenge chris steipp on the security angle. You said that code review has found a low but non-zero number of defects. If we had 2 reviewers, we'd find even more security risks. If we upped it to three, we'd find even more risks. On the other side, there's tons of PHP code we have that has no maintainers, which is a huge security risk, and draw off contributors
[csteipp] It is the only practical way we have of identifying security issues before releasing to production. I think the current trade-off between risks and obstacles is reasonable.
[bawolff] fully standardize what we have to check for, to make reviews actually catch security issues.
[k4-713] can't get rid of CR because of PCI (Payment Card Industry) rules. if thast happened, we'd have to stop using MW, or get someon to do reviews for us.
[bawolff] I think we have to keep CR, but we need to modify how we do it, possibly radical, more structure will help.
[janZ] difference between "moving fast/breaking things" and letting other people clean up afterwards, vs taking a risk together with a reviewer

What we should do differently

[legoktm] Brian proposed a solution on the phab task: assigning a reviewer person to every proposed patch. I think that's the key. Someone has to be in charge of the review process. big problem for maintainer-less areas.
[bawolff] Even elsewhere, patches can fall through the cracks. Have to find a person responsible for it.
[maxsem] How are you proposing to enforce this?
[bawolff] Good question. The first step is to divide up MediaWiki core into sections, and allow interested people to sign up for a section. This comes with responsibilties, and you get removed if you don't meet them. Hope enough people sign up.
[maxsem] Result will be zero signups.
[ori] No effective-way of making non-blocking comments, that get author's attention but don't block the merge. Have to give a -1, otherwise people don't see the comments.

  • [dlynch] (etherpad) Differential helps with this.

[andre_] more structured review approach for reviewers. actively check who is using +1/-1, encourage them to receive +2 rights. Assign reviews that nobody selects. Need a review culture for WMF teams. Improve documentation for reviewers and contributors for what is acceptable.

  • MModell (etherpad): Phabricator will solve this.
    • g1

[mah] Follow up for what Max said, it sounds like a great idea but people won't do it. What is the reward for doing it for free?
[k4-713] Have we tried live review/office hours session type thing? People who wrote code/reviewed code on IRC together. [Audience: no]. Had a lot of success with live feedback.
[MatmaRex] What k4 said happens, but in an ad-hoc pattern.
[millimetric] We use cloud nine(?) get code in shape and send it to gerrit
[adamw] Merge patches if they're passable, leave comments and rely on authors goodwill to fix those issues. There is a big visibility problem with these post-merge comments, though. How does github-style feature branch development work? i.e. move forward with patch and squash later on.
[bawolff] Often times the problem is that no one looks at the patch/comments/reviews/says anything, etc.
[bd808] re: adam's q about github-style development. composer-merge-plugin is developed primarily on github, rationale was to reach normal PHP devs outside of MW community - and it worked, more contributions from outside. Key differences that are crappy about Github: no patch chains (dependent changes), cannot amend other people's patches - no collaboration
[ostriches] Phab gets it right, straddles the difference between GH/Gerrit - less strict than gerrit, but you can do deps and stuff.

Solutions

[ori] Proposal: Get rid of +1's. Bugzilla quip where MZMcBride says "+1 means reviewer has working mouse" Legitimate only for controversial patches, to indicate agreement. (https://tools.wmflabs.org/bash/quip/AU7VVae36snAnmqnK_xL )
[bawolff] patch with +1 means nothing, usually means less likely to be mergable
[marxarelli] Phab improves this. Gerrit is only binary. Responsiblity of merging on reviewer.
[bawolff] I don't want to limit this to current technology options.
[mooeypoo] +1 useful for when there's a single "expert" person, whom we want to give their opinion, but who don't necessarily understand the whole codebase. e.g. moriel is a RTL expert. Comments without +1 are often not noticed.
[bawolff] siebrand +1's i18n changes, acting as "expert".
[ostriches] re ori - that's configurable, if we agree on this, it can be done. I generally agree with ori, but see mooey's point. Another problem is that +2 implies merging, we backed ourselves into a corner here. Phab gets this right by encouraging actual comments, not numbers.

  • (etherpad) general etiquette: leave nitpicking comments vs. quickly patching them. Both work but as a reviewer which is expected? AW: IMO it's most polite to write a followon patch. The original author can apply the patch or not, and doesn't have to worry about merge conflicts.

[apergos] I use +1 and +2 like this: +1 is "I reviewed, haven't tested"; +2 is "Merging this, taking the responsibility". +1 makes sense in this way.
[anomie] Same.
[ostirches] Unlike -2 and +2, a -1 and +1 will disappeara on new non-trivial patchset.
[legoktm] If you leave a number without commenting, you should have your rights taken away ;) Also, we need to give reviewers more recognition - e.g. put them on Special:Version, like the list of authors. The reviewer's name is also hidden in commit notes, which tools like Phab don't show by default. Brian's Phab proposal would be a good starting step. We have an initial list of "owners" in Reviewer-bot config.

  • (AW in etherpad:) Reviewers can already add themselves as automatically watching the repos they can review.
  • (etherpad comment from Brad): If only the reviewer bot could work on inheritance and not just filenames, I could review more extensions too... [fancy!]

[bawolff] In BZ days, we had a monthly report email listing the most active bug closers. That motivated people (or at least, me). Nemo's crstats is also nice. http://koti.kapsi.fi/~federico/crstats/core.txt

  • (etherpad) number of commits vs. number of reviews

[twentyafterfour] +1 to ostriches ;) Biggest CR impediment for me is the automatic merging; I didn't know that +2 implies merge implies deploying (in some repos).
[bawolff] mediawiki-config repo is another conversation.
[Isarra] people have to +1 things because they don't have +2. (In particular volunteers.)
[bawolff] If somebody's only action on gerrit is +1'ing a patch, that's often not a good sign.
[ori] +1 means that someone likes the patch, but thought it was too risky to just merge.

  • (etherpad comment): for me (Tgr) it tends to mean "willing to merge but don't want to prevent others from reviewing". A "scheduled +2" option would be nice.
  • (etherpad) in repos with fewer maintainers (e.g. pywikibot) +1 means: ok for the author to self-merge

[janZ] I have used +1 with the comment "I have reviewed this, please merge" because I don't have permissions. A +1 with a comment that says what was reviewed is useful.
[mmodell] take ownership https://phabricator.wikimedia.org/owners/

Conclusion / recap

[robla] I am only putting questions in the summary (above), because otherwise could "stop" the conversation. Have to look for easy/hard questions later. If the answer is obvios, maybe we could write it down.
[twentyafterfour] Obvious to me: Move to Phabricator. Phab has Owners tool. Call to action: Set up an owners package for stuff you care about.

  • (etherpad comment): Phabricator still does not know about half the repos on gerrit
  • (etherpad): The "moving to differential" session was yesterday, and talked about our path to getting everything set up in Phabricator. (Yes, these sessions should probably have been reversed in order.)

[bawolff] We could've done that in SVN day and yet we didn't...
[robla] Not so obvious :)
[legoktm] Code review guidelines ... ? (missed it)
[everyone discusses 'obvious' answers, see above]
[legoktm] the Owners thing, the problem is social, not technical. List of maintainers at mw.org need updating.
[greg] We need actual guideline/policy for this.
[DanA] Look at Linux, it's reviewed on a mailing list and it works? We have smaller scale too. Maybe we have to look elsewhere.
[bawolff] I don't know much about Linux code, but I think they have components.

  • (etherpad comment): Re "one jerk reviews all the code", we had that (although I don't think Brion or Tim are jerks). It didn't scale.

[bawolff] Indifference is even more sapping that bad review / people being assholes, it meant nobody even cares.
[ariel] Big prestige of being a contributor to Linux kernel, people more likely to suffer through it.
[David Lynch] Owners list is both social and technical. On Phab you get directly added as reviewers, good reason to keep up to date.
[ostriches] CR through email didn't scale for us, a looong time ago. It comes down to lack of ownership, not a technical issue. Need to remove inactive owners, etc. (cf. https://www.mediawiki.org/wiki/Gerrit/Project_ownership#Requesting_repository_ownership ?) Need a way to cease maintainership too.
[Cindy] My own patch is languishing. Need owners(?), but we also need people responsible for new code. Architectural issue, do you add stuff to an existing one or a new class?

  • Reiteration of "How does a contributor find the person responsible for the code? Especially for code where no one is "responsible"?

[bawolff] what is an acceptable way to do things? Standardize CR process, not have it dependent upon which reviewer you get. Need a catch-all owner.

  • CODE Review WRANGLER

[greg-g] freaking out about no action items
[robla] most important thing is to improve the CR process, not make sure the meeting was useful. Need to prioritize list of questions.
[ori] CR has to do with rules about how we interact with software development, hard for people who are not invested with special power to change anythin - kind of meta. We should agree that CR is a severe issue that we should appoint some small committe to make binding recommendations on how to change it.
[bawolff] just to double-check, who thinks that there is a problem with CR?
<room raises hands in agreement>
[DanA] does addressing this issue have sufficient ROI?
[kaldari] Talk with teams that do a very good job at CR, and have volunteers who actually contribute
[Catrope] It comes down to ownership. VE didn't have CR backlog because there is clear responsibility.
[bawolff] Owners don't have to be WMF teams. They can be anyone.
[Catrope] Maintainership is better term than ownership
[robla] we can look into other orgs taking on ownership, i.e. use other people's code rather than reinventing the wheel and writing our own.
[bawolff] Yay I will fix everything.
<thunderous applause>

[bawolff] specific code review guidelines would be interesting to explore tomorrow at unconference!


Video at https://commons.wikimedia.org/wiki/File:DevSummit2016-Code-Review.webm

Related Objects

There are a very large number of changes, so older changes are hidden. Show Older Changes
Krenair added a comment.EditedDec 13 2015, 7:33 PM

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.

-2 from me as well. This benefits only the existing people with +2, and does nothing to help those without it. Do you really see the way that operations/puppet works as a positive thing? In fact, you might be able to argue that it'd make people who are unable to self-merge (as in operations/puppet) less able to get reviews in some repositories.
Really your proposal does nothing to improve code review, it just makes some people able to bypass it.

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!

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-Relations context).

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.

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.

Tgr added a comment.Dec 14 2015, 6:43 AM

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.

Tgr added a comment.Dec 15 2015, 9:40 PM

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.

Tgr added a comment.Dec 17 2015, 11:14 PM

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?

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

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?

ori added a comment.Dec 26 2015, 5:45 PM

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.
TheDJ added a subscriber: TheDJ.Dec 26 2015, 6:17 PM

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

saper added a comment.EditedDec 26 2015, 8:05 PM

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

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

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")

TheDJ added a comment.Dec 28 2015, 8:30 AM

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.

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.

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.

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.

awight added a subscriber: awight.Dec 28 2015, 11:31 PM

@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:

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.

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

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.

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.

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?

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)

Aklapper updated the task description. (Show Details)Jan 12 2016, 7:23 PM
Aklapper added a subscriber: greg.

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.

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.

greg added a comment.Jan 15 2016, 5:23 PM

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

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!

Aklapper closed this task as Resolved.Feb 29 2016, 12:58 PM

I'm closing this task as it has served its purpose at the Developer Summit.
Any conclusions and followup will happen in T101686 (and then T78768).

RobLa-WMF reopened this task as Stalled.Feb 29 2016, 5:55 PM

Could we make sure that we link this task to the video of the event before closing it? That would mean this is blocked by T124127

RobLa-WMF added a subtask: Restricted Task.Feb 29 2016, 5:55 PM

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

Nemo_bis added a comment.EditedApr 17 2016, 6:59 AM

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.

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

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.

ori added a comment.Jun 11 2016, 2:18 AM

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.

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

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?

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

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.

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"?

ksmith added a subscriber: ksmith.Jul 6 2016, 3:45 PM
Aklapper removed a subtask: Restricted Task.Jul 12 2016, 2:16 PM
Aklapper closed this task as Resolved.Jul 25 2016, 2:41 PM

Could we make sure that we link this task to the video of the event before closing it?

Video at https://commons.wikimedia.org/wiki/File:DevSummit2016-Code-Review.webm

Closing this task about running a session at a conference, as that conference took place half a year ago.

Aklapper updated the task description. (Show Details)Jul 25 2016, 2:42 PM
Nemo_bis renamed this task from Make code review not suck to Event on "Make code review not suck".Jul 25 2016, 5:09 PM