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

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

[George Dunlap]

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
maintainers, but not for situations where there is only one
maintainer.

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.

DISCUSSION

This seems to be a change from people's understanding of the current
policy.  Most people's understanding of the current policy seems to 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.

Let's call this the "maintainer-ack" approach (because it must have an
ack or r-b from a maintainer to be checked in), and the proposal in
this patch the "maintainer-approval" (since SoB from a maintainer
indicates approval).

The core issue I have with "maintainer-ack" is that it makes the
maintainer less privileged with regard to writing code than
non-maintainers.  If component X has maintainers A and B, then a
non-maintainer can have code checked in if reviewed either by A or B.
If A or B wants code checked in, they have to wait for exactly one
person to review it.

In fact, if B is quite busy, the easiest way for A really to get their
code checked in might be to hand it to a non-maintainer N, and ask N
to submit it as their own.  Then A can Ack the patches and check them
in.

The current system, therefore, either sets up a perverse incentive (if
you think the behavior described above is unacceptable) or unnecessary
bureaucracy (if you think it's acceptable).  Either way I think we
should set up our system to avoid it.

Other variations on "maintainer-ack" have been proposed:

- Allow maintainer's patches to go in with an R-b from "designated
  reviewers"

- Allow maintainer's patches to go in with an Ack from more general
  maintainer

Both fundamentally make it harder for maintainers to get their code in
and/or reviewed effectively than non-maintainers, setting up the
perverse incentive / unnecessary bureaucracy.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- Modify "sufficient time" to "sufficient time and/or warning".
- Add a comment explicitly stating that there are exceptions.
- Move some of the alternate proposals into the changelog itself

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
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@xen.org>
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>

Another approach would be to say that in the case of multiple
maintainers, the maintainers themselves can decide to mandate each
other'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.
Checks that maintainers themselves have agreed on will produce neither
perverse incentives, nor be considered "unnecessary".
---
 MAINTAINERS | 53 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index eaea4620e2..9d15afa595 100644
--- 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.
+
+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".
+
+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 requirement (#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.
+
+As always, no policy can cover all possible situations.  In
+exceptional circumstances, committers may commit a patch in absence of
+one or more of the above requirements, if they are reasonably
+confident that the other maintainers will approve of their decision in
+retrospect.
+
+       The meaning of nesting
+       ======================
 
 Many maintainership areas are "nested": for example, there are entries
 for xen/arch/x86 as well as xen/arch/x86/mm, and even
@@ -118,11 +164,6 @@ the Ack of the xen/arch/x86/mm/shadow maintainer for that part of the
 patch, but would not require the Ack of the xen/arch/x86 maintainer or
 the xen/arch/x86/mm maintainer.
 
-(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.)
-
 2. In unusual circumstances, a more general maintainer's Ack can stand
 in for or even overrule a specific maintainer's Ack.  Unusual
 circumstances might include:
-- 
2.24.1


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

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

[George Dunlap]

On 1/7/20 12:03 PM, George Dunlap wrote:
> v2:
> - Modify "sufficient time" to "sufficient time and/or warning".
> - Add a comment explicitly stating that there are exceptions.
> - Move some of the alternate proposals into the changelog itself

Sorry, this should obviously have 'v2' in the subject.

 -George

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

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

[Jan Beulich]

On 07.01.2020 13:03, George Dunlap wrote:
> DISCUSSION
> 
> This seems to be a change from people's understanding of the current
> policy.  Most people's understanding of the current policy seems to 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.
> 
> Let's call this the "maintainer-ack" approach (because it must have an
> ack or r-b from a maintainer to be checked in), and the proposal in
> this patch the "maintainer-approval" (since SoB from a maintainer
> indicates approval).
> 
> The core issue I have with "maintainer-ack" is that it makes the
> maintainer less privileged with regard to writing code than
> non-maintainers.  If component X has maintainers A and B, then a
> non-maintainer can have code checked in if reviewed either by A or B.
> If A or B wants code checked in, they have to wait for exactly one
> person to review it.
> 
> In fact, if B is quite busy, the easiest way for A really to get their
> code checked in might be to hand it to a non-maintainer N, and ask N
> to submit it as their own.  Then A can Ack the patches and check them
> in.
> 
> The current system, therefore, either sets up a perverse incentive (if
> you think the behavior described above is unacceptable) or unnecessary
> bureaucracy (if you think it's acceptable).  Either way I think we
> should set up our system to avoid it.

I much appreciate this initiative of yours.

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

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

Jan

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

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

[George Dunlap]

On 1/7/20 1:05 PM, Jan Beulich wrote:
> On 07.01.2020 13:03, George Dunlap wrote:
>> DISCUSSION
>>
>> This seems to be a change from people's understanding of the current
>> policy.  Most people's understanding of the current policy seems to 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.
>>
>> Let's call this the "maintainer-ack" approach (because it must have an
>> ack or r-b from a maintainer to be checked in), and the proposal in
>> this patch the "maintainer-approval" (since SoB from a maintainer
>> indicates approval).
>>
>> The core issue I have with "maintainer-ack" is that it makes the
>> maintainer less privileged with regard to writing code than
>> non-maintainers.  If component X has maintainers A and B, then a
>> non-maintainer can have code checked in if reviewed either by A or B.
>> If A or B wants code checked in, they have to wait for exactly one
>> person to review it.
>>
>> In fact, if B is quite busy, the easiest way for A really to get their
>> code checked in might be to hand it to a non-maintainer N, and ask N
>> to submit it as their own.  Then A can Ack the patches and check them
>> in.
>>
>> The current system, therefore, either sets up a perverse incentive (if
>> you think the behavior described above is unacceptable) or unnecessary
>> bureaucracy (if you think it's acceptable).  Either way I think we
>> should set up our system to avoid it.
> 
> I much appreciate this initiative of yours.
> 
>> --- 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".

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.

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

 -George



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

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

[Jan Beulich]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

On 1/7/20 4:44 PM, Jan Beulich wrote:
> On 07.01.2020 17:17, George Dunlap wrote:
>> On 1/7/20 1:05 PM, Jan Beulich wrote:
>> 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?

I've tried to reword this to make it more clear (see v4).  Just in
general, though, it would be helpful if when you found some wording
insufficient, if you tried to craft something you thought was better.
Even if I don't use it, it gives me a clearer idea the direction you'd
like to go in.

Thanks,
 -George

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

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

[George Dunlap]

On 5/9/19 12:16 PM, Ian Jackson wrote:
> George Dunlap writes ("[PATCH] MAINTAINERS: Add explicit check-in policy section"):
>> +	Check-in policy
>> +	===============
>> +
>> +In order for a patch to be checked in, in general, several conditions
>> +must be met:
> 
> I think it is very helpful to write guidelines, but I am opposed to
> declaring this as a rigid policy.

The phrase "in general" was meant to indicate that it was not a rigid
policy, but... well, a general one, which was understood may not apply
in exceptional circumstances.

> In particular as committer I often bend the rules (I guess, I mean,
> insofar as we have rules, I do things that feel like bending them).

[snip]

> Does this make sense ?

Sure, but is this not already covered by the section at the end of "The
meaning of nesting"?  Or are you specifically thinking of a case where
1) you write the fix, and 2) nobody from THE REST is around to provide
an "stand-in for the more specific maintainer" Ack?

