
Require multiple reviewers for pull requests - clarkbw
https://blog.github.com/2018-03-23-require-multiple-reviewers/
======
scarmig
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.

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

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

~~~
bennofs
Welcome to open source maintainership

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

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

~~~
greglindahl
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 :-)

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

~~~
jacobparker
Have you seen their CODEOWNERS feature?
[https://help.github.com/articles/about-
codeowners/](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...](https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#owners-
files)

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-...](https://gerrit-
review.googlesource.com/Documentation/prolog-cookbook.html) (which links to
this email explaining why: [https://groups.google.com/d/msg/repo-
discuss/wJxTGhlHZMM/Tal...](https://groups.google.com/d/msg/repo-
discuss/wJxTGhlHZMM/TalGqOeo0GYJ) )

~~~
haneefmubarak
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](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.

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

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

------
tomwalker
Blockchain for source control?

