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

["M. J. Everitt" <m.j.everitt@iee.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6433 bytes --]

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


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