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

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

[Michał Górny]

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 = {
-- 
2.16.1

[gentoo-portage-dev] [PATCH v2 2/3] repoman: Remove support for getting messages from stdin

[Michał Górny]

Remove the support for getting commit messages from stdin, in favor
of always using external editor. This was never very useful, especially
given that was implemented poorly and with commit message verification
it will become even more painful to keep.
---
 repoman/pym/repoman/actions.py   |  4 +++-
 repoman/pym/repoman/utilities.py | 22 +---------------------
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/repoman/pym/repoman/actions.py b/repoman/pym/repoman/actions.py
index 91603865c..6f561b64a 100644
--- a/repoman/pym/repoman/actions.py
+++ b/repoman/pym/repoman/actions.py
@@ -585,7 +585,9 @@ class Actions(object):
 				commitmessage = utilities.get_commit_message_with_editor(
 					editor, message=qa_output, prefix=msg_prefix)
 			else:
-				commitmessage = utilities.get_commit_message_with_stdin()
+				print("EDITOR is unset or invalid. Please set EDITOR to your preferred editor.")
+				print(bad("* no EDITOR found for commit message, aborting commit."))
+				sys.exit(1)
 		except KeyboardInterrupt:
 			logging.fatal("Interrupted; exiting...")
 			sys.exit(1)
diff --git a/repoman/pym/repoman/utilities.py b/repoman/pym/repoman/utilities.py
index c204faa8d..1272f3fb6 100644
--- a/repoman/pym/repoman/utilities.py
+++ b/repoman/pym/repoman/utilities.py
@@ -1,6 +1,6 @@
 # -*- coding:utf-8 -*-
 # repoman: Utilities
-# Copyright 2007-2013 Gentoo Foundation
+# Copyright 2007-2018 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
 """This module contains utility functions to help repoman find ebuilds to
@@ -13,7 +13,6 @@ __all__ = [
 	"FindPackagesToScan",
 	"FindPortdir",
 	"get_commit_message_with_editor",
-	"get_commit_message_with_stdin",
 	"get_committer_name",
 	"have_ebuild_dir",
 	"have_profile_dir",
@@ -226,25 +225,6 @@ def get_commit_message_with_editor(editor, message=None, prefix=""):
 			pass
 
 
-def get_commit_message_with_stdin():
-	"""
-	Read a commit message from the user and return it.
-
-	@rtype: string or None
-	@return: A string on success or None if an error occurs.
-	"""
-	print(
-		"Please enter a commit message."
-		" Use Ctrl-d to finish or Ctrl-c to abort.")
-	commitmessage = []
-	while True:
-		commitmessage.append(sys.stdin.readline())
-		if not commitmessage[-1]:
-			break
-	commitmessage = "".join(commitmessage)
-	return commitmessage
-
-
 def FindPortdir(settings):
 	""" Try to figure out what repo we are in and whether we are in a regular
 	tree or an overlay.
-- 
2.16.1

[gentoo-portage-dev] [PATCH v2 3/3] repoman: Verify commit messages when using EDITOR

[Michał Górny]

---
 repoman/pym/repoman/actions.py | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/repoman/pym/repoman/actions.py b/repoman/pym/repoman/actions.py
index 6f561b64a..b2285e787 100644
--- a/repoman/pym/repoman/actions.py
+++ b/repoman/pym/repoman/actions.py
@@ -129,7 +129,23 @@ class Actions(object):
 				else:
 					sys.exit(1)
 		else:
-			commitmessage = self.get_new_commit_message(qa_output)
+			commitmessage = None
+			msg_qa_output = qa_output
+			initial_message = None
+			while True:
+				commitmessage = self.get_new_commit_message(
+						msg_qa_output, commitmessage)
+				res, expl = self.verify_commit_message(commitmessage)
+				if res:
+					break
+				else:
+					full_expl = '''Issues with the commit message were found. Please fix it or remove
+the whole commit message to abort.
+
+''' + expl
+					msg_qa_output = (
+							[' %s\n' % x for x in full_expl.splitlines()]
+							+ qa_output)
 
 		commitmessage = commitmessage.rstrip()
 
@@ -577,8 +593,8 @@ class Actions(object):
 			prefix = "/".join(self.scanner.reposplit[1:]) + ": "
 		return prefix
 
-	def get_new_commit_message(self, qa_output):
-		msg_prefix = self.msg_prefix()
+	def get_new_commit_message(self, qa_output, old_message=None):
+		msg_prefix = old_message or self.msg_prefix()
 		try:
 			editor = os.environ.get("EDITOR")
 			if editor and utilities.editor_is_executable(editor):
-- 
2.16.1

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

[M. J. Everitt]

[-- 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 --]

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

[Brian Dolbec]

[-- 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 --]

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

[M. J. Everitt]

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

On 18/02/18 16:28, Brian Dolbec wrote:
> On Sat, 17 Feb 2018 13:18:03 +0000
> "M. J. Everitt" <m.j.everitt@iee.org> wrote:
>
>> 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.
>
That's probably a better implementation, but for the time being I was
thinking more of an 'actions_commit' module to break out the extra code
from the generic 'actions' module.
Regards,
Michael/veremitz.


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

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

[Brian Dolbec]

On Sat, 17 Feb 2018 13:56:46 +0100
Michał Górny <mgorny@gentoo.org> wrote:

> +
> +	footer_re = re.compile(r'^[\w-]+:')
> +
> +	@classmethod
> +	def verify_commit_message(cls, msg):
...
> +          if all(cls.footer_re.match(l) for l in lines if l.strip()):


Why declare the footer_re in the class space?  It is only used in this
new function.  Then the function could be @staticmethod instead.  I
don't see the advantage of it this way.

If it is for re-use in other possible check functions, then perhaps it
would be best to split this out to it's own class/file that can be
added to easily.  And just run it from the Actions class as M.J.
Everitt suggested.
-- 
Brian Dolbec <dolsen>

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

[Michał Górny]

W dniu nie, 18.02.2018 o godzinie 09∶11 -0800, użytkownik Brian Dolbec
napisał:
> On Sat, 17 Feb 2018 13:56:46 +0100
> Michał Górny <mgorny@gentoo.org> wrote:
> 
> > +
> > +	footer_re = re.compile(r'^[\w-]+:')
> > +
> > +	@classmethod
> > +	def verify_commit_message(cls, msg):
> 
> ...
> > +          if all(cls.footer_re.match(l) for l in lines if l.strip()):
> 
> 
> Why declare the footer_re in the class space?  It is only used in this
> new function.  Then the function could be @staticmethod instead.  I
> don't see the advantage of it this way.
> 
> If it is for re-use in other possible check functions, then perhaps it
> would be best to split this out to it's own class/file that can be
> added to easily.  And just run it from the Actions class as M.J.
> Everitt suggested.

I've declared it outside the function to not have to re-compile it every
time the function is run (well, yes, now that I think of it, it won't
happen more than once most of the time...). Used class space to avoid
moving it too far from code.

-- 
Best regards,
Michał Górny

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

[Michał Górny]

W dniu sob, 17.02.2018 o godzinie 13∶56 +0100, użytkownik Michał Górny
napisał:
> 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 +

Oh, great. Now I see that I haven't added the new tests, and the file is
gone now, and now I'm going to waste another hour writing them all
again...

>  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 = {

-- 
Best regards,
Michał Górny