From e40fc2cd2677c8f7e7c067583df00572430442ec Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 00:41:41 -0500
Subject: [PATCH 01/19] Use TemporaryFile as a context manager

(This also uses assertRaises as a context manager, but that is
just for improved clarity.)
---
 test/test_git.py | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index f12428f9e..a00d2b950 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -260,13 +260,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 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")

From 1de2fdc5234e1d688386f3a345f9cf2f264ba5ea Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 01:34:08 -0500
Subject: [PATCH 02/19] Test that version_info caches

This begins to test the established version_info caching behavior.
---
 test/test_git.py | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index a00d2b950..d8349e9be 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -14,7 +14,7 @@
 import shutil
 import subprocess
 import sys
-from tempfile import TemporaryFile
+import tempfile
 from unittest import skipUnless
 
 if sys.version_info >= (3, 8):
@@ -67,6 +67,24 @@ def _rollback_refresh():
         refresh()
 
 
+@contextlib.contextmanager
+def _fake_git():
+    fake_output = "git version 123.456.789 (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())
+
+
 @ddt.ddt
 class TestGit(TestBase):
     @classmethod
@@ -260,7 +278,7 @@ 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):
-        with TemporaryFile() as tmp_file:
+        with tempfile.TemporaryFile() as tmp_file:
             with self.assertRaises(GitCommandError):
                 self.git.checkout("non-existent-branch", output_stream=tmp_file)
 
@@ -483,6 +501,16 @@ 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):
+        with _rollback_refresh():
+            with _fake_git() as path:
+                new_git = Git()  # Not cached yet.
+                refresh(path)
+                version_info = new_git.version_info  # Caches the value.
+                self.assertEqual(version_info, (123, 456, 789))
+                os.remove(path)  # Arrange that reading a second time would fail.
+                self.assertEqual(new_git.version_info, version_info)  # Cached value.
+
     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()

From 3f107d59399538594fa98d5e3956e4cfe707494b Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 01:35:31 -0500
Subject: [PATCH 03/19] Test that version_info caching is per instance

This independence is also established caching behavior for
version_info.
---
 test/test_git.py | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index d8349e9be..19e448f4a 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -28,6 +28,8 @@
 from git.util import cwd, finalize_process
 from test.lib import TestBase, fixture_path, with_rw_directory
 
+_FAKE_GIT_VERSION_INFO = (123, 456, 789)
+
 
 @contextlib.contextmanager
 def _patch_out_env(name):
@@ -69,7 +71,8 @@ def _rollback_refresh():
 
 @contextlib.contextmanager
 def _fake_git():
-    fake_output = "git version 123.456.789 (fake)"
+    fake_version = ".".join(str(field) for field in _FAKE_GIT_VERSION_INFO)
+    fake_output = f"git version {fake_version} (fake)"
 
     with tempfile.TemporaryDirectory() as tdir:
         if os.name == "nt":
@@ -506,10 +509,21 @@ def test_version_info_is_cached(self):
             with _fake_git() as path:
                 new_git = Git()  # Not cached yet.
                 refresh(path)
-                version_info = new_git.version_info  # Caches the value.
-                self.assertEqual(version_info, (123, 456, 789))
-                os.remove(path)  # Arrange that reading a second time would fail.
-                self.assertEqual(new_git.version_info, version_info)  # Cached value.
+                self.assertEqual(new_git.version_info, _FAKE_GIT_VERSION_INFO)
+                os.remove(path)  # Arrange that a second subprocess call would fail.
+                self.assertEqual(new_git.version_info, _FAKE_GIT_VERSION_INFO)
+
+    def test_version_info_cache_is_per_instance(self):
+        with _rollback_refresh():
+            with _fake_git() 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_options_are_passed_to_git(self):
         # This works because any command after git --version is ignored.

From 2eac36c951553e8554609fdad026e69e7e1e561e Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 02:20:40 -0500
Subject: [PATCH 04/19] Have _fake_git fixture take version_info

This reorganization is to facilitate using two separate fake git
commands with different versions, in forthcoming tests.
---
 test/test_git.py | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index 19e448f4a..7c433e910 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -28,8 +28,6 @@
 from git.util import cwd, finalize_process
 from test.lib import TestBase, fixture_path, with_rw_directory
 
-_FAKE_GIT_VERSION_INFO = (123, 456, 789)
-
 
 @contextlib.contextmanager
 def _patch_out_env(name):
