Paolo Bonzini <pbonz...@redhat.com> writes: > On Mon, Jul 1, 2024 at 4:34 PM Philippe Mathieu-Daudé <phi...@linaro.org> > wrote: >> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org> > > In principle, a Reviewed-by tag is just stating that you don't know of > any issues that would prevent the patch being included. However, as a > frequent participant to the project, your Reviewed-by tag carries some > weight and, to some extent, it is also a statement that you understand > the area being modified. A Reviewed-by from an experienced > contributor may even imply that you could take the patch in one of > your pull requests. (*) That makes it even more important to > understand the area.
I think you are attaching a little too much weight to a r-b tag here as no one was suggesting the patch go in via a different tree. Ultimately the maintainer can always NACK a reviewed patch. > I would expect that anyone with an understanding of command line > parsing would know 1) what -accel kvm -accel tcg does, and 2) what > .merge_lists does; and this would be enough to flag an issue > preventing the patch from being included. Maybe more useful would be re-wording the comment: /* Merge multiple uses of option into a single list? */ to be explicit about its behaviour. > To be clear, I don't expect reviews to be perfect. But in this case > I'm speaking up because the patch is literally a one line declarative > change, and the only way to say "I've reviewed it" is by understanding > the deeper effects of that line. I think that's a fairly subjective requirement for something that generally we can always use more of. I encourage people to review all around the code base to get familiar with new sub-systems. I don't think we should be dissuading people from exploring outside their silos. That simple one liners can trip people up says more about the code than the reviewer. I sympathise with Philippe here who's current brief takes him around our large and interconnected code base more than most. > > Also, I think it's fair that the submitter didn't spot the problem; > it's okay to send out broken patches, that's part of the learning > experience. :) > > Paolo > > (*) as opposed to Acked-by, where your review probably has been more > conceptual than technical, and that you don't really want to take the > patch in a pull request. > > > Paolo -- Alex Bennée Virtualisation Tech Lead @ Linaro
