From 8bfb5c07709177225e289608a18d131d310c4eba Mon Sep 17 00:00:00 2001 From: Andrew Hare Date: Tue, 25 Feb 2020 12:40:40 -0700 Subject: [PATCH 1/3] Initial design of using pre-commit for Git hooks --- dvc/command/install.py | 9 ++++- dvc/repo/install.py | 4 +- dvc/scm/base.py | 9 ++++- dvc/scm/git/__init__.py | 81 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/dvc/command/install.py b/dvc/command/install.py index 1dea3fcb07..39c33802e6 100644 --- a/dvc/command/install.py +++ b/dvc/command/install.py @@ -11,7 +11,7 @@ class CmdInstall(CmdBase): def run(self): try: - self.repo.install() + self.repo.install(self.args.use_pre_commit_tool) except Exception: logger.exception("failed to install DVC Git hooks") return 1 @@ -27,4 +27,11 @@ def add_parser(subparsers, parent_parser): help=INSTALL_HELP, formatter_class=argparse.RawDescriptionHelpFormatter, ) + install_parser.add_argument( + "--use-pre-commit-tool", + action="store_true", + default=False, + help="Install DVC hooks using pre-commit " + "(https://pre-commit.com) if it is installed.", + ) install_parser.set_defaults(func=CmdInstall) diff --git a/dvc/repo/install.py b/dvc/repo/install.py index cff23a15ff..38612d608d 100644 --- a/dvc/repo/install.py +++ b/dvc/repo/install.py @@ -1,2 +1,2 @@ -def install(self): - self.scm.install() +def install(self, use_pre_commit_tool): + self.scm.install(use_pre_commit_tool) diff --git a/dvc/scm/base.py b/dvc/scm/base.py index 779b1009fa..21f2add151 100644 --- a/dvc/scm/base.py +++ b/dvc/scm/base.py @@ -114,8 +114,13 @@ def list_all_commits(self): # pylint: disable=no-self-use """Returns a list of commits in the repo.""" return [] - def install(self): - """Adds dvc commands to SCM hooks for the repo.""" + def install(self, use_pre_commit_tool): + """ + Adds dvc commands to SCM hooks for the repo. + + If use_pre_commit_tool is set and pre-commit is + installed it will be used to install the hooks. + """ def cleanup_ignores(self): """ diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index efcf5b5aa6..81c85b2b1e 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -2,10 +2,14 @@ import logging import os +from shutil import which +from subprocess import check_call +import yaml from funcy import cached_property from pathspec.patterns import GitWildMatchPattern +from dvc.exceptions import DvcException from dvc.exceptions import GitHookAlreadyExistsError from dvc.scm.base import Base from dvc.scm.base import CloneError, FileNotInRepoError, RevError, SCMError @@ -273,7 +277,82 @@ def _install_hook(self, name, preconditions, cmd): os.chmod(hook, 0o777) - def install(self): + def _install_with_pre_commit_tool(self): + if not which("pre-commit"): + raise DvcException("pre-commit is not installed") + + check_call("pre-commit install", shell=True) + + pre_commit_entry = ( + 'bash -c "#!/bin/sh\\n' + 'if [ -n "$(git ls-files --full-name .dvc)" ]; ' + 'then exec dvc status; fi"' + ) + + pre_push_entry = ( + 'bash -c "#!/bin/sh\\n' + 'if [ -n "$(git ls-files --full-name .dvc)" ]; å' + 'then exec dvc push; fi"' + ) + + # post_checkout_entry = ( + # 'bash -c "#!/bin/sh\\n' + # 'if [ -n "$(git ls-files --full-name .dvc)" ] ' + # '&& [ "$3" = "1" ] && [ ! -d .git/rebase-merge ]; ' + # 'then exec dvc checkout; fi"' + # ) + + conf = { + "repos": [ + { + "repo": "local", + "hooks": [ + { + "id": "dvc-pre-commit", + "name": "DVC Pre Commit", + "entry": pre_commit_entry, + "language": "system", + "stages": ["commit"], + }, + { + "id": "dvc-pre-push", + "name": "DVC Pre Push", + "entry": pre_push_entry, + "language": "system", + "stages": ["push"], + }, + # TODO(andrewhare): Enable this with latest + # pre-commit changes. + # { + # "id": "dvc-post-checkout", + # "name": "DVC Post Checkout", + # "entry": post_checkout_entry, + # "language": "system", + # "always_run": True, + # "stages": ["post-checkout"], + # } + ], + } + ] + } + + conf_yaml = ".pre-commit-config.yaml" + with open(conf_yaml, "w+") as f: + existing_conf = yaml.safe_load(f) + if existing_conf: + # TODO(andrewhare): Do an intelligent merge of `conf` + # and `existing_conf` if the user already has a + # pre-commit config YAML. + pass + yaml.dump(conf, f) + + os.chmod(conf_yaml, 0o777) + + def install(self, use_pre_commit_tool): + if use_pre_commit_tool: + self._install_with_pre_commit_tool() + return + self._verify_dvc_hooks() self._install_hook( From 7c0792f29aea9c616dffb90543e16e7afa7212a8 Mon Sep 17 00:00:00 2001 From: Andrew Hare Date: Thu, 5 Mar 2020 15:49:33 -0700 Subject: [PATCH 2/3] Refactor to reuse existing code and provide merge capabilities --- dvc/scm/git/__init__.py | 130 ++++++++++--------------- dvc/scm/git/pre_commit_tool.py | 40 ++++++++ tests/unit/scm/test_pre_commit_tool.py | 31 ++++++ 3 files changed, 122 insertions(+), 79 deletions(-) create mode 100644 dvc/scm/git/pre_commit_tool.py create mode 100644 tests/unit/scm/test_pre_commit_tool.py diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index 81c85b2b1e..fe9a747f6e 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -2,9 +2,10 @@ import logging import os +import yaml from shutil import which from subprocess import check_call -import yaml +from pathlib import Path from funcy import cached_property from pathspec.patterns import GitWildMatchPattern @@ -13,6 +14,8 @@ from dvc.exceptions import GitHookAlreadyExistsError from dvc.scm.base import Base from dvc.scm.base import CloneError, FileNotInRepoError, RevError, SCMError +from dvc.scm.git.pre_commit_tool import pre_commit_tool_conf +from dvc.scm.git.pre_commit_tool import merge_pre_commit_tool_confs from dvc.scm.git.tree import GitTree from dvc.utils import fix_env, is_binary, relpath from dvc.utils.fs import path_isin @@ -257,7 +260,7 @@ def list_tags(self): def list_all_commits(self): return [c.hexsha for c in self.repo.iter_commits("--all")] - def _install_hook(self, name, preconditions, cmd): + def _install_hook(self, name, preconditions, cmd, hook_path_fn): # only run in dvc repo in_dvc_repo = '[ -n "$(git ls-files --full-name .dvc)" ]' @@ -265,7 +268,7 @@ def _install_hook(self, name, preconditions, cmd): " && ".join([in_dvc_repo] + preconditions), cmd ) - hook = self._hook_path(name) + hook = hook_path_fn(name) if os.path.isfile(hook): with open(hook, "r+") as fobj: @@ -277,83 +280,15 @@ def _install_hook(self, name, preconditions, cmd): os.chmod(hook, 0o777) - def _install_with_pre_commit_tool(self): - if not which("pre-commit"): - raise DvcException("pre-commit is not installed") - - check_call("pre-commit install", shell=True) - - pre_commit_entry = ( - 'bash -c "#!/bin/sh\\n' - 'if [ -n "$(git ls-files --full-name .dvc)" ]; ' - 'then exec dvc status; fi"' - ) - - pre_push_entry = ( - 'bash -c "#!/bin/sh\\n' - 'if [ -n "$(git ls-files --full-name .dvc)" ]; å' - 'then exec dvc push; fi"' - ) + def install(self, use_pre_commit_tool): + self._verify_dvc_hooks() - # post_checkout_entry = ( - # 'bash -c "#!/bin/sh\\n' - # 'if [ -n "$(git ls-files --full-name .dvc)" ] ' - # '&& [ "$3" = "1" ] && [ ! -d .git/rebase-merge ]; ' - # 'then exec dvc checkout; fi"' - # ) - - conf = { - "repos": [ - { - "repo": "local", - "hooks": [ - { - "id": "dvc-pre-commit", - "name": "DVC Pre Commit", - "entry": pre_commit_entry, - "language": "system", - "stages": ["commit"], - }, - { - "id": "dvc-pre-push", - "name": "DVC Pre Push", - "entry": pre_push_entry, - "language": "system", - "stages": ["push"], - }, - # TODO(andrewhare): Enable this with latest - # pre-commit changes. - # { - # "id": "dvc-post-checkout", - # "name": "DVC Post Checkout", - # "entry": post_checkout_entry, - # "language": "system", - # "always_run": True, - # "stages": ["post-checkout"], - # } - ], - } - ] - } - - conf_yaml = ".pre-commit-config.yaml" - with open(conf_yaml, "w+") as f: - existing_conf = yaml.safe_load(f) - if existing_conf: - # TODO(andrewhare): Do an intelligent merge of `conf` - # and `existing_conf` if the user already has a - # pre-commit config YAML. - pass - yaml.dump(conf, f) - - os.chmod(conf_yaml, 0o777) + hook_path_fn = self._hook_path - def install(self, use_pre_commit_tool): if use_pre_commit_tool: - self._install_with_pre_commit_tool() - return - - self._verify_dvc_hooks() + hook_path_fn = self._pre_commit_tool_hook_path + path = Path(self._pre_commit_tool_hooks_home) + path.mkdir(parents=True, exist_ok=True) self._install_hook( "post-checkout", @@ -366,9 +301,32 @@ def install(self, use_pre_commit_tool): "[ ! -d .git/rebase-merge ]", ], "checkout", + hook_path_fn, ) - self._install_hook("pre-commit", [], "status") - self._install_hook("pre-push", [], "push") + self._install_hook("pre-commit", [], "status", hook_path_fn) + self._install_hook("pre-push", [], "push", hook_path_fn) + + if use_pre_commit_tool: + self._integrate_pre_commit_tool() + + def _integrate_pre_commit_tool(self): + if not which("pre-commit"): + raise DvcException("pre-commit is not installed") + + conf = pre_commit_tool_conf( + self._pre_commit_tool_hook_path("pre-commit"), + self._pre_commit_tool_hook_path("push"), + self._pre_commit_tool_hook_path("post-checkout"), + ) + + conf_yaml = os.path.join(self.root_dir, ".pre-commit-config.yaml") + if not os.path.isfile(conf_yaml): + check_call("pre-commit install", shell=True) + + with open(conf_yaml, "w+") as conf_yaml_f: + existing_conf = yaml.safe_load(conf_yaml_f) + conf = merge_pre_commit_tool_confs(existing_conf, conf) + yaml.dump(conf, conf_yaml_f) def cleanup_ignores(self): for path in self.ignored_paths: @@ -451,9 +409,17 @@ def close(self): def _hooks_home(self): return os.path.join(self.root_dir, self.GIT_DIR, "hooks") + @cached_property + def _pre_commit_tool_hooks_home(self): + # TODO(andrewhare): Is there a const somewhere for ".dvc/tmp"? + return os.path.join(".dvc", "tmp", "hooks") + def _hook_path(self, name): return os.path.join(self._hooks_home, name) + def _pre_commit_tool_hook_path(self, name): + return os.path.join(self._pre_commit_tool_hooks_home, name) + def _verify_hook(self, name): if os.path.exists(self._hook_path(name)): raise GitHookAlreadyExistsError(name) @@ -462,3 +428,9 @@ def _verify_dvc_hooks(self): self._verify_hook("post-checkout") self._verify_hook("pre-commit") self._verify_hook("pre-push") + + def _verify_pre_commit_tool(self): + if not which("pre-commit"): + raise DvcException("pre-commit is not installed") + + check_call("pre-commit install", shell=True) diff --git a/dvc/scm/git/pre_commit_tool.py b/dvc/scm/git/pre_commit_tool.py new file mode 100644 index 0000000000..45244b14a7 --- /dev/null +++ b/dvc/scm/git/pre_commit_tool.py @@ -0,0 +1,40 @@ +def pre_commit_tool_conf(pre_commit_path, push_path, post_checkout_path): + return { + "repos": [ + { + "repo": "local", + "hooks": [ + { + "id": "dvc-pre-commit", + "name": "DVC Pre Commit", + "entry": pre_commit_path, + "language": "script", + "stages": ["commit"], + }, + { + "id": "dvc-pre-push", + "name": "DVC Pre Push", + "entry": push_path, + "language": "script", + "stages": ["push"], + }, + { + "id": "dvc-post-checkout", + "name": "DVC Post Checkout", + "entry": post_checkout_path, + "language": "script", + "stages": ["checkout"], + }, + ], + } + ] + } + + +def merge_pre_commit_tool_confs(existing_conf, conf): + if not existing_conf or not "repos" in existing_conf: + return conf + + existing_conf["repos"].append(conf["repos"][0]) + return existing_conf + diff --git a/tests/unit/scm/test_pre_commit_tool.py b/tests/unit/scm/test_pre_commit_tool.py new file mode 100644 index 0000000000..f353c0c97f --- /dev/null +++ b/tests/unit/scm/test_pre_commit_tool.py @@ -0,0 +1,31 @@ +from dvc.scm.git.pre_commit_tool import pre_commit_tool_conf +from dvc.scm.git.pre_commit_tool import merge_pre_commit_tool_confs + +from unittest import TestCase + + +class TestPreCommitTool(TestCase): + def setUp(self): + self.conf = pre_commit_tool_conf("a", "b", "c") + + def test_merge_pre_commit_tool_confs_empty(self): + existing_conf = None + merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf) + self.assertEqual(self.conf, merged_conf) + + def test_merge_pre_commit_tool_confs_invalid_yaml(self): + existing_conf = "some invalid yaml" + merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf) + self.assertEqual(self.conf, merged_conf) + + def test_merge_pre_commit_tool_confs_no_repos(self): + existing_conf = {"foo": [1, 2, 3]} + merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf) + self.assertEqual(self.conf, merged_conf) + + def test_merge_pre_commit_tool_confs(self): + existing_conf = {"repos": [{}]} + merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf) + # Merging the new conf in should append the new repo to the end of + # the existing repos array on the existing conf. + self.assertEqual(self.conf["repos"][0], merged_conf["repos"][1]) From 6d5ec2e8e623e9c294342de6af550e03b7f39253 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 5 Mar 2020 22:50:56 +0000 Subject: [PATCH 3/3] Restyled by black --- dvc/scm/git/pre_commit_tool.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dvc/scm/git/pre_commit_tool.py b/dvc/scm/git/pre_commit_tool.py index 45244b14a7..88c3630aac 100644 --- a/dvc/scm/git/pre_commit_tool.py +++ b/dvc/scm/git/pre_commit_tool.py @@ -37,4 +37,3 @@ def merge_pre_commit_tool_confs(existing_conf, conf): existing_conf["repos"].append(conf["repos"][0]) return existing_conf -