Merge pull request #1838 from EliahKagan/refresh-version

Fix version_info cache invalidation, typing, parsing, and serialization

Author: Byron

Diff for: 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 for: 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 for: 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))