Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

["Laszlo Ersek" <lersek@redhat.com>]

On 04/30/20 03:18, Gao, Liming wrote:
> Laszlo:
> 
> 
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, April 30, 2020 2:04 AM
>> To: Sean Brogan <spbrogan@outlook.com>; devel@edk2.groups.io; michael.kubacki@outlook.com
>> Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
>> Justen, Jordan L <jordan.l.justen@intel.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Sean Brogan <sean.brogan@microsoft.com>
>> Subject: Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg
>>
>> On 04/28/20 18:35, Sean Brogan wrote:
>>> I think this was my fault.
>>>
>>> I was under the impression that a patch needed one of developers listed
>>> in the (m) or (r) section of maintainers.txt to provide a reviewed-by.
>>> My new understanding is an ack from the (m) plus anyone providing a
>>> reviewed-by is enough.
>>
>> It depends on the maintainer, too.
>>
>> Personally I give R-b if I carefully review the patch and am pleased
>> with it.
>>
>> I give A-b if I review the patch for general sanity, but don't dig into
>> the details. I can also give A-b if someone I trust to do a good review
>> in the subject technical area provides an R-b, regardless of whether
>> they are an "R" or an otherwise un-designated contributor. With "R"
>> folks the chance is higher for me to see such an R-b posted in the first
>> place, of course.
>>
>> I do think an "M" person should provide "at least" an A-b, even if they
>> delegate the actual detailed review to someone else.
>>
> I don't think there is such requirement to maintainer now. If you think this is required, 
> You can give the proposal to add this requirement in Maintainers.txt.

I feel that the current language is expressive enough:

  M: Package Maintainer: Cc address for patches and questions. Responsible
     for reviewing and pushing package changes to source control.
  R: Package Reviewer: Cc address for patches and questions. Reviewers help
     maintainers review code, but don't have push access. A designated Package
     Reviewer is reasonably familiar with the Package (or some modules
     thereof), and/or provides testing or regression testing for the Package
     (or some modules thereof), in certain platforms and environments.

See "Responsible for reviewing" vs "help [...] review".

NB, I don't want to force other maintainers to ACK explicitly, if they
consider their co-reviewers' feedback definitive / authoritative. It's
just that, when *only* "R" approval has been posted, and the "M" folks
with jurisdiction don't react at all (no feedback, also not merging the
patch), a *different* maintainer that wants to help out may not be able
to decide whether he/she should wait for more ("M") feedback, or just
run with the "R" feedback. An explicit ACK from the owner "M" guys
clears this up at once.

Thanks
Laszlo