Re: [gentoo-portage-dev] [PATCH] repoman: Add commit message verification

[Brian Dolbec]

On Fri,  2 Feb 2018 23:53:05 +0100
Michał Górny <mgorny@g.o> wrote:

> Add a check for common mistakes in commit messages. For now, it is
> pretty rough and exits immediately but it should be integrated with
> the editor in the future.
> ---
>  repoman/pym/repoman/actions.py | 68
> ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68
> insertions(+)
>
> diff --git a/repoman/pym/repoman/actions.py
> b/repoman/pym/repoman/actions.py index b76a6e466..97a71d0f5 100644
> --- a/repoman/pym/repoman/actions.py
> +++ b/repoman/pym/repoman/actions.py
> @@ -124,6 +124,15 @@ class Actions(object):
>
>  		commitmessage = commitmessage.rstrip()
>
> +		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)
> +
>  		# Update copyright for new and changed files
>  		year = time.strftime('%Y', time.gmtime())
>  		for fn in chain(mynew, mychanged):
> @@ -585,3 +594,62 @@ class Actions(object):
>  			print("* no commit message?  aborting
> commit.") sys.exit(1)
>  		return commitmessage
> +
> +	footer_re = re.compile(r'^[\w-]+:')
> +
> +	def verify_commit_message(self, 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 '\n' in summary.strip():
> +			problems.append('commit message must start
> with a *single* line of summary, followed by empty line')
> +		if ':' not in summary:
> +			problems.append('summary line must start
> with a logical unit name, e.g. "cat/pkg:"')
> +		# accept 69 overall or unit+50, in case of very long
> package names
> +		if 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(self.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:
> +					if len(l.strip()) > 72:
> +						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 characters') +
> +		if problems:
> +			return (False, '\n'.join('- %s' % x for x in
> problems))
> +		return (True, None)


I know there are not a lot of repoman unit tests, but, can you please
create some to test this?  That way it doesn't get any worse.

In the PR, you mentioned adding in an editor call to re-edit the
message.  This would be a good thing and I know is not that difficult
to accomplish.  Is this coming in another patch?

-- 
Brian Dolbec <dolsen>