1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
|
# Code Reviews
The `platform/ec` repository makes use of a code review system that tries to
evenly distribute code reviews among available reviewers.
[TOC]
## How to request a review
Add `cros-ec-reviewers@google.com` to the reviewer line in Gerrit. A background
job will come around and replace the `cros-ec-reviewers@google.com` address with
the next available reviewer in the EC reviewer rotation. This typically takes on
the order of minutes.
Optionally, you can click the [FIND OWNERS] button in the UI, and select
`cros-ec-reviewers@google.com`.
## When to use review system
If you are modifying code in `common/`, `chip/`, or `core/`, feel free to use
the `cros-ec-reviewers@google.com` system. It is **never** a requirement to use
`cros-ec-reviewers@google.com`. You can always request a review from a specific
person.
## Responsibilities of reviewers
If the selected reviewer is unfamiliar with code in a CL, then that reviewer
should at least ensure that EC style and paradigms are being followed. Once EC
styles and paradigms are being followed, then the reviewer can give a +1 and add
the appropriate domain expert for that section of code.
Reviewers should try to give an initial response within 1 business day of
receiving a review request. Thereafter, they should try to respond to new
comments by the author within 1 business day.
## How can I join the rotation?
Add your name to the [list of reviewers][1].
## Code review guidelines
### Goals for guidelines
* Minimize review latency.
* Minimize duplication of effort among reviewers.
### Guidelines for all code reviews
* Keep each CL to one logical change.
* Where there are several layers of dependencies, break the patch up into
multiple CLs.
* Avoid significant scope increases in subsequent patchsets; when reviewers
ask for significant additions, consider adding them in a follow-up CL
instead of uploading a new patchset.
* Refactor in a separate CL from functional changes; this also applies to
unrelated style and typo fixes.
* See Google small CL [guidelines][5] for more tips.
* If you are about to send out more than 400 lines of non-trivial changes, and
you haven’t written a design doc, consider whether you need one and who
should review it.
* Prefer to discuss significant design issues in bugs or design docs, before
code review.
* Associate a bug with a code review (`BUG=...`) when context is relevant.
* Indicate clearly when a CL is not yet ready for review and when it is; don’t
start the review until you’ve stopped making changes.
* Use `repo upload --no-emails` to avoid sending emails for CLs not ready for
review.
* Use Gerrit comments as needed to clarify that a CL is now ready for review.
* Recommended: Leave “WIP” in the CL title until it is ready to review.
* Make sure that `make buildall` succeeds after each individual change; this
facilitates bisecting.
* Assign reviewers with a specific scope for each reviewer.
* Ideally one person will be qualified to review the entire CL; pick that
person.
* If multiple domain experts are required, indicate in the review request
which parts each of them should review; don’t add a reviewer without a
scope.
* If someone needs to be aware that a change is happening, add them as CC; do
this conservatively.
* TODO: Use the attention set to clarify whose turn it is to respond, when
that feature becomes available.
* Consider multi-stage review: Get LGTM within a certain scope before moving to
the next reviewer.
* This may help reduce the number of patchsets and duplicated review effort.
* It is probably most helpful when multiple reviewers should look at the same
files.
* Prioritize reviewers who may want fundamental changes, e.g. a component
maintainer or domain expert should look at it before a language expert.
* Keep related changes in a single relation chain if practical.
* If not, consider using [topics][2] in Gerrit to group related CLs. For
example, the topic ["CBI PCB supplier field"][6] links multiple CLs.
* Try to make visible updates actionable for reviewers.
* Don’t post a comment saying that you’ve done something before the patchset
that does it.
* Don’t post comments that say you’ll do something in the next patchset; just
wait for that patchset.
* If practical, don’t upload a new patchset until you can usefully respond to
all unresolved comments.
* Use `repo upload --no-emails` for patchsets that address comments before the
responses to those comments.
* Try to minimize rebases in the middle of a review; if practical, don’t
rebase until just before submitting.
* Try to make review comments maximally actionable for authors, who may be in
different timezones or may be managing a relation chain with multiple
reviewers.
* Try to respond to review requests or follow-up comments within 1 business
day.
* This is an SLO for single responses, not the entire lifecycle of the CL.
* Prefer to provide feedback on an entire CL in one shot, so that the author
could plausibly respond to all of it and need no further review.
* However, consider sending a reply after a partial review, if it would help
keep the response time within 1 business day.
* Resolve outstanding comments before submitting.
* Don’t mark a comment as resolved until the question has been answered,
request completed, concern assuaged, etc.
* Typically, a reviewer should not CR+2 a CL until the reviewer’s comments
have been resolved to their satisfaction. To reduce latency, a reviewer may
CR+2 a CL with the stated assumption that the author will address
outstanding minor comments; in this case, the author should resolve them
before submitting.
* Abandon changes that you don’t intend to keep working on. (`repo abandon`
doesn’t do it on Gerrit.)
* This should probably include CLs that, after review, you want to
substantially rewrite or don’t expect to submit for a long time.
### Guidelines for partner code reviews
* Complete partner-internal code review (using Gerrit) before assigning to
Google reviewers.
* Send CLs to `cros-ec-reviewers@google.com` for review by default.
* Or send to specific Googlers with domain expertise, consistent with above
guidelines.
### Guidelines for extended partner efforts
* Discuss at the beginning of the effort which Googlers should review or be CCed
on CLs.
* Optionally create a [GWSQ][4] specific to the effort.
## Reference
* [Coreboot Gerrit Guidelines][3]
* [Google small CL guidelines][5]
[1]: http://google3/chrome/crosinfra/gwsq/ec_reviewers
[2]: https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics
[3]: https://doc.coreboot.org/getting_started/gerrit_guidelines.html
[4]: https://g3doc.corp.google.com/gws/tools/gwsq/v3/g3doc/gerrit.md
[5]: https://google.github.io/eng-practices/review/developer/small-cls.html
[6]: https://chromium-review.googlesource.com/q/topic:%2522CBI+PCB+supplier+field%2522
|
