Reviewer-merged PRs

Pull request

Table of contents

Problem

We’ve been having authors merged PRs, but that’s not going to work when we get contributors who don’t have merge access. We need a solution.

Background

It’s been mentioned that LLVM favors having authors merge due to the risks of breaking build bots. The LLVM community’s leaning is to favor author merges so that the author can decide whether to try rolling back or fixing forward.

At present Carbon has essentially been using a shared repository model where authors merge their own PRs, but that’s difficult to extend to a fully public setup. The fork and pull model is the other main git collaboration model, and is popular with open source projects.

Proposal

Encourage reviewers to merge when they feel okay doing so. Let reviewers make that choice. Let authors say they’ll merge themselves.

This is a fork and pull model which encourages reviewer merges.

Details

See changes to code review.

Rationale

  • Community and culture
    • Defines a process for accepting contributions from developers who don’t have merge access.

Alternatives considered

Never merge PRs from developers with merge access

We could tell reviewers to never merge PRs from developers with merge access. This is also a fork and pull model, but minimizing reviewer merges.

Advantages:

  • While this proposal suggest authors could opt out from having the reviewer merge, minimizing reviewer merges avoids a gray area when an author forgets or the reviewer misses it (even if enforced, the author might do it wrong).

Disadvantages:

  • Relies on the reviewer figuring out whether the author has merge access, which they may forget to do. The most likely consequence is that outside contributors will need to ping to get PRs merged.
  • Makes reviewer merges less common. This in turn can make them less consistently done well due to unfamiliarity.
    • Contributors without merge access would be following a different process, and as a consequence it increases the chance that a new contributor would be first to discover a problem, which in turn can discourage contributions.

In the future, build bots may cause more issues, and we may lean more in this direction. It could also be that we end up here through standard practice of coordinating with the reviewer is concerned the author may need to fix a break post-commit. However, it doesn’t seem necessary to make it a hard rule at present.

Grant all potential contributors merge access

We could grant the public merge access; in other words, continue with a shared repository model, but fully public instead of private. This way, reviewers would not need to consider access.

Advantages:

  • Authors can always merge, removing the burden on reviewers.

Disadvantages:

  • Relies on approvals heavily for code security, which constrains decisions regarding whether to keep CODEOWNERS.
  • This kind of setup is likely atypical for GitHub projects, and so may be more surprising than alternatives.
  • Harder for reviewers to discern between frequent contributors and new contributors.
  • May create issues when a novice contributor breaks something.
    • There must not be an expectation that novice contributors should understand processes, unless demonstrating an understanding becomes a prerequisite for review.

Allow reviewers to clean up pull request descriptions

We could allow reviewers to clean up pull request descriptions, rather than asking the author to.

Advantages:

  • Decreases the number of review round-trips.

Disadvantages:

  • May lead to PR descriptions not being in the authors voice, frustrating authors.

The preference is to let authors control pull request descriptions.

Only have authors resolve merge conflicts

We could only have authors resolve merge conflicts, instead of having reviewers do it.

Advantages:

  • Lowers the chance of an incorrect merge, because authors are likely to better understand the conflict.
  • Lets ambiguous resolves be handled with the author’s voice.
    • If a bad resolve introduces a bug, it’s the author’s fault, rather than being the reviewer’s fault but attributed to the author.

Disadvantages:

  • Increases the number of review round-trips.
    • Pull requests by authors lacking merge access would have an extra round-trip, because the author would resolve conflicts then the reviewer would merge the pull request. In a worst case this may bounce back and forth due to new conflicts being added before the reviewer merged.

The preference is to minimize review round-trips. However, this could still be a real-world outcome if reviewers generally only merge when there are no outstanding merge conflicts.

Implement a two-person rule for source code changes

We could implement a two-person rule for source code changes, where both an author and reviewer must see the merged code. GitHub supports this with “Dismiss stale pull request approvals when new commits are pushed”, although we may also need to require 2 approvers per pull request.

Advantages:

  • Gives stronger code security, eliminating situations where either the author or reviewer could merge unreviewed changes.

Disadvantages:

  • Increases the number of review round-trips.
    • Right now, reviewers can approve with minor comments (“fix typo”). Fixing those would require a new commit, which in turn would require fresh approval.
    • Truly preventing this issue may require setting 2 approvers per pull request, so that a reviewer couldn’t push a commit to the pull request then approve and merge. Requiring 2 approvers also increases review overhead.

The preference is to minimize review round-trips.