diff --git a/git/cmd.py b/git/cmd.py index f58e6df5b..bfc885222 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -8,6 +8,7 @@ import re import contextlib import io +import itertools import logging import os import signal @@ -25,7 +26,6 @@ UnsafeProtocolError, ) from git.util import ( - LazyMixin, cygpath, expand_path, is_cygwin_git, @@ -287,7 +287,7 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc ## -- End Utilities -- @} -class Git(LazyMixin): +class Git: """The Git class manages communication with the Git binary. It provides a convenient interface to calling the Git binary, such as in:: @@ -307,12 +307,18 @@ class Git(LazyMixin): "cat_file_all", "cat_file_header", "_version_info", + "_version_info_token", "_git_options", "_persistent_git_options", "_environment", ) - _excluded_ = ("cat_file_all", "cat_file_header", "_version_info") + _excluded_ = ( + "cat_file_all", + "cat_file_header", + "_version_info", + "_version_info_token", + ) re_unsafe_protocol = re.compile(r"(.+)::.+") @@ -359,6 +365,8 @@ def __setstate__(self, d: Dict[str, Any]) -> None: the top level ``__init__``. """ + _refresh_token = object() # Since None would match an initial _version_info_token. + @classmethod def refresh(cls, path: Union[None, PathLike] = None) -> bool: """This gets called by the refresh function (see the top level __init__).""" @@ -371,7 +379,9 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: # Keep track of the old and new git executable path. old_git = cls.GIT_PYTHON_GIT_EXECUTABLE + old_refresh_token = cls._refresh_token cls.GIT_PYTHON_GIT_EXECUTABLE = new_git + cls._refresh_token = object() # Test if the new git executable path is valid. A GitCommandNotFound error is # spawned by us. A PermissionError is spawned if the git executable cannot be @@ -400,6 +410,7 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: # Revert to whatever the old_git was. cls.GIT_PYTHON_GIT_EXECUTABLE = old_git + cls._refresh_token = old_refresh_token if old_git is None: # On the first refresh (when GIT_PYTHON_GIT_EXECUTABLE is None) we only @@ -783,6 +794,10 @@ def __init__(self, working_dir: Union[None, PathLike] = None): # Extra environment variables to pass to git commands self._environment: Dict[str, str] = {} + # Cached version slots + self._version_info: Union[Tuple[int, ...], None] = None + self._version_info_token: object = None + # Cached command slots self.cat_file_header: Union[None, TBD] = None self.cat_file_all: Union[None, TBD] = None @@ -795,8 +810,8 @@ def __getattr__(self, name: str) -> Any: Callable object that will execute call :meth:`_call_process` with your arguments. """ - if name[0] == "_": - return LazyMixin.__getattr__(self, name) + if name.startswith("_"): + return super().__getattribute__(name) return lambda *args, **kwargs: self._call_process(name, *args, **kwargs) def set_persistent_git_options(self, **kwargs: Any) -> None: @@ -811,33 +826,36 @@ def set_persistent_git_options(self, **kwargs: Any) -> None: self._persistent_git_options = self.transform_kwargs(split_single_char_options=True, **kwargs) - def _set_cache_(self, attr: str) -> None: - if attr == "_version_info": - # We only use the first 4 numbers, as everything else could be strings in fact (on Windows). - process_version = self._call_process("version") # Should be as default *args and **kwargs used. - version_numbers = process_version.split(" ")[2] - - self._version_info = cast( - Tuple[int, int, int, int], - tuple(int(n) for n in version_numbers.split(".")[:4] if n.isdigit()), - ) - else: - super()._set_cache_(attr) - # END handle version info - @property def working_dir(self) -> Union[None, PathLike]: """:return: Git directory we are working on""" return self._working_dir @property - def version_info(self) -> Tuple[int, int, int, int]: + def version_info(self) -> Tuple[int, ...]: """ - :return: tuple(int, int, int, int) tuple with integers representing the major, minor - and additional version numbers as parsed from git version. + :return: tuple with integers representing the major, minor and additional + version numbers as parsed from git version. Up to four fields are used. This value is generated on demand and is cached. """ + # Refreshing is global, but version_info caching is per-instance. + refresh_token = self._refresh_token # Copy token in case of concurrent refresh. + + # Use the cached version if obtained after the most recent refresh. + if self._version_info_token is refresh_token: + assert self._version_info is not None, "Bug: corrupted token-check state" + return self._version_info + + # Run "git version" and parse it. + process_version = self._call_process("version") + version_string = process_version.split(" ")[2] + version_fields = version_string.split(".")[:4] + leading_numeric_fields = itertools.takewhile(str.isdigit, version_fields) + self._version_info = tuple(map(int, leading_numeric_fields)) + + # This value will be considered valid until the next refresh. + self._version_info_token = refresh_token return self._version_info @overload diff --git a/test/test_git.py b/test/test_git.py index f12428f9e..97e21cad4 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -10,11 +10,12 @@ import os import os.path as osp from pathlib import Path +import pickle import re import shutil import subprocess import sys -from tempfile import TemporaryFile +import tempfile from unittest import skipUnless if sys.version_info >= (3, 8): @@ -67,6 +68,32 @@ def _rollback_refresh(): refresh() +@contextlib.contextmanager +def _fake_git(*version_info): + fake_version = ".".join(map(str, version_info)) + fake_output = f"git version {fake_version} (fake)" + + with tempfile.TemporaryDirectory() as tdir: + if os.name == "nt": + fake_git = Path(tdir, "fake-git.cmd") + script = f"@echo {fake_output}\n" + fake_git.write_text(script, encoding="utf-8") + else: + fake_git = Path(tdir, "fake-git") + script = f"#!/bin/sh\necho '{fake_output}'\n" + fake_git.write_text(script, encoding="utf-8") + fake_git.chmod(0o755) + + yield str(fake_git.absolute()) + + +def _rename_with_stem(path, new_stem): + if sys.version_info >= (3, 9): + path.rename(path.with_stem(new_stem)) + else: + path.rename(path.with_name(new_stem + path.suffix)) + + @ddt.ddt class TestGit(TestBase): @classmethod @@ -260,13 +287,9 @@ def test_it_ignores_false_kwargs(self, git): self.assertTrue("pass_this_kwarg" not in git.call_args[1]) def test_it_raises_proper_exception_with_output_stream(self): - tmp_file = TemporaryFile() - self.assertRaises( - GitCommandError, - self.git.checkout, - "non-existent-branch", - output_stream=tmp_file, - ) + with tempfile.TemporaryFile() as tmp_file: + with self.assertRaises(GitCommandError): + self.git.checkout("non-existent-branch", output_stream=tmp_file) def test_it_accepts_environment_variables(self): filename = fixture_path("ls_tree_empty") @@ -314,12 +337,38 @@ def test_persistent_cat_file_command(self): self.assertEqual(typename, typename_two) self.assertEqual(size, size_two) - def test_version(self): + def test_version_info(self): + """The version_info attribute is a tuple of up to four ints.""" v = self.git.version_info self.assertIsInstance(v, tuple) + self.assertLessEqual(len(v), 4) for n in v: self.assertIsInstance(n, int) - # END verify number types + + def test_version_info_pickleable(self): + """The version_info attribute is usable on unpickled Git instances.""" + deserialized = pickle.loads(pickle.dumps(self.git)) + v = deserialized.version_info + self.assertIsInstance(v, tuple) + self.assertLessEqual(len(v), 4) + for n in v: + self.assertIsInstance(n, int) + + @ddt.data( + (("123", "456", "789"), (123, 456, 789)), + (("12", "34", "56", "78"), (12, 34, 56, 78)), + (("12", "34", "56", "78", "90"), (12, 34, 56, 78)), + (("1", "2", "a", "3"), (1, 2)), + (("1", "-2", "3"), (1,)), + (("1", "2a", "3"), (1,)), # Subject to change. + ) + def test_version_info_is_leading_numbers(self, case): + fake_fields, expected_version_info = case + with _rollback_refresh(): + with _fake_git(*fake_fields) as path: + refresh(path) + new_git = Git() + self.assertEqual(new_git.version_info, expected_version_info) def test_git_exc_name_is_git(self): self.assertEqual(self.git.git_exec_name, "git") @@ -487,6 +536,150 @@ def test_refresh_with_good_relative_git_path_arg(self): refresh(basename) self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) + def test_version_info_is_cached(self): + fake_version_info = (123, 456, 789) + with _rollback_refresh(): + with _fake_git(*fake_version_info) as path: + new_git = Git() # Not cached yet. + refresh(path) + self.assertEqual(new_git.version_info, fake_version_info) + os.remove(path) # Arrange that a second subprocess call would fail. + self.assertEqual(new_git.version_info, fake_version_info) + + def test_version_info_cache_is_per_instance(self): + with _rollback_refresh(): + with _fake_git(123, 456, 789) as path: + git1 = Git() + git2 = Git() + refresh(path) + git1.version_info + os.remove(path) # Arrange that the second subprocess call will fail. + with self.assertRaises(GitCommandNotFound): + git2.version_info + git1.version_info + + def test_version_info_cache_is_not_pickled(self): + with _rollback_refresh(): + with _fake_git(123, 456, 789) as path: + git1 = Git() + refresh(path) + git1.version_info + git2 = pickle.loads(pickle.dumps(git1)) + os.remove(path) # Arrange that the second subprocess call will fail. + with self.assertRaises(GitCommandNotFound): + git2.version_info + git1.version_info + + def test_successful_refresh_with_arg_invalidates_cached_version_info(self): + with _rollback_refresh(): + with _fake_git(11, 111, 1) as path1: + with _fake_git(22, 222, 2) as path2: + new_git = Git() + refresh(path1) + new_git.version_info + refresh(path2) + self.assertEqual(new_git.version_info, (22, 222, 2)) + + def test_failed_refresh_with_arg_does_not_invalidate_cached_version_info(self): + with _rollback_refresh(): + with _fake_git(11, 111, 1) as path1: + with _fake_git(22, 222, 2) as path2: + new_git = Git() + refresh(path1) + new_git.version_info + os.remove(path1) # Arrange that a repeat call for path1 would fail. + os.remove(path2) # Arrange that the new call for path2 will fail. + with self.assertRaises(GitCommandNotFound): + refresh(path2) + self.assertEqual(new_git.version_info, (11, 111, 1)) + + def test_successful_refresh_with_same_arg_invalidates_cached_version_info(self): + """Changing git at the same path and refreshing affects version_info.""" + with _rollback_refresh(): + with _fake_git(11, 111, 1) as path1: + with _fake_git(22, 222, 2) as path2: + new_git = Git() + refresh(path1) + new_git.version_info + shutil.copy(path2, path1) + refresh(path1) # The fake git at path1 has a different version now. + self.assertEqual(new_git.version_info, (22, 222, 2)) + + def test_successful_refresh_with_env_invalidates_cached_version_info(self): + with contextlib.ExitStack() as stack: + stack.enter_context(_rollback_refresh()) + path1 = stack.enter_context(_fake_git(11, 111, 1)) + path2 = stack.enter_context(_fake_git(22, 222, 2)) + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": path1}): + new_git = Git() + refresh() + new_git.version_info + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": path2}): + refresh() + self.assertEqual(new_git.version_info, (22, 222, 2)) + + def test_failed_refresh_with_env_does_not_invalidate_cached_version_info(self): + with contextlib.ExitStack() as stack: + stack.enter_context(_rollback_refresh()) + path1 = stack.enter_context(_fake_git(11, 111, 1)) + path2 = stack.enter_context(_fake_git(22, 222, 2)) + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": path1}): + new_git = Git() + refresh() + new_git.version_info + os.remove(path1) # Arrange that a repeat call for path1 would fail. + os.remove(path2) # Arrange that the new call for path2 will fail. + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": path2}): + with self.assertRaises(GitCommandNotFound): + refresh(path2) + self.assertEqual(new_git.version_info, (11, 111, 1)) + + def test_successful_refresh_with_same_env_invalidates_cached_version_info(self): + """Changing git at the same path/command and refreshing affects version_info.""" + with contextlib.ExitStack() as stack: + stack.enter_context(_rollback_refresh()) + path1 = stack.enter_context(_fake_git(11, 111, 1)) + path2 = stack.enter_context(_fake_git(22, 222, 2)) + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": path1}): + new_git = Git() + refresh() + new_git.version_info + shutil.copy(path2, path1) + refresh() # The fake git at path1 has a different version now. + self.assertEqual(new_git.version_info, (22, 222, 2)) + + def test_successful_default_refresh_invalidates_cached_version_info(self): + """Refreshing updates version after a filesystem change adds a git command.""" + # The key assertion here is the last. The others mainly verify the test itself. + with contextlib.ExitStack() as stack: + stack.enter_context(_rollback_refresh()) + + path1 = Path(stack.enter_context(_fake_git(11, 111, 1))) + path2 = Path(stack.enter_context(_fake_git(22, 222, 2))) + + new_path_var = f"{path1.parent}{os.pathsep}{path2.parent}" + stack.enter_context(mock.patch.dict(os.environ, {"PATH": new_path_var})) + stack.enter_context(_patch_out_env("GIT_PYTHON_GIT_EXECUTABLE")) + + if os.name == "nt": + # On Windows, use a shell so "git" finds "git.cmd". (In the infrequent + # case that this effect is desired in production code, it should not be + # done with this technique. USE_SHELL=True is less secure and reliable, + # as unintended shell expansions can occur, and is deprecated. Instead, + # use a custom command, by setting the GIT_PYTHON_GIT_EXECUTABLE + # environment variable to git.cmd or by passing git.cmd's full path to + # git.refresh. Or wrap the script with a .exe shim. + stack.enter_context(mock.patch.object(Git, "USE_SHELL", True)) + + new_git = Git() + _rename_with_stem(path2, "git") # "Install" git, "late" in the PATH. + refresh() + self.assertEqual(new_git.version_info, (22, 222, 2), 'before "downgrade"') + _rename_with_stem(path1, "git") # "Install" another, higher priority. + self.assertEqual(new_git.version_info, (22, 222, 2), "stale version") + refresh() + self.assertEqual(new_git.version_info, (11, 111, 1), "fresh version") + def test_options_are_passed_to_git(self): # This works because any command after git --version is ignored. git_version = self.git(version=True).NoOp() diff --git a/test/test_index.py b/test/test_index.py index ffe71450d..fd62bb893 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -528,7 +528,7 @@ def test_index_file_diffing(self, rw_repo): index.checkout(test_file) except CheckoutError as e: # Detailed exceptions are only possible in older git versions. - if rw_repo.git._version_info < (2, 29): + if rw_repo.git.version_info < (2, 29): self.assertEqual(len(e.failed_files), 1) self.assertEqual(e.failed_files[0], osp.basename(test_file)) self.assertEqual(len(e.failed_files), len(e.failed_reasons))