On Wed, Oct 6, 2021 at 8:49 PM Jordan Justen <jordan.l.jus...@intel.com> wrote: > > Mike Blumenkrantz <michael.blumenkra...@gmail.com> writes: > > > On Wed, Oct 6, 2021 at 1:27 PM Bas Nieuwenhuizen <b...@basnieuwenhuizen.nl> > > wrote: > > > >> On Wed, Oct 6, 2021 at 7:07 PM Jason Ekstrand <ja...@jlekstrand.net> > >> wrote: > >> > > >> > 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