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

[David Blaikie via llvm-dev]

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.

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))


>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/b814e38c/attachment-0001.html>