<div dir="ltr"><div dir="ltr">On Wed, Oct 6, 2021 at 2:46 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Oct 6, 2021 at 12:37 PM Mike Blumenkrantz<br>
<<a href="mailto:michael.blumenkrantz@gmail.com" target="_blank">michael.blumenkrantz@gmail.com</a>> wrote:<br>
><br>
> On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote:<br>
>><br>
>> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br>
>> ><br>
>> > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt <<a href="mailto:emma@anholt.net" target="_blank">emma@anholt.net</a>> wrote:<br>
>> > ><br>
>> > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz<br>
>> > > <<a href="mailto:michael.blumenkrantz@gmail.com" target="_blank">michael.blumenkrantz@gmail.com</a>> wrote:<br>
>> > > ><br>
>> > > > Hi,<br>
>> > > ><br>
>> > > > 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.<br>
>> > > ><br>
>> > > > 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.<br>
>> > > ><br>
>> > > > * Except some incredibly smart people already know where I'm going with this<br>
>> > > ><br>
>> > > > 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!<br>
>> > > ><br>
>> > > > Anyway, I thought maybe this would be a nice idea to improve everyone's workflows. What do other people think?<br>
>> ><br>
>> > My primary grip with approvals or the 👍 button is that it's the wrong<br>
>> > granularity.  It's per-MR instead of per-patch.  When people are<br>
>> > regularly posting MRs that touch a bunch of different stuff, per-patch<br>
>> > review is pretty common.  I'm not sure I want to lose that.  :-/<br>
>><br>
>> Would it be an option to get Marge to not remove existing Rb tags, so<br>
>> we could get the streamlined process where possible and fall back if<br>
>> the MRs turn more complicated?<br>
><br>
><br>
> 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...<br>
<br>
That's a really easy thing to say but this is an actual problem and<br>
one I hit on a regular basis.  Take, for instance, this MR:<br>
<br>
<a href="https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/13045" rel="noreferrer" target="_blank">https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/13045</a><br>
<br>
What am I supposed to do?  Should I post one MR and merge it as soon<br>
as at least one representative from each touched driver approves?  On<br>
the above MR, I've gotten quite a few people to sign off on the<br>
search-and-replace driver patch but no one has yet to look at the<br>
common bits.  How do I know when those are reviewed?  Or should I<br>
assume everyone who clicks "approve" reviewed every part of the MR<br>
that touches their driver.  That would mean the common bits would get<br>
reviewed 6 times which, while great for code quality, is a waste of<br>
review time.<br>
<br>
Or maybe I can split it up.  Make one MR with all the common<br>
improvements, then 6 per-driver MRs and then another big MR that goes<br>
on top of the per-driver MRs?  In that case, because GitLab also<br>
doesn't have a concept of MR dependencies, the initial common patches<br>
are going to be in all 8, and everything's going to be in the last<br>
one.  How the "what did they review?" confusion is even worse.<br>
<br>
And, no, I can't trust people to approve the MR they intend to.  Just<br>
the other day, I posted !13184 which explicitly said in the<br>
description that it's based on !13156 and you (Mike) reviewed a patch<br>
from !13156 on !13184.<br></blockquote><div><br></div><div>And I stand by that review!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Or should I post the one MR for common code first and then wait for<br>
that to land before posting the per-driver MRs.  That doesn't work out<br>
well because can be very important, when reviewing common code, to see<br>
how it impacts your driver.<br>
<br>
Just to be clear, the above are all genuine questions.  I want the<br>
button-based review process as much as the next person but I have yet<br>
to come up with a scheme that actually works when you start crossing<br>
component boundaries.  The best I've got is typing RB tags in comments<br>
and copy+pasting them into commit messages.  If someone's got a plan<br>
which handles the above way-more-common-than-you'd-think case, I'm all<br>
ears.  As much as it sucks, rebasing and adding RB tags sucks less<br>
than 8MRs.<br>
<br>
--Jason<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
>><br>
>><br>
>> (as an aside I think we should just drop the tags in git, but I'll<br>
>> take anything that moves us forward)<br>
>> ><br>
>> > --Jason<br>
>> ><br>
>> > > I would love to see this be the process across Mesa.  We already don't<br>
>> > > rewrite commit messages for freedreno and i915g, and I only have to do<br>
>> > > the rebase (busy-)work for my projects in other areas of the tree.<br>
>> > ><br>
>> > > I don't think we should have marge-bot require approvals<br>
>> > > per-component, though.  There are times when an MR only incidentally<br>
>> > > touches a component (for example, changing function signatures in<br>
>> > > gallium), and actually getting a dev from every driver to sign off on<br>
>> > > it would be too much.<br>
</blockquote></div></div>