If the latter, we can certainly add some text to say that in such an
exceptional circumstance, a committer may act unilaterally.

I think in general, first the specific maintainer's Ack ought to be
sought; and if that cannot be found, then a then more general
maintainer's Ack should be sought; and if there is nobody from the most
general set of maintainers (THE REST) other than the submitter of the
patch, then the submitter/committer should make a call as to whether to
act unilaterally or to wait.  In all cases where a more general
maintainer has stood in for a more specific maintainer, they should be
prepared to defend their choice.

Is that what you had in mind?

 -George

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

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

[Jan Beulich]

>>> On 09.05.19 at 13:05, <ian.jackson@citrix.com> wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH] MAINTAINERS: Add explicit 
> check-in policy section"):
>> On 5/8/19 12:59 PM, Juergen Gross wrote:
>> > 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.
>> 
>> That changes the current meaning of "Designated Reviewer" somewhat.  At
>> the moment, it only means "Send this person an email"; if we did that,
>> it would mean, "This person is more trusted to do reviews than others".
>> 
>> Not sure what I think about that yet.
> 
> I think this is a bad idea.  The purpose was to provide a way for
> someone to be emailed which doesn't involve giving them authority,
> and which is therefore a status which can be granted on request and
> does not require any assessment etc.
> 
> If we chane this we will have to invent a new status to perform this
> function.

