From eaaf935b3895fd3283f64ce480f08cdcfa38fa0a Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 17 Sep 2019 15:12:24 +0200 Subject: [PATCH 1/4] install: verify no hooks colliding with dvc exist --- dvc/exceptions.py | 10 ++++++++++ dvc/scm/git/__init__.py | 23 ++++++++++++++++++++++- tests/func/test_install.py | 8 ++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 0b992249e0..8bc9d7c7c6 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -294,3 +294,13 @@ def __init__(self): "the given path is a DVC-file, you must specify a data file " "or a directory" ) + + +class GitHookAlreadyExistsError(DvcException): + def __init__(self, hook_name): + super(GitHookAlreadyExistsError, self).__init__( + "Hook '{}' already exists. Please refer to " + "https://man.dvc.org/install " + "for more info." + "".format(hook_name) + ) diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index 2b9a679385..55bef9beb7 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -5,6 +5,9 @@ import os import logging +from funcy import cached_property + +from dvc.exceptions import GitHookAlreadyExistsError from dvc.utils.compat import str, open from dvc.utils import fix_env, relpath from dvc.scm.base import ( @@ -234,7 +237,7 @@ def _install_hook(self, name, cmd): " || exec dvc {}".format(cmd) ) - hook = os.path.join(self.root_dir, self.GIT_DIR, "hooks", name) + hook = self._hook_path(name) if os.path.isfile(hook): with open(hook, "r+") as fobj: @@ -247,6 +250,8 @@ def _install_hook(self, name, cmd): os.chmod(hook, 0o777) def install(self): + self._verify_dvc_hooks() + self._install_hook("post-checkout", "checkout") self._install_hook("pre-commit", "status") self._install_hook("pre-push", "push") @@ -342,3 +347,19 @@ def get_rev(self): def close(self): self.repo.close() + + @cached_property + def _hooks_home(self): + return os.path.join(self.root_dir, self.GIT_DIR, "hooks") + + def _hook_path(self, name): + return os.path.join(self._hooks_home, name) + + def _verify_hook(self, name): + if os.path.exists(self._hook_path(name)): + raise GitHookAlreadyExistsError(name) + + def _verify_dvc_hooks(self): + self._verify_hook("post-checkout") + self._verify_hook("pre-commit") + self._verify_hook("pre-push") diff --git a/tests/func/test_install.py b/tests/func/test_install.py index b4e07f4b86..d9d838313c 100644 --- a/tests/func/test_install.py +++ b/tests/func/test_install.py @@ -30,6 +30,13 @@ def test_should_create_hooks(self, git, dvc_repo): with open(self._hook(fname), "r") as fobj: assert command in fobj.read() + def test_should_fail_if_file_already_exists(self, git, dvc_repo): + with open(self._hook("post-checkout"), "w") as fobj: + fobj.write("hook content") + + assert main(["install"]) != 0 + + @pytest.mark.skip(reason="https://github.com/iterative/dvc/issues/2362") def test_should_append_hooks_if_file_already_exists(self, git, dvc_repo): with open(self._hook("post-checkout"), "w") as fobj: fobj.write("#!/bin/sh\n" "echo hello\n") @@ -47,6 +54,7 @@ def test_should_append_hooks_if_file_already_exists(self, git, dvc_repo): with open(self._hook("post-checkout"), "r") as fobj: assert fobj.read() == expected_script + @pytest.mark.skip(reason="https://github.com/iterative/dvc/issues/2362") def test_should_be_idempotent(self, git, dvc_repo): assert main(["install"]) == 0 assert main(["install"]) == 0 From 2625e094d5f57d2d0bfe4c51e3db4f5f01695874 Mon Sep 17 00:00:00 2001 From: pawel Date: Wed, 18 Sep 2019 17:34:53 +0200 Subject: [PATCH 2/4] test: install: remove obsolete tests --- tests/func/test_install.py | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/tests/func/test_install.py b/tests/func/test_install.py index d9d838313c..c6f21f74b3 100644 --- a/tests/func/test_install.py +++ b/tests/func/test_install.py @@ -36,39 +36,6 @@ def test_should_fail_if_file_already_exists(self, git, dvc_repo): assert main(["install"]) != 0 - @pytest.mark.skip(reason="https://github.com/iterative/dvc/issues/2362") - def test_should_append_hooks_if_file_already_exists(self, git, dvc_repo): - with open(self._hook("post-checkout"), "w") as fobj: - fobj.write("#!/bin/sh\n" "echo hello\n") - - assert main(["install"]) == 0 - - expected_script = ( - "#!/bin/sh\n" - "echo hello\n" - '[ "$3" = "0" ]' - ' || [ -z "$(git ls-files .dvc)" ]' - " || exec dvc checkout\n" - ) - - with open(self._hook("post-checkout"), "r") as fobj: - assert fobj.read() == expected_script - - @pytest.mark.skip(reason="https://github.com/iterative/dvc/issues/2362") - def test_should_be_idempotent(self, git, dvc_repo): - assert main(["install"]) == 0 - assert main(["install"]) == 0 - - expected_script = ( - "#!/bin/sh\n" - '[ "$3" = "0" ]' - ' || [ -z "$(git ls-files .dvc)" ]' - " || exec dvc checkout\n" - ) - - with open(self._hook("post-checkout"), "r") as fobj: - assert fobj.read() == expected_script - def test_should_post_checkout_hook_checkout(self, repo_dir, git, dvc_repo): assert main(["install"]) == 0 From 2e1e18630eef3ab6c0b4b5233d9e48b81767be5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Redzy=C5=84ski?= Date: Thu, 19 Sep 2019 08:50:39 +0200 Subject: [PATCH 3/4] Update tests/func/test_install.py Co-Authored-By: Ruslan Kuprieiev --- tests/func/test_install.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/func/test_install.py b/tests/func/test_install.py index c6f21f74b3..c2a1034ba2 100644 --- a/tests/func/test_install.py +++ b/tests/func/test_install.py @@ -34,7 +34,8 @@ def test_should_fail_if_file_already_exists(self, git, dvc_repo): with open(self._hook("post-checkout"), "w") as fobj: fobj.write("hook content") - assert main(["install"]) != 0 + with pytest.raises(GitHookAlreadyExistsError): + dvc_repo.scm.install() def test_should_post_checkout_hook_checkout(self, repo_dir, git, dvc_repo): assert main(["install"]) == 0 From 9ac1af8bfd0b6b8ce5f56082b9047eba677eae42 Mon Sep 17 00:00:00 2001 From: pawel Date: Thu, 19 Sep 2019 13:27:24 +0200 Subject: [PATCH 4/4] test: install: import GitHookAlreadyExistsError --- dvc/exceptions.py | 3 +-- tests/func/test_install.py | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 8bc9d7c7c6..f7b6739a33 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -301,6 +301,5 @@ def __init__(self, hook_name): super(GitHookAlreadyExistsError, self).__init__( "Hook '{}' already exists. Please refer to " "https://man.dvc.org/install " - "for more info." - "".format(hook_name) + "for more info.".format(hook_name) ) diff --git a/tests/func/test_install.py b/tests/func/test_install.py index c2a1034ba2..73afdf28cd 100644 --- a/tests/func/test_install.py +++ b/tests/func/test_install.py @@ -2,6 +2,8 @@ import sys import pytest + +from dvc.exceptions import GitHookAlreadyExistsError from dvc.utils import file_md5 from dvc.main import main