GitCommitBear.py: Detect revert commits #2241

kriti21 · 2018-01-17T09:42:45Z

This detects revert commits
and checks they have a reason as well.

Closes #337

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

newbazz

You need to add tests.

ishanSrt · 2018-01-17T10:44:21Z

bears/vcs/git/GitCommitBear.py

@@ -123,6 +123,9 @@ def run(self, allow_empty_commit_message: bool = False, **kwargs):
                yield Result(self, 'HEAD commit has no message.')
            return

+        if shortlog.startswith('Revert') and len(body) == 0:


So you are not detecting revert commits if they do have a reason?

I don't think revert commits with reason have any problem.

When revert commits don't have any reason then they are wrong. Hence, detected only then.

I am not sure what needs to be done here. On what was meant by the issue, whether you have to detect revert commits and an additional check if they don't have a reason, or for only when they don't have a reason. You should discuss this with someone cc @sils

I did mention @sils once on gitter asking something like this. Never got any response.

Then you need to change tests again accordingly, so its better to ask someone what to do first ;)

I don’t particularly like this solution since reason could be given in the shortlog as well, and failing coala on a repository due to that is not good.
Also, is there a better way to detect reverting of commits? I couldn’t find one myself, but it seems this might over-trigger quite easily.

I would also say: Simple by default, powerful if needed.
We should implement other settings controlling what is considered a revert commit. But that can be part of a new issue.

So I would remove the len(body) part

And btw: I want to see a setting to enable looking for revert-commits 👍

Makman2 · 2018-01-21T15:58:46Z

Commit message shortlog: Please no space before colon ;)

Makman2 · 2018-01-21T15:59:32Z

unack b4e0f48

Makman2 · 2018-02-13T17:38:06Z

Comments not all addressed :)

Makman2 · 2018-02-13T17:38:14Z

unack 0105d2b

Makman2 · 2018-02-18T14:41:50Z

bears/vcs/git/GitCommitBear.py

@@ -123,6 +127,9 @@ def run(self, allow_empty_commit_message: bool = False, **kwargs):
                yield Result(self, 'HEAD commit has no message.')
            return

+        if shortlog.startswith('Revert') and check_revert_commits:


For performance reasons, switch both conditions:

if check_revert_commits and shortlog.startswith('Revert'):

Makman2 · 2018-02-18T14:42:29Z

bears/vcs/git/GitCommitBear.py

@@ -96,7 +96,9 @@ def get_host_from_remotes():
            netloc = urlparse(url)[1]
        return netloc.split('.')[0]

-    def run(self, allow_empty_commit_message: bool = False, **kwargs):
+    def run(self, allow_empty_commit_message: bool = False,
+            check_revert_commits: bool = True,


maybe to fit the naming scheme: allow_revert_commits? And it's by default False? (tbh I'm not sure what should be default for this anyway...)

Makman2 · 2018-02-18T14:42:58Z

bears/vcs/git/GitCommitBear.py

@@ -105,6 +107,8 @@ def run(self, allow_empty_commit_message: bool = False, **kwargs):

        :param allow_empty_commit_message: Whether empty commit messages are
                                           allowed or not.
+        :param check_revert_commits: whether revert commits are to be
+                                            checked or not.


Bad docstring indentation.

First letter should be upper-case.

Makman2 · 2018-02-18T14:44:19Z

tests/vcs/git/GitCommitBearTest.py

@@ -125,6 +125,14 @@ def test_empty_message(self):
                         [])
        self.assert_no_msgs()

+    def test_revert_commit(self):
+        self.git_commit('Revert : identifies revert commit\n')


Actually, you can maybe try to invoke git revert itself. That way we can see if we are really compliant with git's revert commits. So commit some arbitrary stuff, and then revert and test.

Makman2 · 2018-02-18T14:44:33Z

unack 28aa030

virresh · 2018-05-05T14:02:48Z

tests/vcs/git/GitCommitBearTest.py

