Do we want these lines in the commit description, or just in the commit email?
On Wed, Jan 28, 2009 at 10:02 PM, Erik Kay <erik...@chromium.org> wrote:
>
> This feels like a tools issue to me. In Mondrian, the review has an
> overall score on it (or at least it did the last time I used it). If
> a reviewer replied without a LGTM, then it subtracted 1. A reply with
> a LGTM added 1. It didn't allow you to commit the change until you
> had at least a +1, so with multiple reviewers asking for changes, it
> was clear whether you had something still pending or not. I'm not
> convinced that this precise mechanism is what we'd want, but perhaps
> something related would help.
>
> Erik
>
>
> On Tue, Jan 27, 2009 at 10:04 PM, Brett Wilson <bre...@chromium.org> wrote:
>>
>> There are a lot of patches lately with a lot of reviewers on them,
>> especially related to porting since a lot of people might need to be
>> in the loop for some changes.
>>
>> The problem is that there's no clear responsibility given in these
>> reviews. If I'm the sole reviewer on a change, I know I have to do a
>> good job. When there are three other people, I sometimes assume that
>> somebody else must have looked carefully at some part of the review.
>> I'm sure sometimes all the reviewers think this and the change isn't
>> reviewed properly.
>>
>> In other cases, some reviewers say LGTM for a patch, while others are
>> still expecting changes. The author can get confused as to the status
>> of the review, and I also know some patches have been checked in
>> lately where at least one reviewer expected further changes before
>> checkin.
>>
>> At the same time, we want to encourage many people to participate in
>> the review process and keep tabs on what's going on. I propose several
>> changes to best practices:
>>
>> 1. When a patch author requests more than one reviewer, they should
>> make clear in the review request email what they expect the
>> responsibility of each reviewer to be. Example:
>> agl: bitmap changes
>> evanm: process hacks
>> everybody else: FYI
>> In this case, I might be on the review list because I've asked to be
>> in the loop for multiprocess changes, but I wouldn't be the primary
>> reviewer and the author and other reviewers wouldn't be expecting me
>> to review all the diffs in detail.
>>
>> 2. If you get a review that includes many other people, and the author
>> didn't do (1), please ask them what part you're responsible for if you
>> don't want to review the whole thing in detail.
>>
>> 3. The author should wait for approval from everybody on the reviewer
>> list before checking in.
>>
>> 4. People who are on a review without clear review responsibility
>> should be super responsive and not hold up the review. The patch
>> author should feel free to ping them mercilessly if they are.
>>
>> 5. If you're on "FYI" person on a review and you didn't actually
>> review in detail (or at all), but don't have a problem with the patch,
>> note this. You could say something like "rubber stamp" or "ACK"
>> instead of "LGTM." This way the real reviewers know not to trust that
>> you did their work for them, but the author of the patch knows they
>> don't have to wait for further feedback from you.
>>
>> Hopefully we can still keep everybody in the loop but have clear
>> ownership and detailed reviews. It might even speed up some patches
>> since I can quickly ACK patches I don't care about, and the patch
>> author knows they don't have to wait for feedback from me. Or do you
>> think this has too much overhead?
>>
>> Comments?
>>
>> Brett
>>
>> >
>>
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~----------~----~----~----~------~----~------~--~---
