Messages in this thread | | | Date | Sat, 6 Apr 2024 13:14:59 +0200 | Subject | Re: [PATCH v19 2/9] usb: dwc3: core: Access XHCI address space temporarily to read port info | From | Krzysztof Kozlowski <> |
| |
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
| |