[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: George Dunlap <george.dunlap@xxxxxxxxxx>
  • Date: Tue, 7 Jan 2020 16:17:36 +0000
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=george.dunlap@xxxxxxxxxx; spf=Pass smtp.mailfrom=George.Dunlap@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Autocrypt: addr=george.dunlap@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFPqG+MBEACwPYTQpHepyshcufo0dVmqxDo917iWPslB8lauFxVf4WZtGvQSsKStHJSj 92Qkxp4CH2DwudI8qpVbnWCXsZxodDWac9c3PordLwz5/XL41LevEoM3NWRm5TNgJ3ckPA+J K5OfSK04QtmwSHFP3G/SXDJpGs+oDJgASta2AOl9vPV+t3xG6xyfa2NMGn9wmEvvVMD44Z7R W3RhZPn/NEZ5gaJhIUMgTChGwwWDOX0YPY19vcy5fT4bTIxvoZsLOkLSGoZb/jHIzkAAznug Q7PPeZJ1kXpbW9EHHaUHiCD9C87dMyty0N3TmWfp0VvBCaw32yFtM9jUgB7UVneoZUMUKeHA fgIXhJ7I7JFmw3J0PjGLxCLHf2Q5JOD8jeEXpdxugqF7B/fWYYmyIgwKutiGZeoPhl9c/7RE Bf6f9Qv4AtQoJwtLw6+5pDXsTD5q/GwhPjt7ohF7aQZTMMHhZuS52/izKhDzIufl6uiqUBge 0lqG+/ViLKwCkxHDREuSUTtfjRc9/AoAt2V2HOfgKORSCjFC1eI0+8UMxlfdq2z1AAchinU0 eSkRpX2An3CPEjgGFmu2Je4a/R/Kd6nGU8AFaE8ta0oq5BSFDRYdcKchw4TSxetkG6iUtqOO ZFS7VAdF00eqFJNQpi6IUQryhnrOByw+zSobqlOPUO7XC5fjnwARAQABtCRHZW9yZ2UgVy4g RHVubGFwIDxkdW5sYXBnQHVtaWNoLmVkdT6JAlcEEwEKAEECGwMFCwkIBwMFFQoJCAsFFgID AQACHgECF4ACGQEWIQTXqBy2bTNXPzpOYFimNjwxBZC0bQUCXEowWQUJDCJ7dgAKCRCmNjwx BZC0beKvEACJ75YlJXd7TnNHgFyiCJkm/qPeoQ3sFGSDZuZh7SKcdt9+3V2bFEb0Mii1hQaz 3hRqZb8sYPHJrGP0ljK09k3wf8k3OuNxziLQBJyzvn7WNlE4wBEcy/Ejo9TVBdA4ph5D0YaZ nqdsPmxe/xlTFuSkgu4ep1v9dfVP1TQR0e+JIBa/Ss+cKC5intKm+8JxpOploAHuzaPu0L/X FapzsIXqgT9eIQeBEgO2hge6h9Jov3WeED/vh8kA7f8c6zQ/gs5E7VGALwsiLrhr0LZFcKcw kI3oCCrB/C/wyPZv789Ra8EXbeRSJmTjcnBwHRPjnjwQmetRDD1t+VyrkC6uujT5jmgOBzaj KCqZ8PcMAssOzdzQtKmjUQ2b3ICPs2X13xZ5M5/OVs1W3TG5gkvMh4YoHi4ilFnOk+v3/j7q 65FG6N0JLb94Ndi80HkIOQQ1XVGTyu6bUPaBg3rWK91Csp1682kD/dNVF3FKHrRLmSVtmEQR 5rK0+VGc/FmR6vd4haKGWIRuPxzg+pBR77avIZpU7C7+UXGuZ5CbHwIdY8LojJg2TuUdqaVj yxmEZLOA8rVHipCGrslRNthVbJrGN/pqtKjCClFZHIAYJQ9EGLHXLG9Pj76opfjHij3MpR3o pCGAh6KsCrfrsvjnpDwqSbngGyEVH030irSk4SwIqZ7FwLkBDQRUWmc6AQgAzpc8Ng5Opbrh iZrn69Xr3js28p+b4a+0BOvC48NfrNovZw4eFeKIzmI/t6EkJkSqBIxobWRpBkwGweENsqnd 0qigmsDw4N7J9Xx0h9ARDqiWxX4jr7u9xauI+CRJ1rBNO3VV30QdACwQ4LqhR/WA+IjdhyMH wj3EJGE61NdP/h0zfaLYAbvEg47/TPThFsm4m8Rd6bX7RkrrOgBbL/AOnYOMEivyfZZKX1vv iEemAvLfdk2lZt7Vm6X/fbKbV8tPUuZELzNedJvTTBS3/l1FVz9OUcLDeWhGEdlxqXH0sYWh E9+PXTAfz5JxKH+LMetwEM8DbuOoDIpmIGZKrZ+2fQARAQABiQNbBBgBCgAmAhsCFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKMJ4FCQnQ/OQBKcBdIAQZAQoABgUCVFpnOgAKCRCyFcen x4Qb7cXrCAC0qQeEWmLa9oEAPa+5U6wvG1t/mi22gZN6uzQXH1faIOoDehr7PPESE6tuR/vI CTTnaSrd4UDPNeqOqVF07YexWD1LDcQG6PnRqC5DIX1RGE3BaSaMl2pFJP8y+chews11yP8G DBbxaIsTcHZI1iVIC9XLhoeegWi84vYc8F4ziADVfowbmbvcVw11gE8tmALCwTeBeZVteXjh 0OELHwrc1/4j4yvENjIXRO+QLIgk43kB57Upr4tP2MEcs0odgPM+Q+oETOJ00xzLgkTnLPim C1FIW2bOZdTj+Uq6ezRS2LKsNmW+PRRvNyA5ojEbA/faxmAjMZtLdSSSeFK8y4SoCRCmNjwx BZC0bevWEACRu+GyQgrdGmorUptniIeO1jQlpTiP5WpVnk9Oe8SiLoXUhXXNj6EtzyLGpYmf kEAbki+S6WAKnzZd3shL58AuMyDxtFNNjNeKJOcl6FL7JPBIIgIp3wR401Ep+/s5pl3Nw8Ii 157f0T7o8CPb54w6S1WsMkU78WzTxIs/1lLblSMcvyz1Jq64g4OqiWI85JfkzPLlloVf1rzy ebIBLrrmjhCE2tL1RONpE/KRVb+Q+PIs5+YcZ+Q1e0vXWA7NhTWFbWx3+N6WW6gaGpbFbopo FkYRpj+2TA5cX5zW148/xU5/ATEb5vdUkFLUFVy5YNUSyeBHuaf6fGmBrDc47rQjAOt1rmyD 56MUBHpLUbvA6NkPezb7T6bQpupyzGRkMUmSwHiLyQNJQhVe+9NiJJvtEE3jol0JVJoQ9WVn FAzPNCgHQyvbsIF3gYkCYKI0w8EhEoH5FHYLoKS6Jg880IY5rXzoAEfPvLXegy6mhYl+mNVN QUBD4h9XtOvcdzR559lZuC0Ksy7Xqw3BMolmKsRO3gWKhXSna3zKl4UuheyZtubVWoNWP/bn vbyiYnLwuiKDfNAinEWERC8nPKlv3PkZw5d3t46F1Dx0TMf16NmP+azsRpnMZyzpY8BL2eur feSGAOB9qjZNyzbo5nEKHldKWCKE7Ye0EPEjECS1gjKDwbkBDQRUWrq9AQgA7aJ0i1pQSmUR 6ZXZD2YEDxia2ByR0uZoTS7N0NYv1OjU8v6p017u0Fco5+Qoju/fZ97ScHhp5xGVAk5kxZBF DT4ovJd0nIeSr3bbWwfNzGx1waztfdzXt6n3MBKr7AhioB1m+vuk31redUdnhbtvN7O40MC+ fgSk5/+jRGxY3IOVPooQKzUO7M51GoOg4wl9ia3H2EzOoGhN2vpTbT8qCcL92ZZZwkBRldoA Wn7c1hEKSTuT3f1VpSmhjnX0J4uvKZ1V2R7rooKJYFBcySC0wa8aTmAtAvLgfcpe+legOtgq DKzLuN45xzEjyjCiI521t8zxNMPJY9FiCPNv0sCkDwARAQABiQI8BBgBCgAmAhsMFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKNJYFCQnQrVkACgkQpjY8MQWQtG2Xxg//RrRP+PFYuNXt 9C5hec/JoY24TkGPPd2tMC9usWZVImIk7VlHlAeqHeE0lWU0LRGIvOBITbS9izw6fOVQBvCA Fni56S12fKLusWgWhgu03toT9ZGxZ9W22yfw5uThSHQ4y09wRWAIYvhJsKnPGGC2KDxFvtz5 4pYYNe8Icy4bwsxcgbaSFaRh+mYtts6wE9VzyJvyfTqbe8VrvE+3InG5rrlNn51AO6M4Wv20 iFEgYanJXfhicl0WCQrHyTLfdB5p1w+072CL8uryHQVfD0FcDe+J/wl3bmYze+aD1SlPzFoI MaSIXKejC6oh6DAT4rvU8kMAbX90T834Mvbc3jplaWorNJEwjAH/r+v877AI9Vsmptis+rni JwUissjRbcdlkKBisoUZRPmxQeUifxUpqgulZcYwbEC/a49+WvbaYUriaDLHzg9xisijHwD2 yWV8igBeg+cmwnk0mPz8tIVvwi4lICAgXob7HZiaqKnwaDXs4LiS4vdG5s/ElnE3rIc87yru 24n3ypeDZ6f5LkdqL1UNp5/0Aqbr3EiN7/ina4YVyscy9754l944kyHnnMRLVykg0v+kakj0 h0RJ5LbfLAMM8M52KIA3y14g0Fb7kHLcOUMVcgfQ3PrN6chtC+5l6ouDIlSLR3toxH8Aam7E rIFfe2Dk+lD9A9BVd2rfoHA=
  • Cc: Lars Kurth <lars.kurth@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Wilk <konrad.wilk@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 07 Jan 2020 16:28:37 +0000
  • Ironport-sdr: /VwES3pay9RNiujiC1pB3KnPTgRWkuvVxtV610qmAX82VZ0Yej9R42vfDuzAE6AFumtgvOPmbN 3Fc14iBJPvlsUd0vOHfNh1Y4WcFmomA38/1QWHGvqOOs82gHURL9MQsBblsnUyRbugftOdF+4O HxQAbNLJoez//pkcYPdi+t4UipkLuCAKad/0zfBXfWGXtDkaf11qKE0nN2mrOt6EcyGFBNTnFa Jz0bkgrfZZs7R7YCXklSz7RzpL3tJihfWv/8sxCIlr/KW7GaL5Pr1o6FwMtESqQwiiofSytM8/ ghs=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.