Repository: pre-commit/pre-commit

Issue:
documention regarding adding new python based hooks needs improvement
Apparently we need some kind of how-to or mini tutorial on how to add a new hook to pre-commit as the basic documentation does not help someone without previous pre-commit knowledge.

I wanted to add support for `bashate`, a shell script linter written in python, available on pypi and that installs a shell script with the same name that can be used just like other linters.

Initially I went to https://pre-commit.com/#new-hooks which didn't give me enough info. So, i looked for other linters based on python and I found yamllint, which pointed me to https://github.com/adrienverge/yamllint/blob/master/.pre-commit-hooks.yaml

So the idea was to add the hook definition directly to the linter package. In this case I had to fork bashate in order to test the new, hook. So I ended up creating https://github.com/ssbarnea/bashate/blob/master/.pre-commit-hooks.yaml -- folowing the same model used in yamllint.

Now, I wanted to add and test the hook on one of the repos I maintain so I did create https://github.com/pycontribs/jira/blob/feature/pre-commit/.pre-commit-config.yaml#L25

When I tried to run it using `pre-commit run bashate --all`, it failed with this error:
```
Bashate..................................................................An unexpected error has occurred: OSError: [Errno 2] No such file or directory
Check the log at /Users/ssbarnea/.cache/pre-commit/pre-commit.log

An unexpected error has occurred: OSError: [Errno 2] No such file or directory
Traceback (most recent call last):
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/error_handler.py", line 47, in error_handler
    yield
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/main.py", line 258, in main
    return run(runner, args)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/commands/run.py", line 270, in run
    return _run_hooks(runner.config, repo_hooks, args, environ)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/commands/run.py", line 199, in _run_hooks
    retval |= _run_single_hook(filenames, hook, repo, args, skips, cols)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/commands/run.py", line 110, in _run_single_hook
    hook, tuple(filenames) if hook['pass_filenames'] else (),
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/repository.py", line 207, in run_hook
    return languages[language_name].run_hook(prefix, hook, file_args)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/languages/script.py", line 16, in run_hook
    return xargs(cmd, file_args)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/xargs.py", line 63, in xargs
    *run_cmd, encoding=None, retcode=None
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/site-packages/pre_commit/util.py", line 167, in cmd_output
    proc = subprocess.Popen(cmd, **popen_kwargs)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/subprocess.py", line 390, in __init__
    errread, errwrite)
  File "/Users/ssbarnea/.pyenv/versions/2.7.14/lib/python2.7/subprocess.py", line 1025, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
```

At the moment I have no idea what I did wrong, probably something simple. Still, we need to make it easier to integrate new linters into pre-commit.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 248bb43..932e4b0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,11 +1,3 @@
-1.10.3
-======
-
-### Fixes
-- Fix `pre-push` during a force push without a fetch
-    - #777 issue by @domenkozar.
-    - #778 PR by @asottile.
-
 1.10.2
 ======
 
diff --git a/pre_commit/commands/migrate_config.py b/pre_commit/commands/migrate_config.py
index b43367f..193a002 100644
--- a/pre_commit/commands/migrate_config.py
+++ b/pre_commit/commands/migrate_config.py
@@ -32,7 +32,7 @@ def _migrate_map(contents):
         # will yield a valid configuration
         try:
             trial_contents = header + 'repos:\n' + rest
-            ordered_load(trial_contents)
+            yaml.load(trial_contents)
             contents = trial_contents
         except yaml.YAMLError:
             contents = header + 'repos:\n' + _indent(rest)
diff --git a/pre_commit/parse_shebang.py b/pre_commit/parse_shebang.py
index 3332681..5a2ba72 100644
--- a/pre_commit/parse_shebang.py
+++ b/pre_commit/parse_shebang.py
@@ -42,16 +42,21 @@ def find_executable(exe, _environ=None):
         return None
 
 
-def normexe(orig_exe):
-    if os.sep not in orig_exe:
-        exe = find_executable(orig_exe)
+def normexe(orig):
+    def _error(msg):
+        raise ExecutableNotFoundError('Executable `{}` {}'.format(orig, msg))
+
+    if os.sep not in orig and (not os.altsep or os.altsep not in orig):
+        exe = find_executable(orig)
         if exe is None:
-            raise ExecutableNotFoundError(
-                'Executable `{}` not found'.format(orig_exe),
-            )
+            _error('not found')
         return exe
+    elif not os.access(orig, os.X_OK):
+        _error('not found')
+    elif os.path.isdir(orig):
+        _error('is a directory')
     else:
-        return orig_exe
+        return orig
 
 
 def normalize_cmd(cmd):
diff --git a/pre_commit/repository.py b/pre_commit/repository.py
index e78fba1..0f12bd9 100644
--- a/pre_commit/repository.py
+++ b/pre_commit/repository.py
@@ -218,7 +218,7 @@ class LocalRepository(Repository):
         else:
             return Prefix(self.store.make_local(deps))
 
-    @property
+    @cached_property
     def manifest(self):
         raise NotImplementedError
 
diff --git a/pre_commit/resources/hook-tmpl b/pre_commit/resources/hook-tmpl
index d357585..2a9657e 100755
--- a/pre_commit/resources/hook-tmpl
+++ b/pre_commit/resources/hook-tmpl
@@ -105,10 +105,6 @@ def _exe():
     )
 
 
-def _rev_exists(rev):
-    return not subprocess.call(('git', 'rev-list', '--quiet', rev))
-
-
 def _pre_push(stdin):
     remote = sys.argv[1]
 
@@ -117,7 +113,7 @@ def _pre_push(stdin):
         _, local_sha, _, remote_sha = line.split()
         if local_sha == Z40:
             continue
-        elif remote_sha != Z40 and _rev_exists(remote_sha):
+        elif remote_sha != Z40:
             opts = ('--origin', local_sha, '--source', remote_sha)
         else:
             # First ancestor not found in remote
diff --git a/pre_commit/runner.py b/pre_commit/runner.py
index c172d3f..a6d0f57 100644
--- a/pre_commit/runner.py
+++ b/pre_commit/runner.py
@@ -27,7 +27,11 @@ class Runner(object):
         os.chdir(root)
         return cls(root, config_file)
 
-    @property
+    @cached_property
+    def git_dir(self):
+        return git.get_git_dir(self.git_root)
+
+    @cached_property
     def config_file_path(self):
         return os.path.join(self.git_root, self.config_file)
 
@@ -36,4 +40,12 @@ class Runner(object):
         return load_config(self.config_file_path)
 
     def get_hook_path(self, hook_type):
-        return os.path.join(git.get_git_dir(self.git_root), 'hooks', hook_type)
+        return os.path.join(self.git_dir, 'hooks', hook_type)
+
+    @cached_property
+    def pre_commit_path(self):
+        return self.get_hook_path('pre-commit')
+
+    @cached_property
+    def pre_push_path(self):
+        return self.get_hook_path('pre-push')
diff --git a/pre_commit/store.py b/pre_commit/store.py
index 07702fb..0ca6b70 100644
--- a/pre_commit/store.py
+++ b/pre_commit/store.py
@@ -7,6 +7,8 @@ import os.path
 import sqlite3
 import tempfile
 
+from cached_property import cached_property
+
 import pre_commit.constants as C
 from pre_commit import file_lock
 from pre_commit.util import clean_path_on_failure
@@ -171,6 +173,6 @@ class Store(object):
             'local', C.LOCAL_REPO_VERSION, deps, make_local_strategy,
         )
 
-    @property
+    @cached_property
     def db_path(self):
         return os.path.join(self.directory, 'db.db')
diff --git a/setup.py b/setup.py
index b78dafe..4f0b897 100644
--- a/setup.py
+++ b/setup.py
@@ -13,7 +13,7 @@ setup(
     long_description=long_description,
     long_description_content_type='text/markdown',
     url='https://github.com/pre-commit/pre-commit',
-    version='1.10.3',
+    version='1.10.2',
     author='Anthony Sottile',
     author_email='asottile@umich.edu',
     classifiers=[


Test patch:
diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py
index 6aa9c7f..83ea38d 100644
--- a/tests/commands/install_uninstall_test.py
+++ b/tests/commands/install_uninstall_test.py
@@ -49,21 +49,21 @@ def test_install_pre_commit(tempdir_factory, store):
     path = git_dir(tempdir_factory)
     runner = Runner(path, C.CONFIG_FILE)
     assert not install(runner, store)
-    assert os.access(os.path.join(path, '.git/hooks/pre-commit'), os.X_OK)
+    assert os.access(runner.pre_commit_path, os.X_OK)
 
     assert not install(runner, store, hook_type='pre-push')
-    assert os.access(os.path.join(path, '.git/hooks/pre-push'), os.X_OK)
+    assert os.access(runner.pre_push_path, os.X_OK)
 
 
 def test_install_hooks_directory_not_present(tempdir_factory, store):
     path = git_dir(tempdir_factory)
     # Simulate some git clients which don't make .git/hooks #234
-    hooks = os.path.join(path, '.git/hooks')
+    hooks = os.path.join(path, '.git', 'hooks')
     if os.path.exists(hooks):  # pragma: no cover (latest git)
         shutil.rmtree(hooks)
     runner = Runner(path, C.CONFIG_FILE)
     install(runner, store)
-    assert os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+    assert os.path.exists(runner.pre_commit_path)
 
 
 def test_install_refuses_core_hookspath(tempdir_factory, store):
@@ -80,10 +80,10 @@ def test_install_hooks_dead_symlink(
 ):  # pragma: no cover (non-windows)
     path = git_dir(tempdir_factory)
     runner = Runner(path, C.CONFIG_FILE)
-    mkdirp(os.path.join(path, '.git/hooks'))
-    os.symlink('/fake/baz', os.path.join(path, '.git/hooks/pre-commit'))
+    mkdirp(os.path.dirname(runner.pre_commit_path))
+    os.symlink('/fake/baz', os.path.join(path, '.git', 'hooks', 'pre-commit'))
     install(runner, store)
-    assert os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+    assert os.path.exists(runner.pre_commit_path)
 
 
 def test_uninstall_does_not_blow_up_when_not_there(tempdir_factory):
@@ -96,11 +96,11 @@ def test_uninstall_does_not_blow_up_when_not_there(tempdir_factory):
 def test_uninstall(tempdir_factory, store):
     path = git_dir(tempdir_factory)
     runner = Runner(path, C.CONFIG_FILE)
-    assert not os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+    assert not os.path.exists(runner.pre_commit_path)
     install(runner, store)
-    assert os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+    assert os.path.exists(runner.pre_commit_path)
     uninstall(runner)
-    assert not os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+    assert not os.path.exists(runner.pre_commit_path)
 
 
 def _get_commit_output(tempdir_factory, touch_file='foo', **kwargs):
@@ -282,19 +282,16 @@ EXISTING_COMMIT_RUN = re.compile(
 )
 
 
-def _write_legacy_hook(path):
-    mkdirp(os.path.join(path, '.git/hooks'))
-    with io.open(os.path.join(path, '.git/hooks/pre-commit'), 'w') as f:
-        f.write('#!/usr/bin/env bash\necho "legacy hook"\n')
-    make_executable(f.name)
-
-
 def test_install_existing_hooks_no_overwrite(tempdir_factory, store):
     path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
     with cwd(path):
         runner = Runner(path, C.CONFIG_FILE)
 
-        _write_legacy_hook(path)
+        # Write out an "old" hook
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        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(tempdir_factory, touch_file='baz')
@@ -316,7 +313,11 @@ def test_install_existing_hook_no_overwrite_idempotent(tempdir_factory, store):
     with cwd(path):
         runner = Runner(path, C.CONFIG_FILE)
 
-        _write_legacy_hook(path)
+        # Write out an "old" hook
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        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, store) == 0
@@ -342,10 +343,10 @@ def test_failing_existing_hook_returns_1(tempdir_factory, store):
         runner = Runner(path, C.CONFIG_FILE)
 
         # Write out a failing "old" hook
-        mkdirp(os.path.join(path, '.git/hooks'))
-        with io.open(os.path.join(path, '.git/hooks/pre-commit'), 'w') as f:
-            f.write('#!/usr/bin/env bash\necho "fail!"\nexit 1\n')
-        make_executable(f.name)
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        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, store) == 0
 
@@ -371,7 +372,12 @@ def test_install_overwrite(tempdir_factory, store):
     with cwd(path):
         runner = Runner(path, C.CONFIG_FILE)
 
-        _write_legacy_hook(path)
+        # Write out the "old" hook
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        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, store, overwrite=True) == 0
 
         ret, output = _get_commit_output(tempdir_factory)
@@ -384,7 +390,11 @@ def test_uninstall_restores_legacy_hooks(tempdir_factory, store):
     with cwd(path):
         runner = Runner(path, C.CONFIG_FILE)
 
-        _write_legacy_hook(path)
+        # Write out an "old" hook
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        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, store) == 0
@@ -402,15 +412,17 @@ def test_replace_old_commit_script(tempdir_factory, store):
         runner = Runner(path, C.CONFIG_FILE)
 
         # Install a script that looks like our old script
-        pre_commit_contents = io.open(resource_filename('hook-tmpl')).read()
+        pre_commit_contents = io.open(
+            resource_filename('hook-tmpl'),
+        ).read()
         new_contents = pre_commit_contents.replace(
             CURRENT_HASH, PRIOR_HASHES[-1],
         )
 
-        mkdirp(os.path.join(path, '.git/hooks'))
-        with io.open(os.path.join(path, '.git/hooks/pre-commit'), 'w') as f:
-            f.write(new_contents)
-        make_executable(f.name)
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        with io.open(runner.pre_commit_path, 'w') as pre_commit_file:
+            pre_commit_file.write(new_contents)
+        make_executable(runner.pre_commit_path)
 
         # Install normally
         assert install(runner, store) == 0
@@ -424,14 +436,14 @@ def test_uninstall_doesnt_remove_not_our_hooks(tempdir_factory):
     path = git_dir(tempdir_factory)
     with cwd(path):
         runner = Runner(path, C.CONFIG_FILE)
-        mkdirp(os.path.join(path, '.git/hooks'))
-        with io.open(os.path.join(path, '.git/hooks/pre-commit'), 'w') as f:
-            f.write('#!/usr/bin/env bash\necho 1\n')
-        make_executable(f.name)
+        mkdirp(os.path.dirname(runner.pre_commit_path))
+        with io.open(runner.pre_commit_path, 'w') as pre_commit_file:
+            pre_commit_file.write('#!/usr/bin/env bash\necho 1\n')
+        make_executable(runner.pre_commit_path)
 
         assert uninstall(runner) == 0
 
-        assert os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+        assert os.path.exists(runner.pre_commit_path)
 
 
 PRE_INSTALLED = re.compile(
@@ -495,13 +507,13 @@ def test_installed_from_venv(tempdir_factory, store):
         assert NORMAL_PRE_COMMIT_RUN.match(output)
 
 
-def _get_push_output(tempdir_factory, opts=()):
+def _get_push_output(tempdir_factory):
     return cmd_output_mocked_pre_commit_home(
-        'git', 'push', 'origin', 'HEAD:new_branch', *opts,
+        'git', 'push', 'origin', 'HEAD:new_branch',
         # git push puts pre-commit to stderr
         stderr=subprocess.STDOUT,
         tempdir_factory=tempdir_factory,
-        retcode=None
+        retcode=None,
     )[:2]
 
 
@@ -535,26 +547,6 @@ def test_pre_push_integration_accepted(tempdir_factory, store):
         assert 'Passed' in output
 
 
-def test_pre_push_force_push_without_fetch(tempdir_factory, store):
-    upstream = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
-    path1 = tempdir_factory.get()
-    path2 = tempdir_factory.get()
-    cmd_output('git', 'clone', upstream, path1)
-    cmd_output('git', 'clone', upstream, path2)
-    with cwd(path1):
-        assert _get_commit_output(tempdir_factory)[0] == 0
-        assert _get_push_output(tempdir_factory)[0] == 0
-
-    with cwd(path2):
-        install(Runner(path2, C.CONFIG_FILE), store, hook_type='pre-push')
-        assert _get_commit_output(tempdir_factory, commit_msg='force!')[0] == 0
-
-        retc, output = _get_push_output(tempdir_factory, opts=('--force',))
-        assert retc == 0
-        assert 'Bash hook' in output
-        assert 'Passed' in output
-
-
 def test_pre_push_new_upstream(tempdir_factory, store):
     upstream = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
     upstream2 = git_dir(tempdir_factory)
@@ -591,16 +583,17 @@ def test_pre_push_legacy(tempdir_factory, store):
     with cwd(path):
         runner = Runner(path, C.CONFIG_FILE)
 
-        mkdirp(os.path.join(path, '.git/hooks'))
-        with io.open(os.path.join(path, '.git/hooks/pre-push'), 'w') as f:
-            f.write(
+        hook_path = runner.get_hook_path('pre-push')
+        mkdirp(os.path.dirname(hook_path))
+        with io.open(hook_path, 'w') as hook_file:
+            hook_file.write(
                 '#!/usr/bin/env bash\n'
                 'set -eu\n'
                 'read lr ls rr rs\n'
                 'test -n "$lr" -a -n "$ls" -a -n "$rr" -a -n "$rs"\n'
                 'echo legacy\n',
             )
-        make_executable(f.name)
+        make_executable(hook_path)
 
         install(runner, store, hook_type='pre-push')
         assert _get_commit_output(tempdir_factory)[0] == 0
diff --git a/tests/parse_shebang_test.py b/tests/parse_shebang_test.py
index 3f87aea..bcd6964 100644
--- a/tests/parse_shebang_test.py
+++ b/tests/parse_shebang_test.py
@@ -85,6 +85,22 @@ def test_normexe_does_not_exist():
     assert excinfo.value.args == ('Executable `i-dont-exist-lol` not found',)
 
 
+def test_normexe_does_not_exist_sep():
+    with pytest.raises(OSError) as excinfo:
+        parse_shebang.normexe('./i-dont-exist-lol')
+    assert excinfo.value.args == ('Executable `./i-dont-exist-lol` not found',)
+
+
+def test_normexe_is_a_directory(tmpdir):
+    with tmpdir.as_cwd():
+        tmpdir.join('exe').ensure_dir()
+        exe = os.path.join('.', 'exe')
+        with pytest.raises(OSError) as excinfo:
+            parse_shebang.normexe(exe)
+        msg, = excinfo.value.args
+        assert msg == 'Executable `{}` is a directory'.format(exe)
+
+
 def test_normexe_already_full_path():
     assert parse_shebang.normexe(sys.executable) == sys.executable
 
@@ -107,14 +123,14 @@ def test_normalize_cmd_PATH():
 
 
 def test_normalize_cmd_shebang(in_tmpdir):
-    python = distutils.spawn.find_executable('python')
-    path = write_executable(python.replace(os.sep, '/'))
+    python = distutils.spawn.find_executable('python').replace(os.sep, '/')
+    path = write_executable(python)
     assert parse_shebang.normalize_cmd((path,)) == (python, path)
 
 
 def test_normalize_cmd_PATH_shebang_full_path(in_tmpdir):
-    python = distutils.spawn.find_executable('python')
-    path = write_executable(python.replace(os.sep, '/'))
+    python = distutils.spawn.find_executable('python').replace(os.sep, '/')
+    path = write_executable(python)
     with bin_on_path():
         ret = parse_shebang.normalize_cmd(('run',))
         assert ret == (python, os.path.abspath(path))
diff --git a/tests/runner_test.py b/tests/runner_test.py
index 8d1c042..10b1409 100644
--- a/tests/runner_test.py
+++ b/tests/runner_test.py
@@ -5,6 +5,7 @@ import os.path
 
 import pre_commit.constants as C
 from pre_commit.runner import Runner
+from pre_commit.util import cmd_output
 from testing.fixtures import git_dir
 from testing.util import cwd
 
@@ -42,3 +43,19 @@ def test_config_file_path():
     runner = Runner(os.path.join('foo', 'bar'), C.CONFIG_FILE)
     expected_path = os.path.join('foo', 'bar', C.CONFIG_FILE)
     assert runner.config_file_path == expected_path
+
+
+def test_pre_commit_path(in_tmpdir):
+    path = os.path.join('foo', 'bar')
+    cmd_output('git', 'init', path)
+    runner = Runner(path, C.CONFIG_FILE)
+    expected_path = os.path.join(path, '.git', 'hooks', 'pre-commit')
+    assert runner.pre_commit_path == expected_path
+
+
+def test_pre_push_path(in_tmpdir):
+    path = os.path.join('foo', 'bar')
+    cmd_output('git', 'init', path)
+    runner = Runner(path, C.CONFIG_FILE)
+    expected_path = os.path.join(path, '.git', 'hooks', 'pre-push')
+    assert runner.pre_push_path == expected_path