[Users] [GSOC][Gerrit] add potential reviewers - questions

[Ewoud Kohl van Wijngaarden]

On Tue, Mar 11, 2014 at 05:40:02PM +0100, David Caro wrote:
> On Tue 11 Mar 2014 04:14:26 PM CET, Eyal Edri wrote:
> >
> >
> > ----- Original Message -----
> >> From: "Itamar Heim" <iheim at redhat.com>
> >> To: "Eyal Edri" <eedri at redhat.com>, "Tomasz Kołek" <tomasz-kolek at o2.pl>, users at ovirt.org, "infra" <infra at ovirt.org>
> >> Sent: Tuesday, March 11, 2014 5:10:54 PM
> >> Subject: Re: [Users] [GSOC][Gerrit] add potential reviewers - questions
> >>
> >> On 03/11/2014 05:06 PM, Ewoud Kohl van Wijngaarden wrote:
> >>> On Tue, Mar 11, 2014 at 10:37:22AM -0400, Eyal Edri wrote:
> >>>>> Tomasz Kołek wrote:
> >>>>>
> >>>>> I've got a few questions about project description.
> >>>>> Please tell me if my problem's understanding is good or not.
> >>>>> We need to add a few flags/methods to git review module. This flags
> >>>>> should
> >>>>> allow to add potential reviewers in gerrit.
> >>>>> So:
> >>>>> Let's assume that we've got special flags for this operations. What's
> >>>>> next?
> >>>>> 1. In gerrit system we need to add special place for potential reviewers?
> >>>>> 2. Potential reviewers should agree that they want to review?
> >>>>> 3. We can have more than one accepted reviewer?
> >>>>
> >>>> I'm not sure i understood exactly what you mean by 'potential
> >>>> reviewers'.  do want gerrit (hook?) to automatically add reviewers to
> >>>> a patch according to the code sent?  so in fact you'll have a place
> >>>> somewhere for mapping code & specific developers?
> >>>
> >>> I really like this idea. Gerrit currently requires new users to know who
> >>> to add as reviewers, IMHO impeding new contributors.
> >>>
> >>> One relative simple solution would be to look at who recently touched
> >>> the files that are being modified and add them as reviewers. This can be
> >>> done by looking at the git log for a file. Some pseudo python code
> >>> solution:
> >>>
> >>> reviewers = set()
> >>>
> >>> for modified_file in commit.files:
> >>>      reviewers += set(commit.author for commit in git.log(modified_file))
> >>>
> >>> return reviewers
> >>>
> >>> This gives a system that those who touche a file, become the maintainer
> >>> for that file. A more complex solution could improve on that and limit
> >>> the reviewers added per patch. One can think of limiting to only
> >>> contributions in the last X months, weigh contributions so common
> >>> committers are prefered. It could also combine several methods.
> >>>
> >>> For example to limit to the 5 authors who touched the most files:
> >>>
> >>> reviewers = collections.Counter()  # New in python 2.7
> >>>
> >>> for modified_file in commit.files:
> >>>      reviewers += collections.Counter(commit.author for commit in
> >>>      git.log(modified_file))
> >>>
> >>> return [author for author, count in reviewers.most_common(5)]
> >>>
> >>> Since Counter also accepts a dictionary, one could also weigh the
> >>> touched lines per file. Downside there is big whitespace/formatting
> >>> patches can skew the line count.
> >>>
> >>> In short, I think an entire thesis could be written on the optimal way
> >>> to determine reviewers but a simple algorithm could do to show the
> >>> method works.
> >>>
> >>> Does this help?
> >>
> >> I think if we do this, we want to make sure we cover per file who is
> >> required to +2 it before we consider it acked.
> >>
> >
> > won't it require maintaining static lists of people per file/path/project?

Limiting to those with +2 / submit permissions per project should
suffice since we can trust them to add the required people if needed.
Further automation there could be a strech goal.

To limit the load on +2 maintainers, they could be added once the patch
received at least one +1.

> About dynamically adding reviewers, it should be fairly easy to do 
> using a small gerrit hook to run on patch-submitted. But gerrit will 
> fail if the user is not already created in gerrit so not really 
> allowing non-registered users to get in. But I'm not sure if it's 
> 'polite' to add someone as a reviewer if he did not previously agreed 
> to it, specially when emails will be sent to him.

Considering we already agree that doing it properly is very complex,
maybe it could be done through a Gerrit plugin? Still can't hurt to try
a hook for proof-of-concept. It could just log the reviewers it would
select for analysis instead of actually adding them.

Bonus points for making selecting reviewers pluggable so multiple
algorithms can be implemented.

We already have a whitelist of users who are allowed by jenkins. Maybe a
gerrit plugin can easily create an auto-add me as reviewer setting,
possibly with site wide configurable default to opt-in or opt-out.