Message-ID: <20190520082402.GZ4319@dell>
Date:   Mon, 20 May 2019 09:24:02 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Theodore Ts'o <tytso@....edu>,
        Philippe Mazenauer <philippe.mazenauer@...look.de>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ext4: Variable to signed to check return code

On Sat, 18 May 2019, Theodore Ts'o wrote:

> On Sat, May 18, 2019 at 07:38:34AM +0100, Lee Jones wrote:
> >   "- Acked-by: indicates an agreement by another developer (often a
> >      maintainer of the relevant code) that the patch is appropriate for
> >      inclusion into the kernel."
> > 
> > And I, as a developer (and not a Maintainer in this case) do indicate
> > that this patch is appropriate for inclusion into the kernel.
> > 
> > Reviewed-by has stronger connotations and implies I have in-depth
> > knowledge of the subsystem/driver AND agree to the Reviewer's
> > Statement.  I use Acked-by in this case as a weaker agreement after a
> > shallow review of the patch based on its merits alone.
> 
> Note the "often a maintainer of the relevant code" bit.  And

Exactly, with the *often* (but not always, right!) being the operative
word in that sentence.  As much as I understand the meaning when used
by a Maintainer when commenting on their own subsystem (I use it in
this way a lot too), it doesn't always mean "it's okay for you to take
this patch which usually comes under my jurisdiction".

I think you're missing and if () statement in your understanding:

if (maintainer_of_patches'_subsystem)
   apply_patch_with_supplied_ack();
else
   /* Where 'n' is the regard you hold for the ack supplier. */
   add_n_units_to_patch_credibility(n);

> "appropriate for inclusion into the kernel" means to me that you've
> done the same level of review as Reviewed-by.  So I have very

Actually it doesn't, or else the documentation text for Acked-by would
be just as intense as it is for Reviewed-by.  Reviewed-by IMHO has a
much stronger standing than an Acked-by (caveat: when not provided by
a maintainer of the appropriate subsystem).

> different understanding of how Acked-by and Reviewed-by than you do.

Yes, this is seemingly the case.  It's apparent that the documentation
is not a clear as perhaps it should be, else we wouldn't be having
this conversation.  Maybe this is something which should be discussed
a Kernel Summit.  The result being a patch or two which firms up the
wording/explanation somewhat.

> That being said, no offence to you, but any kind of Acked-by or
> Reviewed-by from you is not going to have as much weight as say, a
> Reviewed-by: from someone like Jan Kara.

Seeing as Jan is a filesystem maintainer, this kind of goes without
saying.  In fact, the only reason a person might have to take the time
to write something like this is to attempt to belittle and cause
offence.  I could be wrong, but probably not. :)

> That's just because I don't have a good sense to your technical
> ability

I guess you could always use Git to gain a reasonable sense of my
technical ability.  The 4,000 committed contributions and many more
thousands of reviews on the mailing list(s), should give you a brief
glimpse.

> and so I'd be doing a full review myself

I'd think a great deal less of you if you didn't.

> and not rely on your review at all....

"at all" - wow!  What kind of message do you think this gives to first
time contributors (like Philippe here), or would-be reviewers?  That
there isn't any point in attempting to review patches, since
Maintainers are unlikely to take it into consideration "at all"?  I
know that when I come to review a patch, if *any* contributor has
taken the time to review a patch, it always plays an important role.

> P.S.  And if I find a problem in the patch, and someone had attached
> their Acked-by or Reviewed-by to it, it would have the same negative
> hit to their reputation either way.  Not a big deal if it happens only
> once, or it's an esepcially tricky issue, but it if happens more than
> once or is really blatent, I as the maintainer definitely do remember.

Again, not really sure of your intentions when you write this out, or
what it has to do with this patch submission or the review there
after, but IMHO this is sending the wrong message to new and would-be
contributors.  As a community we're supposed to be providing a
supportive, encouraging atmosphere.  This paragraph is likely to do
nothing more than scare off people who would otherwise be willing
to have a go [at submitting or reviewing a patch].

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists