6 ms·

Require multiple reviewers for pull requests

80 points by clarkbw 9y ago

haneefmubarak 9y ago
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.
- cakeface 9y ago
  There is already a method in place for this with a CODEOWNERS file. It will automatically add reviewers for different paths in the repository.
- jacobparker 9y ago
  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/code_reviews.md#owners-files 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-cookbook.html https://gerrit-review.googlesource.com/Documentation/prolog-... (which links to this email explaining why: https://groups.google.com/d/msg/repo-discuss/wJxTGhlHZMM/TalGqOeo0GYJ https://groups.google.com/d/msg/repo-discuss/wJxTGhlHZMM/Tal... )
  - haneefmubarak 9y ago
    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.
- piotrkaminski 9y ago
  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.)
- OnlyRepliesToBS 9y ago
  Never work at places that do this shit. The bureaucracy is growing for its own sake. It should be about product, not fucking policy.
chadash 9y ago
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 9y ago
  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.
scarmig 9y ago
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 9y ago
  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 9y ago
    If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.
    - bennofs 9y ago
      Welcome to open source maintainership
      - greglindahl 9y ago
        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 9y ago
        Could you describe your coaching approach? That sounds interesting, and is often a difficult problem to solve
tomwalker 9y ago
Blockchain for source control?
deleted 9y ago
[deleted]

80 points by clarkbw 9y ago

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.

cakeface 9y ago
There is already a method in place for this with a CODEOWNERS file. It will automatically add reviewers for different paths in the repository.
jacobparker 9y ago
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/code_reviews.md#owners-files 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-cookbook.html https://gerrit-review.googlesource.com/Documentation/prolog-... (which links to this email explaining why: https://groups.google.com/d/msg/repo-discuss/wJxTGhlHZMM/TalGqOeo0GYJ https://groups.google.com/d/msg/repo-discuss/wJxTGhlHZMM/Tal... )
- haneefmubarak 9y ago
  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.
piotrkaminski 9y ago
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.)
OnlyRepliesToBS 9y ago
Never work at places that do this shit. The bureaucracy is growing for its own sake. It should be about product, not fucking policy.

chadash 9y ago

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 9y ago
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.

scarmig 9y ago

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 9y ago
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 9y ago
  If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.
  - bennofs 9y ago
    Welcome to open source maintainership
    - greglindahl 9y ago
      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 9y ago
        Could you describe your coaching approach? That sounds interesting, and is often a difficult problem to solve

tomwalker 9y ago

Blockchain for source control?

deleted 9y ago

[deleted]