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

[Michał Górny]

W dniu nie, 04.02.2018 o godzinie 09∶51 -0800, użytkownik Brian Dolbec
napisał:
> 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?
>

I'm still wondering how to implement it best. In particular, how to
handle the non-EDITOR (stdin) branch?

-- 
Best regards,
Michał Górny