Re: [Qemu-trivial] [Qemu-devel] [PATCH 18/47] MAINTAINERS: add missing TCG entry

[Cornelia Huck]

On Wed, 9 Aug 2017 18:30:25 -0300
Philippe Mathieu-Daudé <address@hidden> wrote:

> [I added mails of other person who reply to this series, this mail is 
> not directly addressed to Alex]

Hey, a maintainership discussion! I don't think we had one before 8)

> 
> On 08/09/2017 11:38 AM, Alex Bennée wrote:
> > Philippe Mathieu-Daudé <address@hidden> writes:  
> [...]
> >> Do you think there should be another entry in "Block QAPI, monitor,
> >> command line"?
> >> or this file (and related include) rather deserves an own section
> >> (possibly tagged Odd Fixes) to unburden Richard?  
> > 
> > Well the default for un-matched files is qemu-devel right? It would be
> > nice for it to be properly maintained but that requires someone to
> > step-up to the plate.  
> 
> I wonder if I'm understanding correctly what the MAINTAINERS file is for 
> and how to use it.

The blurb at the start of MAINTAINERS should describe this; I'm
wondering how much it reflects current reality, though (the 'ownership'
part).

> 
>  From an submitter view I feel a bit confused. I thought 
> ./get_maintainer.pl would give me the list of person to email the 
> changes I did on some file from the repository. This script seems 
> correctly named, I'm looking for some ./get_reviewers.pl instead, to 
> know who I'v to keep updated, apart from the ./get_maintainer.pl.
> 
> currently we have:
> "M: Mail patches to: FullName <address@hidden>"
> Does this imply FullName is a maintainer?
> If so is it ok I do this change:
> 
> -    M: Mail patches to: FullName <address@hidden>
> +    M: Maintainer: FullName <address@hidden>
> +       These maintainers must be mailed on patches.

This line seems to have originally come from the Linux kernel's
MAINTAINERS file. But I like your version better.

>       R: Designated reviewer: FullName <address@hidden>
>          These reviewers should be CCed on patches.

I'm wondering whether we need some more detailed descriptions about the
roles of maintainers and reviewers, i.e.
- maintainers are responsible for an area; they review and pick up
  patches; a non-trivial patch merged through someone else needs at
  least an ack from them
- reviewers are interested in an area, the entries are mostly there so
  that they are sure to get cc:ed on patches; while their feedback
  (like everyone's) is valued, it is not essential to getting a change
  merged

> 
> 
> actual default for un-matched: "recent contributors" + qemu-devel@
> 
>    $ ./scripts/get_maintainer.pl -f disas.c
>    get_maintainer.pl: No maintainers found, printing recent contributors.
>    get_maintainer.pl: Do not blindly cc: them on patches!  Use common sense.
>    Peter Maydell <address@hidden> (commit_signer:2/3=67%)
>    Richard Henderson <address@hidden> (commit_signer:1/3=33%)
>    Thomas Huth <address@hidden> (commit_signer:1/3=33%)
>    Michael Tokarev <address@hidden> (commit_signer:1/3=33%)
>    Julian Brown <address@hidden> (commit_signer:1/3=33%)
>    address@hidden (open list:All patches CC here)
> 
> I find the un-matched "recent contributors" list often confuse, due to 
> files being moved, header updated, checkpatch indented.
> Most of the time you get the list of queue maintainers since they accept 
> patches and sign-of the pull request.
> 
> Having to use 'git log --follow' and 'git blame' to figure out is not 
> bad, to be aware of who modified this file before you, but there are 
> some files I already hit 3 times in different series, and wondered about 
> how avoid those manual steps.
> 
> Anyway I now understand these recent contributors are not maintainers 
> but no-designated reviewers, unwilling to be maintainers (else they'd 
> have added a section/entry by themselves).

The root problem is "some files have no maintainers". The reasons range
from "forgot to include the file in the pattern" (easily fixed), over
"file is updated via a script" (the linux-headers case), to "nobody
feels up to the task" (which is the worst case).

In most cases, I don't think the recent contributors list is very
helpful. Somebody who simply did a tree-wide rename is unlikely to be
able to make a good judgment about a complicated logic change. Just
printing qemu-devel as the address to send this to is probably better;
unfortunately, it may cause patches to languish on the list if nobody
takes pity on them.

Do we need someone collecting non-trivial patches like that, who either
pesters others or picks up the patches themselves?

> 
> I also understand why Fam said it sounds weird to add an new section as 
> "Orphan".
> 
> For the linux-headers case, if people want to be notified of changes the 
> easiest seems to modify the update-linux-headers.sh script.

The approach I like the most for this is to add a pattern that covers
both the script and the directories updated by it. If someone
explicitly wants notifications for header changes that are used by
their code, they can add it to their files as well, but I don't think
that should be the default.

> 
> I'll review the whole series thinking differently and resend when 2.11 
> opens.
> 
> Regards,
> 
> Phil.