signed-off-by and reviewed-by

[simo]

On Mon, 2012-11-12 at 10:10 +0100, Michael Adam wrote:
> On 2012-11-12 at 15:54 +1100, Andrew Bartlett wrote:
> > On Sun, 2012-11-11 at 22:26 -0500, simo wrote:
> > > On Mon, 2012-11-12 at 01:26 +0100, Andrew Bartlett wrote:
> > > > The branch, master has been updated
> > > >        via  e0ab14f s4:dsdb/acl_read: make sure confidential attributes require CONTROL_ACCESS (bug #8620)
> > > >        via  21dfaef s4:dsdb/acl_read: fix whitespace formatting errors
> > > >        via  f6fa724 s4:dsdb/acl: only give administrators access to attributes marked as confidential (bug #8620)
> > > >        via  ed8b275 s4:dsdb/acl: reorganize the logic flow in the password filtering checks
> > > >        via  54ad5c7 s4:dsdb/acl: fix search filter cleanup for password attributes
> > > >        via  94649e4 selftest: Avoid test cross-contamination in samba.tests.posixacl
> > > >       from  1d81e52 selftest: Add tests for expected behaviour on directories as well as files
> > > > 
> > > > http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
> > > > 
> > > > 
> > > > - Log -----------------------------------------------------------------
> > > > commit e0ab14f52a52c8317473b4c4cd3cf50265e1f9e4
> > > > Author: Stefan Metzmacher <metze at samba.org>
> > > > Date:   Fri Nov 9 17:23:53 2012 +0100
> > > > 
> > > >     s4:dsdb/acl_read: make sure confidential attributes require CONTROL_ACCESS (bug #8620)
> > > >     
> > > >     Signed-off-by: Stefan Metzmacher <metze at samba.org>
> > > >     Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > > >     
> > > >     Reviewed-by: Andrew Bartlett <abartlet at samba.org>
> > > >     
> > > >     Autobuild-User(master): Andrew Bartlett <abartlet at samba.org>
> > > >     Autobuild-Date(master): Mon Nov 12 01:25:21 CET 2012 on sn-devel-104
> > > 
> > > Andrew,
> > > please do not add sign-off-by if you are not an author.
> > 
> > I signed off that I obtained them from the author, per 
> > 
> > https://www.samba.org/samba/devel/copyright-policy.html
> 
> Well, that text describes how to sign your _own_ work that you
> send in. I.e. you _are_ the author. If you are not the author, but
> review the patches and possibly push, you should (or could) add
> "Reviewed-by: ...".
> 
> I think the case where someone who did not author the patch
> himself adds his signoff is when there is someone in the company
> who wrote the patch but does not have "open source clearance",
> but you do, so you can submit the patch on behalf of that other
> developer, giving your sign-off.  (Case (c) of the text above.)
> 
> In the case where another samba developer, even a (non-team) contributor
> that can sign-off by himself, you should not add your sign-off
> when reviewing unless you were involved in the patch creation
> yourself, e.g. modified some aspects in interactive review-roundtrips
> or similar.
> 
> That is at least my understanding of the matters.
> 
> I have started a wiki document that can, once completed and agreed
> upon, be put into the code repository and on the web site:
> https://wiki.samba.org/index.php/CodeReview
> 
> > > If I understand correctly you are not an author of the patches, you
> > > reviewed them. If you *are* an author than don't add the reviewed-by.
> > 
> > I agree, the two tags are different and distinct things.  I deliberately
> > used both for the intended purpose of each. 
> 
> I _think_ you misunderstood the purpose of sign-off in this case,
> but I may have misunderstood it myself.

Michael,
what you describe is the generally accepted meaning.

Andrew,
I think you misunderstood the meaning of sign-off-by, please read what
Michael wrote on the wiki for how and when to apply tags.

Feel free to read also the Linux kernel applicable paragraphs, we use
the same meaning for tags although for now we restrict ourselves to just
sing-off-by and reviewed-by tags.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>