[Mesa-dev] Workflow Proposal

[Jason Ekstrand]

On Wed, Oct 6, 2021 at 12:37 PM Mike Blumenkrantz
<michael.blumenkrantz at gmail.com> wrote:
>
> 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:
>> >
>> > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt <emma at anholt.net> wrote:
>> > >
>> > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz
>> > > <michael.blumenkrantz at gmail.com> wrote:
>> > > >
>> > > > Hi,
>> > > >
>> > > > It's recently come to my attention that gitlab has Approvals. Was anyone else aware of this feature? You can just click a button and have your name recorded in the system as having signed off on landing a patch? Blew my mind.
>> > > >
>> > > > So with that being said, we also have this thing in the Mesa repo where everyone* has to constantly be adding these esoteric tags like Reviewed-by (I reviewed it), and Acked-by (I rubber stamped it), or Tested-by (I compiled it and maybe ran glxgears), and so forth.
>> > > >
>> > > > * Except some incredibly smart people already know where I'm going with this
>> > > >
>> > > > Instead of continuing to have to manually update each patch with the appropriate and definitely-unforgeable tags, what if we just used Approvals in the UI instead? We could then have marge-bot require approvals as needed in components and bring reviewing into the current year. Just think: no more rewriting all the commit logs and force-pushing the branch again before you merge!
>> > > >
>> > > > Anyway, I thought maybe this would be a nice idea to improve everyone's workflows. What do other people think?
>> >
>> > 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.  :-/
>>
>> 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?
>
>
> 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...

That's a really easy thing to say but this is an actual problem and
one I hit on a regular basis.  Take, for instance, this MR:

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/13045

What am I supposed to do?  Should I post one MR and merge it as soon
as at least one representative from each touched driver approves?  On
the above MR, I've gotten quite a few people to sign off on the
search-and-replace driver patch but no one has yet to look at the
common bits.  How do I know when those are reviewed?  Or should I
assume everyone who clicks "approve" reviewed every part of the MR
that touches their driver.  That would mean the common bits would get
reviewed 6 times which, while great for code quality, is a waste of
review time.

Or maybe I can split it up.  Make one MR with all the common
improvements, then 6 per-driver MRs and then another big MR that goes
on top of the per-driver MRs?  In that case, because GitLab also
doesn't have a concept of MR dependencies, the initial common patches
are going to be in all 8, and everything's going to be in the last
one.  How the "what did they review?" confusion is even worse.

And, no, I can't trust people to approve the MR they intend to.  Just
the other day, I posted !13184 which explicitly said in the
description that it's based on !13156 and you (Mike) reviewed a patch
from !13156 on !13184.

Or should I post the one MR for common code first and then wait for
that to land before posting the per-driver MRs.  That doesn't work out
well because can be very important, when reviewing common code, to see
how it impacts your driver.

Just to be clear, the above are all genuine questions.  I want the
button-based review process as much as the next person but I have yet
to come up with a scheme that actually works when you start crossing
component boundaries.  The best I've got is typing RB tags in comments
and copy+pasting them into commit messages.  If someone's got a plan
which handles the above way-more-common-than-you'd-think case, I'm all
ears.  As much as it sucks, rebasing and adding RB tags sucks less
than 8MRs.

--Jason

>
>>
>>
>> (as an aside I think we should just drop the tags in git, but I'll
>> take anything that moves us forward)
>> >
>> > --Jason
>> >
>> > > I would love to see this be the process across Mesa.  We already don't
>> > > rewrite commit messages for freedreno and i915g, and I only have to do
>> > > the rebase (busy-)work for my projects in other areas of the tree.
>> > >
>> > > I don't think we should have marge-bot require approvals
>> > > per-component, though.  There are times when an MR only incidentally
>> > > touches a component (for example, changing function signatures in
>> > > gallium), and actually getting a dev from every driver to sign off on
>> > > it would be too much.