[PATCH] D99305: [docs] Document our norms around reverts

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: llvm/docs/DeveloperPolicy.rst:330
+* 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;
----------------
I think wording wise we can just remove the "... as a favor to the author." here and reword a little below...


================
Comment at: llvm/docs/DeveloperPolicy.rst:332
+  author.  This is one of the major cases where our norms differ from others;
+  we generally consider reverting someone's change to be a favor to them.  We
+  don't expect contributors to be always available, and the assurance that a
----------------
"we generally consider reverting a normal part of development" or something in this vein perhaps?


================
Comment at: llvm/docs/DeveloperPolicy.rst:341
+
+* Don't follow the guidelines given here blindly.  If the result doesn't
+  make sense given the context, please start with an email in the commit
----------------
"Use your best judgment. If you're uncertain, please start an email on the commit thread asking for assistance." Perhaps?


================
Comment at: llvm/docs/DeveloperPolicy.rst:343
+  make sense given the context, please start with an email in the commit
+  thread.  Life is complicated, and this document can't hope to get
+  every cornercase right.
----------------
"We aren't trying to enumerate every case, but rather give a set of guidelines."


================
Comment at: llvm/docs/DeveloperPolicy.rst:345
+  every cornercase right.
+* 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
----------------
"Know the context. Sometimes a patch is part of a chain and you may need to take all of that into context when making your revert."


================
Comment at: llvm/docs/DeveloperPolicy.rst:365
+  compile time of internal workload, etc.), the reverter is expected to be
+  strongly proactive about working with the patch author to debug and test
+  candidate patches.
----------------
s/strongly//


================
Comment at: llvm/docs/DeveloperPolicy.rst:377
+
+How should you respond if someone reverted your change?
+
----------------
Honestly I think this might be better at the beginning of this. A framing reminder for people who come here after a patch has been reverted.


================
Comment at: llvm/docs/DeveloperPolicy.rst:379
+
+* 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
----------------
I'm not a fan of this sentence. I know where you're trying to get to though and I do like that, but it's a little sticky. I think it can be removed until we can come up with something better though :)


================
Comment at: llvm/docs/DeveloperPolicy.rst:382
+  original commit thread with the reverting patch author.
+* It is normal and healthy to have patches reverted.  Having a patch reverted
+  does not necessarily mean you did anything wrong.
----------------
Perhaps begin this sentence with "Remember: "?

And maybe put it first?


================
Comment at: llvm/docs/DeveloperPolicy.rst:336-337
+  opportunity enables this.
+* The other case for a third-party revert is for serious norm violations.
+  Such situations are rare.
+
----------------
reames wrote:
> rupprecht wrote:
> > I don't know if I have a specific suggestion that is better, but I'm not a fan of the wording here, especially with the "serious norm violations" wording.
> > 
> > While I agree with the general policy around timely reverts, starting a discussion before reverting someone else's change, etc., I also think that documenting these as "serious norm violations" unnecessarily escalates these cases when they do happen. If someone has reverted a change that falls into this category, there's a decent chance that it wasn't an intentional norms violation, and the discussion should start out with "Hey, can we talk more about this revert? I don't know if reverting was the right decision here" and not "This is a serious norms violation as documented in [link to these docs], please undo the revert".
> I want to avoid getting into the discussion of norm violation handling in this review.  You raise several good points, and we should eventually document this, but I expect it to be an involved discussion.  I really want the basics landed before opening that can of worms.
> 
> If anyone can think of wording to further soften this sentence, please throw it out.  I can't think of anything without expanding this bullet a lot, and every attempt I've made ran into something I thought needed discussion I'm hoping to avoid on this review.  
> 
> Maybe I should just strike that bullet for now entirely?  The previous bullet reads reasonably on it's own as an answer to the question.
I'd probably strike it. It's definitely part of the referring statement that got us here or you could just link to the developer policy and say something about that?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99305/new/

https://reviews.llvm.org/D99305