chore: fix review comments

pre_commit/languages/docker.py

@@ -2,42 +2,44 @@
 import json
 import os
 import socket
-import subprocess
 from typing import Sequence
 from typing import Tuple
 
 import pre_commit.constants as C
 from pre_commit.hook import Hook
 from pre_commit.languages import helpers
 from pre_commit.prefix import Prefix
-from pre_commit.util import clean_path_on_failure
+from pre_commit.util import clean_path_on_failure, cmd_output_b
 
 ENVIRONMENT_DIR = 'docker'
 PRE_COMMIT_LABEL = 'PRE_COMMIT'
 get_default_version = helpers.basic_get_default_version
 healthy = helpers.basic_healthy
 
 
-def is_in_docker() -> bool:
+def _is_in_docker() -> bool:
     try:
-        with open('/proc/1/cgroup') as f:
-            return 'docker' in f.read()
+        with open('/proc/1/cgroup', 'rb') as f:
+            return b'docker' in f.read()
     except FileNotFoundError:
         return False
 
 
-def get_docker_path(path: str) -> str:
-    if not is_in_docker():
+def _get_docker_path(path: str) -> str:
+    if not _is_in_docker():
         return path
     hostname = socket.gethostname()
-    docker_output = json.loads(
-        subprocess.check_output(['docker', 'inspect', hostname]),
+
+    _, out, _ = cmd_output_b(
+        ' '.join(['docker', 'inspect', hostname])
     )
+
+    docker_output = json.loads(out)
     for mount_path in docker_output[0]['HostConfig']['Binds']:
-        src_path, to_path = mount_path.split(':')
-        if to_path == path:
-            return src_path
-        elif path.startswith(to_path):
+        src_path, to_path, *_ = mount_path.split(':')
+        if os.path.commonpath([path, to_path]) != os.path.sep:
+            # So there is something in common,
+            # and we can proceed remapping it
             return path.replace(to_path, src_path)
     # we're in Docker, but the path is not mounted, cannot really do anything,
     # so fall back to original path
@@ -102,7 +104,7 @@ def docker_cmd() -> Tuple[str, ...]:  # pragma: win32 no cover
         # https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container-volumes-from
         # The `Z` option tells Docker to label the content with a private
         # unshared label. Only the current container can use a private volume.
-        '-v', f'{get_docker_path(os.getcwd())}:/src:rw,Z',
+        '-v', f'{_get_docker_path(os.getcwd())}:/src:rw,Z',
         '--workdir', '/src',
     )
 

tests/languages/docker_test.py

@@ -1,90 +1,132 @@
+import builtins
 import json
-import subprocess
 from typing import List
 from unittest import mock
 from unittest.mock import mock_open
 
+import pytest
+
 from pre_commit.languages import docker
 
 
 def test_docker_fallback_user():
     def invalid_attribute():
         raise AttributeError
+
     with mock.patch.multiple(
-        'os', create=True,
-        getuid=invalid_attribute,
-        getgid=invalid_attribute,
+            'os', create=True,
+            getuid=invalid_attribute,
+            getgid=invalid_attribute,
     ):
         assert docker.get_docker_user() == ()
 
 
 class TestInDocker:
-    @mock.patch('builtins.open', new_callable=mock_open)
-    def test_in_docker_no_file(self, mock_file):
-        mock_file.side_effect = FileNotFoundError
 
-        assert docker.is_in_docker() is False
-        mock_file.assert_called()
+    @pytest.fixture
+    def mock_file_fixture(self):
+        return lambda read_data: \
+            mock.patch.object(builtins, 'open',
+                              new_callable=mock_open,
+                              read_data=read_data)
+
+    def test_in_docker_no_file(self, mock_file_fixture):
+        with mock_file_fixture(None) as m:
+            m.side_effect = FileNotFoundError
 
-    @mock.patch('builtins.open', new_callable=mock_open, read_data='tdockert')
-    def test_in_docker_docker_in_file(self, _):
-        assert docker.is_in_docker() is True
+            assert docker._is_in_docker() is False
+            m.assert_called()
 
-    @mock.patch('builtins.open', new_callable=mock_open, read_data='testtest')
-    def test_in_docker_docker_not_in_file(self, _):
-        assert docker.is_in_docker() is False
+    def test_in_docker_docker_in_file(self, mock_file_fixture):
+        with mock_file_fixture(b'tdockert'):
+            assert docker._is_in_docker() is True
+
+    def test_in_docker_docker_not_in_file(self, mock_file_fixture):
+        with mock_file_fixture(b'testtest'):
+            assert docker._is_in_docker() is False
 
 
 class TestDockerPath:
-    @mock.patch.object(docker, 'is_in_docker', return_value=False)
-    def test_not_in_docker_returns_same(self, _):
-        assert docker.get_docker_path('abc') == 'abc'
+    @pytest.fixture
+    def in_docker(self):
+        with mock.patch.object(docker, '_is_in_docker', return_value=True):
+            yield
+
+    @pytest.fixture
+    def not_in_docker(self):
+        with mock.patch.object(docker, '_is_in_docker', return_value=False):
+            yield
 
-    @mock.patch.object(docker, 'is_in_docker', return_value=True)
-    def test_in_docker_no_binds_same_path(self, _):
+    def test_not_in_docker_returns_same(self, not_in_docker):
+        assert docker._get_docker_path('abc') == 'abc'
+
+    def test_in_docker_no_binds_same_path(self, in_docker):
         binds_list: List[str] = []
         output_string = json.dumps([{'HostConfig': {'Binds': binds_list}}])
         with mock.patch.object(
-            subprocess, 'check_output',
-            return_value=output_string,
+                docker, 'cmd_output_b',
+                return_value=(0, output_string, b''),
         ):
-            assert docker.get_docker_path('abc') == 'abc'
+            assert docker._get_docker_path('abc') == 'abc'
 
-    @mock.patch.object(docker, 'is_in_docker', return_value=True)
-    def test_in_docker_binds_path_equal(self, _):
+    def test_in_docker_binds_path_equal(self, in_docker):
         binds_list = [
             '/opt/my_code:/project',
         ]
         output_string = json.dumps([{'HostConfig': {'Binds': binds_list}}])
         with mock.patch.object(
-            subprocess, 'check_output',
-            return_value=output_string,
+                docker, 'cmd_output_b',
+                return_value=(0, output_string, b''),
         ):
-            assert docker.get_docker_path('/project') == '/opt/my_code'
+            assert docker._get_docker_path('/project') == '/opt/my_code'
 
-    @mock.patch.object(docker, 'is_in_docker', return_value=True)
-    def test_in_docker_binds_path_complex(self, _):
+    def test_in_docker_binds_path_complex(self, in_docker):
         binds_list = [
             '/opt/my_code:/project',
         ]
         output_string = json.dumps([{'HostConfig': {'Binds': binds_list}}])
         with mock.patch.object(
-            subprocess, 'check_output',
-            return_value=output_string,
+                docker, 'cmd_output_b',
+                return_value=(0, output_string, b''),
         ):
-            assert docker.get_docker_path('/project/test/something') == \
-                '/opt/my_code/test/something'
+            path = '/project/test/something'
+            expected = '/opt/my_code/test/something'
+            assert docker._get_docker_path(path) == expected
 
-    @mock.patch.object(docker, 'is_in_docker', return_value=True)
-    def test_in_docker_binds_path_many_binds(self, _):
+    def test_in_docker_no_substring(self, in_docker):
+        binds_list = [
+            '/opt/my_code:/project',
+        ]
+        output_string = json.dumps([{'HostConfig': {'Binds': binds_list}}])
+        with mock.patch.object(
+                docker, 'cmd_output_b',
+                return_value=(0, output_string, b''),
+        ):
+            path = '/projectSUffix/test/something'
+            assert docker._get_docker_path(path) == path
+
+    def test_in_docker_binds_path_many_binds(self, in_docker):
         binds_list = [
             '/something_random:/not_related',
             '/opt/my_code:/project',
             '/something_random2:/not_related2',
         ]
         output_string = json.dumps([{'HostConfig': {'Binds': binds_list}}])
         with mock.patch.object(
-            subprocess, 'check_output',
-            return_value=output_string,
+                docker, 'cmd_output_b',
+                return_value=(0, output_string, b''),
+        ):
+            assert docker._get_docker_path('/project') == '/opt/my_code'
+
+    def test_in_docker_two_colons(self, in_docker):
+        binds_list = [
+            '/opt/my_code:/project:qqqrandom',
+        ]
+        output_string = json.dumps([{'HostConfig': {'Binds': binds_list}}])
+        with mock.patch.object(
+                docker, 'cmd_output_b',
+                return_value=(0, output_string, b''),
         ):
-            assert docker.get_docker_path('/project') == '/opt/my_code'
+            path = '/project/test/something'
+            expected = '/opt/my_code/test/something'
+            assert docker._get_docker_path(path) == expected