Require multiple reviewers for pull requests | Hacker News Reader

Require multiple reviewers for pull requests

blog.github.com

80 pointsclarkbw34 comments

Having multiple reviewers is often bad. A diffusion of responsibility means no responsibility.

If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.

No, that's just that no one knows who's turn it is.

Having two people check something over is a good thing, telling two people that they should both start simultaneously to do work that might be duplicate is bad.

There's an element of that but imagine one of the other reviewers found e.g. a massive security flaw and you said the code was fine?

I agree. I'd like the ability for reviewers to give different levels of approval. A reviewer who is in a rush can give a "looks okay, but I spent less than a minute looking at it" approval. A reviewer who takes their time can give a "I reviewed every line you changed and completely understand everything" approval. Before the change gets merged, X people need to give a quick approval and Y people need to give a line-by-line approval.

In my experience, bugs pop up more often in changes where reviewers aren't honest in how well they actually understand the changes, and are maybe afraid of admitting their confusion (even though it's usually the case that if one person in the room is confused, everyone is).

True. But having just one reviewer can be bad too: when two people review each other often, they often "collude" and start to rubber-stamp shitty code. Or the other extreme - they can go into an argument over some issue with no way to break the stalemate.

Two reviewers seems optimal if you can afford it. Possibly with one person doing the bulk of the reviewing and the other more in the role of providing oversight and breaking ties if they occur.

If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.

Welcome to open source maintainership

If you have new folks who aren't fully socialized to the project yet, having multiple reviewers is much worse than having a couple of people specifically individually coaching new would-be committers.

I use a coaching approach for new hires at my startups.

Could you describe your coaching approach? That sounds interesting, and is often a difficult problem to solve

Well, the basic idea for coaching a brand-new employee is that you hired a perfectly smart person for the job, but your company has a strong coding culture with a particular style that the new engineer isn't familiar with. So it's a big part of on-boarding to not only teach this smart engineer the crazy weirdnesses of your system, but also teach them (what they will perceive as) the oddities of your corporate coding style.

It can be good to do this outside of the public eye, so the initial cycle of "wtf, why is this considered standard" followed by "well, it's not so much that this is the best way, but it's more likely that we'll be able to understand each other's code if we all do it one way" can be done in private.

Once the engineer is well-versed in the system, they are unleashed to quietly subvert it, if that is their will :-)

Maintaining a popular open source project is a great way to learn to politely say “fuck you this is crap”.

It’s important to be consistent with your online voice though. If you’re always blunt and terse then, generally, people won’t take it badly. But if you slide between extraneous gestures and out right profanity then eventually someone will take it personally.

Here are some of the reasons people will submit shitty code: they (even subconsciosly) think it will be somebody else's problem; they are junior; they don't fully understand the system they are changing; they don't feel polishing the code is valued. Many of these problems can be improved with a good review system.

I don't want to enter the people vs. systems debate but certainly a good system can help.

There are also 2 cases of tunnel vision that I started noticing in reviews.

1. You've been working on some non-trivial change for long enough that you find some element of it trivial and not worth documenting, but others have no idea why it's there.

2. You've been looking at something so long that a "temporary code, I'll fix it later" starts looking natural and it takes someone else to call it out.

> you've got bigger problems than needing a better review system

I'll set aside the fallacious premise that everybody can stack their team with 10x engineers. Some of us actually work with interns and juniors.

That being said, your comment doesn't invalidate the need for a better review system. Software quality isn't defined by any one tool or process; the product is the end result of a chain of tools and processes...

I'm all for good review systems. But any review system that labels a large fraction of the submitted code as very deficient indicates that the solution is to act before review.

And I've coded with (and hired) interns and juniors, so it's not just a selection effect. In fact, the worst code I've seen came from a senior engineer, with strong opinions, who wrote the world's most blecherous code. We fired him, but not soon enough.

I think 1 reviewer works better in general, but multiple reviewers could be made to work if the reviews and the reviewers were separate and unaware of the other?

Maybe don’t let the reviewers know there are other reviewers? Like a BCC feature.

Yeah, over the years no one would possibly notice. Especially smart engineers.

It’s a psychological thing, like hiding the number of upvotes a comment has received on HN. Everyone knows they’re being hidden, but it helps prevent bandwagoning anyway.

You get a diffusion of responsibility when there are more people who could do a task than are required (i.e., when anyone can decide not to do it).

This change allows you to prevent that from happening, by requiring as many reviewers to approve a request as your team has—so that everyone has to approve it. Previously, GitHub only let you set one required reviewer, which enabled diffusion of responsibility.

