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

[Philip Reames via Phabricator via llvm-commits]

reames marked 14 inline comments as done.
reames added a comment.

Closing out inline comments and replying where it seems to make sense.

I'm getting slightly swamped by the volume of comments.  If I miss replying to something you think needs more attention, please re-raise.  It's entirely possible I missed something.



================
Comment at: llvm/docs/DeveloperPolicy.rst:317-318
+* 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 receive substantial :ref:`post-commit review <post_commit_review>`
----------------
hubert.reinterpretcast wrote:
> reames wrote:
> > hubert.reinterpretcast wrote:
> > > hubert.reinterpretcast wrote:
> > > > How portable and self-contained is this test case?
> > > 
> > I can't think of anyway to craft a useful guideline here (beyond what is already said elsewhere).  If you have suggested wording, please share.
> > 
> > (Though, do you mind if we defer this to a future change?  This seems minor compared to the rest of it.)
> I think moving the "expectations" part up would help towards addressing my concern here.
I don't get what you're going for here, and I think you might be getting too detailed for a high level guideline.  If you have suggestions for specific wording, please make them.


================
Comment at: llvm/docs/DeveloperPolicy.rst:317-318
+* 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 receive substantial :ref:`post-commit review <post_commit_review>`
----------------
reames wrote:
> hubert.reinterpretcast wrote:
> > reames wrote:
> > > hubert.reinterpretcast wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > How portable and self-contained is this test case?
> > > > 
> > > I can't think of anyway to craft a useful guideline here (beyond what is already said elsewhere).  If you have suggested wording, please share.
> > > 
> > > (Though, do you mind if we defer this to a future change?  This seems minor compared to the rest of it.)
> > I think moving the "expectations" part up would help towards addressing my concern here.
> I don't get what you're going for here, and I think you might be getting too detailed for a high level guideline.  If you have suggestions for specific wording, please make them.
I played with this, and it seemed to negatively impact readability overall for me.  I'm going to leave the order as is.

See also the newly added first clause of the expectation section.  I think that might help with the core concern.  


================
Comment at: llvm/docs/DeveloperPolicy.rst:320
+* 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.)
----------------
hubert.reinterpretcast wrote:
> reames wrote:
> > hubert.reinterpretcast wrote:
> > > ... doesn't the "further destabilize" note below actually apply here too?
> > Judgement is always required.  I don't think it's useful to repeat that in every guideline.  Do you think I need to emphasize that more somewhere?
> For this too, I think moving the "expectations" part up will clarify the "priorities".
The expectation section always applies.  I don't want to start duplicating it everywhere.  I think that would greatly harm readability.


================
Comment at: llvm/docs/DeveloperPolicy.rst:323
+* 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).
----------------
hubert.reinterpretcast wrote:
> reames wrote:
> > hubert.reinterpretcast wrote:
> > > This point along with the "if the author themselves would revert" below means that unilateral reverts without discussion is being encouraged here. I'm not in favour of action without discussion.
> > This is our long standing practice.  
> Sure, but probably for specific reasons and not for all. Otherwise, there wouldn't be "requests" to revert at all (except from people with no commit access).
Again, long standing practice.  I'm explicitly not changing anything, I'm just documenting.  I ask you move this point to the llvm-dev thread for broader discussion and possibly a follow up patch if warranted.  


================
Comment at: llvm/docs/DeveloperPolicy.rst:338
+
+What are the expectations around a revert?
+
----------------
hubert.reinterpretcast wrote:
> I think moving this part up (and reducing the assumptions below that the reverting party is not the author and also the assumptions that the //action// of "reverting" is taking place) would be an improvement.
I decided moving the section was a net negative, but did try to reword a few of the clauses to be more explicit about it applying regardless of whether you were reverting your own patch or someone elses.


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

https://reviews.llvm.org/D99305