Support pre-commit from inside submodules

asottile · asottile · commit c3c98afe4fb9 · 2015-12-18T15:35:44.000-08:00
diff --git a/pre_commit/git.py b/pre_commit/git.py
@@ -24,10 +24,18 @@ def get_root():
         )
 
 
+def get_git_dir(git_root):
+    return os.path.normpath(os.path.join(
+        git_root,
+        cmd_output('git', 'rev-parse', '--git-dir', cwd=git_root)[1].strip(),
+    ))
+
+
 def is_in_merge_conflict():
+    git_dir = get_git_dir('.')
     return (
-        os.path.exists(os.path.join('.git', 'MERGE_MSG')) and
-        os.path.exists(os.path.join('.git', 'MERGE_HEAD'))
+        os.path.exists(os.path.join(git_dir, 'MERGE_MSG')) and
+        os.path.exists(os.path.join(git_dir, 'MERGE_HEAD'))
     )
 
 
@@ -46,7 +54,7 @@ def get_conflicted_files():
     logger.info('Checking merge-conflict files only.')
     # Need to get the conflicted files from the MERGE_MSG because they could
     # have resolved the conflict by choosing one side or the other
-    merge_msg = open(os.path.join('.git', 'MERGE_MSG')).read()
+    merge_msg = open(os.path.join(get_git_dir('.'), 'MERGE_MSG')).read()
     merge_conflict_filenames = parse_merge_msg_for_conflicts(merge_msg)
 
     # This will get the rest of the changes made after the merge.
diff --git a/pre_commit/main.py b/pre_commit/main.py
@@ -25,6 +25,13 @@
 # https://github.com/pre-commit/pre-commit/issues/300
 # In git 2.6.3 (maybe others), git exports this while running pre-commit hooks
 os.environ.pop('GIT_WORK_TREE', None)
+# In git 1.9.1 (maybe others), git exports these while running pre-commit hooks
+# in submodules.  In the general case this causes problems.
+# These are covered by test_install_in_submodule_and_run
+# Causes git clone to clone wrong thing
+os.environ.pop('GIT_DIR', None)
+# Causes 'error invalid object ...' during commit
+os.environ.pop('GIT_INDEX_FILE', None)
 
 
 def main(argv=None):
diff --git a/pre_commit/runner.py b/pre_commit/runner.py
@@ -30,6 +30,10 @@ def create(cls):
         os.chdir(root)
         return cls(root)
 
+    @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, C.CONFIG_FILE)
@@ -44,7 +48,7 @@ def repositories(self):
         return repositories
 
     def get_hook_path(self, hook_type):
-        return os.path.join(self.git_root, '.git', 'hooks', hook_type)
+        return os.path.join(self.git_dir, 'hooks', hook_type)
 
     @cached_property
     def pre_commit_path(self):
diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py
@@ -170,6 +170,21 @@ def test_install_pre_commit_and_run(tempdir_factory):
         assert NORMAL_PRE_COMMIT_RUN.match(output)
 
 
+def test_install_in_submodule_and_run(tempdir_factory):
+    src_path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
+    parent_path = git_dir(tempdir_factory)
+    with cwd(parent_path):
+        cmd_output('git', 'submodule', 'add', src_path, 'sub')
+        cmd_output('git', 'commit', '-am', 'foo')
+
+    sub_pth = os.path.join(parent_path, 'sub')
+    with cwd(sub_pth):
+        assert install(Runner(sub_pth)) == 0
+        ret, output = _get_commit_output(tempdir_factory)
+        assert ret == 0
+        assert NORMAL_PRE_COMMIT_RUN.match(output)
+
+
 def test_install_idempotent(tempdir_factory):
     path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
     with cwd(path):
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -15,6 +15,7 @@
 from pre_commit.store import Store
 from pre_commit.util import cmd_output
 from pre_commit.util import cwd
+from testing.fixtures import git_dir
 from testing.fixtures import make_consuming_repo
 
 
@@ -40,6 +41,26 @@ def in_tmpdir(tempdir_factory):
         yield path
 
 
+def _make_conflict():
+    cmd_output('git', 'checkout', 'origin/master', '-b', 'foo')
+    with io.open('conflict_file', 'w') as conflict_file:
+        conflict_file.write('herp\nderp\n')
+    cmd_output('git', 'add', 'conflict_file')
+    with io.open('foo_only_file', 'w') as foo_only_file:
+        foo_only_file.write('foo')
+    cmd_output('git', 'add', 'foo_only_file')
+    cmd_output('git', 'commit', '-m', 'conflict_file')
+    cmd_output('git', 'checkout', 'origin/master', '-b', 'bar')
+    with io.open('conflict_file', 'w') as conflict_file:
+        conflict_file.write('harp\nddrp\n')
+    cmd_output('git', 'add', 'conflict_file')
+    with io.open('bar_only_file', 'w') as bar_only_file:
+        bar_only_file.write('bar')
+    cmd_output('git', 'add', 'bar_only_file')
+    cmd_output('git', 'commit', '-m', 'conflict_file')
+    cmd_output('git', 'merge', 'foo', retcode=None)
+
+
 @pytest.yield_fixture
 def in_merge_conflict(tempdir_factory):
     path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
