Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitCommitBear.py: Detect revert commits #2241

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions bears/vcs/git/GitCommitBear.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ def check_prerequisites(cls):
else:
return True

@classmethod
def get_revert_checks_metadata(cls):
return FunctionMetadata.from_function(
cls.check_revert,
omit={'self', 'shortlog', 'body'})

@classmethod
def get_shortlog_checks_metadata(cls):
return FunctionMetadata.from_function(
Expand All @@ -91,6 +97,7 @@ def get_metadata(cls):
FunctionMetadata.from_function(
cls.run,
omit={'self', 'dependency_results'}),
cls.get_revert_checks_metadata(),
cls.get_shortlog_checks_metadata(),
cls.get_body_checks_metadata(),
cls.get_issue_checks_metadata())
Expand Down Expand Up @@ -141,6 +148,10 @@ def run(self,
yield Result(self, 'HEAD commit has no message.')
return

yield from self.check_revert(
shortlog,
body,
**self.get_revert_checks_metadata().filter_parameters(kwargs))
yield from self.check_shortlog(
shortlog,
**self.get_shortlog_checks_metadata().filter_parameters(kwargs))
Expand All @@ -151,6 +162,59 @@ def run(self,
body,
**self.get_issue_checks_metadata().filter_parameters(kwargs))

def check_revert(self, shortlog, body,
allow_revert_commits_without_reason: bool = False):
"""
Validates revert commits.

:param shortlog:
The shortlog message string.
:param body:
The body of the commit message of HEAD.
:param allow_revert_commits_without_reason:
Whether reason is needed for revert commits.
"""
if not shortlog.startswith('Revert '):
return

shortlog_start_index = shortlog.find('"')
Copy link
Member

@ksdme ksdme May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't I use single quotes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git revert doesnt do that?

Copy link
Member

@ksdme ksdme May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about the requirement of the shortlog of the revert commit specifically to be written as Revert "Another Test commit2". I don't see any reason why using single quotes should cause a failure.

shortlog_end_index = shortlog.rfind('"')
reverted_shortlog = shortlog[shortlog_start_index +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shortlog_start_index, shortlog_end_index and reverted_shortlog are confusing names in this context as, in other places of the code shortlog refers to the entire string including the subject and the message.

1: shortlog_end_index]

body = body.strip('\n')
blankline = body.find('\n\n')
headline = body[:blankline] if blankline != -1 else body

if not headline.startswith('This reverts '):
yield Result(self, 'Invalid revert commit.')
return

sha_regex = re.compile(r'([0-9a-f]{40})')
Copy link
Member

@ksdme ksdme May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the intent is also to maintain readability why not support short SHA1 hashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git revert doesnt do that

commit_sha = re.findall(sha_regex, headline)[0]
command = ('git log -n 1 ' +
str(commit_sha) + ' --pretty=%B')
previous_commit_stdout, previous_commit_stderr = run_shell_command(
command)

if previous_commit_stderr:
yield Result(self, 'Invalid sha for reverted commit.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sha -> SHA

return

previous_commit_stdout = previous_commit_stdout.rstrip('\n')
position = previous_commit_stdout.find('\n')
old_shortlog = previous_commit_stdout[:position] if position != - \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this linebreak looks pretty confusing try breaking before the if

1 else previous_commit_stdout

if old_shortlog != reverted_shortlog:
Copy link
Member

@ksdme ksdme May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a problem if shortlog for the previous commit barely fit within the length limit as, to be a valid revert commit, it'll have to be rewritten with extra characters in the new commit and thereby cause an impossible situation.

I think the solution would be to rethink about the shortlog structure you require for a valid revert commit. I propose it should be written as

08b54a: Revert Commit

This reason to revert would be...

Reference if any

and this structure is also more aligned with the general guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format followed is one that is generated by git itself for revert commits. This PR only checks if a reason for revert is mentioned or not. See link
I don't think changing shortlog structure would be most apt solution. One possible solution could be to increase shorlog length limit for revert commits (to maybe 60). @ksdme @jayvdb @damngamerz

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible solution could be to increase shorlog length limit for revert commits

Yes, whenever a revert commit is detected, and revert commits have been permitted in the repo, other rules which might reject the commit must be turned off.

yield Result(self, 'Shortlog of revert commit does '
'not match the original commit.')

reason = body[blankline+1:] if blankline != -1 else ''

if not allow_revert_commits_without_reason and len(reason) == 0:
yield Result(self, 'Revert commit does not have a reason.')

def check_shortlog(self, shortlog,
shortlog_length: int = 50,
shortlog_regex: str = '',
Expand Down
68 changes: 66 additions & 2 deletions tests/vcs/git/GitCommitBearTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,13 @@ def test_get_metadata(self):
metadata = GitCommitBear.get_metadata()
self.assertEqual(
metadata.name,
"<Merged signature of 'run', 'check_shortlog', 'check_body'"
", 'check_issue_reference'>")
"<Merged signature of 'run', 'check_revert', 'check_shortlog'"
", 'check_body', 'check_issue_reference'>")
Copy link
Member

@damngamerz damngamerz May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't change existing test unless there's a reason. I don't see any such change in your patch which might affect this test.
Maybe write a new separate test for current change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay it all cool 👍 you just added check_revert


# Test if at least one parameter of each signature is used.
self.assertIn('allow_empty_commit_message', metadata.optional_params)
self.assertIn('allow_revert_commits_without_reason',
metadata.optional_params)
self.assertIn('shortlog_length', metadata.optional_params)
self.assertIn('body_line_length', metadata.optional_params)
self.assertIn('body_close_issue', metadata.optional_params)
Expand All @@ -125,6 +127,68 @@ def test_empty_message(self):
[])
self.assert_no_msgs()

def test_revert_checks(self):
self.git_commit('Test commit')
commit_hash, _ = run_shell_command('git rev-parse HEAD')
self.git_commit('Revert "Test commit"\n\n'
'Abcdef blablabla commit ' + str(commit_hash) + '\n\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test content should be self explanatory about its existence.
Please use proper sentence in place of
Abcdef blablabla commit :P

'Reason for the revert commit is explained here.\n')
self.assertEqual(self.run_uut(),
['Invalid revert commit.'])
self.assert_no_msgs()

self.git_commit('Test commit1\n\n'
'This is body of test commit1\n')
commit_hash, _ = run_shell_command('git rev-parse HEAD')

self.git_commit('Revert "Another Test commit2"\n\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test method contains many testcases of revert features. I think it will be better if we can split this into several test method like test_revert_check_success, test_revert_check_commit_no_reason, test_revert_check_invalid_sha_commit, etc

'This reverts commit ' + str(commit_hash) + '\n\n'
'Reason for the revert commit is explained here.\n')
self.assertEqual(
self.run_uut(),
['Shortlog of revert commit does not match the original commit.'])
self.assert_no_msgs()

self.git_commit('Test commit3')
commit_hash, _ = run_shell_command('git rev-parse HEAD')
self.git_commit('Revert "Test commit3"\n\n'
'This reverts commit ' + str(commit_hash) + '\n\n')
self.assertEqual(self.run_uut(),
['Revert commit does not have a reason.'])
self.assert_no_msgs()

self.git_commit('Revert "Test commit3"\n\n'
'This reverts commit ' + str(commit_hash) + '\n\n')
self.assertEqual(
self.run_uut(allow_revert_commits_without_reason=True),
[])
self.assert_no_msgs()

self.git_commit('Revert "Test commit3"\n\n'
'This reverts commit ' + str(commit_hash) + '\n\n'
'Reason for the revert commit is explained here.\n')
self.assertEqual(self.run_uut(), [])
self.assert_no_msgs()

invalid_commit_hash = '0000000000000000000000000000000000000000'
self.git_commit('Revert "Test commit3"\n\n'
'This reverts commit ' + invalid_commit_hash + '\n\n'
'Reason for the revert commit is explained here.\n')
self.assertEqual(self.run_uut(),
['Invalid sha for reverted commit.'])
self.assert_no_msgs()

self.git_commit('Test commit4\n\n'
'This is body of test commit1\n')
commit_hash, _ = run_shell_command('git rev-parse HEAD')
self.git_commit('Revert "Another Test commit5"\n\n'
'This reverts commit ' + str(commit_hash) + '\n\n'
'Reason for the revert commit is explained here.\n')
self.assertEqual(
self.run_uut(),
['Shortlog of revert commit does not match the original commit.'])
self.assert_no_msgs()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kriti21
It would be great to have a test such as

        _,__ = run_shell_command('echo "a" >> testfile.txt ')
        self.run_git_command('add', 'testfile.txt')
        self.run_git_command('commit', '-m', '"a commit"')
        commit_hash, _ = run_shell_command('git rev-parse HEAD')
        self.run_git_command('revert', 'HEAD', '--no-edit')
        self.assertEqual(self.run_uut(), 
                         ['Revert commit does not have a reason.'])

@unittest.mock.patch('bears.vcs.git.GitCommitBear.run_shell_command',
return_value=('one-liner-message\n', ''))
def test_pure_oneliner_message(self, patch):
Expand Down