Re: [PATCH] docs: submitting-patches: Clarify half-baked review tags

[Weijie Yuan <wy@wyuan.org>]

Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com> writes:

> Refactoring of driver subsystems or some core driver parts often come
> with change of the actual drivers.  If that happens within one patch,
> people giving "Reviewed-by" tag only to piece of the patch, like one
> driver, and ignoring entire rest including the main concept of the
> patch, are not really reviewing "the patch".
> 
> As explained in Submitting Patches document, the "Reviewer's statement
> of oversight" is for patch:
> 
>   (a) I have carried out a technical review of this patch to
>       evaluate its appropriateness and readiness for inclusion into
>       the mainline kernel.
> 
> The sentence is complete, meaning it does not allow implicit variations
> "this patch or part of this patch".

Agreed.

> Reviewing pieces of patch while ignoring the main part of it, is
> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
> not be applicable.

But in this case, I suspect that your "Acked-by" trailer will still be
interpreted as an ack of the entire patch in future git logs, rather
than a partial ack and/or a partial review.

IMO, it would be better to clearly specify which specific parts I
reviewed, like "Reviewed-by: foo # for part a". But if there were no
such usage, I would actually think it would be better not to give
any type of trailers.

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> 
> ---
> 
> And let me even bring a better example. Imagine a patch, doing massive
> refactoring of core SPI stuff, 1000 lines of code, complex locking and
> also simultaneously updates drivers to the new API changed in here.
> 
> Reviewing that 1000 lines of code in core SPI with complex locking takes
> someone 30 minutes. He gives Rb tag, gets credit.
> 
> Now another person comes, looks at a SINGLE line in one driver he is
> responsible for and gives Rb tag. For 10 seconds job of looking at one
> changed call in one driver.

Yup, therefore, we better suggest that the latter should not give out
any trailers, or explicitly state which parts I have reviewed.

That said, my understanding of these is not as profound as yours.

Thanks.

> ---
>  Documentation/process/submitting-patches.rst | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index f271d7413f72..d2aa31bdee1e 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -585,6 +585,8 @@ serves to give credit to reviewers and to inform maintainers of the degree of
>  review which has been done on the patch.  Reviewed-by: tags, when supplied by
>  reviewers known to understand the subject area and to perform thorough reviews,
>  will normally increase the likelihood of your patch getting into the kernel.
> +If the reviewer checks only piece of the patch, leaving the rest unreviewed,
> +then more appropriately is Acked-by tag.
>  
>  Both Tested-by and Reviewed-by tags, once received on mailing list from tester
>  or reviewer, should be added by author to the applicable patches when sending
> -- 
> 2.53.0
> 
> 
>