Commit f885edf6 authored by Steve Kobes's avatar Steve Kobes Committed by Commit Bot

Update policy for using TBR.

(1) Add text allowing TBR with relands which previously appeared on the
    sites page.
(2) Add guidance that TBR'ed reverts should be clean and recent.
(3) Make "OWNERS file details" a subsection of "OWNERS files".

Discussed on chromium-dev at: goo.gl/kCw2HK

Change-Id: Ic0329574652926483f9cb33100635f89fa8b0107
Reviewed-on: https://chromium-review.googlesource.com/1217167Reviewed-by: default avatarDirk Pranke <dpranke@chromium.org>
Reviewed-by: default avatarWez <wez@chromium.org>
Reviewed-by: default avatarWill Harris <wfh@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590286}
parent f6c70a2c
...@@ -98,11 +98,68 @@ Seldom-updated directories may have exceptions to the "substantiality" and ...@@ -98,11 +98,68 @@ Seldom-updated directories may have exceptions to the "substantiality" and
"recency" requirements. Directories in `third_party` should list those most "recency" requirements. Directories in `third_party` should list those most
familiar with the library, regardless of how often the code is updated. familiar with the library, regardless of how often the code is updated.
### OWNERS file details
Refer to the [source code](https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/owners.py)
for all details on the file format.
This example indicates that two people are owners, in addition to any owners
from the parent directory. `git cl owners` will list the comment after an
owner address, so this is a good place to include restrictions or special
instructions.
```
# You can include comments like this.
a@chromium.org
b@chromium.org # Only for the frobinator.
```
A `*` indicates that all committers are owners:
```
*
```
The text `set noparent` will stop owner propagation from parent directories.
This should be rarely used. If you want to use `set noparent` except for IPC
related files, please first reach out to chrome-eng-review@google.com.
In this example, only the two listed people are owners:
```
set noparent
a@chromium.org
b@chromium.org
```
The `per-file` directive allows owners to be added that apply only to files
matching a pattern. In this example, owners from the parent directory
apply, plus one person for some classes of files, and all committers are
owners for the readme:
```
per-file foo_bar.cc=a@chromium.org
per-file foo.*=a@chromium.org
per-file readme.txt=*
```
Note that `per-file` directives cannot directly specify subdirectories, e.g:
```
per-file foo/bar.cc=a@chromium.org
```
is not OK; instead, place a `per-file` directive in `foo/OWNERS`.
Other `OWNERS` files can be included by reference by listing the path to the
file with `file://...`. This example indicates that only the people listed in
`//ipc/SECURITY_OWNERS` can review the messages files:
```
per-file *_messages*.h=set noparent
per-file *_messages*.h=file://ipc/SECURITY_OWNERS
```
## TBR ("To Be Reviewed") ## TBR ("To Be Reviewed")
"TBR" is our mechanism for post-commit review. It should be used rarely and "TBR" is our mechanism for post-commit review. It should be used rarely and
only in cases where a review is unnecessary or as described below. The most only in cases where a normal review is unnecessary, as described under
common use of TBR is to revert patches that broke the build. "When to TBR", below.
TBR does not mean "no review." A reviewer TBR-ed on a change should still TBR does not mean "no review." A reviewer TBR-ed on a change should still
review the change. If there are comments after landing, the author is obligated review the change. If there are comments after landing, the author is obligated
...@@ -111,6 +168,8 @@ to address them in a followup patch. ...@@ -111,6 +168,8 @@ to address them in a followup patch.
Do not use TBR just because a change is urgent or the reviewer is being slow. Do not use TBR just because a change is urgent or the reviewer is being slow.
Contact the reviewer directly or find somebody else to review your change. Contact the reviewer directly or find somebody else to review your change.
### How to TBR
To send a change TBR, annotate the description and send email like normal. To send a change TBR, annotate the description and send email like normal.
Otherwise the reviewer won't know to review the patch. Otherwise the reviewer won't know to review the patch.
...@@ -129,10 +188,29 @@ Otherwise the reviewer won't know to review the patch. ...@@ -129,10 +188,29 @@ Otherwise the reviewer won't know to review the patch.
reviewer2: Please review changes to bar/ reviewer2: Please review changes to bar/
``` ```
### TBR-ing certain types of mechanical changes ### When to TBR
#### Reverts and relands
The most common use of TBR is to revert patches that broke the build. Clean
reverts of recent patches may be submitted TBR. However, TBR should not be used
if the revert required non-trivial conflict resolution, or if the patch being
reverted is older than a few days.
Sometimes you might do something that affects many callers in different A developer relanding a patch can TBR the OWNERS for changes which are identical
directories. For example, adding a parameter to a common function in to the original (reverted) patch. If the reland patch contains any new changes
(such as bug fixes) on top of the original, those changes should go through the
normal review process.
When creating a reland patch, you should first upload an up-to-date patchset
with the exact content of the original (reverted) patch, and then upload the
patchset to be relanded. This is important for the reviewers to understand what
the fix for relanding was.
#### Mechanical changes
You can use TBR with certain mechanical changes that affect many callers in
different directories. For example, adding a parameter to a common function in
`//base`, with callers in `//chrome/browser/foo`, `//net/bar`, and many other `//base`, with callers in `//chrome/browser/foo`, `//net/bar`, and many other
directories. If the updates to the callers is mechanical, you can: directories. If the updates to the callers is mechanical, you can:
...@@ -151,7 +229,7 @@ This process ensures that all code is reviewed prior to checkin and that the ...@@ -151,7 +229,7 @@ This process ensures that all code is reviewed prior to checkin and that the
concept of the change is reviewed by a qualified person, but you don't have to concept of the change is reviewed by a qualified person, but you don't have to
wait for many individual owners to review trivial changes to their directories. wait for many individual owners to review trivial changes to their directories.
### TBR-ing documentation updates #### Documentation updates
You can TBR documentation updates. Documentation means markdown files, text You can TBR documentation updates. Documentation means markdown files, text
documents, and high-level comments in code. At finer levels of detail, comments documents, and high-level comments in code. At finer levels of detail, comments
...@@ -170,61 +248,3 @@ comments inside functions. ...@@ -170,61 +248,3 @@ comments inside functions.
* Be sure to actually send out the email for the code review. If you get one, * Be sure to actually send out the email for the code review. If you get one,
please actually read the changes. please actually read the changes.
## More information
### OWNERS file details
Refer to the [source code](https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/owners.py)
for all details on the file format.
This example indicates that two people are owners, in addition to any owners
from the parent directory. `git cl owners` will list the comment after an
owner address, so this is a good place to include restrictions or special
instructions.
```
# You can include comments like this.
a@chromium.org
b@chromium.org # Only for the frobinator.
```
A `*` indicates that all committers are owners:
```
*
```
The text `set noparent` will stop owner propagation from parent directories.
This should be rarely used. If you want to use `set noparent` except for IPC
related files, please first reach out to chrome-eng-review@google.com.
In this example, only the two listed people are owners:
```
set noparent
a@chromium.org
b@chromium.org
```
The `per-file` directive allows owners to be added that apply only to files
matching a pattern. In this example, owners from the parent directory
apply, plus one person for some classes of files, and all committers are
owners for the readme:
```
per-file foo_bar.cc=a@chromium.org
per-file foo.*=a@chromium.org
per-file readme.txt=*
```
Note that `per-file` directives cannot directly specify subdirectories, e.g:
```
per-file foo/bar.cc=a@chromium.org
```
is not OK; instead, place a `per-file` directive in `foo/OWNERS`.
Other `OWNERS` files can be included by reference by listing the path to the
file with `file://...`. This example indicates that only the people listed in
`//ipc/SECURITY_OWNERS` can review the messages files:
```
per-file *_messages*.h=set noparent
per-file *_messages*.h=file://ipc/SECURITY_OWNERS
```
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment