diff options
author | Joris Roovers <joris.roovers@gmail.com> | 2017-07-14 15:07:27 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-07-14 15:07:27 -0700 |
commit | 2881b5297d783ce1e2df5a427b26e3c15b04aaa8 (patch) | |
tree | cc99e7a85510865e430fa6d442d77b80ca2e1049 | |
parent | bc05276188ca566b0b7ebc8fd43d95d44b60a78d (diff) |
Gitlint now correctly handles refspec (#35)
Gitlint did not correctly handle all refspec formats that were supported
by `git rev-list`. It does now :)
This fixes #23.
-rw-r--r-- | CHANGELOG.md | 8 | ||||
-rw-r--r-- | gitlint/cli.py | 2 | ||||
-rw-r--r-- | gitlint/git.py | 14 | ||||
-rw-r--r-- | gitlint/tests/test_cli.py | 21 | ||||
-rw-r--r-- | gitlint/tests/test_git.py | 85 | ||||
-rw-r--r-- | qa/base.py | 50 | ||||
-rw-r--r-- | qa/test_commits.py | 28 |
7 files changed, 134 insertions, 74 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index fba098f..174b3fc 100644 --- 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 index d541cc5..a9dc19a 100644 --- 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 index c155ecd..62730c6 100644 --- a/gitlint/git.py +++ b/gitlint/git.py @@ -148,7 +148,7 @@ class GitContext(object): 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 @@ class GitContext(object): '_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 index 624a2e8..07d295b 100644 --- a/gitlint/tests/test_cli.py +++ b/gitlint/tests/test_cli.py @@ -73,15 +73,15 @@ class CLITests(BaseTestCase): 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 @@ class CLITests(BaseTestCase): 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 @@ class CLITests(BaseTestCase): 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 @@ class CLITests(BaseTestCase): 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 index a4f439d..ec6a8aa 100644 --- a/gitlint/tests/test_git.py +++ b/gitlint/tests/test_git.py @@ -10,27 +10,53 @@ from gitlint.git import GitContext, GitCommit, GitCommitMessage, GitContextError 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 @@ class GitTests(BaseTestCase): 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 @@ class GitTests(BaseTestCase): @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")) @@ -20,30 +20,40 @@ class BaseTestCase(TestCase): @classmethod def setUpClass(cls): """ Sets up the integration tests by creating a new temporary git repository """ - cls.tmp_git_repo = os.path.realpath("/tmp/gitlint-test-%s" % datetime.now().strftime("%Y%m%d-%H%M%S")) - git("init", cls.tmp_git_repo) + cls._tmp_git_repos = [cls.create_tmp_git_repo()] + cls.tmp_git_repo = cls._tmp_git_repos[0] + + @classmethod + def tearDownClass(cls): + """ Cleans up the temporary git repositories """ + for repo in cls._tmp_git_repos: + rm("-rf", repo) + + @classmethod + def create_tmp_git_repo(cls): + """ Creates a temporary git repository and returns its directory path """ + tmp_git_repo = os.path.realpath("/tmp/gitlint-test-%s" % datetime.now().strftime("%Y%m%d-%H%M%S")) + git("init", tmp_git_repo) # configuring name and email is required in every git repot - git("config", "user.name", "gitlint-test-user", _cwd=cls.tmp_git_repo) - git("config", "user.email", "gitlint@test.com", _cwd=cls.tmp_git_repo) + git("config", "user.name", "gitlint-test-user", _cwd=tmp_git_repo) + git("config", "user.email", "gitlint@test.com", _cwd=tmp_git_repo) # Git does not by default print unicode paths, fix that by setting core.quotePath to false # http://stackoverflow.com/questions/34549040/git-not-displaying-unicode-file-names # ftp://www.kernel.org/pub/software/scm/git/docs/git-config.html - git("config", "core.quotePath", "false", _cwd=cls.tmp_git_repo) + git("config", "core.quotePath", "false", _cwd=tmp_git_repo) # Git on mac doesn't like unicode characters by default, so we need to set this option # http://stackoverflow.com/questions/5581857/git-and-the-umlaut-problem-on-mac-os-x - git("config", "core.precomposeunicode", "true", _cwd=cls.tmp_git_repo) + git("config", "core.precomposeunicode", "true", _cwd=tmp_git_repo) + return tmp_git_repo - @classmethod - def tearDownClass(cls): - """ Cleans up the temporary git repository """ - rm("-rf", cls.tmp_git_repo) - - def _create_simple_commit(self, message, out=None, ok_code=None, env=None): + def _create_simple_commit(self, message, out=None, ok_code=None, env=None, git_repo=None): """ Creates a simple commit with an empty test file. :param message: Commit message for the commit. """ + git_repo = self.tmp_git_repo if git_repo is None else git_repo + # Let's make sure that we copy the environment in which this python code was executed as environment # variables can influence how git runs. # This was needed to fix https://github.com/jorisroovers/gitlint/issues/15 as we need to make sure to use @@ -53,13 +63,13 @@ class BaseTestCase(TestCase): environment.update(env) test_filename = u"test-fïle-" + str(uuid4()) - touch(test_filename, _cwd=self.tmp_git_repo) - git("add", test_filename, _cwd=self.tmp_git_repo) + touch(test_filename, _cwd=git_repo) + git("add", test_filename, _cwd=git_repo) # https://amoffat.github.io/sh/#interactive-callbacks if not ok_code: ok_code = [0] - git("commit", "-m", message, _cwd=self.tmp_git_repo, _tty_in=True, _out=out, _ok_code=ok_code, _env=environment) + git("commit", "-m", message, _cwd=git_repo, _tty_in=True, _out=out, _ok_code=ok_code, _env=environment) return test_filename @staticmethod @@ -72,11 +82,13 @@ class BaseTestCase(TestCase): samples_dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), "samples") return os.path.join(samples_dir, filename) - def get_last_commit_short_hash(self): - return git("rev-parse", "--short", "HEAD", _cwd=self.tmp_git_repo, _tty_in=True).replace("\n", "") + def get_last_commit_short_hash(self, git_repo=None): + git_repo = self.tmp_git_repo if git_repo is None else git_repo + return git("rev-parse", "--short", "HEAD", _cwd=git_repo, _tty_in=True).replace("\n", "") - def get_last_commit_hash(self): - return git("rev-parse", "HEAD", _cwd=self.tmp_git_repo, _tty_in=True).replace("\n", "") + def get_last_commit_hash(self, git_repo=None): + git_repo = self.tmp_git_repo if git_repo is None else git_repo + return git("rev-parse", "HEAD", _cwd=git_repo, _tty_in=True).replace("\n", "") @staticmethod def get_expected(filename="", variable_dict=None): diff --git a/qa/test_commits.py b/qa/test_commits.py index 82deafb..eb7b590 100644 --- a/qa/test_commits.py +++ b/qa/test_commits.py @@ -5,7 +5,8 @@ from qa.base import BaseTestCase class CommitsTests(BaseTestCase): - """ Integration tests for linting multiple commits at once """ + """ Integration tests for the --commits argument, i.e. linting multiple commits at once or linting specific commits + """ def test_successful(self): """ Test linting multiple commits without violations """ @@ -41,13 +42,34 @@ class CommitsTests(BaseTestCase): self.assertEqual(output.exit_code, 4) self.assertEqual(output, expected) - def test_single_commit(self): + def test_lint_single_commit(self): self._create_simple_commit(u"Sïmple title.\n") self._create_simple_commit(u"Sïmple title2.\n") commit_sha = self.get_last_commit_hash() + refspec = "{0}^...{0}".format(commit_sha) self._create_simple_commit(u"Sïmple title3.\n") - output = gitlint("--commits", commit_sha, _cwd=self.tmp_git_repo, _tty_in=True, _ok_code=[2]) + output = gitlint("--commits", refspec, _cwd=self.tmp_git_repo, _tty_in=True, _ok_code=[2]) expected = (u"1: T3 Title has trailing punctuation (.): \"Sïmple title2.\"\n" + u"3: B6 Body message is missing\n") self.assertEqual(output.exit_code, 2) self.assertEqual(output, expected) + + def test_lint_head(self): + """ Testing whether we can also recognize special refs like 'HEAD' """ + tmp_git_repo = self.create_tmp_git_repo() + self._create_simple_commit(u"Sïmple title.\n\nSimple bödy describing the commit", git_repo=tmp_git_repo) + self._create_simple_commit(u"Sïmple title", git_repo=tmp_git_repo) + self._create_simple_commit(u"WIP: Sïmple title\n\nSimple bödy describing the commit", git_repo=tmp_git_repo) + output = gitlint("--commits", "HEAD", _cwd=tmp_git_repo, _tty_in=True, _ok_code=[3]) + revlist = git("rev-list", "HEAD", _tty_in=True, _cwd=tmp_git_repo).split() + + expected = ( + u"Commit {0}:\n".format(revlist[0][:10]) + + u"1: T5 Title contains the word 'WIP' (case-insensitive): \"WIP: Sïmple title\"\n\n" + + u"Commit {0}:\n".format(revlist[1][:10]) + + u"3: B6 Body message is missing\n\n" + + u"Commit {0}:\n".format(revlist[2][:10]) + + u"1: T3 Title has trailing punctuation (.): \"Sïmple title.\"\n" + ) + + self.assertEqual(output, expected) |