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

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

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