Add some guidance on rollups and priorities. (#402)

* Add some guidance on rollups and priorities.

* Update for comments from manishearth.

* Clarify relationship of maybe and iffy.

* Update link to rollups section.

* Clarify rollup=maybe default.

* Remove "pure refactoring" from rollup guidance.

Author: ehuss

src/compiler/reviews.md

@@ -2,7 +2,7 @@
 
 Every PR that lands in the compiler and its associated crates must be
 reviewed by at least one person who is knowledgeable with the code in
-question. 
+question.
 
 When a PR is opened, you can request a reviewer by including `r?
 @username` in the PR description. If you don't do so, the highfive bot
@@ -27,11 +27,76 @@ delegate+` or `@bors delegate=username`. This will allow the PR author
 to approve the PR by issuing `@bors` commands like the ones above
 (but this privilege is limited to the single PR).
 
-## High priority issues
+## Rollups
 
-When merging high priority issues (`P-critical` and `P-high`) it's
-recommended to avoid rollups and bump a bit the priority of the PR in
-the homu queue by issuing `@bors r+ rollup=never p=1`.
+All reviewers are strongly encouraged to explicitly mark a PR as to whether or
+not it should be part of a [rollup] with one of the following:
+
+- `rollup=always`: These PRs are very unlikely to break tests or have performance
+  implications. Example scenarios:
+    - Changes are limited to documentation, comments, etc. that is highly
+      unlikely to fail a build.
+    - Changes cannot have performance implications.
+    - Your PR is not landing possibly-breaking or behavior altering changes.
+        - Feature stabilization without other changes is likely fine to
+          rollup, though.
+- `rollup=maybe`: This is the **default** if you do not specify a rollup
+  status. Use this if you don't have much confidence that it won't break
+  tests. This can be used if you aren't sure if it should be one of the other
+  categories. Since this is the default, there is usually no need to
+  explicitly specify this, unless you are un-marking the rollup level from a
+  previous command.
+- `rollup=iffy`: Use this for mildly risky PRs (more risky than "maybe").
+  Example scenarios:
+    - The PR is large and non-additive (note: adding 2000 lines of completely
+      new tests is fine to rollup).
+    - Messes too much with:
+        - LLVM or code generation
+        - bootstrap or the build system
+        - build-manifest
+    - Has platform-specific changes that are not checked by the normal PR checks.
+    - May be affected by MIR migrate mode.
+- `rollup=never`: This should *never* be included in a rollup (**please**
+  include a comment explaining why you have chosen this). Example scenarios:
+    - May have performance implications.
+    - May cause unclear regressions (we would likely want to bisect to this PR
+      specifically, as it would be hard to identify as the cause from a
+      rollup).
+    - Has a high chance of failure.
+    - Is otherwise dangerous to rollup.
+
+> **Note**:\
+> `@bors rollup` is equivalent to `@bors rollup=always`\
+> `@bors rollup-` is equivalent to `@bors rollup=never`
+
+## Priority
+
+Reviewers are encouraged to set one of the rollup statuses listed above
+instead of setting priority. Bors automatically sorts based on the rollup
+status (never is the highest priority, always is the lowest), and also by PR
+age. If you do change the priority, please use your best judgment to balance
+fairness with other PRs.
+
+The following is some guidance for setting priorities:
+
+- 1-5
+    - P-high issue fixes
+    - Toolstate fixes
+    - Beta-nominated PRs
+    - Submodule updates
+- 5+
+    - P-critical issue fixes
+- 10+
+    - Bitrot-prone PRs (particularly very large ones that touch many files)
+    - Urgent PRs
+    - Beta backports
+- 20+
+    - High priority that needs to jump ahead of any rollups
+    - Fixes or changes something that has a high risk of being re-broken by
+      another PR in the queue.
+- 1000
+    - Absolutely critical fixes
+    - Release promotions
 
 ## Expectations for r+
 
@@ -46,3 +111,5 @@ done the review, and the code has not changed substantially since the
 review was done.  Rebasing is fine, but changes in functionality
 typically require re-review (though it's a good idea to try and
 highlight what has changed, to help the reviewer).
+
+[rollup]: ../release/rollups.md

src/libs/maintaining-std.md

@@ -217,7 +217,7 @@ PRs to [`rust-lang/rust`] aren’t merged manually using GitHub’s UI or by pus
 
 For Libs PRs, rolling up is usually fine, in particular if it's only a new unstable addition or if it only touches docs (with the exception of intra doc links which complicates things while the feature has bugs...).
 
-If a submodule is affected then probably don't `rollup`. If the feature affects perf then also avoid `rollup` -- mark it as `rollup=never`.
+See the [rollup guidelines] for more details on when to rollup.
 
 ### When there’s new public items
 
@@ -263,3 +263,4 @@ Where `unsafe` and `const` is involved, e.g., for operations which are "unconst"
 [Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops
 [rust/pull/46799]: https://github.com/rust-lang/rust/pull/46799
 [hashbrown/pull/119]: https://github.com/rust-lang/hashbrown/pull/119
+[rollup guidelines]: ../compiler/reviews.md#rollups

src/release/rollups.md

@@ -11,13 +11,9 @@ dependent are marked with the `rollup` command to bors (`@bors r+ rollup` to
 approve a PR and mark as a rollup, `@bors rollup` to mark a previously approved
 PR, `@bors rollup-` to un-mark as a rollup).  'Performing a Rollup' then means
 collecting these changes into one PR and merging them all at once. The rollup
-command accepts three values `always`, `maybe`, and `never`. `@bors rollup` is
-equivalent to `rollup=always` (which will indicate that a PR should always be
-included in a rollup), and `@bors rollup-` is equivalent to `@bors rollup=maybe`
-which is used to indicate that someone should try rollup the PR. `rollup=never`
-indicates that a PR should never be included in a rollup, this should generally
-only be used for PRs which are large additions or changes which could cause
-breakage or large perf changes.
+command accepts four values `always`, `maybe`, `iffy`, and `never`. See [the
+Rollups section] of the review policies for guidance on what these different
+statuses mean.
 
 You can see the list of rollup PRs on Rust's [Homu queue], they are
 listed at the bottom of the 'approved' queue with a priority of 'rollup' meaning
@@ -78,3 +74,4 @@ If a rollup continues to fail you can run the `@bors rollup=never` command to
 never rollup the PR in question.
 
 [Homu queue]: https://buildbot2.rust-lang.org/homu/queue/rust
+[the Rollups section]: ../compiler/reviews.md#rollups