carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-09-24 19:40:10 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Reviewer-merged PRs (#1190)

Browse Source 
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 <chandlerc@gmail.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This commit is contained in:
Jon Meow
2022-05-04 13:12:18 -07:00
committed by GitHub
co-authored by Chandler Carruth Richard Smith
parent b87d04bf9b
commit e18675608b
2 changed files with 227 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/project/code_review.md
+32
View File
@@ -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
proposals/p1190.md
+195
View File
@@ -0,0 +1,195 @@
# Reviewer-merged PRs
<!--
Part of the Carbon Language project, under the Apache License v2.0 with LLVM
Exceptions. See /LICENSE for license information.
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-->
[Pull request](https://github.com/carbon-language/carbon-lang/pull/1190)
<!-- toc -->
## 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)
<!-- tocstop -->
## 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.