On Wed, Oct 6, 2021 at 2:46 PM Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Wed, Oct 6, 2021 at 12:37 PM Mike Blumenkrantz > <michael.blumenkra...@gmail.com> wrote: > > > > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen < > b...@basnieuwenhuizen.nl> wrote: > >> > >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand <ja...@jlekstrand.net> > wrote: > >> > > >> > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt <e...@anholt.net> wrote: > >> > > > >> > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz > >> > > <michael.blumenkra...@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. >
