[llvm] 0918f44 - [docs] Document our norms around reverts

Wed Apr 7 21:02:32 PDT 2021

Author: Philip Reames
Date: 2021-04-07T21:02:19-07:00
New Revision: 0918f44e2670ed9beece4b37c33ff803de5199bb

URL: https://github.com/llvm/llvm-project/commit/0918f44e2670ed9beece4b37c33ff803de5199bb
DIFF: https://github.com/llvm/llvm-project/commit/0918f44e2670ed9beece4b37c33ff803de5199bb.diff

LOG: [docs] Document our norms around reverts

This has come up a few times recently, and I was surprised to notice that we don't have anything in the docs.

This patch deliberately sticks to stuff that is uncontroversial in the community. Everything herein is thought to be widely agreed to by a large majority of the community.  A few things were noted and removed in review which failed this standard, if you spot anything else, please point it out.

Differential Revision: https://reviews.llvm.org/D99305

Added: 
    

Modified: 
    llvm/docs/CodeReview.rst
    llvm/docs/DeveloperPolicy.rst

Removed: 
    


################################################################################
diff  --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
index 52af331c56af..c272cf34694f 100644

--- a/llvm/docs/CodeReview.rst
+++ b/llvm/docs/CodeReview.rst
@@ -36,13 +36,15 @@ Please note that the developer responsible for a patch is also
 responsible for making all necessary review-related changes, including
 those requested during any post-commit review.
 
+.. _post_commit_review:
+
 Can Code Be Reviewed After It Is Committed?
 -------------------------------------------
 
 Post-commit review is encouraged, and can be accomplished using any of the
 tools detailed below. There is a strong expectation that authors respond
 promptly to post-commit feedback and address it. Failure to do so is cause for
-the patch to be reverted.
+the patch to be :ref:`reverted <revert_policy>`.
 
 If a community member expresses a concern about a recent commit, and this
 concern would have been significant enough to warrant a conversation during

diff  --git a/llvm/docs/DeveloperPolicy.rst b/llvm/docs/DeveloperPolicy.rst
index 361aedb8e6c5..c2171eb588e2 100644
--- a/llvm/docs/DeveloperPolicy.rst
+++ b/llvm/docs/DeveloperPolicy.rst
@@ -298,6 +298,88 @@ For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and
 omissions can be handled by sending a reply to the commits mailing list.
 
+.. _revert_policy:
+
+Patch reversion policy
+----------------------
+
+As a community, we strongly value having the tip of tree in a good state while
+allowing rapid iterative development.  As such, we tend to make much heavier
+use of reverts to keep the tree healthy than some other open source projects,
+and our norms are a bit 
diff erent.
+
+How should you respond if someone reverted your change?
+
+* Remember, it is normal and healthy to have patches reverted.  Having a patch
+  reverted does not necessarily mean you did anything wrong.
+* We encourage explicitly thanking the person who reverted the patch for doing
+  the task on your behalf.
+* If you need more information to address the problem, please follow up in the
+  original commit thread with the reverting patch author.
+
+When should you revert your own change?
+
+* Any time you learn of a serious problem with a change, you should revert it.
+  We strongly encourage "revert to green" as opposed to "fixing forward".  We
+  encourage reverting first, investigating offline, and then reapplying the
+  fixed patch - possibly after another round of review if warranted.
+* If you break a buildbot in a way which can't be quickly fixed, please revert.
+* If a test case that demonstrates a problem is reported in the commit thread,
+  please revert and investigate offline.
+* If you receive substantial :ref:`post-commit review <post_commit_review>`
+  feedback, please revert and address said feedback before recommitting.
+  (Possibly after another round of review.)
+* If you are asked to revert by another contributor, please revert and discuss
+  the merits of the request offline (unless doing so would further destabilize
+  tip of tree).
+
+When should you revert someone else's change?
+
+* In general, if the author themselves would revert the change per these
+  guidelines, we encourage other contributors to do so as a courtesy to the
+  author.  This is one of the major cases where our norms 
diff er from others;
+  we generally consider reverting a normal part of development.  We don't
+  expect contributors to be always available, and the assurance that a
+  problematic patch will be reverted and we can return to it at our next
+  opportunity enables this.
+
+What are the expectations around a revert?
+
+* Use your best judgment. If you're uncertain, please start an email on
+  the commit thread asking for assistance.  We aren't trying to enumerate
+  every case, but rather give a set of guidelines.
+* You should be sure that reverting the change improves the stability of tip
+  of tree.  Sometimes reverting one change in a series can worsen things
+  instead of improving them.  We expect reasonable judgment to ensure that
+  the proper patch or set of patches is being reverted.
+* The commit message for the reverting commit should explain why patch
+  is being reverted.
+* It is customary to respond to the original commit email mentioning the
+  revert.  This serves as both a notice to the original author that their
+  patch was reverted, and helps others following llvm-commits track context.
+* Ideally, you should have a publicly reproducible test case ready to share.  
+  Where possible, we encourage sharing of test cases in commit threads, or
+  in PRs.  We encourage the reverter to minimize the test case and to prune
+  dependencies where practical.  This even applies when reverting your own
+  patch; documenting the reasons for others who might be following along
+  is critical.
+* It is not considered reasonable to revert without at least the promise to
+  provide a means for the patch author to debug the root issue.  If a situation
+  arises where a public reproducer can not be shared for some reason (e.g.
+  requires hardware patch author doesn't have access to, sharp regression in
+  compile time of internal workload, etc.), the reverter is expected to be
+  proactive about working with the patch author to debug and test candidate
+  patches.
+* Reverts should be reasonably timely.  A change submitted two hours ago
+  can be reverted without prior discussion.  A change submitted two years ago
+  should not be.  Where exactly the transition point is is hard to say, but
+  it's probably in the handful of days in tree territory.  If you are unsure,
+  we encourage you to reply to the commit thread, give the author a bit to
+  respond, and then proceed with the revert if the author doesn't seem to be
+  actively responding.
+* When re-applying a reverted patch, the commit message should be updated to
+  indicate the problem that was addressed and how it was addressed.
+
 Obtaining Commit Access
 -----------------------
 


        
