On 05/04/2024 22:36, Dmitry Baryshkov wrote: >>>>> >>>>> Also get those internal reviewers to sign-off on the commits and have >>>>> that show up when you post them next. That way they are also >>>>> responsible for this patchset, it's not fair that they are making you do >>>>> all the work here :) >>>>> >>>> >>>> I like this idea and I'm open to us changing our way of handling this. >>>> >>>> But unless such internal review brings significant input to the >>>> development I'd say a s-o-b would take the credit from the actual >>>> author. >>> >>> It does not do that at all. It provides proof that someone else has >>> reviewed it and agrees with it. Think of it as a "path of blame" for >>> when things go bad (i.e. there is a bug in the submission.) Putting >>> your name on it makes you take responsibility if that happens. >>> >> >> Right, this is why I like your idea. >> >> But as s-o-b either builds a trail of who handled the patch, or reflects >> that it was co-authored by multiple people, I don't think either one >> properly reflects reality. >> >>>> We've discussed a few times about carrying Reviewed-by et al from the >>>> internal reviews, but as maintainer I dislike this because I'd have no >>>> way to know if a r-b on vN means the patch was reviewed, or if it was >>>> just "accidentally" carried from v(N-1). >>>> But it might be worth this risk, is this something you think would be >>>> appropriate? >>> >>> For some companies we REQUIRE this to happen due to low-quality >>> submissions and waste of reviewer's time. Based on the track record >>> here for some of these patchsets, hopefully it doesn't become a >>> requirement for this company as well :) >>> >> >> Interesting, I was under the impression that we (maintainers) didn't >> want such internally originating tags. > > But why? It just means that the patch has been reviewed. In some rare > cases we explicitly ask a developer to have all the patches reviewed > before sending them upstream. In such a case having an R-B tag > fulfills the expectation of the maintainer: it shows that another > engineer has reviewed the patch. Wait, there are two types of internal reviews. Automatic, +1 from Gerrit or from whatever internal processes require, which are not useful because these internal reviewers do not actually review. I have seen a lot of such and I complain. It's easy to spot them - a patchset consisting of few patches, including trivial ones, all of them carrying one more more review tags. Even fixing a typo: reviewed tag. Plus then you see that quality of the patchset is actually poor. Another are real reviews done internally. If they are real, I find them useful. Best regards, Krzysztof
