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

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

[Michał Górny]

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)
-- 
2.16.1

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

[Ulrich Mueller]

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

>>>>> On Fri, 2 Feb 2018, Michał Górny 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.

Have you tested this against existing commits in the gentoo repo?

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

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

[Michał Górny]

W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller
napisał:
> > > > > > On Fri, 2 Feb 2018, Michał Górny 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.
> 
> Have you tested this against existing commits in the gentoo repo?
> 

Nah, just hand-tested all of the conditionals.

-- 
Best regards,
Michał Górny

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

[Michał Górny]

W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller
napisał:
> > > > > > On Fri, 2 Feb 2018, Michał Górny 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.
> 
> Have you tested this against existing commits in the gentoo repo?
> 

After checking the 10000 most recent commits, I have the following stats
on invalid commit messages:

138 klausman@gentoo.org
99 slyfox@gentoo.org
61 leio@gentoo.org
20 dilfridge@gentoo.org
18 monsieurp@gentoo.org
15 pacho@gentoo.org
15 whissi@gentoo.org
13 alicef@gentoo.org
12 floppym@gentoo.org
12 xmw@gentoo.org
12 jer@gentoo.org
11 axs@gentoo.org
11 amynka@gentoo.org
10 polynomial-c@gentoo.org
...

Most of the violations are uses of Gentoo-Bug and very long single-line
commit messages; most likely people using '-m "very long message because
I am lazy and can't use editor properly"'.

Full list of violations in the sample:
https://gist.github.com/mgorny/fa81ffe07ca9565198e4e44fb040bb19

-- 
Best regards,
Michał Górny

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

[Ulrich Mueller]

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

>>>>> On Sun, 04 Feb 2018, Michał Górny wrote:

> W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller
> napisał:
>> > 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.
>> 
>> Have you tested this against existing commits in the gentoo repo?

> After checking the 10000 most recent commits, I have the following
> stats on invalid commit messages:

> [...]

> Most of the violations are uses of Gentoo-Bug and very long
> single-line commit messages; most likely people using '-m "very long
> message because I am lazy and can't use editor properly"'.

> Full list of violations in the sample:
> https://gist.github.com/mgorny/fa81ffe07ca9565198e4e44fb040bb19

IMHO this shows that some of the tests are too rigid. Especially,
"body lines should be wrapped at 72 characters" often triggers in
cases where rewrapping wouldn't improve readability.

Since GLEP 66 says that "the body *should* be wrapped at 72
characters" (my emphasis), I would suggest to relax this test a bit
and only flag body lines longer than 80 characters. Also, any lines
containing long URIs should be excluded from the test.

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

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

[Michał Górny]

W dniu nie, 04.02.2018 o godzinie 22∶15 +0100, użytkownik Ulrich Mueller
napisał:
> > > > > > On Sun, 04 Feb 2018, Michał Górny wrote:
> > W dniu sob, 03.02.2018 o godzinie 09∶58 +0100, użytkownik Ulrich Mueller
> > napisał:
> > > > 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.
> > > 
> > > Have you tested this against existing commits in the gentoo repo?
> > After checking the 10000 most recent commits, I have the following
> > stats on invalid commit messages:
> > [...]
> > Most of the violations are uses of Gentoo-Bug and very long
> > single-line commit messages; most likely people using '-m "very long
> > message because I am lazy and can't use editor properly"'.
> > Full list of violations in the sample:
> > https://gist.github.com/mgorny/fa81ffe07ca9565198e4e44fb040bb19
> 
> IMHO this shows that some of the tests are too rigid. Especially,
> "body lines should be wrapped at 72 characters" often triggers in
> cases where rewrapping wouldn't improve readability.
> 
> Since GLEP 66 says that "the body *should* be wrapped at 72
> characters" (my emphasis), I would suggest to relax this test a bit
> and only flag body lines longer than 80 characters.

WFM.

>  Also, any lines
> containing long URIs should be excluded from the test.
> 

For that specific purpose, I've excluded footer from test. Maybe I'll
also be able to exclude long lines that look like URL.

-- 
Best regards,
Michał Górny

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@gentoo.org> 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>

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@gentoo.org> 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

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

[Brian Dolbec]

On Sun, 04 Feb 2018 18:58:21 +0100
Michał Górny <mgorny@gentoo.org> wrote:

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

hmm, yeah, that was not an option for the work tool I wrote and added
the re-edit to.  It had 2 files it needed to edit or re-edit.

In the case of passing in a pre-saved message, that should be left for
the user to fix.

Perhaps in the case of a stdin -m "foo" message we could just let the
user rely on bash their history to bring up that previous command, to
edit before hitting enter again.

-- 
Brian Dolbec <dolsen>