Code review required for commits - formal Team vote.

[Michael Adam]

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.

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.

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.

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

Cheers - Michael

On 2012-10-17 at 00:07 -0700, Jeremy Allison wrote:
> On Wed, Oct 17, 2012 at 01:00:58AM +0200, Michael Adam wrote:
> > So, since we are discussing anyways, and the discussion takes
> > a lot longer and gets a lot more heated than I had anticipated,
> > I think we should take some momentum out of it and not force
> > things.
> > 
> > SUGGESTION:
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 1. I suggest that we defer a team vote on a commited trial period
> >    or even a policy change for some time, at least a couple of weeks
> >    until we have thought and discussed things through thoroughly
> >    and without hastening, maybe until after 4.0 is released.
> 
> I really don't want to wait that long. IMHO I think we
> do need to resolve this as soon as possible. "Waiting for
> 4.0" means next year, which I think is too late.
> 
> > 2. In the meanwhile, we encourage team members to do reviews
> >    more often on a voluntary basis, which means both asking for
> >    review before pushing and giving review for others.
> 
> I've certainly started to lead by example here, and
> will continue to do so.
> 
> > 3. We also encourage to have the reviewer push.
> 
> Again, I will adhere to this.
> 
> > 4. We can also try any mechanisms/tools on a voluntary basis.
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This would be really helpful. Do you have any in particular
> you'd like to set up ?
> 
> > This allows us to concentrate on shipping 4.0 which is the most
> > important goal currently! The worst we can do now is delay or
> > spoil the 4.0 effort by working ourselves into the ground over
> > heated policy discussions.
> 
> Shipping 4.0.0 proceeds in parallel, based on bug reports.
> I think we can do both (I certainly can :-).
> 
> > And it allows us on a completely voluntary basis to gain more
> > experience and confidence in the reviewing business and possibly
> > get an idea of the load and mechanisms.
> 
> The problem with "completely voluntary" is that master will
> keep getting screwed up with poorly reviewed patches.
> 
> Note I'm as guilty as anyone here of this, but I'm really,
> *really* trying to fix it.
> 
> As Christian and Ira (and many others) have noted, breaking the
> master build should no longer be acceptible.
> 
> > This will certainly move towards mandatory reviews much slower
> > than the hard trial period, but it is more respectful for the
> > folks that have serious concerns. I think that we will still make
> > progress, based on the following observations:
> > 
> > - I think we can safely assume that we all want to sustain and
> >   improve the quality of our commits and would consider changing
> >   our mode of operation on the master branch if suitable.
> > 
> > - I got from mails and discussions that nobody actually
> >   thinks that doing reviews is a bad thing!
> 
> Well this is progress !
> 
> > The concerns we should address are these:
> > 
> > - How will we provide the ressources to review 6000 patches / year?
> >   (or will it imply a drop in productivity?)
> 
> 6000 patches, but how many *patchsets* ? Patches don't get
> reviewed, patchsets do. There's an important distinction
> here.
> 
> > - Do we need a tracking tool for reviews so patches don't get lost?
> >   (this is roughly a.k.a how do I get a reviewer for my patch?)
> 
> Right now I'm using my inbox. What would you suggest as
> a different tool ? At work we use gerrit, but Simo has
> some valid (Java cough, cough :-) reasons to avoid that :-).
> 
> > - Can we find *two* team reviewers even for the most exotic areas
> >   in code ore do we need to establish more flexible rules?
> 
> Yes, we can certainly do this. In fact we *must*. No area
> of the code should be understood by just one person, that
> is a terrible "bus-factor" to use a Linux kernel term :-).
> 
> > I hope that even a voluntary period (as has been suggested by
> > others) will lead us to some insight about theses issues in the
> > next couple of weeks and that we can move forward with voting
> > about something more concrete and with a better feeling attached
> > to it after 4.0 is out.
> 
> That's too much like waiting for Godot to me :-). I think we
> can proceed without waiting for 4.0.0.
> 
> > PS: Jeremy, I apologize for back-pedalling here. I still think
> > we should do it, but not by all means. This is the only way
> > I see how we can continue in a reasonable and respectful way.
> 
> We need to address this issue in a timely fashion. What time
> limit are you suggesting on discussions ?
> 
> What concerns me about "agreeing to more discussions" is 
> this is an open-ended way to indefinitely postpone the
> issue. I don't want that to happen.
> 
> Jeremy.

-------------- 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/88be4070/attachment.pgp>