From 5f0cab9114146420e5088227daf998b6e91c855c Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sun, 13 Apr 2014 23:20:26 -0700 Subject: [PATCH 1/3] Add failing test. --- tests/commands_test.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/commands_test.py b/tests/commands_test.py index ae4e3b4c..40530461 100644 --- a/tests/commands_test.py +++ b/tests/commands_test.py @@ -275,3 +275,34 @@ def test_no_stash(repo_with_passing_hook, no_stash, all_files, expect_stash): assert warning_msg in printed else: assert warning_msg not in printed + + +@pytest.yield_fixture +def in_merge_conflict(repo_with_passing_hook): + local['git']['add', C.CONFIG_FILE]() + local['git']['commit', '-m' 'add hooks file']() + local['git']['clone', '.', 'foo']() + with local.cwd('foo'): + local['git']['checkout', 'origin/master', '-b', 'foo']() + with open('conflict_file', 'w') as conflict_file: + conflict_file.write('herp\nderp\n') + local['git']['add', 'conflict_file']() + local['git']['commit', '-m', 'conflict_file']() + local['git']['checkout', 'origin/master', '-b', 'bar']() + with open('conflict_file', 'w') as conflict_file: + conflict_file.write('harp\nddrp\n') + local['git']['add', 'conflict_file']() + local['git']['commit', '-m', 'conflict_file']() + local['git']['merge', 'foo'](retcode=None) + yield os.path.join(repo_with_passing_hook, 'foo') + + +def test_merge_conflict(in_merge_conflict): + # Touch another file so we have unstaged non-conflicting things + assert os.path.exists('dummy') + with open('dummy', 'w') as dummy_file: + dummy_file.write('bar\nbaz\n') + + ret, printed = _do_run(in_merge_conflict, _get_opts()) + assert ret == 1 + assert 'Resolve merge conflicts before committing' in printed From 846ddeed6739961599d068c0bc2f692c903938ac Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sun, 13 Apr 2014 23:44:53 -0700 Subject: [PATCH 2/3] Fixes crash during merge conflict. Closes #82. --- pre_commit/commands.py | 16 ++++++++++++++++ tests/commands_test.py | 19 ++++++++++++++++++- tests/staged_files_only_test.py | 2 +- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/pre_commit/commands.py b/pre_commit/commands.py index ac9d4487..defd4199 100644 --- a/pre_commit/commands.py +++ b/pre_commit/commands.py @@ -30,6 +30,9 @@ COLS = int(subprocess.Popen(['tput', 'cols'], stdout=subprocess.PIPE).communicat PASS_FAIL_LENGTH = 6 +# Grabbed from `git help status` +CONFLICTING_GIT_STATUSES = set(('DD', 'AU', 'UD', 'UA', 'DU', 'AA', 'UU')) + def install(runner): """Install the pre-commit hooks.""" @@ -229,11 +232,24 @@ def _run_hook(runner, hook_id, args, write): return 1 +def _has_unmerged_paths(runner): + _, stdout, _ = runner.cmd_runner.run( + ['git', 'status', '--short'], retcode=None, + ) + codes = set(line[:2] for line in stdout.splitlines()) + return codes & CONFLICTING_GIT_STATUSES > set() + + def run(runner, args, write=sys.stdout.write): # Set up our logging handler logger.addHandler(LoggingHandler(args.color, write=write)) logger.setLevel(logging.INFO) + # Check if we have unresolved merge conflict files and fail fast. + if _has_unmerged_paths(runner): + logger.error('Unmerged files. Resolve before committing.') + return 1 + if args.no_stash or args.all_files: ctx = noop_context() else: diff --git a/tests/commands_test.py b/tests/commands_test.py index 40530461..37fe3aea 100644 --- a/tests/commands_test.py +++ b/tests/commands_test.py @@ -277,6 +277,17 @@ def test_no_stash(repo_with_passing_hook, no_stash, all_files, expect_stash): assert warning_msg not in printed +@pytest.mark.parametrize( + ('mode', 'expected'), + [(status, True) for status in commands.CONFLICTING_GIT_STATUSES] + + [(' A', False), (' D', False), (' M', False)] +) +def test_has_unmerged_paths(mode, expected): + mock_runner = mock.Mock() + mock_runner.cmd_runner.run.return_value = (1, mode + ' foo', '') + assert commands._has_unmerged_paths(mock_runner) is expected + + @pytest.yield_fixture def in_merge_conflict(repo_with_passing_hook): local['git']['add', C.CONFIG_FILE]() @@ -298,6 +309,12 @@ def in_merge_conflict(repo_with_passing_hook): def test_merge_conflict(in_merge_conflict): + ret, printed = _do_run(in_merge_conflict, _get_opts()) + assert ret == 1 + assert 'Unmerged files. Resolve before committing.' in printed + + +def test_merge_conflict_modified(in_merge_conflict): # Touch another file so we have unstaged non-conflicting things assert os.path.exists('dummy') with open('dummy', 'w') as dummy_file: @@ -305,4 +322,4 @@ def test_merge_conflict(in_merge_conflict): ret, printed = _do_run(in_merge_conflict, _get_opts()) assert ret == 1 - assert 'Resolve merge conflicts before committing' in printed + assert 'Unmerged files. Resolve before committing.' in printed diff --git a/tests/staged_files_only_test.py b/tests/staged_files_only_test.py index adb0b015..e3533733 100644 --- a/tests/staged_files_only_test.py +++ b/tests/staged_files_only_test.py @@ -227,7 +227,7 @@ def fake_logging_handler(): self.logs = [] def emit(self, record): - self.logs.append(record) + self.logs.append(record) # pragma: no cover (only hit in failure) pre_commit_logger = logging.getLogger('pre_commit') original_level = pre_commit_logger.getEffectiveLevel() From 2e4cde9cb0f16030e20cbb5c08fe3814d42a9c38 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sun, 13 Apr 2014 23:52:52 -0700 Subject: [PATCH 3/3] Simplify has_unmerged_paths --- pre_commit/commands.py | 10 ++-------- tests/commands_test.py | 10 +++------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/pre_commit/commands.py b/pre_commit/commands.py index defd4199..33e36879 100644 --- a/pre_commit/commands.py +++ b/pre_commit/commands.py @@ -30,9 +30,6 @@ COLS = int(subprocess.Popen(['tput', 'cols'], stdout=subprocess.PIPE).communicat PASS_FAIL_LENGTH = 6 -# Grabbed from `git help status` -CONFLICTING_GIT_STATUSES = set(('DD', 'AU', 'UD', 'UA', 'DU', 'AA', 'UU')) - def install(runner): """Install the pre-commit hooks.""" @@ -233,11 +230,8 @@ def _run_hook(runner, hook_id, args, write): def _has_unmerged_paths(runner): - _, stdout, _ = runner.cmd_runner.run( - ['git', 'status', '--short'], retcode=None, - ) - codes = set(line[:2] for line in stdout.splitlines()) - return codes & CONFLICTING_GIT_STATUSES > set() + _, stdout, _ = runner.cmd_runner.run(['git', 'ls-files', '--unmerged']) + return bool(stdout.strip()) def run(runner, args, write=sys.stdout.write): diff --git a/tests/commands_test.py b/tests/commands_test.py index 37fe3aea..e3e6eb17 100644 --- a/tests/commands_test.py +++ b/tests/commands_test.py @@ -277,14 +277,10 @@ def test_no_stash(repo_with_passing_hook, no_stash, all_files, expect_stash): assert warning_msg not in printed -@pytest.mark.parametrize( - ('mode', 'expected'), - [(status, True) for status in commands.CONFLICTING_GIT_STATUSES] + - [(' A', False), (' D', False), (' M', False)] -) -def test_has_unmerged_paths(mode, expected): +@pytest.mark.parametrize(('output', 'expected'), (('some', True), ('', False))) +def test_has_unmerged_paths(output, expected): mock_runner = mock.Mock() - mock_runner.cmd_runner.run.return_value = (1, mode + ' foo', '') + mock_runner.cmd_runner.run.return_value = (1, output, '') assert commands._has_unmerged_paths(mock_runner) is expected