@@ -70,8 +68,8 @@ def _rollback_refresh():
 
 
 @contextlib.contextmanager
-def _fake_git():
-    fake_version = ".".join(str(field) for field in _FAKE_GIT_VERSION_INFO)
+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:
@@ -505,17 +503,18 @@ def test_refresh_with_good_relative_git_path_arg(self):
                 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() as path:
+            with _fake_git(*fake_version_info) as path:
                 new_git = Git()  # Not cached yet.
                 refresh(path)
-                self.assertEqual(new_git.version_info, _FAKE_GIT_VERSION_INFO)
+                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_GIT_VERSION_INFO)
+                self.assertEqual(new_git.version_info, fake_version_info)
 
     def test_version_info_cache_is_per_instance(self):
         with _rollback_refresh():
-            with _fake_git() as path:
+            with _fake_git(123, 456, 789) as path:
                 git1 = Git()
                 git2 = Git()
                 refresh(path)

From 2d6311bf35ee1f1bf6a7347edc06327cfa18b118 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 22:56:23 -0500
Subject: [PATCH 05/19] Test version_info on unpickled Git instance

The new test doesn't pass yet, as #1836 is not yet fixed.
---
 test/test_git.py | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index 7c433e910..0269bca4f 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -10,6 +10,7 @@
 import os
 import os.path as osp
 from pathlib import Path
+import pickle
 import re
 import shutil
 import subprocess
@@ -329,12 +330,20 @@ 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 ints."""
         v = self.git.version_info
         self.assertIsInstance(v, tuple)
         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)
+        for n in v:
+            self.assertIsInstance(n, int)
 
     def test_git_exc_name_is_git(self):
         self.assertEqual(self.git.git_exec_name, "git")

From f699a387e19764e591dbca392035a5f9e457b06d Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 23:20:44 -0500
Subject: [PATCH 06/19] Fix Git.version_info pickling

For #1836.

This uses a property with the same logic as before for version_info
caching, except that the _version_info backing attribute holds the
value None, rather than being unset, for the uncomputed state. This
rectifies the inconistency between the property's behavior and the
way the associated states are represented by its __setstate__ and
__getstate__ methods for pickling.

Because the Git class only used LazyMixin as an implementation
detail of the version_info attribute, it is removed as a subclass.
---
 git/cmd.py | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/git/cmd.py b/git/cmd.py
index f58e6df5b..e5edb4959 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -25,7 +25,6 @@
     UnsafeProtocolError,
 )
 from git.util import (
-    LazyMixin,
     cygpath,
     expand_path,
     is_cygwin_git,
@@ -287,7 +286,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::
@@ -784,6 +783,7 @@ def __init__(self, working_dir: Union[None, PathLike] = None):
         self._environment: Dict[str, str] = {}
 
         # Cached command slots
+        self._version_info: Union[Tuple[int, int, int, int], None] = None
         self.cat_file_header: Union[None, TBD] = None
         self.cat_file_all: Union[None, TBD] = None
 
@@ -795,8 +795,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,20 +811,6 @@ 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"""
@@ -838,6 +824,16 @@ def version_info(self) -> Tuple[int, int, int, int]:
 
             This value is generated on demand and is cached.
         """
+        if self._version_info is None:
+            # 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()),
+            )
+
         return self._version_info
 
     @overload

From 634b61812cd14a13cccb975d5d54d34001c56449 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 23:37:56 -0500
Subject: [PATCH 07/19] Use documented version_info in test_index_file_diffing

Instead of directly accessing the _version_info attribute.

This fixes a new test failure since the way the public version_info
property uses the _version_info backing attribute has changed, and
also shows the usage that is documented (accessing _version_info
was never guaranteed to work, and now no longer works as before).
---
 test/test_index.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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))

From 626a550d22d91dc94b7cc0bfda3fef3ab7f10189 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 18:43:38 -0500
Subject: [PATCH 08/19] Test that refreshing invalidates cached version_info

Most of these don't pass yet, as such invalidation isn't
implemented yet (#1829).
---
 test/test_git.py | 78 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/test/test_git.py b/test/test_git.py
index 0269bca4f..9c18de77d 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -533,6 +533,84 @@ def test_version_info_cache_is_per_instance(self):
                     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_options_are_passed_to_git(self):
         # This works because any command after git --version is ignored.
         git_version = self.git(version=True).NoOp()

From 9151c38b18474d6642857ece114a39e34fe7dd09 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 19:37:27 -0500
Subject: [PATCH 09/19] Test "installing" another git and refreshing

This is trickier to test than the other cases, but important enough
that I think it deserves its own test.
---
 test/test_git.py | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/test/test_git.py b/test/test_git.py
index 9c18de77d..10c51cd8f 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -24,6 +24,7 @@
     import mock  # To be able to examine call_args.kwargs on a mock.
 
 import ddt
+import pytest
 
 from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd
 from git.util import cwd, finalize_process
@@ -611,6 +612,30 @@ def test_successful_refresh_with_same_env_invalidates_cached_version_info(self):
                 refresh()  # The fake git at path1 has a different version now.
                 self.assertEqual(new_git.version_info, (22, 222, 2))
 
+    @pytest.mark.xfail(
+        os.name == "nt",
+        reason="""Name "git" won't find .bat/.cmd (need shim, custom name, or shell)""",
+        raises=GitCommandNotFound,
+    )
+    def test_successful_refresh_in_default_scenario_invalidates_cached_version(self):
+        """Refreshing updates version after a filesystem change adds a git command."""
+        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)))
+            sep = os.pathsep
+            new_path_var = f"{path1.parent}{sep}{path2.parent}{sep}{os.environ['PATH']}"
+            stack.enter_context(mock.patch.dict(os.environ, {"PATH": new_path_var}))
+            stack.enter_context(_patch_out_env("GIT_PYTHON_GIT_EXECUTABLE"))
+            new_git = Git()
+            path2.rename(path2.with_stem("git"))
+            refresh()
+            self.assertEqual(new_git.version_info, (22, 222, 2), 'before "downgrade"')
+            path1.rename(path1.with_stem("git"))
+            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()

