2:51 a.m.
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(a)suse.com> (maintainer:libxl)
>>> libvir-list(a)redhat.com (open list:All patches)
>>>
>>> Or
>>>
>>> $ git show e7cb9c4e230c3c77e35e72334c261b5b0a2211c6 | \
>>> ./build-aux/get_maintainer.pl
>>> Jiri Denemar <jdenemar(a)redhat.com> (maintainer:CPU modelling)
>>
>> s/Denemar/Denemark/g :-)
>>
>>> libvir-list(a)redhat.com (open list:All patches)
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
>>
>> 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