Live data from Hacker News

Require multiple reviewers for pull requests

blog.github.com

31–37 of 37 posts

Re: Require multiple reviewers for pull requests

#31
post #9

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.

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…

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.

Re: Require multiple reviewers for pull requests

#32
post #9

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.

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.

Re: Require multiple reviewers for pull requests

#33

Earlier quoted context omitted.

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.

Re: Require multiple reviewers for pull requests

#35
post #9

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.

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

Re: Require multiple reviewers for pull requests

#36

Earlier quoted context omitted.

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

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.

Re: Require multiple reviewers for pull requests

#37

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.

Never work at places that do this shit.

The bureaucracy is growing for its own sake.

It should be about product, not fucking policy.

Post reply on HN