BryanDavis has uploaded a new change for review. https://gerrit.wikimedia.org/r/304432
Change subject: Validate Change-Id and Depends-On values ...................................................................... Validate Change-Id and Depends-On values A valid Gerrit change-id is a SHA-1 hash represented as a 40 digit hex string prefixed with I to distinguish it from a commit hash. Bug: T142801 Change-Id: I98733c7e87f02ca835b0968415ce1e3253996cf6 --- M commit_message_validator/__init__.py M commit_message_validator/tests/data/check_message_errors.msg M commit_message_validator/tests/data/check_message_errors.out A commit_message_validator/tests/data/invalid_change_id.msg A commit_message_validator/tests/data/invalid_change_id.out A commit_message_validator/tests/data/invalid_depends_on.msg A commit_message_validator/tests/data/invalid_depends_on.out A commit_message_validator/tests/data/multiple_change_id.msg A commit_message_validator/tests/data/multiple_change_id.out 9 files changed, 46 insertions(+), 16 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/integration/commit-message-validator refs/changes/32/304432/1 diff --git a/commit_message_validator/__init__.py b/commit_message_validator/__init__.py index ee64e30..59964d4 100644 --- a/commit_message_validator/__init__.py +++ b/commit_message_validator/__init__.py @@ -98,17 +98,10 @@ if m.group(2) != ' ': yield "Expected one space after 'Depends-On:'" - change_id = m.group(3).strip() - change_id_is_hex = True - try: - int(change_id[1:], 16) - except ValueError: - change_id_is_hex = False - if change_id.upper().startswith('I') and change_id_is_hex: - if change_id[0] != 'I': - yield "The Depends-On value must use uppercase I" - else: - yield "The Depends-On value is not a Gerrit Change-Id" + +def is_valid_change_id(s): + """A Gerrit change id is a 40 character hex string prefixed with 'I'.""" + return re.match('^I[a-f0-9]{40}$', s) def check_message(lines): @@ -120,8 +113,10 @@ - For any "^Bug: " line, prior line is another Bug: line or empty - For any "^Depends-On: " line, next line is not blank - For any "^Depends-On: " line, prior line is Bug|Depends-On or empty + - For any "^Depends-On: " line, the RHS is a valid change id - Exactly one "Change-Id: " line per commit - For "Change-Id: " line, prior line is empty or "^(Bug|Depends-On): " + - For "Change-Id: " line, the RHS is a valid change id - No blank lines between any "(Bug|Depends-On): " lines and "Change-Id: " - Only "(cherry picked from commit" can follow "Change-Id: " - Message has at least 3 lines (subject, blank, Change-Id) @@ -166,7 +161,13 @@ errors.append( "Line %d: Expected blank line before Depends-On:" % rline) - if line.startswith('Change-Id: I'): + (label, rhs) = line.split(' ', 1) + if not is_valid_change_id(rhs): + errors.append( + "Line %d: value must be a single valid Gerrit change id" + % rline) + + if line.startswith('Change-Id:'): # Only expect one "Change-Id: " line if changeid_line is not False: errors.append( @@ -196,6 +197,12 @@ changeid_line = rline + (label, rhs) = line.split(' ', 1) + if not is_valid_change_id(rhs): + errors.append( + "Line %d: value must be a single valid Gerrit change id" + % rline) + last_lineno = rline last_line = line diff --git a/commit_message_validator/tests/data/check_message_errors.msg b/commit_message_validator/tests/data/check_message_errors.msg index 3d2343f..d28e005 100644 --- a/commit_message_validator/tests/data/check_message_errors.msg +++ b/commit_message_validator/tests/data/check_message_errors.msg @@ -11,12 +11,12 @@ Bug: T12 Bug: t13 -Depends-On: I1234 +Depends-On: I395fd614bfa8bcfc46a35f955fb77ec6f03f7a01 Bug: t14 -Depends-On: I5678 -Change-Id: I1234567890abcdef123456 -Change-Id: I1234567890abcdef123457 +Depends-On: I00d0f7c3b294c3ddc656f9a5447df89c63142203 +Change-Id: I20c70ff250085de49e4b961da4b53258858df1dd +Change-Id: I62a5bbf5a94da6f121a7836859371a2a2f60873d Depends-On: I9abc Foo bar diff --git a/commit_message_validator/tests/data/check_message_errors.out b/commit_message_validator/tests/data/check_message_errors.out index 2d22d18..fff8ef8 100644 --- a/commit_message_validator/tests/data/check_message_errors.out +++ b/commit_message_validator/tests/data/check_message_errors.out @@ -22,6 +22,7 @@ Line 16: The phabricator task ID must use uppercase T Line 18: Extra Change-Id found, next at 19 Line 20: Expected blank line before Depends-On: +Line 20: value must be a single valid Gerrit change id Line 21: Unexpected blank line after Depends-On: Line 20: Unexpected line after Change-Id Line 21: Unexpected line after Change-Id diff --git a/commit_message_validator/tests/data/invalid_change_id.msg b/commit_message_validator/tests/data/invalid_change_id.msg new file mode 100644 index 0000000..6f6a5d7 --- /dev/null +++ b/commit_message_validator/tests/data/invalid_change_id.msg @@ -0,0 +1,3 @@ +This change id is not valid + +Change-Id: Ideafbeef diff --git a/commit_message_validator/tests/data/invalid_change_id.out b/commit_message_validator/tests/data/invalid_change_id.out new file mode 100644 index 0000000..63b86ac --- /dev/null +++ b/commit_message_validator/tests/data/invalid_change_id.out @@ -0,0 +1,4 @@ +commit-message-validator v%version% +The following errors were found: +Line 3: value must be a single valid Gerrit change id +Please review <https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines> and update your commit message accordingly diff --git a/commit_message_validator/tests/data/invalid_depends_on.msg b/commit_message_validator/tests/data/invalid_depends_on.msg new file mode 100644 index 0000000..4f8d180 --- /dev/null +++ b/commit_message_validator/tests/data/invalid_depends_on.msg @@ -0,0 +1,4 @@ +This Depends-On is not valid + +Depends-On: pizza +Change-Id: I9cd485c1923dd40f05bd61ef776cb6a9a57187f0 diff --git a/commit_message_validator/tests/data/invalid_depends_on.out b/commit_message_validator/tests/data/invalid_depends_on.out new file mode 100644 index 0000000..63b86ac --- /dev/null +++ b/commit_message_validator/tests/data/invalid_depends_on.out @@ -0,0 +1,4 @@ +commit-message-validator v%version% +The following errors were found: +Line 3: value must be a single valid Gerrit change id +Please review <https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines> and update your commit message accordingly diff --git a/commit_message_validator/tests/data/multiple_change_id.msg b/commit_message_validator/tests/data/multiple_change_id.msg new file mode 100644 index 0000000..7e5c8fb --- /dev/null +++ b/commit_message_validator/tests/data/multiple_change_id.msg @@ -0,0 +1,3 @@ +Only one change id per line + +Change-Id: I62a5bbf5a94da6f121a7836859371a2a2f60873d, I9cd485c1923dd40f05bd61ef776cb6a9a57187f0 diff --git a/commit_message_validator/tests/data/multiple_change_id.out b/commit_message_validator/tests/data/multiple_change_id.out new file mode 100644 index 0000000..63b86ac --- /dev/null +++ b/commit_message_validator/tests/data/multiple_change_id.out @@ -0,0 +1,4 @@ +commit-message-validator v%version% +The following errors were found: +Line 3: value must be a single valid Gerrit change id +Please review <https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines> and update your commit message accordingly -- To view, visit https://gerrit.wikimedia.org/r/304432 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I98733c7e87f02ca835b0968415ce1e3253996cf6 Gerrit-PatchSet: 1 Gerrit-Project: integration/commit-message-validator Gerrit-Branch: master Gerrit-Owner: BryanDavis <bda...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits