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

From: Juergen Gross <jgross@suse.com>
To: George Dunlap <george.dunlap@citrix.com>, xen-devel@lists.xenproject.org
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>, Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	Ian Jackson <ian.jackson@citrix.com>
Subject: Re: [PATCH] MAINTAINERS: Add explicit check-in policy section
Date: Wed, 8 May 2019 13:59:46 +0200	[thread overview]
Message-ID: <d56ae8a0-7770-cb30-7996-a0d68d85e073@suse.com> (raw)
In-Reply-To: <20190508113947.11920-1-george.dunlap@citrix.com>

On 08/05/2019 13:39, George Dunlap 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>
> ---
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Lars Kurth <lars.kurth@citrix.com>
> 
> This is a follow-up to the discussion in `[PATCH for-4.12]
> passthrough/vtd: Drop the "workaround_bios_bug" logic entirely`, specifically
> Message-ID: <5C9CF25A020000780022291B@prv1-mh.provo.novell.com>
> 
> 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.
> 
> A third approach would be to say that in the case of multiple
> maintainers, the maintainers themselves can decide to mandate the
> other maintainer's Ack.  For instance, Dario and I could agree that we
> don't need each others' ack for changes to the scheduler, but Andy and
> Jan could agree that they do need each other's Ack for changes to the
> x86 code.

What about variant 2b:

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 the submitter is a maintainer of a modified file it must
have an Ack or Review from either a "nested" maintainer or a Designated
reviewer of that file.

> ---
>  MAINTAINERS | 46 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e43388ddb0..65ba35f02d 100644
> --- 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.
> +
> +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.
> +
> +4. There must be no "open" objections.
> +
> +In a case where one person submits a patch and a maintainer gives an
> +Ack, the Ack stands in for both the approval requirement (#1) and the
> +Acked-by-non-submitter requirement (#2).
> +
> +In a case where a maintainer themselves submits a patch, the
> +Signed-off-by meets the approval requriment (#1); so an Ack or Review
> +from anyone in the community suffices for requirement #2.
> +
> +Maintainers may choose to override non-maintainer objections in the
> +case that consensus can't be reached.
> +
> +       The meaning of nesting
> +       ======================

Everywhere else tabs are used for indenting.

Juergen

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