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