From c2d72ffde8b046d1f6d5314512fad24bd1b923eb Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 19:40:26 -0500
Subject: [PATCH 10/19] Simplify patched PATH env var construction

This also makes the xfail marking for Windows correct.
---
 test/test_git.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index 10c51cd8f..bf9e2373f 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -623,8 +623,7 @@ def test_successful_refresh_in_default_scenario_invalidates_cached_version(self)
             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)))
-            sep = os.pathsep
-            new_path_var = f"{path1.parent}{sep}{path2.parent}{sep}{os.environ['PATH']}"
+            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"))
             new_git = Git()

From ee385bd8058473f4fdd06c2ba7f7b980a45e3ada Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Wed, 21 Feb 2024 20:44:25 -0500
Subject: [PATCH 11/19] Make "install" and refresh version_info test portable

This enables it to run on Windows, removing the xfail marking.

+ Rename the test.
+ Add comments and reformat.

The test still does not pass (on any platform) because it is one of
the tests of the invalidation feature that is not yet implemented.
---
 test/test_git.py | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index bf9e2373f..80316f8f8 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -24,7 +24,6 @@
     import mock  # To be able to examine call_args.kwargs on a mock.
 
 import ddt
-import pytest
 
 from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd
 from git.util import cwd, finalize_process
@@ -612,25 +611,34 @@ def test_successful_refresh_with_same_env_invalidates_cached_version_info(self):
                 refresh()  # The fake git at path1 has a different version now.
                 self.assertEqual(new_git.version_info, (22, 222, 2))
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason="""Name "git" won't find .bat/.cmd (need shim, custom name, or shell)""",
-        raises=GitCommandNotFound,
-    )
-    def test_successful_refresh_in_default_scenario_invalidates_cached_version(self):
+    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()
-            path2.rename(path2.with_stem("git"))
+            path2.rename(path2.with_stem("git"))  # "Install" git, "late" in the PATH.
             refresh()
             self.assertEqual(new_git.version_info, (22, 222, 2), 'before "downgrade"')
