[PATCH] D89995: Make the post-commit review expectations more explicit with respect to revert

Sun Oct 25 17:36:34 PDT 2020

sivachandra added inline comments.

================
Comment at: llvm/docs/CodeReview.rst:49-51
+author, unless they are unresponsive). Developers often disagree, and erring on
+the side of the developer asking for more review prevents any lingering
+disagreement over code in the tree. This does not indicate any fault from the
----------------
sivachandra wrote:
> mehdi_amini wrote:
> > sivachandra wrote:
> > > mehdi_amini wrote:
> > > > sivachandra wrote:
> > > > > The wording seems to imply that this would apply even for opinion driven disagreements. I would prefer a wording which implies something more concrete. Also, in a way, it seems like the post-commit review practice is in conflict with the wording here. As in, if concerns are raised post-commit, then the post-commit review practice requires the original author to address those concerns even post-commit. I would think a revert will only happen if the original author is not acting on/addressing the concerns. Even in such cases, the revert should be justified. An example justifiable reason can be that //progress// cannot be made with out a revert.
> > > > > 
> > > > > There are of course other obvious reasons to revert, but I am assuming that this discussion is not about those reasons.
> > > > > The wording seems to imply that this would apply even for opinion driven disagreement
> > > > 
> > > > How to you handle this in pre-commit review? Feel free to suggest an alternative wording, but keep in mind the spirit (see below).
> > > > 
> > > > In general the mindset is that there shouldn't be a difference whether I express my opposition to a patch 10min before you want to push it or 10 min after you pushed it.
> > > > 
> > > > >  As in, if concerns are raised post-commit, then the post-commit review practice requires the original author to address those concerns even post-commit. 
> > > > 
> > > > This is a case-by-case: if someone has the kind of concerns that would block a patch from landing (like design concerns), revert is the right approach I believe.
> > > > 
> > > > > Even in such cases, the revert should be justified. An example justifiable reason can be that progress cannot be made with out a revert.
> > > > 
> > > > There is an obvious justification: the patch should not have been landed. A practical concern is that revert become harder quickly as other part of the codebase start to build on top of the landed change.
> > > > In the extreme what you're describing would justify that I could land anything and we would not revert it unless it is breaking or blocking someone else. This does not seem consistent with the rest of this doc, neither with the actual practices.
> > > > 
> > > > 
> > > > 
> > > [As with my other comment, I am assuming we are not discussing the obvious cases. Also, I was not aware of the other thread before I started commenting here.]
> > > 
> > > IIUC, the current policy of post-commit review assumes good faith actors. So, if a good faith post-commit reviewer has concerns with a commit, the good faith committer is expected to address them. How they are to be addressed is something the reviewer and committer should work out. It could require a revert, or it could just be a simple follow up fix. It should be left up to the committer and the reviewer to work out the way forward. This arrangement does not make the post-commit review carry any less weight than a pre-commit review.
> > > 
> > > That said, if you want to prescribe how a post-commit review should be addressed/approached in a policy document, I have no problems with it. But, the wording should not allow bad actors to exploit it. That is why I say the prescription should be about more concrete situations. As in, prescribe exactly when a revert is required as apposed to a blanket rule to revert first. For example, does the post-commit reviewer have concrete suggestions for clear cut issues identified, or are they just listing their opinions. And, when the reviewer does have concrete suggestions, does it need a revert to address them, or can they be addressed in a follow up change? I am unable to see why a blanket revert first policy is the best approach. If you have some situations where a revert is the best way forward, then do share them here.
> > Did you read the email thread I linked in the description by the way?
> > 
> > > That said, if you want to prescribe how a post-commit review should be addressed/approached in a policy document, I have no problems with it. But, the wording should not allow bad actors to exploit it.
> > 
> > What kind of "bad actors" do you expect? Have you seen "bad actors" in the community so far?
> > I believe our guidelines / docs in general assume good faith from the community and encode the generally acceptable practices assuming this. 
> > 
> > > I am unable to see why a blanket revert first policy is the best approach. If you have some situations where a revert is the best way forward, then do share them here.
> > 
> > My angle here is quite "simple": pre and post commit review should be aligned: if my concern with the design aspect of your patch would prevent you from landing it, then I should be able to also ask you to revert it if I think there is a design problem that can't be addressed in-tree.
> > The text here does not prescribe a "blanket revert first policy", only a "revert on request policy".
> > 
> > Did you read the email thread I linked in the description by the way?
> 
> Yes, I did :)
> 
> > The text here does not prescribe a "blanket revert first policy", only a "revert on request policy".
> 
> Ah, sorry! I had this tab open and forgot to refresh and missed your update which changed the wording to "they may ask for a revert". I wrote my reply based on the earlier diff which said, "the patch should be reverted promptly". Sorry again, and this latest diff LGTM.
> 
> 
> What kind of "bad actors" do you expect?

For example, someone can ask for a revert without actually showing a path forward or raising specific concerns. For example, they can just say, "this landed without enough review, please revert." Should one revert in response?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89995