From 377cffbd27339d101783bb58c74d533ad4e44642 Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Mon, 13 Feb 2017 14:37:03 +0100 Subject: [PATCH 1/7] Add support for python and ruby local hooks --- pre_commit/languages/docker.py | 1 + pre_commit/languages/golang.py | 1 + pre_commit/languages/node.py | 1 + pre_commit/languages/pcre.py | 1 + pre_commit/languages/python.py | 5 ++++- pre_commit/languages/ruby.py | 14 +++++++++----- pre_commit/languages/script.py | 1 + pre_commit/languages/swift.py | 1 + pre_commit/languages/system.py | 1 + pre_commit/repository.py | 1 + testing/fixtures.py | 10 +++++----- tests/repository_test.py | 28 ++++++++++++++++++++++++++++ 12 files changed, 54 insertions(+), 11 deletions(-) diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index 7d3f8d04..c4406648 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -58,6 +58,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): # pragma: windows no cover assert repo_cmd_runner.exists('Dockerfile'), ( 'No Dockerfile was found in the hook repository' diff --git a/pre_commit/languages/golang.py b/pre_commit/languages/golang.py index c0bfbcbc..d5924536 100644 --- a/pre_commit/languages/golang.py +++ b/pre_commit/languages/golang.py @@ -48,6 +48,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): helpers.assert_version_default('golang', version) directory = repo_cmd_runner.path( diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index ef557a16..6896b358 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -37,6 +37,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): # pragma: windows no cover additional_dependencies = tuple(additional_dependencies) assert repo_cmd_runner.exists('package.json') diff --git a/pre_commit/languages/pcre.py b/pre_commit/languages/pcre.py index 314ea090..39741da0 100644 --- a/pre_commit/languages/pcre.py +++ b/pre_commit/languages/pcre.py @@ -13,6 +13,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): """Installation for pcre type is a noop.""" raise AssertionError('Cannot install pcre repo.') diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py index 1e60a3ed..d3215045 100644 --- a/pre_commit/languages/python.py +++ b/pre_commit/languages/python.py @@ -58,6 +58,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): additional_dependencies = tuple(additional_dependencies) directory = helpers.environment_dir(ENVIRONMENT_DIR, version) @@ -73,10 +74,12 @@ def install_environment( else: venv_cmd.extend(['-p', os.path.realpath(sys.executable)]) repo_cmd_runner.run(venv_cmd, cwd='/') + to_install = () if is_local_hook else ('.') + to_install += additional_dependencies with in_env(repo_cmd_runner, version): helpers.run_setup_cmd( repo_cmd_runner, - ('pip', 'install', '.') + additional_dependencies, + ('pip', 'install') + to_install, ) diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py index d3896d90..3c2b36ab 100644 --- a/pre_commit/languages/ruby.py +++ b/pre_commit/languages/ruby.py @@ -100,6 +100,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): # pragma: windows no cover additional_dependencies = tuple(additional_dependencies) directory = helpers.environment_dir(ENVIRONMENT_DIR, version) @@ -115,15 +116,18 @@ def install_environment( _install_ruby(repo_cmd_runner, version) # Need to call this after installing to set up the shims helpers.run_setup_cmd(repo_cmd_runner, ('rbenv', 'rehash')) - helpers.run_setup_cmd( - repo_cmd_runner, - ('gem', 'build') + repo_cmd_runner.star('.gemspec'), - ) + if not is_local_hook: + helpers.run_setup_cmd( + repo_cmd_runner, + ('gem', 'build') + repo_cmd_runner.star('.gemspec'), + ) + to_install = () if is_local_hook else repo_cmd_runner.star('.gem') + to_install += additional_dependencies helpers.run_setup_cmd( repo_cmd_runner, ( ('gem', 'install', '--no-ri', '--no-rdoc') + - repo_cmd_runner.star('.gem') + additional_dependencies + to_install ), ) diff --git a/pre_commit/languages/script.py b/pre_commit/languages/script.py index 762ae763..151bf5df 100644 --- a/pre_commit/languages/script.py +++ b/pre_commit/languages/script.py @@ -11,6 +11,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): """Installation for script type is a noop.""" raise AssertionError('Cannot install script repo.') diff --git a/pre_commit/languages/swift.py b/pre_commit/languages/swift.py index 4d171c5b..e418a5e6 100644 --- a/pre_commit/languages/swift.py +++ b/pre_commit/languages/swift.py @@ -32,6 +32,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): # pragma: windows no cover helpers.assert_version_default('swift', version) helpers.assert_no_additional_deps('swift', additional_dependencies) diff --git a/pre_commit/languages/system.py b/pre_commit/languages/system.py index c9e1c5dc..424ba4b5 100644 --- a/pre_commit/languages/system.py +++ b/pre_commit/languages/system.py @@ -11,6 +11,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), + is_local_hook = False, ): """Installation for system type is a noop.""" raise AssertionError('Cannot install system repo.') diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 8ec6302b..56a53c86 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -179,6 +179,7 @@ class Repository(object): language.install_environment( self.cmd_runner, language_version, self.additional_dependencies[language_name][language_version], + is_local_hooks(self.repo_config), ) # Write our state to indicate we're installed write_state(venv, language_name, language_version) diff --git a/testing/fixtures.py b/testing/fixtures.py index aaa4203d..665c3c31 100644 --- a/testing/fixtures.py +++ b/testing/fixtures.py @@ -66,14 +66,14 @@ def modify_config(path='.', commit=True): cmd_output('git', 'commit', '-am', 'update config', cwd=path) -def config_with_local_hooks(): +def config_with_local_hooks(language='pcre'): return OrderedDict(( ('repo', 'local'), ('hooks', [OrderedDict(( - ('id', 'do_not_commit'), - ('name', 'Block if "DO NOT COMMIT" is found'), - ('entry', 'DO NOT COMMIT'), - ('language', 'pcre'), + ('id', language), + ('name', language), + ('entry', language), + ('language', language), ('files', '^(.*)$'), ))]) )) diff --git a/tests/repository_test.py b/tests/repository_test.py index 984973f5..663fca61 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -571,6 +571,34 @@ def test_additional_golang_dependencies_installed( assert 'hello' in binaries +@skipif_slowtests_false +@xfailif_windows_no_ruby +@pytest.mark.integration +def test_install_local_ruby_hook( + tempdir_factory, store, +): # pragma: no cover (non-windows) + config = config_with_local_hooks('ruby') + config['hooks'][0]['additional_dependencies'] = ['thread_safe'] + repo = Repository.create(config, store) + repo.require_installed() + with ruby.in_env(repo.cmd_runner, 'default'): + output = cmd_output('gem', 'list', '--local')[1] + assert 'thread_safe' in output + + +@pytest.mark.integration +def test_install_local_python_hook( + tempdir_factory, store, +): # pragma: no cover (non-windows) + config = config_with_local_hooks('python') + config['hooks'][0]['additional_dependencies'] = ['mccabe'] + repo = Repository.create(config, store) + repo.require_installed() + with python.in_env(repo.cmd_runner, 'default'): + output = cmd_output('pip', 'freeze', '-l')[1] + assert 'mccabe' in output + + def test_reinstall(tempdir_factory, store, log_info_mock): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) From 9a8fb17070a6a935a99502788146c702dfb73e2d Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Tue, 14 Feb 2017 23:47:02 +0100 Subject: [PATCH 2/7] Install local hooks in the ~/.pre-commit folder --- pre_commit/clientlib/validate_config.py | 1 + pre_commit/commands/autoupdate.py | 4 +- pre_commit/languages/docker.py | 2 +- pre_commit/languages/golang.py | 2 +- pre_commit/languages/node.py | 2 +- pre_commit/languages/pcre.py | 2 +- pre_commit/languages/python.py | 4 +- pre_commit/languages/ruby.py | 2 +- pre_commit/languages/script.py | 2 +- pre_commit/languages/swift.py | 2 +- pre_commit/languages/system.py | 2 +- pre_commit/repository.py | 22 ++------ pre_commit/runner.py | 4 +- pre_commit/store.py | 68 ++++++++++++++++++++----- tests/repository_test.py | 59 ++++++++++----------- 15 files changed, 103 insertions(+), 75 deletions(-) diff --git a/pre_commit/clientlib/validate_config.py b/pre_commit/clientlib/validate_config.py index 3624e62b..4f2caf1b 100644 --- a/pre_commit/clientlib/validate_config.py +++ b/pre_commit/clientlib/validate_config.py @@ -70,6 +70,7 @@ def validate_config_extra(config): raise InvalidConfigError( '"sha" property provided for local hooks' ) + repo['sha'] = 'local' elif 'sha' not in repo: raise InvalidConfigError( 'Missing "sha" field for repository {}'.format(repo['repo']) diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index 293c81fc..502799d9 100644 --- a/pre_commit/commands/autoupdate.py +++ b/pre_commit/commands/autoupdate.py @@ -30,7 +30,7 @@ def _update_repo(repo_config, runner, tags_only): Args: repo_config - A config for a repository """ - repo = Repository.create(repo_config, runner.store) + repo = Repository.create(repo_config, runner.store, runner.git_root) with cwd(repo.repo_path_getter.repo_path): cmd_output('git', 'fetch') @@ -51,7 +51,7 @@ def _update_repo(repo_config, runner, tags_only): # Construct a new config with the head sha new_config = OrderedDict(repo_config) new_config['sha'] = rev - new_repo = Repository.create(new_config, runner.store) + new_repo = Repository.create(new_config, runner.store, runner.git_root) # See if any of our hooks were deleted with the new commits hooks = {hook['id'] for hook in repo.repo_config['hooks']} diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index c4406648..00954577 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -58,7 +58,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): # pragma: windows no cover assert repo_cmd_runner.exists('Dockerfile'), ( 'No Dockerfile was found in the hook repository' diff --git a/pre_commit/languages/golang.py b/pre_commit/languages/golang.py index d5924536..459b453f 100644 --- a/pre_commit/languages/golang.py +++ b/pre_commit/languages/golang.py @@ -48,7 +48,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): helpers.assert_version_default('golang', version) directory = repo_cmd_runner.path( diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index 6896b358..8297213d 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -37,7 +37,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): # pragma: windows no cover additional_dependencies = tuple(additional_dependencies) assert repo_cmd_runner.exists('package.json') diff --git a/pre_commit/languages/pcre.py b/pre_commit/languages/pcre.py index 39741da0..b1b9d7cb 100644 --- a/pre_commit/languages/pcre.py +++ b/pre_commit/languages/pcre.py @@ -13,7 +13,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): """Installation for pcre type is a noop.""" raise AssertionError('Cannot install pcre repo.') diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py index d3215045..bfeb983d 100644 --- a/pre_commit/languages/python.py +++ b/pre_commit/languages/python.py @@ -58,7 +58,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): additional_dependencies = tuple(additional_dependencies) directory = helpers.environment_dir(ENVIRONMENT_DIR, version) @@ -74,7 +74,7 @@ def install_environment( else: venv_cmd.extend(['-p', os.path.realpath(sys.executable)]) repo_cmd_runner.run(venv_cmd, cwd='/') - to_install = () if is_local_hook else ('.') + to_install = () if is_local_hook else ('.',) to_install += additional_dependencies with in_env(repo_cmd_runner, version): helpers.run_setup_cmd( diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py index 3c2b36ab..54dafcdf 100644 --- a/pre_commit/languages/ruby.py +++ b/pre_commit/languages/ruby.py @@ -100,7 +100,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): # pragma: windows no cover additional_dependencies = tuple(additional_dependencies) directory = helpers.environment_dir(ENVIRONMENT_DIR, version) diff --git a/pre_commit/languages/script.py b/pre_commit/languages/script.py index 151bf5df..2fef2704 100644 --- a/pre_commit/languages/script.py +++ b/pre_commit/languages/script.py @@ -11,7 +11,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): """Installation for script type is a noop.""" raise AssertionError('Cannot install script repo.') diff --git a/pre_commit/languages/swift.py b/pre_commit/languages/swift.py index e418a5e6..eafa3a1c 100644 --- a/pre_commit/languages/swift.py +++ b/pre_commit/languages/swift.py @@ -32,7 +32,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): # pragma: windows no cover helpers.assert_version_default('swift', version) helpers.assert_no_additional_deps('swift', additional_dependencies) diff --git a/pre_commit/languages/system.py b/pre_commit/languages/system.py index 424ba4b5..c698ad51 100644 --- a/pre_commit/languages/system.py +++ b/pre_commit/languages/system.py @@ -11,7 +11,7 @@ def install_environment( repo_cmd_runner, version='default', additional_dependencies=(), - is_local_hook = False, + is_local_hook=False, ): """Installation for system type is a noop.""" raise AssertionError('Cannot install system repo.') diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 56a53c86..db384c14 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -11,7 +11,6 @@ import pkg_resources from cached_property import cached_property from pre_commit import five -from pre_commit import git from pre_commit.clientlib.validate_config import is_local_hooks from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA from pre_commit.jsonschema_extensions import apply_defaults @@ -38,13 +37,13 @@ class Repository(object): self.__installed = False @classmethod - def create(cls, config, store): + def create(cls, config, store, owner): + repo_path_getter = store.get_repo_path_getter( + config['repo'], config['sha'], owner + ) if is_local_hooks(config): - return LocalRepository(config) + return LocalRepository(config, repo_path_getter) else: - repo_path_getter = store.get_repo_path_getter( - config['repo'], config['sha'] - ) return cls(config, repo_path_getter) @cached_property @@ -198,9 +197,6 @@ class Repository(object): class LocalRepository(Repository): - def __init__(self, repo_config): - super(LocalRepository, self).__init__(repo_config, None) - @cached_property def hooks(self): return tuple( @@ -208,14 +204,6 @@ class LocalRepository(Repository): for hook in self.repo_config['hooks'] ) - @cached_property - def cmd_runner(self): - return PrefixedCommandRunner(git.get_root()) - - @cached_property - def manifest(self): - raise NotImplementedError - class _UniqueList(list): def __init__(self): diff --git a/pre_commit/runner.py b/pre_commit/runner.py index 985e6456..53545812 100644 --- a/pre_commit/runner.py +++ b/pre_commit/runner.py @@ -41,7 +41,9 @@ class Runner(object): def repositories(self): """Returns a tuple of the configured repositories.""" config = load_config(self.config_file_path) - repositories = tuple(Repository.create(x, self.store) for x in config) + repositories = tuple( + Repository.create(x, self.store, self.git_root) for x in config + ) for repository in repositories: repository.require_installed() return repositories diff --git a/pre_commit/store.py b/pre_commit/store.py index aecc7dc1..ec948310 100644 --- a/pre_commit/store.py +++ b/pre_commit/store.py @@ -9,6 +9,7 @@ import tempfile from cached_property import cached_property +from pre_commit.clientlib.validate_config import _LOCAL_HOOKS_MAGIC_REPO_STRING from pre_commit.prefixed_command_runner import PrefixedCommandRunner from pre_commit.util import clean_path_on_failure from pre_commit.util import cmd_output @@ -45,6 +46,17 @@ class Store(object): def repo_path(self): return self._store.clone(self._repo, self._ref) + class LocalRepoPathGetter(RepoPathGetter): + def __init__(self, repo, sha, store, owner): + super(Store.LocalRepoPathGetter, self).__init__( + repo, sha, store, + ) + self._owner = owner + + @cached_property + def repo_path(self): + return self._store.initialize_local_repo(self._owner) + def __init__(self, directory=None): if directory is None: directory = self.get_default_directory() @@ -97,19 +109,31 @@ class Store(object): self._create() self.__created = True - def clone(self, url, ref): - """Clone the given url and checkout the specific ref.""" - self.require_created() - - # Check if we already exist + def find_local_path(self, repo, ref): with sqlite3.connect(self.db_path) as db: result = db.execute( 'SELECT path FROM repos WHERE repo = ? AND ref = ?', - [url, ref], + [repo, ref], ).fetchone() if result: return result[0] + def store_path(self, repo, ref, path): + with sqlite3.connect(self.db_path) as db: + db.execute( + 'INSERT INTO repos (repo, ref, path) VALUES (?, ?, ?)', + [repo, ref, path], + ) + + def clone(self, url, sha): + """Clone the given url and checkout the specific sha.""" + self.require_created() + + # Check if we already exist + local_path = self.find_local_path(url, sha) + if local_path: + return local_path + logger.info('Initializing environment for {}.'.format(url)) dir = tempfile.mkdtemp(prefix='repo', dir=self.directory) @@ -121,15 +145,33 @@ class Store(object): cmd_output('git', 'reset', ref, '--hard', env=no_git_env()) # Update our db with the created repo - with sqlite3.connect(self.db_path) as db: - db.execute( - 'INSERT INTO repos (repo, ref, path) VALUES (?, ?, ?)', - [url, ref, dir], - ) + self.store_path(url, sha, dir) return dir - def get_repo_path_getter(self, repo, ref): - return self.RepoPathGetter(repo, ref, self) + def initialize_local_repo(self, owner): + """Initializes a repo in the default repository for the local repo""" + self.require_created() + local_path = self.find_local_path( + _LOCAL_HOOKS_MAGIC_REPO_STRING, owner, + ) + if local_path: + return local_path + + logger.info('Initializing environment for {}.'.format( + _LOCAL_HOOKS_MAGIC_REPO_STRING, + )) + + dir = tempfile.mkdtemp(prefix='repo', dir=self.directory) + + # Update our db with the created repo + self.store_path(_LOCAL_HOOKS_MAGIC_REPO_STRING, owner, dir) + return dir + + def get_repo_path_getter(self, repo, sha, owner=None): + if sha == _LOCAL_HOOKS_MAGIC_REPO_STRING: + return self.LocalRepoPathGetter(repo, sha, self, owner) + else: + return self.RepoPathGetter(repo, sha, self) @cached_property def cmd_runner(self): diff --git a/tests/repository_test.py b/tests/repository_test.py index 663fca61..3ac4cf70 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -50,11 +50,12 @@ def _test_hook_repo( ): path = make_repo(tempdir_factory, repo_path) config = make_config_from_repo(path, **(config_kwargs or {})) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) hook_dict = [ hook for repo_hook_id, hook in repo.hooks if repo_hook_id == hook_id ][0] ret = repo.run_hook(hook_dict, args) + print ret assert ret[0] == expected_return_code assert ret[1].replace(b'\r\n', b'\n') == expected @@ -108,7 +109,7 @@ def test_switch_language_versions_doesnt_clobber(tempdir_factory, store): config = make_config_from_repo( path, hooks=[{'id': 'python3-hook', 'language_version': version}], ) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) hook_dict, = [ hook for repo_hook_id, hook in repo.hooks @@ -462,7 +463,7 @@ def test_repo_url(mock_repo_config): def test_languages(tempdir_factory, store): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) assert repo.languages == {('python', 'default')} @@ -471,7 +472,7 @@ def test_additional_dependencies(tempdir_factory, store): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = ['pep8'] - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) assert repo.additional_dependencies['python']['default'] == ['pep8'] @@ -483,7 +484,7 @@ def test_additional_dependencies_duplicated( config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = [ 'thread_safe', 'tins', 'thread_safe'] - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) assert repo.additional_dependencies['ruby']['default'] == [ 'thread_safe', 'tins'] @@ -493,7 +494,7 @@ def test_additional_python_dependencies_installed(tempdir_factory, store): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = ['mccabe'] - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() with python.in_env(repo.cmd_runner, 'default'): output = cmd_output('pip', 'freeze', '-l')[1] @@ -505,11 +506,11 @@ def test_additional_dependencies_roll_forward(tempdir_factory, store): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) # Run the repo once without additional_dependencies - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() # Now run it with additional_dependencies config['hooks'][0]['additional_dependencies'] = ['mccabe'] - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() # We should see our additional dependency installed with python.in_env(repo.cmd_runner, 'default'): @@ -526,7 +527,7 @@ def test_additional_ruby_dependencies_installed( path = make_repo(tempdir_factory, 'ruby_hooks_repo') config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = ['thread_safe', 'tins'] - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() with ruby.in_env(repo.cmd_runner, 'default'): output = cmd_output('gem', 'list', '--local')[1] @@ -544,7 +545,7 @@ def test_additional_node_dependencies_installed( config = make_config_from_repo(path) # Careful to choose a small package that's not depped by npm config['hooks'][0]['additional_dependencies'] = ['lodash'] - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() with node.in_env(repo.cmd_runner, 'default'): cmd_output('npm', 'config', 'set', 'global', 'true') @@ -561,7 +562,7 @@ def test_additional_golang_dependencies_installed( # A small go package deps = ['github.com/golang/example/hello'] config['hooks'][0]['additional_dependencies'] = deps - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() binaries = os.listdir(repo.cmd_runner.path( helpers.environment_dir(golang.ENVIRONMENT_DIR, 'default'), 'bin', @@ -579,7 +580,8 @@ def test_install_local_ruby_hook( ): # pragma: no cover (non-windows) config = config_with_local_hooks('ruby') config['hooks'][0]['additional_dependencies'] = ['thread_safe'] - repo = Repository.create(config, store) + validate_config_extra([config]) + repo = Repository.create(config, store, '/path/to/repo/') repo.require_installed() with ruby.in_env(repo.cmd_runner, 'default'): output = cmd_output('gem', 'list', '--local')[1] @@ -592,7 +594,8 @@ def test_install_local_python_hook( ): # pragma: no cover (non-windows) config = config_with_local_hooks('python') config['hooks'][0]['additional_dependencies'] = ['mccabe'] - repo = Repository.create(config, store) + validate_config_extra([config]) + repo = Repository.create(config, store, '/path/to/repo/') repo.require_installed() with python.in_env(repo.cmd_runner, 'default'): output = cmd_output('pip', 'freeze', '-l')[1] @@ -602,7 +605,7 @@ def test_install_local_python_hook( def test_reinstall(tempdir_factory, store, log_info_mock): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() # We print some logging during clone (1) + install (3) assert log_info_mock.call_count == 4 @@ -611,7 +614,7 @@ def test_reinstall(tempdir_factory, store, log_info_mock): repo.require_installed() assert log_info_mock.call_count == 0 # Reinstall on another run should not trigger another install - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() assert log_info_mock.call_count == 0 @@ -620,7 +623,7 @@ def test_control_c_control_c_on_install(tempdir_factory, store): """Regression test for #186.""" path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) hook = repo.hooks[0][1] class MyKeyboardInterrupt(KeyboardInterrupt): @@ -657,7 +660,7 @@ def test_really_long_file_paths(tempdir_factory, store): config = make_config_from_repo(path) with cwd(really_long_path): - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) repo.require_installed() @@ -666,11 +669,11 @@ def test_config_overrides_repo_specifics(tempdir_factory, store): path = make_repo(tempdir_factory, 'script_hooks_repo') config = make_config_from_repo(path) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) assert repo.hooks[0][1]['files'] == '' # Set the file regex to something else config['hooks'][0]['files'] = '\\.sh$' - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) assert repo.hooks[0][1]['files'] == '\\.sh$' @@ -690,28 +693,20 @@ def test_tags_on_repositories(in_tmpdir, tempdir_factory, store): ) repo_1 = Repository.create( - make_config_from_repo(git_dir_1, sha=tag), store, + make_config_from_repo(git_dir_1, sha=tag), store, git_dir_1 ) ret = repo_1.run_hook(repo_1.hooks[0][1], ['-L']) assert ret[0] == 0 assert ret[1].strip() == _norm_pwd(in_tmpdir) repo_2 = Repository.create( - make_config_from_repo(git_dir_2, sha=tag), store, + make_config_from_repo(git_dir_2, sha=tag), store, git_dir_2 ) ret = repo_2.run_hook(repo_2.hooks[0][1], ['bar']) assert ret[0] == 0 assert ret[1] == b'bar\nHello World\n' -def test_local_repository(): - config = config_with_local_hooks() - local_repo = Repository.create(config, 'dummy') - with pytest.raises(NotImplementedError): - local_repo.manifest - assert len(local_repo.hooks) == 1 - - @pytest.yield_fixture def fake_log_handler(): handler = mock.Mock(level=logging.INFO) @@ -725,7 +720,7 @@ def test_hook_id_not_present(tempdir_factory, store, fake_log_handler): path = make_repo(tempdir_factory, 'script_hooks_repo') config = make_config_from_repo(path) config['hooks'][0]['id'] = 'i-dont-exist' - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) with pytest.raises(SystemExit): repo.install() assert fake_log_handler.handle.call_args[0][0].msg == ( @@ -740,7 +735,7 @@ def test_too_new_version(tempdir_factory, store, fake_log_handler): with modify_manifest(path) as manifest: manifest[0]['minimum_pre_commit_version'] = '999.0.0' config = make_config_from_repo(path) - repo = Repository.create(config, store) + repo = Repository.create(config, store, path) with pytest.raises(SystemExit): repo.install() msg = fake_log_handler.handle.call_args[0][0].msg @@ -762,4 +757,4 @@ def test_versions_ok(tempdir_factory, store, version): manifest[0]['minimum_pre_commit_version'] = version config = make_config_from_repo(path) # Should succeed - Repository.create(config, store).install() + Repository.create(config, store, path).install() From e21cd01a706681d5fb9daee0333cbbba4980aa85 Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Wed, 15 Feb 2017 00:37:44 +0100 Subject: [PATCH 3/7] Fix tests --- pre_commit/clientlib/validate_config.py | 1 - pre_commit/repository.py | 6 +++++- tests/languages/all_test.py | 9 +++++++-- tests/repository_test.py | 2 -- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/pre_commit/clientlib/validate_config.py b/pre_commit/clientlib/validate_config.py index 4f2caf1b..3624e62b 100644 --- a/pre_commit/clientlib/validate_config.py +++ b/pre_commit/clientlib/validate_config.py @@ -70,7 +70,6 @@ def validate_config_extra(config): raise InvalidConfigError( '"sha" property provided for local hooks' ) - repo['sha'] = 'local' elif 'sha' not in repo: raise InvalidConfigError( 'Missing "sha" field for repository {}'.format(repo['repo']) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index db384c14..165961f9 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -11,6 +11,7 @@ import pkg_resources from cached_property import cached_property from pre_commit import five +from pre_commit.clientlib.validate_config import _LOCAL_HOOKS_MAGIC_REPO_STRING from pre_commit.clientlib.validate_config import is_local_hooks from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA from pre_commit.jsonschema_extensions import apply_defaults @@ -39,7 +40,10 @@ class Repository(object): @classmethod def create(cls, config, store, owner): repo_path_getter = store.get_repo_path_getter( - config['repo'], config['sha'], owner + config['repo'], + _LOCAL_HOOKS_MAGIC_REPO_STRING if + is_local_hooks(config) else config['sha'], + owner, ) if is_local_hooks(config): return LocalRepository(config, repo_path_getter) diff --git a/tests/languages/all_test.py b/tests/languages/all_test.py index 73b89cb5..7a7af5cb 100644 --- a/tests/languages/all_test.py +++ b/tests/languages/all_test.py @@ -11,10 +11,15 @@ from pre_commit.languages.all import languages @pytest.mark.parametrize('language', all_languages) def test_install_environment_argspec(language): expected_argspec = inspect.ArgSpec( - args=['repo_cmd_runner', 'version', 'additional_dependencies'], + args=[ + 'repo_cmd_runner', + 'version', + 'additional_dependencies', + 'is_local_hook', + ], varargs=None, keywords=None, - defaults=('default', ()), + defaults=('default', (), False), ) argspec = inspect.getargspec(languages[language].install_environment) assert argspec == expected_argspec diff --git a/tests/repository_test.py b/tests/repository_test.py index 3ac4cf70..5b0fc278 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -580,7 +580,6 @@ def test_install_local_ruby_hook( ): # pragma: no cover (non-windows) config = config_with_local_hooks('ruby') config['hooks'][0]['additional_dependencies'] = ['thread_safe'] - validate_config_extra([config]) repo = Repository.create(config, store, '/path/to/repo/') repo.require_installed() with ruby.in_env(repo.cmd_runner, 'default'): @@ -594,7 +593,6 @@ def test_install_local_python_hook( ): # pragma: no cover (non-windows) config = config_with_local_hooks('python') config['hooks'][0]['additional_dependencies'] = ['mccabe'] - validate_config_extra([config]) repo = Repository.create(config, store, '/path/to/repo/') repo.require_installed() with python.in_env(repo.cmd_runner, 'default'): From 6cb92e067c766583fa4aaa8db71613f4f636875e Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Wed, 15 Feb 2017 00:43:10 +0100 Subject: [PATCH 4/7] Remove print statement --- tests/repository_test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/repository_test.py b/tests/repository_test.py index 5b0fc278..29ab9983 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -55,7 +55,6 @@ def _test_hook_repo( hook for repo_hook_id, hook in repo.hooks if repo_hook_id == hook_id ][0] ret = repo.run_hook(hook_dict, args) - print ret assert ret[0] == expected_return_code assert ret[1].replace(b'\r\n', b'\n') == expected From 3c4d67367487fd27d53a9ce484db6b48b8d459b6 Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Wed, 15 Feb 2017 10:53:41 +0100 Subject: [PATCH 5/7] Clean up --- pre_commit/repository.py | 5 +-- pre_commit/store.py | 88 ++++++++++++---------------------------- tests/store_test.py | 14 +++---- 3 files changed, 34 insertions(+), 73 deletions(-) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 165961f9..7605d837 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -11,7 +11,6 @@ import pkg_resources from cached_property import cached_property from pre_commit import five -from pre_commit.clientlib.validate_config import _LOCAL_HOOKS_MAGIC_REPO_STRING from pre_commit.clientlib.validate_config import is_local_hooks from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA from pre_commit.jsonschema_extensions import apply_defaults @@ -41,9 +40,7 @@ class Repository(object): def create(cls, config, store, owner): repo_path_getter = store.get_repo_path_getter( config['repo'], - _LOCAL_HOOKS_MAGIC_REPO_STRING if - is_local_hooks(config) else config['sha'], - owner, + owner if is_local_hooks(config) else config['sha'], ) if is_local_hooks(config): return LocalRepository(config, repo_path_getter) diff --git a/pre_commit/store.py b/pre_commit/store.py index ec948310..5f767346 100644 --- a/pre_commit/store.py +++ b/pre_commit/store.py @@ -44,18 +44,7 @@ class Store(object): @cached_property def repo_path(self): - return self._store.clone(self._repo, self._ref) - - class LocalRepoPathGetter(RepoPathGetter): - def __init__(self, repo, sha, store, owner): - super(Store.LocalRepoPathGetter, self).__init__( - repo, sha, store, - ) - self._owner = owner - - @cached_property - def repo_path(self): - return self._store.initialize_local_repo(self._owner) + return self._store.initialize_repo(self._repo, self._sha) def __init__(self, directory=None): if directory is None: @@ -109,69 +98,44 @@ class Store(object): self._create() self.__created = True - def find_local_path(self, repo, ref): + def initialize_repo(self, repo, sha): + """Initializes the repository by cloning a remote or preparing an + empty local folder for local hooks. If the repo if 'local', + the sha used is the path to git_root of this repo so that several + local hooks for different projects are separated.""" + self.require_created() + + # Check if we already exist with sqlite3.connect(self.db_path) as db: result = db.execute( 'SELECT path FROM repos WHERE repo = ? AND ref = ?', - [repo, ref], + [repo, sha], ).fetchone() if result: return result[0] - def store_path(self, repo, ref, path): + logger.info('Initializing environment for {}.'.format(repo)) + + dir = tempfile.mkdtemp(prefix='repo', dir=self.directory) + if repo != _LOCAL_HOOKS_MAGIC_REPO_STRING: + with clean_path_on_failure(dir): + cmd_output( + 'git', 'clone', '--no-checkout', repo, dir, + env=no_git_env(), + ) + with cwd(dir): + cmd_output('git', 'reset', sha, '--hard', env=no_git_env()) + + # Update our db with the created repo with sqlite3.connect(self.db_path) as db: db.execute( 'INSERT INTO repos (repo, ref, path) VALUES (?, ?, ?)', - [repo, ref, path], + [repo, sha, dir], ) - - def clone(self, url, sha): - """Clone the given url and checkout the specific sha.""" - self.require_created() - - # Check if we already exist - local_path = self.find_local_path(url, sha) - if local_path: - return local_path - - logger.info('Initializing environment for {}.'.format(url)) - - dir = tempfile.mkdtemp(prefix='repo', dir=self.directory) - with clean_path_on_failure(dir): - cmd_output( - 'git', 'clone', '--no-checkout', url, dir, env=no_git_env(), - ) - with cwd(dir): - cmd_output('git', 'reset', ref, '--hard', env=no_git_env()) - - # Update our db with the created repo - self.store_path(url, sha, dir) return dir - def initialize_local_repo(self, owner): - """Initializes a repo in the default repository for the local repo""" - self.require_created() - local_path = self.find_local_path( - _LOCAL_HOOKS_MAGIC_REPO_STRING, owner, - ) - if local_path: - return local_path - - logger.info('Initializing environment for {}.'.format( - _LOCAL_HOOKS_MAGIC_REPO_STRING, - )) - - dir = tempfile.mkdtemp(prefix='repo', dir=self.directory) - - # Update our db with the created repo - self.store_path(_LOCAL_HOOKS_MAGIC_REPO_STRING, owner, dir) - return dir - - def get_repo_path_getter(self, repo, sha, owner=None): - if sha == _LOCAL_HOOKS_MAGIC_REPO_STRING: - return self.LocalRepoPathGetter(repo, sha, self, owner) - else: - return self.RepoPathGetter(repo, sha, self) + def get_repo_path_getter(self, repo, sha): + return self.RepoPathGetter(repo, sha, self) @cached_property def cmd_runner(self): diff --git a/tests/store_test.py b/tests/store_test.py index 950693cf..bba241a7 100644 --- a/tests/store_test.py +++ b/tests/store_test.py @@ -79,14 +79,14 @@ def test_does_not_recreate_if_directory_already_exists(store): assert not os.path.exists(os.path.join(store.directory, 'README')) -def test_clone(store, tempdir_factory, log_info_mock): +def test_initialize_repo(store, tempdir_factory, log_info_mock): path = git_dir(tempdir_factory) with cwd(path): cmd_output('git', 'commit', '--allow-empty', '-m', 'foo') sha = get_head_sha(path) cmd_output('git', 'commit', '--allow-empty', '-m', 'bar') - ret = store.clone(path, sha) + ret = store.initialize_repo(path, sha) # Should have printed some stuff assert log_info_mock.call_args_list[0][0][0].startswith( 'Initializing environment for ' @@ -98,7 +98,7 @@ def test_clone(store, tempdir_factory, log_info_mock): # Directory should start with `repo` _, dirname = os.path.split(ret) assert dirname.startswith('repo') - # Should be checked out to the sha we specified + # Should be checked out to the sha we specifieds assert get_head_sha(ret) == sha # Assert there's an entry in the sqlite db for this @@ -110,11 +110,11 @@ def test_clone(store, tempdir_factory, log_info_mock): assert path == ret -def test_clone_cleans_up_on_checkout_failure(store): +def test_initialize_repo_cleans_up_on_checkout_failure(store): try: # This raises an exception because you can't clone something that # doesn't exist! - store.clone('/i_dont_exist_lol', 'fake_sha') + store.initialize_repo('/i_dont_exist_lol', 'fake_sha') except Exception as e: assert '/i_dont_exist_lol' in five.text(e) @@ -130,7 +130,7 @@ def test_has_cmd_runner_at_directory(store): assert ret.prefix_dir == store.directory + os.sep -def test_clone_when_repo_already_exists(store): +def test_initialize_repo_when_repo_already_exists(store): # Create an entry in the sqlite db that makes it look like the repo has # been cloned. store.require_created() @@ -141,7 +141,7 @@ def test_clone_when_repo_already_exists(store): 'VALUES ("fake_repo", "fake_ref", "fake_path")' ) - assert store.clone('fake_repo', 'fake_ref') == 'fake_path' + assert store.initialize_repo('fake_repo', 'fake_ref') == 'fake_path' def test_require_created_when_directory_exists_but_not_db(store): From fbdd8406922a77b2bcaad40a1eae80a8601b8ab6 Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Wed, 15 Feb 2017 11:31:48 +0100 Subject: [PATCH 6/7] Use a proper runner for local hooks --- pre_commit/repository.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 7605d837..596d7471 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -11,6 +11,7 @@ import pkg_resources from cached_property import cached_property from pre_commit import five +from pre_commit import git from pre_commit.clientlib.validate_config import is_local_hooks from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA from pre_commit.jsonschema_extensions import apply_defaults @@ -104,6 +105,16 @@ class Repository(object): def cmd_runner(self): return PrefixedCommandRunner(self.repo_path_getter.repo_path) + @cached_property + def local_cmd_runner(self): + return self.cmd_runner + + def language_cmd_runner(self, language): + if language in ['script', 'system']: + return self.local_cmd_runner + else: + return self.cmd_runner + def require_installed(self): if self.__installed: return @@ -193,7 +204,7 @@ class Repository(object): """ self.require_installed() return languages[hook['language']].run_hook( - self.cmd_runner, hook, file_args, + self.language_cmd_runner(hook['language']), hook, file_args, ) @@ -205,6 +216,10 @@ class LocalRepository(Repository): for hook in self.repo_config['hooks'] ) + @cached_property + def local_cmd_runner(self): + return PrefixedCommandRunner(git.get_root()) + class _UniqueList(list): def __init__(self): From 4a2c93658a73f30f00f3bc38361c56141fc4bb45 Mon Sep 17 00:00:00 2001 From: Thierry Deo Date: Wed, 15 Feb 2017 22:27:29 +0100 Subject: [PATCH 7/7] Fix after mistake rebasing --- pre_commit/store.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pre_commit/store.py b/pre_commit/store.py index 5f767346..a205a84a 100644 --- a/pre_commit/store.py +++ b/pre_commit/store.py @@ -44,7 +44,7 @@ class Store(object): @cached_property def repo_path(self): - return self._store.initialize_repo(self._repo, self._sha) + return self._store.initialize_repo(self._repo, self._ref) def __init__(self, directory=None): if directory is None: