From 4b43fd8cdc1b7885bf975247ad195747c628e94f Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 08:20:47 -0700 Subject: [PATCH 1/8] Add integration test for existing install behaviour --- pre_commit/store.py | 5 +++- testing/fixtures.py | 3 +++ tests/commands/install_test.py | 45 ++++++++++++++++++++++++++++++++++ tests/store_test.py | 8 ++++++ 4 files changed, 60 insertions(+), 1 deletion(-) diff --git a/pre_commit/store.py b/pre_commit/store.py index c35bcbae..c4942919 100644 --- a/pre_commit/store.py +++ b/pre_commit/store.py @@ -22,7 +22,10 @@ def _get_default_directory(): `Store.get_default_directory` can be mocked in tests and `_get_default_directory` can be tested. """ - return os.path.join(os.environ['HOME'], '.pre-commit') + return os.environ.get( + 'PRE_COMMIT_HOME', + os.path.join(os.environ['HOME'], '.pre-commit'), + ) class Store(object): diff --git a/testing/fixtures.py b/testing/fixtures.py index 2fa5f535..3bf18273 100644 --- a/testing/fixtures.py +++ b/testing/fixtures.py @@ -66,4 +66,7 @@ def make_consuming_repo(tmpdir_factory, repo_source): config = make_config_from_repo(path) git_path = git_dir(tmpdir_factory) write_config(git_path, config) + with local.cwd(git_path): + git('add', C.CONFIG_FILE) + git('commit', '-m', 'Add hooks config') return git_path diff --git a/tests/commands/install_test.py b/tests/commands/install_test.py index 53965e6e..aa6bd8b9 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_test.py @@ -4,12 +4,38 @@ from __future__ import unicode_literals import io import os import os.path +import re import pkg_resources +import subprocess import stat +from plumbum import local from pre_commit.commands.install import install from pre_commit.runner import Runner from testing.fixtures import git_dir +from testing.fixtures import make_consuming_repo + + +def _get_commit_output(tmpdir_factory): + # Don't want to write to home directory + env = dict(os.environ, **{'PRE_COMMIT_HOME': tmpdir_factory.get()}) + return local['git']( + 'commit', '-m', 'Commit!', '--allow-empty', + # git commit puts pre-commit to stderr + stderr=subprocess.STDOUT, + env=env, + ) + + +NORMAL_PRE_COMMIT_RUN = re.compile( + r'^\[INFO\] Installing environment for .+.\n' + r'\[INFO\] Once installed this environment will be reused.\n' + r'\[INFO\] This may take a few minutes...\n' + r'Bash hook' + r'\.+' + r'\(no files to check\) Skipped\n' + r'\[master [a-f0-9]{7}\] Commit!\n$' +) def test_install_pre_commit(tmpdir_factory): @@ -26,3 +52,22 @@ def test_install_pre_commit(tmpdir_factory): assert pre_commit_contents == expected_contents stat_result = os.stat(runner.pre_commit_path) assert stat_result.st_mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + + +def test_install_pre_commit_and_run(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + assert install(Runner(path)) == 0 + + output = _get_commit_output(tmpdir_factory) + assert NORMAL_PRE_COMMIT_RUN.match(output) + + +def test_install_idempotent(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + assert install(Runner(path)) == 0 + assert install(Runner(path)) == 0 + + output = _get_commit_output(tmpdir_factory) + assert NORMAL_PRE_COMMIT_RUN.match(output) diff --git a/tests/store_test.py b/tests/store_test.py index 5ebfe05d..bc8cee6e 100644 --- a/tests/store_test.py +++ b/tests/store_test.py @@ -31,6 +31,14 @@ def test_get_default_directory_defaults_to_home(): assert ret == os.path.join(os.environ['HOME'], '.pre-commit') +def test_uses_environment_variable_when_present(): + with mock.patch.dict( + os.environ, {'PRE_COMMIT_HOME': '/tmp/pre_commit_home'} + ): + ret = _get_default_directory() + assert ret == '/tmp/pre_commit_home' + + def test_store_require_created(store): assert not os.path.exists(store.directory) store.require_created() From 39470c98caa07e92c1098633bd4bd48fac59f98d Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 13:03:00 -0700 Subject: [PATCH 2/8] Add expected behaviour for failure to find pre_commit. --- tests/commands/install_test.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/commands/install_test.py b/tests/commands/install_test.py index aa6bd8b9..965d5803 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_test.py @@ -71,3 +71,21 @@ def test_install_idempotent(tmpdir_factory): output = _get_commit_output(tmpdir_factory) assert NORMAL_PRE_COMMIT_RUN.match(output) + + +def test_environment_not_sourced(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + assert install(Runner(path)) == 0 + + ret, stdout, stderr = local['git'].run( + ['commit', '--allow-empty', '-m', 'foo'], + env={}, + retcode=None, + ) + assert ret == 1 + assert stdout == '' + assert stderr == ( + '`pre-commit` not found. ' + 'Did you forget to activate your virtualenv?\n' + ) From a744a6484e982e6597103f605ac2ba7756cd8fb5 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 13:07:54 -0700 Subject: [PATCH 3/8] git mv pre_commit/resources/pre-commit.sh pre_commit/resources/pre-commit-hook --- pre_commit/commands/install.py | 2 +- pre_commit/resources/{pre-commit.sh => pre-commit-hook} | 0 setup.py | 2 +- tests/commands/install_test.py | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename pre_commit/resources/{pre-commit.sh => pre-commit-hook} (100%) diff --git a/pre_commit/commands/install.py b/pre_commit/commands/install.py index 9630f577..d208ff7f 100644 --- a/pre_commit/commands/install.py +++ b/pre_commit/commands/install.py @@ -9,7 +9,7 @@ import stat def install(runner): """Install the pre-commit hooks.""" pre_commit_file = pkg_resources.resource_filename( - 'pre_commit', 'resources/pre-commit.sh', + 'pre_commit', 'resources/pre-commit-hook', ) with open(runner.pre_commit_path, 'w') as pre_commit_file_obj: pre_commit_file_obj.write(open(pre_commit_file).read()) diff --git a/pre_commit/resources/pre-commit.sh b/pre_commit/resources/pre-commit-hook similarity index 100% rename from pre_commit/resources/pre-commit.sh rename to pre_commit/resources/pre-commit-hook diff --git a/setup.py b/setup.py index fb2d7099..83ff2005 100644 --- a/setup.py +++ b/setup.py @@ -27,7 +27,7 @@ setup( packages=find_packages('.', exclude=('tests*', 'testing*')), package_data={ 'pre_commit': [ - 'resources/pre-commit.sh' + 'resources/pre-commit-hook' ] }, install_requires=[ diff --git a/tests/commands/install_test.py b/tests/commands/install_test.py index 965d5803..1aacb1c1 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_test.py @@ -46,7 +46,7 @@ def test_install_pre_commit(tmpdir_factory): assert os.path.exists(runner.pre_commit_path) pre_commit_contents = io.open(runner.pre_commit_path).read() pre_commit_sh = pkg_resources.resource_filename( - 'pre_commit', 'resources/pre-commit.sh', + 'pre_commit', 'resources/pre-commit-hook', ) expected_contents = io.open(pre_commit_sh).read() assert pre_commit_contents == expected_contents From 8f3f5c364ae554b359f8cbd3d7908992f33c260c Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 14:27:30 -0700 Subject: [PATCH 4/8] Add test for failure condition. --- tests/commands/install_test.py | 51 ++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/tests/commands/install_test.py b/tests/commands/install_test.py index 1aacb1c1..d87356e6 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_test.py @@ -17,24 +17,39 @@ from testing.fixtures import make_consuming_repo def _get_commit_output(tmpdir_factory): + local['touch']('foo') + local['git']('add', 'foo') # Don't want to write to home directory env = dict(os.environ, **{'PRE_COMMIT_HOME': tmpdir_factory.get()}) - return local['git']( - 'commit', '-m', 'Commit!', '--allow-empty', + return local['git'].run( + ['commit', '-m', 'Commit!', '--allow-empty'], # git commit puts pre-commit to stderr stderr=subprocess.STDOUT, env=env, - ) + retcode=None, + )[:2] NORMAL_PRE_COMMIT_RUN = re.compile( - r'^\[INFO\] Installing environment for .+.\n' - r'\[INFO\] Once installed this environment will be reused.\n' - r'\[INFO\] This may take a few minutes...\n' - r'Bash hook' - r'\.+' - r'\(no files to check\) Skipped\n' - r'\[master [a-f0-9]{7}\] Commit!\n$' + r'^\[INFO\] Installing environment for .+\.\n' + r'\[INFO\] Once installed this environment will be reused\.\n' + r'\[INFO\] This may take a few minutes\.\.\.\n' + r'Bash hook\.+Passed\n' + r'\[master [a-f0-9]{7}\] Commit!\n' + r' 0 files changed\n' + r' create mode 100644 foo\n$' +) + + +FAILING_PRE_COMMIT_RUN = re.compile( + r'^\[INFO\] Installing environment for .+\.\n' + r'\[INFO\] Once installed this environment will be reused\.\n' + r'\[INFO\] This may take a few minutes\.\.\.\n' + r'Failing hook\.+Failed\n' + r'\n' + r'Fail\n' + r'foo\n' + r'\n$' ) @@ -59,7 +74,8 @@ def test_install_pre_commit_and_run(tmpdir_factory): with local.cwd(path): assert install(Runner(path)) == 0 - output = _get_commit_output(tmpdir_factory) + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 0 assert NORMAL_PRE_COMMIT_RUN.match(output) @@ -69,7 +85,8 @@ def test_install_idempotent(tmpdir_factory): assert install(Runner(path)) == 0 assert install(Runner(path)) == 0 - output = _get_commit_output(tmpdir_factory) + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 0 assert NORMAL_PRE_COMMIT_RUN.match(output) @@ -89,3 +106,13 @@ def test_environment_not_sourced(tmpdir_factory): '`pre-commit` not found. ' 'Did you forget to activate your virtualenv?\n' ) + + +def test_failing_hooks_returns_nonzero(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'failing_hook_repo') + with local.cwd(path): + assert install(Runner(path)) == 0 + + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 1 + assert FAILING_PRE_COMMIT_RUN.match(output) From 1d8394afd01b62997cd2cec3192a22ceff0c2c12 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 15:31:22 -0700 Subject: [PATCH 5/8] Add first pass at migration mode. --- pre_commit/commands/install.py | 32 +++++-- pre_commit/resources/pre-commit-hook | 22 +++++ tests/commands/install_test.py | 136 +++++++++++++++++++++------ 3 files changed, 153 insertions(+), 37 deletions(-) diff --git a/pre_commit/commands/install.py b/pre_commit/commands/install.py index d208ff7f..606cfd25 100644 --- a/pre_commit/commands/install.py +++ b/pre_commit/commands/install.py @@ -1,24 +1,44 @@ from __future__ import print_function from __future__ import unicode_literals +import io import os +import os.path import pkg_resources import stat +# This is used to identify the hook file we install +IDENTIFYING_HASH = 'd8ee923c46731b42cd95cc869add4062' + + +def is_our_pre_commit(filename): + return IDENTIFYING_HASH in io.open(filename).read() + + +def make_executable(filename): + original_mode = os.stat(filename).st_mode + os.chmod( + filename, + original_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH, + ) + def install(runner): """Install the pre-commit hooks.""" pre_commit_file = pkg_resources.resource_filename( 'pre_commit', 'resources/pre-commit-hook', ) + + # If we have an existing hook, move it to pre-commit.legacy + if ( + os.path.exists(runner.pre_commit_path) and + not is_our_pre_commit(runner.pre_commit_path) + ): + os.rename(runner.pre_commit_path, runner.pre_commit_path + '.legacy') + with open(runner.pre_commit_path, 'w') as pre_commit_file_obj: pre_commit_file_obj.write(open(pre_commit_file).read()) - - original_mode = os.stat(runner.pre_commit_path).st_mode - os.chmod( - runner.pre_commit_path, - original_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH, - ) + make_executable(runner.pre_commit_path) print('pre-commit installed at {0}'.format(runner.pre_commit_path)) return 0 diff --git a/pre_commit/resources/pre-commit-hook b/pre_commit/resources/pre-commit-hook index 1a93b653..60c620cf 100755 --- a/pre_commit/resources/pre-commit-hook +++ b/pre_commit/resources/pre-commit-hook @@ -1,4 +1,10 @@ #!/usr/bin/env bash +# This is a randomish md5 to identify this script +# d8ee923c46731b42cd95cc869add4062 + +HERE=$(dirname $(readlink -f "$0")) + +retv=0 which pre-commit > /dev/null if [ $? -ne 0 ]; then @@ -6,4 +12,20 @@ if [ $? -ne 0 ]; then exit 1 fi + +# Run the legacy pre-commit if it exists +if [ -x "$HERE"/pre-commit.legacy ]; then + "$HERE"/pre-commit.legacy + if [ $? -ne 0 ]; then + retv=1 + fi +fi + + +# Run pre-commit pre-commit +if [ $? -ne 0 ]; then + retv=1 +fi + +exit $retv diff --git a/tests/commands/install_test.py b/tests/commands/install_test.py index d87356e6..00bca826 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_test.py @@ -11,14 +11,44 @@ import stat from plumbum import local from pre_commit.commands.install import install +from pre_commit.commands.install import is_our_pre_commit +from pre_commit.commands.install import make_executable from pre_commit.runner import Runner from testing.fixtures import git_dir from testing.fixtures import make_consuming_repo -def _get_commit_output(tmpdir_factory): - local['touch']('foo') - local['git']('add', 'foo') +def test_is_not_our_pre_commit(): + assert is_our_pre_commit('setup.py') is False + + +def test_is_our_pre_commit(): + assert is_our_pre_commit( + pkg_resources.resource_filename( + 'pre_commit', 'resources/pre-commit-hook', + ) + ) is True + + +def test_install_pre_commit(tmpdir_factory): + path = git_dir(tmpdir_factory) + runner = Runner(path) + ret = install(runner) + assert ret == 0 + assert os.path.exists(runner.pre_commit_path) + pre_commit_contents = io.open(runner.pre_commit_path).read() + pre_commit_script = pkg_resources.resource_filename( + 'pre_commit', 'resources/pre-commit-hook', + ) + expected_contents = io.open(pre_commit_script).read() + assert pre_commit_contents == expected_contents + stat_result = os.stat(runner.pre_commit_path) + assert stat_result.st_mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + + +def _get_commit_output(tmpdir_factory, touch_file='foo'): + local['touch'](touch_file) + local['git']('add', touch_file) # Don't want to write to home directory env = dict(os.environ, **{'PRE_COMMIT_HOME': tmpdir_factory.get()}) return local['git'].run( @@ -41,34 +71,6 @@ NORMAL_PRE_COMMIT_RUN = re.compile( ) -FAILING_PRE_COMMIT_RUN = re.compile( - r'^\[INFO\] Installing environment for .+\.\n' - r'\[INFO\] Once installed this environment will be reused\.\n' - r'\[INFO\] This may take a few minutes\.\.\.\n' - r'Failing hook\.+Failed\n' - r'\n' - r'Fail\n' - r'foo\n' - r'\n$' -) - - -def test_install_pre_commit(tmpdir_factory): - path = git_dir(tmpdir_factory) - runner = Runner(path) - ret = install(runner) - assert ret == 0 - assert os.path.exists(runner.pre_commit_path) - pre_commit_contents = io.open(runner.pre_commit_path).read() - pre_commit_sh = pkg_resources.resource_filename( - 'pre_commit', 'resources/pre-commit-hook', - ) - expected_contents = io.open(pre_commit_sh).read() - assert pre_commit_contents == expected_contents - stat_result = os.stat(runner.pre_commit_path) - assert stat_result.st_mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) - - def test_install_pre_commit_and_run(tmpdir_factory): path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') with local.cwd(path): @@ -108,6 +110,18 @@ def test_environment_not_sourced(tmpdir_factory): ) +FAILING_PRE_COMMIT_RUN = re.compile( + r'^\[INFO\] Installing environment for .+\.\n' + r'\[INFO\] Once installed this environment will be reused\.\n' + r'\[INFO\] This may take a few minutes\.\.\.\n' + r'Failing hook\.+Failed\n' + r'\n' + r'Fail\n' + r'foo\n' + r'\n$' +) + + def test_failing_hooks_returns_nonzero(tmpdir_factory): path = make_consuming_repo(tmpdir_factory, 'failing_hook_repo') with local.cwd(path): @@ -116,3 +130,63 @@ def test_failing_hooks_returns_nonzero(tmpdir_factory): ret, output = _get_commit_output(tmpdir_factory) assert ret == 1 assert FAILING_PRE_COMMIT_RUN.match(output) + + +EXISTING_COMMIT_RUN = re.compile( + r'^legacy hook\n' + r'\[master [a-f0-9]{7}\] Commit!\n' + r' 0 files changed\n' + r' create mode 100644 baz\n$' +) + + +def test_install_existing_hooks_no_overwrite(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + runner = Runner(path) + + # Write out an "old" hook + with io.open(runner.pre_commit_path, 'w') as hook_file: + hook_file.write('#!/usr/bin/env bash\necho "legacy hook"\n') + make_executable(runner.pre_commit_path) + + # Make sure we installed the "old" hook correctly + ret, output = _get_commit_output(tmpdir_factory, touch_file='baz') + assert ret == 0 + assert EXISTING_COMMIT_RUN.match(output) + + # Now install pre-commit (no-overwrite) + assert install(Runner(path)) == 0 + + # We should run both the legacy and pre-commit hooks + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 0 + assert output.startswith('legacy hook\n') + assert NORMAL_PRE_COMMIT_RUN.match(output[len('legacy hook\n'):]) + + +FAIL_OLD_HOOK = re.compile( + r'fail!\n' + r'\[INFO\] Installing environment for .+\.\n' + r'\[INFO\] Once installed this environment will be reused\.\n' + r'\[INFO\] This may take a few minutes\.\.\.\n' + r'Bash hook\.+Passed\n' +) + + +def test_failing_existing_hook_returns_1(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + runner = Runner(path) + + # Write out a failing "old" hook + with io.open(runner.pre_commit_path, 'w') as hook_file: + hook_file.write('#!/usr/bin/env bash\necho "fail!"\nexit 1\n') + make_executable(runner.pre_commit_path) + + assert install(Runner(path)) == 0 + + # We should get a failure from the legacy hook + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 1 + assert FAIL_OLD_HOOK.match(output) From ac735e85e28a9af1622aac7841dc1fbf719ceafd Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 17:33:25 -0700 Subject: [PATCH 6/8] Make install -f / --overwrite work. --- pre_commit/commands/install.py | 16 +++++++++-- pre_commit/main.py | 10 +++++-- pre_commit/runner.py | 9 +++++- tests/commands/install_test.py | 52 ++++++++++++++++++++++++++++++++-- 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/pre_commit/commands/install.py b/pre_commit/commands/install.py index 606cfd25..242d2e83 100644 --- a/pre_commit/commands/install.py +++ b/pre_commit/commands/install.py @@ -7,6 +7,7 @@ import os.path import pkg_resources import stat + # This is used to identify the hook file we install IDENTIFYING_HASH = 'd8ee923c46731b42cd95cc869add4062' @@ -23,7 +24,7 @@ def make_executable(filename): ) -def install(runner): +def install(runner, overwrite=False): """Install the pre-commit hooks.""" pre_commit_file = pkg_resources.resource_filename( 'pre_commit', 'resources/pre-commit-hook', @@ -34,7 +35,18 @@ def install(runner): os.path.exists(runner.pre_commit_path) and not is_our_pre_commit(runner.pre_commit_path) ): - os.rename(runner.pre_commit_path, runner.pre_commit_path + '.legacy') + os.rename(runner.pre_commit_path, runner.pre_commit_legacy_path) + + # If we specify overwrite, we simply delete the legacy file + if overwrite and os.path.exists(runner.pre_commit_legacy_path): + os.remove(runner.pre_commit_legacy_path) + elif os.path.exists(runner.pre_commit_legacy_path): + print( + 'Running in migration mode with existing hooks at {0}\n' + 'Use -f to use only pre-commit.'.format( + runner.pre_commit_legacy_path, + ) + ) with open(runner.pre_commit_path, 'w') as pre_commit_file_obj: pre_commit_file_obj.write(open(pre_commit_file).read()) diff --git a/pre_commit/main.py b/pre_commit/main.py index e1e8ff73..af8f2a13 100644 --- a/pre_commit/main.py +++ b/pre_commit/main.py @@ -28,7 +28,13 @@ def main(argv): subparsers = parser.add_subparsers(dest='command') - subparsers.add_parser('install', help='Intall the pre-commit script.') + install_parser = subparsers.add_parser( + 'install', help='Intall the pre-commit script.', + ) + install_parser.add_argument( + '-f', '--overwrite', action='store_true', + help='Overwrite existing hooks / remove migration mode.', + ) subparsers.add_parser('uninstall', help='Uninstall the pre-commit script.') @@ -67,7 +73,7 @@ def main(argv): runner = Runner.create() if args.command == 'install': - return install(runner) + return install(runner, overwrite=args.overwrite) elif args.command == 'uninstall': return uninstall(runner) elif args.command == 'clean': diff --git a/pre_commit/runner.py b/pre_commit/runner.py index 1768a336..bbba62c7 100644 --- a/pre_commit/runner.py +++ b/pre_commit/runner.py @@ -41,7 +41,14 @@ class Runner(object): @cached_property def pre_commit_path(self): - return os.path.join(self.git_root, '.git/hooks/pre-commit') + return os.path.join(self.git_root, '.git', 'hooks', 'pre-commit') + + @cached_property + def pre_commit_legacy_path(self): + """The path in the 'hooks' directory representing the temporary + storage for existing pre-commit hooks. + """ + return self.pre_commit_path + '.legacy' @cached_property def cmd_runner(self): diff --git a/tests/commands/install_test.py b/tests/commands/install_test.py index 00bca826..44842774 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_test.py @@ -156,7 +156,28 @@ def test_install_existing_hooks_no_overwrite(tmpdir_factory): assert EXISTING_COMMIT_RUN.match(output) # Now install pre-commit (no-overwrite) - assert install(Runner(path)) == 0 + assert install(runner) == 0 + + # We should run both the legacy and pre-commit hooks + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 0 + assert output.startswith('legacy hook\n') + assert NORMAL_PRE_COMMIT_RUN.match(output[len('legacy hook\n'):]) + + +def test_install_existing_hook_no_overwrite_idempotent(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + runner = Runner(path) + + # Write out an "old" hook + with io.open(runner.pre_commit_path, 'w') as hook_file: + hook_file.write('#!/usr/bin/env bash\necho "legacy hook"\n') + make_executable(runner.pre_commit_path) + + # Install twice + assert install(runner) == 0 + assert install(runner) == 0 # We should run both the legacy and pre-commit hooks ret, output = _get_commit_output(tmpdir_factory) @@ -184,9 +205,36 @@ def test_failing_existing_hook_returns_1(tmpdir_factory): hook_file.write('#!/usr/bin/env bash\necho "fail!"\nexit 1\n') make_executable(runner.pre_commit_path) - assert install(Runner(path)) == 0 + assert install(runner) == 0 # We should get a failure from the legacy hook ret, output = _get_commit_output(tmpdir_factory) assert ret == 1 assert FAIL_OLD_HOOK.match(output) + + +def test_install_overwrite_no_existing_hooks(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + assert install(Runner(path), overwrite=True) == 0 + + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 0 + assert NORMAL_PRE_COMMIT_RUN.match(output) + + +def test_install_overwrite(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + runner = Runner(path) + + # Write out the "old" hook + with io.open(runner.pre_commit_path, 'w') as hook_file: + hook_file.write('#!/usr/bin/env bash\necho "legacy hook"\n') + make_executable(runner.pre_commit_path) + + assert install(runner, overwrite=True) == 0 + + ret, output = _get_commit_output(tmpdir_factory) + assert ret == 0 + assert NORMAL_PRE_COMMIT_RUN.match(output) From f4d16b9cdc74fb0043e43b7ade505261330fbef9 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 17:44:48 -0700 Subject: [PATCH 7/8] Combine install and uninstall. --- .../{install.py => install_uninstall.py} | 8 ++++++ pre_commit/commands/uninstall.py | 13 ---------- pre_commit/main.py | 4 +-- ...tall_test.py => install_uninstall_test.py} | 24 ++++++++++++++--- tests/commands/uninstall_test.py | 26 ------------------- 5 files changed, 31 insertions(+), 44 deletions(-) rename pre_commit/commands/{install.py => install_uninstall.py} (88%) delete mode 100644 pre_commit/commands/uninstall.py rename tests/commands/{install_test.py => install_uninstall_test.py} (91%) delete mode 100644 tests/commands/uninstall_test.py diff --git a/pre_commit/commands/install.py b/pre_commit/commands/install_uninstall.py similarity index 88% rename from pre_commit/commands/install.py rename to pre_commit/commands/install_uninstall.py index 242d2e83..abc6c8ad 100644 --- a/pre_commit/commands/install.py +++ b/pre_commit/commands/install_uninstall.py @@ -54,3 +54,11 @@ def install(runner, overwrite=False): print('pre-commit installed at {0}'.format(runner.pre_commit_path)) return 0 + + +def uninstall(runner): + """Uninstall the pre-commit hooks.""" + if os.path.exists(runner.pre_commit_path): + os.remove(runner.pre_commit_path) + print('pre-commit uninstalled') + return 0 diff --git a/pre_commit/commands/uninstall.py b/pre_commit/commands/uninstall.py deleted file mode 100644 index 52e0dca3..00000000 --- a/pre_commit/commands/uninstall.py +++ /dev/null @@ -1,13 +0,0 @@ -from __future__ import print_function -from __future__ import unicode_literals - -import os -import os.path - - -def uninstall(runner): - """Uninstall the pre-commit hooks.""" - if os.path.exists(runner.pre_commit_path): - os.remove(runner.pre_commit_path) - print('pre-commit uninstalled') - return 0 diff --git a/pre_commit/main.py b/pre_commit/main.py index af8f2a13..eb678a90 100644 --- a/pre_commit/main.py +++ b/pre_commit/main.py @@ -6,9 +6,9 @@ import pkg_resources from pre_commit import color from pre_commit.commands.autoupdate import autoupdate from pre_commit.commands.clean import clean -from pre_commit.commands.install import install +from pre_commit.commands.install_uninstall import install +from pre_commit.commands.install_uninstall import uninstall from pre_commit.commands.run import run -from pre_commit.commands.uninstall import uninstall from pre_commit.runner import Runner from pre_commit.util import entry diff --git a/tests/commands/install_test.py b/tests/commands/install_uninstall_test.py similarity index 91% rename from tests/commands/install_test.py rename to tests/commands/install_uninstall_test.py index 44842774..bd0780ac 100644 --- a/tests/commands/install_test.py +++ b/tests/commands/install_uninstall_test.py @@ -10,9 +10,10 @@ import subprocess import stat from plumbum import local -from pre_commit.commands.install import install -from pre_commit.commands.install import is_our_pre_commit -from pre_commit.commands.install import make_executable +from pre_commit.commands.install_uninstall import install +from pre_commit.commands.install_uninstall import is_our_pre_commit +from pre_commit.commands.install_uninstall import make_executable +from pre_commit.commands.install_uninstall import uninstall from pre_commit.runner import Runner from testing.fixtures import git_dir from testing.fixtures import make_consuming_repo @@ -46,6 +47,23 @@ def test_install_pre_commit(tmpdir_factory): assert stat_result.st_mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) +def test_uninstall_does_not_blow_up_when_not_there(tmpdir_factory): + path = git_dir(tmpdir_factory) + runner = Runner(path) + ret = uninstall(runner) + assert ret == 0 + + +def test_uninstall(tmpdir_factory): + path = git_dir(tmpdir_factory) + runner = Runner(path) + assert not os.path.exists(runner.pre_commit_path) + install(runner) + assert os.path.exists(runner.pre_commit_path) + uninstall(runner) + assert not os.path.exists(runner.pre_commit_path) + + def _get_commit_output(tmpdir_factory, touch_file='foo'): local['touch'](touch_file) local['git']('add', touch_file) diff --git a/tests/commands/uninstall_test.py b/tests/commands/uninstall_test.py deleted file mode 100644 index 9d5a38ed..00000000 --- a/tests/commands/uninstall_test.py +++ /dev/null @@ -1,26 +0,0 @@ -from __future__ import absolute_import -from __future__ import unicode_literals - -import os.path - -from pre_commit.runner import Runner -from pre_commit.commands.install import install -from pre_commit.commands.uninstall import uninstall -from testing.fixtures import git_dir - - -def test_uninstall_does_not_blow_up_when_not_there(tmpdir_factory): - path = git_dir(tmpdir_factory) - runner = Runner(path) - ret = uninstall(runner) - assert ret == 0 - - -def test_uninstall(tmpdir_factory): - path = git_dir(tmpdir_factory) - runner = Runner(path) - assert not os.path.exists(runner.pre_commit_path) - install(runner) - assert os.path.exists(runner.pre_commit_path) - uninstall(runner) - assert not os.path.exists(runner.pre_commit_path) From 0cde0fdc480a5579645c8a32fce89fbad57d0a1a Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 16 Jun 2014 17:48:56 -0700 Subject: [PATCH 8/8] Uninstall restores hooks. --- pre_commit/commands/install_uninstall.py | 5 +++++ tests/commands/install_uninstall_test.py | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pre_commit/commands/install_uninstall.py b/pre_commit/commands/install_uninstall.py index abc6c8ad..4698ee1e 100644 --- a/pre_commit/commands/install_uninstall.py +++ b/pre_commit/commands/install_uninstall.py @@ -61,4 +61,9 @@ def uninstall(runner): if os.path.exists(runner.pre_commit_path): os.remove(runner.pre_commit_path) print('pre-commit uninstalled') + + if os.path.exists(runner.pre_commit_legacy_path): + os.rename(runner.pre_commit_legacy_path, runner.pre_commit_path) + print('Restored previous hooks to {0}'.format(runner.pre_commit_path)) + return 0 diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py index bd0780ac..4832afbb 100644 --- a/tests/commands/install_uninstall_test.py +++ b/tests/commands/install_uninstall_test.py @@ -256,3 +256,23 @@ def test_install_overwrite(tmpdir_factory): ret, output = _get_commit_output(tmpdir_factory) assert ret == 0 assert NORMAL_PRE_COMMIT_RUN.match(output) + + +def test_uninstall_restores_legacy_hooks(tmpdir_factory): + path = make_consuming_repo(tmpdir_factory, 'script_hooks_repo') + with local.cwd(path): + runner = Runner(path) + + # Write out an "old" hook + with io.open(runner.pre_commit_path, 'w') as hook_file: + hook_file.write('#!/usr/bin/env bash\necho "legacy hook"\n') + make_executable(runner.pre_commit_path) + + # Now install and uninstall pre-commit + assert install(runner) == 0 + assert uninstall(runner) == 0 + + # Make sure we installed the "old" hook correctly + ret, output = _get_commit_output(tmpdir_factory, touch_file='baz') + assert ret == 0 + assert EXISTING_COMMIT_RUN.match(output)