Fix: --commits doesn't take commit specific config into account

When linting multiple commits at once, gitlint will now take
commit-specific configuration into account for every commit, not just of
the last commit. Implemented by cloning the lintconfig builder object
for every commit.

Also added a few comments to existing tests.

This fixes #24.

gitlint/cli.py

@@ -122,28 +122,30 @@ def lint(ctx):
         ctx.exit(GIT_CONTEXT_ERROR_CODE)
 
     number_of_commits = len(gitcontext.commits)
-
     # Exit if we don't have commits in the specified range. Use a 0 exit code, since a popular use-case is one
     # where users are using --commits in a check job to check the commit messages inside a CI job. By returning 0, we
     # ensure that these jobs don't fail if for whatever reason the specified commit range is empty.
-
     if number_of_commits == 0:
         click.echo(u'No commits in range "{0}".'.format(ctx.obj[2]))
         ctx.exit(0)
 
-    config_builder = ctx.obj[1]
+    general_config_builder = ctx.obj[1]
     last_commit = gitcontext.commits[-1]
-    # Apply an additional config that is specified in the last commit message
-    config_builder.set_config_from_commit(last_commit)
-    lint_config = config_builder.build(lint_config)
 
     # Let's get linting!
-    linter = GitLinter(lint_config)
     first_violation = True
-
     exit_code = 0
     for commit in gitcontext.commits:
+
+        # Build a config_builder and linter taking into account the commit specific config (if any)
+        config_builder = general_config_builder.clone()
+        config_builder.set_config_from_commit(commit)
+        lint_config = config_builder.build(lint_config)
+        linter = GitLinter(lint_config)
+
+        # Actually do the linting
         violations = linter.lint(commit)
+        # exit code equals the total number of violations in all commits
         exit_code += len(violations)
         if violations:
             # Display the commit hash & new lines intelligently
@@ -155,6 +157,8 @@ def lint(ctx):
             linter.print_violations(violations)
             first_violation = False
 
+    # cap actual max exit code because bash doesn't like exit codes larger than 255:
+    # http://tldp.org/LDP/abs/html/exitcodes.html
     exit_code = min(MAX_VIOLATION_ERROR_CODE, exit_code)
     ctx.exit(exit_code)
 

gitlint/config.py

@@ -5,6 +5,7 @@
     # python 3.x
     from configparser import ConfigParser, Error as ConfigParserError  # pragma: no cover, pylint: disable=import-error
 
+import copy
 import re
 import os
 import shutil
