[Mesa-dev] Workflow Proposal

Wed Oct 6 18:59:23 UTC 2021

On Wed, Oct 6, 2021 at 2:46 PM Jason Ekstrand <jason at jlekstrand.net> wrote:

> 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.
>

And I stand by that review!

>
> 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
>

More seriously, I'm not sure why it matters what an approval is given for.
If someone reviews 1 patch in a series, says "rb <this patch>" in a
comment, and gives an approval, they've still only reviewed that patch.
There's a problem with granularity in the Approval UI, but the comments can
fill in that gap.

I think maybe people can still leave comments with tags as they have been,
but then instead of the author doing the rewrites, reviewers can add their
Approval; maybe we'd develop a system where there's a thread on each "big"
MR that only contains the review/ack/whatever tags to make them easily
visible? I don't know, but it doesn't seem impossible to solve.

>
> >
> >>
> >>
> >> (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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20211006/1728431b/attachment-0001.htm>