Message-ID: <20190520153639.GB3933@mit.edu>
Date:   Mon, 20 May 2019 11:36:39 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     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 Mon, May 20, 2019 at 09:24:02AM +0100, Lee Jones wrote:
> > "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).

Well quoting from submitting-patches: "It [Acked-by: ] is a record
that the acker has AT LEAST REVIEWED THE PATCH" (emphasis mine).

The primary use of it is to "signify and record their approval of it".
And...

    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 the part which affects
    that maintainer's code.  Judgement should be used here.  When in
    doubt people should refer to the original discussion in the
    mailing list archives.

My question is what is the *point* of including a non-maintainer's
Acked-by: to the git record?  And if it's not the maintainer (or a
core developer for a subsystem), then it becomes unclear what portion
of the patch the non-Maintainer has reviewed.  So at that point, how
can a Maintainer rely on a non-Maintainer Acked-by at all in the first
place?

> > 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.

So if I'm going to have to do a full review (which you approve), that
by definition means I'm not relying on the review at all, right?

The way I handle things is that while I'm not going to rely on a
Reviewed-by from an unknown reviewer, I do remember who provides
reviews, and this will bump up their reputation so that perhaps in the
future, I will rely on their reviews.  Reviews that including some
kind of substantive comments (if correct) will enhance the reviewer's
reputation much more than a blind "Reviewd-by: ".

BTW, empty reviews for a patch authored by alice@...pany-foo.com,
coming from bob@...pany-foo.com (e.g., where the only content of the
review is Reviewed-by: bob@...pany-foo.com) are things that I give
much less weight, especially when bob@...pany-foo.com is not a known
developer to me.

(Yes, I know that sometimes patches get developed behind closed doors,
and then squirt out fully formed with a four or five Reviewed-by:
lines.  But if I haven't seen the process, it doesn't give much value
to the maintainer trying to judge the suitability of the patch.  Red
Hat's approach to do all patch discussions in the open is highly to be
commended here.)

> 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].

One of the things that I worry about are people who game git
statistics by submitting a lot of empty Reviewed-by or Acked-by lines.
It's right up there with people who send huge numbers of whitespace
fixes.  So my personal approach is to not include Reviewed-by or
Acked-by if it didn't add any value to the project.  It may indeed
still value to the reviewer's reputation, and if the reviewer has
helped to improve the patch, I'll make sure they get credit.

I do agree that we should provide a supportive atmosphere.  But we
also need provide encouragement that contributors provide more
substantive patches and more substantive reviewes.

Cheers,

					- Ted

Powered by blists - more mailing lists