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

[Yedidyah Bar David]

----- Original Message -----
> From: "Itamar Heim" <iheim at redhat.com>
> To: "Eyal Edri" <eedri at redhat.com>
> Cc: users at ovirt.org, "Tomasz Kołek" <tomasz-kolek at o2.pl>, "infra" <infra at ovirt.org>
> Sent: Tuesday, March 11, 2014 5:20:29 PM
> Subject: Re: [Users] [GSOC][Gerrit] add potential reviewers - questions
> 
> On 03/11/2014 05:14 PM, 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?
> >>> _______________________________________________
> >>> Users mailing list
> >>> Users at ovirt.org
> >>> http://lists.ovirt.org/mailman/listinfo/users
> >>>
> >>
> >> 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?
> >
> 
> yes, but considering our project layout, i don't see an alternative.
> (some of the layout could be improved to be path based, rather than file
> based)

Extending Ewoud's idea, we can also check the list of people committing
(rather than reviewing) changes to each file in the change.
-- 
Didi