Re: [Xen-devel] [PATCH] MAINTAINERS: Add explicit check-in policy section

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2020 17:17, George Dunlap wrote:
> On 1/7/20 1:05 PM, Jan Beulich wrote:
>> On 07.01.2020 13:03, George Dunlap wrote:
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -104,7 +104,53 @@ Descriptions of section entries:
>>>        xen-maintainers-<version format number of this file>
>>>  
>>>  
>>> -The meaning of nesting:
>>> +   Check-in policy
>>> +   ===============
>>> +
>>> +In order for a patch to be checked in, in general, several conditions
>>> +must be met:
>>> +
>>> +1. In order to get a change to a given file committed, it must have
>>> +   the approval of at least one maintainer of that file.
>>> +
>>> +   A patch of course needs Acks from the maintainers of each file that
>>> +   it changes; so a patch which changes xen/arch/x86/traps.c,
>>> +   xen/arch/x86/mm/p2m.c, and xen/arch/x86/mm/shadow/multi.c would
>>> +   require an Ack from each of the three sets of maintainers.
>>> +
>>> +   See below for rules on nested maintainership.
>>> +
>>> +2. It must have an Acked-by or a Reviewed-by from someone other than
>>> +   the submitter.
>>
>> I'd like to propose some further distinction here, albeit I'm not sure
>> this isn't implied anyway. It might be that making explicit the
>> distinction between A-b and R-b is sufficient - our current common
>> understanding looks to be that only maintainers can "ack", and others
>> would "review".
> 
> Well first of all, I don't think that's strictly true.  If a
> non-maintainer raises a concern, the patch can't be checked in unless
> that person is satisfied.  We sometimes assume silence is consent, but
> it's much better for the person who raised the concern to say, "I am now
> satisfied with this patch"; and the clearest and most concise way to do
> that is to say "Acked-by".

Hmm, that's a possible model, but one I would never have thought of
given the meaning we assign to "Acked-by". In a case like what you
describe I would always have expected indication of consent by other
than a formal tag, if the person wouldn't anyway be in the position
to ack a patch (or part of it).

> But that sort of "Acked-by" isn't really what is meant by this section.
>  I guess you'd like to say that such an Acked-by would not be sufficient
> to check in a patch; it would have to be the stronger Reviewed-by.
> 
> The point of this sentence is not to define what Ack and Reviewed-by
> mean, but that it must come from someone who is not the submitter.
> However, it is true that someone may read that and be confused;
> particularly as we don't seem to define it anywhere else in the tree, so
> perhaps it's worth trying to clarify.
> 
>> Since the latter is implying a more thorough look at a
>> patch, I think it wouldn't be right to allow (quoting text further
>> down) "anyone in the community" to ack a random patch (I could probably
>> talk my son into ack-ing my patches ;-) ). Perhaps, rather than
>> limiting acks to maintainers of the changed code, we could extend this
>> to maintainers of just some code for maintainer submitted patches (i.e.
>> anyone named as M: at least once in ./MAINTAINERS)? People outside of
>> whatever subset we might pick would be eligible to offer R-b only,
>> implying of course that they actually did do a review.
> 
> I do actually prefer that only people in a "direct line" of
> maintainership for that exact code (i.e., is a maintainer at whatever
> level of specificity) be able to get Acks; and that anyone else should
> be required to give a Reviewed-by.
> 
> This is of course again slightly more aggregate work for a maintianer
> than for someone else, but I think that makes sense in this case.
> 
> How about this:
> 
> 2. It must have either a an Acked-by from a maintainer, or a
>    Reviewed-by.  This must come from someone other than the submitter.

Better, but leaving ambiguous whether "maintainer" means "any one"
or "of the code being touched". I think you mean the former, in
which case I'd prefer to see it amended along the lines of "...
from a maintainer (of any component), or ...". Or possibly you
mean any maintainer up the "nesting" chain, in which case the
wording would need to be yet different?

>>> +3. Sufficient time and/or warning must have been given for anyone to
>>> +   respond.  This depends in large part upon the urgency and nature of
>>> +   the patch.  For a straightforward uncontroversial patch, a day or
>>> +   two is sufficient; for a controversial patch, perhaps waiting a
>>> +   week and then saying "I intend to check this in tomorrow unless I
>>> +   hear otherwise".
>>
>> To me as non-native speaker, this last sentence looks incomplete (as
>> in missing e.g. "would be appropriate" at the end), or alternatively
>> it would feel like wanting the two "ing" dropped from the verbs.
> 
> I see what you mean.  But on reflection, I think the intent of this
> paragraph has gotten skewed.  Patches should be given sufficent time for
> *anyone* to give input before being checked in.
> 
> What about changing this as follows:
> 
> ---
> 3. Sufficient time must have been given for anyone to respond.  This
>    depends in large part upon the urgency and nature of the patch.
>    For a straightforward uncontroversial patch, a day or two may be
>    sufficient; for a controversial patch, a week or two may be better.
> ---
> 
> And then adding a para below:
> 
> ---
> Before a maintainer checks in their own patch with another community
> member's R-b but no co-maintainer Ack, it is especially important to
> give their co-maintainer opportunity to give feedback, perhaps
> declaring their intention to check it in without their co-maintainers
> ack a day before doing so.
> ---

This sounds good to me.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel