Re: [PATCH v3 1/3] docs: introduce dedicated page about code provenance / sign-off

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jun 04, 2025 at 08:44:55AM +0200, Markus Armbruster wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> From: Daniel P. Berrangé <berrange@redhat.com>
> >>
> >> Currently we have a short paragraph saying that patches must include
> >> a Signed-off-by line, and merely link to the kernel documentation.
> >> The linked kernel docs have a lot of content beyond the part about
> >> sign-off an thus are misleading/distracting to QEMU contributors.
> >>
> >> This introduces a dedicated 'code-provenance' page in QEMU talking
> >> about why we require sign-off, explaining the other tags we commonly
> >> use, and what to do in some edge cases.
> >>
> >> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  docs/devel/code-provenance.rst    | 218 ++++++++++++++++++++++++++++++
> >>  docs/devel/index-process.rst      |   1 +
> >>  docs/devel/submitting-a-patch.rst |  18 +--
> >>  3 files changed, 221 insertions(+), 16 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..4fc12061b5
> >> --- /dev/null
> >> +++ b/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. Subsystem maintainers who
> >> +   review a patch should add this even if they are also adding their
> >> +   ``Signed-off-by`` to the same commit.
> >> +
> >> + * **``Acked-by``**: when a QEMU subsystem maintainer approves a patch that
> >> +   touches their subsystem, but intends to allow a different maintainer to
> >> +   queue it and send a pull request, they would send a mail containing a
> >> +   ``Acked-by`` tag. Where a patch touches multiple subsystems, ``Acked-by``
> >> +   only implies review of the maintainers' own areas of responsibility. If a
> >> +   maintainer wants to indicate they have done a full review they should use
> >> +   a ``Reviewed-by`` tag.
> >> +
> >> + * **``Tested-by``**: when a QEMU community member has functionally tested the
> >> +   behaviour of the patch in some manner, they should send an email reply
> >> +   containing a ``Tested-by`` tag.
> >> +
> >> + * **``Reported-by``**: when a QEMU community member reports a problem via the
> >> +   mailing list, or some other informal channel that is not the issue tracker,
> >> +   it is good practice to credit them by including a ``Reported-by`` tag on
> >> +   any patch fixing the issue. When the problem is reported via the GitLab
> >> +   issue tracker, however, it is sufficient to just include a link to the
> >> +   issue.
> >
> > We don't mention the Link: or Message-Id: tags.
> 
> Yes, but should it go into code-provenance.rst or
> submitting-a-patch.rst?

I considered those other general tags to be under the scope
of submitting-a-patch.rst, as they're not directly related
to the legal code provenance.

> 
> You asked for guidance on use of "Message-Id:" in your review of v2.  I
> understand the practice, and can write guidance, but I wanted to get
> this out before my vacation next week, so I left it for later, as
> mentioned in the cover letter.
> 
> How do we use "Link:"?  What about "Closes:"?
> 
> Here's what the kernel's submitting-patches.rst has to say:
> 
>     Describe your changes
>     ---------------------
> 
>     [...]
> 
>     If related discussions or any other background information behind the change
>     can be found on the web, add 'Link:' tags pointing to it. If the patch is a
>     result of some earlier mailing list discussions or something documented on the
>     web, point to it.
> 
>     When linking to mailing list archives, preferably use the lore.kernel.org
>     message archiver service. To create the link URL, use the contents of the
>     ``Message-ID`` header of the message without the surrounding angle brackets.
>     For example::
> 
>         Link: https://lore.kernel.org/30th.anniversary.repost@klaava.Helsinki.FI
> 
>     Please check the link to make sure that it is actually working and points
>     to the relevant message.
> 
>     However, try to make your explanation understandable without external
>     resources. In addition to giving a URL to a mailing list archive or bug,
>     summarize the relevant points of the discussion that led to the
>     patch as submitted.
> 
>     In case your patch fixes a bug, use the 'Closes:' tag with a URL referencing
>     the report in the mailing list archives or a public bug tracker. For example::
> 
>             Closes: https://example.com/issues/1234
> 
>     Some bug trackers have the ability to close issues automatically when a
>     commit with such a tag is applied. Some bots monitoring mailing lists can
>     also track such tags and take certain actions. Private bug trackers and
>     invalid URLs are forbidden.
> 
> and
> 
>     Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
>     ----------------------------------------------------------------------
> 
>     The Reported-by tag gives credit to people who find bugs and report them and it
>     hopefully inspires them to help us again in the future. The tag is intended for
>     bugs; please do not use it to credit feature requests. The tag should be
>     followed by a Closes: tag pointing to the report, unless the report is not
>     available on the web. The Link: tag can be used instead of Closes: if the patch
>     fixes a part of the issue(s) being reported. Note, the Reported-by tag is one
>     of only three tags you might be able to use without explicit permission of the
>     person named (see 'Tagging people requires permission' below for details).
> 

> >> +git commands
> >> +^^^^^^^^^^^^
> >> +
> >> +When creating, or amending, a commit the ``-s`` flag to ``git commit`` will
> >> +append a suitable line matching the configured git author details.
> >> +
> >> +If preparing patches using the ``git format-patch`` tool, the ``-s`` flag can
> >> +be used to append a suitable line in the emails it creates, without modifying
> >> +the local commits. Alternatively to modify all the local commits on a branch::
> >> +
> >> +  git rebase master -x 'git commit --amend --no-edit -s'
> >> +
> >
> > Much as I love Emacs I wonder if this next section is worth it given the
> > multiple ways you can solve this (I use yas-snippet expansions for
> > example).
> 
> Showing one of them could still be useful for less experienced Emacs
> users.  We could mention it's just of many ways.

Yep, IMHO it is worth guiding users to a simple example that works.
If they are advanced users of emacs or other editors wanting to figure
out other options they can ignore this guidance.

> 
> > If we do want to mention the editors we should probably also mention b4.
> 
> Can do if somebody contributes a suitable configuration snippet.
> 
> Thanks!
> 
> [...]
> 

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