@@ -125,6 +127,45 @@ def test_empty_message(self):
                         [])
        self.assert_no_msgs()

+    def test_revert_checks(self):
+        self.run_git_command('commit', stdin='Test commit')


Please use self.run_git_command for committing only after doing a git add.
Otherwise this will fail due to empty diffs and as a result no commit would be made.
And a git rev-parse HEAD on a repository with no commit will result in failures.

Consider using self.git_commit instead as it allows empty commits and can be done even without doing a git add

Rather if you want to use run_git_command, use it to generate a revert commit
For e.g self.run_git_command('revert', 'head')

virresh · 2018-05-05T14:08:38Z

bears/vcs/git/GitCommitBear.py

+        headline_regex = re.compile(r'^This reverts commit [0-9a-f]{40}')
+        sha_regex = re.compile(r'\b[0-9a-f]{40}\b')
+
+        if (not allow_revert_commits_without_reason and


Question:
What will happen in case the user allows revert commits without reason and while generating a revert commit, say the shortlog doesn't match or the commit sha is altered for some reason ? (via a git commit --amend for example)

kriti21 · 2018-05-06T08:36:18Z

Please review @jayvdb @virresh

virresh · 2018-05-06T11:00:39Z

tests/vcs/git/GitCommitBearTest.py

+                        'This is body of test commit1\n')
+        commit_hash, _ = run_shell_command('git rev-parse HEAD')
+
+        self.run_git_command('revert', 'head')


These revert commits are redundant since you are already making another commit on top of these, so Commit Bear is never run on them
(Similarly on line 179)

virresh · 2018-05-06T11:02:19Z

tests/vcs/git/GitCommitBearTest.py

