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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.05.19 at 13:39, <george.dunlap@citrix.com> wrote:
> The "nesting" section in the MAINTAINERS file was not initially
> intended to describe the check-in policy for patches, but only how
> nesting worked; but since there was no check-in policy, it has been
> acting as a de-facto policy.
> 
> One problem with this is that the policy is not complete: It doesn't
> cover open objections, time to check-in, or so on.  The other problem
> with the policy is that, as written, it doesn't account for
> maintainers submitting patches to files which they themselves
> maintain.  This is fine for situations where there are are multiple
> maintaniers, but not for situations where there is only one
> maintianer.
> 
> Add an explicit "Check-in policy" section to the MAINTAINERS document
> to serve as the canonical reference for the check-in policy.  Move
> paragraphs not explicitly related to nesting into it.
> 
> While here, "promote" the "The meaning of nesting" section title.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

First of all - thanks for putting this together and this way kicking off
the discussion which model to standardize on. As a nit - I think the
whole thing could do with running through a spell checker: There's
multiple spellings of "maintainer" above, and I think I've noticed a
few more issues.

> This encodes my understanding of the policy, and what I think is the
> best one.
> 
> A second approach would be:
> 
> 1.  In order to get a change to a given file committed, it must have
> an Ack or Review from at least one maintainer of that file other than
> the submitter.
> 
> 2. In the case where a file has only one maintainer, it must have an
> Ack or Review from a "nested" maintainer.
> 
> I.e., if I submitted something to x86/mm, it would require an Ack from
> Jan or Andy, or (in exceptional circumstances) The Rest; but an Ack from
> (say) Roger or Juergen wouldn't suffice.

This, FTR, is my understanding of the current policy.

A general remark on your use of "nested": My (non-native-English)
understanding of this would mean a more specific rather than a more
general area. Hence to avoid mis-interpretation I'd prefer if the
terms "more general" and "more specific" could be used, just like the
pre-existing section (also) does.

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -99,7 +99,46 @@ Descriptions of section entries:
>  	   One regex pattern per line.  Multiple K: lines acceptable.
>  
>  
> -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.

For one, the way we use Acked-by and Reviewed-by, an ack from a
non-maintainer IMO makes no sense. But then this is too lax for my
taste anyway. I'd be happy to see dedicated reviewers' R-b or more
general maintainers' A-b be sufficient. (This is relevant for maintainer
submitted patches only anyway, as otherwise item 1 will cover it in
full already.)

> +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 is
> +   sufficient; for a controversial patch, longer (maybe a week) would
> +   be better.

Is a week really enough? I realize (not the least from experiencing
overly long delays) that waiting longer reduces overall progress,
but there have been a number of cases in the not so distant past
where waiting even multiple weeks or months turned out to not be
enough for objections to finally surface (sometimes post commit).
So as a submitter I'd prefer short timeout periods here, but as a
maintainer I think longer might be preferable. But of course, since
maintainer acks are needed anyway, this may be infrequent enough
a problem to not worry about it in the general policy.

What you don't mention at all is the not uncommon case of patches
sitting there indefinitely without receiving any acks or reviews (or
where the discussion simply gets stalled), despite pings. I really
think we need to at least mention this situation as well, even if only
to set expectations. But I also could imagine some reasonable
timeout to be established for such changes to go in without all the
needed acks (as I'm intending - policy or not by that time - to do for
"[PATCH v7 00/10] x86: indirect call overhead reduction" once the
tree is open again, as previously indicated).

Jan


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