On 05/09/2018 03:11 PM, John Ferlan wrote: > > > On 05/09/2018 04:57 AM, Daniel P. Berrangé wrote: >> On Wed, May 09, 2018 at 10:46:19AM +0200, Jiri Denemark wrote: >>> On Tue, May 08, 2018 at 14:07:31 +0100, Daniel P. Berrangé wrote: >>>> Currently all patches are simply sent to the main libvirt development >>>> mailing list. Sometimes individual developers are also CC'd but this is >>>> typically the exception. >>>> >>>> Libvirt does not follow a subsystem maintainer model, so there is no >>>> notion of owners for the different areas of code, but there certainly >>>> are people with high levels of expertize in specific areas. >>>> >>>> This patch thus proposes pulling in QEMU's get_maintainer.pl script and >>>> creating MAINTAINERS file to list who the experts are for specific >>>> areas. Combined with git-pubish, this will help ensure that patches are >>>> brought to the attention of people with direct expertize. >>>> >>>> For example: >>>> >>>> $ git show b04629b62934caa8786e73c3db985672422fc662 | \ >>>> ./build-aux/get_maintainer.pl >>>> Jim Fehlig <jfehlig@xxxxxxxx> (maintainer:libxl) >>>> libvir-list@xxxxxxxxxx (open list:All patches) >>>> >>>> Or >>>> >>>> $ git show e7cb9c4e230c3c77e35e72334c261b5b0a2211c6 | \ >>>> ./build-aux/get_maintainer.pl >>>> Jiri Denemar <jdenemar@xxxxxxxxxx> (maintainer:CPU modelling) >>> >>> s/Denemar/Denemark/g :-) >>> >>>> libvir-list@xxxxxxxxxx (open list:All patches) >>>> >>>> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> >>> >>> Anyway, I don't think we have such a high volume of patches that >>> reviewers can't watch incoming mails for patches that touch an area they >>> care about or are interested in. In other words, I don't mind this if >>> it's optional, but I don't see a real benefit. If someone doesn't have >>> time to review a particular patch, CCing them won't help anyway. And I'm >>> afraid it could even cause others to not look at the patch since they >>> would think the "maintainer" has to handle it. >> >> Sure, I'm not claiming it magically makes time to review something, but we >> do have a pretty high volume of incoming patches to track, making it easy >> for things to get lost / missed. For people who work on libvirt only part >> time, tracking all mails is a non-negligible undertaking. > > IMO, the volume of patches recently seems to be split between 3 things - > libvirt proper, libvirt dbus, and "build related" with the latter two > really being busy. I've personally assumed that the latter two based on > their fairly quick review response time are "in good hands", I just mark > them that way as part of my libvir-list patch work flow processing. That > just leaves the review cycle time for libvirt specific patches. > >> >> The idea of CC'ing is that it alerts people to patches where they might have >> a direct interest. Even if they don't have time to review, it gives them an >> alert that someone is proposing something that might be relevant / impacting >> on them. >> > Perhaps, depends on your CC and libvir-list processing and filtering > rules... If that mail ends up in my "Inbox" instead of "libvir-list" > folder, then it's more time consuming for me as a regular reader to > handle that and I may not notice it "right away" either. This is a matter of e-mail filtering. For instance any patch that I'm CCed on will end up in a separate folder different to INBOX (which also depends on your work flow: some like to keep inbox empty, whereas I "only" read e-mails and don't move them). > > Then there are those that don't like being CC'd - something you learn > the more time you spend following the list. Unfortunately there are > those that use CC just because someone has reviewed their patch > previously or responded to a query - even though the next series or > question has nothing to do with the previous. Again, I'd say this is matter of proper filter set up. > > Another downside to MAINTAINERS is maintaining it. Even more painful if > someone wants to change their email or something similar. Those that > follow this list, do reviews, and have commit access have a general idea > of where expertise lies. If I'm reviewing something in CPU or Migration > and am not sure, I will either ask or wait for Jirka to have a looksee > as well. Well, that's only part of the problem. For instance, I've written QoS and I'd like to review any patch against src/util/virnetdevbandwidth.c (just read the comment in virNetDevBandwidthSet and you'll see how complicated this feature is). And quite frankly, if I need to catch up with the upstream list (e.g. after some vacation), I merely just skim over patches, and if any of them has a reply I skip the whole thread. That is, I don't read others reviews. But if I were CC'ed, I'd get another e-mail that would end up in different folder (with much lower traffic) and therefore it would get my attention. Sure, it's matter of fine tuning - if there would be somebody CCed on every patch than this does not make things any better. I view this patch of Dan's as a way for us, "owners" of some features to be notified when somebody tries to patch "our" code. > > While it's far from perfect, the current model seems to for the most > part work. If a patch is missed, someone interested will ping on it. In > the end, it's generally more of a "time" problem for those of us > developing and reviewing/pushing. Well, long gone are times when Eric was around and not only read every e-mail on the list but also compared patches sent to the list with those actually pushed. I honestly say, if any patch has a review (unless it has subject that interests me) I skip the whole patch set and mark it as read. tl;dr I like this patch. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
