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

Sun Oct 25 04:21:40 PDT 2020

rengolin added a subscriber: dblaikie.
rengolin added a comment.

I don't want to "take sides" and I'm only looking at this review from what it is and my own experience in the community. But looking at D83088 <https://reviews.llvm.org/D83088>, I see a somewhat problematic review.

@dblaikie has a long list of deep reflections on the design and goals of the patch, which indicates to me that the patch wasn't clear to begin with. The patch was approved by @arsenm but the author didn't request a final check by David, who was the main reviewer, who himself later on (I assume as soon as he could), replied he still wasn't happy with the patch. If I was the author I would have reverted as soon as David ask for it.

In this particular case, the change is in the CFG, which is a core part of the compiler and touches so many things outside of its scope that it's almost impossible to "get it right" from the beginning without breaking bots, patterns, codegen and other people's work in progress. You just have to look at Chandler's decade old attempt at changing the pass manager to see that changes deep in the compiler take time, and are very bumpy.

If some CFG design is either wrong, or could have been better, the longer we take to revert the harder, or even impossible, it will be to do so. Too much code depends on it and by extending the window we may be creating an ecosystem of current patches now relying on that design to work. That is why we have the fast revert + reapply cycle. We don't have good pre-commit validation, so we must act swiftly on post-commit review and requests.

To me, personally, @silvas and @mehdi_amini's addenda to the policy make sense and clearly apply to this particular case.

================
Comment at: llvm/docs/CodeReview.rst:47-48

-In addition, if substantial problems are identified, it is expected that the
-patch is reverted and fixed offline. Before being recommitted, the patch
-generally undergoes further review, including by the community member who
-identified the problem and, in cases where the patch triggered a
-hardware-specific buildbot failure, a community member with access to hardware
-similar to that on the buildbot that the patch previously caused to fail.
+If a developer thinks that a recently committed patch would benefit from some
+more review, the patch should be reverted promptly (preferably by the original
+author, unless they are unresponsive). Developers often disagree, and erring on
----------------
mehdi_amini wrote:
> rengolin wrote:
> > nhaehnle wrote:
> > > This is too low a bar for reverting given how unresponsive reviewers are in practice. I would suggest instead:
> > > 
> > > > If a developer **identifies a problem** in a recently committed patch, the patch should reverted promptly ...
> > > 
> > > This also makes more sense because it aligns with the second paragraph, which talks about "the community member who **identified the problem**".
> > > 
> > > You can always think that more review is better, but unless there is something **concrete**, we err too much on the side of review limbo.
> > Actually, I agree with this statement. "thinks that ... would benefit" is a really low bar.
> > 
> > I originally read in the naive "standard LLVM approach" where developers are mostly sensible, but I agree with @nhaehnle in this one. Once we "encode" in a *policy*, there could be people taking it more literally.
> > 
> > I would also try to put some protection on the author that they must be contacted and in case of delay, other reviewers must be contacted, etc. We really want to avoid people silently reverting commits because they don't like how something is implemented.
> > Actually, I agree with this statement. "thinks that ... would benefit" is a really low bar.
> 
> Fair! What about (changes are in bold): 
> 
> > If a developer expresses **concerns that would have prevented from landing the patch in a pre-commit review**(including around the need for more design discussions) and ask for a revert, the patch should reverted promptly ..."
> 
> This addresses mainly that we shouldn't treat differently a review posted 10 min before pushing or 10 min after (or a post-commit review): revert fast is the default.
> 
> 
> > We really want to avoid people silently reverting commits because they don't like how something is implemented.
> 
> (the same can be said of pre-commit review about blocking a revision because "they don't like how something is implemented", seems like something to be addressed uniformly)
> 
> This is in the parenthesis actually: `(preferably by the original author, unless they are unresponsive)`. In practice people are reasonable about this I think, but we can tweak it further, what about changing the end to say:
> 
> > If a developer expresses concerns that would have prevented from landing the patch in a pre-commit review (including around the need for more design discussions), they may **ask for a revert to the original author who is responsible to revert the patch promptly**. 
> 
> We can also add an expectation of active engagement from the reviewer in paragraph that follows, by slightly changing it like this:
> 
> > Before being recommitted, the patch generally undergoes further review.
> > **The community member who identified the problem is expected to engage
> > actively in the review**. In cases where the patch triggered a
> > hardware-specific buildbot failure, a community member with access to [...]
> 
> 
> > If a developer expresses **concerns that would have prevented from landing the patch in a pre-commit review**(including around the need for more design discussions) and ask for a revert, the patch should reverted promptly ..."
> 
> This addresses mainly that we shouldn't treat differently a review posted 10 min before pushing or 10 min after (or a post-commit review): revert fast is the default.

Agreed. Our lax policy of review and approvals does occasionally lead to soft approvals and lack of diverse reviews, which often comes after commit when the patch lands on people's CI or development trees.

We can't have it both ways. We either have a strong pre-commit validation, or we have a reasonable post-commit policy. Reverting a patch is *not* a reflection on the code quality or the author's competence, just the need for more reviews.

The longer we take to revert the patch, the harder it is and the patch can always be re-applied later.

> (the same can be said of pre-commit review about blocking a revision because "they don't like how something is implemented", seems like something to be addressed uniformly)

Good point.

> > If a developer expresses concerns that would have prevented from landing the patch in a pre-commit review (including around the need for more design discussions), they may **ask for a revert to the original author who is responsible to revert the patch promptly**. 

Right! This is a burden on people that want to contribute, not on everybody else working on their own parts of the project. It's a similar burden we put on new projects, back-ends, front-ends, otherwise it does not scale.

> We can also add an expectation of active engagement from the reviewer in paragraph that follows, by slightly changing it like this:
> 
> > Before being recommitted, the patch generally undergoes further review.
> > **The community member who identified the problem is expected to engage
> > actively in the review**. In cases where the patch triggered a
> > hardware-specific buildbot failure, a community member with access to [...]

SGTM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89995