@@ -255,7 +256,7 @@ def set_config_from_commit(self, commit):
             Supported:
              - gitlint-ignore: all
         """
-        for line in commit.message.full.split("\n"):
+        for line in commit.message.body:
             pattern = re.compile(r"^gitlint-ignore:\s*(.*)")
             matches = pattern.match(line)
             if matches and len(matches.groups()) == 1:
@@ -316,6 +317,13 @@ def build(self, config=None):
 
         return config
 
+    def clone(self):
+        """ Creates an exact copy of a LintConfigBuilder.  """
+        builder = LintConfigBuilder()
+        builder._config_blueprint = copy.deepcopy(self._config_blueprint)
+        builder._config_path = self._config_path
+        return builder
+
 
 GITLINT_CONFIG_TEMPLATE_SRC_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), "files/gitlint")
 

gitlint/tests/test_cli.py

@@ -29,12 +29,14 @@ def setUp(self):
         self.cli = CliRunner()
 
     def test_version(self):
+        """ Test for --version option """
         result = self.cli.invoke(cli.cli, ["--version"])
         self.assertEqual(result.output.split("\n")[0], "cli, version {0}".format(__version__))
 
     @patch('gitlint.git.sh')
     @patch('gitlint.cli.sys')
     def test_lint(self, sys, sh):
+        """ Test for basic simple linting functionality """
         sys.stdin.isatty.return_value = True
 
         def git_log_side_effect(*_args, **_kwargs):
@@ -52,6 +54,7 @@ def git_log_side_effect(*_args, **_kwargs):
     @patch('gitlint.git.sh')
     @patch('gitlint.cli.sys')
     def test_lint_multiple_commits(self, sys, sh):
+        """ Test for --commits option """
         sys.stdin.isatty.return_value = True
 
         sh.git.log.side_effect = [u"test åuthor1,test-email1@föo.com,2016-12-03 15:28:15 01:00,åbc\n"
@@ -78,7 +81,38 @@ def test_lint_multiple_commits(self, sys, sh):
             self.assertEqual(stderr.getvalue(), expected)
             self.assertEqual(result.exit_code, 3)
 
+    @patch('gitlint.git.sh')
+    @patch('gitlint.cli.sys')
+    def test_lint_multiple_commits_config(self, sys, sh):
+        """ Test for --commits option where some of the commits have gitlint config in the commit message """
+        sys.stdin.isatty.return_value = True
+
+        # Note that the second commit title has a trailing period that is being ignored by gitlint-ignore: T3
+        sh.git.log.side_effect = [u"test åuthor1,test-email1@föo.com,2016-12-03 15:28:15 01:00,åbc\n"
+                                  u"commït-title1\n\ncommït-body1",
+                                  u"test åuthor2,test-email3@föo.com,2016-12-04 15:28:15 01:00,åbc\n"
+                                  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" +
+                              "25053ccec5e28e1bb8f7551fdbb5ab213ada2401\n" +
+                              "4da2656b0dadc76c7ee3fd0243a96cb64007f125\n",
+                              u"file1.txt\npåth/to/file2.txt\n",
+                              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)
+            # 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'
+                        u"Commit 4da2656b0d:\n"
+                        u'3: B5 Body message is too short (12<20): "commït-body3"\n')
+            self.assertEqual(stderr.getvalue(), expected)
+            self.assertEqual(result.exit_code, 2)
+
     def test_input_stream(self):
+        """ Test for linting when a message is passed via stdin """
         expected_output = u"1: T2 Title has trailing whitespace: \"WIP: tïtle \"\n" + \
                           u"1: T5 Title contains the word 'WIP' (case-insensitive): \"WIP: tïtle \"\n" + \
                           u"3: B6 Body message is missing\n"
@@ -90,13 +124,15 @@ def test_input_stream(self):
             self.assertEqual(result.output, "")
 
     def test_silent_mode(self):
+        """ Test for --silent option """
         with patch('gitlint.display.stderr', new=StringIO()) as stderr:
             result = self.cli.invoke(cli.cli, ["--silent"], input=u"WIP: tïtle \n")
             self.assertEqual(stderr.getvalue(), "")
             self.assertEqual(result.exit_code, 3)
             self.assertEqual(result.output, "")
 
     def test_verbosity(self):
+        """ Test for --verbosity option """
         # We only test -v and -vv, more testing is really not required here
         # -v
         with patch('gitlint.display.stderr', new=StringIO()) as stderr:
@@ -124,6 +160,7 @@ def test_verbosity(self):
             self.assertEqual(result.output, "Config Error: Option 'verbosity' must be set between 0 and 3\n")
 
     def test_debug(self):
+        """ Test for --debug option """
         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"], input=u"WIP: tëst")
@@ -135,6 +172,7 @@ def test_debug(self):
             self.assertEqual(result.exit_code, 2)
 
     def test_extra_path(self):
+        """ Test for --extra-path flag """
         with patch('gitlint.display.stderr', new=StringIO()) as stderr:
             extra_path = self.get_sample_path("user_rules")
             result = self.cli.invoke(cli.cli, ["--extra-path", extra_path, "--debug"], input=u"Test tïtle\n")
@@ -144,6 +182,7 @@ def test_extra_path(self):
             self.assertEqual(result.exit_code, 2)
 
     def test_config_file(self):
+        """ Test for --config option """
         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], input=u"WIP: tëst")
@@ -152,6 +191,7 @@ def test_config_file(self):
             self.assertEqual(result.exit_code, 2)
 
     def test_config_file_negative(self):
+        """ Negative test for --config option """
         # Directory as config file
         config_path = self.get_sample_path("config")
         result = self.cli.invoke(cli.cli, ["--config", config_path])
@@ -175,6 +215,7 @@ def test_config_file_negative(self):
 
     @patch('gitlint.cli.sys')
     def test_target(self, sys):
+        """ Test for the --target option """
         sys.stdin.isatty.return_value = True
         result = self.cli.invoke(cli.cli, ["--target", "/tmp"])
         # We expect gitlint to tell us that /tmp is not a git repo (this proves that it takes the target parameter
@@ -184,6 +225,7 @@ def test_target(self, sys):
         self.assertEqual(result.output, "%s is not a git repository.\n" % expected_path)
 
     def test_target_negative(self):
+        """ Negative test for the --target option """
         # try setting a non-existing target
         result = self.cli.invoke(cli.cli, ["--target", u"/föo/bar"])
         self.assertEqual(result.exit_code, self.USAGE_ERROR_CODE)
@@ -199,6 +241,7 @@ def test_target_negative(self):
 
     @patch('gitlint.config.LintConfigGenerator.generate_config')
     def test_generate_config(self, generate_config):
+        """ Test for the generate-config subcommand """
         result = self.cli.invoke(cli.cli, ["generate-config"], input=u"tëstfile\n")
         self.assertEqual(result.exit_code, 0)
         expected_msg = u"Please specify a location for the sample gitlint config file [.gitlint]: tëstfile\n" + \
@@ -207,6 +250,7 @@ def test_generate_config(self, generate_config):
         generate_config.assert_called_once_with(os.path.abspath(u"tëstfile"))
 
     def test_generate_config_negative(self):
+        """ Negative test for the generate-config subcommand """
         # Non-existing directory
         result = self.cli.invoke(cli.cli, ["generate-config"], input=u"/föo/bar")
         self.assertEqual(result.exit_code, self.USAGE_ERROR_CODE)
@@ -226,6 +270,7 @@ def test_generate_config_negative(self):
     @patch('gitlint.git.sh')
     @patch('gitlint.cli.sys')
     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")
         result = self.cli.invoke(cli.cli)
@@ -234,6 +279,7 @@ def test_git_error(self, sys, sh):
     @patch('gitlint.git.sh')
     @patch('gitlint.cli.sys')
     def test_no_commits_in_range(self, sys, sh):
+        """ Test for --commits with the specified range being empty. """
         sys.stdin.isatty.return_value = True
         sh.git.side_effect = lambda *_args, **_kwargs: ""
         result = self.cli.invoke(cli.cli, ["--commits", "master...HEAD"])
@@ -243,6 +289,7 @@ def test_no_commits_in_range(self, sys, sh):
 
     @patch('gitlint.hooks.GitHookInstaller.install_commit_msg_hook')
     def test_install_hook(self, install_hook):
+        """ Test for install-hook subcommand """
         result = self.cli.invoke(cli.cli, ["install-hook"])
         expected_path = os.path.join(os.getcwd(), hooks.COMMIT_MSG_HOOK_DST_PATH)
         expected = "Successfully installed gitlint commit-msg hook in {0}\n".format(expected_path)
@@ -254,6 +301,7 @@ def test_install_hook(self, install_hook):
 
     @patch('gitlint.hooks.GitHookInstaller.install_commit_msg_hook')
     def test_install_hook_target(self, install_hook):
+        """  Test for install-hook subcommand with a specific --target option specified """
         # Specified target
         result = self.cli.invoke(cli.cli, ["--target", self.SAMPLES_DIR, "install-hook"])
         expected_path = os.path.realpath(os.path.join(self.SAMPLES_DIR, hooks.COMMIT_MSG_HOOK_DST_PATH))
@@ -267,6 +315,7 @@ def test_install_hook_target(self, install_hook):
 
     @patch('gitlint.hooks.GitHookInstaller.install_commit_msg_hook', side_effect=hooks.GitHookInstallerError(u"tëst"))
     def test_install_hook_negative(self, install_hook):
+        """ Negative test for install-hook subcommand """
         result = self.cli.invoke(cli.cli, ["install-hook"])
         self.assertEqual(result.exit_code, self.GIT_CONTEXT_ERROR_CODE)
         self.assertEqual(result.output, u"tëst\n")
@@ -276,6 +325,7 @@ def test_install_hook_negative(self, install_hook):
 
     @patch('gitlint.hooks.GitHookInstaller.uninstall_commit_msg_hook')
     def test_uninstall_hook(self, uninstall_hook):
+        """ Test for uninstall-hook subcommand """
         result = self.cli.invoke(cli.cli, ["uninstall-hook"])
         expected_path = os.path.realpath(os.path.join(os.getcwd(), hooks.COMMIT_MSG_HOOK_DST_PATH))
         expected = "Successfully uninstalled gitlint commit-msg hook from {0}\n".format(expected_path)
@@ -287,6 +337,7 @@ def test_uninstall_hook(self, uninstall_hook):
 
     @patch('gitlint.hooks.GitHookInstaller.uninstall_commit_msg_hook', side_effect=hooks.GitHookInstallerError(u"tëst"))
     def test_uninstall_hook_negative(self, uninstall_hook):
+        """ Negative test for uninstall-hook subcommand """
         result = self.cli.invoke(cli.cli, ["uninstall-hook"])
         self.assertEqual(result.exit_code, self.GIT_CONTEXT_ERROR_CODE)
         self.assertEqual(result.output, u"tëst\n")

gitlint/tests/test_config_builder.py

@@ -6,6 +6,31 @@
 
 
 class LintConfigBuilderTests(BaseTestCase):
+    def test_set_option(self):
+        config_builder = LintConfigBuilder()
+        config = config_builder.build()
+
+        # assert some defaults
+        self.assertEqual(config.get_rule_option('title-max-length', 'line-length'), 72)
+        self.assertEqual(config.get_rule_option('body-max-line-length', 'line-length'), 80)
+        self.assertListEqual(config.get_rule_option('title-must-not-contain-word', 'words'), ["WIP"])
+        self.assertEqual(config.verbosity, 3)
+
+        # Make some changes and check blueprint
+        config_builder.set_option('title-max-length', 'line-length', 100)
+        config_builder.set_option('general', 'verbosity', 2)
+        config_builder.set_option('title-must-not-contain-word', 'words', ["foo", "bar"])
+        expected_blueprint = {'title-must-not-contain-word': {'words': ['foo', 'bar']},
+                              'title-max-length': {'line-length': 100}, 'general': {'verbosity': 2}}
+        self.assertDictEqual(config_builder._config_blueprint, expected_blueprint)
+
+        # Build config and verify that the changes have occurred and no other changes
+        config = config_builder.build()
+        self.assertEqual(config.get_rule_option('title-max-length', 'line-length'), 100)
+        self.assertEqual(config.get_rule_option('body-max-line-length', 'line-length'), 80)  # should be unchanged
+        self.assertListEqual(config.get_rule_option('title-must-not-contain-word', 'words'), ["foo", "bar"])
+        self.assertEqual(config.verbosity, 2)
+
     def test_set_from_commit_ignore_all(self):
         config = LintConfig()
         original_rules = config.rules
@@ -107,11 +132,6 @@ def test_set_from_config_file_negative(self):
 
     def test_set_config_from_string_list(self):
         config = LintConfig()
-        # assert some defaults
-        self.assertEqual(config.get_rule_option('title-max-length', 'line-length'), 72)
-        self.assertEqual(config.get_rule_option('body-max-line-length', 'line-length'), 80)
-        self.assertListEqual(config.get_rule_option('title-must-not-contain-word', 'words'), ["WIP"])
-        self.assertEqual(config.verbosity, 3)
 
         # change and assert changes
         config_builder = LintConfigBuilder()
@@ -160,8 +180,24 @@ def test_rebuild_config(self):
         lint_config = config_builder.build()
         self.assertEqual(lint_config.verbosity, 3)
 
-        # check that existing config changes when we rebuild it
+        # check that existing config gets overwritten when we pass it to a configbuilder with different options
         existing_lintconfig = LintConfig()
         existing_lintconfig.verbosity = 2
         lint_config = config_builder.build(existing_lintconfig)
         self.assertEqual(lint_config.verbosity, 3)
+        self.assertEqual(existing_lintconfig.verbosity, 3)
+
+    def test_clone(self):
+        config_builder = LintConfigBuilder()
+        config_builder.set_option('general', 'verbosity', 2)
+        config_builder.set_option('title-max-length', 'line-length', 100)
+        expected = {'title-max-length': {'line-length': 100}, 'general': {'verbosity': 2}}
+        self.assertDictEqual(config_builder._config_blueprint, expected)
+
+        # Clone and verify that the blueprint is the same as the original
+        cloned_builder = config_builder.clone()
+        self.assertDictEqual(cloned_builder._config_blueprint, expected)
+
+        # Modify the original and make sure we're not modifying the clone (i.e. check that the copy is a deep copy)
+        config_builder.set_option('title-max-length', 'line-length', 120)
+        self.assertDictEqual(cloned_builder._config_blueprint, expected)

qa/test_config.py

@@ -7,7 +7,7 @@
 class ConfigTests(BaseTestCase):
     """ Integration tests for gitlint configuration and configuration precedence. """
 
-    def test_ignore_by_code(self):
+    def test_ignore_by_id(self):
         self._create_simple_commit(u"WIP: Thïs is a title.\nContënt on the second line")
         output = gitlint("--ignore", "T5,B4", _cwd=self.tmp_git_repo, _tty_in=True, _ok_code=[1])
         expected = u"1: T3 Title has trailing punctuation (.): \"WIP: Thïs is a title.\"\n"