Re: [gentoo-portage-dev] [PATCH v2 1/3] repoman: Add commit message verification

[Brian Dolbec <dolsen@gentoo.org>]

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

On Sat, 17 Feb 2018 13:18:03 +0000
"M. J. Everitt" <m.j.everitt@iee.org> wrote:

> On 17/02/18 12:56, Michał Górny wrote:
> > Add a check for common mistakes in commit messages. For now, it is
> > pretty rough and works only for -m/-F. It will be extended to work
> > in the interactive mode in the future.
> > ---
> >  repoman/pym/repoman/actions.py                  | 74
> > ++++++++++++++++++++++++-
> > repoman/pym/repoman/tests/commit/__init__.py    |  2 +
> > repoman/pym/repoman/tests/commit/__test__.py    |  1 +
> > repoman/pym/repoman/tests/simple/test_simple.py |  8 +-- 4 files
> > changed, 80 insertions(+), 5 deletions(-) create mode 100644
> > repoman/pym/repoman/tests/commit/__init__.py create mode 100644
> > repoman/pym/repoman/tests/commit/__test__.py
> >
> > diff --git a/repoman/pym/repoman/actions.py
> > b/repoman/pym/repoman/actions.py index b76a6e466..91603865c 100644
> > --- a/repoman/pym/repoman/actions.py
> > +++ b/repoman/pym/repoman/actions.py
> > @@ -119,7 +119,16 @@ class Actions(object):
> >  			if commitmessage[:9].lower() in ("cat/pkg:
> > ",): commitmessage = self.msg_prefix() + commitmessage[9:]
> >  
> > -		if not commitmessage or not commitmessage.strip():
> > +		if commitmessage is not None and
> > commitmessage.strip():
> > +			res, expl =
> > self.verify_commit_message(commitmessage)
> > +			if not res:
> > +				print(bad("RepoMan does not like
> > your commit message:"))
> > +				print(expl)
> > +				if self.options.force:
> > +					print('(but proceeding due
> > to --force)')
> > +				else:
> > +					sys.exit(1)
> > +		else:
> >  			commitmessage =
> > self.get_new_commit_message(qa_output) 
> >  		commitmessage = commitmessage.rstrip()
> > @@ -585,3 +594,66 @@ class Actions(object):
> >  			print("* no commit message?  aborting
> > commit.") sys.exit(1)
> >  		return commitmessage
> > +
> > +	footer_re = re.compile(r'^[\w-]+:')
> > +
> > +	@classmethod
> > +	def verify_commit_message(cls, msg):
> > +		"""
> > +		Check whether the message roughly complies with
> > GLEP66. Returns
> > +		(True, None) if it does, (False, <explanation>) if
> > it does not.
> > +		"""
> > +
> > +		problems = []
> > +		paras = msg.strip().split('\n\n')
> > +		summary = paras.pop(0)
> > +
> > +		if ':' not in summary:
> > +			problems.append('summary line must start
> > with a logical unit name, e.g. "cat/pkg:"')
> > +		if '\n' in summary.strip():
> > +			problems.append('commit message must start
> > with a *single* line of summary, followed by empty line')
> > +		# accept 69 overall or unit+50, in case of very
> > long package names
> > +		elif len(summary.strip()) > 69 and
> > len(summary.split(':', 1)[-1]) > 50:
> > +			problems.append('summary line is too long
> > (max 69 characters)') +
> > +		multiple_footers = False
> > +		gentoo_bug_used = False
> > +		bug_closes_without_url = False
> > +		body_too_long = False
> > +
> > +		found_footer = False
> > +		for p in paras:
> > +			lines = p.splitlines()
> > +			# if all lines look like footer material,
> > we assume it's footer
> > +			# else, we assume it's body text
> > +			if all(cls.footer_re.match(l) for l in
> > lines if l.strip()):
> > +				# multiple footer-like blocks?
> > +				if found_footer:
> > +					multiple_footers = True
> > +				found_footer = True
> > +				for l in lines:
> > +					if
> > l.startswith('Gentoo-Bug'):
> > +						gentoo_bug_used =
> > True
> > +					elif l.startswith('Bug:')
> > or l.startswith('Closes:'):
> > +						if 'http://' not
> > in l and 'https://' not in l:
> > +
> > bug_closes_without_url = True
> > +			else:
> > +				for l in lines:
> > +					# we recommend wrapping at
> > 72 but accept up to 80;
> > +					# do not complain if we
> > have single word (usually
> > +					# it means very long URL)
> > +					if len(l.strip()) > 80 and
> > len(l.split()) > 1:
> > +						body_too_long =
> > True +
> > +		if multiple_footers:
> > +			problems.append('multiple footer-style
> > blocks found, please combine them into one')
> > +		if gentoo_bug_used:
> > +			problems.append('please replace Gentoo-Bug
> > with GLEP 66-compliant Bug/Closes')
> > +		if bug_closes_without_url:
> > +			problems.append('Bug/Closes footers
> > require full URL')
> > +		if body_too_long:
> > +			problems.append('body lines should be
> > wrapped at 72 (max 80) characters') +
> > +		if problems:
> > +			return (False, '\n'.join('- %s' % x for x
> > in problems))
> > +		return (True, None)
> > diff --git a/repoman/pym/repoman/tests/commit/__init__.py
> > b/repoman/pym/repoman/tests/commit/__init__.py new file mode 100644
> > index 000000000..d74fd94a7
> > --- /dev/null
> > +++ b/repoman/pym/repoman/tests/commit/__init__.py
> > @@ -0,0 +1,2 @@
> > +# Copyright 2011-2018 Gentoo Foundation
> > +# Distributed under the terms of the GNU General Public License v2
> > diff --git a/repoman/pym/repoman/tests/commit/__test__.py
> > b/repoman/pym/repoman/tests/commit/__test__.py new file mode 100644
> > index 000000000..8b1378917
> > --- /dev/null
> > +++ b/repoman/pym/repoman/tests/commit/__test__.py
> > @@ -0,0 +1 @@
> > +
> > diff --git a/repoman/pym/repoman/tests/simple/test_simple.py
> > b/repoman/pym/repoman/tests/simple/test_simple.py index
> > a24e0d5a3..3d7a70ad0 100644 ---
> > a/repoman/pym/repoman/tests/simple/test_simple.py +++
> > b/repoman/pym/repoman/tests/simple/test_simple.py @@ -1,4 +1,4 @@
> > -# Copyright 2011-2015 Gentoo Foundation
> > +# Copyright 2011-2018 Gentoo Foundation
> >  # Distributed under the terms of the GNU General Public License v2
> >  
> >  import subprocess
> > @@ -194,13 +194,13 @@ class SimpleRepomanTestCase(TestCase):
> >  			("", repoman_cmd + ("full", "-d")),
> >  			("", cp_cmd + (test_ebuild,
> > test_ebuild[:-8] + "2.ebuild")), ("", git_cmd + ("add",
> > test_ebuild[:-8] + "2.ebuild")),
> > -			("", repoman_cmd + ("commit", "-m", "bump
> > to version 2")),
> > +			("", repoman_cmd + ("commit", "-m",
> > "cat/pkg: bump to version 2")), ("", cp_cmd + (test_ebuild,
> > test_ebuild[:-8] + "3.ebuild")), ("", git_cmd + ("add",
> > test_ebuild[:-8] + "3.ebuild")),
> > -			("dev-libs", repoman_cmd + ("commit",
> > "-m", "bump to version 3")),
> > +			("dev-libs", repoman_cmd + ("commit",
> > "-m", "cat/pkg: bump to version 3")), ("", cp_cmd + (test_ebuild,
> > test_ebuild[:-8] + "4.ebuild")), ("", git_cmd + ("add",
> > test_ebuild[:-8] + "4.ebuild")),
> > -			("dev-libs/A", repoman_cmd + ("commit",
> > "-m", "bump to version 4")),
> > +			("dev-libs/A", repoman_cmd + ("commit",
> > "-m", "cat/pkg: bump to version 4")), )
> >  
> >  		env = {  
> Might I suggest breaking out checks into a separate module? I think
> that hard-coding it all is likely to become a pain as time goes on,
> more checks get added, etc  ...
> 


So, you would like the commit message checks to be  a plug-in system
like the ebuild checks are now.

Yes that is possible,  that would also allow overlays and sub-distros
to customize it to their liking.  But you do realize it would be a
module system with only the one plug-in.  It would add a slight amount
of overhead and subsequently a tiny bit more time.  Although it is not
much as I remember when I first developed the plug-in system for emaint.

I will consider that for the stage3 work in progress, but for the
master branch of repoman, the hard-coded version above would be fine.

-- 
Brian Dolbec <dolsen>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]