Remove CODEOWNERS (#1367)

Remove CODEOWNERS and rely on repository commit access

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Geoff Romer <gromer@google.com>

.github/assign_prs/explorer.yaml

@@ -0,0 +1,20 @@
+# 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
+
+# Set to true to add reviewers to pull requests
+addReviewers: true
+
+# Set to true to add assignees to pull requests
+addAssignees: false
+
+# A list of reviewers to be added to pull requests (GitHub user name)
+reviewers:
+  - geoffromer
+  - jonmeow
+  - jsiek
+  - zygoloid
+
+# A number of reviewers added to the pull request
+# Set 0 to add all the reviewers (default: 0)
+numberOfReviewers: 1

.github/assign_prs/fallback.yaml

@@ -0,0 +1,20 @@
+# 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
+
+# Set to true to add reviewers to pull requests
+addReviewers: true
+
+# Set to true to add assignees to pull requests
+addAssignees: false
+
+# A list of reviewers to be added to pull requests (GitHub user name)
+reviewers:
+  - chandlerc
+  - jonmeow
+  - josh11b
+  - zygoloid
+
+# A number of reviewers added to the pull request
+# Set 0 to add all the reviewers (default: 0)
+numberOfReviewers: 1

.github/assign_prs/leads.yaml

@@ -0,0 +1,19 @@
+# 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
+
+# Set to true to add reviewers to pull requests
+addReviewers: true
+
+# Set to true to add assignees to pull requests
+addAssignees: false
+
+# A list of reviewers to be added to pull requests (GitHub user name)
+reviewers:
+  - KateGregory
+  - chandlerc
+  - zygoloid
+
+# A number of reviewers added to the pull request
+# Set 0 to add all the reviewers (default: 0)
+numberOfReviewers: 1

.github/assign_prs/toolchain.yaml

@@ -0,0 +1,19 @@
+# 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
+
+# Set to true to add reviewers to pull requests
+addReviewers: true
+
+# Set to true to add assignees to pull requests
+addAssignees: false
+
+# A list of reviewers to be added to pull requests (GitHub user name)
+reviewers:
+  - chandlerc
+  - jonmeow
+  - zygoloid
+
+# A number of reviewers added to the pull request
+# Set 0 to add all the reviewers (default: 0)
+numberOfReviewers: 1

.github/workflows/assign_prs.yaml

@@ -0,0 +1,56 @@
+# 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
+
+name: 'Auto Assign'
+on:
+  pull_request:
+    types: [opened, ready_for_review]
+
+jobs:
+  assign_reviewer:
+    runs-on: ubuntu-latest
+    steps:
+      - id: path-filter
+        uses: dorny/paths-filter@v2
+        with:
+          filters: |
+            leads:
+              - '*.md'
+              - 'LICENSE'
+              - 'docs/project/principles/*'
+              - 'docs/project/evolution.md'
+              - 'docs/project/goals.md'
+              - 'docs/project/roadmap.md'
+              - 'proposals/*.md'
+            explorer:
+              - 'explorer/**'
+            toolchain:
+              - 'toolchain/**'
+
+      - id: assign-leads
+        if: steps.changes.outputs.leads == 'true'
+        uses: shufo/auto-assign-reviewer-by-files@v1.1.3
+        with:
+          configuration-path: '.github/assign_prs/leads.yaml'
+
+      - id: assign-explorer
+        if: steps.changes.outputs.explorer == 'true'
+        uses: shufo/auto-assign-reviewer-by-files@v1.1.3
+        with:
+          configuration-path: '.github/assign_prs/explorer.yaml'
+
+      - id: assign-toolchain
+        if: steps.changes.outputs.toolchain == 'true'
+        uses: shufo/auto-assign-reviewer-by-files@v1.1.3
+        with:
+          configuration-path: '.github/assign_prs/toolchain.yaml'
+
+      - id: assign-fallback
+        if:
+          steps.changes.outputs.leads != 'true' &&
+          steps.changes.outputs.explorer != 'true' &&
+          steps.changes.outputs.toolchain != 'true'
+        uses: shufo/auto-assign-reviewer-by-files@v1.1.3
+        with:
+          configuration-path: '.github/assign_prs/fallback.yaml'

docs/project/code_review.md

@@ -72,22 +72,31 @@ partial review can be a good way to start contributing to Carbon. Contributors
 with specific domain expertise or familiarity should also try to provide review
 on changes touching relevant parts of the project.
 
-Additionally, at least one _code owner_ of any file changed needs to review that
-change. The code owners and what files they are responsible for are defined
-using the
-[`CODEOWNERS`](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-syntax)
-file in the root of the repository. Pull requests will automatically request
-reviewers based on this file and enforce that these reviews take place.
-
-While we do encourage people interested in contributing to Carbon by reviewing
-changes to do so, we also suggest not overloading a single review. It can be
-daunting for the author of a change to get feedback from a large number of
-reviewers, and so we suggest keeping the number of reviewers reasonably small.
+Additionally, at least one developer with commit access must review each change.
+In Carbon, developers will focus on particular areas, loosely broken down as:
+
+-   [Carbon leads](groups.md#carbon-leads): [proposals](/proposals/) and other
+    important project documents, including the [Main README](/README.md),
+    [Code of Conduct](/CODE_OF_CONDUCT.md), [license](/LICENSE), and
+    [goals](goals.md).
+
+-   [Implementation team](groups.md#implementation-team): general changes.
+
+    -   We split out auto-assignment by
+        [explorer](/.github/assign_prs/explorer.yaml),
+        [toolchain](/.github/assign_prs/toolchain.yaml), and
+        [other files, including documentation](/.github/assign_prs/fallback.yaml).
+
+[Auto-assignment](/.github/workflows/assign_prs.yaml) will help find owners, but
+won't always be perfect -- developers may take a PR they weren't auto-assigned
+in order to help review go quickly. Contributors can also request multiple
+reviewers, but it can be daunting to get feedback from a large number of
+reviewers, so we suggest keeping the number of reviewers reasonably small.
 
 Any reviews that explicitly request changes should be addressed, either with the
 changes or an explanation of why not, before a pull request is merged. Further,
-any code owners who have requested changes should explicitly confirm they're
-happy with the resolution before the change is merged.
+any owners who have requested changes should explicitly confirm they're happy
+with the resolution before the change is merged.
 
 When a team gives an affirm decision on an [evolution proposal](evolution.md),
 each team member should explicitly note any of their comments on the pull
@@ -116,13 +125,13 @@ mechanical best practices to most effectively navigate them.
     -   Make any in-file comments needed, but add them to a pending review
         rather than sending them directly.
     -   Finish the review and add any top-level review comments there.
--   If you are a code owner who will be providing approval for the change, then
-    make sure to mark a review as requesting changes when you want the author to
+-   If you are an owner who will be providing approval for the change, then make
+    sure to mark a review as requesting changes when you want the author to
     begin addressing your comment. Only use the "comment" review state if you
     are still in the process of reviewing and don't expect the author to begin
     working on further changes.
-    -   If you are not a code owner asked to approve, use the difference between
-        a comment and requesting a change to help the author know whether to
+    -   If you are not an owner asked to approve, use the difference between a
+        comment and requesting a change to help the author know whether to
         circle back with you before landing the pull request if the relevant
         owner(s) approve it.
 -   Don't reply to in-file comment threads in the conversation view, or with
@@ -400,9 +409,9 @@ in GitHub, but also state this explicitly in the message since this is the
 default and doesn't indicate that your feedback _is_ addressed. For example, say
 that "my comments are addressed, but leaving the final review to others" to
 clearly indicate that you're happy but are deferring the decision to others. If
-you are a code owner and deferring to someone else, it is essential to suggest
-specific other reviewers. Otherwise, we risk all the code owners assuming
-another is going to approve the change.
+you are an owner and deferring to someone else, it is essential to suggest
+specific other reviewers. Otherwise, we risk all the owners assuming another is
+going to approve the change.
 
 An important technique to make progress, especially with different working hours
 and timezones, is to approve changes even with outstanding comments. For
@@ -454,7 +463,7 @@ There are two techniques to use to resolve these situations that should be tried
 early on:
 
 1. Bring another person into the review to help address the specific issue.
-   Typically they should at least be a code owner, and may usefully be a
+   Typically they should at least be an owner, and may usefully be a
    [Carbon lead](groups.md#carbon-leads).
 
 2. Ask the specific question in a broader forum, such as Discord, in order to
@@ -464,8 +473,8 @@ The goal of these steps isn't to override the author or the reviewer, but to get
 more perspectives and voices involved. Often this will clarify the issue and its
 trade-offs, and provide a simple resolution that all parties are happy with.
 However, in some cases, the underlying conflict isn't actually addressed. While
-there is a desire to generally bias towards the direction of the code owners
-during reviews, reviews should _not_ turn into a voting process. The reason for
+there is a desire to generally bias towards the direction of the owners during
+reviews, reviews should _not_ turn into a voting process. The reason for
 proceeding in a specific direction should always be explained sufficiently that
 all parties on the review are satisfied by the explanation and don't feel the
 need to escalate.

docs/project/pull_request_workflow.md

@@ -120,7 +120,8 @@ Specifically, one pull request cannot serve as the _base_ for another pull
 request, so each pull request will include all of the commits and diffs of the
 preceding pull requests in the stack.
 
-We suggest a specific workflow to address this:
+We suggest a specific workflow to address this (note, commit access is
+required):
 
 1.  Create your initial pull request from a branch of your fork, nothing special
     is needed at this step. Let's say you have a branch `feature-basic` in your
@@ -158,9 +159,6 @@ We suggest a specific workflow to address this:
     be force pushed as necessary and deleted. These branch names should only be
     used for this ephemeral purpose. All other branch names are protected.
 
-    If you don't yet have this permission, just ask an [admin](groups.md#admins)
-    for help.
-
 3.  Create your stacked branch on your fork:
 
     ```shell

proposals/p1367.md

@@ -0,0 +1,188 @@
+# Remove CODEOWNERS
+
+<!--
+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/1367)
+
+<!-- toc -->
+
+## Table of contents
+
+-   [Problem](#problem)
+-   [Background](#background)
+    -   [Current CODEOWNERS](#current-codeowners)
+    -   [CODEOWNERS issues](#codeowners-issues)
+-   [Proposal](#proposal)
+-   [Details](#details)
+    -   [Group access controls](#group-access-controls)
+    -   [rm CODEOWNERS](#rm-codeowners)
+    -   [Auto-assignment](#auto-assignment)
+    -   [Stacked PRs](#stacked-prs)
+-   [Rationale](#rationale)
+-   [Alternatives considered](#alternatives-considered)
+    -   [Fine-grained CODEOWNERS](#fine-grained-codeowners)
+    -   [Broaden CODEOWNERS](#broaden-codeowners)
+
+<!-- tocstop -->
+
+## Problem
+
+We want to restrict who can merge files, in particular to ensure reviews are
+done.
+
+Currently we use CODEOWNERS for this purpose. However, CODEOWNERS can lead to
+review bottlenecks, creating possibly unnecessary hindrances to development, as
+discussed in the background.
+
+## Background
+
+### Current CODEOWNERS
+
+We currently use, in brief:
+
+```CODEOWNERS
+*                   @carbon-language/implementation-team
+/*                  @carbon-language/implementation-team
+/*.md               @carbon-language/carbon-leads @carbon-language/implementation-team
+/docs/              @carbon-language/carbon-leads @carbon-language/implementation-team
+/proposals/         @carbon-language/carbon-leads
+/LICENSE            @carbon-language/carbon-leads
+CODEOWNERS          @carbon-language/carbon-leads
+```
+
+### CODEOWNERS issues
+
+[Issue #413: Consider removing CODEOWNERS -- at least for now](https://github.com/carbon-language/carbon-lang/issues/413)
+was used to discuss this issue. This can lead to a few issues:
+
+-   CODEOWNERS defaults to assignment to the group, which can lead to a "tragedy
+    of the commons" situation where everybody expects someone else to review.
+    -   In assigning to the group, CODEOWNERS also makes for noisy PR
+        notifications.
+    -   Note both of these issues can be addressed with
+        [GitHub review settings](https://docs.github.com/en/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team).
+-   CODEOWNERS doesn't accurately handle the difference between a "major" change
+    and a "minor" change.
+    -   For example, a new proposal should only be approved by carbon-leads,
+        while a typo fix to a proposal would be fine for anyone to approve.
+    -   Decisions to make minor proposal edits more common, as in
+        [Issue #1009: Should doc edits maintain proposal links?](https://github.com/carbon-language/carbon-lang/issues/1009),
+        exacerbate this issue.
+
+## Proposal
+
+Remove CODEOWNERS, and instead use commit access to gate PRs.
+
+## Details
+
+### Group access controls
+
+-   Base: read (unchanged)
+    -   Should be the standard public permission.
+-   Contributors: "contributor" (previously write)
+    -   The "Contributors" group should be easy to get access to. On top of
+        read, this should include the ability to add/remove labels and projects
+        (may require some work to get right).
+    -   Versus "triage", "triage" seems to have powers including deleting
+        conversations, etc, that may not make sense for this group.
+-   Implementation team: write (unchanged)
+    -   For people expected to be reviewing commits.
+-   Moderators: triage
+    -   Includes
+        [separate GitHub moderation permissions](https://docs.github.com/en/communities/moderating-comments-and-conversations).
+-   Leads: maintain (unchanged)
+-   Admin team: admin (unchanged)
+
+For people who have access, settings are at:
+
+-   [Organization repository roles](https://github.com/organizations/carbon-language/settings/roles)
+-   [Organization moderators](https://github.com/organizations/carbon-language/settings/moderators)
+-   [Repository collaborators and teams](https://github.com/carbon-language/carbon-lang/settings/access)
+
+### rm CODEOWNERS
+
+This should only be done after group access controls are updated. Once done,
+group access controls are the last word on who can commit PRs.
+
+### Auto-assignment
+
+This PR [introduces auto-assignment](/.github/workflows/assign_prs.yaml) in
+order to ensure PRs aren't lost. It provides categories of assignment, and a
+fallback for other PRs that don't have explicit assignment.
+
+### Stacked PRs
+
+Advice for
+[stacked PRs](/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests)
+is no longer "just ask an admin"; it is "commit access required". This is
+because removing CODEOWNERS removes boundary between merge access _to a feature
+branch_ and merge access _to trunk_.
+
+## Rationale
+
+-   [Community and culture](/docs/project/goals.md#community-and-culture)
+    -   The intent is to make it easier to get PRs reviewed and reduce
+        bottlenecks.
+
+## Alternatives considered
+
+### Fine-grained CODEOWNERS
+
+We could keep the [current fine-grained CODEOWNERS](#current-codeowners). In
+this setup, we would try to have specific directories owned by specific teams.
+
+We've ended up on this path so far so that we have some mechanical enforcement
+of who's reviewing. We may need to change up the groups a little, but a
+reasonable long-term expectation would be something like a docs team, an
+explorer team, a toolchain team, etc, with each owning their own files. For
+example, /docs is owned only by the docs team, and there are people in the docs
+team who aren't in other teams (and the other way around).
+
+As previously mentioned, if we were using this option, we may want to change
+[GitHub review settings](https://docs.github.com/en/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team)
+in order to fix email/assignment.
+
+Advantages:
+
+-   Relies on built-in GitHub CODEOWNERS featureset, intended for this purpose.
+    -   Can use built-in features for review assignment.
+-   Works with current
+    [stacked PR](/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests)
+    advice.
+
+Diadvantages:
+
+-   Retains team-specific bottlenecks: if none of a directory's owners are
+    available, other contributors cannot step in.
+    -   This could also be considered to be an advantage by some.
+-   GitHub's built-in features for review assignment are not as feature-rich as
+    some custom tooling.
+
+It's mainly because of the relative inflexibility of CODEOWNERS that we aren't
+doing this.
+
+### Broaden CODEOWNERS
+
+We could broaden the CODEOWNERS to something basic, like a single group owning
+everything. Reviews in a particular area become best practice, not mechanically
+enforced.
+
+Advantages:
+
+-   Simple to understand.
+    -   Avoids custom tooling.
+
+Disadvantages:
+
+-   We expect multiple subprojects within the carbon-lang repository, and this
+    approach would make it difficult to determine when to review a PR.
+    -   A large group means GitHub's built-in auto-assignment would assign to
+        arbitrary CODEOWNERS.
+    -   The [notification issues](#codeowners-issues) couldn't be addressed.
+
+This option is likely worse for our purposes than
+[fine-grained CODEOWNERS](#fine-grained-codeowners).