Re: [Ksummit-discuss] Reforming Acked-by (was Re: [TOPIC] Encouraging more reviewers)

[Olof Johansson <olof@lixom.net>]

On Thu, May 29, 2014 at 2:03 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, May 29, 2014 02:27:53 PM Theodore Ts'o wrote:
>> On Wed, May 28, 2014 at 06:48:47PM +0000, Paul Walmsley wrote:
>> >
>> > Also long-overdue is a clarification on exactly what "Acked-by" means.
>> > Right now it is being used for at least two distinct and
>> > mutually-incompatible purposes:
>> >
>> > 1. A maintainer A for code affected by a patch, who is distinct from a
>> > maintainer B queuing a patch, has reviewed the patch and has cleared it as
>> > being OK for maintainer B to send upstream
>> >
>> > 2. A casual review has been done by someone who is not a maintainer for
>> > the code in question
>> >
>> > What I would propose is to have the first use replaced by a new tag,
>> > "Maintainer-acked-by:", and the second use abolished, along with
>> > "Acked-by:", and replaced by "Reviewed-by:".
>>
>> I agree in general, but if we're going to abolish the 2nd use
>> entirely, then it's much simpler to keep Acked-by for its original
>> meaning; it's easier to type, after all.
>>
>> This is basically I do for ext4 patches today; if someone sends me an
>> acked-by in the #2 sense, I simply won't add it to the s-o-b section,
>> and I don't let the fact that someone has asserted that they have done
>> a casual review to give me a false sense of security; if I still have
>> to do a deep review, I'm going to catch the casual stuff anyway, and
>> the fact that a casual review doesn't obviate the need for a careful
>> review.
>>
>> But if a senior ext4 developer adds a Reviewed-by:, that does lend a
>> lot of value to me as a maintainer, since I can trust that certain
>> folks like Jan and Eric and Lukas and others will do a good job doing
>> the review, and that actually *does* offload significant amounts of
>> work off my shoulders.
>
> Well, perhaps we can reserve the Acked-by for maintainers and add
> something like Supported-by for the 2nd meaning.

What are we really trying to fix here? Is the current process really
broken or are we trying to create more process that's not needed for
some other reason?

As a maintainer for arm-soc, I know which subsystem maintainers I
should get an Acked-by before I'm ok merging in a branch with, say,
some driver changes from a platform maintainer. I mostly know because
we keep intersecting with the same subsystems, but for new ones
there's one natural place I go to look: MAINTAINERS. Figuring out
merge paths and when something should go through our tree instead of
the maintainers tree is always going to be something where people
actually need to talk to each other and make a decision, I don't think
we can make tools and process for it.

Renaming the tag that they use isn't going to change the due diligence
I have to make (I still need to make sure they're actually the right
person to give it out, etc). And I'm definitely not worried by the
possible conflict of the "I gave this a casual review and I think we
should let it go in" acks since a maintainer is unlikely to give out
those kind of acks to code that he would otherwise merge himself.


-Olof