From c3c98afe4fb96caf98278edf0c1d21319f8ced91 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Fri, 18 Dec 2015 14:21:30 -0800 Subject: [PATCH] Support pre-commit from inside submodules --- pre_commit/git.py | 14 +++++-- pre_commit/main.py | 7 ++++ pre_commit/runner.py | 6 ++- tests/commands/install_uninstall_test.py | 15 +++++++ tests/conftest.py | 52 ++++++++++++++++-------- tests/git_test.py | 9 ++++ tests/runner_test.py | 15 ++++--- 7 files changed, 92 insertions(+), 26 deletions(-) diff --git a/pre_commit/git.py b/pre_commit/git.py index 8b9a0f96..17b7af13 100644 --- 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 index 28c4f714..e292c72c 100644 --- a/pre_commit/main.py +++ b/pre_commit/main.py @@ -25,6 +25,13 @@ os.environ.pop('__PYVENV_LAUNCHER__', None) # 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 index ae720d05..88028939 100644 --- a/pre_commit/runner.py +++ b/pre_commit/runner.py @@ -30,6 +30,10 @@ class Runner(object): 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 @@ class Runner(object): 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 index 96d49396..758e7f2a 100644 --- 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 index ae15ea94..ea50bee6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,7 @@ from pre_commit.runner import Runner 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 index ffd405bb..084c15b2 100644 --- 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 index 58efab51..7642887c 100644 --- a/tests/runner_test.py +++ b/tests/runner_test.py @@ -7,6 +7,7 @@ import os.path 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