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

[gentoo-portage-dev] [PATCH v3 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/commit/test_commitmsg.py | 109 +++++++++++++++++++++
 repoman/pym/repoman/tests/simple/test_simple.py    |   8 +-
 5 files changed, 189 insertions(+), 5 deletions(-)
 create mode 100644 repoman/pym/repoman/tests/commit/__init__.py
 create mode 100644 repoman/pym/repoman/tests/commit/__test__.py
 create mode 100644 repoman/pym/repoman/tests/commit/test_commitmsg.py

diff --git a/repoman/pym/repoman/actions.py b/repoman/pym/repoman/actions.py
index b76a6e466..57b528312 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
+
+	@staticmethod
+	def verify_commit_message(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
+
+		footer_re = re.compile(r'^[\w-]+:')
+
+		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(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/commit/test_commitmsg.py b/repoman/pym/repoman/tests/commit/test_commitmsg.py
new file mode 100644
index 000000000..a734c1065
--- /dev/null
+++ b/repoman/pym/repoman/tests/commit/test_commitmsg.py
@@ -0,0 +1,109 @@
+# Copyright 2011-2018 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+from repoman.actions import Actions
+from repoman.tests import TestCase
+
+
+class CommitMessageVerificationTest(TestCase):
+	def assertGood(self, commitmsg):
+		res, expl = Actions.verify_commit_message(commitmsg)
+		self.assertTrue(res,
+				'''Commit message verification failed for:
+%s
+
+Error:
+%s''' % (commitmsg, expl))
+
+	def assertBad(self, commitmsg, reason_re):
+		res, expl = Actions.verify_commit_message(commitmsg)
+		self.assertFalse(res,
+				'''Commit message verification succeeded unexpectedly, for:
+%s
+
+Expected: /%s/''' % (commitmsg, reason_re))
+		self.assertNotIn('\n', expl.strip(),
+				'''Commit message verification returned multiple errors (one expected):
+%s
+
+Expected: /%s/
+Errors:
+%s''' % (commitmsg, reason_re, expl))
+		self.assertRegexpMatches(expl, reason_re,
+				'''Commit message verification did not return expected error, for:
+%s
+
+Expected: /%s/
+Errors:
+%s''' % (commitmsg, reason_re, expl))
+
+	def test_summary_only(self):
+		self.assertGood('dev-foo/bar: Actually good commit message')
+
+	def test_summary_and_body(self):
+		self.assertGood('''dev-foo/bar: Good commit message
+
+Extended description goes here and is properly wrapped at 72 characters
+which is very nice and blah blah.
+
+Another paragraph for the sake of having one.''')
+
+	def test_summary_and_footer(self):
+		self.assertGood('''dev-foo/bar: Good commit message
+
+Closes: https://bugs.gentoo.org/NNNNNN''')
+
+	def test_summary_body_and_footer(self):
+		self.assertGood('''dev-foo/bar: Good commit message
+
+Extended description goes here and is properly wrapped at 72 characters
+which is very nice and blah blah.
+
+Another paragraph for the sake of having one.
+
+Closes: https://bugs.gentoo.org/NNNNNN''')
+
+	def test_summary_without_unit_name(self):
+		self.assertBad('Version bump', r'summary.*logical unit name')
+
+	def test_multiline_summary(self):
+		self.assertBad('''dev-foo/bar: Commit message with very long summary
+that got wrapped because of length''', r'single.*line.*summary')
+
+	def test_overlong_summary(self):
+		self.assertBad('dev-foo/bar: Commit message with very long summary \
+in a single line that should trigger an explicit error',
+				r'summary.*too long')
+
+	def test_summary_with_very_long_package_name(self):
+		self.assertGood('dev-foo/foo-bar-bar-baz-bar-bar-foo-bar-bar-\
+baz-foo-baz-baz-foo: We do not fail because pkgname was long')
+
+	def test_multiple_footers(self):
+		self.assertBad('''dev-foo/bar: Good summary
+
+Bug: https://bugs.gentoo.org/NNNNNN
+
+Closes: https://github.com/gentoo/gentoo/pull/NNNN''', r'multiple footer')
+
+	def test_gentoo_bug(self):
+		self.assertBad('''dev-foo/bar: Good summary
+
+Gentoo-Bug: NNNNNN''', r'Gentoo-Bug')
+
+	def test_bug_with_number(self):
+		self.assertBad('''dev-foo/bar: Good summary
+
+Bug: NNNNNN''', r'Bug.*full URL')
+
+	def test_closes_with_number(self):
+		self.assertBad('''dev-foo/bar: Good summary
+
+Closes: NNNNNN''', r'Closes.*full URL')
+
+	def test_body_too_long(self):
+		self.assertBad('''dev-foo/bar: Good summary
+
+But the body is not wrapped properly and has very long lines that are \
+very hard to read and blah blah blah
+blah blah.''', r'body.*wrapped')
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.2

[gentoo-portage-dev] [PATCH v3 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 57b528312..cd954223a 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.2

[gentoo-portage-dev] [PATCH v3 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 cd954223a..8e23322c8 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.2

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

[Brian Dolbec]

On Mon, 19 Feb 2018 16:00:55 +0100
Michał Górny <mgorny@gentoo.org> 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.
> ---


This version series looks good, thank you :)

-- 
Brian Dolbec <dolsen>