[Mesa-dev] Workflow Proposal

[Bas Nieuwenhuizen]

On Wed, Oct 6, 2021 at 8:49 PM Jordan Justen <jordan.l.justen at intel.com> wrote:
>
> Mike Blumenkrantz <michael.blumenkrantz at gmail.com> writes:
>
> > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> > wrote:
> >
> >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand <jason at jlekstrand.net>
> >> wrote:
> >> >
> >> > My primary grip with approvals or the 👍 button is that it's the wrong
> >> > granularity.  It's per-MR instead of per-patch.  When people are
> >> > regularly posting MRs that touch a bunch of different stuff, per-patch
> >> > review is pretty common.  I'm not sure I want to lose that.  :-/
>
> Could a hybrid approach work? Marge could just add:
>
> Approved-by: @jljusten
>
> to the commit message based on the state of the MR. But, for MR's where
> r-b is more appropriate, the developer can still manually add
> Reviewed-by.
>
> Personally I don't find adding the r-b and force pushing to be much of a
> burden, but I could see how in some cases of a small MR, it could be
> nice to just click some buttons on the web-page and be done with it.
>
> But, I really would like Marge to add something to the commit messages
> indicating who approved it. Yeah, you can get that info today by
> following the Part-of link, but there's no guarantees about that being
> around forever.
>
> >> Would it be an option to get Marge to not remove existing Rb tags, so
> >> we could get the streamlined process where possible and fall back if
> >> the MRs turn more complicated?
>
> I guess I missed where it was suggested that Marge should remove
> Reviewed-by tags. I don't think Marge should ever remove something from
> the commit message.

AFAIU this is upstream Marge behavior. Once you enable the
Approval->Rb tag conversion it removes existing Rb tags. Hence why we
don't have the conversion enabled.

>
> > If people really, truly care about per-patch Approval, couldn't they just
> > split out patches from bigger MRs and get Approvals there? Otherwise it
> > should be trivial enough to check the gitlab MR and see who reviewed which
> > patch if it becomes an issue at a later date. Odds are at that point you're
> > already going to the MR to see wtf someone was thinking...
>
> I don't like the idea of saying "just split out the MRs". That doesn't
> work in a lot of cases where patches have dependencies, and just causes
> potential reviewers to have to look in more places to see the big
> picture.
>
> -Jordan