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
