Repository: pre-commit/pre-commit
Pull request #497 (2017-02-22 02:32:01)

Make hook-tmpl resilient to future changes
I realized this wasn't necessarily rollback safe.  This makes it slightly more future-proof.

This feature really isn't used too much anyway (we usually run install with `--force`)

Perhaps the default behaviour of install should be changed as well?

asottile: Actually, I think this is mostly unnecessary.  We're idempotently writing the file so we don't suffer from the install problem.  The template can change freely and will still be properly reinstalled.

I'm going to scale this back just to have the simplifications and not need the new generated hashes

Patch:
diff --git a/pre_commit/commands/install_uninstall.py b/pre_commit/commands/install_uninstall.py
index 069fb9d..13621bf 100644
--- a/pre_commit/commands/install_uninstall.py
+++ b/pre_commit/commands/install_uninstall.py
@@ -12,29 +12,21 @@ from pre_commit.util import resource_filename
 
 
 # This is used to identify the hook file we install
-PREVIOUS_IDENTIFYING_HASHES = (
+PRIOR_HASHES = (
     '4d9958c90bc262f47553e2c073f14cfe',
     'd8ee923c46731b42cd95cc869add4062',
     '49fd668cb42069aa1b6048464be5d395',
     '79f09a650522a87b0da915d0d983b2de',
     'e358c9dae00eac5d06b38dfdb1e33a8c',
 )
+CURRENT_HASH = '138fd403232d2ddd5efb44317e38bf03'
 
 
-IDENTIFYING_HASH = '138fd403232d2ddd5efb44317e38bf03'
-
-
-def is_our_pre_commit(filename):
-    if not os.path.exists(filename):
-        return False
-    return IDENTIFYING_HASH in io.open(filename).read()
-
-
-def is_previous_pre_commit(filename):
+def is_our_script(filename):
     if not os.path.exists(filename):
         return False
     contents = io.open(filename).read()
-    return any(hash in contents for hash in PREVIOUS_IDENTIFYING_HASHES)
+    return any(h in contents for h in (CURRENT_HASH,) + PRIOR_HASHES)
 
 
 def install(runner, overwrite=False, hooks=False, hook_type='pre-commit'):
@@ -45,11 +37,7 @@ def install(runner, overwrite=False, hooks=False, hook_type='pre-commit'):
     mkdirp(os.path.dirname(hook_path))
 
     # If we have an existing hook, move it to pre-commit.legacy
-    if (
-        os.path.lexists(hook_path) and
-        not is_our_pre_commit(hook_path) and
-        not is_previous_pre_commit(hook_path)
-    ):
+    if os.path.lexists(hook_path) and not is_our_script(hook_path):
         os.rename(hook_path, legacy_path)
 
     # If we specify overwrite, we simply delete the legacy file
@@ -97,12 +85,7 @@ def uninstall(runner, hook_type='pre-commit'):
     hook_path = runner.get_hook_path(hook_type)
     legacy_path = hook_path + '.legacy'
     # If our file doesn't exist or it isn't ours, gtfo.
-    if (
-        not os.path.exists(hook_path) or (
-            not is_our_pre_commit(hook_path) and
-            not is_previous_pre_commit(hook_path)
-        )
-    ):
+    if not os.path.exists(hook_path) or not is_our_script(hook_path):
         return 0
 
     os.remove(hook_path)
diff --git a/requirements-dev.txt b/requirements-dev.txt
index 3455541..e181222 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -4,6 +4,7 @@ coverage
 flake8
 mock
 pytest
+pytest-env
 
 # setuptools breaks pypy3 with extraneous output
 setuptools<18.5
diff --git a/tox.ini b/tox.ini
index 805e293..104ce6b 100644
--- a/tox.ini
+++ b/tox.ini
@@ -6,12 +6,6 @@ envlist = py27,py34,py35,pypy
 [testenv]
 deps = -rrequirements-dev.txt
 passenv = GOROOT HOME HOMEPATH PROGRAMDATA TERM
-setenv =
-    VIRTUALENV_NO_DOWNLOAD = 1
-    GIT_AUTHOR_NAME = "test"
-    GIT_COMMITTER_NAME = "test"
-    GIT_AUTHOR_EMAIL = "test@example.com"
-    GIT_COMMITTER_EMAIL = "test@example.com"
 commands =
     coverage erase
     coverage run -m pytest {posargs:tests}
@@ -25,3 +19,11 @@ commands =
 
 [pep8]
 ignore = E265,E309,E501
+
+[pytest]
+env =
+    GIT_AUTHOR_NAME=test
+    GIT_COMMITTER_NAME=test
+    GIT_AUTHOR_EMAIL=test@example.com
+    GIT_COMMITTER_EMAIL=test@example.com
+    VIRTUALENV_NO_DOWNLOAD=1


Test patch:
diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py
index 9136a0c..0467f74 100644
--- a/tests/commands/install_uninstall_test.py
+++ b/tests/commands/install_uninstall_test.py
@@ -11,12 +11,11 @@ import sys
 import mock
 
 import pre_commit.constants as C
-from pre_commit.commands.install_uninstall import IDENTIFYING_HASH
+from pre_commit.commands.install_uninstall import CURRENT_HASH
 from pre_commit.commands.install_uninstall import install
 from pre_commit.commands.install_uninstall import install_hooks
-from pre_commit.commands.install_uninstall import is_our_pre_commit
-from pre_commit.commands.install_uninstall import is_previous_pre_commit
-from pre_commit.commands.install_uninstall import PREVIOUS_IDENTIFYING_HASHES
+from pre_commit.commands.install_uninstall import is_our_script
+from pre_commit.commands.install_uninstall import PRIOR_HASHES
 from pre_commit.commands.install_uninstall import uninstall
 from pre_commit.runner import Runner
 from pre_commit.util import cmd_output
@@ -30,27 +29,18 @@ from testing.util import cmd_output_mocked_pre_commit_home
 from testing.util import xfailif_no_symlink
 
 
-def test_is_not_our_pre_commit():
-    assert is_our_pre_commit('setup.py') is False
+def test_is_not_script():
+    assert is_our_script('setup.py') is False
 
 
-def test_is_our_pre_commit():
-    assert is_our_pre_commit(resource_filename('hook-tmpl'))
+def test_is_script():
+    assert is_our_script(resource_filename('hook-tmpl'))
 
 
-def test_is_not_previous_pre_commit():
-    assert is_previous_pre_commit('setup.py') is False
-
-
-def test_is_also_not_previous_pre_commit():
-    assert not is_previous_pre_commit(resource_filename('hook-tmpl'))
-
-
-def test_is_previous_pre_commit(in_tmpdir):
-    with io.open('foo', 'w') as foo_file:
-        foo_file.write(PREVIOUS_IDENTIFYING_HASHES[0])
-
-    assert is_previous_pre_commit('foo')
+def test_is_previous_pre_commit(tmpdir):
+    f = tmpdir.join('foo')
+    f.write(PRIOR_HASHES[0] + '\n')
+    assert is_our_script(f.strpath)
 
 
 def test_install_pre_commit(tempdir_factory):
@@ -408,7 +398,7 @@ def test_replace_old_commit_script(tempdir_factory):
             resource_filename('hook-tmpl'),
         ).read()
         new_contents = pre_commit_contents.replace(
-            IDENTIFYING_HASH, PREVIOUS_IDENTIFYING_HASHES[-1],
+            CURRENT_HASH, PRIOR_HASHES[-1],
         )
 
         mkdirp(os.path.dirname(runner.pre_commit_path))