Re: [Mesa-dev] Workflow Proposal
> > >> Upstream should do what's best for upstream, not for Intel's "unique" > > >> management. > > >> > > >>Not sure how from Emma explaining how Rb tags were used by Intel > > >>management it came the conclusion that it were used in that way only > > >> by > > >>Intel management. Spoiler: it is not. > > > > > > Sorry, I'll make that point more emphatic. > > > > > > Upstream must do what's best for upstream without zero regard for the > > > whims of management. Doubly so for bad management. > > > > If the r-b process ever had any notice from any company's management, I > > haven't seen it. (Actually, I think most management would rather have > > the short sighted view of skipping code review to more quickly merge > > patches.) In terms of who to "track down", that is also a tenuous > > connection. > > All of the above is true but also totally irrelevant to the actual discussion. > > When R-b as a metric came up at the time of the first switch, I wrote > a really trivial Python script which used the GitLab API to scrape MR > discussions and pull 'Reviewed-by: ...' comments out and print a > leaderboard for number of reviewed MRs over the past calendar month. > Adapting that to look at approvals rather than comments would cut it > down to about 10 LoC. > > Whether it's Reviewed-by in the trailer or an approval, both are > explicitly designed to be machine readable, which means it's trivial > to turn it into a metric if you want to. Whether or not that's a good > idea is the problem of whoever wields it. Fair enough. I don't have strong views on the tags themselves. If upstream has good reasons to use them, let's use them; if upstream has good reasons to omit them, let's omit them. What I oppose is management metrics playing a role in upstream's discussion. That goes for any company's management; I do not mean to single out Intel here. But given the aforementioned script, even the managers will be happy either way. My apologies for prolonging what in retrospect is a silly discussion.
Re: [Mesa-dev] Workflow Proposal
Daniel Stone writes: > On Wed, 13 Oct 2021 at 20:13, Jordan Justen wrote: >> Alyssa Rosenzweig writes: >> > >> > Sorry, I'll make that point more emphatic. >> > >> > Upstream must do what's best for upstream without zero regard for the >> > whims of management. Doubly so for bad management. >> >> If the r-b process ever had any notice from any company's management, I >> haven't seen it. (Actually, I think most management would rather have >> the short sighted view of skipping code review to more quickly merge >> patches.) In terms of who to "track down", that is also a tenuous >> connection. > > All of the above is true but also totally irrelevant to the actual discussion. > > When R-b as a metric came up at the time of the first switch, I wrote > a really trivial Python script which used the GitLab API to scrape MR > discussions and pull 'Reviewed-by: ...' comments out and print a > leaderboard for number of reviewed MRs over the past calendar month. > Adapting that to look at approvals rather than comments would cut it > down to about 10 LoC. > > Whether it's Reviewed-by in the trailer or an approval, both are > explicitly designed to be machine readable, which means it's trivial > to turn it into a metric if you want to. Whether or not that's a good > idea is the problem of whoever wields it. Correct. So let's not try to play whac-a-mole/manager/evil-genius. :) I think several people have already said that it's good to take the time to recognize the often overlooked and thankless job of reviewing. Potentially stripping Reviewed-by tags seems counter to that. Adding Approved-by from the web page could be helpful to simplify reviews for many merge-requests. But, retaining per-patch Reviewed-by is helpful in other cases. So hopefully we can add Approved-by *without* destroying Reviewed-by tags in the process. I would like to be able to use either in the cases where they make sense. -Jordan
Re: [Mesa-dev] Workflow Proposal
On Wed, 13 Oct 2021 at 20:13, Jordan Justen wrote: > Alyssa Rosenzweig writes: > >> Upstream should do what's best for upstream, not for Intel's "unique" > >> management. > >> > >>Not sure how from Emma explaining how Rb tags were used by Intel > >>management it came the conclusion that it were used in that way only by > >>Intel management. Spoiler: it is not. > > > > Sorry, I'll make that point more emphatic. > > > > Upstream must do what's best for upstream without zero regard for the > > whims of management. Doubly so for bad management. > > If the r-b process ever had any notice from any company's management, I > haven't seen it. (Actually, I think most management would rather have > the short sighted view of skipping code review to more quickly merge > patches.) In terms of who to "track down", that is also a tenuous > connection. All of the above is true but also totally irrelevant to the actual discussion. When R-b as a metric came up at the time of the first switch, I wrote a really trivial Python script which used the GitLab API to scrape MR discussions and pull 'Reviewed-by: ...' comments out and print a leaderboard for number of reviewed MRs over the past calendar month. Adapting that to look at approvals rather than comments would cut it down to about 10 LoC. Whether it's Reviewed-by in the trailer or an approval, both are explicitly designed to be machine readable, which means it's trivial to turn it into a metric if you want to. Whether or not that's a good idea is the problem of whoever wields it. Cheers, Daniel
Re: [Mesa-dev] Workflow Proposal
Alyssa Rosenzweig writes: >> Upstream should do what's best for upstream, not for Intel's "unique" >> management. >> >>Not sure how from Emma explaining how Rb tags were used by Intel >>management it came the conclusion that it were used in that way only by >>Intel management. Spoiler: it is not. > > Sorry, I'll make that point more emphatic. > > Upstream must do what's best for upstream without zero regard for the > whims of management. Doubly so for bad management. If the r-b process ever had any notice from any company's management, I haven't seen it. (Actually, I think most management would rather have the short sighted view of skipping code review to more quickly merge patches.) In terms of who to "track down", that is also a tenuous connection. The value of r-b is to give reviewers credit for the hard work that they do. (Which, I believe is what Matt and apinheiro are also saying.) Personally I try to make a rework log on patch commit messages to give reviewers more explicit credit for changes that are made based on their code review. I hope Marge Bot doesn't start stripping the r-b tags. But, if Marge can add Approved-by which allows the review process to flow more quickly for some types of merge-requests, then I think that's a good thing. I wouldn't be surprised if this became the most commonly used review process in Mesa. -Jordan
Re: [Mesa-dev] Workflow Proposal
> > >> 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. > > > Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost > > > devs do, which I'm fine with. But it's not a requirement to merging. > > > > > > The arguments about "who can help support this years from now?" are moot > > > at our scale... the team is small enough that the name on the reviewer > > > is likely the code owner / maintainer, and patches regularly go in > > > unreviewed for lack of review bandwidth. > > > > There is another reason to the Rb tag, that is to measure the quantity > > of patch review people do. > > > > This was well summarized some years ago by Matt Turner, as it was > > minimized (even suggested to be removed) on a different thread: > > > > https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html > > I was part of the Intel team when people started doing this r-b > counting. I believe that it was being done due to Intel management's > failure to understand who was doing the work on the team and credit > them appropriately, and also to encourage those doing less to step up. > Unfortunately, the problem with Intel management wasn't a lack of > available information, and I didn't see publishing the counts change > reviews either. Upstream should do what's best for upstream, not for Intel's "unique" management.
Re: [Mesa-dev] Workflow Proposal
> Upstream should do what's best for upstream, not for Intel's "unique" > management. > >Not sure how from Emma explaining how Rb tags were used by Intel >management it came the conclusion that it were used in that way only by >Intel management. Spoiler: it is not. Sorry, I'll make that point more emphatic. Upstream must do what's best for upstream without zero regard for the whims of management. Doubly so for bad management.
Re: [Mesa-dev] Workflow Proposal
I'd like gitlab macros :rb: and :ab: that put the tags into the comment. Marek On Tue, Oct 12, 2021 at 5:01 PM Jason Ekstrand wrote: > On Tue, Oct 12, 2021 at 3:56 PM apinheiro wrote: > > > > > > On 12/10/21 13:55, Alyssa Rosenzweig wrote: > > > > 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. > > > > Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost > > devs do, which I'm fine with. But it's not a requirement to merging. > > > > The arguments about "who can help support this years from now?" are moot > > at our scale... the team is small enough that the name on the reviewer > > is likely the code owner / maintainer, and patches regularly go in > > unreviewed for lack of review bandwidth. > > > > There is another reason to the Rb tag, that is to measure the quantity > > of patch review people do. > > > > This was well summarized some years ago by Matt Turner, as it was > > minimized (even suggested to be removed) on a different thread: > > > > https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html > > > > I was part of the Intel team when people started doing this r-b > > counting. I believe that it was being done due to Intel management's > > failure to understand who was doing the work on the team and credit > > them appropriately, and also to encourage those doing less to step up. > > > > > > That's basically the same problem with trying to measure and compare > developers just by commit count. In theory commit count is a bad measure > for that. In practice it is used somehow. > > > > Unfortunately, the problem with Intel management wasn't a lack of > > available information, and I didn't see publishing the counts change > > reviews either. > > > > > > > > Upstream should do what's best for upstream, not for Intel's "unique" > > management. > > > > > > Not sure how from Emma explaining how Rb tags were used by Intel > management it came the conclusion that it were used in that way only by > Intel management. Spoiler: it is not. > > > > Replying both, that's is one of the reasons I pointed original Matt > Turner email. He never mentioned explicitly Intel management, neither > pointed this as an accurate measure of the use. Quoting: > > > > "The number of R-b tags is not a 100% accurate picture of the > > situation, but it gives at least a good overview of who is doing the > > tedious work of patch review. " > > > > In any case, just to be clear here: Im not saying that the Rb tags main > use is this one. Just saying that is one of their uses, and the value for > such use can be debatable, but it is not zero. > > Negative numbers aren't zero! > > --Jason >
Re: [Mesa-dev] Workflow Proposal
On Tue, Oct 12, 2021 at 3:56 PM apinheiro wrote: > > > On 12/10/21 13:55, Alyssa Rosenzweig wrote: > > 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. > > Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost > devs do, which I'm fine with. But it's not a requirement to merging. > > The arguments about "who can help support this years from now?" are moot > at our scale... the team is small enough that the name on the reviewer > is likely the code owner / maintainer, and patches regularly go in > unreviewed for lack of review bandwidth. > > There is another reason to the Rb tag, that is to measure the quantity > of patch review people do. > > This was well summarized some years ago by Matt Turner, as it was > minimized (even suggested to be removed) on a different thread: > > https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html > > I was part of the Intel team when people started doing this r-b > counting. I believe that it was being done due to Intel management's > failure to understand who was doing the work on the team and credit > them appropriately, and also to encourage those doing less to step up. > > > That's basically the same problem with trying to measure and compare > developers just by commit count. In theory commit count is a bad measure for > that. In practice it is used somehow. > > Unfortunately, the problem with Intel management wasn't a lack of > available information, and I didn't see publishing the counts change > reviews either. > > > > Upstream should do what's best for upstream, not for Intel's "unique" > management. > > > Not sure how from Emma explaining how Rb tags were used by Intel management > it came the conclusion that it were used in that way only by Intel > management. Spoiler: it is not. > > Replying both, that's is one of the reasons I pointed original Matt Turner > email. He never mentioned explicitly Intel management, neither pointed this > as an accurate measure of the use. Quoting: > > "The number of R-b tags is not a 100% accurate picture of the > situation, but it gives at least a good overview of who is doing the > tedious work of patch review. " > > In any case, just to be clear here: Im not saying that the Rb tags main use > is this one. Just saying that is one of their uses, and the value for such > use can be debatable, but it is not zero. Negative numbers aren't zero! --Jason
Re: [Mesa-dev] Workflow Proposal
On 12/10/21 13:55, Alyssa Rosenzweig wrote: 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. Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost devs do, which I'm fine with. But it's not a requirement to merging. The arguments about "who can help support this years from now?" are moot at our scale... the team is small enough that the name on the reviewer is likely the code owner / maintainer, and patches regularly go in unreviewed for lack of review bandwidth. There is another reason to the Rb tag, that is to measure the quantity of patch review people do. This was well summarized some years ago by Matt Turner, as it was minimized (even suggested to be removed) on a different thread: https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html I was part of the Intel team when people started doing this r-b counting. I believe that it was being done due to Intel management's failure to understand who was doing the work on the team and credit them appropriately, and also to encourage those doing less to step up. That's basically the same problem with trying to measure and compare developers just by commit count. In theory commit count is a bad measure for that. In practice it is used somehow. Unfortunately, the problem with Intel management wasn't a lack of available information, and I didn't see publishing the counts change reviews either. Upstream should do what's best for upstream, not for Intel's "unique" management. Not sure how from Emma explaining how Rb tags were used by Intel management it came the conclusion that it were used in that way only by Intel management. Spoiler: it is not. Replying both, that's is one of the reasons I pointed original Matt Turner email. He never mentioned explicitly Intel management, neither pointed this as an accurate measure of the use. Quoting: "The number of R-b tags is not a 100% accurate picture of the situation, but it gives at least a good overview of who is doing the tedious work of patch review. " In any case, just to be clear here: Im not saying that the Rb tags main use is this one. Just saying that is one of their uses, and the value for such use can be debatable, but it is not zero. BR
Re: [Mesa-dev] Workflow Proposal
On Sun, Oct 10, 2021 at 4:44 PM apinheiro wrote: > > Answering here, as it is the second time it is mentioned that Rb is only > for "who can help support this years from now?", but not specifically to > this email. > > On 7/10/21 15:00, Alyssa Rosenzweig wrote: > >> 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. > > Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost > > devs do, which I'm fine with. But it's not a requirement to merging. > > > > The arguments about "who can help support this years from now?" are moot > > at our scale... the team is small enough that the name on the reviewer > > is likely the code owner / maintainer, and patches regularly go in > > unreviewed for lack of review bandwidth. > > There is another reason to the Rb tag, that is to measure the quantity > of patch review people do. > > This was well summarized some years ago by Matt Turner, as it was > minimized (even suggested to be removed) on a different thread: > > https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html I was part of the Intel team when people started doing this r-b counting. I believe that it was being done due to Intel management's failure to understand who was doing the work on the team and credit them appropriately, and also to encourage those doing less to step up. Unfortunately, the problem with Intel management wasn't a lack of available information, and I didn't see publishing the counts change reviews either.
Re: [Mesa-dev] Workflow Proposal
On Thu, 2021-10-07 at 09:38 +0300, Martin Roukala (néé Peres) wrote: > > 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 Hi, I've never had a problem with manually adding Reviewed-by tags, but best would be to have an approve or review button next to each commit, which would allow us to approve individual commits rather than entire merge requests. If GitLab only supports per-merge-request granularity, the approve approach sounds fine for merge requests that aren't cross-cutting and therefore don't need reviewers from multiple teams. (I guess these are the the majority of MRs.) However, for larger MRs that touch eg. multiple drivers and/or NIR, I think the old review process is better (until we can do per-commit approval in GitLab). Best regards, Timur
Re: [Mesa-dev] Workflow Proposal
Hi, On 7.10.2021 16.00, Alyssa Rosenzweig wrote: 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. Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost devs do, which I'm fine with. But it's not a requirement to merging. The arguments about "who can help support this years from now?" are moot at our scale... the team is small enough that the name on the reviewer is likely the code owner / maintainer, and patches regularly go in unreviewed for lack of review bandwidth. I was thinking more of outsiders trying to track down issues (who may be using older Mesa versions, and wanting both to ask about bisected commit, and find out whether there's a fix for given issue in upstream). Longer list of people is useful in case Mesa driver team has been changing in intervening years. But this is very much a corner-case concern. I think it's reasonable that small driver teams and large common components have different review needs, and it's ok for the process to reflect that. +1 - Eero
Re: [Mesa-dev] Workflow Proposal
Answering here, as it is the second time it is mentioned that Rb is only for "who can help support this years from now?", but not specifically to this email. On 7/10/21 15:00, Alyssa Rosenzweig wrote: 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. Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost devs do, which I'm fine with. But it's not a requirement to merging. The arguments about "who can help support this years from now?" are moot at our scale... the team is small enough that the name on the reviewer is likely the code owner / maintainer, and patches regularly go in unreviewed for lack of review bandwidth. There is another reason to the Rb tag, that is to measure the quantity of patch review people do. This was well summarized some years ago by Matt Turner, as it was minimized (even suggested to be removed) on a different thread: https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html I think it's reasonable that small driver teams and large common components have different review needs, and it's ok for the process to reflect that.
Re: [Mesa-dev] Workflow Proposal
> 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. Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost devs do, which I'm fine with. But it's not a requirement to merging. The arguments about "who can help support this years from now?" are moot at our scale... the team is small enough that the name on the reviewer is likely the code owner / maintainer, and patches regularly go in unreviewed for lack of review bandwidth. I think it's reasonable that small driver teams and large common components have different review needs, and it's ok for the process to reflect that.
Re: [Mesa-dev] Workflow Proposal
Despite all the time it takes to add the tags and force-push, I have no objection to doing that. It captures per-commit reviews well. Marek On Thu, Oct 7, 2021 at 1:17 PM Eero Tamminen wrote: > Hi, > > On 7.10.2021 19.51, Daniel Stone wrote: > > On Thu, 7 Oct 2021 at 09:38, Eero Tamminen > wrote: > >> This sounds horrible from the point of view of trying to track down > >> somebody who knows about what's & why's of some old commit that is later > >> on found to cause issues... > > > > But why would your first point of call not be to go back to the review > > discussion and look at the context and what was said at the time? Then > > when you do that, you can see not only what happened, but also who was > > involved and saying what at the time. > > You're assuming that: > - The review discussion is still available [1] > - One can find it based on given individual commit > > [1] system hosting it could be down, or network could be down. > > It's maybe a bit contrived situation, but I kind of prefer > self-contained information. What, why and who is better to be in commit > itself than only in MR. > > > - Eero >
Re: [Mesa-dev] Workflow Proposal
Hi, On 7.10.2021 19.51, Daniel Stone wrote: On Thu, 7 Oct 2021 at 09:38, Eero Tamminen wrote: This sounds horrible from the point of view of trying to track down somebody who knows about what's & why's of some old commit that is later on found to cause issues... But why would your first point of call not be to go back to the review discussion and look at the context and what was said at the time? Then when you do that, you can see not only what happened, but also who was involved and saying what at the time. You're assuming that: - The review discussion is still available [1] - One can find it based on given individual commit [1] system hosting it could be down, or network could be down. It's maybe a bit contrived situation, but I kind of prefer self-contained information. What, why and who is better to be in commit itself than only in MR. - Eero
Re: [Mesa-dev] Workflow Proposal
On Thu, 7 Oct 2021 at 09:38, Eero Tamminen wrote: > This sounds horrible from the point of view of trying to track down > somebody who knows about what's & why's of some old commit that is later > on found to cause issues... But why would your first point of call not be to go back to the review discussion and look at the context and what was said at the time? Then when you do that, you can see not only what happened, but also who was involved and saying what at the time. Cheers, Daniel
Re: [Mesa-dev] Workflow Proposal
" On Thu, Oct 7, 2021 at 1:38 AM Eero Tamminen wrote: > > Hi, > > On 6.10.2021 23.00, Jordan Justen wrote: > > 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." > > This sounds horrible from the point of view of trying to track down > somebody who knows about what's & why's of some old commit that is later > on found to cause issues... > > For whom those extra Rb tags are a problem and why? To explain Marge's behavior here: I think their concern is if it was assigned to Marge with one set of reviewers, then failed to merge, then assigned to Marge again with another set of reviewers flagged in the MR, then they want to update the set of reviewers associated with the merge without leaving in someone who had retracted their review. For Mesa where people provide per-patch review, that's silly. We could fork and strip out that behavior, but in the original proposal this flag wasn't getting enabled anyway so Marge's behavior is moot. And, again, if you want to "track down somebody who knows about what's & why's of some old commit", just click the link that's right there in the commit, which gives you not just the names but also the comments they had about the commit back when they reviewed it!
Re: [Mesa-dev] Workflow Proposal
Hi, On 6.10.2021 23.00, Jordan Justen wrote: 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." This sounds horrible from the point of view of trying to track down somebody who knows about what's & why's of some old commit that is later on found to cause issues... For whom those extra Rb tags are a problem and why? 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. +1 - Eero
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
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
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.
