[llvm-dev] Formalize "revert for more design review" policy.

[Hal Finkel via llvm-dev]

----- Original Message -----

> From: "David Blaikie" <dblaikie at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Sean Silva" <chisophugis at gmail.com>, "Chris Lattner"
> <clattner at apple.com>, "Rafael Ávila de Espíndola"
> <rafael.espindola at gmail.com>, "Michael Spencer"
> <bigcheesegs at gmail.com>, "Chandler Carruth" <chandlerc at gmail.com>,
> "David Li" <davidxl at google.com>, "Daniel Berlin"
> <dberlin at dberlin.org>, "Easwaran Raman" <eraman at google.com>,
> "llvm-dev" <llvm-dev at lists.llvm.org>
> Sent: Tuesday, March 8, 2016 11:22:11 PM
> Subject: Re: Formalize "revert for more design review" policy.

> On Tue, Mar 8, 2016 at 9:13 PM, Hal Finkel < hfinkel at anl.gov > wrote:

> > ----- Original Message -----
> 

> > > From: "Sean Silva" < chisophugis at gmail.com >
> 
> > > To: "llvm-dev" < llvm-dev at lists.llvm.org >
> 
> > > Cc: "Chris Lattner" < clattner at apple.com >, "Rafael Ávila de
> > > Espíndola" < rafael.espindola at gmail.com >, "Michael Spencer"
> 
> > > < bigcheesegs at gmail.com >, "Chandler Carruth" <
> > > chandlerc at gmail.com
> > > >, "David Li" < davidxl at google.com >, "David Blaikie"
> 
> > > < dblaikie at gmail.com >, "Daniel Berlin" < dberlin at dberlin.org >,
> > > "Easwaran Raman" < eraman at google.com >, "Hal Finkel"
> 
> > > < hfinkel at anl.gov >
> 
> > > Sent: Tuesday, March 8, 2016 11:03:09 PM
> 
> > > Subject: Re: Formalize "revert for more design review" policy.
> 
> > >
> 
> > >
> 
> > > +Hal, who somehow slipped through the cracks.
> 
> > >
> 

> > Thanks :-)
> 

> > >
> 
> > > On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <
> > > chisophugis at gmail.com
> > > >
> 
> > > wrote:
> 
> > >
> 
> > >
> 
> > >
> 
> > > Recently there's been some friction over reversions (I can
> > > remember
> 
> > > two cases in recent memory). In both issues the general feel I
> > > got
> 
> > > is that as a community we should honor "revert for more design
> 
> > > review" requests unconditionally.
> 
> > >
> 
> > >
> 
> > > What do you guys think of adding something like this to
> 
> > > DeveloperPolicy.rst as an item at the end of the numbered list in
> 
> > > http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?
> 
> > >
> 
> > >
> 
> > >
> 
> > > #. Sometimes patches get committed that need more discussion.
> 
> > > If a developer thinks that a patch would benefit from some more
> 
> > > review
> 
> > > and promptly communicates this, the patch should be reverted
> 
> > > (preferably
> 
> > > by the original 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.
> 
> > >
> 
> > >
> 
> > > "promptly" is there mostly to avoid suggesting a "necro-revert";
> > > once
> 
> > > the code has been in tree for long enough at some point it would
> > > be
> 
> > > more appropriate to open a bug report or start a fresh
> > > discussion.
> 
> > >
> 
> > >
> 
> > > "unresponsive" add some nebulousness, but I think it's an
> > > important
> 
> > > exception to call out for the "preferably by the original
> > > author".
> 
> > >
> 

> > I think this generally sounds right, and matches what we currently
> > expect in practice. To this we might add that it is then the
> > responsibility of the developer requesting the revert to ensure
> > that
> > the review happens promptly.
> 

> I think this is slightly problematic - it's not uncommon that we have
> new contributors send something for review, then without waiting
> very long (by our community standards - which is admittedly quite a
> while, but seen this in under a week, etc) and then commit. This
> usually results in a knee-jerk "please revert".

> Committing prematurely shouldn't be a way to cause a priority
> increase for a patch.

Completely agree. Although that does not fall into the "revert for more design review" category. 

> But, yeah, that doesn't necessarily address the sort of situations
> that instigated this thread either. Probably reasonable to expect
> someone to say /something/ in a review after a certain point, if
> they want to review it.

> (one piece of the review puzzle that may also not be formalized:
> generally I treat review approval as being "I would feel comfortable
> committing this without review" (ie: two people who wouldn't
> independently commit a patch don't become able to commit it by
> consensus - but maybe that's not a bad thing to allow, just not the
> way I've generally treated code review))
Interesting point. I think we generally expect someone with expertise and experience in a particular area to approve substantial patches. For smaller fixups or enhancements, that might not be necessary. We normally rely on the honor system and the comfort level of the reviewer to allow for self selection on expertise and experience, and I'm unsure if/how we should try to formalize that. 

Thanks again, 
Hal 

> > Do we have a formal policy for other kinds of reverts? We also
> > generally revert if someone claims to have bisected a miscompile to
> > a particular commit, and this is true even if a test case is not
> > immediately available (although we do need some kind of reproducer
> > before too long).
> 

> Yeah, this gets into interesting territory as well - if the problem
> is bad enough, we do generally accept reverting a patch without a
> reproducer being currently available (but, as you say, expecting it
> to come along shortly thereafter).

> > -Hal
> 

> > >
> 
> > >
> 
> > > -- Sean Silva
> 
> > >
> 

> > --
> 
> > Hal Finkel
> 
> > Assistant Computational Scientist
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/78e2aa72/attachment.html>