[Qemu-devel] [PATCH] MAINTAINERS: fix pattern for PC

[Qemu-devel] [PATCH] MAINTAINERS: fix pattern for PC

[Michael S. Tsirkin]

Looks like hw/i386/ does not work, hw/i386/*
seems to work better.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d48a6b..39a01ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -578,7 +578,7 @@ PC
 M: Michael S. Tsirkin <mst@redhat.com>
 S: Supported
 F: include/hw/i386/
-F: hw/i386/
+F: hw/i386/*
 F: hw/pci-host/piix.c
 F: hw/pci-host/q35.c
 F: hw/pci-host/pam.c
-- 
MST

Re: [Qemu-devel] [PATCH] MAINTAINERS: fix pattern for PC

[Peter Maydell]

On 29 June 2015 at 10:54, Michael S. Tsirkin <mst@redhat.com> wrote:
> Looks like hw/i386/ does not work, hw/i386/*
> seems to work better.

This is kind of vague... The documentation at the top
of MAINTAINERS says the difference is that "hw/i386/"
means "all files in and below hw/i386/", whereas
"hw/i386/*" means "all files in hw/i386, but not below"
(so won't match anything in hw/i386/kvm/ or hw/i386/xen/).
Is this the effect you're trying to achieve? It would
be nice to mention the symptoms of the problem this patch
is fixing in the commit message...

> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d48a6b..39a01ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -578,7 +578,7 @@ PC
>  M: Michael S. Tsirkin <mst@redhat.com>
>  S: Supported
>  F: include/hw/i386/
> -F: hw/i386/
> +F: hw/i386/*
>  F: hw/pci-host/piix.c
>  F: hw/pci-host/q35.c
>  F: hw/pci-host/pam.c

thanks
-- PMM

Re: [Qemu-devel] [PATCH] MAINTAINERS: fix pattern for PC

[Laszlo Ersek]

On 06/29/15 12:00, Peter Maydell wrote:
> On 29 June 2015 at 10:54, Michael S. Tsirkin <mst@redhat.com> wrote:
>> Looks like hw/i386/ does not work, hw/i386/*
>> seems to work better.
> 
> This is kind of vague... The documentation at the top
> of MAINTAINERS says the difference is that "hw/i386/"
> means "all files in and below hw/i386/", whereas
> "hw/i386/*" means "all files in hw/i386, but not below"
> (so won't match anything in hw/i386/kvm/ or hw/i386/xen/).
> Is this the effect you're trying to achieve? It would
> be nice to mention the symptoms of the problem this patch
> is fixing in the commit message...

Assume I format the patch series:

  http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg06677.html

into a single file, with

  git format-patch --notes --cover-letter --numbered --stdout

and then run scripts/get_maintainer.pl on the resultant patch series file.

Before this patch, I get:

  Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
  Richard Henderson <rth@twiddle.net> (maintainer:X86)
  Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
  "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)

Since Michael was listed at the bottom of that list, I didn't CC him. (I
wanted to give him a breather after my many PXB iterations.)

Turns out that wasn't a good choice. With this patch for MAINTAINERS in
place, the script reports Michael at the top:

  "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
  Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
  Richard Henderson <rth@twiddle.net> (maintainer:X86)
  Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)

Maybe I should have considered something else than just the ordering of
the names in the list, not sure...

Hm, yes, this is at least partly (if not fully) my fault. MAINTAINERS says

	S: Status, one of the following:
	   Supported:	Someone is actually paid to look after this.
	   Maintained:	Someone actually looks after it.

I didn't realize this distinction, and I also didn't realize that
"supporter" and "maintainer" were derived directly from "Supported" and
"Maintained".

It would be helpful if developers with more jurisdiction (according to
the Supported / Maintained / Odd Fixes classification) were listed
higher in the output.

Thanks
Laszlo

> 
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  MAINTAINERS | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3d48a6b..39a01ab 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -578,7 +578,7 @@ PC
>>  M: Michael S. Tsirkin <mst@redhat.com>
>>  S: Supported
>>  F: include/hw/i386/
>> -F: hw/i386/
>> +F: hw/i386/*
>>  F: hw/pci-host/piix.c
>>  F: hw/pci-host/q35.c
>>  F: hw/pci-host/pam.c
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] MAINTAINERS: fix pattern for PC

[Peter Maydell]

On 29 June 2015 at 11:23, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/29/15 12:00, Peter Maydell wrote:
>> On 29 June 2015 at 10:54, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> Looks like hw/i386/ does not work, hw/i386/*
>>> seems to work better.
>>
>> This is kind of vague... The documentation at the top
>> of MAINTAINERS says the difference is that "hw/i386/"
>> means "all files in and below hw/i386/", whereas
>> "hw/i386/*" means "all files in hw/i386, but not below"
>> (so won't match anything in hw/i386/kvm/ or hw/i386/xen/).
>> Is this the effect you're trying to achieve? It would
>> be nice to mention the symptoms of the problem this patch
>> is fixing in the commit message...
>
> Assume I format the patch series:
>
>   http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg06677.html
>
> into a single file, with
>
>   git format-patch --notes --cover-letter --numbered --stdout
>
> and then run scripts/get_maintainer.pl on the resultant patch series file.
>
> Before this patch, I get:
>
>   Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
>   Richard Henderson <rth@twiddle.net> (maintainer:X86)
>   Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
>   "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
>
> Since Michael was listed at the bottom of that list, I didn't CC him. (I
> wanted to give him a breather after my many PXB iterations.)
>
> Turns out that wasn't a good choice. With this patch for MAINTAINERS in
> place, the script reports Michael at the top:
>
>   "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
>   Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
>   Richard Henderson <rth@twiddle.net> (maintainer:X86)
>   Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
>
> Maybe I should have considered something else than just the ordering of
> the names in the list, not sure...
>
> Hm, yes, this is at least partly (if not fully) my fault. MAINTAINERS says
>
>         S: Status, one of the following:
>            Supported:   Someone is actually paid to look after this.
>            Maintained:  Someone actually looks after it.
>
> I didn't realize this distinction, and I also didn't realize that
> "supporter" and "maintainer" were derived directly from "Supported" and
> "Maintained".
>
> It would be helpful if developers with more jurisdiction (according to
> the Supported / Maintained / Odd Fixes classification) were listed
> higher in the output.

Mmm. If this is what we're trying to fix then messing with
our MAINTAINERS file seems like the wrong thing.

Personally I think that the best approach would be just to
cc everybody that get_maintainers.pl says is a maintainer
or supporter; they're in the file because they *want* this
email, after all... If your patchset touches two areas then
that doesn't mean it's OK to drop a 'maintainer' email for
area 2 just because area 1 happens to have two or three
'supporter' emails listed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] MAINTAINERS: fix pattern for PC

[Laszlo Ersek]

On 06/29/15 12:42, Peter Maydell wrote:
> On 29 June 2015 at 11:23, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/29/15 12:00, Peter Maydell wrote:
>>> On 29 June 2015 at 10:54, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> Looks like hw/i386/ does not work, hw/i386/*
>>>> seems to work better.
>>>
>>> This is kind of vague... The documentation at the top
>>> of MAINTAINERS says the difference is that "hw/i386/"
>>> means "all files in and below hw/i386/", whereas
>>> "hw/i386/*" means "all files in hw/i386, but not below"
>>> (so won't match anything in hw/i386/kvm/ or hw/i386/xen/).
>>> Is this the effect you're trying to achieve? It would
>>> be nice to mention the symptoms of the problem this patch
>>> is fixing in the commit message...
>>
>> Assume I format the patch series:
>>
>>   http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg06677.html
>>
>> into a single file, with
>>
>>   git format-patch --notes --cover-letter --numbered --stdout
>>
>> and then run scripts/get_maintainer.pl on the resultant patch series file.
>>
>> Before this patch, I get:
>>
>>   Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
>>   Richard Henderson <rth@twiddle.net> (maintainer:X86)
>>   Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
>>   "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
>>
>> Since Michael was listed at the bottom of that list, I didn't CC him. (I
>> wanted to give him a breather after my many PXB iterations.)
>>
>> Turns out that wasn't a good choice. With this patch for MAINTAINERS in
>> place, the script reports Michael at the top:
>>
>>   "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
>>   Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
>>   Richard Henderson <rth@twiddle.net> (maintainer:X86)
>>   Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
>>
>> Maybe I should have considered something else than just the ordering of
>> the names in the list, not sure...
>>
>> Hm, yes, this is at least partly (if not fully) my fault. MAINTAINERS says
>>
>>         S: Status, one of the following:
>>            Supported:   Someone is actually paid to look after this.
>>            Maintained:  Someone actually looks after it.
>>
>> I didn't realize this distinction, and I also didn't realize that
>> "supporter" and "maintainer" were derived directly from "Supported" and
>> "Maintained".
>>
>> It would be helpful if developers with more jurisdiction (according to
>> the Supported / Maintained / Odd Fixes classification) were listed
>> higher in the output.
> 
> Mmm. If this is what we're trying to fix then messing with
> our MAINTAINERS file seems like the wrong thing.
> 
> Personally I think that the best approach would be just to
> cc everybody that get_maintainers.pl says is a maintainer
> or supporter; they're in the file because they *want* this
> email, after all...

Are you sure? See commit c6561586.

... Hm, although, that's specifically for the case when the MAINTAINERS
file has failed to help. So you can indeed say that whenever MAINTAINERS
matches, those people *all* want to be Cc'd.

> If your patchset touches two areas then
> that doesn't mean it's OK to drop a 'maintainer' email for
> area 2 just because area 1 happens to have two or three
> 'supporter' emails listed.

I didn't omit some Cc's because of area mismatches. I just wanted to
avoid spamming people, and took only the top of the list (Paolo). In the
future I'll include everyone. (Hopefully that won't lead to all people
on the Cc list thinking that someone else on that long Cc list should
review the patch...)

Thanks
Laszlo