[Mesa-dev] Workflow Proposal

[Jordan Justen]

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.

> 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