llvm dev - Mar 2016 - Formalize "revert for more design review" policy.

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

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

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

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>

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

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.
>
I think we should assume good faith, which makes this a non-issue.

>
> 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).
>
I have thought a bit about this, and I think it is sufficiently nuanced to
probably be worth a separate discussion.

-- Sean Silva

>
>
>>
>>  -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/c6a052e9/attachment.html>

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>