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

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Oct 24 16:11:59 PDT 2020


mehdi_amini added a comment.

In D89995#2352053 <https://reviews.llvm.org/D89995#2352053>, @nhaehnle wrote:

> Considering that you are doing this in response to D83088 <https://reviews.llvm.org/D83088>, I think we have pre-existing evidence that this isn't workable as-is.

You are refusing to adjust to the existing practices and revert: this is hardly an evidence of anything to me at the moment.

> In particular, if they have previously demonstrated that they cannot be relied on for this, then the rule should not apply.

I don't know what this means or how there is anything practical about this, feel free to suggest more rules around this though through a new RFC.
I'm not trying to propose anything new here: these practices seems clear to most 4 years ago, and we have been working this way for a long time now.



================
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
----------------
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 [...]




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89995



More information about the llvm-commits mailing list