Merge pull request #2569 from m-rsha/cmd-retcode-to-check

Change `cmd_output_b`s `retcode` arg to a boolean `check`

pre_commit/commands/run.py

@@ -263,7 +263,7 @@ def _all_filenames(args: argparse.Namespace) -> Collection[str]:
 
 def _get_diff() -> bytes:
     _, out, _ = cmd_output_b(
-        'git', 'diff', '--no-ext-diff', '--ignore-submodules', retcode=None,
+        'git', 'diff', '--no-ext-diff', '--ignore-submodules', check=False,
     )
     return out
 
@@ -318,7 +318,7 @@ def _has_unmerged_paths() -> bool:
 def _has_unstaged_config(config_file: str) -> bool:
     retcode, _, _ = cmd_output_b(
         'git', 'diff', '--no-ext-diff', '--exit-code', config_file,
-        retcode=None,
+        check=False,
     )
     # be explicit, other git errors don't mean it has an unstaged config.
     return retcode == 1

pre_commit/error_handler.py

@@ -25,7 +25,7 @@ def _log_and_exit(
     error_msg = f'{msg}: {type(exc).__name__}: '.encode() + force_bytes(exc)
     output.write_line_b(error_msg)
 
-    _, git_version_b, _ = cmd_output_b('git', '--version', retcode=None)
+    _, git_version_b, _ = cmd_output_b('git', '--version', check=False)
     git_version = git_version_b.decode(errors='backslashreplace').rstrip()
 
     storedir = Store().directory

pre_commit/git.py

@@ -187,11 +187,11 @@ def head_rev(remote: str) -> str:
 
 def has_diff(*args: str, repo: str = '.') -> bool:
     cmd = ('git', 'diff', '--quiet', '--no-ext-diff', *args)
-    return cmd_output_b(*cmd, cwd=repo, retcode=None)[0] == 1
+    return cmd_output_b(*cmd, cwd=repo, check=False)[0] == 1
 
 
 def has_core_hookpaths_set() -> bool:
-    _, out, _ = cmd_output_b('git', 'config', 'core.hooksPath', retcode=None)
+    _, out, _ = cmd_output_b('git', 'config', 'core.hooksPath', check=False)
     return bool(out.strip())
 
 

pre_commit/languages/node.py

@@ -75,7 +75,7 @@ def in_env(
 
 def health_check(prefix: Prefix, language_version: str) -> str | None:
     with in_env(prefix, language_version):
-        retcode, _, _ = cmd_output_b('node', '--version', retcode=None)
+        retcode, _, _ = cmd_output_b('node', '--version', check=False)
         if retcode != 0:  # pragma: win32 no cover
             return f'`node --version` returned {retcode}'
         else:

pre_commit/languages/rust.py

@@ -36,7 +36,7 @@ def get_default_version() -> str:
     # Just detecting the executable does not suffice, because if rustup is
     # installed but no toolchain is available, then `cargo` exists but
     # cannot be used without installing a toolchain first.
-    if cmd_output_b('cargo', '--version', retcode=None)[0] == 0:
+    if cmd_output_b('cargo', '--version', check=False)[0] == 0:
         return 'system'
     else:
         return C.DEFAULT

pre_commit/staged_files_only.py

@@ -52,7 +52,7 @@ def _unstaged_changes_cleared(patch_dir: str) -> Generator[None, None, None]:
     retcode, diff_stdout_binary, _ = cmd_output_b(
         'git', 'diff-index', '--ignore-submodules', '--binary',
         '--exit-code', '--no-color', '--no-ext-diff', tree, '--',
-        retcode=None,
+        check=False,
     )
     if retcode and diff_stdout_binary.strip():
         patch_filename = f'patch{int(time.time())}-{os.getpid()}'

pre_commit/util.py

@@ -83,14 +83,12 @@ class CalledProcessError(RuntimeError):
             self,
             returncode: int,
             cmd: tuple[str, ...],
-            expected_returncode: int,
             stdout: bytes,
             stderr: bytes | None,
     ) -> None:
-        super().__init__(returncode, cmd, expected_returncode, stdout, stderr)
+        super().__init__(returncode, cmd, stdout, stderr)
         self.returncode = returncode
         self.cmd = cmd
-        self.expected_returncode = expected_returncode
         self.stdout = stdout
         self.stderr = stderr
 
@@ -104,7 +102,6 @@ class CalledProcessError(RuntimeError):
         return b''.join((
             f'command: {self.cmd!r}\n'.encode(),
             f'return code: {self.returncode}\n'.encode(),
-            f'expected return code: {self.expected_returncode}\n'.encode(),
             b'stdout:', _indent_or_none(self.stdout), b'\n',
             b'stderr:', _indent_or_none(self.stderr),
         ))
@@ -124,7 +121,7 @@ def _oserror_to_output(e: OSError) -> tuple[int, bytes, None]:
 
 def cmd_output_b(
         *cmd: str,
-        retcode: int | None = 0,
+        check: bool = True,
         **kwargs: Any,
 ) -> tuple[int, bytes, bytes | None]:
     _setdefault_kwargs(kwargs)
@@ -142,8 +139,8 @@ def cmd_output_b(
             stdout_b, stderr_b = proc.communicate()
             returncode = proc.returncode
 
-    if retcode is not None and retcode != returncode:
+    if check and returncode:
-        raise CalledProcessError(returncode, cmd, retcode, stdout_b, stderr_b)
+        raise CalledProcessError(returncode, cmd, stdout_b, stderr_b)
 
     return returncode, stdout_b, stderr_b
 
@@ -196,10 +193,10 @@ if os.name != 'nt':  # pragma: win32 no cover
 
     def cmd_output_p(
             *cmd: str,
-            retcode: int | None = 0,
+            check: bool = True,
             **kwargs: Any,
     ) -> tuple[int, bytes, bytes | None]:
-        assert retcode is None
+        assert check is False
         assert kwargs['stderr'] == subprocess.STDOUT, kwargs['stderr']
         _setdefault_kwargs(kwargs)
 

pre_commit/xargs.py

@@ -154,7 +154,7 @@ def xargs(
             run_cmd: tuple[str, ...],
     ) -> tuple[int, bytes, bytes | None]:
         return cmd_fn(
-            *run_cmd, retcode=None, stderr=subprocess.STDOUT, **kwargs,
+            *run_cmd, check=False, stderr=subprocess.STDOUT, **kwargs,
         )
 
     threads = min(len(partitions), target_concurrency)

tests/commands/init_templatedir_test.py

@@ -135,7 +135,7 @@ def test_init_templatedir_skip_on_missing_config(
         retcode, output = git_commit(
             fn=cmd_output_mocked_pre_commit_home,
             tempdir_factory=tempdir_factory,
-            retcode=None,
+            check=False,
         )
 
         assert retcode == commit_retcode

tests/commands/install_uninstall_test.py

@@ -126,7 +126,7 @@ def _get_commit_output(tempdir_factory, touch_file='foo', **kwargs):
     cmd_output('git', 'add', touch_file)
     return git_commit(
         fn=cmd_output_mocked_pre_commit_home,
-        retcode=None,
+        check=False,
         tempdir_factory=tempdir_factory,
         **kwargs,
     )
@@ -286,7 +286,7 @@ def test_environment_not_sourced(tempdir_factory, store):
                 'GIT_AUTHOR_EMAIL': os.environ['GIT_AUTHOR_EMAIL'],
                 'GIT_COMMITTER_EMAIL': os.environ['GIT_COMMITTER_EMAIL'],
             },
-            retcode=None,
+            check=False,
         )
         assert ret == 1
         assert out == (
@@ -551,7 +551,7 @@ def _get_push_output(tempdir_factory, remote='origin', opts=()):
     return cmd_output_mocked_pre_commit_home(
         'git', 'push', remote, 'HEAD:new_branch', *opts,
         tempdir_factory=tempdir_factory,
-        retcode=None,
+        check=False,
     )[:2]
 
 

tests/commands/run_test.py

@@ -718,7 +718,7 @@ def test_non_ascii_hook_id(repo_with_passing_hook, tempdir_factory):
     with cwd(repo_with_passing_hook):
         _, stdout, _ = cmd_output_mocked_pre_commit_home(
             sys.executable, '-m', 'pre_commit.main', 'run', '☃',
-            retcode=None, tempdir_factory=tempdir_factory,
+            check=False, tempdir_factory=tempdir_factory,
         )
         assert 'UnicodeDecodeError' not in stdout
         # Doesn't actually happen, but a reasonable assertion
@@ -737,7 +737,7 @@ def test_stdout_write_bug_py26(repo_with_failing_hook, store, tempdir_factory):
         _, out = git_commit(
             fn=cmd_output_mocked_pre_commit_home,
             tempdir_factory=tempdir_factory,
-            retcode=None,
+            check=False,
         )
         assert 'UnicodeEncodeError' not in out
         # Doesn't actually happen, but a reasonable assertion

tests/conftest.py

@@ -68,7 +68,7 @@ def _make_conflict():
         bar_only_file.write('bar')
     cmd_output('git', 'add', 'bar_only_file')
     git_commit(msg=_make_conflict.__name__)
-    cmd_output('git', 'merge', 'foo', retcode=None)
+    cmd_output('git', 'merge', 'foo', check=False)
 
 
 @pytest.fixture

tests/error_handler_test.py

@@ -162,7 +162,7 @@ def test_error_handler_non_ascii_exception(mock_store_dir):
 def test_error_handler_non_utf8_exception(mock_store_dir):
     with pytest.raises(SystemExit):
         with error_handler.error_handler():
-            raise CalledProcessError(1, ('exe',), 0, b'error: \xa0\xe1', b'')
+            raise CalledProcessError(1, ('exe',), b'error: \xa0\xe1', b'')
 
 
 def test_error_handler_non_stringable_exception(mock_store_dir):
@@ -183,10 +183,11 @@ def test_error_handler_no_tty(tempdir_factory):
         'from pre_commit.error_handler import error_handler\n'
         'with error_handler():\n'
         '    raise ValueError("\\u2603")\n',
-        retcode=3,
+        check=False,
         tempdir_factory=tempdir_factory,
         pre_commit_home=pre_commit_home,
     )
+    assert ret == 3
     log_file = os.path.join(pre_commit_home, 'pre-commit.log')
     out_lines = out.splitlines()
     assert out_lines[-2] == 'An unexpected error has occurred: ValueError: ☃'

tests/git_test.py

@@ -104,7 +104,7 @@ def test_is_in_merge_conflict_submodule(in_conflicting_submodule):
 def test_cherry_pick_conflict(in_merge_conflict):
     cmd_output('git', 'merge', '--abort')
     foo_ref = cmd_output('git', 'rev-parse', 'foo')[1].strip()
-    cmd_output('git', 'cherry-pick', foo_ref, retcode=None)
+    cmd_output('git', 'cherry-pick', foo_ref, check=False)
     assert git.is_in_merge_conflict() is False
 
 

tests/languages/docker_test.py

@@ -178,6 +178,6 @@ def test_get_docker_path_in_docker_windows(in_docker):
 
 def test_get_docker_path_in_docker_docker_in_docker(in_docker):
     # won't be able to discover "self" container in true docker-in-docker
-    err = CalledProcessError(1, (), 0, b'', b'')
+    err = CalledProcessError(1, (), b'', b'')
     with mock.patch.object(docker, 'cmd_output_b', side_effect=err):
         assert docker._get_docker_path('/project') == '/project'

tests/store_test.py

@@ -127,7 +127,7 @@ def test_clone_shallow_failure_fallback_to_complete(
 
     # Force shallow clone failure
     def fake_shallow_clone(self, *args, **kwargs):
-        raise CalledProcessError(1, (), 0, b'', None)
+        raise CalledProcessError(1, (), b'', None)
     store._shallow_clone = fake_shallow_clone
 
     ret = store.clone(path, rev)

tests/util_test.py

@@ -18,11 +18,10 @@ from pre_commit.util import tmpdir
 
 
 def test_CalledProcessError_str():
-    error = CalledProcessError(1, ('exe',), 0, b'output', b'errors')
+    error = CalledProcessError(1, ('exe',), b'output', b'errors')
     assert str(error) == (
         "command: ('exe',)\n"
         'return code: 1\n'
-        'expected return code: 0\n'
         'stdout:\n'
         '    output\n'
         'stderr:\n'
@@ -31,11 +30,10 @@ def test_CalledProcessError_str():
 
 
 def test_CalledProcessError_str_nooutput():
-    error = CalledProcessError(1, ('exe',), 0, b'', b'')
+    error = CalledProcessError(1, ('exe',), b'', b'')
     assert str(error) == (
         "command: ('exe',)\n"
         'return code: 1\n'
-        'expected return code: 0\n'
         'stdout: (none)\n'
         'stderr: (none)'
     )
@@ -83,14 +81,14 @@ def test_tmpdir():
 
 
 def test_cmd_output_exe_not_found():
-    ret, out, _ = cmd_output('dne', retcode=None)
+    ret, out, _ = cmd_output('dne', check=False)
     assert ret == 1
     assert out == 'Executable `dne` not found'
 
 
 @pytest.mark.parametrize('fn', (cmd_output_b, cmd_output_p))
 def test_cmd_output_exe_not_found_bytes(fn):
-    ret, out, _ = fn('dne', retcode=None, stderr=subprocess.STDOUT)
+    ret, out, _ = fn('dne', check=False, stderr=subprocess.STDOUT)
     assert ret == 1
     assert out == b'Executable `dne` not found'
 
@@ -101,7 +99,7 @@ def test_cmd_output_no_shebang(tmpdir, fn):
     make_executable(f)
 
     # previously this raised `OSError` -- the output is platform specific
-    ret, out, _ = fn(str(f), retcode=None, stderr=subprocess.STDOUT)
+    ret, out, _ = fn(str(f), check=False, stderr=subprocess.STDOUT)
     assert ret == 1
     assert isinstance(out, bytes)
     assert out.endswith(b'\n')