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

[Easwaran Raman via llvm-dev]

On Tue, Mar 8, 2016 at 9:22 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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.
>
This is slightly OT to the subject of this thread, but +1 to "Probably
reasonable to expect someone to say /something/ in a review after a certain
point, if they want to review it.". Even a "I don't have the time to give
detailed review comments right now, but I have concerns about this patch"
puts the onus on the submitter to work with the reviewer to find out and
address the concerns.




> (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/20160309/682f57da/attachment.html>