-            path1.rename(path1.with_stem("git"))
+            path1.rename(path1.with_stem("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")

From 29406626f985c93404ef283f4e5d922e5bf96978 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 00:26:58 -0500
Subject: [PATCH 12/19] Test that version_info is not pickled

That is, that its value is not pickle serialized/deserialized, but
always recomputed with a subprocess call the first time it is
accessed even on a Git instance that is created by unpickling.
---
 test/test_git.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/test_git.py b/test/test_git.py
index 80316f8f8..cfccc0c48 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -533,6 +533,18 @@ def test_version_info_cache_is_per_instance(self):
                     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:

From e3e568760f7ee002921de292dd98564fdedf6126 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 01:34:32 -0500
Subject: [PATCH 13/19] Fix tests for Python <3.9 which lack Path.with_stem

---
 test/test_git.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/test/test_git.py b/test/test_git.py
index cfccc0c48..70c90c2bc 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -87,6 +87,13 @@ def _fake_git(*version_info):
         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
@@ -647,10 +654,10 @@ def test_successful_default_refresh_invalidates_cached_version_info(self):
                 stack.enter_context(mock.patch.object(Git, "USE_SHELL", True))
 
             new_git = Git()
-            path2.rename(path2.with_stem("git"))  # "Install" git, "late" in the PATH.
+            _rename_with_stem(path2, "git")  # "Install" git, "late" in the PATH.
             refresh()
             self.assertEqual(new_git.version_info, (22, 222, 2), 'before "downgrade"')
-            path1.rename(path1.with_stem("git"))  # "Install" another, higher priority.
+            _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")

From 24b065e4920e11b10fb6230abcc137e2f49eb8f3 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 01:11:41 -0500
Subject: [PATCH 14/19] Invalidate all cached version_info on refresh

For #1829.
---
 git/cmd.py | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/git/cmd.py b/git/cmd.py
index e5edb4959..c1b69da91 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -306,12 +306,18 @@ class Git:
         "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"(.+)::.+")
 
@@ -358,6 +364,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__)."""
@@ -370,7 +378,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
@@ -399,6 +409,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
@@ -782,8 +793,11 @@ def __init__(self, working_dir: Union[None, PathLike] = None):
         # Extra environment variables to pass to git commands
         self._environment: Dict[str, str] = {}
 
-        # Cached command slots
+        # Cached version slots
         self._version_info: Union[Tuple[int, int, int, 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
 
@@ -824,7 +838,11 @@ def version_info(self) -> Tuple[int, int, int, int]:
 
             This value is generated on demand and is cached.
         """
-        if self._version_info is None:
+        refresh_token = self._refresh_token  # Copy it, in case of a concurrent refresh.
+
+        # Ask git for its version if we haven't done so since the last refresh.
+        # (Refreshing is global, but version information caching is per-instance.)
+        if self._version_info_token is not refresh_token:
             # 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]
@@ -833,6 +851,7 @@ def version_info(self) -> Tuple[int, int, int, int]:
                 Tuple[int, int, int, int],
                 tuple(int(n) for n in version_numbers.split(".")[:4] if n.isdigit()),
             )
+            self._version_info_token = refresh_token
 
         return self._version_info
 

From 82b0a1e0681a662d5d418277292cf73fd26cae92 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 11:49:49 -0500
Subject: [PATCH 15/19] Clarify comments; add assertion (helps mypy)

---
 git/cmd.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git/cmd.py b/git/cmd.py
index c1b69da91..9e5713956 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -838,10 +838,11 @@ def version_info(self) -> Tuple[int, int, int, int]:
 
             This value is generated on demand and is cached.
         """
-        refresh_token = self._refresh_token  # Copy it, in case of a concurrent refresh.
+        # Use a copy of this global state, in case of a concurrent refresh.
+        refresh_token = self._refresh_token
 
         # Ask git for its version if we haven't done so since the last refresh.
-        # (Refreshing is global, but version information caching is per-instance.)
+        # (Refreshing is global, but version_info caching is per-instance.)
         if self._version_info_token is not refresh_token:
             # 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.
@@ -853,6 +854,7 @@ def version_info(self) -> Tuple[int, int, int, int]:
             )
             self._version_info_token = refresh_token
 
+        assert self._version_info is not None, "Bug: token check should never let None through"
         return self._version_info
 
     @overload

From eb438ee63add2376e885890489cd5e7484cc8a9d Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 12:29:55 -0500
Subject: [PATCH 16/19] Refactor and further clarify comments

+ Put assertion only on the branch where mypy benefits from it.
---
 git/cmd.py | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/git/cmd.py b/git/cmd.py
index 9e5713956..a1b3c9e45 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -833,28 +833,29 @@ def working_dir(self) -> Union[None, PathLike]:
     @property
     def version_info(self) -> Tuple[int, int, int, 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(int, int, int, int) tuple with integers representing the major,
+            minor and additional version numbers as parsed from git version.
 
             This value is generated on demand and is cached.
         """
-        # Use a copy of this global state, in case of a concurrent refresh.
-        refresh_token = self._refresh_token
-
-        # Ask git for its version if we haven't done so since the last refresh.
-        # (Refreshing is global, but version_info caching is per-instance.)
-        if self._version_info_token is not refresh_token:
-            # 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()),
-            )
-            self._version_info_token = refresh_token
+        # 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
+
+        # 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()),
+        )
+        self._version_info_token = refresh_token
 
