hooks: enhance Change-Id check to catch case-insensitive duplicates Update check_commit_msg_changeid_field to check for multiple Change-Id footers in a case-insensitive manner (including variations like Change-ID or change-id), and explicitly reject non-matching cases or malformed value formats. Previously, casing and format checks were coupled, causing malformed ID values (such as a missing 'I' prefix) to be falsely reported as field casing errors. This change decouples field name casing validation from value format validation into independent checks with actionable error messages, and ensures that duplicate Change-Id footers trigger an explicit rejection even when mixed casing or format errors are present. Also replaces the single batch unit test with 7 focused test methods covering valid messages, casing errors, format errors, duplicate lines, and mixed combinations. Bug: 533035135 Test: python3 -m unittest rh/hooks_unittest.py Test: tried to upload change with duplicate change-id Change-Id: I9ab168a3d99f7b6cd3f9200c2c2c389e9163525d Reviewed-on: https://gerrit-review.googlesource.com/c/git-repohooks/+/605723 Reviewed-by: Mike Frysinger <vapier@google.com> Commit-Queue: Achim Thesmann <achim@google.com> Tested-by: Achim Thesmann <achim@google.com>
diff --git a/rh/hooks.py b/rh/hooks.py index f28fe6b..7bdb8b1 100644 --- a/rh/hooks.py +++ b/rh/hooks.py
@@ -618,35 +618,81 @@ def check_commit_msg_changeid_field(project, commit, desc, _diff, options=None): """Check the commit message for a 'Change-Id:' line.""" field = "Change-Id" - regex = rf"^{field}: I[a-f0-9]+$" - check_re = re.compile(regex) + exact_prefix = f"{field}:" + lower_prefix = exact_prefix.lower() + value_pattern = r"I[a-f0-9]+$" + valid_line_re = re.compile(rf"^{exact_prefix} {value_pattern}") if options.args(): raise ValueError(f"commit msg {field} check takes no options") - found = [] + total_count = 0 + has_casing_error = False + has_format_error = False for line in desc.splitlines(): - if check_re.match(line): - found.append(line) + line_lower = line.lower() + if line_lower.startswith(lower_prefix): + total_count += 1 + if not line.startswith(exact_prefix): + has_casing_error = True + elif not valid_line_re.match(line): + has_format_error = True - if not found: - error = ( - f'Commit message is missing a "{field}:" line. It must match the\n' - f"following case-sensitive regex:\n\n {regex}" + ret = [] + if has_casing_error: + ret.append( + rh.results.HookResult( + f'commit msg: "{field}:" check', + project, + commit, + error=( + f'Commit message has invalid casing for "{field}:". ' + f'It must match exact case "{field}:".' + ), + ) ) - elif len(found) > 1: - error = ( - f'Commit message has too many "{field}:" lines. There can be ' - "only one." - ) - else: - return None - return [ - rh.results.HookResult( - f'commit msg: "{field}:" check', project, commit, error=error + if has_format_error: + ret.append( + rh.results.HookResult( + f'commit msg: "{field}:" check', + project, + commit, + error=( + f'Commit message has an invalid "{field}:" value format. ' + f"It must start with 'I' followed by hex digits " + f"(regex: {value_pattern})." + ), + ) ) - ] + + if total_count == 0: + ret.append( + rh.results.HookResult( + f'commit msg: "{field}:" check', + project, + commit, + error=( + f'Commit message is missing a "{field}:" line. ' + f"It must match the following case-sensitive regex:\n\n " + f"^{exact_prefix} {value_pattern}" + ), + ) + ) + elif total_count > 1: + ret.append( + rh.results.HookResult( + f'commit msg: "{field}:" check', + project, + commit, + error=( + f'Commit message has too many "{field}:" lines. ' + "There can be only one." + ), + ) + ) + + return ret or None PREBUILT_APK_MSG = """Commit message is missing required prebuilt APK
diff --git a/rh/hooks_unittest.py b/rh/hooks_unittest.py index 2b6d207..1403a31 100755 --- a/rh/hooks_unittest.py +++ b/rh/hooks_unittest.py
@@ -395,6 +395,27 @@ bool(ret), msg="Should have rejected: {{{" + desc + "}}}" ) + def _test_commit_message_errors( + self, func, desc, expected_errors, files=None + ): + """Helper for testing hooks that reject messages with error strings.""" + if files: + diff = [rh.git.RawDiffEntry(file=x) for x in files] + else: + diff = [] + ret = func(self.project, "commit", desc, diff, options=self.options) + self.assertIsNotNone(ret, msg=f"Should have rejected: {{{desc}}}") + assert len(ret) == len(expected_errors) + errors = [r.error for r in ret] + for expected in expected_errors: + self.assertTrue( + any(expected in e for e in errors), + msg=( + f'Expected error substring "{expected}" ' + f"not found in {errors}" + ), + ) + def _test_file_filter(self, mock_check, func, files): """Helper for testing hooks that filter by files and run external tools. @@ -587,22 +608,69 @@ ) def test_commit_msg_changeid_field(self, _mock_check, _mock_run): - """Verify the commit_msg_changeid_field builtin hook.""" - # Check some good messages. + """Verify check_commit_msg_changeid_field accepts valid messages.""" self._test_commit_messages( rh.hooks.check_commit_msg_changeid_field, True, ("subj\n\nChange-Id: I1234\n",), ) - # Check some bad messages. - self._test_commit_messages( + def test_commit_msg_changeid_field_invalid_casing( + self, _mock_check, _mock_run + ): + """Verify check_commit_msg_changeid_field rejects bad casing.""" + self._test_commit_message_errors( rh.hooks.check_commit_msg_changeid_field, - False, + "subj\n\nChange-ID: I1234\n", + ("invalid casing",), + ) + + def test_commit_msg_changeid_field_invalid_format( + self, _mock_check, _mock_run + ): + """Verify check_commit_msg_changeid_field rejects malformed IDs.""" + self._test_commit_message_errors( + rh.hooks.check_commit_msg_changeid_field, + "subj\n\nChange-Id: 1234\n", + ('invalid "Change-Id:" value format',), + ) + + def test_commit_msg_changeid_field_duplicates(self, _mock_check, _mock_run): + """Verify check_commit_msg_changeid_field rejects duplicate footers.""" + self._test_commit_message_errors( + rh.hooks.check_commit_msg_changeid_field, + "subj\n\nChange-Id: I1234\nChange-Id: I5678\n", + ('too many "Change-Id:" lines',), + ) + + def test_commit_msg_changeid_field_missing(self, _mock_check, _mock_run): + """Verify check_commit_msg_changeid_field rejects missing footers.""" + self._test_commit_message_errors( + rh.hooks.check_commit_msg_changeid_field, + "subj", + ('missing a "Change-Id:" line',), + ) + + def test_commit_msg_changeid_field_valid_and_invalid_casing( + self, _mock_check, _mock_run + ): + """Verify check_commit_msg_changeid_field catches casing/duplicates.""" + self._test_commit_message_errors( + rh.hooks.check_commit_msg_changeid_field, + "subj\n\nChange-Id: I1234\nChange-ID: I5678\n", + ("invalid casing", 'too many "Change-Id:" lines'), + ) + + def test_commit_msg_changeid_field_valid_and_invalid_format( + self, _mock_check, _mock_run + ): + """Verify check_commit_msg_changeid_field catches format/duplicates.""" + self._test_commit_message_errors( + rh.hooks.check_commit_msg_changeid_field, + "subj\n\nChange-Id: I1234\nChange-Id: 5678\n", ( - "subj", - "subj\n\nChange-Id: 1234\n", - "subj\n\nChange-ID: I1234\n", + 'invalid "Change-Id:" value format', + 'too many "Change-Id:" lines', ), )