On 04/30/20 03:18, Gao, Liming wrote:
> Laszlo:
>
>
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek <ler...@redhat.com>
>> Sent: Thursday, April 30, 2020 2:04 AM
>> To: Sean Brogan <spbro...@outlook.com>; devel@edk2.groups.io;
>> michael.kuba...@outlook.com
>> Cc: Andrew Fish <af...@apple.com>; Ard Biesheuvel <ard.biesheu...@arm.com>;
>> Bret Barkelew <bret.barke...@microsoft.com>;
>> Justen, Jordan L <jordan.l.jus...@intel.com>; Leif Lindholm
>> <l...@nuviainc.com>; Gao, Liming <liming....@intel.com>; Kinney,
>> Michael D <michael.d.kin...@intel.com>; Ni, Ray <ray...@intel.com>; Sean
>> Brogan <sean.bro...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58431): https://edk2.groups.io/g/devel/message/58431
Mute This Topic: https://groups.io/mt/73251653/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
