| Message ID | 1416341031-6204-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 2014-11-18 at 20:03 +0000, Julien Grall wrote: > By default, the script get_maintainer.pl will remove duplicates email as soon > as it appends the list of maintainers of a new file, and therefore override > the role of the developper. > > On complex patch (see [1]), this will result to ommitting randomly some > maintainers. > > This could be fixed Are you proposing an alternative/better fix here? or describing what this patch does? > by not removing the duplicate email in the list. Once the > list is created, when it's necessary, the script will drop the "REST" people > and remove duplicata. > > Example: > > Patch: https://patches.linaro.org/41083/ > > Before: > > Daniel De Graaf <dgdegra@tycho.nsa.gov> > Ian Jackson <ian.jackson@eu.citrix.com> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Ian Campbell <ian.campbell@citrix.com> > Wei Liu <wei.liu2@citrix.com> > George Dunlap <george.dunlap@eu.citrix.com> > xen-devel@lists.xen.org > > After: > > Daniel De Graaf <dgdegra@tycho.nsa.gov> > Ian Jackson <ian.jackson@eu.citrix.com> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Ian Campbell <ian.campbell@citrix.com> > Wei Liu <wei.liu2@citrix.com> > Stefano Stabellini <stefano.stabellini@citrix.com> > Tim Deegan <tim@xen.org> > Keir Fraser <keir@xen.org> > Jan Beulich <jbeulich@suse.com> > George Dunlap <george.dunlap@eu.citrix.com> > xen-devel@lists.xen.org > > [1] http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg00060.html > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > CC: Don Slutz <dslutz@verizon.com> > > --- > I would like to see this patch in Xen 4.5 and backported to Xen 4.4 (first > time the script has been introduced). > > Developpers using this script won't ommitted to cc some maintainers, and it > will avoid maintainers complaining about miss CC. > > The only drawbacks I can see is there is too much people CCed (the > patch d67738db was intended to avoid CCing Keir too often). My tree doesn't have in it d67738db but from the example you give above it seems like this patch will regress that? As someone who already gets too much mail and is listed in THE REST these days I am very much in favour of not mailing THE REST when other maintainers have been found. > Also, if the maintainers is referenced twice in the file MAINTAINERS with > different email, the script won't notice it's duplicated and list 2 times. > Though, for this one it could be fixed by modifying the MAINTAINERS file. > Is it worth for Xen 4.5? For know, it seems to only happen with Stefano. That's fine IMHO. The script shouldn't be expected to be smart enough to reconcile two distinct strings which happen to refer to the same person into a single string. If someone cares they should patch MAINTAINERS to refer to themselves in a consistent way. > --- > scripts/get_maintainer.pl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl > index df920e2..cc445cd 100755 > --- a/scripts/get_maintainer.pl > +++ b/scripts/get_maintainer.pl > @@ -35,7 +35,7 @@ my $email_git_min_percent = 5; > my $email_git_since = "1-year-ago"; > my $email_hg_since = "-365"; > my $interactive = 0; > -my $email_remove_duplicates = 1; > +my $email_remove_duplicates = 0; > my $email_use_mailmap = 1; > my $email_drop_the_rest_supporter_if_supporter_found = 1; > my $output_multiline = 1;
Hi Ian, On 11/20/2014 04:08 PM, Ian Campbell wrote: > On Tue, 2014-11-18 at 20:03 +0000, Julien Grall wrote: >> By default, the script get_maintainer.pl will remove duplicates email as soon >> as it appends the list of maintainers of a new file, and therefore override >> the role of the developper. >> >> On complex patch (see [1]), this will result to ommitting randomly some >> maintainers. >> >> This could be fixed > > Are you proposing an alternative/better fix here? or describing what > this patch does? Describing what the patch does. >> by not removing the duplicate email in the list. Once the >> list is created, when it's necessary, the script will drop the "REST" people >> and remove duplicata. >> >> Example: >> >> Patch: https://patches.linaro.org/41083/ >> >> Before: >> >> Daniel De Graaf <dgdegra@tycho.nsa.gov> >> Ian Jackson <ian.jackson@eu.citrix.com> >> Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Ian Campbell <ian.campbell@citrix.com> >> Wei Liu <wei.liu2@citrix.com> >> George Dunlap <george.dunlap@eu.citrix.com> >> xen-devel@lists.xen.org >> >> After: >> >> Daniel De Graaf <dgdegra@tycho.nsa.gov> >> Ian Jackson <ian.jackson@eu.citrix.com> >> Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Ian Campbell <ian.campbell@citrix.com> >> Wei Liu <wei.liu2@citrix.com> >> Stefano Stabellini <stefano.stabellini@citrix.com> >> Tim Deegan <tim@xen.org> >> Keir Fraser <keir@xen.org> >> Jan Beulich <jbeulich@suse.com> >> George Dunlap <george.dunlap@eu.citrix.com> >> xen-devel@lists.xen.org >> >> [1] http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg00060.html >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> CC: Don Slutz <dslutz@verizon.com> >> >> --- >> I would like to see this patch in Xen 4.5 and backported to Xen 4.4 (first >> time the script has been introduced). >> >> Developpers using this script won't ommitted to cc some maintainers, and it >> will avoid maintainers complaining about miss CC. >> >> The only drawbacks I can see is there is too much people CCed (the >> patch d67738db was intended to avoid CCing Keir too often). > > My tree doesn't have in it d67738db but from the example you give above > it seems like this patch will regress that? As someone who already gets > too much mail and is listed in THE REST these days I am very much in > favour of not mailing THE REST when other maintainers have been found. It's still the case with this patch. Before if a maintainer was both in x86 section and "THE REST". It may end up to completely drop the maintainer in the CC list. By drawbacks I meant, if there is another bug in the script then we may end up to cc too many people. Honestly I don't believe it's the case. >> Also, if the maintainers is referenced twice in the file MAINTAINERS with >> different email, the script won't notice it's duplicated and list 2 times. >> Though, for this one it could be fixed by modifying the MAINTAINERS file. >> Is it worth for Xen 4.5? For know, it seems to only happen with Stefano. > Regards.
On 11/20/2014 04:15 PM, Julien Grall wrote: > Hi Ian, > > On 11/20/2014 04:08 PM, Ian Campbell wrote: >> On Tue, 2014-11-18 at 20:03 +0000, Julien Grall wrote: >>> By default, the script get_maintainer.pl will remove duplicates email as soon >>> as it appends the list of maintainers of a new file, and therefore override >>> the role of the developper. >>> >>> On complex patch (see [1]), this will result to ommitting randomly some >>> maintainers. >>> >>> This could be fixed >> >> Are you proposing an alternative/better fix here? or describing what >> this patch does? > > Describing what the patch does. > >>> by not removing the duplicate email in the list. Once the >>> list is created, when it's necessary, the script will drop the "REST" people >>> and remove duplicata. >>> >>> Example: >>> >>> Patch: https://patches.linaro.org/41083/ >>> >>> Before: >>> >>> Daniel De Graaf <dgdegra@tycho.nsa.gov> >>> Ian Jackson <ian.jackson@eu.citrix.com> >>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> Ian Campbell <ian.campbell@citrix.com> >>> Wei Liu <wei.liu2@citrix.com> >>> George Dunlap <george.dunlap@eu.citrix.com> >>> xen-devel@lists.xen.org >>> >>> After: >>> >>> Daniel De Graaf <dgdegra@tycho.nsa.gov> >>> Ian Jackson <ian.jackson@eu.citrix.com> >>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> Ian Campbell <ian.campbell@citrix.com> >>> Wei Liu <wei.liu2@citrix.com> >>> Stefano Stabellini <stefano.stabellini@citrix.com> >>> Tim Deegan <tim@xen.org> >>> Keir Fraser <keir@xen.org> >>> Jan Beulich <jbeulich@suse.com> >>> George Dunlap <george.dunlap@eu.citrix.com> >>> xen-devel@lists.xen.org >>> >>> [1] http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg00060.html >>> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>> CC: Don Slutz <dslutz@verizon.com> >>> >>> --- >>> I would like to see this patch in Xen 4.5 and backported to Xen 4.4 (first >>> time the script has been introduced). >>> >>> Developpers using this script won't ommitted to cc some maintainers, and it >>> will avoid maintainers complaining about miss CC. >>> >>> The only drawbacks I can see is there is too much people CCed (the >>> patch d67738db was intended to avoid CCing Keir too often). >> >> My tree doesn't have in it d67738db but from the example you give above >> it seems like this patch will regress that? As someone who already gets >> too much mail and is listed in THE REST these days I am very much in >> favour of not mailing THE REST when other maintainers have been found. Forgot to add, the example above show the difference without and with the patch. The list is correct because both ARM and x86 maintainers should be CC. Because of this all "THE REST" maintainers are added.
On Thu, 2014-11-20 at 16:21 +0000, Julien Grall wrote: > On 11/20/2014 04:15 PM, Julien Grall wrote: > > Hi Ian, > > > > On 11/20/2014 04:08 PM, Ian Campbell wrote: > >> On Tue, 2014-11-18 at 20:03 +0000, Julien Grall wrote: > >>> By default, the script get_maintainer.pl will remove duplicates email as soon > >>> as it appends the list of maintainers of a new file, and therefore override > >>> the role of the developper. > >>> > >>> On complex patch (see [1]), this will result to ommitting randomly some > >>> maintainers. > >>> > >>> This could be fixed > >> > >> Are you proposing an alternative/better fix here? or describing what > >> this patch does? > > > > Describing what the patch does. > > > >>> by not removing the duplicate email in the list. Once the > >>> list is created, when it's necessary, the script will drop the "REST" people > >>> and remove duplicata. > >>> > >>> Example: > >>> > >>> Patch: https://patches.linaro.org/41083/ > >>> > >>> Before: > >>> > >>> Daniel De Graaf <dgdegra@tycho.nsa.gov> > >>> Ian Jackson <ian.jackson@eu.citrix.com> > >>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >>> Ian Campbell <ian.campbell@citrix.com> > >>> Wei Liu <wei.liu2@citrix.com> > >>> George Dunlap <george.dunlap@eu.citrix.com> > >>> xen-devel@lists.xen.org > >>> > >>> After: > >>> > >>> Daniel De Graaf <dgdegra@tycho.nsa.gov> > >>> Ian Jackson <ian.jackson@eu.citrix.com> > >>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >>> Ian Campbell <ian.campbell@citrix.com> > >>> Wei Liu <wei.liu2@citrix.com> > >>> Stefano Stabellini <stefano.stabellini@citrix.com> > >>> Tim Deegan <tim@xen.org> > >>> Keir Fraser <keir@xen.org> > >>> Jan Beulich <jbeulich@suse.com> > >>> George Dunlap <george.dunlap@eu.citrix.com> > >>> xen-devel@lists.xen.org > >>> > >>> [1] http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg00060.html > >>> > >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >>> CC: Don Slutz <dslutz@verizon.com> > >>> > >>> --- > >>> I would like to see this patch in Xen 4.5 and backported to Xen 4.4 (first > >>> time the script has been introduced). > >>> > >>> Developpers using this script won't ommitted to cc some maintainers, and it > >>> will avoid maintainers complaining about miss CC. > >>> > >>> The only drawbacks I can see is there is too much people CCed (the > >>> patch d67738db was intended to avoid CCing Keir too often). > >> > >> My tree doesn't have in it d67738db but from the example you give above > >> it seems like this patch will regress that? As someone who already gets > >> too much mail and is listed in THE REST these days I am very much in > >> favour of not mailing THE REST when other maintainers have been found. > > Forgot to add, the example above show the difference without and with > the patch. The list is correct because both ARM and x86 maintainers > should be CC. Because of this all "THE REST" maintainers are added. Just to be clear, you mean that everyone under THE REST is added solely because they also happen to be maintainers of some other relevant bit of code, not that THE REST is explicitly added in this case, right? Ian.
On Thu, 2014-11-20 at 16:15 +0000, Julien Grall wrote: > Hi Ian, > > On 11/20/2014 04:08 PM, Ian Campbell wrote: > > On Tue, 2014-11-18 at 20:03 +0000, Julien Grall wrote: > >> By default, the script get_maintainer.pl will remove duplicates email as soon > >> as it appends the list of maintainers of a new file, and therefore override > >> the role of the developper. > >> > >> On complex patch (see [1]), this will result to ommitting randomly some > >> maintainers. > >> > >> This could be fixed > > > > Are you proposing an alternative/better fix here? or describing what > > this patch does? > > Describing what the patch does. Then I think you meant "This is fixed" or "This patches fixes this by ...". > By drawbacks I meant, if there is another bug in the script then we may > end up to cc too many people. Honestly I don't believe it's the case. Sure, lets assume not. Ian
On 11/20/2014 04:29 PM, Ian Campbell wrote: >> Forgot to add, the example above show the difference without and with >> the patch. The list is correct because both ARM and x86 maintainers >> should be CC. Because of this all "THE REST" maintainers are added. > > Just to be clear, you mean that everyone under THE REST is added solely > because they also happen to be maintainers of some other relevant bit of > code, not that THE REST is explicitly added in this case, right? Yes, my description was confusing. With setting $email_remove_duplicates to 0, the script will: 1) Append the list of maintainers for every file 2) Filter the list to remove the entry with "THE REST" role 3) Remove duplicated address The previous behavior was: 1) Get the list of maintainers of the file (incidentally all the maintainers in "THE REST" role are added). If the email address already exists in the global list, skip it. 2) Filter the list to remove the entry with "THE REST" role So if a maintainers is marked on the "THE REST" on the first file and actually be an x86 maintainers on the second file, the scripts will only retain the "THE REST" role. If it's more clear, I can add the explanation above in the commit message. Regards,
On Thu, 2014-11-20 at 16:43 +0000, Julien Grall wrote: > On 11/20/2014 04:29 PM, Ian Campbell wrote: > >> Forgot to add, the example above show the difference without and with > >> the patch. The list is correct because both ARM and x86 maintainers > >> should be CC. Because of this all "THE REST" maintainers are added. > > > > Just to be clear, you mean that everyone under THE REST is added solely > > because they also happen to be maintainers of some other relevant bit of > > code, not that THE REST is explicitly added in this case, right? > > Yes, my description was confusing. With setting $email_remove_duplicates > to 0, the script will: > 1) Append the list of maintainers for every file > 2) Filter the list to remove the entry with "THE REST" role > 3) Remove duplicated address > > The previous behavior was: > 1) Get the list of maintainers of the file (incidentally all the > maintainers in "THE REST" role are added). If the email address already > exists in the global list, skip it. > 2) Filter the list to remove the entry with "THE REST" role > > So if a maintainers is marked on the "THE REST" on the first file and > actually be an x86 maintainers on the second file, the scripts will only > retain the "THE REST" role. > > If it's more clear, I can add the explanation above in the commit message. It is, please do. Ian.
On Thu, 2014-11-20 at 16:52 +0000, Ian Campbell wrote: > On Thu, 2014-11-20 at 16:43 +0000, Julien Grall wrote: > > On 11/20/2014 04:29 PM, Ian Campbell wrote: > > >> Forgot to add, the example above show the difference without and with > > >> the patch. The list is correct because both ARM and x86 maintainers > > >> should be CC. Because of this all "THE REST" maintainers are added. > > > > > > Just to be clear, you mean that everyone under THE REST is added solely > > > because they also happen to be maintainers of some other relevant bit of > > > code, not that THE REST is explicitly added in this case, right? > > Just a small clarification... > > Yes, my description was confusing. With setting $email_remove_duplicates > > to 0, the script will: > > 1) Append the list of maintainers for every file At this point each maintainer remains associated with the role/reason they are added, right? > > 2) Filter the list to remove the entry with "THE REST" role And this only happens if there are roles other than "THE REST" in the list? > > 3) Remove duplicated address > > > > The previous behavior was: > > 1) Get the list of maintainers of the file (incidentally all the > > maintainers in "THE REST" role are added). If the email address already > > exists in the global list, skip it. > > 2) Filter the list to remove the entry with "THE REST" role Whereas here the link from maintainer to the role is lost, hence everyone in THE REST is blindly removed? > > So if a maintainers is marked on the "THE REST" on the first file and > > actually be an x86 maintainers on the second file, the scripts will only > > retain the "THE REST" role. > > > > If it's more clear, I can add the explanation above in the commit message. > > It is, please do. > > Ian. > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 11/20/2014 04:57 PM, Ian Campbell wrote: > On Thu, 2014-11-20 at 16:52 +0000, Ian Campbell wrote: >> On Thu, 2014-11-20 at 16:43 +0000, Julien Grall wrote: >>> On 11/20/2014 04:29 PM, Ian Campbell wrote: >>>>> Forgot to add, the example above show the difference without and with >>>>> the patch. The list is correct because both ARM and x86 maintainers >>>>> should be CC. Because of this all "THE REST" maintainers are added. >>>> >>>> Just to be clear, you mean that everyone under THE REST is added solely >>>> because they also happen to be maintainers of some other relevant bit of >>>> code, not that THE REST is explicitly added in this case, right? >>> > > Just a small clarification... > >>> Yes, my description was confusing. With setting $email_remove_duplicates >>> to 0, the script will: >>> 1) Append the list of maintainers for every file > > At this point each maintainer remains associated with the role/reason > they are added, right? Yes. Every time the maintainers is listed we add in the list no matters if the mail already exists. So the maintainers may be listed twice with different roles (for instance: x86 and "THE REST"). >>> 2) Filter the list to remove the entry with "THE REST" role > > And this only happens if there are roles other than "THE REST" in the > list? Yes. > >>> 3) Remove duplicated address >>> >>> The previous behavior was: >>> 1) Get the list of maintainers of the file (incidentally all the >>> maintainers in "THE REST" role are added). If the email address already >>> exists in the global list, skip it. >>> 2) Filter the list to remove the entry with "THE REST" role > > Whereas here the link from maintainer to the role is lost, hence > everyone in THE REST is blindly removed? Yes. >>> So if a maintainers is marked on the "THE REST" on the first file and >>> actually be an x86 maintainers on the second file, the scripts will only >>> retain the "THE REST" role. >>> >>> If it's more clear, I can add the explanation above in the commit message. >> >> It is, please do. I will send a new version. Regards,
On 11/20/2014 04:30 PM, Ian Campbell wrote: > On Thu, 2014-11-20 at 16:15 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 11/20/2014 04:08 PM, Ian Campbell wrote: >>> On Tue, 2014-11-18 at 20:03 +0000, Julien Grall wrote: >>>> By default, the script get_maintainer.pl will remove duplicates email as soon >>>> as it appends the list of maintainers of a new file, and therefore override >>>> the role of the developper. >>>> >>>> On complex patch (see [1]), this will result to ommitting randomly some >>>> maintainers. >>>> >>>> This could be fixed >>> >>> Are you proposing an alternative/better fix here? or describing what >>> this patch does? >> >> Describing what the patch does. > > Then I think you meant "This is fixed" or "This patches fixes this > by ...". Ok. I will fix it. > >> By drawbacks I meant, if there is another bug in the script then we may >> end up to cc too many people. Honestly I don't believe it's the case. > > Sure, lets assume not. I will drop this drawbacks. Regards,
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl index df920e2..cc445cd 100755 --- a/scripts/get_maintainer.pl +++ b/scripts/get_maintainer.pl @@ -35,7 +35,7 @@ my $email_git_min_percent = 5; my $email_git_since = "1-year-ago"; my $email_hg_since = "-365"; my $interactive = 0; -my $email_remove_duplicates = 1; +my $email_remove_duplicates = 0; my $email_use_mailmap = 1; my $email_drop_the_rest_supporter_if_supporter_found = 1; my $output_multiline = 1;
By default, the script get_maintainer.pl will remove duplicates email as soon as it appends the list of maintainers of a new file, and therefore override the role of the developper. On complex patch (see [1]), this will result to ommitting randomly some maintainers. This could be fixed by not removing the duplicate email in the list. Once the list is created, when it's necessary, the script will drop the "REST" people and remove duplicata. Example: Patch: https://patches.linaro.org/41083/ Before: Daniel De Graaf <dgdegra@tycho.nsa.gov> Ian Jackson <ian.jackson@eu.citrix.com> Stefano Stabellini <stefano.stabellini@eu.citrix.com> Ian Campbell <ian.campbell@citrix.com> Wei Liu <wei.liu2@citrix.com> George Dunlap <george.dunlap@eu.citrix.com> xen-devel@lists.xen.org After: Daniel De Graaf <dgdegra@tycho.nsa.gov> Ian Jackson <ian.jackson@eu.citrix.com> Stefano Stabellini <stefano.stabellini@eu.citrix.com> Ian Campbell <ian.campbell@citrix.com> Wei Liu <wei.liu2@citrix.com> Stefano Stabellini <stefano.stabellini@citrix.com> Tim Deegan <tim@xen.org> Keir Fraser <keir@xen.org> Jan Beulich <jbeulich@suse.com> George Dunlap <george.dunlap@eu.citrix.com> xen-devel@lists.xen.org [1] http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg00060.html Signed-off-by: Julien Grall <julien.grall@linaro.org> CC: Don Slutz <dslutz@verizon.com> --- I would like to see this patch in Xen 4.5 and backported to Xen 4.4 (first time the script has been introduced). Developpers using this script won't ommitted to cc some maintainers, and it will avoid maintainers complaining about miss CC. The only drawbacks I can see is there is too much people CCed (the patch d67738db was intended to avoid CCing Keir too often). Also, if the maintainers is referenced twice in the file MAINTAINERS with different email, the script won't notice it's duplicated and list 2 times. Though, for this one it could be fixed by modifying the MAINTAINERS file. Is it worth for Xen 4.5? For know, it seems to only happen with Stefano. --- scripts/get_maintainer.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)