I believe scarming is making a different point. While you're saying one reviewer from a pool means no one ever actually reviews the code, he's saying that with 2 (or 5) reviewers instead of 1, you're more likely to have the reviewers examine the code in less detail.

The responsibility (or blame?) for bugs is diffused over more people, so people will care less.

I'm not sure that I agree, but I can at least understand the argument.

It all depends on a particular teams workflow. I think it's great to see GitHub continuing to invest in workflow features like this.

I developed an app (https://pullreminders.com) that helps teams stay on top of open pull requests and in talking to customers, there is a huge demand for features and tools to support a better code review process.

(Disclaimer: I am the creator of Pull Reminders)

I’ve lived in a lot of collective houses. I’ve seen a lot of chore systems come and go.

What I’ve decided is that it’s folly to try to enforce accountability. A good system provides visibility, that’s it. Who did what? Document it, but allow implicit social process to handle enforcement. Hard rules just lead to people cooking the books, or opting out of the system entirely.

I feel the same about code review. Document who reviewed. But it’s up to individuals to take responsibility for the outcome. And at the end of the day it’s the committer’s code. It’s their name in the git history.

Collective houses generally have no management other than visibility.

Companies have managers. It's very important that mangers at a company set the tone for using mistakes to create better engineering processes. And that involves a lot more than shaming the committer.

If every dev was equally responsible or thorough, we would be fine with just 1 reviewer. But the reality is that devs are different, and can have good/bad days. Operationally, in every mission-critical workflow, usually at least 2 people need to turn the key, fly the airplane, press the button, sign off, or what have you. There is good historic precedent for distributing responsibility and reducing errors.

> If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth.

in my experience, this is already happening. When you get pinged to review PR after PR and have your own work to do, it's easy to skim. Requiring 2 reviewers operationally has often meant i'd get a chance to look and take a deep dive after someone caught the glaring issues.

When it's ok for just one dev to sign-off, time and time again that's how i've seen bugs/debt sneak in. I think giving organizations this option is only a plus, and is not really something we need to have a public debate about; it's an organizational decision to use or not use this that github has facilitated.

> If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.

There is no way I am looking at every single line, and even if I did it doesn't mean I understand the code.

I think the next step from here is giving the ability to assign maintainers for certain sets of files (in this directory or matching a particular regex) and then when a pull request comes in, GH can looked at the changes, match the maintainers, and require all of them to sign off prior to allowing merging.

There is already a method in place for this with a CODEOWNERS file. It will automatically add reviewers for different paths in the repository.

Have you seen their CODEOWNERS feature? https://help.github.com/articles/about-codeowners/ there is a check box to enforce reviews by owners.

Chrome/Google have something like that but more flexible (e.g. you can have nested OWNERS files) called OWNERS https://chromium.googlesource.com/chromium/src/+/master/docs...

There's a more general/kinda crazy system in Gerrit (by Google) that let's you write custom rules in Prolog (which helps for solving for a minimal set of reviewers that could approve a given PR.) I'm not sure if it gets much use but its a neat thought for sure: https://gerrit-review.googlesource.com/Documentation/prolog-... (which links to this email explaining why: https://groups.google.com/d/msg/repo-discuss/wJxTGhlHZMM/Tal... )

CODEOWNERS is a good start, but it only works for the main branch, which means that it doesn't really work well with the Git Flow (https://leanpub.com/git-flow/read). Keeping a different CODEOWNERS file in each branch would be suboptimal, because that means cross-branch merges become a touch nasty. Perhaps if CODEOWNERS was extended to allow matching against rules that also include the branch to be merged into? Alternatively, if that might break existing CODEOWNERS files, doing the same in a MAINTAINERS file might be a viable solution instead.

I think the constraint-solver method in Gerrit is pretty neat, although I could see integrating that into GitHub and devising a non-painful UI for that as a major challenge. However, if they managed to do it, that would be an insanely powerful feature to have, especially for larger or more complex projects.

FWIW, Reviewable allows you to write a custom review completion condition in JS where you can match paths and participants to your heart's content and determine whether the review is complete or not. This is posted to GitHub as a status check so you can use it to block/allow merging. (Disclaimer: I built Reviewable.)

TLDR: github has a new feature that allows you to require X number of people to approve a PR before merging to a protected branch.

From the title, I initially thought this would be an article about why you should have multiple people reviewing PRs, which sounded ridiculous (obviously we don't all have the time/resources for that).

Where X is between one and six.

I think perhaps they should have made it infinite, because having a set range causes anchoring. It might seem like 3 is a good choice because 2 is at the low end of things.

Blockchain for source control?

Reply on news.ycombinator.com