Code review required for commits - formal Team vote.

Michael Adam obnox at samba.org
Tue Oct 23 06:10:25 MDT 2012


On 2012-10-23 at 08:03 -0400, simo wrote:
> On Tue, 2012-10-23 at 08:49 +0200, Michael Adam wrote:
> > 
> > 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.

Er, yes, that is what I tried to express. :-)

Interestingly, patchwork seems to have the concept of patch
bundles internally.

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

Yeah, I can do that.

I personally prefer to use "git format-patch" and attach the output
to mails. If working with patchsets, I use "git format-patch
--stdout" to create a single mbox-style file instead of having
to deal with lots of attachment files or even (worse) lots of
mails for one logical unit. But maybe it is only a matter of
getting used to ot

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

Right. But that redundancy would clearly separate it from our
earlier (mis-)use where we frequently added sign-off tags
to indicate review.

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

Thanks - 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/20121023/e5bfeb0d/attachment.pgp>


More information about the samba-technical mailing list