Bug 107528

Summary: webkit-patch suggest-reviewers should limit itself to 5 reviewers
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107525    
Attachments:
Description Flags
Preliminary patch
none
Patch eric: review+

Description Eric Seidel (no email) 2013-01-22 02:00:00 PST
webkit-patch suggest-reviewers should limit itself to 5 reviewers

Right now it will CC an unbounded number, which is bad.

Those should also be the 5 most relevant reviewers, ideally.  Easy to calculate based on number of annotated lines in the diff attributed to each reviewer.
Comment 1 Zan Dobersek 2013-01-23 06:37:55 PST
(In reply to comment #0)
> Those should also be the 5 most relevant reviewers, ideally.  Easy to calculate based on number of annotated lines in the diff attributed to each reviewer.

How about deciding about relevant reviewers by picking five who most recently reviewed patches around the code? This kind of guarantees that the offered reviewers is up-to-date with the current status of the code that's changed.

I think that's also easier to implement than sorting by the sum of reviewed lines.
Comment 2 Zan Dobersek 2013-01-23 07:42:37 PST
Created attachment 184231 [details]
Preliminary patch
Comment 3 Zan Dobersek 2013-01-23 07:46:55 PST
(In reply to comment #2)
> Created an attachment (id=184231) [details]
> Preliminary patch

A WIP patch that outlines the 'five-latest-reviewers' approach. No unit test breakage. I believe this keeps the current behavior though it's probably not in line with what bug #107525 would like to accomplish.
Comment 4 Eric Seidel (no email) 2013-01-23 10:59:13 PST
Comment on attachment 184231 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184231&action=review

> Tools/Scripts/webkitpy/common/checkout/checkout.py:146
> +        reviewers = []
> +        for info in commit_infos:
> +            if info.reviewer() and info.reviewer() not in reviewers:
> +                reviewers.append(info.reviewer())
> +            if info.author() and info.author().can_review and info.author() not in reviewers:
> +                reviewers.append(info.author())
> +        return reviewers

It seems all you did here was remove the list comprehensions?
Comment 5 Zan Dobersek 2013-01-23 12:01:10 PST
Comment on attachment 184231 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184231&action=review

>> Tools/Scripts/webkitpy/common/checkout/checkout.py:146
>> +        return reviewers
> 
> It seems all you did here was remove the list comprehensions?

This ugly piece of code actually just tries to use a list as an ordered set. The returned list contains all the reviewers connected to the changed files but lists them in order of their last activity, from most to least recent.
Comment 6 Zan Dobersek 2013-01-24 02:15:15 PST
Created attachment 184447 [details]
Patch
Comment 7 Eric Seidel (no email) 2013-02-15 12:38:37 PST
Comment on attachment 184447 [details]
Patch

OK.
Comment 8 Zan Dobersek 2013-02-15 12:47:33 PST
Committed r143033: <http://trac.webkit.org/changeset/143033>