git hooks for reviews mandatory?

[Kevin Ottens]

On Friday 20 June 2014 13:13:20 Aleix Pol wrote:
> On Fri, Jun 20, 2014 at 7:40 AM, Kevin Ottens <ervin at kde.org> wrote:
> > On Friday 20 June 2014 01:46:10 Aleix Pol wrote:
> > > On Thu, Jun 19, 2014 at 11:21 PM, Marco Martin <notmart at gmail.com>
> > 
> > wrote:
> > > > Hi all,
> > > > I was thinking, since the policy for committing in frameworks is to
> > 
> > always
> > 
> > > > asking for a review, what about on repositories under frameworks/*
> > 
> > adding
> > 
> > > > an
> > > > hook that accepts pushes only if the comment has a REVIEW: line?
> > > > 
> > > > I have been guilty too many times of not respecting that, mostly for
> > 
> > not
> > 
> > > > thinking about it at all, maybe I'm not the only one, an artificial
> > > > enforce of
> > > > discipline like that *may* make sense.
> > > > 
> > > > opinions? would be useful, or mostly just an annoyance?
> > > 
> > > I've heard of many complaints about how noisy is kde-frameworks mailing
> > > list because of review requests.
> > 
> > Well, the policy doesn't necessarily mean going through reviewboard. As
> > pointed by Luigi "Reviewed by:" is also allowed, and that can be through a
> > pastebin over IRC or pair programming or whatever you want to get
> > something
> > more synchronous and direct as a review. Reviewboard is really for the
> > asynchronous case (no relevant people available) or "I'm not sure what I'm
> > doing" case. The rest could be done through other means of reviews.
> > 
> > > Also, I fear there's people not working at
> > > full speed in their respective projects because of review requests.
> > 
> > Yeah, let's get crap out at full speed. :-)
> > 
> > I think I see what you mean. That said, I'd like to remind being careful
> > with
> > the "full speed" argument.
> > 
> > >  Such change would scare me a bit, TBH.
> > 
> > If we limit it to "REVIEW:" then it would concern me as well as it'd force
> > too
> > much of a workflow. If we support both "REVIEW:" and "Reviewed by:" then
> > it is
> > less of a concern IMO.
> > 
> > Regards.
> > --
> > Kévin Ottens, http://ervin.ipsquad.net
> > 
> > KDAB - proud supporter of KDE, http://www.kdab.com
> > 
> > 
> > _______________________________________________
> > Kde-frameworks-devel mailing list
> > Kde-frameworks-devel at kde.org
> > https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
> 
> What about merging a branch? It would be good if only the merge commit
> required the review statement.

Well, as discussed, we try to avoid branches altogether. If that's a local 
branch, you can always compensate that locally easily... Now if we assume one 
of those seldom published branches, the question is relevant. I wonder if 
that's easy to do with the git hook.

Doesn't it just mean allowing commits without review tags in the branches 
other than master? Then the merge commit will be in master and so have the 
constraint.

Looks like something doable to me.
 
Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud supporter of KDE, http://www.kdab.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140621/801cdf9b/attachment-0001.sig>