signed-off-by and reviewed-by

[Michael Adam]

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.

Cheers - Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121112/6bf486d2/attachment.pgp>