| Message ID | 20231123114026.3589272-2-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | docs: define policy forbidding use of "AI" / LLM code generators | expand |
On 23/11/23 12:40, 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 alot of content beyond the part about > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 18 +-- > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,197 @@ > +.. _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> > + > +This existence of this line asserts that the author of the patch is > +contributing in accordance with the `Developer's Certificate of > +Origin <https://developercertifcate.org>`__: Typo: 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 also one of the patch authors, > +it is further expected that the mail ``From:`` line name & address match > +one of the ``Signed-off-by`` lines. > + > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple > +authors. In such a scenario, a git commit will usually be expected > +to have a ``Signed-off-by`` line for each contributor involved in > +creatin of the patch. Some edge cases: "creating" > + > + * 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. > + > + * 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 contributeo from their employer "contribute" > + who is the copyright holder. It is none the less still preferrable "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. > + > +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. > + > + NB: a subsystem maintainer sending a pull request would replace > + their own ``Reviewed-by`` with another ``Signed-off-by`` Hmm not sure about replacing, they have different meaning. You can merge patch you haven't reviewed. But as a maintainer you must S-o-b what you end merging (what is mentioned below in "subsystem maintainer"). > + > + * **``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. > + > + * **``Tested-by``**: when a QEMU community member has functionally > + tested the behaviour of the patch in some manner, they should > + send an email reply conmtaning a ``Tested-by`` tag. "containing" > + > + * **``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. Hmm isn't related to the "Resolves:" 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. > + > +The subsystem maintainer submitting a pull request is **NOT** expected to > +have a ``Reviewed-by`` tag on the patch, since this is implied by their > +own ``Signed-off-by``. > + > +Tools for adding ``Signed-of-by`` "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 the > +local commits on a branch en-mass:: > + > + 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. > + > +If the abandoned patch already had a ``Signed-off-by`` from the original > +author this **must** be preserved. The new contributor **must** then add > +their own ``Signed-off-by`` after the original one if they made any > +further changes to it. It is common to include a comment just prior to > +the new ``Signed-off-by`` indicating what extra changes were made. For > +example:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@example.com> > 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 c641d948f1..ec541b3d15 100644 > --- a/docs/devel/submitting-a-patch.rst > +++ b/docs/devel/submitting-a-patch.rst > @@ -322,21 +322,9 @@ 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 > +this and happy for it to go into QEMU". For full guidance, read the > +:ref:`code-provenance` documentation. > + > information see `SubmittingPatches 1.12 > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. >
On Thu, 23 Nov 2023 at 11:40, 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 alot of content beyond the part about "a lot" > sign-off an thus is misleading/distracting to QEMU contributors. "and thus are" > > 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. Good idea; I've felt for a while now that it was a little awkward to have to point people at that big kernel doc page. > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > docs/devel/code-provenance.rst | 197 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 18 +-- > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,197 @@ > +.. _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> > + > +This existence of this line asserts that the author of the patch is > +contributing in accordance with the `Developer's Certificate of > +Origin <https://developercertifcate.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 also one of the patch authors, > +it is further expected that the mail ``From:`` line name & address match > +one of the ``Signed-off-by`` lines. Is it? Patches sent via the sr.ht service won't do that, and I'm pretty sure we've had a few contributors in the past who send patches from different addresses to avoid problems with their corporate mail server mangling patches. I think this would be better softened to something like a recommendation ("Generally you should use the same email addresses ... "). > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple > +authors. In such a scenario, a git commit will usually be expected > +to have a ``Signed-off-by`` line for each contributor involved in > +creatin of the patch. Some edge cases: "creation" (not "creating") > + > + * 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. > + > + * 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 contributeo from their employer > + who is the copyright holder. It is none the less still preferrable > + 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. > + > +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 missing '.' (or perhaps ':'). > + > + * **``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. > + > + NB: a subsystem maintainer sending a pull request would replace > + their own ``Reviewed-by`` with another ``Signed-off-by`` I agree with Philippe here -- you add signed-off-by, you don't replace reviewed-by. > + > + * **``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. I would personally also say "Acked-by does not imply a full code review of the patch; if the subsystem maintainer has done a full review, they should use the Reviewed-by tag instead." But I know that there are some differences of opinion on exactly what Acked-by: means... > + > + * **``Tested-by``**: when a QEMU community member has functionally > + tested the behaviour of the patch in some manner, they should > + send an email reply conmtaning 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. Maybe we should add a bit of encouraging text here along the lines of: Reviewing and testing is something anybody can do -- if you've reviewed the code or tested it, feel free to send an email with your tag to say you've done that, or to ask questions if there's part of the patch you don't understand. ? Or perhaps that would be better elsewhere; IDK. > + > +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. > + > +The subsystem maintainer submitting a pull request is **NOT** expected to > +have a ``Reviewed-by`` tag on the patch, since this is implied by their > +own ``Signed-off-by``. As above, Signed-off-by doesn't imply Reviewed-by. If the submaintainer has reviewed the patch, they add the R-by, but if they haven't done that, then they only add the S-o-by. > + > +Tools for adding ``Signed-of-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 the > +local commits on a branch en-mass:: > + > + 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. > + > +If the abandoned patch already had a ``Signed-off-by`` from the original > +author this **must** be preserved. The new contributor **must** then add > +their own ``Signed-off-by`` after the original one if they made any > +further changes to it. It is common to include a comment just prior to > +the new ``Signed-off-by`` indicating what extra changes were made. For > +example:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@example.com> You might want to use two different email domains in this example; an abandoned project picked up by somebody from the same company (assuming the usual copyright-belongs-to-company) is a bit different from an abandoned project picked up by an entirely unrelated person. I think in this case it's also worth stating the general principles: ===begin=== The general principles with picking up abandoned work are: * we should continue to credit the first author for their work * we should track the provenance of the code * we should also acknowledge the efforts of the person picking up the work * the commit messages should indicate who is responsible for what parts of the final patch In complicated cases or if in doubt, you can always ask on the mailing list for advice. If the new work you'd need to do to resubmit the patches is significant, it's worth dropping the original author a friendly email to let them know, in case you might be duplicating something the original author is still working on. ===endit=== perhaps ? thanks -- PMM
Am 23.11.2023 um 12:40 hat Daniel P. Berrangé geschrieben: > 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 alot of content beyond the part about > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 18 +-- > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,197 @@ > +.. _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> > + > +This existence of this line asserts that the author of the patch is > +contributing in accordance with the `Developer's Certificate of > +Origin <https://developercertifcate.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 also one of the patch authors, > +it is further expected that the mail ``From:`` line name & address match > +one of the ``Signed-off-by`` lines. Isn't the S-o-b expected even if the person sending the mail isn't one of the patch authors, i.e. certifying (c) rather than (a) or (b) from the DCO? This is essentially the same case as what a subsystem maintainer does. > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple > +authors. In such a scenario, a git commit will usually be expected > +to have a ``Signed-off-by`` line for each contributor involved in > +creatin 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. > + > + * 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 contributeo from their employer > + who is the copyright holder. It is none the less still preferrable > + 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. > + > +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. > + > + NB: a subsystem maintainer sending a pull request would replace > + their own ``Reviewed-by`` with another ``Signed-off-by`` As Philippe already mentioned, this isn't necessarily the case. It's a common enough practice to add a S-o-b (which technically only certifies the DCO) without removing the R-b (which tells that the content was actually reviewed in detail - maintainers don't always do that if there are already R-bs from trusted community members). > + * **``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. > + Trailing whitespace? > + * **``Tested-by``**: when a QEMU community member has functionally > + tested the behaviour of the patch in some manner, they should > + send an email reply conmtaning 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. > + > +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. > + > +The subsystem maintainer submitting a pull request is **NOT** expected to > +have a ``Reviewed-by`` tag on the patch, since this is implied by their > +own ``Signed-off-by``. Considering the above, I would remove this last paragraph. Kevin
On Thu, Nov 23, 2023 at 11:40:25AM +0000, 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 alot of content beyond the part about > sign-off an thus is 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> Great initiative! I think we needed this for a while now. > --- > docs/devel/code-provenance.rst | 197 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 18 +-- > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,197 @@ > +.. _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> > + > +This existence of this line asserts that the author of the patch is The existence? > +contributing in accordance with the `Developer's Certificate of > +Origin <https://developercertifcate.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 also one of the patch authors, > +it is further expected that the mail ``From:`` line name & address match > +one of the ``Signed-off-by`` lines. > + > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple > +authors. In such a scenario, a git commit will usually be expected > +to have a ``Signed-off-by`` line for each contributor involved in > +creatin of the patch. Some edge cases: creation > + > + * 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. It is still a good policy to include attribution, e.g. by adding a Suggested-by tag. > + > + * 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 contributeo from their employer contribute > + who is the copyright holder. It is none the less still preferrable > + 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. > + > +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. > + > + NB: a subsystem maintainer sending a pull request would replace > + their own ``Reviewed-by`` with another ``Signed-off-by`` > + > + * **``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. > + > + * **``Tested-by``**: when a QEMU community member has functionally > + tested the behaviour of the patch in some manner, they should > + send an email reply conmtaning 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 is also common. As long as we are here, let's document Fixes: and Cc: ? > +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. Below you say **must** - I think that is better, no need to shout. > + > +The subsystem maintainer submitting a pull request is **NOT** expected to > +have a ``Reviewed-by`` tag on the patch, since this is implied by their > +own ``Signed-off-by``. > + > +Tools for adding ``Signed-of-by`` 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 the > +local commits on a branch en-mass:: > + > + 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. > + > +If the abandoned patch already had a ``Signed-off-by`` from the original > +author this **must** be preserved. The new contributor **must** then add > +their own ``Signed-off-by`` after the original one if they made any > +further changes to it. It is common to include a comment just prior to > +the new ``Signed-off-by`` indicating what extra changes were made. For > +example:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@example.com> > 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 c641d948f1..ec541b3d15 100644 > --- a/docs/devel/submitting-a-patch.rst > +++ b/docs/devel/submitting-a-patch.rst > @@ -322,21 +322,9 @@ 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 > +this and happy for it to go into QEMU". For full guidance, read the > +:ref:`code-provenance` documentation. > + > information see `SubmittingPatches 1.12 > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. this "information" now looks orphaned or am I confused? > -- > 2.41.0
On Thu, Nov 23, 2023 at 11:40:25AM +0000, 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 alot of content beyond the part about > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 18 +-- > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,197 @@ > +.. _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> > + > +This existence of this line asserts that the author of the patch is > +contributing in accordance with the `Developer's Certificate of > +Origin <https://developercertifcate.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 also one of the patch authors, > +it is further expected that the mail ``From:`` line name & address match > +one of the ``Signed-off-by`` lines. > + > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple > +authors. In such a scenario, a git commit will usually be expected > +to have a ``Signed-off-by`` line for each contributor involved in > +creatin 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. > + > + * 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 contributeo from their employer s/contributeo/contribute/ > + who is the copyright holder. It is none the less still preferrable > + 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. > + > +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. > + > + NB: a subsystem maintainer sending a pull request would replace > + their own ``Reviewed-by`` with another ``Signed-off-by`` > + > + * **``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. > + > + * **``Tested-by``**: when a QEMU community member has functionally > + tested the behaviour of the patch in some manner, they should > + send an email reply conmtaning a ``Tested-by`` tag. s/conmtaning/containing/ > + > + * **``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. > + > +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. > + > +The subsystem maintainer submitting a pull request is **NOT** expected to > +have a ``Reviewed-by`` tag on the patch, since this is implied by their > +own ``Signed-off-by``. > + > +Tools for adding ``Signed-of-by`` s/Signed-of-by/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 the > +local commits on a branch en-mass:: > + > + 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. > + > +If the abandoned patch already had a ``Signed-off-by`` from the original > +author this **must** be preserved. The new contributor **must** then add > +their own ``Signed-off-by`` after the original one if they made any > +further changes to it. It is common to include a comment just prior to > +the new ``Signed-off-by`` indicating what extra changes were made. For > +example:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@example.com> > 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 c641d948f1..ec541b3d15 100644 > --- a/docs/devel/submitting-a-patch.rst > +++ b/docs/devel/submitting-a-patch.rst > @@ -322,21 +322,9 @@ 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 > +this and happy for it to go into QEMU". For full guidance, read the > +:ref:`code-provenance` documentation. > + > information see `SubmittingPatches 1.12 > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. > > -- > 2.41.0 >
On Thu, Nov 23, 2023 at 12:58:18PM +0100, Philippe Mathieu-Daudé wrote: > On 23/11/23 12:40, 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 alot of content beyond the part about > > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > > docs/devel/index-process.rst | 1 + > > docs/devel/submitting-a-patch.rst | 18 +-- > > 3 files changed, 201 insertions(+), 15 deletions(-) > > create mode 100644 docs/devel/code-provenance.rst > > +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. > > + > > + NB: a subsystem maintainer sending a pull request would replace > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > Hmm not sure about replacing, they have different meaning. You can merge > patch you haven't reviewed. But as a maintainer you must S-o-b what you > end merging (what is mentioned below in "subsystem maintainer"). I've always taken it as implied that patches I queue are reviewed by me, but replies here suggest I'm in a minority on that. That shows why it is worth documenting this for QEMU explicitly :-) > > + * **``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. > > Hmm isn't related to the "Resolves:" tag? Gitlab supports a huge varity - resolves/fixes/closes/etc I don't think this wants to turn into a full guide on what info to include in a commit message, as we already have that in the submitting-a-patch doc, explaining the bug link syntax. So I'll still to just the tags that explicitly credit humans. With regards, Daniel
On Thu, Nov 23, 2023 at 01:01:00PM +0000, Peter Maydell wrote: > On Thu, 23 Nov 2023 at 11:40, 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 alot of content beyond the part about > > "a lot" > > > sign-off an thus is misleading/distracting to QEMU contributors. > > "and thus are" > > > > > 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. > > Good idea; I've felt for a while now that it was a little awkward > to have to point people at that big kernel doc page. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > docs/devel/code-provenance.rst | 197 ++++++++++++++++++++++++++++++ > > docs/devel/index-process.rst | 1 + > > docs/devel/submitting-a-patch.rst | 18 +-- > > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > > --- /dev/null > > +++ b/docs/devel/code-provenance.rst > > @@ -0,0 +1,197 @@ > > +.. _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> > > + > > +This existence of this line asserts that the author of the patch is > > +contributing in accordance with the `Developer's Certificate of > > +Origin <https://developercertifcate.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 also one of the patch authors, > > +it is further expected that the mail ``From:`` line name & address match > > +one of the ``Signed-off-by`` lines. > > Is it? Patches sent via the sr.ht service won't do that, and I'm > pretty sure we've had a few contributors in the past who send > patches from different addresses to avoid problems with their > corporate mail server mangling patches. I think this would be > better softened to something like a recommendation ("Generally > you should use the same email addresses ... "). Yes, I forgot about sr.ht being wierd in this respect, so I'll take your suggestion. > > + > > + * **``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. > > + > > + NB: a subsystem maintainer sending a pull request would replace > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > I agree with Philippe here -- you add signed-off-by, you don't > replace reviewed-by. Yep, will change that. > > > + > > + * **``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. > > I would personally also say "Acked-by does not imply a full code > review of the patch; if the subsystem maintainer has done a full > review, they should use the Reviewed-by tag instead." > > But I know that there are some differences of opinion on exactly > what Acked-by: means... I'll incorporate something along those lines with a little fuzzyness to give flexibility. > > + > > + * **``Tested-by``**: when a QEMU community member has functionally > > + tested the behaviour of the patch in some manner, they should > > + send an email reply conmtaning 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. > > Maybe we should add a bit of encouraging text here along the lines of: > > Reviewing and testing is something anybody can do -- if you've > reviewed the code or tested it, feel free to send an email with > your tag to say you've done that, or to ask questions if there's > part of the patch you don't understand. > > ? Or perhaps that would be better elsewhere; IDK. I'll put a little bit in here but want to keep it relatively concise, since we have other docs about more general contribution practices. > > +If the abandoned patch already had a ``Signed-off-by`` from the original > > +author this **must** be preserved. The new contributor **must** then add > > +their own ``Signed-off-by`` after the original one if they made any > > +further changes to it. It is common to include a comment just prior to > > +the new ``Signed-off-by`` indicating what extra changes were made. For > > +example:: > > + > > + Signed-off-by: Some Person <some.person@example.com> > > + [Rebased and added support for 'foo'] > > + Signed-off-by: New Person <new.person@example.com> > > You might want to use two different email domains in this example; > an abandoned project picked up by somebody from the same company > (assuming the usual copyright-belongs-to-company) is a bit different > from an abandoned project picked up by an entirely unrelated person. Yes good idea. > I think in this case it's also worth stating the general principles: > > ===begin=== > The general principles with picking up abandoned work are: > * we should continue to credit the first author for their work > * we should track the provenance of the code > * we should also acknowledge the efforts of the person picking > up the work > * the commit messages should indicate who is responsible for > what parts of the final patch > > In complicated cases or if in doubt, you can always ask on the > mailing list for advice. > > If the new work you'd need to do to resubmit the patches is > significant, it's worth dropping the original author a > friendly email to let them know, in case you might be > duplicating something the original author is still working on. > ===endit=== > > perhaps ? I'll incorporate somethnig along these lines. With regards, Daniel
On Thu, Nov 23, 2023 at 02:16:36PM +0100, Kevin Wolf wrote: > Am 23.11.2023 um 12:40 hat Daniel P. Berrangé geschrieben: > > 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 alot of content beyond the part about > > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > > docs/devel/index-process.rst | 1 + > > docs/devel/submitting-a-patch.rst | 18 +-- > > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > > --- /dev/null > > +++ b/docs/devel/code-provenance.rst > > @@ -0,0 +1,197 @@ > > +.. _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> > > + > > +This existence of this line asserts that the author of the patch is > > +contributing in accordance with the `Developer's Certificate of > > +Origin <https://developercertifcate.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 also one of the patch authors, > > +it is further expected that the mail ``From:`` line name & address match > > +one of the ``Signed-off-by`` lines. > > Isn't the S-o-b expected even if the person sending the mail isn't one > of the patch authors, i.e. certifying (c) rather than (a) or (b) from > the DCO? This is essentially the same case as what a subsystem > maintainer does. Yes, you are right. > > +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. > > + > > + NB: a subsystem maintainer sending a pull request would replace > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > As Philippe already mentioned, this isn't necessarily the case. It's a > common enough practice to add a S-o-b (which technically only certifies > the DCO) without removing the R-b (which tells that the content was > actually reviewed in detail - maintainers don't always do that if there > are already R-bs from trusted community members). Yes, will change. With regards, Daniel
On Thu, Nov 23, 2023 at 09:25:13AM -0500, Michael S. Tsirkin wrote: > On Thu, Nov 23, 2023 at 11:40:25AM +0000, 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 alot of content beyond the part about > > sign-off an thus is 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> > > > + * 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. > > It is still a good policy to include attribution, e.g. > by adding a Suggested-by tag. Will add this tag. > > +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. > > + > > + NB: a subsystem maintainer sending a pull request would replace > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > + > > + * **``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. > > + > > + * **``Tested-by``**: when a QEMU community member has functionally > > + tested the behaviour of the patch in some manner, they should > > + send an email reply conmtaning 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 is also common. > > As long as we are here, let's document Fixes: and Cc: ? The submitting-a-patch doc covers more general commit message information. I think this doc just ought to focus on tags that identify humans involved in the process. I've never been sure what the point of the 'Cc' tag is, when you actually want to use the Cc email header ? > > diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst > > index c641d948f1..ec541b3d15 100644 > > --- a/docs/devel/submitting-a-patch.rst > > +++ b/docs/devel/submitting-a-patch.rst > > @@ -322,21 +322,9 @@ 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 > > +this and happy for it to go into QEMU". For full guidance, read the > > +:ref:`code-provenance` documentation. > > + > > information see `SubmittingPatches 1.12 > > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. > > this "information" now looks orphaned or am I confused? Yes, forgot to cull it. With regards, Daniel
On Thu, Nov 23, 2023 at 05:16:45PM +0000, Daniel P. Berrangé wrote: > On Thu, Nov 23, 2023 at 09:25:13AM -0500, Michael S. Tsirkin wrote: > > On Thu, Nov 23, 2023 at 11:40:25AM +0000, 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 alot of content beyond the part about > > > sign-off an thus is 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> > > > > > > + * 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. > > > > It is still a good policy to include attribution, e.g. > > by adding a Suggested-by tag. > > Will add this tag. > > > > > +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. > > > + > > > + NB: a subsystem maintainer sending a pull request would replace > > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > > + > > > + * **``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. > > > + > > > + * **``Tested-by``**: when a QEMU community member has functionally > > > + tested the behaviour of the patch in some manner, they should > > > + send an email reply conmtaning 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 is also common. > > > > As long as we are here, let's document Fixes: and Cc: ? > > The submitting-a-patch doc covers more general commit message information. > I think this doc just ought to focus on tags that identify humans involved > in the process. > > I've never been sure what the point of the 'Cc' tag is, when you actually > want to use the Cc email header ? > It records the fact that these people have been copied but did not respond. > > > diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst > > > index c641d948f1..ec541b3d15 100644 > > > --- a/docs/devel/submitting-a-patch.rst > > > +++ b/docs/devel/submitting-a-patch.rst > > > @@ -322,21 +322,9 @@ 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 > > > +this and happy for it to go into QEMU". For full guidance, read the > > > +:ref:`code-provenance` documentation. > > > + > > > information see `SubmittingPatches 1.12 > > > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. > > > > this "information" now looks orphaned or am I confused? > > Yes, forgot to cull it. > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 23, 2023 at 05:08:46PM +0000, Daniel P. Berrangé wrote: > On Thu, Nov 23, 2023 at 12:58:18PM +0100, Philippe Mathieu-Daudé wrote: > > On 23/11/23 12:40, 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 alot of content beyond the part about > > > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > > > docs/devel/index-process.rst | 1 + > > > docs/devel/submitting-a-patch.rst | 18 +-- > > > 3 files changed, 201 insertions(+), 15 deletions(-) > > > create mode 100644 docs/devel/code-provenance.rst > > > > +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. > > > + > > > + NB: a subsystem maintainer sending a pull request would replace > > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > > > Hmm not sure about replacing, they have different meaning. You can merge > > patch you haven't reviewed. But as a maintainer you must S-o-b what you > > end merging (what is mentioned below in "subsystem maintainer"). > > I've always taken it as implied that patches I queue are reviewed by me, Well sometimes I queue patches not in my area that I have seen languish on list with no replies for too long. I generally do a cursory review but not to the level that I feel justifies Reviewed-by. > but replies here suggest I'm in a minority on that. That shows why it is > worth documenting this for QEMU explicitly :-) Absolutely.
Am 23.11.2023 um 18:16 hat Daniel P. Berrangé geschrieben: > > Suggested-by is also common. > > > > As long as we are here, let's document Fixes: and Cc: ? > > The submitting-a-patch doc covers more general commit message information. > I think this doc just ought to focus on tags that identify humans involved > in the process. > > I've never been sure what the point of the 'Cc' tag is, when you actually > want to use the Cc email header ? By default, git-send-email automatically copies the addresses mentioned with Cc: in the commit message, so I always assumed that this is what people intend with these tags. Of course, in practice many of us have suppresscc = "all" in their config to avoid downstream accidents, so maybe there is another use? The only time I use it is for "Cc: qemu-stable@nongnu.org". I'm not sure if it still works like this, but people used to look for this in the commit history when preparing stable releases. (It's useful because sometimes people forget to actually CC the qemu-stable list when sending the patches.) Kevin
On 23/11/23 18:33, Michael S. Tsirkin wrote: > On Thu, Nov 23, 2023 at 05:16:45PM +0000, Daniel P. Berrangé wrote: >> On Thu, Nov 23, 2023 at 09:25:13AM -0500, Michael S. Tsirkin wrote: >>> On Thu, Nov 23, 2023 at 11:40:25AM +0000, 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 alot of content beyond the part about >>>> sign-off an thus is 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> >>> >> >>>> + * 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. >>> >>> It is still a good policy to include attribution, e.g. >>> by adding a Suggested-by tag. >> >> Will add this tag. Thanks! >>>> +Other commit tags >>>> +~~~~~~~~~~~~~~~~~ >>> As long as we are here, let's document Fixes: and Cc: ? >> >> The submitting-a-patch doc covers more general commit message information. >> I think this doc just ought to focus on tags that identify humans involved >> in the process. >> >> I've never been sure what the point of the 'Cc' tag is, when you actually >> want to use the Cc email header ? >> > > It records the fact that these people have been copied but did not > respond. This might be felt aggressive or forcing. My understanding of this Cc tag in a commit is "now that it is merged, you can't complain". We can be absent, sick, on holidays... If I missed a merged patch review I'll try to kindly ask on the list if it can be reworked, or suggest a patch to fix what I missed. Not sure this is really useful to commit that to the repository. IMHO the only useful Cc tag is for qemu-stable@nongnu.org, as Kevin mentioned. If you want to be sure your patch is Cc to a set of developers, you can add Cc: lines below the '---' patch separator. My 2 cents eh... Regards, Phil.
On Fri, Nov 24, 2023 at 12:11:30PM +0100, Philippe Mathieu-Daudé wrote: > On 23/11/23 18:33, Michael S. Tsirkin wrote: > > On Thu, Nov 23, 2023 at 05:16:45PM +0000, Daniel P. Berrangé wrote: > > > On Thu, Nov 23, 2023 at 09:25:13AM -0500, Michael S. Tsirkin wrote: > > > > On Thu, Nov 23, 2023 at 11:40:25AM +0000, 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 alot of content beyond the part about > > > > > sign-off an thus is 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> > > > > > > > > > > > > + * 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. > > > > > > > > It is still a good policy to include attribution, e.g. > > > > by adding a Suggested-by tag. > > > > > > Will add this tag. > > Thanks! > > > > > > +Other commit tags > > > > > +~~~~~~~~~~~~~~~~~ > > > > > > As long as we are here, let's document Fixes: and Cc: ? > > > > > > The submitting-a-patch doc covers more general commit message information. > > > I think this doc just ought to focus on tags that identify humans involved > > > in the process. > > > > > > I've never been sure what the point of the 'Cc' tag is, when you actually > > > want to use the Cc email header ? > > > > > > > It records the fact that these people have been copied but did not > > respond. > This might be felt aggressive or forcing. > My understanding of this Cc > tag in a commit is "now that it is merged, you can't complain". We can > be absent, sick, on holidays... If I missed a merged patch review I'll > try to kindly ask on the list if it can be reworked, or suggest a patch > to fix what I missed. > Not sure this is really useful to commit that to the repository. I don't see it as forcing. Sometimes I do a fly-by review of a patch that caught my eye not in my area. Later people address my comments and start copying me but I don't have time to re-review. Recoding the fact that they copied me seems important. This info might be helpful in git history for other reasons - helps looking for someone to help review backports - to guess at code quality - to help understand whether code had all the needed people copied > > IMHO the only useful Cc tag is for qemu-stable@nongnu.org, as Kevin > mentioned. > > If you want to be sure your patch is Cc to a set of developers, you can > add Cc: lines below the '---' patch separator. My 2 cents eh... > > Regards, > > Phil. If people feel threatened by CC I don't have a problem to ask people to put it in a note so it comes after ---.
Hi Daniel, On Thu, Nov 23, 2023 at 11:40:25AM +0000, Daniel P. Berrangé wrote: > Date: Thu, 23 Nov 2023 11:40:25 +0000 > From: "Daniel P. Berrangé" <berrange@redhat.com> > Subject: [PATCH 1/2] docs: introduce dedicated page about code provenance / > sign-off > > 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 alot of content beyond the part about > sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ > docs/devel/index-process.rst | 1 + > docs/devel/submitting-a-patch.rst | 18 +-- > 3 files changed, 201 insertions(+), 15 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..b4591a2dec > --- /dev/null > +++ b/docs/devel/code-provenance.rst > @@ -0,0 +1,197 @@ > +.. _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> > + > +This existence of this line asserts that the author of the patch is > +contributing in accordance with the `Developer's Certificate of > +Origin <https://developercertifcate.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 also one of the patch authors, > +it is further expected that the mail ``From:`` line name & address match > +one of the ``Signed-off-by`` lines. > + > +Multiple authorship > +~~~~~~~~~~~~~~~~~~~ > + > +It is not uncommon for a patch to have contributions from multiple > +authors. In such a scenario, a git commit will usually be expected > +to have a ``Signed-off-by`` line for each contributor involved in > +creatin 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. > + > + * 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 contributeo from their employer > + who is the copyright holder. For this case, maybe it needs the "Co-developed-by"? > It is none the less still preferrable > + 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. > + > +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. Maybe just a question, the people should drop the Reviewed/ACKed/Tested tags that have been obtained if he make the any code changes (including function/variable renaming) as well as commit message changes during the patch refresh process, am I understand correctly? ;-) > + > + NB: a subsystem maintainer sending a pull request would replace > + their own ``Reviewed-by`` with another ``Signed-off-by`` > + > + * **``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. > + > + * **``Tested-by``**: when a QEMU community member has functionally > + tested the behaviour of the patch in some manner, they should > + send an email reply conmtaning a ``Tested-by`` tag. Is there any requirement for the order of tags? My previous understanding was that if the Reviewed-by/Tested-by tags were obtained by the author within his company, then those tags should be placed before the signed-off-by of the author. If the Reviewed-by/ Tested-by were acquired in the community, then they should be placed after the author's signed-off-by, right? > + > + * **``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. > + > +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. > + > +The subsystem maintainer submitting a pull request is **NOT** expected to > +have a ``Reviewed-by`` tag on the patch, since this is implied by their > +own ``Signed-off-by``. > + > +Tools for adding ``Signed-of-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 the > +local commits on a branch en-mass:: > + > + 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. > + > +If the abandoned patch already had a ``Signed-off-by`` from the original > +author this **must** be preserved. I find some people added Originally-by, e.g., 8e86851bd6b9. I guess if the code has been changed very significantly, or if the original implementation has just been referenced and significantly refactored, then Originally-by should be preferred instead of Signed-off-by from the original author, right? Thanks, Zhao > The new contributor **must** then add > +their own ``Signed-off-by`` after the original one if they made any > +further changes to it. It is common to include a comment just prior to > +the new ``Signed-off-by`` indicating what extra changes were made. For > +example:: > + > + Signed-off-by: Some Person <some.person@example.com> > + [Rebased and added support for 'foo'] > + Signed-off-by: New Person <new.person@example.com> > 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 c641d948f1..ec541b3d15 100644 > --- a/docs/devel/submitting-a-patch.rst > +++ b/docs/devel/submitting-a-patch.rst > @@ -322,21 +322,9 @@ 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 > +this and happy for it to go into QEMU". For full guidance, read the > +:ref:`code-provenance` documentation. > + > information see `SubmittingPatches 1.12 > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. > > -- > 2.41.0 > >
On Sat, Jan 27, 2024 at 10:36:24PM +0800, Zhao Liu wrote: > Hi Daniel, > > On Thu, Nov 23, 2023 at 11:40:25AM +0000, Daniel P. Berrangé wrote: > > +Multiple authorship > > +~~~~~~~~~~~~~~~~~~~ > > + > > +It is not uncommon for a patch to have contributions from multiple > > +authors. In such a scenario, a git commit will usually be expected > > +to have a ``Signed-off-by`` line for each contributor involved in > > +creatin 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. > > + > > + * 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 contributeo from their employer > > + who is the copyright holder. > > For this case, maybe it needs the "Co-developed-by"? If you're going to go to the trouble of adding multiple tags to the commit for each author who participated, then IMHO they should all be Signed-off-by. IOW, either just have S-o-B from the main author within a company, or have S-o-B for every author. Co-developed-by doesn't have value IMHO. > > It is none the less still preferrable > > + 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. > > + > > +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. > > Maybe just a question, the people should drop the Reviewed/ACKed/Tested > tags that have been obtained if he make the any code changes (including > function/variable renaming) as well as commit message changes during > the patch refresh process, am I understand correctly? ;-) It is a judgement call as to whether a Reviewed-by/etc should be kept or dropped. It depends on the scale of the changes that were made to the commit since the Reviewed-by/etc was first given. > > + NB: a subsystem maintainer sending a pull request would replace > > + their own ``Reviewed-by`` with another ``Signed-off-by`` > > + > > + * **``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. > > + > > + * **``Tested-by``**: when a QEMU community member has functionally > > + tested the behaviour of the patch in some manner, they should > > + send an email reply conmtaning a ``Tested-by`` tag. > > Is there any requirement for the order of tags? > > My previous understanding was that if the Reviewed-by/Tested-by tags > were obtained by the author within his company, then those tags should > be placed before the signed-off-by of the author. If the Reviewed-by/ > Tested-by were acquired in the community, then they should be placed > after the author's signed-off-by, right? Common practice is for Signed-off-by tags to be kept in time order from earliest author to latest author / maintainer. Common case is 2 S-o-B, the first from the patch author, and the last from the sub-system maintainer who sends the pull request. For other tags I don't see any broadly acceptable pattern. Some people add Reviewed-by before the S-o-B, others add Reviewed-by after the S-o-B. Either is fine IMHO. > > +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. > > + > > +If the abandoned patch already had a ``Signed-off-by`` from the original > > +author this **must** be preserved. > > I find some people added Originally-by, e.g., 8e86851bd6b9. > > I guess if the code has been changed very significantly, or if the > original implementation has just been referenced and significantly > refactored, then Originally-by should be preferred instead of > Signed-off-by from the original author, right? If patch submitted still contains any code that can be considered copyrightable (ie anything non-trivial) from the original author, then I would expect the original authors Signed-off-by to be retained. I think the cases where it is ok to use Originally-by, without a Signed-off-by, would be exceedingly. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: >> Is there any requirement for the order of tags? >> >> My previous understanding was that if the Reviewed-by/Tested-by >> tags >> were obtained by the author within his company, then those tags >> should >> be placed before the signed-off-by of the author. If the >> Reviewed-by/ >> Tested-by were acquired in the community, then they should be >> placed >> after the author's signed-off-by, right? > > Common practice is for Signed-off-by tags to be kept in time > order > from earliest author to latest author / maintainer. Common case > is > 2 S-o-B, the first from the patch author, and the last from the > sub-system maintainer who sends the pull request. > > For other tags I don't see any broadly acceptable pattern. Some > people > add Reviewed-by before the S-o-B, others add Reviewed-by after > the > S-o-B. Either is fine IMHO. From what I've seen in other projects, S-o-B means that you accept accountability for everything above. One scenario would be: - Send original patch, which has been tested inside the company: Tested-by: Tester <tester@example.com> Signed-off-by: Developper <developper@example.com> - Get some R-b, but need to make some requested minor changes and resend a new patch series: Tested-by: Tester <tester@example.com> Reviewed-by: Reviewer <reviewer@othercompany.com> Signed-off-by: Developper <developper@example.com> This is a way of saying "I guarantee that the R-b still applies after the new changes I made to this series" - Then reviewed and pulled into their tree by the maintainer: Tested-by: Tester <tester@example.com> Reviewed-by: Reviewer <reviewer@othercompany.com> Signed-off-by: Developper <developper@example.com> Reviewed-by: Maintainer <maintainer@org.org> Signed-off-by: Maintainer <maintainer@org.org> If, after being reviewed, the initial patch would not have needed any change, the order would have been: Tested-by: Tester <tester@example.com> Signed-off-by: Developper <developper@example.com> Reviewed-by: Reviewer <reviewer@othercompany.com> Reviewed-by: Maintainer <maintainer@org.org> Signed-off-by: Maintainer <maintainer@org.org> This is consistent with what software like "b4" do: if the S-o of the current user is present, it is moved last, as the current user is the one accepting accountability at this point. However, this is not what QEMU has been using as far as I can see, as S-o-b tend to stay in their original positions. I even opened an issue on b4 a few weeks ago because of this <https://github.com/mricon/b4/issues/16>, and I reverted to using git-publish. But if this is ok to use an arbitrary order for non-S-o-b headers, I can get back to b4. Sam
On Mon, 29 Jan 2024 at 09:47, Samuel Tardieu <sam@rfc1149.net> wrote: > However, this is not what QEMU has been using as far as I can see, > as S-o-b tend to stay in their original positions. I even opened > an issue on b4 a few weeks ago because of this > <https://github.com/mricon/b4/issues/16>, and I reverted to using > git-publish. But if this is ok to use an arbitrary order for > non-S-o-b headers, I can get back to b4. I think QEMU doesn't have a specific existing practice here. What you see is largely the result of people using whatever tooling they have and accepting the ordering it gives them. So I don't think you should stop using b4 just because the ordering it happens to produce isn't the same as somebody else's tooling. I think trying to impose some subtle distinction of meaning on the ordering of tags is not going to work, because there are going to be too many cases where people don't adhere to the ordering distinction because they don't know about it or don't understand it. As Daniel says, as long as the Signed-off-by tags are in basically the right order for developer vs maintainer that's the only strong ordering constraint we have. thanks -- PMM
On Mon, Jan 29, 2024 at 10:41:38AM +0000, Peter Maydell wrote: > On Mon, 29 Jan 2024 at 09:47, Samuel Tardieu <sam@rfc1149.net> wrote: > > However, this is not what QEMU has been using as far as I can see, > > as S-o-b tend to stay in their original positions. I even opened > > an issue on b4 a few weeks ago because of this > > <https://github.com/mricon/b4/issues/16>, and I reverted to using > > git-publish. But if this is ok to use an arbitrary order for > > non-S-o-b headers, I can get back to b4. > > I think QEMU doesn't have a specific existing practice here. > What you see is largely the result of people using whatever > tooling they have and accepting the ordering it gives them. > So I don't think you should stop using b4 just because > the ordering it happens to produce isn't the same as > somebody else's tooling. > > I think trying to impose some subtle distinction of meaning > on the ordering of tags is not going to work, because there > are going to be too many cases where people don't adhere > to the ordering distinction because they don't know about > it or don't understand it. > > As Daniel says, as long as the Signed-off-by tags are > in basically the right order for developer vs maintainer > that's the only strong ordering constraint we have. To think of it another way.... Signed-off-by is the only tag which has defined legal meaning in terms of asserting that the people involved have permission to contribute. All the other tags (Reviewed/Tested/etc) are merely a historical record of the development process, and have no legal implications. This makes Signed-off-by the important one, and the others all in the "nice to have" category. With regards, Daniel
diff --git a/docs/devel/code-provenance.rst b/docs/devel/code-provenance.rst new file mode 100644 index 0000000000..b4591a2dec --- /dev/null +++ b/docs/devel/code-provenance.rst @@ -0,0 +1,197 @@ +.. _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> + +This existence of this line asserts that the author of the patch is +contributing in accordance with the `Developer's Certificate of +Origin <https://developercertifcate.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 also one of the patch authors, +it is further expected that the mail ``From:`` line name & address match +one of the ``Signed-off-by`` lines. + +Multiple authorship +~~~~~~~~~~~~~~~~~~~ + +It is not uncommon for a patch to have contributions from multiple +authors. In such a scenario, a git commit will usually be expected +to have a ``Signed-off-by`` line for each contributor involved in +creatin 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. + + * 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 contributeo from their employer + who is the copyright holder. It is none the less still preferrable + 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. + +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. + + NB: a subsystem maintainer sending a pull request would replace + their own ``Reviewed-by`` with another ``Signed-off-by`` + + * **``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. + + * **``Tested-by``**: when a QEMU community member has functionally + tested the behaviour of the patch in some manner, they should + send an email reply conmtaning 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. + +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. + +The subsystem maintainer submitting a pull request is **NOT** expected to +have a ``Reviewed-by`` tag on the patch, since this is implied by their +own ``Signed-off-by``. + +Tools for adding ``Signed-of-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 the +local commits on a branch en-mass:: + + 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. + +If the abandoned patch already had a ``Signed-off-by`` from the original +author this **must** be preserved. The new contributor **must** then add +their own ``Signed-off-by`` after the original one if they made any +further changes to it. It is common to include a comment just prior to +the new ``Signed-off-by`` indicating what extra changes were made. For +example:: + + Signed-off-by: Some Person <some.person@example.com> + [Rebased and added support for 'foo'] + Signed-off-by: New Person <new.person@example.com> 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 c641d948f1..ec541b3d15 100644 --- a/docs/devel/submitting-a-patch.rst +++ b/docs/devel/submitting-a-patch.rst @@ -322,21 +322,9 @@ 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 +this and happy for it to go into QEMU". For full guidance, read the +:ref:`code-provenance` documentation. + information see `SubmittingPatches 1.12 <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
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 alot of content beyond the part about sign-off an thus is 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 | 197 ++++++++++++++++++++++++++++++ docs/devel/index-process.rst | 1 + docs/devel/submitting-a-patch.rst | 18 +-- 3 files changed, 201 insertions(+), 15 deletions(-) create mode 100644 docs/devel/code-provenance.rst