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

[Itamar Heim]

On 03/11/2014 10:08 PM, Maor Lipchuk wrote:
> On 03/11/2014 05:20 PM, Itamar Heim wrote:
>> 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)
> I think it could be done automatically by analysing the file and see who
> mostly changed it recently, since the "owner" of the file might be
> dynamic, who ever changed most of it few days ago might be more familiar
> with it today
>
> IMO the algorithm of adding the reviewers should be flexible.
> For example, using a folder which will contain files, where each file
> implement an algorithm to add the reviewers.
>
> for instance we can have two files:
> 1. Add a reviewers by blame - the contributor which changed recently the
> code lines
> 2. Add a reviewers by file - the contributor who changed most of the
> file recently.
>
> Each file will implement the functional operation and will output the
> reviewers emails.
>
> The user can then add a new algorithm or change it to be more specific
> to its project.
> for example the user can add also the maintainers which acked the patch
> that was blamed.
>> _______________________________________________
>> Users mailing list
>> Users at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/users
>

this shouldn't be automatic. we need to clearly define ownership. we 
can't do this per repo for the engine/vdsm. we can do this per repo for 
the other repo's probably (though solving the folder/file approach would 
cover the simpler repos as a private case).

yes, it will require some work, maybe some moving around of files to 
make this easier by folders (topics) which should be relevant anyway.