@@ -51,26 +72,23 @@ def in_merge_conflict(tempdir_factory):
     conflict_path = tempdir_factory.get()
     cmd_output('git', 'clone', path, conflict_path)
     with cwd(conflict_path):
-        cmd_output('git', 'checkout', 'origin/master', '-b', 'foo')
-        with io.open('conflict_file', 'w') as conflict_file:
-            conflict_file.write('herp\nderp\n')
-        cmd_output('git', 'add', 'conflict_file')
-        with io.open('foo_only_file', 'w') as foo_only_file:
-            foo_only_file.write('foo')
-        cmd_output('git', 'add', 'foo_only_file')
-        cmd_output('git', 'commit', '-m', 'conflict_file')
-        cmd_output('git', 'checkout', 'origin/master', '-b', 'bar')
-        with io.open('conflict_file', 'w') as conflict_file:
-            conflict_file.write('harp\nddrp\n')
-        cmd_output('git', 'add', 'conflict_file')
-        with io.open('bar_only_file', 'w') as bar_only_file:
-            bar_only_file.write('bar')
-        cmd_output('git', 'add', 'bar_only_file')
-        cmd_output('git', 'commit', '-m', 'conflict_file')
-        cmd_output('git', 'merge', 'foo', retcode=None)
+        _make_conflict()
         yield os.path.join(conflict_path)
 
 
+@pytest.yield_fixture
+def in_conflicting_submodule(tempdir_factory):
+    git_dir_1 = git_dir(tempdir_factory)
+    git_dir_2 = git_dir(tempdir_factory)
+    with cwd(git_dir_2):
+        cmd_output('git', 'commit', '--allow-empty', '-m', 'init!')
+    with cwd(git_dir_1):
+        cmd_output('git', 'submodule', 'add', git_dir_2, 'sub')
+    with cwd(os.path.join(git_dir_1, 'sub')):
+        _make_conflict()
+        yield
+
+
 @pytest.yield_fixture(scope='session', autouse=True)
 def dont_write_to_home_directory():
     """pre_commit.store.Store will by default write to the home directory
diff --git a/tests/git_test.py b/tests/git_test.py
@@ -43,6 +43,10 @@ def test_is_in_merge_conflict(in_merge_conflict):
     assert git.is_in_merge_conflict() is True
 
 
+def test_is_in_merge_conflict_submodule(in_conflicting_submodule):
+    assert git.is_in_merge_conflict() is True
+
+
 def test_cherry_pick_conflict(in_merge_conflict):
     cmd_output('git', 'merge', '--abort')
     foo_ref = cmd_output('git', 'rev-parse', 'foo')[1].strip()
@@ -111,6 +115,11 @@ def test_get_conflicted_files(in_merge_conflict):
     assert ret == set(('conflict_file', 'other_file'))
 
 
+def test_get_conflicted_files_in_submodule(in_conflicting_submodule):
+    resolve_conflict()
+    assert set(git.get_conflicted_files()) == set(('conflict_file',))
+
+
 def test_get_conflicted_files_unstaged_files(in_merge_conflict):
     # If they for whatever reason did pre-commit run --no-stash during a
     # conflict
diff --git a/tests/runner_test.py b/tests/runner_test.py
@@ -7,6 +7,7 @@
 import pre_commit.constants as C
 from pre_commit.ordereddict import OrderedDict
 from pre_commit.runner import Runner
+from pre_commit.util import cmd_output
 from pre_commit.util import cwd
 from testing.fixtures import add_config_to_repo
 from testing.fixtures import git_dir
@@ -79,15 +80,19 @@ def test_local_hooks(tempdir_factory, mock_out_store_directory):
     assert len(runner.repositories[0].hooks) == 2
 
 
-def test_pre_commit_path():
-    runner = Runner(os.path.join('foo', 'bar'))
-    expected_path = os.path.join('foo', 'bar', '.git', 'hooks', 'pre-commit')
+def test_pre_commit_path(in_tmpdir):
+    path = os.path.join('foo', 'bar')
+    cmd_output('git', 'init', path)
+    runner = Runner(path)
+    expected_path = os.path.join(path, '.git', 'hooks', 'pre-commit')
     assert runner.pre_commit_path == expected_path
 
 
 def test_pre_push_path():
-    runner = Runner(os.path.join('foo', 'bar'))
-    expected_path = os.path.join('foo', 'bar', '.git', 'hooks', 'pre-push')
+    path = os.path.join('foo', 'bar')
+    cmd_output('git', 'init', path)
+    runner = Runner(path)
+    expected_path = os.path.join(path, '.git', 'hooks', 'pre-push')
     assert runner.pre_push_path == expected_path
 
 
