update hook for git to check push commit message

update hook for git to check push commit message

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

Hi

Attached is a small hook for updates that are pushed into repo.
Right now it only checks the first line of the commit and expects
module: summary

I have not tested it at all

Comments ?

Thanks
-Khem



[-- Attachment #2: update.py --]
[-- Type: text/x-python, Size: 1127 bytes --]

#!/usr/bin/env python
# usage update.py <oldrev> <newrev>
# currently it only checks for first line of the commit message
# which should look like e.g
# uclibc: <pretty summary>

import sys,os
from subprocess import *
oldrev = sys.argv[1]
newrev = sys.argv[2]
user = os.getenv('USER')
def ErrorOut(message):
    print "Please reformat the message per guidelines on http://wiki.openembedded.org/index.php/Commit_Policy"
    print "A sample is at http://wiki.openembedded.org/index.php/Commit_log_example"
    print "\nFaulty Commit Message:\n\n ", message
    sys.exit(1)

p = os.popen("git rev-list %s...%s" %(oldrev, newrev))
missed_revs = p.read()
for r in missed_revs.split():
    m1 = Popen(["git", "cat-file", "commit", r],stdout=PIPE)
    m2 = Popen(["sed", "1,/^$/d"], stdin=m1.stdout, stdout=PIPE)
    rev = m2.communicate()[0]
    first_word = rev.split()[0];
    if first_word[-1] is ':':
	print "Your message is not formatted according to commit poilicy"
	ErrorOut(rev)
    summary = rev.split(1)
    if summary is "":
	print "Commit Message does not have summary line"
	ErrorOut(rev)

Re: update hook for git to check push commit message

[Phil Blundell]

On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> Attached is a small hook for updates that are pushed into repo.
> Right now it only checks the first line of the commit and expects
> module: summary

I'm not really in favour of applying the commit message policy quite
that harshly.  The wiki page that you linked to doesn't really seem to
support this kind of check: the actual guidance it gives about checkin
messages is:

* Have a clear commit message (example):
    - The first line of commit is a summary of the changes.
    - The first line should start with the name of the recipe the change affects.
    - The rest of the message should give more details on the change as appropriate.
    - Mention the affected bug numbers if appropriate.
    - Give credit where credit is due. If you commit someone else's work more or 
      less verbatim, you should use git commit --author $mail-of-author. If pulling
      changes from somewhere like Poky or OpenMoko there is no problem with that
      but mention where the changes came from.
    - Include a Signed-off-by: line indicating the change has valid certificate 
      of origin as per the Linux kernel

... which is nowhere near as prescriptive as the rule that you seem to
be implementing in your script.  Also, the wiki page has an explicit
statement that "[these] rules are not hard fast rules", which would
suggest that even such guidance as it does provide is not intended to be
taken too dogmatically. 

Personally I think that the current situation of the TSC applying
pressure to those people who seem to be chronically unable to write
suitable checkin comments is a perfectly fine way of solving the
problem.

On a technical level, I think you could probably do without the sed
subprocess, and "summary = rev.split(1)" doesn't look quite right
either.

p.


p.

Re: update hook for git to check push commit message

[Frans Meulenbroeks]

2010/1/28 Phil Blundell <philb@gnu.org>:
> On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
>> Attached is a small hook for updates that are pushed into repo.
>> Right now it only checks the first line of the commit and expects
>> module: summary
>
> I'm not really in favour of applying the commit message policy quite
> that harshly.  The wiki page that you linked to doesn't really seem to
> support this kind of check: the actual guidance it gives about checkin
> messages is:
>
> * Have a clear commit message (example):
>    - The first line of commit is a summary of the changes.
>    - The first line should start with the name of the recipe the change affects.

What if a commit is not about a recipe but e.g. about a class file
and what if a commit changes multiple recipes ? (i had this one once
when eliminating lots of perl native recipes replacing them with
BBCLASSEXTEND="native")
This was a pain editing already. I think I would have stopped doing it
if I had to commit each individual patch

>    - The rest of the message should give more details on the change as appropriate.
>    - Mention the affected bug numbers if appropriate.
>    - Give credit where credit is due. If you commit someone else's work more or
>      less verbatim, you should use git commit --author $mail-of-author. If pulling
>      changes from somewhere like Poky or OpenMoko there is no problem with that
>      but mention where the changes came from.
>    - Include a Signed-off-by: line indicating the change has valid certificate
>      of origin as per the Linux kernel

Not sure if we can influence the template that you get with git commit
but I would surely like to learn if one way or another I can add a
Signed-off-by lne automatically

>
> ... which is nowhere near as prescriptive as the rule that you seem to
> be implementing in your script.  Also, the wiki page has an explicit
> statement that "[these] rules are not hard fast rules", which would
> suggest that even such guidance as it does provide is not intended to be
> taken too dogmatically.
>
> Personally I think that the current situation of the TSC applying
> pressure to those people who seem to be chronically unable to write
> suitable checkin comments is a perfectly fine way of solving the
> problem.

Agree

FM

>
> On a technical level, I think you could probably do without the sed
> subprocess, and "summary = rev.split(1)" doesn't look quite right
> either.
>
> p.
>
>
> p.
>
>
>
> _______________________________________________
> Openembedded-devel mailing list
> Openembedded-devel@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-devel
>

Re: update hook for git to check push commit message

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

Am Donnerstag, den 28.01.2010, 23:01 +0100 schrieb Frans Meulenbroeks:
> 2010/1/28 Phil Blundell <philb@gnu.org>:

[…]

> >    - Include a Signed-off-by: line indicating the change has valid certificate
> >      of origin as per the Linux kernel
> 
> Not sure if we can influence the template that you get with git commit
> but I would surely like to learn if one way or another I can add a
> Signed-off-by lne automatically

	$ git commit -s

[…]


Thanks,

Paul

[-- Attachment #2: Dies ist ein digital signierter Nachrichtenteil --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

Re: update hook for git to check push commit message

[Khem Raj]

On (28/01/10 21:52), Phil Blundell wrote:
> On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> > Attached is a small hook for updates that are pushed into repo.
> > Right now it only checks the first line of the commit and expects
> > module: summary
> 
> I'm not really in favour of applying the commit message policy quite
> that harshly.  The wiki page that you linked to doesn't really seem to
> support this kind of check: the actual guidance it gives about checkin
> messages is:

Thats true. However I have seen many devs complain about the module:
summary. That was my motivation. 

> 
> * Have a clear commit message (example):
>     - The first line of commit is a summary of the changes.
>     - The first line should start with the name of the recipe the change affects.
>     - The rest of the message should give more details on the change as appropriate.
>     - Mention the affected bug numbers if appropriate.
>     - Give credit where credit is due. If you commit someone else's work more or 
>       less verbatim, you should use git commit --author $mail-of-author. If pulling
>       changes from somewhere like Poky or OpenMoko there is no problem with that
>       but mention where the changes came from.
>     - Include a Signed-off-by: line indicating the change has valid certificate 
>       of origin as per the Linux kernel
> 
> ... which is nowhere near as prescriptive as the rule that you seem to
> be implementing in your script.  Also, the wiki page has an explicit
> statement that "[these] rules are not hard fast rules", which would
> suggest that even such guidance as it does provide is not intended to be
> taken too dogmatically. 
> 
> Personally I think that the current situation of the TSC applying
> pressure to those people who seem to be chronically unable to write
> suitable checkin comments is a perfectly fine way of solving the
> problem.

Probably it could help developers to check the commit messages on their own
by making this as post commit or pre push hook of some sort in their clones .git/hooks

> 
> On a technical level, I think you could probably do without the sed
> subprocess, and "summary = rev.split(1)" doesn't look quite right
> either.

true. I could have got it if I have tested it. The check for : should also
be inverted.

Re: update hook for git to check push commit message

[Tom Rini]

On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> Hi
> 
> Attached is a small hook for updates that are pushed into repo.
> Right now it only checks the first line of the commit and expects
> module: summary

Is there any way to force the commit to happen, even if the script
doesn't like it?  Can the script edit the commit message, even?  ie if
there's no git commit --ignore-prehook type option, could it see if the
first line is FORCE or something, edit that out and commit?  This will
catch the poorly formed commit messages without being a big burden on
people doing things that don't fit well into the "module: summary" model
(that said, lib*-perl-native: Convert to BBCLASSEXTEND as a first line
makes sense to me).

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: update hook for git to check push commit message

[Chris Larson]

On Thu, Jan 28, 2010 at 3:59 PM, Tom Rini <tom_rini@mentor.com> wrote:

> On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> > Hi
> >
> > Attached is a small hook for updates that are pushed into repo.
> > Right now it only checks the first line of the commit and expects
> > module: summary
>
> Is there any way to force the commit to happen, even if the script
> doesn't like it?  Can the script edit the commit message, even?  ie if
> there's no git commit --ignore-prehook type option, could it see if the
> first line is FORCE or something, edit that out and commit?  This will
> catch the poorly formed commit messages without being a big burden on
> people doing things that don't fit well into the "module: summary" model
> (that said, lib*-perl-native: Convert to BBCLASSEXTEND as a first line
> makes sense to me).
>

My worry with editing a commit at push time is, the user's local repository
now no longer matches upstream.  One would hope a rebase would intelligently
handle it, but from a configuration management perspective I don't think
modifying commits behind the user's back is a very sane thing.

Now, if you did this locally at commit time rather than push time, I think
that would be less crazy, but unfortunately a clone can't bring along hooks
:\
-- 
Chris Larson
clarson at kergoth dot com
clarson at mvista dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Software Engineer
MontaVista Software, Inc.

Re: update hook for git to check push commit message

[Tom Rini]

On Thu, 2010-01-28 at 16:22 -0700, Chris Larson wrote:
> On Thu, Jan 28, 2010 at 3:59 PM, Tom Rini <tom_rini@mentor.com> wrote:
> 
> > On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> > > Hi
> > >
> > > Attached is a small hook for updates that are pushed into repo.
> > > Right now it only checks the first line of the commit and expects
> > > module: summary
> >
> > Is there any way to force the commit to happen, even if the script
> > doesn't like it?  Can the script edit the commit message, even?  ie if
> > there's no git commit --ignore-prehook type option, could it see if the
> > first line is FORCE or something, edit that out and commit?  This will
> > catch the poorly formed commit messages without being a big burden on
> > people doing things that don't fit well into the "module: summary" model
> > (that said, lib*-perl-native: Convert to BBCLASSEXTEND as a first line
> > makes sense to me).
> >
> 
> My worry with editing a commit at push time is, the user's local repository
> now no longer matches upstream.  One would hope a rebase would intelligently
> handle it, but from a configuration management perspective I don't think
> modifying commits behind the user's back is a very sane thing.
> 
> Now, if you did this locally at commit time rather than push time, I think
> that would be less crazy, but unfortunately a clone can't bring along hooks
> :\

Ah, I forgot it would be on the server side and I don't see a way to
force the push if a hook fails.

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: update hook for git to check push commit message

[Jens Seidel]

On Thu, Jan 28, 2010 at 04:22:32PM -0700, Chris Larson wrote:
> On Thu, Jan 28, 2010 at 3:59 PM, Tom Rini <tom_rini@mentor.com> wrote:
> > On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> > > Attached is a small hook for updates that are pushed into repo.
> >
> > Is there any way to force the commit to happen, even if the script
> > doesn't like it?  Can the script edit the commit message, even?  ie if
> > there's no git commit --ignore-prehook type option, could it see if the
> > first line is FORCE or something, edit that out and commit?  This will
> > catch the poorly formed commit messages without being a big burden on
> > people doing things that don't fit well into the "module: summary" model
> > (that said, lib*-perl-native: Convert to BBCLASSEXTEND as a first line
> > makes sense to me).
> 
> My worry with editing a commit at push time is, the user's local repository
> now no longer matches upstream.

Right, what about two repositories, one with a hook which denies some
commits and another one without such a hook. The second repository could be
synced with the first via a cronjob and users are always forced to commit
to the first repository except they know *exactly* what they are doing
(maybe by restricting access to a few people only) and want to bypass these
checks?

Jens

Re: update hook for git to check push commit message

[Martin Jansa]

On Fri, Jan 29, 2010 at 09:21:38AM +0100, Jens Seidel wrote:
> On Thu, Jan 28, 2010 at 04:22:32PM -0700, Chris Larson wrote:
> > On Thu, Jan 28, 2010 at 3:59 PM, Tom Rini <tom_rini@mentor.com> wrote:
> > > On Thu, 2010-01-28 at 12:00 -0800, Khem Raj wrote:
> > > > Attached is a small hook for updates that are pushed into repo.
> > >
> > > Is there any way to force the commit to happen, even if the script
> > > doesn't like it?  Can the script edit the commit message, even?  ie if
> > > there's no git commit --ignore-prehook type option, could it see if the
> > > first line is FORCE or something, edit that out and commit?  This will
> > > catch the poorly formed commit messages without being a big burden on
> > > people doing things that don't fit well into the "module: summary" model
> > > (that said, lib*-perl-native: Convert to BBCLASSEXTEND as a first line
> > > makes sense to me).
> > 
> > My worry with editing a commit at push time is, the user's local repository
> > now no longer matches upstream.
> 
> Right, what about two repositories, one with a hook which denies some
> commits and another one without such a hook. The second repository could be
> synced with the first via a cronjob and users are always forced to commit
> to the first repository except they know *exactly* what they are doing
> (maybe by restricting access to a few people only) and want to bypass these
> checks?

This looks like a bit overcomplicated sollution. I think that every
commit message "on edge" of commit policy can be made to go through.

ie: 
fso_recipes: switch from SRCREV to SRCPV
instead of
fso recipes: switch from SRCREV to SRCPV

lib*-perl-native: Convert to BBCLASSEXTEND
could go through as long as it's splited by ' '

is almost as readable as original and easier to s/ /_/ then pusing to
different repo and waiting for cron job to sync it, hoping that nobody
else didn't commit conflicting change to right repo with right commit
message first.

Regards,

-- 
uin:136542059                jid:Martin.Jansa@gmail.com
Jansa Martin                 sip:jamasip@voip.wengo.fr 
JaMa