git hooks for reviews mandatory?

[Michael Pyne]

On Sat, June 21, 2014 19:34:21 Kevin Ottens wrote:
> On Saturday 21 June 2014 11:22:28 Michael Pyne wrote:
> > On Thu, June 19, 2014 23:21:22 Marco Martin 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 think it would be mostly an annoyance, but if it were possible to
> > override (REVIEW:IRC, REVIEW:TrustMe, etc.) in situations where a
> > Reviewboard request is unneeded it could help with ensuring we don't
> > accidentally commit something needing review.
> 
> Exactly why I think we need to support:
>  * REVIEW: <reviewboard id>
>  * Reviewed-By: <free string>
> 
> The free string then allow to specify "trust me" or a name.
> 
> I don't think there's much to do in a hook apart from checking at least one
> of the two is present.

Yes. Actually I didn't realize there was an entire rest of thread to this when 
I sent my reply. KMail sometimes gives me two entries for POP3-filtered mail 
and I didn't see the rest of the thread until after I'd already read the 
second entry and sent my reply.

> > We'd also want to make sure to come up with pre-commit hooks for devs to
> > use client side, or at least a git-commit template reminding to use an
> > appropriate REVIEW keyword so that devs don't have to wait until they try
> > to push to figure out their commit can't land as-is.
> 
> Sounds harder than it sound, I think quite some people rely on the following
> workflow for contributing patches:
>  * commit locally;
>  * use a script to pick the commit and push it on reviewboard;
>  * reword the commit log to add the newly allocated reviewboard id.
> 
> Which means the initial commit can't have a REVIEW tag yet... chicken and
> egg problem AFAICT.

Good point, though a git-commit template message would still be useful here, a 
similar template was made by somebody (forget who) for KDE 4 which I've used 
for a couple of years now and found helpful.

Regards,
 - Michael Pyne