Code review required for commits - formal Team vote.

[simo]

On Tue, 2012-10-23 at 08:49 +0200, Michael Adam wrote:
> Hi Jeremy,
> Hi list,
> 
> while the discussion about a formally committed test period seems
> to have been on hold to some extend for the last week or so,
> people have been busy discussing the more tool- and workflow-specific
> points of the review business. I have also sensed a great level
> of willingness to actually do reviews and work on th process.
> 
> Some points that have been touched:
> 
> tracking tool -  patchwork:
> 
>   Simo has set up patchwork for the samba code.
>   See the corresponding subthread for details.
> 
>   It looks rather promising. The only thing I am not quite
>   certain about is the inability to track whole patchsets
>   handed in by mail.

This came out funny, I think you meant:
patchwork can't handle multiple patches in a single mail message.

> git send-email:
> 
>   There have been various hints and discussions about
>   using git send-email.
>   It would be great to collect these on a wiki page.

can you create a page and send the link to the Mailing List ?
I will put my advice in there.

> commit messsage tags:
> 
>   Some of us had started to use "Signed-off-by: " as a
>   tag for reviews in the commit messages. As pointed out
>   by Simo, this is not what the Linux kernel uses Signed-off-by
>   for. Here is a proposal, which summarizes our discussion:
> 
>   - The Author(s) of a patch should add their signature to
>     the commit message by adding a "Signed-off-by: " tag.
> 
>     This tag indicates that the the person it lists was
>     involved in the creation of the patch, that it is fine
>     with the state of the patch and submits it under the
>     license(s) of the affected files.
> 
>   - If the patch was done by various people, they can also
>     add the "Pair-programmed-with: " tag.

Note that with 2 sign-offs this becomes effectively redundant, so I
would phase it out to keep things simple.

>   - A reviewer should add a "Reviewed-by: " tag.
> 
>   Here is a reference for the linux kernel document describing
>   the various tags used by linux:
>   http://www.kernel.org/doc/Documentation/SubmittingPatches
> 
>   There is also the "Acked-by: " patch, but I suggest that we
>   don't use it for now to keep things simple, since it does not
>   make a lot of sense, unless we have established maintainers
>   all over the code base and those need to Ack patches for their
>   areas.
> 
>   I also talked with some git folks, whether native support for
>   adding "reviewed-by" like for "signed-off-by" coule be added
>   to git, but I think chances are bad, because they say the don't
>   want a reviewer to change the commit message and hence the
>   commit object hash... They suggest to instead use merges and
>   use the merge commit for indicating reviewed state. I
>   personally don't like that very much, and think we should stay
>   with the message-changing pick/rebase model for now.

+1

Simo.

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