Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Require multiple reviewers for pull requests

blog.github.com

Post reply on HN
Print view View original

11–20 of 37 posts

Re: Require multiple reviewers for pull requests

#11

Post by codemac »

scarmig wrote: 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.

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.

Re: Require multiple reviewers for pull requests

#12

Post by seanwilson »

scarmig wrote: 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.

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?

Re: Require multiple reviewers for pull requests

#13

Post by testplzignore »

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

Re: Require multiple reviewers for pull requests

#14

Post by dunkelheit »

scarmig wrote: 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.

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.

Re: Require multiple reviewers for pull requests

#15

Post by booleandilemma »

scarmig wrote: 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.

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

Re: Require multiple reviewers for pull requests

#16

Post by greglindahl »

dunkelheit wrote: post #14
scarmig wrote: 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.

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…

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

Re: Require multiple reviewers for pull requests

#17

Post by geofft »

scarmig wrote: 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.

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.

Re: Require multiple reviewers for pull requests

#18

Post by bennofs »

greglindahl wrote: post #16
dunkelheit wrote: post #14

Earlier quoted context omitted.

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…

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

Re: Require multiple reviewers for pull requests

#19

Post by greglindahl »

bennofs wrote: post #18
greglindahl wrote: post #16

Earlier quoted context omitted.

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.

Re: Require multiple reviewers for pull requests

#20

Post by joshuamorton »

geofft wrote: post #17
scarmig wrote: 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.

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.

Post reply on HN