| Message ID | 20221208104924.76637-1-george.dunlap@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MAINTAINERS: Clarify check-in requirements for mixed-author patches | expand |
On Thu, Dec 8, 2022 at 10:49 AM George Dunlap <george.dunlap@cloud.com> wrote: > From: George Dunlap <george.dunlap@citrix.com> > > There was a question raised recently about the requriements for > checking in a patch which was originally written by one maintainer, > then picked up and modified by a second maintainer, and which they now both > agree should be checked in. > > It was proposed that in that case, the following set of tags would suffice: > > Signed-off-by: First Author <...> > Signed-off-by: Second Author <...> > Reviewed-by: First Author <...> > > The rationale was as follows: > > 1. The patch will be a mix of code, whose copyright is owned by the > various authors (or the companies they work for). It's important to > keep this information around in the event, for instance, of a license > change or something else requiring knowledge of the copyright owner. > > 2. The Signed-off-by of the Second Author approves not only their own > code, but First Author's code; the Reviewed-by of the First Author > approves not only their own code, but the Second Author's code. Thus > all the code has been approved by a maintainer, as well as someone who > was not the author. > > In support of this, several arguments were put forward: > > * We shouldn't make it harder for maintainers to get their code in > than for non-maintiners > > * The system we set up should not add pointless bureaucracy; nor > discourage collaboration; nor encourage contributors to get around > the rules by dropping important information. (For instance, by > removing the first SoB, so that the patch appears to have been > written entirely by Second Author.) > > Concerns were raised about two maintainers from the same company > colluding to get a patch in from their company; but such maintainers > could already collude, by working on the patch in secret, and posting > it publicly with only a single author's SoB, and having the other > person review it. > > There's also something slightly strange about adding "Reviewed-by" to > code that you've written; but in the end you're reviewing not only the > code itself, but the final arrangement of it. There's no need to > overcomplicate things. > > Encode this in MAINTAINERS as follows: > > * Refine the wording of requirement #2 in the check-in policy; such > that *each change* must have approval from someone other than *the > person who wrote it*. > > * Add a paragraph explicitly stating that the multiple-SoB-approval > system satisfies the requirements, and why. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > For clarity: As of the community call, we don't have consensus on this; so we should wait I think at least 2 weeks before checking it in to allow THE REST to object if they wish (unless we get Acks / R-b's from everyone, of course). -George
On 08.12.2022 11:49, George Dunlap wrote: > From: George Dunlap <george.dunlap@citrix.com> > > There was a question raised recently about the requriements for > checking in a patch which was originally written by one maintainer, > then picked up and modified by a second maintainer, and which they now both > agree should be checked in. > > It was proposed that in that case, the following set of tags would suffice: > > Signed-off-by: First Author <...> > Signed-off-by: Second Author <...> > Reviewed-by: First Author <...> > > The rationale was as follows: > > 1. The patch will be a mix of code, whose copyright is owned by the > various authors (or the companies they work for). It's important to > keep this information around in the event, for instance, of a license > change or something else requiring knowledge of the copyright owner. > > 2. The Signed-off-by of the Second Author approves not only their own > code, but First Author's code; The wording in ./MAINTAINERS looks good to me, so it may be benign that here a perhaps relevant (in the course of development) aspect is not expressed correctly: Second Author may have fixed a bug in the original patch, which surely he then doesn't approve. So I'd be inclined to suggest making this "..., but also the unchanged parts of First Author's code". > the Reviewed-by of the First Author > approves not only their own code, but the Second Author's code. Thus > all the code has been approved by a maintainer, as well as someone who > was not the author. > > In support of this, several arguments were put forward: > > * We shouldn't make it harder for maintainers to get their code in > than for non-maintiners > > * The system we set up should not add pointless bureaucracy; nor > discourage collaboration; nor encourage contributors to get around > the rules by dropping important information. (For instance, by > removing the first SoB, so that the patch appears to have been > written entirely by Second Author.) > > Concerns were raised about two maintainers from the same company > colluding to get a patch in from their company; but such maintainers > could already collude, by working on the patch in secret, and posting > it publicly with only a single author's SoB, and having the other > person review it. > > There's also something slightly strange about adding "Reviewed-by" to > code that you've written; but in the end you're reviewing not only the > code itself, but the final arrangement of it. There's no need to > overcomplicate things. > > Encode this in MAINTAINERS as follows: > > * Refine the wording of requirement #2 in the check-in policy; such > that *each change* must have approval from someone other than *the > person who wrote it*. > > * Add a paragraph explicitly stating that the multiple-SoB-approval > system satisfies the requirements, and why. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> Since the actual change to the policy looks good to me: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On Thu, Dec 8, 2022 at 11:34 AM Jan Beulich <jbeulich@suse.com> wrote: > On 08.12.2022 11:49, George Dunlap wrote: > > From: George Dunlap <george.dunlap@citrix.com> > > > > There was a question raised recently about the requriements for > > checking in a patch which was originally written by one maintainer, > > then picked up and modified by a second maintainer, and which they now > both > > agree should be checked in. > > > > It was proposed that in that case, the following set of tags would > suffice: > > > > Signed-off-by: First Author <...> > > Signed-off-by: Second Author <...> > > Reviewed-by: First Author <...> > > > > The rationale was as follows: > > > > 1. The patch will be a mix of code, whose copyright is owned by the > > various authors (or the companies they work for). It's important to > > keep this information around in the event, for instance, of a license > > change or something else requiring knowledge of the copyright owner. > > > > 2. The Signed-off-by of the Second Author approves not only their own > > code, but First Author's code; > > The wording in ./MAINTAINERS looks good to me, so it may be benign that > here a perhaps relevant (in the course of development) aspect is not > expressed correctly: Second Author may have fixed a bug in the original > patch, which surely he then doesn't approve. So I'd be inclined to > suggest making this "..., but also the unchanged parts of First Author's > code". > Given the wording in #1, "The patch will be a mix of code...", I think the context should be clear, that we're talking about the code *in the patch that's being submitted*, not some other code in some other patch that's been submitted previously. Since the actual change to the policy looks good to me: > Acked-by: Jan Beulich <jbeulich@suse.com> > Thanks, -George
On 08/12/2022 10:49, George Dunlap wrote: > Concerns were raised about two maintainers from the same company > colluding to get a patch in from their company; but such maintainers > could already collude, by working on the patch in secret, and posting > it publicly with only a single author's SoB, and having the other > person review it. I know this was how the concern was voices, but it was fairly bogus even as stated. "same company" or not has no bearing at all on two maintainers choosing to collude in secret. The mitigation to all of this is the fact that being a maintainer starts from having gained trust / reputation in the community, and comes with the responsibility to not violate that trust. Furthermore, there are mechanisms in place to deal with issues around said trust being violated. > There's also something slightly strange about adding "Reviewed-by" to > code that you've written; but in the end you're reviewing not only the > code itself, but the final arrangement of it. There's no need to > overcomplicate things. > > Encode this in MAINTAINERS as follows: > > * Refine the wording of requirement #2 in the check-in policy; such > that *each change* must have approval from someone other than *the > person who wrote it*. > > * Add a paragraph explicitly stating that the multiple-SoB-approval > system satisfies the requirements, and why. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On Thu, Dec 8, 2022 at 12:02 PM Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > On 08/12/2022 10:49, George Dunlap wrote: > > Concerns were raised about two maintainers from the same company > > colluding to get a patch in from their company; but such maintainers > > could already collude, by working on the patch in secret, and posting > > it publicly with only a single author's SoB, and having the other > > person review it. > > I know this was how the concern was voices, but it was fairly bogus even > as stated. "same company" or not has no bearing at all on two > maintainers choosing to collude in secret. > > The mitigation to all of this is the fact that being a maintainer starts > from having gained trust / reputation in the community, and comes with > the responsibility to not violate that trust. Furthermore, there are > mechanisms in place to deal with issues around said trust being violated. > Right; this sort of "factionalism" might arise due to the shared perspective of being at the same company; but it might just as well arise from something else. For instance, you could imagine pro-"server" maintainers at different companies colluding to get a patch in that they knew was objected to by the part of the community focused on embedded; or pro-"automotive" maintainers at different companies colluding to get a patch in that was objected to by the part of the community focusing on "true embedded" use cases. Each of these hypothetical cases are covered by requirements #3 and #4: that "sufficient time" must be given to respond, and that there be no open objections. We have mechanisms in place to address whether such requirements were violated. If you think it's important to have that in the commit message, I can try to re-send it with words to that effect. Personally I think it's fine as it is. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks. -George
Hi George, On 08/12/2022 10:49, George Dunlap wrote: > From: George Dunlap <george.dunlap@citrix.com> > > There was a question raised recently about the requriements for Typo: s/requriements/requirements/ > checking in a patch which was originally written by one maintainer, > then picked up and modified by a second maintainer, and which they now both > agree should be checked in. > > It was proposed that in that case, the following set of tags would suffice: > > Signed-off-by: First Author <...> > Signed-off-by: Second Author <...> > Reviewed-by: First Author <...> > > The rationale was as follows: > > 1. The patch will be a mix of code, whose copyright is owned by the > various authors (or the companies they work for). It's important to > keep this information around in the event, for instance, of a license > change or something else requiring knowledge of the copyright owner. > > 2. The Signed-off-by of the Second Author approves not only their own > code, but First Author's code; the Reviewed-by of the First Author > approves not only their own code, but the Second Author's code. Thus > all the code has been approved by a maintainer, as well as someone who > was not the author. > > In support of this, several arguments were put forward: > > * We shouldn't make it harder for maintainers to get their code in > than for non-maintiners Typo: s/non-maintiners/maintainers/ > > * The system we set up should not add pointless bureaucracy; nor > discourage collaboration; nor encourage contributors to get around > the rules by dropping important information. (For instance, by > removing the first SoB, so that the patch appears to have been > written entirely by Second Author.) > > Concerns were raised about two maintainers from the same company > colluding to get a patch in from their company; but such maintainers > could already collude, by working on the patch in secret, and posting > it publicly with only a single author's SoB, and having the other > person review it. > > There's also something slightly strange about adding "Reviewed-by" to > code that you've written; but in the end you're reviewing not only the > code itself, but the final arrangement of it. There's no need to > overcomplicate things. > > Encode this in MAINTAINERS as follows: > > * Refine the wording of requirement #2 in the check-in policy; such > that *each change* must have approval from someone other than *the > person who wrote it*. > > * Add a paragraph explicitly stating that the multiple-SoB-approval > system satisfies the requirements, and why. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> Acked-by: Julien Grall <jgrall@amazon.com> Somewhat unrelated. I see you switched to you @cloud.com e-mails. Are the @citrix.com to work in the future? If not, then you (and other from citrix) may want to send an e-mail to update MAINTAINERS accordingly. Cheers,
On Thu, Dec 8, 2022 at 1:58 PM Julien Grall <julien@xen.org> wrote: > Hi George, > > On 08/12/2022 10:49, George Dunlap wrote: > > From: George Dunlap <george.dunlap@citrix.com> > > > > There was a question raised recently about the requriements for > > Typo: s/requriements/requirements/ ... > Typo: s/non-maintiners/maintainers/ > Thanks, I've changed these locally. > Acked-by: Julien Grall <jgrall@amazon.com> > Great, thanks. > > Somewhat unrelated. I see you switched to you @cloud.com e-mails. Are > the @citrix.com to work in the future? If not, then you (and other from > citrix) may want to send an e-mail to update MAINTAINERS accordingly. Yes, I think all the XenServer people will do that at some point, once some things have settled down. -George
On Thu, Dec 8, 2022 at 2:26 PM George Dunlap <george.dunlap@cloud.com> wrote: > > > On Thu, Dec 8, 2022 at 1:58 PM Julien Grall <julien@xen.org> wrote: > >> Hi George, >> >> On 08/12/2022 10:49, George Dunlap wrote: >> > From: George Dunlap <george.dunlap@citrix.com> >> > >> > There was a question raised recently about the requriements for >> >> Typo: s/requriements/requirements/ > > ... > >> Typo: s/non-maintiners/maintainers/ >> > > Thanks, I've changed these locally. > > >> Acked-by: Julien Grall <jgrall@amazon.com> >> > > Great, thanks. > This has now been checked in. -George
diff --git a/MAINTAINERS b/MAINTAINERS index 175f10f33f..0e5eba2312 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -120,8 +120,8 @@ must be met: See below for rules on nested maintainership. -2. It must have appropriate approval from someone other than the - submitter. This can be either: +2. Each change must have appropriate approval from someone other than + the person who wrote it. This can be either: a. An Acked-by from a maintainer of the code being touched (a co-maintainer if available, or a more general level maintainer if @@ -150,6 +150,20 @@ 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. +In the case where two people collaborate on a patch, at least one of +whom is a maintainer -- typically where one maintainer will do an +early version of the patch, and another maintainer will pick it up and +revise it -- there should be two Signed-off-by's and one Acked-by or +Reviewed-by; with the maintainer who did the most recent change +sending the patch, and an Acked-by or Reviewed-by coming from the +maintainer who did not most recently edit the patch. This satisfies +the requirement #2 because a) the Signed-off-by of the sender approves +the final version of the patch; including all parts of the patch that +the sender did not write b) the Reviewed-by approves the final version +of the patch, including all patches that the reviewer did not write. +Thus all code in the patch has been approved by someone who did not +write it. + Maintainers may choose to override non-maintainer objections in the case that consensus can't be reached.