Re: [Mesa-dev] Workflow Proposal
On 07/10/2021 00:56, Emma Anholt wrote: On Wed, Oct 6, 2021 at 10:07 AM Jason Ekstrand wrote: On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz 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. :-/ If we leave aside the "marge bot requires approvals" thing, which I don't think is plausible, then we could have it both ways: easy MRs get a thumbs up from someone reasonable and we hand it directly to marge, or complicated MRs can have people doing per-patch review like they do today, and someone at the end decides to hand to marge (with or without per-patch rbs rebased in). I'm with Jordan and Emma on this. Just have Marge add as many "Approved-by: @USERID" to every commit in the series as there are people who pressed the "Approve" button, and be done with it :) Since it is a different tag, we know it was a "Whole-MR ACK" rather than a per-patch one, which would come in the Reviewed/Acked/Tested-by form. Thanks for raising this up Mike! Martin
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 12:28 PM Jordan Justen wrote: > > Mike Blumenkrantz writes: > > > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen > > wrote: > > > >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand > >> 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. Part of the deal with gitlab was that sysadmins would be keeping the gitlab links working at least in a static form if we ever decided to turn down gitlab. Just like bugzilla still responds to queries even after we decided to migrate away.
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 10:07 AM Jason Ekstrand wrote: > > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: > > > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz > > 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. :-/ If we leave aside the "marge bot requires approvals" thing, which I don't think is plausible, then we could have it both ways: easy MRs get a thumbs up from someone reasonable and we hand it directly to marge, or complicated MRs can have people doing per-patch review like they do today, and someone at the end decides to hand to marge (with or without per-patch rbs rebased in).
Re: [Mesa-dev] Workflow Proposal
Bas Nieuwenhuizen writes: > On Wed, Oct 6, 2021 at 8:49 PM Jordan Justen > wrote: >> >> 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. > > AFAIU this is upstream Marge behavior. Once you enable the > Approval->Rb tag conversion it removes existing Rb tags. Hence why we > don't have the conversion enabled. > Ah, I guess it is documented for --add-reviewers here: https://github.com/smarkets/marge-bot#adding-reviewed-by-tested-and-part-of-to-commit-messages "All existing Reviewed-by: trailers on commits in the branch will be stripped." I hope we would wait for Marge to add a --add-approvers switch which would leave Reviewed-by tags alone, but add Approved-by tags. -Jordan
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 2:46 PM Jason Ekstrand wrote: > On Wed, Oct 6, 2021 at 12:37 PM Mike Blumenkrantz > 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 > wrote: > >> > > >> > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: > >> > > > >> > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz > >> > > 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 " in a comment, and gives an approval, they've still only reviewed that patch. There's a problem with granularity in the Appr
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 8:49 PM Jordan Justen wrote: > > Mike Blumenkrantz writes: > > > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen > > wrote: > > > >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand > >> 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. AFAIU this is upstream Marge behavior. Once you enable the Approval->Rb tag conversion it removes existing Rb tags. Hence why we don't have the conversion enabled. > > > 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
Re: [Mesa-dev] Workflow Proposal
Mike Blumenkrantz writes: > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen > wrote: > >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand >> 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
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 12:37 PM Mike Blumenkrantz wrote: > > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen > wrote: >> >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand wrote: >> > >> > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: >> > > >> > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz >> > > 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 shoul
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen wrote: > On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand > wrote: > > > > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: > > > > > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz > > > 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... > > (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. >
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand wrote: > > On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: > > > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz > > 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? (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.
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 11:24 AM Emma Anholt wrote: > > On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz > 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. :-/ --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.
Re: [Mesa-dev] Workflow Proposal
On Wed, Oct 6, 2021 at 9:20 AM Mike Blumenkrantz 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? 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.
[Mesa-dev] Workflow Proposal
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? Mike