I think we will want to do this anyway: "Designated Reviewer" isn't
the same as "Cc <person> on patches" (which may be just because
someone wants to monitor what's being submitted / discussed). At
least that's my (non-native-English) understanding of "designated".

Jan



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

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

[Ian Jackson]

George Dunlap writes ("[PATCH] MAINTAINERS: Add explicit check-in policy section"):
> +	Check-in policy
> +	===============
> +
> +In order for a patch to be checked in, in general, several conditions
> +must be met:

I think it is very helpful to write guidelines, but I am opposed to
declaring this as a rigid policy.

In particular as committer I often bend the rules (I guess, I mean,
insofar as we have rules, I do things that feel like bending them).
I do this when ISTM that all the "relevant" people either have
approved, or will approve of my action when they find out about it.

I may be wrong but I don't think I have ever caused anyone to feel
like I overstepped a boundary, by deliberately (for example)
committing something which seems to lack an appropriate ack.
(That's not to say that I don't make errors; but that is rather a
different question.)

At the very least, I am often very keen to commit things which unbreak
recent serious regressions, eg which unbreak the build.  I have even
on occasion done a substantive review of a patch, as part of
convincing myself that the maintainer will approve of it (or at least,
approve of my decision to commit it).

I'm not sure exactly how to codify this.  For me the key test is:
if I do this, is anyone going to be annoyed because they felt their
ack should have been waited for *and they wouldn't have granted it*;
ie, they felt the patch ought not to have been committed.  If that
were to happen I would have to apologise and recalibrate my
understanding of when such a thing is appropriate (and this might
indeed depend on which maintainer(s) were involved, etc.)

Obviously the best way to avoid such friction is to wait for the
explicit ack, and chase it if need be; but sometimes (not usually, but
occasionally) that is not practical for whatever reason.

Does this make sense ?

Regards,
Ian.

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

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

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH] MAINTAINERS: Add explicit check-in policy section"):
> On 5/8/19 12:59 PM, Juergen Gross wrote:
> > 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.
> 
> That changes the current meaning of "Designated Reviewer" somewhat.  At
> the moment, it only means "Send this person an email"; if we did that,
> it would mean, "This person is more trusted to do reviews than others".
> 
> Not sure what I think about that yet.

I think this is a bad idea.  The purpose was to provide a way for
someone to be emailed which doesn't involve giving them authority,
and which is therefore a status which can be granted on request and
does not require any assessment etc.

If we chane this we will have to invent a new status to perform this
function.

Ian.

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

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

[Stefano Stabellini]

On Wed, 8 May 2019, George Dunlap wrote:
> +	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.

Not that I am opposed to it, but this is not how Julien and I have been
working so far: when one of us sends a patch the other needs to review
it or at least ack it.

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

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

[Jan Beulich]

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

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

[George Dunlap]

On 5/8/19 12:59 PM, Juergen Gross wrote:
> 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.

That changes the current meaning of "Designated Reviewer" somewhat.  At
the moment, it only means "Send this person an email"; if we did that,
it would mean, "This person is more trusted to do reviews than others".

Not sure what I think about that yet.

>> +       The meaning of nesting
>> +       ======================
> 
> Everywhere else tabs are used for indenting.

I did try to get my editor to use tabs.  I'll try harder next time I
guess. :-/

 -George

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

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

[Juergen Gross]

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

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

[George Dunlap]

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.
---
 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
+       ======================
 
 Many maintainership areas are "nested": for example, there are entries
 for xen/arch/x86 as well as xen/arch/x86/mm, and even
@@ -113,11 +152,6 @@ the Ack of the xen/arch/x86/mm/shadow maintainer for that part of the
 patch, but would not require the Ack of the xen/arch/x86 maintainer or
 the xen/arch/x86/mm maintainer.
 
-(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.)
-
 2. In unusual circumstances, a more general maintainer's Ack can stand
 in for or even overrule a specific maintainer's Ack.  Unusual
 circumstances might include:
-- 
2.20.1


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