+        commit_hash, _ = run_shell_command('git rev-parse HEAD')
+        self.run_git_command('revert', 'head')
+
+        self.git_commit('Revert "Test commit3"\n\n'


This is the default format of a revert commit, you can omit creating this one manually and just use the commit generated in line 155

virresh · 2018-05-06T11:07:43Z

bears/vcs/git/GitCommitBear.py

+
+        if revert_shortlog_regex.match(shortlog):
+            reverted_shortlog = shortlog.lstrip('Revert ')
+            reverted_shortlog = reverted_shortlog.strip('"')


Consider having a look at capturing and non-capturing regexes

virresh · 2018-05-06T15:00:30Z

bears/vcs/git/GitCommitBear.py

+            commit_sha = re.search(HEADLINE_REGEX, headline).group(2)
+            command = ('git log -n 1 ' +
+                       str(commit_sha) + ' --pretty=%B')
+            rstdout, _ = run_shell_command(command)


Question : What would happen if the commit_sha is an invalid number (or a commit_hash that doesn't exist) ?
Refer.

what does the r in rstdout stand for?

use long self-explanatory variable names.

jayvdb · 2018-05-06T15:09:54Z

bears/vcs/git/GitCommitBear.py

@@ -112,8 +119,7 @@ def get_host_from_remotes():
        netloc = parsed_url.host
        return netloc.split('.')[0]

-    def run(self,
-            allow_empty_commit_message: bool = False,
+    def run(self, allow_empty_commit_message: bool = False,


why is this being modified?

jayvdb · 2018-05-06T15:11:34Z

bears/vcs/git/GitCommitBear.py

@@ -151,6 +161,47 @@ 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,):


a trailing comma is silly if the ) follows it.
You need to understand why you should use trailing commas.

jayvdb · 2018-05-06T15:12:39Z

bears/vcs/git/GitCommitBear.py

+        """
+        Validates revert commits.
+
+        :param shortlog:                 The shortlog message string.


these descriptions are laid out very messily.

jayvdb · 2018-05-06T15:13:56Z

bears/vcs/git/GitCommitBear.py

+        REVERT_REGEX = re.compile(r'(Revert ")([\S+\s*]*)("$)')
+        HEADLINE_REGEX = re.compile(r'(^This reverts commit )([0-9a-f]{40})')
+
+        if REVERT_REGEX.match(shortlog):


use

if not REVERT_REGEX.match(shortlog): return

jayvdb · 2018-05-06T15:14:10Z

bears/vcs/git/GitCommitBear.py

+        :param allow_revert_commits_without_reason: Whether reason is
+                                           needed for revert commits.
+        """
+        REVERT_REGEX = re.compile(r'(Revert ")([\S+\s*]*)("$)')


this regex creates useless capture groups, making it harder to read than it needs to be.

Also [\S+\s*]* is invalid semantically.

Furthermore, there is no need to use a regex for this.

jayvdb · 2018-05-06T15:18:19Z

bears/vcs/git/GitCommitBear.py

+                                           needed for revert commits.
+        """
+        REVERT_REGEX = re.compile(r'(Revert ")([\S+\s*]*)("$)')
+        HEADLINE_REGEX = re.compile(r'(^This reverts commit )([0-9a-f]{40})')


Again, this is using capture groups which are not used.

jayvdb · 2018-05-06T15:20:08Z

bears/vcs/git/GitCommitBear.py

+            commit_sha = re.search(HEADLINE_REGEX, headline).group(2)
+            command = ('git log -n 1 ' +
+                       str(commit_sha) + ' --pretty=%B')
+            rstdout, _ = run_shell_command(command)


what does the r in rstdout stand for?

use long self-explanatory variable names.

This detects revert commits and checks they have a reason as well. Closes coala#337

kriti21 · 2018-05-06T20:18:33Z

Please review @jayvdb @virresh @nalinbhardwaj @ishanSrt @newbazz

virresh · 2018-05-08T08:56:54Z

tests/vcs/git/GitCommitBearTest.py

+            self.run_uut(),
+            ['Shortlog of revert commit does not match the original commit.'])
+        self.assert_no_msgs()
+


@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.'])

damngamerz · 2018-05-08T10:26:10Z

tests/vcs/git/GitCommitBearTest.py

-            "<Merged signature of 'run', 'check_shortlog', 'check_body'"
-            ", 'check_issue_reference'>")
+            "<Merged signature of 'run', 'check_revert', 'check_shortlog'"
+            ", 'check_body', 'check_issue_reference'>")


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?

Okay it all cool 👍 you just added check_revert

damngamerz · 2018-05-08T10:36:15Z

tests/vcs/git/GitCommitBearTest.py

+        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'


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

ksdme · 2018-05-09T07:15:26Z

bears/vcs/git/GitCommitBear.py

+            command)
+
+        if previous_commit_stderr:
+            yield Result(self, 'Invalid sha for reverted commit.')


ksdme · 2018-05-09T07:19:50Z

bears/vcs/git/GitCommitBear.py

+
+        shortlog_start_index = shortlog.find('"')
+        shortlog_end_index = shortlog.rfind('"')
+        reverted_shortlog = shortlog[shortlog_start_index +


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.

ksdme · 2018-05-09T07:23:05Z

bears/vcs/git/GitCommitBear.py

+        old_shortlog = previous_commit_stdout[:position] if position != - \
+            1 else previous_commit_stdout
+
+        if old_shortlog != reverted_shortlog:


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.~~

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

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.

ksdme · 2018-05-09T07:30:43Z

bears/vcs/git/GitCommitBear.py

+            yield Result(self, 'Invalid revert commit.')
+            return
+
+        sha_regex = re.compile(r'([0-9a-f]{40})')


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

git revert doesnt do that

ksdme · 2018-05-09T07:31:07Z

bears/vcs/git/GitCommitBear.py

+        if not shortlog.startswith('Revert '):
+            return
+
+        shortlog_start_index = shortlog.find('"')


~~Why can't I use single quotes?~~

git revert doesnt do that?

~~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.~~

seblat · 2018-05-09T08:39:04Z

bears/vcs/git/GitCommitBear.py

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


this linebreak looks pretty confusing try breaking before the if

jayvdb · 2018-05-09T21:07:06Z

Lots of manual conflict resolution needed now that #2310 has landed.

refeed · 2018-05-11T23:17:26Z

tests/vcs/git/GitCommitBearTest.py

+                        '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'


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

jayvdb · 2018-08-10T07:47:33Z

Duplicate of #2652

gitmate-bot added process/pending review size/XS labels Jan 17, 2018

kriti21 force-pushed the vcsbear branch 2 times, most recently from d98e1dd to e1d9e2f Compare January 17, 2018 09:56

newbazz suggested changes Jan 17, 2018

View reviewed changes

ishanSrt reviewed Jan 17, 2018

View reviewed changes

kriti21 force-pushed the vcsbear branch 6 times, most recently from 480d390 to b4e0f48 Compare January 19, 2018 12:30

Makman2 added process/wip and removed process/pending review labels Jan 21, 2018

kriti21 force-pushed the vcsbear branch from b4e0f48 to 0105d2b Compare January 21, 2018 16:15

gitmate-bot added process/pending review and removed process/wip labels Jan 21, 2018

kriti21 force-pushed the vcsbear branch 4 times, most recently from a80f473 to 28aa030 Compare February 16, 2018 17:53

Makman2 reviewed Feb 18, 2018

View reviewed changes

gitmate-bot added process/pending review size/S and removed size/XS labels May 5, 2018

virresh reviewed May 5, 2018

View reviewed changes

kriti21 force-pushed the vcsbear branch from 5ef4092 to 0bff328 Compare May 6, 2018 08:02

virresh reviewed May 6, 2018

View reviewed changes

kriti21 force-pushed the vcsbear branch from 0bff328 to f98d234 Compare May 6, 2018 13:51

virresh reviewed May 6, 2018

View reviewed changes

jayvdb requested changes May 6, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 6, 2018

GitCommitBear.py: Detect revert commits

f0c5f5c

This detects revert commits and checks they have a reason as well. Closes coala#337

kriti21 force-pushed the vcsbear branch from f98d234 to f0c5f5c Compare May 6, 2018 17:29

gitmate-bot added process/pending review and removed process/wip labels May 6, 2018

virresh reviewed May 8, 2018

View reviewed changes

damngamerz suggested changes May 8, 2018

View reviewed changes

ksdme suggested changes May 9, 2018

View reviewed changes

seblat reviewed May 9, 2018

View reviewed changes

jayvdb added process/wip and removed status/blocked process/pending review labels May 9, 2018

refeed reviewed May 11, 2018

View reviewed changes

jayvdb mentioned this pull request Jul 17, 2018

Python minimum version 3.4.4 #2612

Merged

jayvdb marked this as a duplicate of #2652 Aug 10, 2018

jayvdb closed this Aug 10, 2018

GitCommitBear.py: Detect revert commits #2241

GitCommitBear.py: Detect revert commits #2241

Conversation

kriti21 commented Jan 17, 2018

Checklist

newbazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishanSrt Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makman2 commented Jan 21, 2018

Makman2 commented Jan 21, 2018

Makman2 commented Feb 13, 2018

Makman2 commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makman2 commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriti21 commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriti21 commented May 6, 2018 • edited Loading

Choose a reason for hiding this comment

damngamerz May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksdme May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksdme May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksdme May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksdme May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented May 9, 2018

Choose a reason for hiding this comment

jayvdb commented Aug 10, 2018

ishanSrt Jan 17, 2018 •

edited

Loading

kriti21 commented May 6, 2018 •

edited

Loading

damngamerz May 8, 2018 •

edited

Loading

ksdme May 9, 2018 •

edited

Loading

ksdme May 9, 2018 •

edited

Loading

ksdme May 9, 2018 •

edited

Loading

ksdme May 9, 2018 •

edited

Loading