| Message ID | 20240516162230.937047-2-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | docs: define policy forbidding use of "AI" / LLM code generators | expand |
On Thu, 16 May 2024 at 17:22, Daniel P. Berrangé <berrange@redhat.com> wrote: > > Currently we have a short paragraph saying that patches must include > a Signed-off-by line, and merely link to the kernel documentation. > The linked kernel docs have a lot of content beyond the part about > sign-off an thus are misleading/distracting to QEMU contributors. Thanks for this -- I've felt for ages that it was a bit awkward that we didn't have a good place to link people to for the fuller explanation of this. > This introduces a dedicated 'code-provenance' page in QEMU talking > about why we require sign-off, explaining the other tags we commonly > use, and what to do in some edge cases. The version of the kernel SubmittingPatches we used to link to includes the text "sorry, no pseudonyms or anonymous contributions". This new documentation doesn't say anything either way about our approach to pseudonyms. I think we should probably say something, but I don't know if we have an in-practice consensus there, so maybe we should approach that as a separate change on top of this patch. So for this patch: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Thu, May 16, 2024 at 05:22:28PM +0100, Daniel P. Berrangé wrote: > Currently we have a short paragraph saying that patches must include > a Signed-off-by line, and merely link to the kernel documentation. > The linked kernel docs have a lot of content beyond the part about > sign-off an thus are misleading/distracting to QEMU contributors. > > This introduces a dedicated 'code-provenance' page in QEMU talking > about why we require sign-off, explaining the other tags we commonly > use, and what to do in some edge cases. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > docs/devel/code-provenance.rst | 212 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 19 +-- > 3 files changed, 215 insertions(+), 17 deletions(-) > create mode 100644 docs/devel/code-provenance.rst > > diff --git a/docs/devel/code-provenance.rst b/docs/devel/code-provenance.rst > new file mode 100644 > index 0000000000..7c42fae571 > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,212 @@ > +.. _code-provenance: > + > +Code provenance > +=============== > + > +Certifying patch submissions > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +The QEMU community **mandates** all contributors to certify provenance of > +patch submissions they make to the project. To put it another way, > +contributors must indicate that they are legally permitted to contribute to > +the project. > + > +Certification is achieved with a low overhead by adding a single line to the > +bottom of every git commit:: > + > + Signed-off-by: YOUR NAME <YOUR@EMAIL> > + > +The addition of this line asserts that the author of the patch is contributing > +in accordance with the clauses specified in the > +`Developer's Certificate of Origin <https://developercertificate.org>`__: Why are you linking to this one? It's slightly different from kernel, with copyright and prohibition to change it. there's also a bit more text in the kernel, e.g. the rule against anonymous contributions. > +.. _dco: > + > +:: > + Developer's Certificate of Origin 1.1 > + By making a contribution to this project, I certify that: > + > + (a) The contribution was created in whole or in part by me and I > + have the right to submit it under the open source license > + indicated in the file; or > + > + (b) The contribution is based upon previous work that, to the best > + of my knowledge, is covered under an appropriate open source > + license and I have the right under that license to submit that > + work with modifications, whether created in whole or in part > + by me, under the same open source license (unless I am > + permitted to submit under a different license), as indicated > + in the file; or > + > + (c) The contribution was provided directly to me by some other > + person who certified (a), (b) or (c) and I have not modified > + it. > + > + (d) I understand and agree that this project and the contribution > + are public and that a record of the contribution (including all > + personal information I submit with it, including my sign-off) is > + maintained indefinitely and may be redistributed consistent with > + this project or the open source license(s) involved. > + > +It is generally expected that the name and email addresses used in one of the > +``Signed-off-by`` lines, matches that of the git commit ``Author`` field. > + > +If the person sending the mail is not one of the patch authors, they are none > +the less expected to add their own ``Signed-off-by`` to comply with the DCO > +clause (c). > + > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple authors. In > +this scenario, git commits will usually be expected to have a ``Signed-off-by`` > +line for each contributor involved in creation of the patch. Some edge cases: > + > + * The non-primary author's contributions were so trivial that they can be > + considered not subject to copyright. In this case the secondary authors > + need not include a ``Signed-off-by``. > + > + This case most commonly applies where QEMU reviewers give short snippets > + of code as suggested fixes to a patch. The reviewers don't need to have > + their own ``Signed-off-by`` added unless their code suggestion was > + unusually large, but it is common to add ``Suggested-by`` as a credit > + for non-trivial code. > + > + * Both contributors work for the same employer and the employer requires > + copyright assignment. > + > + It can be said that in this case a ``Signed-off-by`` is indicating that > + the person has permission to contribute from their employer who is the > + copyright holder. It is none the less still preferable to include a > + ``Signed-off-by`` for each contributor, as in some countries employees are > + not able to assign copyright to their employer, and it also covers any > + time invested outside working hours. > + > +When multiple ``Signed-off-by`` tags are present, they should be strictly kept > +in order of authorship, from oldest to newest. > + > +Other commit tags > +~~~~~~~~~~~~~~~~~ > + > +While the ``Signed-off-by`` tag is mandatory, there are a number of other tags > +that are commonly used during QEMU development: > + > + * **``Reviewed-by``**: when a QEMU community member reviews a patch on the > + mailing list, if they consider the patch acceptable, they should send an > + email reply containing a ``Reviewed-by`` tag. Subsystem maintainers who > + review a patch should add this even if they are also adding their > + ``Signed-off-by`` to the same commit. > + > + * **``Acked-by``**: when a QEMU subsystem maintainer approves a patch that > + touches their subsystem, but intends to allow a different maintainer to > + queue it and send a pull request, they would send a mail containing a > + ``Acked-by`` tag. Where a patch touches multiple subsystems, ``Acked-by`` > + only implies review of the maintainers' own areas of responsibility. If a > + maintainer wants to indicate they have done a full review they should use > + a ``Reviewed-by`` tag. > + > + * **``Tested-by``**: when a QEMU community member has functionally tested the > + behaviour of the patch in some manner, they should send an email reply > + containing a ``Tested-by`` tag. > + > + * **``Reported-by``**: when a QEMU community member reports a problem via the > + mailing list, or some other informal channel that is not the issue tracker, > + it is good practice to credit them by including a ``Reported-by`` tag on > + any patch fixing the issue. When the problem is reported via the GitLab > + issue tracker, however, it is sufficient to just include a link to the > + issue. > + > + * **``Suggested-by``**: when a reviewer or other 3rd party makes non-trivial > + suggestions for how to change a patch, it is good practice to credit them > + by including a ``Suggested-by`` tag. > + > +Subsystem maintainer requirements > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +When a subsystem maintainer accepts a patch from a contributor, in addition to > +the normal code review points, they are expected to validate the presence of > +suitable ``Signed-off-by`` tags. > + > +At the time they queue the patch in their subsystem tree, the maintainer > +**must** also then add their own ``Signed-off-by`` to indicate that they have > +done the aforementioned validation. This is in addition to any of their own > +``Reviewed-by`` tags the subsystem maintainer may wish to include. > + > +Tools for adding ``Signed-off-by`` > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +There are a variety of ways tools can support adding ``Signed-off-by`` tags > +for patches, avoiding the need for contributors to manually type in this > +repetitive text each time. > + > +git commands > +^^^^^^^^^^^^ > + > +When creating, or amending, a commit the ``-s`` flag to ``git commit`` will > +append a suitable line matching the configuring git author details. > + > +If preparing patches using the ``git format-patch`` tool, the ``-s`` flag can > +be used to append a suitable line in the emails it creates, without modifying > +the local commits. Alternatively to modify all the local commits on a branch:: > + > + git rebase master -x 'git commit --amend --no-edit -s' > + > +emacs > +^^^^^ > + > +In the file ``$HOME/.emacs.d/abbrev_defs`` add:: > + > + (define-abbrev-table 'global-abbrev-table > + '( > + ("8rev" "Reviewed-by: YOUR NAME <your@email.addr>" nil 1) > + ("8ack" "Acked-by: YOUR NAME <your@email.addr>" nil 1) > + ("8test" "Tested-by: YOUR NAME <your@email.addr>" nil 1) > + ("8sob" "Signed-off-by: YOUR NAME <your@email.addr>" nil 1) > + )) > + > +with this change, if you type (for example) ``8rev`` followed by ``<space>`` > +or ``<enter>`` it will expand to the whole phrase. > + > +vim > +^^^ > + > +In the file ``$HOME/.vimrc`` add:: > + > + iabbrev 8rev Reviewed-by: YOUR NAME <your@email.addr> > + iabbrev 8ack Acked-by: YOUR NAME <your@email.addr> > + iabbrev 8test Tested-by: YOUR NAME <your@email.addr> > + iabbrev 8sob Signed-off-by: YOUR NAME <your@email.addr> > + > +with this change, if you type (for example) ``8rev`` followed by ``<space>`` > +or ``<enter>`` it will expand to the whole phrase. > + > +Re-starting abandoned work > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +For a variety of reasons there are some patches that get submitted to QEMU but > +never merged. An unrelated contributor may decide (months or years later) to > +continue working from the abandoned patch and re-submit it with extra changes. > + > +The general principles when picking up abandoned work are: > + > + * Continue to credit the original author for their work, by maintaining their > + original ``Signed-off-by`` > + * Indicate where the original patch was obtained from (mailing list, bug > + tracker, author's git repo, etc) when sending it for review > + * Acknowledge the extra work of the new contributor by including their > + ``Signed-off-by`` in the patch in addition to the orignal author's > + * Indicate who is responsible for what parts of the patch. This is typically > + done via a note in the commit message, just prior to the new contributor's > + ``Signed-off-by``:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@mycorp.test> > + > +In complicated cases, or if otherwise unsure, ask for advice on the project > +mailing list. > + > +It is also recommended to attempt to contact the original author to let them > +know you are interested in taking over their work, in case they still intended > +to return to the work, or had any suggestions about the best way to continue. > diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst > index 362f97ee30..b54e58105e 100644 > --- a/docs/devel/index-process.rst > +++ b/docs/devel/index-process.rst > @@ -13,6 +13,7 @@ Notes about how to interact with the community and how and where to submit patch > maintainers > style > submitting-a-patch > + code-provenance > trivial-patches > stable-process > submitting-a-pull-request > diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst > index 83e9092b8c..2cc4d53ff6 100644 > --- a/docs/devel/submitting-a-patch.rst > +++ b/docs/devel/submitting-a-patch.rst > @@ -322,23 +322,8 @@ Patch emails must include a ``Signed-off-by:`` line > > Your patches **must** include a Signed-off-by: line. This is a hard > requirement because it's how you say "I'm legally okay to contribute > -this and happy for it to go into QEMU". The process is modelled after > -the `Linux kernel > -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ > -policy. > - > -If you wrote the patch, make sure your "From:" and "Signed-off-by:" > -lines use the same spelling. It's okay if you subscribe or contribute to > -the list via more than one address, but using multiple addresses in one > -commit just confuses things. I gather you no longer see value in discussing this use-case? Maybe mention in commit log, why. > If someone else wrote the patch, git will > -include a "From:" line in the body of the email (different from your > -envelope From:) that will give credit to the correct author; but again, > -that author's Signed-off-by: line is mandatory, with the same spelling. > - > -There are various tooling options for automatically adding these tags > -include using ``git commit -s`` or ``git format-patch -s``. For more > -information see `SubmittingPatches 1.12 > -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. > +this and happy for it to go into QEMU". For full guidance, read the > +:ref:`code-provenance` documentation. > > .. _include_a_meaningful_cover_letter: > > -- > 2.43.0
On Thu, May 16, 2024 at 06:29:39PM +0100, Peter Maydell wrote: > On Thu, 16 May 2024 at 17:22, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > Currently we have a short paragraph saying that patches must include > > a Signed-off-by line, and merely link to the kernel documentation. > > The linked kernel docs have a lot of content beyond the part about > > sign-off an thus are misleading/distracting to QEMU contributors. > > Thanks for this -- I've felt for ages that it was a bit awkward > that we didn't have a good place to link people to for the fuller > explanation of this. > > > This introduces a dedicated 'code-provenance' page in QEMU talking > > about why we require sign-off, explaining the other tags we commonly > > use, and what to do in some edge cases. > > The version of the kernel SubmittingPatches we used to link to > includes the text "sorry, no pseudonyms or anonymous contributions". > This new documentation doesn't say anything either way about > our approach to pseudonyms. I think we should probably say > something, but I don't know if we have an in-practice consensus > there, so maybe we should approach that as a separate change on > top of this patch. Well given we referred to kernel previously then I guess that's the concensus, no? > So for this patch: > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM
On Thu, 16 May 2024 at 18:34, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, May 16, 2024 at 06:29:39PM +0100, Peter Maydell wrote: > > On Thu, 16 May 2024 at 17:22, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > Currently we have a short paragraph saying that patches must include > > > a Signed-off-by line, and merely link to the kernel documentation. > > > The linked kernel docs have a lot of content beyond the part about > > > sign-off an thus are misleading/distracting to QEMU contributors. > > > > Thanks for this -- I've felt for ages that it was a bit awkward > > that we didn't have a good place to link people to for the fuller > > explanation of this. > > > > > This introduces a dedicated 'code-provenance' page in QEMU talking > > > about why we require sign-off, explaining the other tags we commonly > > > use, and what to do in some edge cases. > > > > The version of the kernel SubmittingPatches we used to link to > > includes the text "sorry, no pseudonyms or anonymous contributions". > > This new documentation doesn't say anything either way about > > our approach to pseudonyms. I think we should probably say > > something, but I don't know if we have an in-practice consensus > > there, so maybe we should approach that as a separate change on > > top of this patch. > > > Well given we referred to kernel previously then I guess that's > the concensus, no? AIUI the kernel devs have changed their point of view on the pseudonym question, so it's a question of whether we were deliberately referring to that specific revision of the kernel's practice because we agreed with it or just by chance... https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330 is where the kernel changed to saying merely "no anonymous contributions", dropping the 'pseudonyms' part. -- PMM
On 16/05/2024 19.43, Peter Maydell wrote: > On Thu, 16 May 2024 at 18:34, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Thu, May 16, 2024 at 06:29:39PM +0100, Peter Maydell wrote: >>> On Thu, 16 May 2024 at 17:22, Daniel P. Berrangé <berrange@redhat.com> wrote: >>>> >>>> Currently we have a short paragraph saying that patches must include >>>> a Signed-off-by line, and merely link to the kernel documentation. >>>> The linked kernel docs have a lot of content beyond the part about >>>> sign-off an thus are misleading/distracting to QEMU contributors. >>> >>> Thanks for this -- I've felt for ages that it was a bit awkward >>> that we didn't have a good place to link people to for the fuller >>> explanation of this. >>> >>>> This introduces a dedicated 'code-provenance' page in QEMU talking >>>> about why we require sign-off, explaining the other tags we commonly >>>> use, and what to do in some edge cases. >>> >>> The version of the kernel SubmittingPatches we used to link to >>> includes the text "sorry, no pseudonyms or anonymous contributions". >>> This new documentation doesn't say anything either way about >>> our approach to pseudonyms. I think we should probably say >>> something, but I don't know if we have an in-practice consensus >>> there, so maybe we should approach that as a separate change on >>> top of this patch. >> >> >> Well given we referred to kernel previously then I guess that's >> the concensus, no? > > AIUI the kernel devs have changed their point of view on the > pseudonym question, so it's a question of whether we were > deliberately referring to that specific revision of the kernel's > practice because we agreed with it or just by chance... > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330 > > is where the kernel changed to saying merely "no anonymous > contributions", dropping the 'pseudonyms' part. FWIW, we had a clear statement in our document in the past: https://gitlab.com/qemu-project/qemu/-/commit/ca127fe96ddb827f3ea153610c1e8f6e374708e2#9620a1442f724c9d8bfd5408e4611ba1839fcb8a_315_321 Quoting: "Please use your real name to sign a patch (not an alias or acronym)." But it got lost in that rework, I assume by accident? So IMHO we had a consensus once to not allow anonymous contributions. I'm in favor of adding such a sentence back here now. Thomas
On Fri, May 17, 2024 at 07:05:05AM +0200, Thomas Huth wrote: > On 16/05/2024 19.43, Peter Maydell wrote: > > On Thu, 16 May 2024 at 18:34, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Thu, May 16, 2024 at 06:29:39PM +0100, Peter Maydell wrote: > > > > On Thu, 16 May 2024 at 17:22, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > > > > > Currently we have a short paragraph saying that patches must include > > > > > a Signed-off-by line, and merely link to the kernel documentation. > > > > > The linked kernel docs have a lot of content beyond the part about > > > > > sign-off an thus are misleading/distracting to QEMU contributors. > > > > > > > > Thanks for this -- I've felt for ages that it was a bit awkward > > > > that we didn't have a good place to link people to for the fuller > > > > explanation of this. > > > > > > > > > This introduces a dedicated 'code-provenance' page in QEMU talking > > > > > about why we require sign-off, explaining the other tags we commonly > > > > > use, and what to do in some edge cases. > > > > > > > > The version of the kernel SubmittingPatches we used to link to > > > > includes the text "sorry, no pseudonyms or anonymous contributions". > > > > This new documentation doesn't say anything either way about > > > > our approach to pseudonyms. I think we should probably say > > > > something, but I don't know if we have an in-practice consensus > > > > there, so maybe we should approach that as a separate change on > > > > top of this patch. > > > > > > > > > Well given we referred to kernel previously then I guess that's > > > the concensus, no? > > > > AIUI the kernel devs have changed their point of view on the > > pseudonym question, so it's a question of whether we were > > deliberately referring to that specific revision of the kernel's > > practice because we agreed with it or just by chance... > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330 > > > > is where the kernel changed to saying merely "no anonymous > > contributions", dropping the 'pseudonyms' part. > > FWIW, we had a clear statement in our document in the past: > > https://gitlab.com/qemu-project/qemu/-/commit/ca127fe96ddb827f3ea153610c1e8f6e374708e2#9620a1442f724c9d8bfd5408e4611ba1839fcb8a_315_321 > > Quoting: "Please use your real name to sign a patch (not an alias or acronym)." > > But it got lost in that rework, I assume by accident? Yeah, probably an oversight. > So IMHO we had a consensus once to not allow anonymous contributions. I'm in > favor of adding such a sentence back here now. That text has been in the submitting-a-patch file since day 1, but that content was originally a copy of the old wiki page, and the wiki edits never had any formal peer review, so we should be wary of claiming too much about a consensus. Going back in history we can see the specific wording arrived with this change: https://wiki.qemu.org/index.php?title=Contribute%2FSubmitAPatch&type=revision&diff=2173&oldid=2094 This may have been an informally held opinion amongst at least some of those in the community at the time, but don't recall there was a specific debate about the allowance of psuedonyms, etc. I have traditionally been in favour of requiring real names, which I had pretty much interpreted to imply a person's legal name. That was mostly because I was following what I (apparently incorrectly) thought was the kernel's intent in this respect. Looking at the kernel commit above, I have sympathy with the view that interpreting "real name" too strictly as a "legal name" is exclusionary. Thus I'd be in favour of following the kernels' clarified intent, which broadly aligns with the CNCF explanatory text, that "real name" can be loosely interpreted to be "a commonly known identity in the community". With regards, Daniel
On Thu, May 16, 2024 at 01:33:01PM -0400, Michael S. Tsirkin wrote: > On Thu, May 16, 2024 at 05:22:28PM +0100, Daniel P. Berrangé wrote: > > Currently we have a short paragraph saying that patches must include > > a Signed-off-by line, and merely link to the kernel documentation. > > The linked kernel docs have a lot of content beyond the part about > > sign-off an thus are misleading/distracting to QEMU contributors. > > > > This introduces a dedicated 'code-provenance' page in QEMU talking > > about why we require sign-off, explaining the other tags we commonly > > use, and what to do in some edge cases. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > docs/devel/code-provenance.rst | 212 ++++++++++++++++++++++++++++++ > > docs/devel/index-process.rst | 1 + > > docs/devel/submitting-a-patch.rst | 19 +-- > > 3 files changed, 215 insertions(+), 17 deletions(-) > > create mode 100644 docs/devel/code-provenance.rst > > > > diff --git a/docs/devel/code-provenance.rst b/docs/devel/code-provenance.rst > > new file mode 100644 > > index 0000000000..7c42fae571 > > --- /dev/null > > +++ b/docs/devel/code-provenance.rst > > @@ -0,0 +1,212 @@ > > +.. _code-provenance: > > + > > +Code provenance > > +=============== > > + > > +Certifying patch submissions > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > + > > +The QEMU community **mandates** all contributors to certify provenance of > > +patch submissions they make to the project. To put it another way, > > +contributors must indicate that they are legally permitted to contribute to > > +the project. > > + > > +Certification is achieved with a low overhead by adding a single line to the > > +bottom of every git commit:: > > + > > + Signed-off-by: YOUR NAME <YOUR@EMAIL> > > + > > +The addition of this line asserts that the author of the patch is contributing > > +in accordance with the clauses specified in the > > +`Developer's Certificate of Origin <https://developercertificate.org>`__: > > Why are you linking to this one? The kernel doesn't have a standalone copy of the text, it is just inline in the middle of their huge SubmittingPatches document. We don't want to mislead people into thinking we're following the kernel's patch submision rules in general, instead define our own clear policy. > It's slightly different from kernel, with copyright and prohibition to change it. That difference is not of any consequence. The probhition aganist changing makes sense, to protect the value of the "Developer Certificate of Origin" term to have a fixed meaning. The 4 clauses that you must certify against are all identical to the kernel's copy, which is what matters. > there's also a bit more text in the kernel, e.g. the rule against > anonymous contributions. Yes, we should clarify our intent in this respect, per the other part of this thread around what we interpret "real name" to mean for QEMU. > > diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst > > index 83e9092b8c..2cc4d53ff6 100644 > > --- a/docs/devel/submitting-a-patch.rst > > +++ b/docs/devel/submitting-a-patch.rst > > @@ -322,23 +322,8 @@ Patch emails must include a ``Signed-off-by:`` line > > > > Your patches **must** include a Signed-off-by: line. This is a hard > > requirement because it's how you say "I'm legally okay to contribute > > -this and happy for it to go into QEMU". The process is modelled after > > -the `Linux kernel > > -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ > > -policy. > > - > > -If you wrote the patch, make sure your "From:" and "Signed-off-by:" > > -lines use the same spelling. It's okay if you subscribe or contribute to > > -the list via more than one address, but using multiple addresses in one > > -commit just confuses things. > > > I gather you no longer see value in discussing this use-case? > Maybe mention in commit log, why. I should have preserved this phrase in the new doc. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > Currently we have a short paragraph saying that patches must include > a Signed-off-by line, and merely link to the kernel documentation. > The linked kernel docs have a lot of content beyond the part about > sign-off an thus are misleading/distracting to QEMU contributors. > > This introduces a dedicated 'code-provenance' page in QEMU talking > about why we require sign-off, explaining the other tags we commonly > use, and what to do in some edge cases. > <snip> > + > +Other commit tags > +~~~~~~~~~~~~~~~~~ > + > +While the ``Signed-off-by`` tag is mandatory, there are a number of other tags > +that are commonly used during QEMU development: > + > + * **``Reviewed-by``**: when a QEMU community member reviews a patch on the > + mailing list, if they consider the patch acceptable, they should send an > + email reply containing a ``Reviewed-by`` tag. Subsystem maintainers who > + review a patch should add this even if they are also adding their > + ``Signed-off-by`` to the same commit. > + > + * **``Acked-by``**: when a QEMU subsystem maintainer approves a patch that > + touches their subsystem, but intends to allow a different maintainer to > + queue it and send a pull request, they would send a mail containing a > + ``Acked-by`` tag. Where a patch touches multiple subsystems, ``Acked-by`` > + only implies review of the maintainers' own areas of responsibility. If a > + maintainer wants to indicate they have done a full review they should use > + a ``Reviewed-by`` tag. > + > + * **``Tested-by``**: when a QEMU community member has functionally tested the > + behaviour of the patch in some manner, they should send an email reply > + containing a ``Tested-by`` tag. > + > + * **``Reported-by``**: when a QEMU community member reports a problem via the > + mailing list, or some other informal channel that is not the issue tracker, > + it is good practice to credit them by including a ``Reported-by`` tag on > + any patch fixing the issue. When the problem is reported via the GitLab > + issue tracker, however, it is sufficient to just include a link to the > + issue. > + > + * **``Suggested-by``**: when a reviewer or other 3rd party makes non-trivial > + suggestions for how to change a patch, it is good practice to credit them > + by including a ``Suggested-by`` tag. Should we mention our use of Message-Id in so far the informal good practice is that we keep the Message-Id's of the last time a patch was posted and potentially the message-ids of previous posters? But this is definitely an improvement of what we had before so: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > + > +Subsystem maintainer requirements > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +When a subsystem maintainer accepts a patch from a contributor, in addition to > +the normal code review points, they are expected to validate the presence of > +suitable ``Signed-off-by`` tags. > + > +At the time they queue the patch in their subsystem tree, the maintainer > +**must** also then add their own ``Signed-off-by`` to indicate that they have > +done the aforementioned validation. This is in addition to any of their own > +``Reviewed-by`` tags the subsystem maintainer may wish to include. > + > +Tools for adding ``Signed-off-by`` > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +There are a variety of ways tools can support adding ``Signed-off-by`` tags > +for patches, avoiding the need for contributors to manually type in this > +repetitive text each time. > + > +git commands > +^^^^^^^^^^^^ > + > +When creating, or amending, a commit the ``-s`` flag to ``git commit`` will > +append a suitable line matching the configuring git author details. > + > +If preparing patches using the ``git format-patch`` tool, the ``-s`` flag can > +be used to append a suitable line in the emails it creates, without modifying > +the local commits. Alternatively to modify all the local commits on a branch:: > + > + git rebase master -x 'git commit --amend --no-edit -s' > + > +emacs > +^^^^^ > + > +In the file ``$HOME/.emacs.d/abbrev_defs`` add:: > + > + (define-abbrev-table 'global-abbrev-table > + '( > + ("8rev" "Reviewed-by: YOUR NAME <your@email.addr>" nil 1) > + ("8ack" "Acked-by: YOUR NAME <your@email.addr>" nil 1) > + ("8test" "Tested-by: YOUR NAME <your@email.addr>" nil 1) > + ("8sob" "Signed-off-by: YOUR NAME <your@email.addr>" nil 1) > + )) > + > +with this change, if you type (for example) ``8rev`` followed by ``<space>`` > +or ``<enter>`` it will expand to the whole phrase. > + > +vim > +^^^ > + > +In the file ``$HOME/.vimrc`` add:: > + > + iabbrev 8rev Reviewed-by: YOUR NAME <your@email.addr> > + iabbrev 8ack Acked-by: YOUR NAME <your@email.addr> > + iabbrev 8test Tested-by: YOUR NAME <your@email.addr> > + iabbrev 8sob Signed-off-by: YOUR NAME <your@email.addr> > + > +with this change, if you type (for example) ``8rev`` followed by ``<space>`` > +or ``<enter>`` it will expand to the whole phrase. > + > +Re-starting abandoned work > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +For a variety of reasons there are some patches that get submitted to QEMU but > +never merged. An unrelated contributor may decide (months or years later) to > +continue working from the abandoned patch and re-submit it with extra changes. > + > +The general principles when picking up abandoned work are: > + > + * Continue to credit the original author for their work, by maintaining their > + original ``Signed-off-by`` > + * Indicate where the original patch was obtained from (mailing list, bug > + tracker, author's git repo, etc) when sending it for review > + * Acknowledge the extra work of the new contributor by including their > + ``Signed-off-by`` in the patch in addition to the orignal author's > + * Indicate who is responsible for what parts of the patch. This is typically > + done via a note in the commit message, just prior to the new contributor's > + ``Signed-off-by``:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@mycorp.test> > + > +In complicated cases, or if otherwise unsure, ask for advice on the project > +mailing list. > + > +It is also recommended to attempt to contact the original author to let them > +know you are interested in taking over their work, in case they still intended > +to return to the work, or had any suggestions about the best way to continue. > diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst > index 362f97ee30..b54e58105e 100644 > --- a/docs/devel/index-process.rst > +++ b/docs/devel/index-process.rst > @@ -13,6 +13,7 @@ Notes about how to interact with the community and how and where to submit patch > maintainers > style > submitting-a-patch > + code-provenance > trivial-patches > stable-process > submitting-a-pull-request > diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst > index 83e9092b8c..2cc4d53ff6 100644 > --- a/docs/devel/submitting-a-patch.rst > +++ b/docs/devel/submitting-a-patch.rst > @@ -322,23 +322,8 @@ Patch emails must include a ``Signed-off-by:`` line > > Your patches **must** include a Signed-off-by: line. This is a hard > requirement because it's how you say "I'm legally okay to contribute > -this and happy for it to go into QEMU". The process is modelled after > -the `Linux kernel > -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ > -policy. > - > -If you wrote the patch, make sure your "From:" and "Signed-off-by:" > -lines use the same spelling. It's okay if you subscribe or contribute to > -the list via more than one address, but using multiple addresses in one > -commit just confuses things. If someone else wrote the patch, git will > -include a "From:" line in the body of the email (different from your > -envelope From:) that will give credit to the correct author; but again, > -that author's Signed-off-by: line is mandatory, with the same spelling. > - > -There are various tooling options for automatically adding these tags > -include using ``git commit -s`` or ``git format-patch -s``. For more > -information see `SubmittingPatches 1.12 > -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. > +this and happy for it to go into QEMU". For full guidance, read the > +:ref:`code-provenance` documentation. > > .. _include_a_meaningful_cover_letter:
diff --git a/docs/devel/code-provenance.rst b/docs/devel/code-provenance.rst new file mode 100644 index 0000000000..7c42fae571 --- /dev/null +++ b/docs/devel/code-provenance.rst @@ -0,0 +1,212 @@ +.. _code-provenance: + +Code provenance +=============== + +Certifying patch submissions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The QEMU community **mandates** all contributors to certify provenance of +patch submissions they make to the project. To put it another way, +contributors must indicate that they are legally permitted to contribute to +the project. + +Certification is achieved with a low overhead by adding a single line to the +bottom of every git commit:: + + Signed-off-by: YOUR NAME <YOUR@EMAIL> + +The addition of this line asserts that the author of the patch is contributing +in accordance with the clauses specified in the +`Developer's Certificate of Origin <https://developercertificate.org>`__: + +.. _dco: + +:: + Developer's Certificate of Origin 1.1 + + By making a contribution to this project, I certify that: + + (a) The contribution was created in whole or in part by me and I + have the right to submit it under the open source license + indicated in the file; or + + (b) The contribution is based upon previous work that, to the best + of my knowledge, is covered under an appropriate open source + license and I have the right under that license to submit that + work with modifications, whether created in whole or in part + by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated + in the file; or + + (c) The contribution was provided directly to me by some other + person who certified (a), (b) or (c) and I have not modified + it. + + (d) I understand and agree that this project and the contribution + are public and that a record of the contribution (including all + personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with + this project or the open source license(s) involved. + +It is generally expected that the name and email addresses used in one of the +``Signed-off-by`` lines, matches that of the git commit ``Author`` field. + +If the person sending the mail is not one of the patch authors, they are none +the less expected to add their own ``Signed-off-by`` to comply with the DCO +clause (c). + +Multiple authorship +~~~~~~~~~~~~~~~~~~~ + +It is not uncommon for a patch to have contributions from multiple authors. In +this scenario, git commits will usually be expected to have a ``Signed-off-by`` +line for each contributor involved in creation of the patch. Some edge cases: + + * The non-primary author's contributions were so trivial that they can be + considered not subject to copyright. In this case the secondary authors + need not include a ``Signed-off-by``. + + This case most commonly applies where QEMU reviewers give short snippets + of code as suggested fixes to a patch. The reviewers don't need to have + their own ``Signed-off-by`` added unless their code suggestion was + unusually large, but it is common to add ``Suggested-by`` as a credit + for non-trivial code. + + * Both contributors work for the same employer and the employer requires + copyright assignment. + + It can be said that in this case a ``Signed-off-by`` is indicating that + the person has permission to contribute from their employer who is the + copyright holder. It is none the less still preferable to include a + ``Signed-off-by`` for each contributor, as in some countries employees are + not able to assign copyright to their employer, and it also covers any + time invested outside working hours. + +When multiple ``Signed-off-by`` tags are present, they should be strictly kept +in order of authorship, from oldest to newest. + +Other commit tags +~~~~~~~~~~~~~~~~~ + +While the ``Signed-off-by`` tag is mandatory, there are a number of other tags +that are commonly used during QEMU development: + + * **``Reviewed-by``**: when a QEMU community member reviews a patch on the + mailing list, if they consider the patch acceptable, they should send an + email reply containing a ``Reviewed-by`` tag. Subsystem maintainers who + review a patch should add this even if they are also adding their + ``Signed-off-by`` to the same commit. + + * **``Acked-by``**: when a QEMU subsystem maintainer approves a patch that + touches their subsystem, but intends to allow a different maintainer to + queue it and send a pull request, they would send a mail containing a + ``Acked-by`` tag. Where a patch touches multiple subsystems, ``Acked-by`` + only implies review of the maintainers' own areas of responsibility. If a + maintainer wants to indicate they have done a full review they should use + a ``Reviewed-by`` tag. + + * **``Tested-by``**: when a QEMU community member has functionally tested the + behaviour of the patch in some manner, they should send an email reply + containing a ``Tested-by`` tag. + + * **``Reported-by``**: when a QEMU community member reports a problem via the + mailing list, or some other informal channel that is not the issue tracker, + it is good practice to credit them by including a ``Reported-by`` tag on + any patch fixing the issue. When the problem is reported via the GitLab + issue tracker, however, it is sufficient to just include a link to the + issue. + + * **``Suggested-by``**: when a reviewer or other 3rd party makes non-trivial + suggestions for how to change a patch, it is good practice to credit them + by including a ``Suggested-by`` tag. + +Subsystem maintainer requirements +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When a subsystem maintainer accepts a patch from a contributor, in addition to +the normal code review points, they are expected to validate the presence of +suitable ``Signed-off-by`` tags. + +At the time they queue the patch in their subsystem tree, the maintainer +**must** also then add their own ``Signed-off-by`` to indicate that they have +done the aforementioned validation. This is in addition to any of their own +``Reviewed-by`` tags the subsystem maintainer may wish to include. + +Tools for adding ``Signed-off-by`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +There are a variety of ways tools can support adding ``Signed-off-by`` tags +for patches, avoiding the need for contributors to manually type in this +repetitive text each time. + +git commands +^^^^^^^^^^^^ + +When creating, or amending, a commit the ``-s`` flag to ``git commit`` will +append a suitable line matching the configuring git author details. + +If preparing patches using the ``git format-patch`` tool, the ``-s`` flag can +be used to append a suitable line in the emails it creates, without modifying +the local commits. Alternatively to modify all the local commits on a branch:: + + git rebase master -x 'git commit --amend --no-edit -s' + +emacs +^^^^^ + +In the file ``$HOME/.emacs.d/abbrev_defs`` add:: + + (define-abbrev-table 'global-abbrev-table + '( + ("8rev" "Reviewed-by: YOUR NAME <your@email.addr>" nil 1) + ("8ack" "Acked-by: YOUR NAME <your@email.addr>" nil 1) + ("8test" "Tested-by: YOUR NAME <your@email.addr>" nil 1) + ("8sob" "Signed-off-by: YOUR NAME <your@email.addr>" nil 1) + )) + +with this change, if you type (for example) ``8rev`` followed by ``<space>`` +or ``<enter>`` it will expand to the whole phrase. + +vim +^^^ + +In the file ``$HOME/.vimrc`` add:: + + iabbrev 8rev Reviewed-by: YOUR NAME <your@email.addr> + iabbrev 8ack Acked-by: YOUR NAME <your@email.addr> + iabbrev 8test Tested-by: YOUR NAME <your@email.addr> + iabbrev 8sob Signed-off-by: YOUR NAME <your@email.addr> + +with this change, if you type (for example) ``8rev`` followed by ``<space>`` +or ``<enter>`` it will expand to the whole phrase. + +Re-starting abandoned work +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +For a variety of reasons there are some patches that get submitted to QEMU but +never merged. An unrelated contributor may decide (months or years later) to +continue working from the abandoned patch and re-submit it with extra changes. + +The general principles when picking up abandoned work are: + + * Continue to credit the original author for their work, by maintaining their + original ``Signed-off-by`` + * Indicate where the original patch was obtained from (mailing list, bug + tracker, author's git repo, etc) when sending it for review + * Acknowledge the extra work of the new contributor by including their + ``Signed-off-by`` in the patch in addition to the orignal author's + * Indicate who is responsible for what parts of the patch. This is typically + done via a note in the commit message, just prior to the new contributor's + ``Signed-off-by``:: + + Signed-off-by: Some Person <some.person@example.com> + [Rebased and added support for 'foo'] + Signed-off-by: New Person <new.person@mycorp.test> + +In complicated cases, or if otherwise unsure, ask for advice on the project +mailing list. + +It is also recommended to attempt to contact the original author to let them +know you are interested in taking over their work, in case they still intended +to return to the work, or had any suggestions about the best way to continue. diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst index 362f97ee30..b54e58105e 100644 --- a/docs/devel/index-process.rst +++ b/docs/devel/index-process.rst @@ -13,6 +13,7 @@ Notes about how to interact with the community and how and where to submit patch maintainers style submitting-a-patch + code-provenance trivial-patches stable-process submitting-a-pull-request diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst index 83e9092b8c..2cc4d53ff6 100644 --- a/docs/devel/submitting-a-patch.rst +++ b/docs/devel/submitting-a-patch.rst @@ -322,23 +322,8 @@ Patch emails must include a ``Signed-off-by:`` line Your patches **must** include a Signed-off-by: line. This is a hard requirement because it's how you say "I'm legally okay to contribute -this and happy for it to go into QEMU". The process is modelled after -the `Linux kernel -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ -policy. - -If you wrote the patch, make sure your "From:" and "Signed-off-by:" -lines use the same spelling. It's okay if you subscribe or contribute to -the list via more than one address, but using multiple addresses in one -commit just confuses things. If someone else wrote the patch, git will -include a "From:" line in the body of the email (different from your -envelope From:) that will give credit to the correct author; but again, -that author's Signed-off-by: line is mandatory, with the same spelling. - -There are various tooling options for automatically adding these tags -include using ``git commit -s`` or ``git format-patch -s``. For more -information see `SubmittingPatches 1.12 -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. +this and happy for it to go into QEMU". For full guidance, read the +:ref:`code-provenance` documentation. .. _include_a_meaningful_cover_letter:
Currently we have a short paragraph saying that patches must include a Signed-off-by line, and merely link to the kernel documentation. The linked kernel docs have a lot of content beyond the part about sign-off an thus are misleading/distracting to QEMU contributors. This introduces a dedicated 'code-provenance' page in QEMU talking about why we require sign-off, explaining the other tags we commonly use, and what to do in some edge cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/devel/code-provenance.rst | 212 ++++++++++++++++++++++++++++++ docs/devel/index-process.rst | 1 + docs/devel/submitting-a-patch.rst | 19 +-- 3 files changed, 215 insertions(+), 17 deletions(-) create mode 100644 docs/devel/code-provenance.rst