Gitlint now correctly handles refspec

Gitlint did not correctly handle all refspec formats that were supported by `git rev-list`. It does now :) This fixes #23.
jorisroovers · Jul 14, 2017 · 3cc6bcc · 3cc6bcc
1 parent bc05276
commit 3cc6bcc
Show file tree

Hide file tree

Showing 7 changed files with 134 additions and 74 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,9 +1,15 @@
 # Changelog #
 
-## v0.8.3 (In Progress) ##
+## v0.9.0 (In Progress) ##
 
 - New Rule: ```author-valid-email``` enforces a valid author email address. Details can be found in the
   [Rules section of the documentation](http://jorisroovers.github.io/gitlint/rules/).
+- **Breaking change**: The ```--commits``` commandline flag now strictly follows the refspec format as interpreted
+  by the [```git rev-list <refspec>```](https://git-scm.com/docs/git-rev-list) command. This means
+  that linting a single commit using ```gitlint --commits <SHA>``` won't work anymore. Instead, for single commits,
+  users now need to specificy ```gitlint --commits <SHA>^...<SHA>```. On the upside, this change also means
+  that gitlint will now understand all refspec formatters, including ```gitlint --commits HEAD``` to lint all commits
+  in the repository.
 - Debug output improvements
 
 ## v0.8.2 (2017-04-25) ##

diff --git a/gitlint/cli.py b/gitlint/cli.py
@@ -100,7 +100,7 @@ def build_config(ctx, target, config_path, c, extra_path, ignore, verbose, silen
 @click.option('-c', multiple=True,
               help="Config flags in format <rule>.<option>=<value> (e.g.: -c T1.line-length=80). " +
                    "Flag can be used multiple times to set multiple config values.")  # pylint: disable=bad-continuation
-@click.option('--commits', default="HEAD", help="The range of commits to lint. [default: HEAD]")
+@click.option('--commits', default=None, help="The range of commits to lint. [default: HEAD]")
 @click.option('-e', '--extra-path', help="Path to a directory or python module with extra user-defined rules",
               type=click.Path(exists=True, resolve_path=True, readable=True))
 @click.option('--ignore', default="", help="Ignore rules (comma-separated by id or name).")

diff --git a/gitlint/git.py b/gitlint/git.py
@@ -148,7 +148,7 @@ def from_commit_msg(commit_msg_str):
         return context
 
     @staticmethod
-    def from_local_repository(repository_path, refspec="HEAD"):
+    def from_local_repository(repository_path, refspec=None):
         """ Retrieves the git context from a local git repository.
         :param repository_path: Path to the git repository to retrieve the context from
         :param refspec: The commit(s) to retrieve
@@ -162,11 +162,13 @@ def from_local_repository(repository_path, refspec="HEAD"):
                 '_cwd': repository_path
             }
 
-            sha_list = sh.git("rev-list",
-                              # If refspec contains a dot it is a range
-                              # eg HEAD^.., upstream/master...HEAD
-                              '--max-count={0}'.format(-1 if "." in refspec else 1),
-                              refspec, **sh_special_args).split()
+            if refspec is None:
+                # We tried many things here e.g.: defaulting to e.g. HEAD or HEAD^... (incl. dealing with
+                # repos that only have a single commit - HEAD^... doesn't work there), but then we still get into
+                # problems with e.g. merge commits. Easiest solution is just taking the SHA from `git log -1`.
+                sha_list = [sh.git.log("-1", "--pretty=%H", **sh_special_args).replace("\n", "")]
+            else:
+                sha_list = sh.git("rev-list", refspec, **sh_special_args).split()
 
             for sha in sha_list:
                 # Get info from the local git repository: https://git-scm.com/docs/pretty-formats

diff --git a/gitlint/tests/test_cli.py b/gitlint/tests/test_cli.py
@@ -73,15 +73,15 @@ def test_lint_multiple_commits(self, sys, sh):
                                   u"commït-title2\n\ncommït-body2",
                                   u"test åuthor3,test-email3@föo.com,2016-12-05 15:28:15 01:00,åbc\n"
                                   u"commït-title3\n\ncommït-body3", ]
-        sh.git.side_effect = ["6f29bf81a8322a04071bb794666e48c443a90360\n" +
+        sh.git.side_effect = ["6f29bf81a8322a04071bb794666e48c443a90360\n" +  # git rev-list <SHA>
                               "25053ccec5e28e1bb8f7551fdbb5ab213ada2401\n" +
                               "4da2656b0dadc76c7ee3fd0243a96cb64007f125\n",
-                              u"file1.txt\npåth/to/file2.txt\n",
+                              u"file1.txt\npåth/to/file2.txt\n",  # git diff-tree <SHA>
                               u"file4.txt\npåth/to/file5.txt\n",
                               u"file6.txt\npåth/to/file7.txt\n"]
 
         with patch('gitlint.display.stderr', new=StringIO()) as stderr:
-            result = self.cli.invoke(cli.cli)
+            result = self.cli.invoke(cli.cli, ["--commits", "foo...bar"])
             expected = (u"Commit 6f29bf81a8:\n"
                         u'3: B5 Body message is too short (12<20): "commït-body1"\n\n'
                         u"Commit 25053ccec5:\n"
@@ -104,15 +104,15 @@ def test_lint_multiple_commits_config(self, sys, sh):
                                   u"commït-title2.\n\ncommït-body2\ngitlint-ignore: T3\n",
                                   u"test åuthor3,test-email3@föo.com,2016-12-05 15:28:15 01:00,åbc\n"
                                   u"commït-title3\n\ncommït-body3", ]
-        sh.git.side_effect = ["6f29bf81a8322a04071bb794666e48c443a90360\n" +
+        sh.git.side_effect = ["6f29bf81a8322a04071bb794666e48c443a90360\n" +  # git rev-list <SHA>
                               "25053ccec5e28e1bb8f7551fdbb5ab213ada2401\n" +
                               "4da2656b0dadc76c7ee3fd0243a96cb64007f125\n",
-                              u"file1.txt\npåth/to/file2.txt\n",
+                              u"file1.txt\npåth/to/file2.txt\n",  # git diff-tree <SHA>
                               u"file4.txt\npåth/to/file5.txt\n",
                               u"file6.txt\npåth/to/file7.txt\n"]
 
         with patch('gitlint.display.stderr', new=StringIO()) as stderr:
-            result = self.cli.invoke(cli.cli)
+            result = self.cli.invoke(cli.cli, ["--commits", "foo...bar"])
             # We expect that the second commit has no failures because of 'gitlint-ignore: T3' in its commit msg body
             expected = (u"Commit 6f29bf81a8:\n"
                         u'3: B5 Body message is too short (12<20): "commït-body1"\n\n'
@@ -182,16 +182,17 @@ def test_debug(self, sys, sh):
                                   u"commït-title2.\n\ncommït-body2",
                                   u"test åuthor3,test-email3@föo.com,2016-12-05 15:28:15 01:00,abc\n"
                                   u"föo\nbar"]
-        sh.git.side_effect = ["6f29bf81a8322a04071bb794666e48c443a90360\n"
+        sh.git.side_effect = ["6f29bf81a8322a04071bb794666e48c443a90360\n"  # git rev-list <SHA>
                               "25053ccec5e28e1bb8f7551fdbb5ab213ada2401\n"
                               "4da2656b0dadc76c7ee3fd0243a96cb64007f125\n",
-                              u"file1.txt\npåth/to/file2.txt\n",
+                              u"file1.txt\npåth/to/file2.txt\n",  # git diff-tree <SHA>
                               u"file4.txt\npåth/to/file5.txt\n",
                               u"file6.txt\npåth/to/file7.txt\n"]
 
         with patch('gitlint.display.stderr', new=StringIO()) as stderr:
             config_path = self.get_sample_path("config/gitlintconfig")
-            result = self.cli.invoke(cli.cli, ["--config", config_path, "--debug"])
+            result = self.cli.invoke(cli.cli, ["--config", config_path, "--debug", "--commits",
+                                               "foo...bar"])
 
             expected = "Commit 6f29bf81a8:\n3: B5\n\n" + \
                        "Commit 25053ccec5:\n1: T3\n3: B5\n\n" + \
@@ -331,7 +332,7 @@ def test_generate_config_negative(self):
     def test_git_error(self, sys, sh):
         """ Tests that the cli handles git errors properly """
         sys.stdin.isatty.return_value = True
-        sh.git.side_effect = CommandNotFound("git")
+        sh.git.log.side_effect = CommandNotFound("git")
         result = self.cli.invoke(cli.cli)
         self.assertEqual(result.exit_code, self.GIT_CONTEXT_ERROR_CODE)
 

diff --git a/gitlint/tests/test_git.py b/gitlint/tests/test_git.py
@@ -10,27 +10,53 @@
 
 
 class GitTests(BaseTestCase):
+    expected_sh_special_args = {
+        '_tty_out': False,
+        '_cwd': u"fåke/path"
+    }
+
     @patch('gitlint.git.sh')
     def test_get_latest_commit(self, sh):
         sample_sha = "d8ac47e9f2923c7f22d8668e3a1ed04eb4cdbca9"
 
-        def git_log_side_effect(*_args, **_kwargs):
-            return (u"test åuthor,test-emåil@foo.com,2016-12-03 15:28:15 01:00,åbc\n"
-                    u"cömmit-title\n\ncömmit-body")
+        sh.git.side_effect = [u"file1.txt\npåth/to/file2.txt\n"]
+        sh.git.log.side_effect = [sample_sha, u"test åuthor,test-emåil@foo.com,2016-12-03 15:28:15 01:00,åbc\n"
+                                              u"cömmit-title\n\ncömmit-body"]
+
+        context = GitContext.from_local_repository(u"fåke/path")
+        # assert that commit info was read using git command
+        expected_calls = [
+            call.log("-1", "--pretty=%H", **self.expected_sh_special_args),
+            call.log(sample_sha, "-1", "--pretty=%aN,%aE,%ai,%P%n%B", **self.expected_sh_special_args),
+            call('diff-tree', '--no-commit-id', '--name-only', '-r', sample_sha, **self.expected_sh_special_args)
+        ]
+        self.assertListEqual(sh.git.mock_calls, expected_calls)
+
+        last_commit = context.commits[-1]
+        self.assertEqual(last_commit.message.title, u"cömmit-title")
+        self.assertEqual(last_commit.message.body, ["", u"cömmit-body"])
+        self.assertEqual(last_commit.author_name, u"test åuthor")
+        self.assertEqual(last_commit.author_email, u"test-emåil@foo.com")
+        self.assertEqual(last_commit.date, datetime.datetime(2016, 12, 3, 15, 28, 15,
+                                                             tzinfo=dateutil.tz.tzoffset("+0100", 3600)))
+        self.assertListEqual(last_commit.parents, [u"åbc"])
+        self.assertFalse(last_commit.is_merge_commit)
+        self.assertListEqual(last_commit.changed_files, ["file1.txt", u"påth/to/file2.txt"])
+
+    @patch('gitlint.git.sh')
+    def test_from_local_repository_specific_ref(self, sh):
+        sample_sha = "myspecialref"
 
         sh.git.side_effect = [sample_sha, u"file1.txt\npåth/to/file2.txt\n"]
-        sh.git.log.side_effect = git_log_side_effect
+        sh.git.log.side_effect = [u"test åuthor,test-emåil@foo.com,2016-12-03 15:28:15 01:00,åbc\n"
+                                  u"cömmit-title\n\ncömmit-body"]
 
-        context = GitContext.from_local_repository(u"fake/påth")
-        expected_sh_special_args = {
-            '_tty_out': False,
-            '_cwd': u"fake/påth"
-        }
+        context = GitContext.from_local_repository(u"fåke/path", sample_sha)
         # assert that commit info was read using git command
         expected_calls = [
-            call("rev-list", "--max-count=1", "HEAD", **expected_sh_special_args),
-            call.log(sample_sha, "-1", "--pretty=%aN,%aE,%ai,%P%n%B", _cwd=u"fake/påth", _tty_out=False),
-            call('diff-tree', '--no-commit-id', '--name-only', '-r', sample_sha, **expected_sh_special_args)
+            call("rev-list", sample_sha, **self.expected_sh_special_args),
+            call.log(sample_sha, "-1", "--pretty=%aN,%aE,%ai,%P%n%B", **self.expected_sh_special_args),
+            call('diff-tree', '--no-commit-id', '--name-only', '-r', sample_sha, **self.expected_sh_special_args)
         ]
         self.assertListEqual(sh.git.mock_calls, expected_calls)
 
@@ -49,23 +75,16 @@ def git_log_side_effect(*_args, **_kwargs):
     def test_get_latest_commit_merge_commit(self, sh):
         sample_sha = "d8ac47e9f2923c7f22d8668e3a1ed04eb4cdbca9"
 
-        def git_log_side_effect(*_args, **_kwargs):
-            return (u"test åuthor,test-emåil@foo.com,2016-12-03 15:28:15 01:00,åbc def\n"
-                    u"Merge \"foo bår commit\"")
-
-        sh.git.side_effect = [sample_sha, u"file1.txt\npåth/to/file2.txt\n"]
-        sh.git.log.side_effect = git_log_side_effect
+        sh.git.side_effect = [u"file1.txt\npåth/to/file2.txt\n"]
+        sh.git.log.side_effect = [sample_sha, u"test åuthor,test-emåil@foo.com,2016-12-03 15:28:15 01:00,åbc def\n"
+                                              u"Merge \"foo bår commit\""]
 
         context = GitContext.from_local_repository(u"fåke/path")
-        expected_sh_special_args = {
-            '_tty_out': False,
-            '_cwd': u"fåke/path"
-        }
         # assert that commit info was read using git command
         expected_calls = [
-            call("rev-list", "--max-count=1", "HEAD", **expected_sh_special_args),
-            call.log(sample_sha, "-1", "--pretty=%aN,%aE,%ai,%P%n%B", _cwd=u"fåke/path", _tty_out=False),
-            call('diff-tree', '--no-commit-id', '--name-only', '-r', sample_sha, **expected_sh_special_args)
+            call.log("-1", "--pretty=%H", **self.expected_sh_special_args),
+            call.log(sample_sha, "-1", "--pretty=%aN,%aE,%ai,%P%n%B", **self.expected_sh_special_args),
+            call('diff-tree', '--no-commit-id', '--name-only', '-r', sample_sha, **self.expected_sh_special_args)
         ]
 
         self.assertEqual(sh.git.mock_calls, expected_calls)
@@ -83,38 +102,36 @@ def git_log_side_effect(*_args, **_kwargs):
 
     @patch('gitlint.git.sh')
     def test_get_latest_commit_command_not_found(self, sh):
-        sh.git.side_effect = CommandNotFound("git")
+        sh.git.log.side_effect = CommandNotFound("git")
         expected_msg = "'git' command not found. You need to install git to use gitlint on a local repository. " + \
                        "See https://git-scm.com/book/en/v2/Getting-Started-Installing-Git on how to install git."
         with self.assertRaisesRegex(GitNotInstalledError, expected_msg):
             GitContext.from_local_repository(u"fåke/path")
 
         # assert that commit message was read using git command
-        sh.git.assert_called_once_with("rev-list", "--max-count=1", "HEAD", _tty_out=False, _cwd=u"fåke/path")
+        sh.git.log.assert_called_once_with("-1", "--pretty=%H", **self.expected_sh_special_args)
 
     @patch('gitlint.git.sh')
     def test_get_latest_commit_git_error(self, sh):
         # Current directory not a git repo
         err = b"fatal: Not a git repository (or any of the parent directories): .git"
-        sh.git.side_effect = ErrorReturnCode("git rev-list --max-count=1 HEAD", b"", err)
+        sh.git.log.side_effect = ErrorReturnCode("git log -1 --pretty=%H", b"", err)
 
         with self.assertRaisesRegex(GitContextError, u"fåke/path is not a git repository."):
             GitContext.from_local_repository(u"fåke/path")
 
         # assert that commit message was read using git command
-        sh.git.assert_called_once_with("rev-list", "--max-count=1", "HEAD",
-                                       _tty_out=False, _cwd=u"fåke/path")
+        sh.git.log.assert_called_once_with("-1", "--pretty=%H", **self.expected_sh_special_args)
         sh.git.reset_mock()
         err = b"fatal: Random git error"
-        sh.git.side_effect = ErrorReturnCode("git rev-list --max-count=1 HEAD", b"", err)
+        sh.git.log.side_effect = ErrorReturnCode("git log -1 --pretty=%H", b"", err)
 
-        expected_msg = u"An error occurred while executing 'git rev-list --max-count=1 HEAD': {0}".format(err)
+        expected_msg = u"An error occurred while executing 'git log -1 --pretty=%H': {0}".format(err)
         with self.assertRaisesRegex(GitContextError, expected_msg):
             GitContext.from_local_repository(u"fåke/path")
 
         # assert that commit message was read using git command
-        sh.git.assert_called_once_with("rev-list", "--max-count=1", "HEAD",
-                                       _tty_out=False, _cwd=u"fåke/path")
+        sh.git.log.assert_called_once_with("-1", "--pretty=%H", **self.expected_sh_special_args)
 
     def test_from_commit_msg_full(self):
         gitcontext = GitContext.from_commit_msg(self.get_sample("commit_message/sample1"))