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

[Leif Lindholm]

On Thu, Apr 30, 2020 at 13:24:57 +0200, Laszlo Ersek wrote:
> On 04/30/20 03:18, Gao, Liming wrote:
> > Laszlo:
> > 
> > 
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek at redhat.com>
> >> Sent: Thursday, April 30, 2020 2:04 AM
> >> To: Sean Brogan <spbrogan at outlook.com>; devel at edk2.groups.io; michael.kubacki at outlook.com
> >> Cc: Andrew Fish <afish at apple.com>; Ard Biesheuvel <ard.biesheuvel at arm.com>; Bret Barkelew <Bret.Barkelew at microsoft.com>;
> >> Justen, Jordan L <jordan.l.justen at intel.com>; Leif Lindholm <leif at nuviainc.com>; Gao, Liming <liming.gao at intel.com>; Kinney,
> >> Michael D <michael.d.kinney at intel.com>; Ni, Ray <ray.ni at intel.com>; Sean Brogan <sean.brogan at 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.

Agreed.

As a (probably unnexessary) counterpoint, I'll just add that in
instances where the Reviewer clearly is better placed to determine
what is correct and the Maintainer is mainly driving the bus, it could
be misleading for the Maintainer to add an A-b (whereas actually
pushing the patch is a very clear explicit approval).

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#58440): https://edk2.groups.io/g/devel/message/58440
Mute This Topic: https://groups.io/mt/73251653/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-