From e18675608bcc7dcd164806d50fcf866044f15f95 Mon Sep 17 00:00:00 2001 From: Jon Meow Date: Wed, 4 May 2022 13:12:18 -0700 Subject: [PATCH] Reviewer-merged PRs (#1190) Encourage reviewers to merge when they feel okay doing so. Let reviewers make that choice. Let authors say they'll merge themselves. Co-authored-by: Chandler Carruth Co-authored-by: Richard Smith --- docs/project/code_review.md | 32 ++++++ proposals/p1190.md | 195 ++++++++++++++++++++++++++++++++++++ 2 files changed, 227 insertions(+) create mode 100644 proposals/p1190.md diff --git a/docs/project/code_review.md b/docs/project/code_review.md index d238b64ff71c..e85413bc55af 100644 --- a/docs/project/code_review.md +++ b/docs/project/code_review.md @@ -27,6 +27,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - [What should be covered by a review?](#what-should-be-covered-by-a-review) - [Writing review comments](#writing-review-comments) - [Approving the change](#approving-the-change) +- [Merging pull requests](#merging-pull-requests) + - [Merge commit descriptions](#merge-commit-descriptions) - [Resolving an impasse or conflict](#resolving-an-impasse-or-conflict) - [Escalation](#escalation) @@ -410,6 +412,36 @@ author can always come back to you if they have questions, and we can always revert changes if the resolution for some reason diverges wildly from your expectations. +## Merging pull requests + +Pull requests are ready to be merged when reviewers have indicated they're happy +(for example, "LGTM" or "Looks good to me") or have approved the pull request. +While all merges require at least one approval, a reviewer might approve before +others are finished reviewing; all reviewers should be given time to comment to +ensure there's a consensus. + +Either the author or reviewer may merge and resolve conflicts. The author may +indicate they want to merge by informing the reviewer and adding the +`DO NOT MERGE` label. The reviewer is encouraged to coordinate with the author +about merge timing if there are concerns about breaks. In either case, the +developer doing the merge is expected to be available to help address +post-commit issues, whether through a fix-forward or a rollback. + +### Merge commit descriptions + +When squashing and merging, GitHub tries to generate a description, but it's +recommended to use the first comment on the pull request review for the squashed +commit description. Authors should keep it up-to-date so that reviewers can +merge when the change is ready. Reviewers shouldn't edit or rewrite this message +themselves, and instead ask the author make those changes (possibly with +suggestions) just like other parts of the code review. It's important that the +commit message is one the author is comfortable with when merged. + +When suggested edits have been merged into a pull request, GitHub will append a +`Co-authored-by:` line to its default proposed commit message for each reviewer +who suggested edits that were applied. These lines should be retained and +appended to the message from the initial comment. + ## Resolving an impasse or conflict At some point, a review may reach an impasse or a genuine conflict. While our diff --git a/proposals/p1190.md b/proposals/p1190.md new file mode 100644 index 000000000000..46ffd1fa73a2 --- /dev/null +++ b/proposals/p1190.md @@ -0,0 +1,195 @@ +# Reviewer-merged PRs + + + +[Pull request](https://github.com/carbon-language/carbon-lang/pull/1190) + + + +## Table of contents + +- [Problem](#problem) +- [Background](#background) +- [Proposal](#proposal) +- [Details](#details) +- [Rationale](#rationale) +- [Alternatives considered](#alternatives-considered) + - [Never merge PRs from developers with merge access](#never-merge-prs-from-developers-with-merge-access) + - [Grant all potential contributors merge access](#grant-all-potential-contributors-merge-access) + - [Allow reviewers to clean up pull request descriptions](#allow-reviewers-to-clean-up-pull-request-descriptions) + - [Only have authors resolve merge conflicts](#only-have-authors-resolve-merge-conflicts) + - [Implement a two-person rule for source code changes](#implement-a-two-person-rule-for-source-code-changes) + + + +## 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](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/about-collaborative-development-models#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](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/about-collaborative-development-models#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](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/about-collaborative-development-models#fork-and-pull-model) +which encourages reviewer merges. + +## Details + +See changes to [code review](/docs/project/code_review.md). + +## Rationale + +- [Community and culture](/docs/project/goals.md#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](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/about-collaborative-development-models#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](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/about-collaborative-development-models#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](https://github.com/carbon-language/carbon-lang/issues/413). +- 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. + - GitHub has an option, + ["Dismiss stale pull request approvals when new commits"](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule): + we would likely want to enable it to reduce the weight on reviewers, + although it means reviewers would need to approve again for any change + (likely including merge commits). +- 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](https://en.wikipedia.org/wiki/Two-man_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"](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule), +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.