-        assert self._version_info is not None, "Bug: token check should never let None through"
         return self._version_info
 
     @overload

From dc6b90fa5a3617c490e95aeaf37fb80cfd17a215 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 15:20:23 -0500
Subject: [PATCH 17/19] Fix version_info type hint + more refactoring

This broadens the type annoation on the Git.version_info property,
for #1830.

It also revises the docstring for clarity, and continues
refactoring and comment changes (also for clarity).
---
 git/cmd.py | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/git/cmd.py b/git/cmd.py
index a1b3c9e45..f534c6673 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -794,7 +794,7 @@ def __init__(self, working_dir: Union[None, PathLike] = None):
         self._environment: Dict[str, str] = {}
 
         # Cached version slots
-        self._version_info: Union[Tuple[int, int, int, int], None] = None
+        self._version_info: Union[Tuple[int, ...], None] = None
         self._version_info_token: object = None
 
         # Cached command slots
@@ -831,10 +831,10 @@ def working_dir(self) -> Union[None, PathLike]:
         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.
         """
@@ -846,16 +846,14 @@ def version_info(self) -> Tuple[int, int, int, int]:
             assert self._version_info is not None, "Bug: corrupted token-check state"
             return self._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]
+        # Run "git version" and parse it.
+        process_version = self._call_process("version")
+        version_string = process_version.split(" ")[2]
+        version_fields = version_string.split(".")[:4]
+        self._version_info = tuple(int(n) for n in version_fields if n.isdigit())
 
-        self._version_info = cast(
-            Tuple[int, int, int, int],
-            tuple(int(n) for n in version_numbers.split(".")[:4] if n.isdigit()),
-        )
+        # This value will be considered valid until the next refresh.
         self._version_info_token = refresh_token
-
         return self._version_info
 
     @overload

From ac20325133649876749a12c2f611dbf66f5bc17c Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 15:41:11 -0500
Subject: [PATCH 18/19] Test version_info parsing

This modifies two existing test cases to include an assertion about
the length. These test cases are retained as there is value in
testing on output from a real git command rather than only with
test doubles.

More importantly, this also adds a parameterized test method to
check parsing of:

- All numeric, shorter than the limit - all fields used.
- All numeric, at the limit - all fields used.
- All numeric, longer than the limit - extra fields dropped.
- Has unambiguous non-numeric - dropped from there on.
- Has ambiguous non-numeric, negative int - dropped from there on.
- Has ambiguous non-numeric, number+letter - dropped from there on.

The cases for parsing when a field is not numeric (or not fully or
unambiguously numeric) currently all fail, because the existing
logic drops intermediate non-numeric fields (#1833).

Parsing should instead stop at (or, *perhaps* in cases like "2a",
after) such fields. When the code is changed to stop at them
rather than dropping them and presenting the subsequent field as
though it were a previous field, these test cases should also pass.
---
 test/test_git.py | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/test/test_git.py b/test/test_git.py
index 70c90c2bc..97e21cad4 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -338,9 +338,10 @@ def test_persistent_cat_file_command(self):
         self.assertEqual(size, size_two)
 
     def test_version_info(self):
-        """The version_info attribute is a tuple of ints."""
+        """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)
 
@@ -349,9 +350,26 @@ def test_version_info_pickleable(self):
         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")
 

From 629fd87f46a91baccb298bef6fc34ac60ecc5727 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 22 Feb 2024 16:25:03 -0500
Subject: [PATCH 19/19] Fix how version_info omits non-numeric fields

For #1833.

This makes version_info parsing stop including fields after the
first non-numeric field, rather than skipping non-numeric fields
and including subsequent numeric fields that then wrongly appear to
have the original position and significance of the dropped field.

This actually stops at (rather than merely after) a non-numeric
field, i.e., potentially parseable fields that are not fully
numeric, such as "2a", are stopped at, rather than parsed as "2".
---
 git/cmd.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git/cmd.py b/git/cmd.py
index f534c6673..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
@@ -850,7 +851,8 @@ def version_info(self) -> Tuple[int, ...]:
         process_version = self._call_process("version")
         version_string = process_version.split(" ")[2]
         version_fields = version_string.split(".")[:4]
-        self._version_info = tuple(int(n) for n in version_fields if n.isdigit())
+        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