Re: [PATCH 2/3] docs: submitting-patches: clarify difference between Acked-by and Reviewed-by

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Jan 12, 2025 at 10:50:32AM -0500, Neal Gompa wrote:
> On Sun, Jan 12, 2025 at 10:30 AM Miguel Ojeda <ojeda@kernel.org> wrote:
> >
> > Newcomers to the kernel need to learn the different tags that are
> > used in commit messages and when to apply them. Acked-by is sometimes
> > misunderstood, since the documentation did not really clarify (up to
> > the previous commit) when it should be used, especially compared to
> > Reviewed-by.
> >
> > The previous commit already clarified who the usual providers of Acked-by
> > tags are, with examples. Thus provide a clarification paragraph for
> > the comparison with Reviewed-by, and give a couple examples reusing the
> > cases given above, in the previous commit.
> >
> > Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> > Acked-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > ---
> >  Documentation/process/submitting-patches.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> > index c7a28af235f7..7b0ac7370cb1 100644
> > --- a/Documentation/process/submitting-patches.rst
> > +++ b/Documentation/process/submitting-patches.rst
> > @@ -480,6 +480,12 @@ mergers will sometimes manually convert an acker's "yep, looks good to me"
> >  into an Acked-by: (but note that it is usually better to ask for an
> >  explicit ack).
> >
> > +Acked-by: is also less formal than Reviewed-by:.  For instance, maintainers may
> > +use it to signify that they are OK with a patch landing, but they may not have
> > +reviewed it as thoroughly as if a Reviewed-by: was provided.  Similarly, a key
> > +user may not have carried out a technical review of the patch, yet they may be
> > +satisfied with the general approach, the feature or the user-facing interface.
> > +
> >  Acked-by: does not necessarily indicate acknowledgement of the entire patch.
> >  For example, if a patch affects multiple subsystems and has an Acked-by: from
> >  one subsystem maintainer then this usually indicates acknowledgement of just
> > --
> > 2.48.0
> >
> 
> This doesn't make sense as a distinction. What defines "thoroughly"?
> To be honest, I think you should go the other way and become okay with
> people sending Reviewed-by tags when people have looked over a patch
> and consider it good to land.
> 
> To me, Acked-by mostly makes sense as a tag for people who *won't*
> review the code, not for those who *will*. Blending Acked-by and
> Reviewed-by just creates confusion.

Not a maintainer anymore, but --

I only give out a Reviewed-by: if I can say with a straight face "I read
this code thoroughly and understand it well enough to transform / build
on top of / maintain it if need be." I'd accept one from anyone who I
thought was either really familiar with the codebase or has become their
manager's stuc^Wappointee for maintenance.

Compare that to an Acked-by, which means "I scanned this while
doomscrolling fsdevel over coffee and none of it is now in the
keyboard", which is a much lower standard.  I'd accept one from pretty
much anyone, because that just means you're in the email blasting radius
if/when things go wrong.  Even moreso if the person qualifies their ack
with a "# XXXX" to contextualize their acknowledgement.

Concretely, I might ignore an RVB from Sam Naghshineh if he showed up
claiming to be an expert on some ext4 thing, but I wouldn't drop an Ack
from Neal because then who do I pull in when boffins demonstrate that
fallocate implements a Turing machine and hence in need of a libvirt
port?

I would, however, explicitly point out that maintainers can drop or
ignore tags as they please; and that doing so may discourage future
participation by people who feel ignored.

--D

> 
> 
> -- 
> 真実はいつも一つ！/ Always, there's only one truth!