[llvm-dev] Documenting community norms around reverts (original) (raw)
Philip Reames via llvm-dev llvm-dev at lists.llvm.org
Wed Mar 24 15:52:51 PDT 2021
- Previous message: [llvm-dev] isHardwareLoopProfitable() called with empty assumption cache in hwloops pass
- Next message: [llvm-dev] Documenting community norms around reverts
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
I've posted a patch <https://reviews.llvm.org/D99305> to document our norms around reverts. I was recently surprised to not find anything in this spirit. A lot of newcomers find our norms a bit odd, so having this explicitly spelled out seems very worthwhile.
I tried to stick to stuff that is uncontroversial in the community. If you see something here that less than 90+% of the community would agree with, please point it out. I'll remove it for now, and possibly return to individual points if warranted in future reviews.
Philip
p.s. Draft text follows, but please make detailed wording suggestions on the review.
Reverts
As a community, we strongly value having the tip of tree in a good state. As such, we tend to make much heaver use of reverts than some other open source projects, and our norms are a bit different.
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 is reported in the commit thread which demonstrates a problem please revert, and investigate offline.
- 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 trip 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 favor to the author. This is one of the major cases where our norms differ from others; we generally consider reverting someones change to be a favor to them. 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.
- The other case for a third-party revert is for serious norm violations. As a general rule, you should never be reverting a patch for this unless you've already started a discussion highlight the problem on the original commit thread. There are exceptions where a rapid revert for a norm violation may be called for, but if those are relevant for you, you already know.
What are the expectations around a revert?
- 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.
- You should have a publicly reproducible test case ready to share. It is not considered reasonable to revert without at least the promise to produce a public test case in the near term. 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.
- Reverts should be reasonably timely. A change submitted two hours ago can be reverted by a third-party without prior discussion. A change submitted two years ago probably shouldn't 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.
How should you respond if someone reverted your change?
- In general, the appropriate response is to start by thanking them. If you need more information to address the problem, please follow up in the original commit thread with the reverting patch author.
- It is normal and healthy to have patches reverted. Having a patch reverted does not neccessarily mean you did anything wrong.
-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210324/f09e0171/attachment.html>
- Previous message: [llvm-dev] isHardwareLoopProfitable() called with empty assumption cache in hwloops pass
- Next message: [llvm-dev] Documenting community norms around reverts
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]