From 28142630f6c3bfd83dcd654c102235c5c00075e5 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 01:47:38 -0400
Subject: [PATCH 01/19] Add a missing PermissionError xfail on Windows

One of the tests that was commented as being skipped as a result of
SkipTest rasied in git.util.rmtree or one of the functions that
calls it, test_git_submodule_compatibility, was not skipped in that
way and was actually failing on Windows with PermissionError. It
appears that the cause of failure changed over time, so that it
once involved rmtree but no longer does.

This removes the outdated comment and adds an xfail mark instead,
specific to PermissionError and with a message identifying where in
the test case's logic the PermissionError is currently triggered.
---
 test/test_submodule.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/test_submodule.py b/test/test_submodule.py
index 79ff2c5f2..0ebd8c51d 100644
--- a/test/test_submodule.py
+++ b/test/test_submodule.py
@@ -819,9 +819,11 @@ def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
         assert commit_sm.binsha == sm_too.binsha
         assert sm_too.binsha != sm.binsha
 
-    # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,  ## ACTUALLY skipped by `git.submodule.base#L869`.
-    #         "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
-    #         "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx")  # noqa E501
+    @pytest.mark.xfail(
+        HIDE_WINDOWS_KNOWN_ERRORS,
+        reason='"The process cannot access the file because it is being used by another process" on call to sm.move',
+        raises=PermissionError,
+    )
     @with_rw_directory
     def test_git_submodule_compatibility(self, rwdir):
         parent = git.Repo.init(osp.join(rwdir, "parent"))

From fba59aa32119e22b1b300fea8959c0abd3c9f863 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 04:39:56 -0400
Subject: [PATCH 02/19] Update "ACTUALLY skipped by" comments

The remaining "ACTUALLY skipped by" comments in the test suite were
for tests that are actually skipped by SkipTest exceptions raised
from the code under test. But the information provided about where
in the code they were skipped was out of date, and also not
detailed enough because references to line numbers become stale
when code is added or removed in the referenced module before the
referenced code.

This updates them and also provides more detailed information about
the referenced code doing the skipping.

The error messages are the same as before, and the paths are the
same in relevant details, so this doesn't modify those parts of the
comments.
---
 test/test_docs.py      | 5 ++++-
 test/test_submodule.py | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/test/test_docs.py b/test/test_docs.py
index 79e1f1be4..b38ac31b0 100644
--- a/test/test_docs.py
+++ b/test/test_docs.py
@@ -21,7 +21,10 @@ def tearDown(self):
 
         gc.collect()
 
-    # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,  ## ACTUALLY skipped by `git.submodule.base#L869`.
+    # ACTUALLY skipped by git.objects.submodule.base.Submodule.remove, at the last
+    # rmtree call (in "handle separate bare repository"), line 1082.
+    #
+    # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
     #         "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx")  # noqa E501
     @with_rw_directory
diff --git a/test/test_submodule.py b/test/test_submodule.py
index 0ebd8c51d..1c105c816 100644
--- a/test/test_submodule.py
+++ b/test/test_submodule.py
@@ -457,7 +457,10 @@ def _do_base_tests(self, rwrepo):
             True,
         )
 
-    # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,  ## ACTUALLY skipped by `git.submodule.base#L869`.
+    # ACTUALLY skipped by git.util.rmtree (in local onerror function), called via
+    # git.objects.submodule.base.Submodule.remove at "method(mp)", line 1018.
+    #
+    # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: fails with: PermissionError: [WinError 32] The process cannot access the file because"
     #         "it is being used by another process: "
     #         "'C:\\Users\\ankostis\\AppData\\Local\\Temp\\tmp95c3z83bnon_bare_test_base_rw\\git\\ext\\gitdb\\gitdb\\ext\\smmap'")  # noqa E501

From 5039df3560d321af1746bbecbeb1b2838daf7f91 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 06:31:27 -0400
Subject: [PATCH 03/19] Eliminate duplicate rmtree try-except logic

In git.util.rmtree, exceptions are caught and conditionally
(depending on the value of HIDE_WINDOWS_KNOWN_ERRORS) reraised
wrapped in a unittest.SkipTest exception. Although this logic is
part of git.util.rmtree itself, two of the calls to that rmtree
function contain this same logic.

This is not quite a refactoring: because SkipTest derives from
Exception, and Exception rather than PermissionError is being
caught including in the duplicated logic, duplicated logic where
git.util.rmtree was called added another layer of indirection in
the chained exceptions leading back to the original that was raised
in an unsuccessful attempt to delete a file or directory in rmtree.
However, that appeared unintended and may be considered a minor
bug. The new behavior, differing only subtly, is preferable.
---
 git/objects/submodule/base.py | 20 ++------------------
 test/test_docs.py             |  4 ++--
 test/test_submodule.py        |  2 +-
 3 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
index c7e7856f0..6fe946084 100644
--- a/git/objects/submodule/base.py
+++ b/git/objects/submodule/base.py
@@ -29,7 +29,6 @@
     unbare_repo,
     IterableList,
 )
-from git.util import HIDE_WINDOWS_KNOWN_ERRORS
 
 import os.path as osp
 
@@ -1060,28 +1059,13 @@ def remove(
                     import gc
 
                     gc.collect()
-                    try:
-                        rmtree(str(wtd))
-                    except Exception as ex:
-                        if HIDE_WINDOWS_KNOWN_ERRORS:
-                            from unittest import SkipTest
-
-                            raise SkipTest("FIXME: fails with: PermissionError\n  {}".format(ex)) from ex
-                        raise
+                    rmtree(str(wtd))
                 # END delete tree if possible
             # END handle force
 
             if not dry_run and osp.isdir(git_dir):
                 self._clear_cache()
-                try:
-                    rmtree(git_dir)
-                except Exception as ex:
-                    if HIDE_WINDOWS_KNOWN_ERRORS:
-                        from unittest import SkipTest
-
-                        raise SkipTest(f"FIXME: fails with: PermissionError\n  {ex}") from ex
-                    else:
-                        raise
+                rmtree(git_dir)
             # end handle separate bare repository
         # END handle module deletion
 
diff --git a/test/test_docs.py b/test/test_docs.py
index b38ac31b0..f17538aeb 100644
--- a/test/test_docs.py
+++ b/test/test_docs.py
@@ -21,8 +21,8 @@ def tearDown(self):
 
         gc.collect()
 
-    # ACTUALLY skipped by git.objects.submodule.base.Submodule.remove, at the last
-    # rmtree call (in "handle separate bare repository"), line 1082.
+    # ACTUALLY skipped by git.util.rmtree (in local onerror function), from the last call to it via
+    # git.objects.submodule.base.Submodule.remove (at "handle separate bare repository"), line 1068.
     #
     # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
diff --git a/test/test_submodule.py b/test/test_submodule.py
index 1c105c816..318a5afde 100644
--- a/test/test_submodule.py
+++ b/test/test_submodule.py
@@ -458,7 +458,7 @@ def _do_base_tests(self, rwrepo):
         )
 
     # ACTUALLY skipped by git.util.rmtree (in local onerror function), called via
-    # git.objects.submodule.base.Submodule.remove at "method(mp)", line 1018.
+    # git.objects.submodule.base.Submodule.remove at "method(mp)", line 1017.
     #
     # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: fails with: PermissionError: [WinError 32] The process cannot access the file because"

From 683a3eeba838bb786bb1f334c963deb8e13eed0f Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 13:18:09 -0400
Subject: [PATCH 04/19] Clean up git.objects.submodule.base imports

This reorders them lexicographically within each group, makes
spacing/formatting more consistent, and removes the old comment
about needing a dict to set .name, which had originally been on
what later became the BytesIO import but had become separate from
it. (In Python 2, there was a cStringIO type, which could provide a
speed advantage over StringIO, but its instances, not having
instance dictionaries, didn't support the dynamic creation of new
attributes. This was changed to StringIO in 00ce31a to allow .name
to be added. It was changed to BytesIO in bc8c912 to work with
bytes on both Python 2 and Python 3. The comment about needing a
dict later ended up on the preceding line in 0210e39, at which
point its meaning was unclear. Because Python 2 is no longer
supported and Python 3 has no cStringIO type, the comment is no
longer needed, and this commit removes it.)
---
 git/objects/submodule/base.py | 24 +++++++++---------------
 test/test_docs.py             |  2 +-
 test/test_submodule.py        |  2 +-
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
index 6fe946084..13d897263 100644
--- a/git/objects/submodule/base.py
+++ b/git/objects/submodule/base.py
@@ -1,43 +1,37 @@
-# need a dict to set bloody .name field
 from io import BytesIO
 import logging
 import os
+import os.path as osp
 import stat
 import uuid
 
 import git
 from git.cmd import Git
-from git.compat import (
-    defenc,
-    is_win,
-)
-from git.config import SectionConstraint, GitConfigParser, cp
+from git.compat import defenc, is_win
+from git.config import GitConfigParser, SectionConstraint, cp
 from git.exc import (
+    BadName,
     InvalidGitRepositoryError,
     NoSuchPathError,
     RepositoryDirtyError,
-    BadName,
 )
 from git.objects.base import IndexObject, Object
 from git.objects.util import TraversableIterableObj
-
 from git.util import (
-    join_path_native,
-    to_native_path_linux,
+    IterableList,
     RemoteProgress,
+    join_path_native,
     rmtree,
+    to_native_path_linux,
     unbare_repo,
-    IterableList,
 )
 
-import os.path as osp
-
 from .util import (
+    SubmoduleConfigParser,
+    find_first_remote_branch,
     mkhead,
     sm_name,
     sm_section,
-    SubmoduleConfigParser,
-    find_first_remote_branch,
 )
 
 
diff --git a/test/test_docs.py b/test/test_docs.py
index f17538aeb..d1ed46926 100644
--- a/test/test_docs.py
+++ b/test/test_docs.py
@@ -22,7 +22,7 @@ def tearDown(self):
         gc.collect()
 
     # ACTUALLY skipped by git.util.rmtree (in local onerror function), from the last call to it via
-    # git.objects.submodule.base.Submodule.remove (at "handle separate bare repository"), line 1068.
+    # git.objects.submodule.base.Submodule.remove (at "handle separate bare repository"), line 1062.
     #
     # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
diff --git a/test/test_submodule.py b/test/test_submodule.py
index 318a5afde..31a555ce2 100644
--- a/test/test_submodule.py
+++ b/test/test_submodule.py
@@ -458,7 +458,7 @@ def _do_base_tests(self, rwrepo):
         )
 
     # ACTUALLY skipped by git.util.rmtree (in local onerror function), called via
-    # git.objects.submodule.base.Submodule.remove at "method(mp)", line 1017.
+    # git.objects.submodule.base.Submodule.remove at "method(mp)", line 1011.
     #
     # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: fails with: PermissionError: [WinError 32] The process cannot access the file because"

From 2fe7f3c4a6f9870bb332761740c883a2c2ff2487 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 22:46:43 -0400
Subject: [PATCH 05/19] Test current expected behavior of git.util.rmtree

---
 test/test_util.py | 70 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 7 deletions(-)

diff --git a/test/test_util.py b/test/test_util.py
index 2b1e518ed..552700c98 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -5,11 +5,13 @@
 # the BSD License: https://opensource.org/license/bsd-3-clause/
 
 import os
+import pathlib
 import pickle
+import stat
 import sys
 import tempfile
 import time
-from unittest import mock, skipUnless
+from unittest import SkipTest, mock, skipIf, skipUnless
 from datetime import datetime
 
 import ddt
@@ -19,25 +21,26 @@
 from git.compat import is_win
 from git.objects.util import (
     altz_to_utctz_str,
-    utctz_to_altz,
-    verify_utctz,
+    from_timestamp,
     parse_date,
     tzoffset,
-    from_timestamp,
+    utctz_to_altz,
+    verify_utctz,
 )
 from test.lib import (
     TestBase,
     with_rw_repo,
 )
 from git.util import (
-    LockFile,
-    BlockingLockFile,
-    get_user_id,
     Actor,
+    BlockingLockFile,
     IterableList,
+    LockFile,
     cygpath,
     decygpath,
+    get_user_id,
     remove_password_if_present,
+    rmtree,
 )
 
 
@@ -85,6 +88,59 @@ def setup(self):
             "array": [42],
         }
 
+    def test_rmtree_deletes_nested_dir_with_files(self):
+        with tempfile.TemporaryDirectory() as parent:
+            td = pathlib.Path(parent, "testdir")
+            for d in td, td / "q", td / "s":
+                d.mkdir()
+            for f in td / "p", td / "q" / "w", td / "q" / "x", td / "r", td / "s" / "y", td / "s" / "z":
+                f.write_bytes(b"")
+
+            try:
+                rmtree(td)
+            except SkipTest as ex:
+                self.fail(f"rmtree unexpectedly attempts skip: {ex!r}")
+
+            self.assertFalse(td.exists())
+
+    @skipIf(sys.platform == "cygwin", "Cygwin can't set the permissions that make the test meaningful.")
+    def test_rmtree_deletes_dir_with_readonly_files(self):
+        # Automatically works on Unix, but requires special handling on Windows.
+        with tempfile.TemporaryDirectory() as parent:
+            td = pathlib.Path(parent, "testdir")
+            for d in td, td / "sub":
+                d.mkdir()
+            for f in td / "x", td / "sub" / "y":
+                f.write_bytes(b"")
+                f.chmod(0)
+
+            try:
+                rmtree(td)
+            except SkipTest as ex:
+                self.fail(f"rmtree unexpectedly attempts skip: {ex!r}")
+
+            self.assertFalse(td.exists())
+
+    @skipIf(sys.platform == "cygwin", "Cygwin can't set the permissions that make the test meaningful.")
+    @skipIf(sys.version_info < (3, 8), "In 3.7, TemporaryDirectory doesn't clean up after weird permissions.")
+    def test_rmtree_can_wrap_exceptions(self):
+        with tempfile.TemporaryDirectory() as parent:
+            td = pathlib.Path(parent, "testdir")
+            td.mkdir()
+            (td / "x").write_bytes(b"")
+            (td / "x").chmod(stat.S_IRUSR)  # Set up PermissionError on Windows.
+            td.chmod(stat.S_IRUSR | stat.S_IXUSR)  # Set up PermissionError on Unix.
+
+            # Access the module through sys.modules so it is unambiguous which module's
+            # attribute we patch: the original git.util, not git.index.util even though
+            # git.index.util "replaces" git.util and is what "import git.util" gives us.
+            with mock.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", True):
+                # Disable common chmod functions so the callback can't fix the problem.
+                with mock.patch.object(os, "chmod"), mock.patch.object(pathlib.Path, "chmod"):
+                    # Now we can see how an intractable PermissionError is treated.
+                    with self.assertRaises(SkipTest):
+                        rmtree(td)
+
     # FIXME: Mark only the /proc-prefixing cases xfail, somehow (or fix them).
     @pytest.mark.xfail(
         reason="Many return paths prefixed /proc/cygdrive instead.",

From d42cd721112d748c35d0abd11ba8dfc71052e864 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Mon, 9 Oct 2023 01:45:57 -0400
Subject: [PATCH 06/19] Test situations git.util.rmtree shouldn't wrap

One of the new test cases fails, showing the bug where
git.util.rmtree wraps any exception in SkipTest when
HIDE_WINDOWS_KNOWN_ERRORS is true, even though the message it uses
(and its purpose) is specific to PermissionError.

The other new cases pass, because wrapping exceptions in SkipTest
rightly does not occur when HIDE_WINDOWS_KNOWN_ERRORS is false.
---
 test/test_util.py | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/test/test_util.py b/test/test_util.py
index 552700c98..a852eb975 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -4,6 +4,7 @@
 # This module is part of GitPython and is released under
 # the BSD License: https://opensource.org/license/bsd-3-clause/
 
+import contextlib
 import os
 import pathlib
 import pickle
@@ -121,16 +122,31 @@ def test_rmtree_deletes_dir_with_readonly_files(self):
 
             self.assertFalse(td.exists())
 
-    @skipIf(sys.platform == "cygwin", "Cygwin can't set the permissions that make the test meaningful.")
-    @skipIf(sys.version_info < (3, 8), "In 3.7, TemporaryDirectory doesn't clean up after weird permissions.")
-    def test_rmtree_can_wrap_exceptions(self):
+    @staticmethod
+    @contextlib.contextmanager
+    def _tmpdir_to_force_permission_error():
+        if sys.platform == "cygwin":
+            raise SkipTest("Cygwin can't set the permissions that make the test meaningful.")
+        if sys.version_info < (3, 8):
+            raise SkipTest("In 3.7, TemporaryDirectory doesn't clean up after weird permissions.")
+
         with tempfile.TemporaryDirectory() as parent:
             td = pathlib.Path(parent, "testdir")
             td.mkdir()
             (td / "x").write_bytes(b"")
             (td / "x").chmod(stat.S_IRUSR)  # Set up PermissionError on Windows.
             td.chmod(stat.S_IRUSR | stat.S_IXUSR)  # Set up PermissionError on Unix.
+            yield td
 
+    @staticmethod
+    @contextlib.contextmanager
+    def _tmpdir_for_file_not_found():
+        with tempfile.TemporaryDirectory() as parent:
+            yield pathlib.Path(parent, "testdir")  # It is deliberately never created.
+
+    def test_rmtree_can_wrap_exceptions(self):
+        """Our rmtree wraps PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is true."""
+        with self._tmpdir_to_force_permission_error() as td:
             # Access the module through sys.modules so it is unambiguous which module's
             # attribute we patch: the original git.util, not git.index.util even though
             # git.index.util "replaces" git.util and is what "import git.util" gives us.
@@ -141,6 +157,25 @@ def test_rmtree_can_wrap_exceptions(self):
                     with self.assertRaises(SkipTest):
                         rmtree(td)
 
+    @ddt.data(
+        (False, PermissionError, _tmpdir_to_force_permission_error),
+        (False, FileNotFoundError, _tmpdir_for_file_not_found),
+        (True, FileNotFoundError, _tmpdir_for_file_not_found),
+    )
+    def test_rmtree_does_not_wrap_unless_called_for(self, case):
+        """Our rmtree doesn't wrap non-PermissionError, nor when HIDE_WINDOWS_KNOWN_ERRORS is false."""
+        hide_windows_known_errors, exception_type, tmpdir_context_factory = case
+
+        with tmpdir_context_factory() as td:
+            # See comments in test_rmtree_can_wrap_exceptions regarding the patching done here.
+            with mock.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", hide_windows_known_errors):
+                with mock.patch.object(os, "chmod"), mock.patch.object(pathlib.Path, "chmod"):
+                    with self.assertRaises(exception_type):
+                        try:
+                            rmtree(td)
+                        except SkipTest as ex:
+                            self.fail(f"rmtree unexpectedly attempts skip: {ex!r}")
+
     # FIXME: Mark only the /proc-prefixing cases xfail, somehow (or fix them).
     @pytest.mark.xfail(
         reason="Many return paths prefixed /proc/cygdrive instead.",

From 2a32e25bbd1cb42878928ef57a57100a17366202 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Mon, 9 Oct 2023 02:30:22 -0400
Subject: [PATCH 07/19] Fix test bug that assumed staticmethod callability

staticmethod objects are descriptors that cause the same-named
attribute on a class or its instances to be callable (and the first
argument, representing a class or instance, not to be passed). But
the actual staticmethod objects themselves are only callable
starting in Python 3.10. This reorgnizes the just-added test code
so it no longer wrongly relies on being able to call such objects.
---
 test/test_util.py | 49 +++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/test/test_util.py b/test/test_util.py
index a852eb975..b108dc146 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -80,6 +80,30 @@ def __repr__(self):
         return "TestIterableMember(%r)" % self.name
 
 
+@contextlib.contextmanager
+def _tmpdir_to_force_permission_error():
+    """Context manager to test permission errors in situations where we do not fix them."""
+    if sys.platform == "cygwin":
+        raise SkipTest("Cygwin can't set the permissions that make the test meaningful.")
+    if sys.version_info < (3, 8):
+        raise SkipTest("In 3.7, TemporaryDirectory doesn't clean up after weird permissions.")
+
+    with tempfile.TemporaryDirectory() as parent:
+        td = pathlib.Path(parent, "testdir")
+        td.mkdir()
+        (td / "x").write_bytes(b"")
+        (td / "x").chmod(stat.S_IRUSR)  # Set up PermissionError on Windows.
+        td.chmod(stat.S_IRUSR | stat.S_IXUSR)  # Set up PermissionError on Unix.
+        yield td
+
+
+@contextlib.contextmanager
+def _tmpdir_for_file_not_found():
+    """Context manager to test errors deleting a directory that are not due to permissions."""
+    with tempfile.TemporaryDirectory() as parent:
+        yield pathlib.Path(parent, "testdir")  # It is deliberately never created.
+
+
 @ddt.ddt
 class TestUtils(TestBase):
     def setup(self):
@@ -107,6 +131,7 @@ def test_rmtree_deletes_nested_dir_with_files(self):
     @skipIf(sys.platform == "cygwin", "Cygwin can't set the permissions that make the test meaningful.")
     def test_rmtree_deletes_dir_with_readonly_files(self):
         # Automatically works on Unix, but requires special handling on Windows.
+        # Not to be confused with _tmpdir_to_force_permission_error (which is used below).
         with tempfile.TemporaryDirectory() as parent:
             td = pathlib.Path(parent, "testdir")
             for d in td, td / "sub":
@@ -122,31 +147,9 @@ def test_rmtree_deletes_dir_with_readonly_files(self):
 
             self.assertFalse(td.exists())
 
-    @staticmethod
-    @contextlib.contextmanager
-    def _tmpdir_to_force_permission_error():
-        if sys.platform == "cygwin":
-            raise SkipTest("Cygwin can't set the permissions that make the test meaningful.")
-        if sys.version_info < (3, 8):
-            raise SkipTest("In 3.7, TemporaryDirectory doesn't clean up after weird permissions.")
-
-        with tempfile.TemporaryDirectory() as parent:
-            td = pathlib.Path(parent, "testdir")
-            td.mkdir()
-            (td / "x").write_bytes(b"")
-            (td / "x").chmod(stat.S_IRUSR)  # Set up PermissionError on Windows.
-            td.chmod(stat.S_IRUSR | stat.S_IXUSR)  # Set up PermissionError on Unix.
-            yield td
-
-    @staticmethod
-    @contextlib.contextmanager
-    def _tmpdir_for_file_not_found():
-        with tempfile.TemporaryDirectory() as parent:
-            yield pathlib.Path(parent, "testdir")  # It is deliberately never created.
-
     def test_rmtree_can_wrap_exceptions(self):
         """Our rmtree wraps PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is true."""
-        with self._tmpdir_to_force_permission_error() as td:
+        with _tmpdir_to_force_permission_error() as td:
             # Access the module through sys.modules so it is unambiguous which module's
             # attribute we patch: the original git.util, not git.index.util even though
             # git.index.util "replaces" git.util and is what "import git.util" gives us.

From b8e009e8d31e32a2c0e247e0a7dc41ccdd3556e7 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 13:59:03 -0400
Subject: [PATCH 08/19] In rmtree, have onerror catch only PermissionError

The onerror function is still called on, and tries to resolve, any
exception. But now, when it re-calls the file deletion function
passed as func, the only exception it catches to conditionally
convert to SkipTest is PermissionError (or derived exceptions).

The old behavior of catching Exception was overly broad, and
inconsistent with the hard-coded prefix of "FIXME: fails with:
PermissionError" used to build the SkipTest exception messages.

This commit also changes the message to use an f-string (which was
one of the styles in the equivalent but differently coded duplicate
logic eliminated in 5039df3, and seems clearer in this case). That
change is a pure refactoring, not affecting generated messages.
---
 git/util.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git/util.py b/git/util.py
index 48901ba0c..c0e40d7e6 100644
--- a/git/util.py
+++ b/git/util.py
@@ -188,11 +188,11 @@ def onerror(func: Callable, path: PathLike, exc_info: str) -> None:
 
         try:
             func(path)  # Will scream if still not possible to delete.
-        except Exception as ex:
+        except PermissionError as ex:
             if HIDE_WINDOWS_KNOWN_ERRORS:
                 from unittest import SkipTest
 
-                raise SkipTest("FIXME: fails with: PermissionError\n  {}".format(ex)) from ex
+                raise SkipTest(f"FIXME: fails with: PermissionError\n  {ex}") from ex
             raise
 
     return shutil.rmtree(path, False, onerror)

From ccbb2732efcfa265568f1a535a8b746ed07ed82a Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 18:14:19 -0400
Subject: [PATCH 09/19] Fix onerror callback type hinting, improve style

The onerror parameter of shutil.rmtree receives a 3-tuple like what
sys.exc_info() gives, not a string. Since we are not using that
parameter anyway, I've fixed it in the onerror function defined
in git.util.rmtree by changing it to Any rather than hinting it
narrowly.

I've also renamed the parameters so the names are based on those
that are documented in the shutil.rmtree documentation. The names
are not part of the function's interface (this follows both from
the documentation and the typeshed hints) but using those names may
make it easier to understand the function.

- func is renamed to function.

- path remains path.

- exc_info is renamed to _excinfo. This parameter is documented as
  excinfo, but I've prepended an underscore to signifity that it is
  unused.

These changes are to a local function and non-breaking.

Finally, although not all type checkers will flag this as an error
automatically, the git.util.rmtree function, like the shutil.rmtree
function it calls, is conceptually void, so it should not have any
return statements with operands. Because the return statement
appeared at the end, I've removed "return".
---
 git/util.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git/util.py b/git/util.py
index c0e40d7e6..6e1e95e49 100644
--- a/git/util.py
+++ b/git/util.py
@@ -182,12 +182,12 @@ def rmtree(path: PathLike) -> None:
     :note: we use shutil rmtree but adjust its behaviour to see whether files that
         couldn't be deleted are read-only. Windows will not remove them in that case"""
 
-    def onerror(func: Callable, path: PathLike, exc_info: str) -> None:
+    def onerror(function: Callable, path: PathLike, _excinfo: Any) -> None:
         # Is the error an access error ?
         os.chmod(path, stat.S_IWUSR)
 
         try:
-            func(path)  # Will scream if still not possible to delete.
+            function(path)  # Will scream if still not possible to delete.
         except PermissionError as ex:
             if HIDE_WINDOWS_KNOWN_ERRORS:
                 from unittest import SkipTest
@@ -195,7 +195,7 @@ def onerror(func: Callable, path: PathLike, exc_info: str) -> None:
                 raise SkipTest(f"FIXME: fails with: PermissionError\n  {ex}") from ex
             raise
 
-    return shutil.rmtree(path, False, onerror)
+    shutil.rmtree(path, False, onerror)
 
 
 def rmfile(path: PathLike) -> None:

From 0b88012471d8021fffe61beb9d058840c0235f5d Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 20:16:38 -0400
Subject: [PATCH 10/19] Use onexc callback where supported

The shutil.rmtree callback defined as a local function in
git.util.rmtree was already capable of being used as both the old
onerror parameter and the new onexc parameter--introduced in Python
3.12, which also deprecates onerror--because they differ only in
the meaning of their third parameter (excinfo), which the callback
defined in git.util.rmtree does not use.

This modifies git.util.rmtree to pass it as onexc on 3.12 and
later, while still passing it as onerror on 3.11 and earlier.

Because the default value of ignore_errors is False, this makes the
varying logic clearer by omitting that argument and using a keyword
argument both when passing onexc (which is keyword-only) and when
passing onerror (which is not keyword-only but can only be passed
positionally if ignore_errors is passed explicitly).
---
 git/util.py | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/git/util.py b/git/util.py
index 6e1e95e49..8a4a20253 100644
--- a/git/util.py
+++ b/git/util.py
@@ -5,24 +5,24 @@
 # the BSD License: https://opensource.org/license/bsd-3-clause/
 
 from abc import abstractmethod
-import os.path as osp
-from .compat import is_win
 import contextlib
 from functools import wraps
 import getpass
 import logging
 import os
+import os.path as osp
+import pathlib
 import platform
-import subprocess
 import re
 import shutil
 import stat
-from sys import maxsize
+import subprocess
+import sys
 import time
 from urllib.parse import urlsplit, urlunsplit
 import warnings
 
-# from git.objects.util import Traversable
+from .compat import is_win
 
 # typing ---------------------------------------------------------
 
@@ -42,22 +42,17 @@
     Tuple,
     TypeVar,
     Union,
-    cast,
     TYPE_CHECKING,
+    cast,
     overload,
 )
 
-import pathlib
-
 if TYPE_CHECKING:
     from git.remote import Remote
     from git.repo.base import Repo
     from git.config import GitConfigParser, SectionConstraint
     from git import Git
 
-    # from git.objects.base import IndexObject
-
-
 from .types import (
     Literal,
     SupportsIndex,
@@ -75,7 +70,6 @@
 
 # ---------------------------------------------------------------------
 
-
 from gitdb.util import (  # NOQA @IgnorePep8
     make_sha,
     LockedFD,  # @UnusedImport
@@ -88,7 +82,6 @@
     hex_to_bin,  # @UnusedImport
 )
 
-
 # NOTE:  Some of the unused imports might be used/imported by others.
 # Handle once test-cases are back up and running.
 # Most of these are unused here, but are for use by git-python modules so these
@@ -182,7 +175,7 @@ def rmtree(path: PathLike) -> None:
     :note: we use shutil rmtree but adjust its behaviour to see whether files that
         couldn't be deleted are read-only. Windows will not remove them in that case"""
 
-    def onerror(function: Callable, path: PathLike, _excinfo: Any) -> None:
+    def handler(function: Callable, path: PathLike, _excinfo: Any) -> None:
         # Is the error an access error ?
         os.chmod(path, stat.S_IWUSR)
 
@@ -195,7 +188,10 @@ def onerror(function: Callable, path: PathLike, _excinfo: Any) -> None:
                 raise SkipTest(f"FIXME: fails with: PermissionError\n  {ex}") from ex
             raise
 
-    shutil.rmtree(path, False, onerror)
+    if sys.version_info >= (3, 12):
+        shutil.rmtree(path, onexc=handler)
+    else:
+        shutil.rmtree(path, onerror=handler)
 
 
 def rmfile(path: PathLike) -> None:
@@ -995,7 +991,7 @@ def __init__(
         self,
         file_path: PathLike,
         check_interval_s: float = 0.3,
-        max_block_time_s: int = maxsize,
+        max_block_time_s: int = sys.maxsize,
     ) -> None:
         """Configure the instance
 

From 7dd59043b44d0f2169304f90da74b1b2f7f2b02e Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 8 Oct 2023 20:27:08 -0400
Subject: [PATCH 11/19] Revise and update rmtree docstrings and comments

---
 git/util.py | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/git/util.py b/git/util.py
index 8a4a20253..d7f348d48 100644
--- a/git/util.py
+++ b/git/util.py
@@ -170,17 +170,18 @@ def patch_env(name: str, value: str) -> Generator[None, None, None]:
 
 
 def rmtree(path: PathLike) -> None:
-    """Remove the given recursively.
+    """Remove the given directory tree recursively.
 
-    :note: we use shutil rmtree but adjust its behaviour to see whether files that
-        couldn't be deleted are read-only. Windows will not remove them in that case"""
+    :note: We use :func:`shutil.rmtree` but adjust its behaviour to see whether files that
+        couldn't be deleted are read-only. Windows will not remove them in that case."""
 
     def handler(function: Callable, path: PathLike, _excinfo: Any) -> None:
-        # Is the error an access error ?
+        """Callback for :func:`shutil.rmtree`. Works either as ``onexc`` or ``onerror``."""
+        # Is the error an access error?
         os.chmod(path, stat.S_IWUSR)
 
         try:
-            function(path)  # Will scream if still not possible to delete.
+            function(path)
         except PermissionError as ex:
             if HIDE_WINDOWS_KNOWN_ERRORS:
                 from unittest import SkipTest

From 196cfbeefb5ab0844d37da0b1e010b6ee7cf9041 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Mon, 9 Oct 2023 03:27:58 -0400
Subject: [PATCH 12/19] Clean up test_util, reorganizing for readability

- Slightly improve import sorting, grouping, and formatting.

- Move the cygpath pairs parameters into the test class, so they
  can be immediately above the tests that use them. This was almost
  the case in the past, but stopped being the case when helpers for
  some new tests above those were introduced (and those helpers
  can't be moved inside the class without extra complexity).

- Rename TestIterableMember to _Member, so it is no longer named as
  a test class. The unittest framework wouldn't consider it one,
  since it doesn't derive from unittest.TestCase, but the pytest
  runner, which we're actually using, does. More importanly (since
  it has no test methods anyway), this makes clear to humans that
  it is a helper class for tests, rather than a class of tests.

- Improve the style of _Member, and have its __repr__ show the
  actual class of the instance, so if future tests ever use a
  derived class of it--or if its name ever changes again--the
  type name in the repr will be correct.

- Remove the setup method (of TestUtils). It looks like this may
  at one time have been intended as a setUp method (note the case
  difference), but it is unused and there doesn't seem to be any
  attempt to use the instance attribute it was setting.

- Use R"" instead of r"" for raw strings representing Windows
  paths, so that some editors (at least VS Code) refrain from
  highlighting their contents as regular expressions.

- Other very minor reformatting and slight comment rewording.
---
 test/test_util.py | 101 +++++++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 55 deletions(-)

diff --git a/test/test_util.py b/test/test_util.py
index b108dc146..7b37918d1 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -5,6 +5,7 @@
 # the BSD License: https://opensource.org/license/bsd-3-clause/
 
 import contextlib
+from datetime import datetime
 import os
 import pathlib
 import pickle
@@ -13,7 +14,6 @@
 import tempfile
 import time
 from unittest import SkipTest, mock, skipIf, skipUnless
-from datetime import datetime
 
 import ddt
 import pytest
@@ -28,10 +28,6 @@
     utctz_to_altz,
     verify_utctz,
 )
-from test.lib import (
-    TestBase,
-    with_rw_repo,
-)
 from git.util import (
     Actor,
     BlockingLockFile,
@@ -43,41 +39,19 @@
     remove_password_if_present,
     rmtree,
 )
+from test.lib import TestBase, with_rw_repo
 
 
-_norm_cygpath_pairs = (
-    (r"foo\bar", "foo/bar"),
-    (r"foo/bar", "foo/bar"),
-    (r"C:\Users", "/cygdrive/c/Users"),
-    (r"C:\d/e", "/cygdrive/c/d/e"),
-    ("C:\\", "/cygdrive/c/"),
-    (r"\\server\C$\Users", "//server/C$/Users"),
-    (r"\\server\C$", "//server/C$"),
-    ("\\\\server\\c$\\", "//server/c$/"),
-    (r"\\server\BAR/", "//server/BAR/"),
-    (r"D:/Apps", "/cygdrive/d/Apps"),
-    (r"D:/Apps\fOO", "/cygdrive/d/Apps/fOO"),
-    (r"D:\Apps/123", "/cygdrive/d/Apps/123"),
-)
-
-_unc_cygpath_pairs = (
-    (r"\\?\a:\com", "/cygdrive/a/com"),
-    (r"\\?\a:/com", "/cygdrive/a/com"),
-    (r"\\?\UNC\server\D$\Apps", "//server/D$/Apps"),
-)
-
-
-class TestIterableMember(object):
-
-    """A member of an iterable list"""
+class _Member:
+    """A member of an IterableList."""
 
-    __slots__ = "name"
+    __slots__ = ("name",)
 
     def __init__(self, name):
         self.name = name
 
     def __repr__(self):
-        return "TestIterableMember(%r)" % self.name
+        return f"{type(self).__name__}({self.name!r})"
 
 
 @contextlib.contextmanager
@@ -106,13 +80,6 @@ def _tmpdir_for_file_not_found():
 
 @ddt.ddt
 class TestUtils(TestBase):
-    def setup(self):
-        self.testdict = {
-            "string": "42",
-            "int": 42,
-            "array": [42],
-        }
-
     def test_rmtree_deletes_nested_dir_with_files(self):
         with tempfile.TemporaryDirectory() as parent:
             td = pathlib.Path(parent, "testdir")
@@ -131,7 +98,7 @@ def test_rmtree_deletes_nested_dir_with_files(self):
     @skipIf(sys.platform == "cygwin", "Cygwin can't set the permissions that make the test meaningful.")
     def test_rmtree_deletes_dir_with_readonly_files(self):
         # Automatically works on Unix, but requires special handling on Windows.
-        # Not to be confused with _tmpdir_to_force_permission_error (which is used below).
+        # Not to be confused with what _tmpdir_to_force_permission_error sets up (see below).
         with tempfile.TemporaryDirectory() as parent:
             td = pathlib.Path(parent, "testdir")
             for d in td, td / "sub":
@@ -179,6 +146,27 @@ def test_rmtree_does_not_wrap_unless_called_for(self, case):
                         except SkipTest as ex:
                             self.fail(f"rmtree unexpectedly attempts skip: {ex!r}")
 
+    _norm_cygpath_pairs = (
+        (R"foo\bar", "foo/bar"),
+        (R"foo/bar", "foo/bar"),
+        (R"C:\Users", "/cygdrive/c/Users"),
+        (R"C:\d/e", "/cygdrive/c/d/e"),
+        ("C:\\", "/cygdrive/c/"),
+        (R"\\server\C$\Users", "//server/C$/Users"),
+        (R"\\server\C$", "//server/C$"),
+        ("\\\\server\\c$\\", "//server/c$/"),
+        (R"\\server\BAR/", "//server/BAR/"),
+        (R"D:/Apps", "/cygdrive/d/Apps"),
+        (R"D:/Apps\fOO", "/cygdrive/d/Apps/fOO"),
+        (R"D:\Apps/123", "/cygdrive/d/Apps/123"),
+    )
+
+    _unc_cygpath_pairs = (
+        (R"\\?\a:\com", "/cygdrive/a/com"),
+        (R"\\?\a:/com", "/cygdrive/a/com"),
+        (R"\\?\UNC\server\D$\Apps", "//server/D$/Apps"),
+    )
+
     # FIXME: Mark only the /proc-prefixing cases xfail, somehow (or fix them).
     @pytest.mark.xfail(
         reason="Many return paths prefixed /proc/cygdrive instead.",
@@ -192,16 +180,16 @@ def test_cygpath_ok(self, case):
         self.assertEqual(cwpath, cpath, wpath)
 
     @pytest.mark.xfail(
-        reason=r'2nd example r".\bar" -> "bar" fails, returns "./bar"',
+        reason=R'2nd example r".\bar" -> "bar" fails, returns "./bar"',
         raises=AssertionError,
     )
     @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
     @ddt.data(
-        (r"./bar", "bar"),
-        (r".\bar", "bar"),  # FIXME: Mark only this one xfail, somehow (or fix it).
-        (r"../bar", "../bar"),
-        (r"..\bar", "../bar"),
-        (r"../bar/.\foo/../chu", "../bar/chu"),
+        (R"./bar", "bar"),
+        (R".\bar", "bar"),  # FIXME: Mark only this one xfail, somehow (or fix it).
+        (R"../bar", "../bar"),
+        (R"..\bar", "../bar"),
+        (R"../bar/.\foo/../chu", "../bar/chu"),
     )
     def test_cygpath_norm_ok(self, case):
         wpath, cpath = case
@@ -210,12 +198,12 @@ def test_cygpath_norm_ok(self, case):
 
     @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
     @ddt.data(
-        r"C:",
-        r"C:Relative",
-        r"D:Apps\123",
-        r"D:Apps/123",
-        r"\\?\a:rel",
-        r"\\share\a:rel",
+        R"C:",
+        R"C:Relative",
+        R"D:Apps\123",
+        R"D:Apps/123",
+        R"\\?\a:rel",
+        R"\\share\a:rel",
     )
     def test_cygpath_invalids(self, wpath):
         cwpath = cygpath(wpath)
@@ -380,15 +368,18 @@ def test_actor_from_string(self):
             Actor("name last another", "some-very-long-email@example.com"),
         )
 
-    @ddt.data(("name", ""), ("name", "prefix_"))
+    @ddt.data(
+        ("name", ""),
+        ("name", "prefix_"),
+    )
     def test_iterable_list(self, case):
         name, prefix = case
         ilist = IterableList(name, prefix)
 
         name1 = "one"
         name2 = "two"
-        m1 = TestIterableMember(prefix + name1)
-        m2 = TestIterableMember(prefix + name2)
+        m1 = _Member(prefix + name1)
+        m2 = _Member(prefix + name2)
 
         ilist.extend((m1, m2))
 

From 100ab989fcba0b1d1bd89b5b4b41ea5014da3d82 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sat, 7 Oct 2023 15:31:25 -0400
Subject: [PATCH 13/19] Add initial test_env_vars_for_windows_tests

The new test method just verifies the current behavior of the
HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS
environment variables. This is so there is a test to modify when
changing that behavior. The purpose of these tests is *not* to
claim that the behavior of either variable is something code that
uses GitPython can (or has ever been able to) rely on.

This also adds pytest-subtests as a dependency, so multiple
failures from the subtests can be seen in the same test run.
---
 test-requirements.txt |  1 +
 test/test_util.py     | 45 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/test-requirements.txt b/test-requirements.txt
index 9414da09c..a69181be1 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -7,4 +7,5 @@ pre-commit
 pytest
 pytest-cov
 pytest-instafail
+pytest-subtests
 pytest-sugar
diff --git a/test/test_util.py b/test/test_util.py
index 7b37918d1..d65b25d49 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -4,12 +4,14 @@
 # This module is part of GitPython and is released under
 # the BSD License: https://opensource.org/license/bsd-3-clause/
 
+import ast
 import contextlib
 from datetime import datetime
 import os
 import pathlib
 import pickle
 import stat
+import subprocess
 import sys
 import tempfile
 import time
@@ -502,3 +504,46 @@ def test_remove_password_from_command_line(self):
 
         assert cmd_4 == remove_password_if_present(cmd_4)
         assert cmd_5 == remove_password_if_present(cmd_5)
+
+    @ddt.data("HIDE_WINDOWS_KNOWN_ERRORS", "HIDE_WINDOWS_FREEZE_ERRORS")
+    def test_env_vars_for_windows_tests(self, name):
+        def run_parse(value):
+            command = [
+                sys.executable,
+                "-c",
+                f"from git.util import {name}; print(repr({name}))",
+            ]
+            output = subprocess.check_output(
+                command,
+                env=None if value is None else dict(os.environ, **{name: value}),
+                text=True,
+            )
+            return ast.literal_eval(output)
+
+        assert_true_iff_win = self.assertTrue if os.name == "nt" else self.assertFalse
+
+        truthy_cases = [
+            ("unset", None),
+            ("true-seeming", "1"),
+            ("true-seeming", "true"),
+            ("true-seeming", "True"),
+            ("true-seeming", "yes"),
+            ("true-seeming", "YES"),
+            ("false-seeming", "0"),
+            ("false-seeming", "false"),
+            ("false-seeming", "False"),
+            ("false-seeming", "no"),
+            ("false-seeming", "NO"),
+            ("whitespace", " "),
+        ]
+        falsy_cases = [
+            ("empty", ""),
+        ]
+
+        for msg, env_var_value in truthy_cases:
+            with self.subTest(msg, env_var_value=env_var_value):
+                assert_true_iff_win(run_parse(env_var_value))
+
+        for msg, env_var_value in falsy_cases:
+            with self.subTest(msg, env_var_value=env_var_value):
+                self.assertFalse(run_parse(env_var_value))

From 7604da185ce12b9ef540aff3255580db02c88268 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sat, 7 Oct 2023 16:06:49 -0400
Subject: [PATCH 14/19] Warn if HIDE_WINDOWS_*_ERRORS set in environment

This warns if the HIDE_WINDOWS_KNOWN_ERRORS or
HIDE_WINDOWS_FREEZE_ERRORS environment variables are set. These
behave unexpectedly, including (and especially) in their effect on
the same-named git.util module attributes, and neither their
effects nor those of those attributes are documented in a way that
would have supported code outside the project relying on their
specific semantics.

The new warning message characterizes their status as deprecated.

- This is now the case for HIDE_WINDOWS_KNOWN_ERRORS, and
  almost so for the same-named attribute, whose existence (though
  not its meaning) can technically be relied on due to inclusion in
  `__all__` (which this does *not* change).

- But the HIDE_WINDOWS_FREEZE_ERRORS attribute was never guaranteed
  even to exist, so technically neither it nor the same-named
  environment variable are not *even* deprecated. The attribute's
  presence has never been reflected in the public interface of any
  GitPython component in any way.

However, these attributes are still used by the tests. Furthermore,
in the case of HIDE_WINDOWS_KNOWN_ERRORS, setting it is the only
way to disable the behavior of converting errors from some file
deletion operations into SkipTest exceptions on Windows. Since that
behavior has not yet changed, but is unlikely to be desired outside
of testing, no *attributes* are deprecated at this time, and no
effort to warn from accessing or using attributes is attempted.
---
 git/util.py | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/git/util.py b/git/util.py
index d7f348d48..5ffe93e70 100644
--- a/git/util.py
+++ b/git/util.py
@@ -109,14 +109,28 @@
 
 log = logging.getLogger(__name__)
 
-# types############################################################
+
+def _read_env_flag(name: str, default: bool) -> Union[bool, str]:
+    try:
+        value = os.environ[name]
+    except KeyError:
+        return default
+
+    log.warning(
+        "The %s environment variable is deprecated. Its effect has never been documented and changes without warning.",
+        name,
+    )
+
+    # FIXME: This should always return bool, as that is how it is used.
+    # FIXME: This should treat some values besides "" as expressing falsehood.
+    return value
 
 
 #: We need an easy way to see if Appveyor TCs start failing,
 #: so the errors marked with this var are considered "acknowledged" ones, awaiting remedy,
 #: till then, we wish to hide them.
-HIDE_WINDOWS_KNOWN_ERRORS = is_win and os.environ.get("HIDE_WINDOWS_KNOWN_ERRORS", True)
-HIDE_WINDOWS_FREEZE_ERRORS = is_win and os.environ.get("HIDE_WINDOWS_FREEZE_ERRORS", True)
+HIDE_WINDOWS_KNOWN_ERRORS = is_win and _read_env_flag("HIDE_WINDOWS_KNOWN_ERRORS", True)
+HIDE_WINDOWS_FREEZE_ERRORS = is_win and _read_env_flag("HIDE_WINDOWS_FREEZE_ERRORS", True)
 
 # { Utility Methods
 

From eb51277e73ca274e3948a3f009168d210dd587ca Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sat, 7 Oct 2023 17:01:46 -0400
Subject: [PATCH 15/19] Make HIDE_* attributes always bool

For now, this doesn't change how the correspondng environment
variables are interpreted, in terms of truth and falsehood.
But it does *convert* them to bool, so that the values of the
HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS
attributes are always bools.

It also updates the tests accordingly, to validate this behavior.
---
 git/util.py       | 5 ++---
 test/test_util.py | 6 +++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/git/util.py b/git/util.py
index 5ffe93e70..a6153f1f3 100644
--- a/git/util.py
+++ b/git/util.py
@@ -110,7 +110,7 @@
 log = logging.getLogger(__name__)
 
 
-def _read_env_flag(name: str, default: bool) -> Union[bool, str]:
+def _read_env_flag(name: str, default: bool) -> bool:
     try:
         value = os.environ[name]
     except KeyError:
@@ -121,9 +121,8 @@ def _read_env_flag(name: str, default: bool) -> Union[bool, str]:
         name,
     )
 
-    # FIXME: This should always return bool, as that is how it is used.
     # FIXME: This should treat some values besides "" as expressing falsehood.
-    return value
+    return bool(value)
 
 
 #: We need an easy way to see if Appveyor TCs start failing,
diff --git a/test/test_util.py b/test/test_util.py
index d65b25d49..5912ea4a0 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -520,7 +520,7 @@ def run_parse(value):
             )
             return ast.literal_eval(output)
 
-        assert_true_iff_win = self.assertTrue if os.name == "nt" else self.assertFalse
+        true_iff_win = os.name == "nt"  # Same as is_win, but don't depend on that here.
 
         truthy_cases = [
             ("unset", None),
@@ -542,8 +542,8 @@ def run_parse(value):
 
         for msg, env_var_value in truthy_cases:
             with self.subTest(msg, env_var_value=env_var_value):
-                assert_true_iff_win(run_parse(env_var_value))
+                self.assertIs(run_parse(env_var_value), true_iff_win)
 
         for msg, env_var_value in falsy_cases:
             with self.subTest(msg, env_var_value=env_var_value):
-                self.assertFalse(run_parse(env_var_value))
+                self.assertIs(run_parse(env_var_value), False)

From 333896b4447c56093fa4ae402a3d22491928ce29 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sat, 7 Oct 2023 17:29:04 -0400
Subject: [PATCH 16/19] Treat false-seeming HIDE_* env var values as false

This changes how HIDE_WINDOWS_KNOWN_ERRORS and
HIDE_WINDOWS_FREEZE_ERRORS environment variables, if present, are
interpreted, so that values that strongly seem intuitivley to
represent falsehood now do.

Before, only the empty string was treated as false. Now:

- "0", "false", "no", and their case variants, as well as the empty
  string, are treated as false.

- The presence of leading and trailing whitespace in the value now
  longer changes the truth value it represents. For example,
  all-whitespace (e.g., a space) is treated as false.

- Values other than the above false values, and the recognized true
  values "1", "true", "yes", and their variants, are still treated
  as true, but issue a warning about how they are unrecognied.
---
 git/util.py       | 10 ++++++++--
 test/test_util.py |  8 ++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/git/util.py b/git/util.py
index a6153f1f3..97f461a83 100644
--- a/git/util.py
+++ b/git/util.py
@@ -121,8 +121,14 @@ def _read_env_flag(name: str, default: bool) -> bool:
         name,
     )
 
-    # FIXME: This should treat some values besides "" as expressing falsehood.
-    return bool(value)
+    adjusted_value = value.strip().lower()
+
+    if adjusted_value in {"", "0", "false", "no"}:
+        return False
+    if adjusted_value in {"1", "true", "yes"}:
+        return True
+    log.warning("%s has unrecognized value %r, treating as %r.", name, value, default)
+    return default
 
 
 #: We need an easy way to see if Appveyor TCs start failing,
diff --git a/test/test_util.py b/test/test_util.py
index 5912ea4a0..647a02833 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -529,15 +529,15 @@ def run_parse(value):
             ("true-seeming", "True"),
             ("true-seeming", "yes"),
             ("true-seeming", "YES"),
+        ]
+        falsy_cases = [
+            ("empty", ""),
+            ("whitespace", " "),
             ("false-seeming", "0"),
             ("false-seeming", "false"),
             ("false-seeming", "False"),
             ("false-seeming", "no"),
             ("false-seeming", "NO"),
-            ("whitespace", " "),
-        ]
-        falsy_cases = [
-            ("empty", ""),
         ]
 
         for msg, env_var_value in truthy_cases:

From c11b36660382c713709b36bbca1da8a1acb3a4ec Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sat, 7 Oct 2023 17:52:27 -0400
Subject: [PATCH 17/19] Simplify HIDE_* env var test; add missing cases

Now that the expected truth values are intuitive, it is no longer
necessary to group them by result and include messages that
acknowldge the unintuitive cases. This reorders them so that pairs
(like "yes" and "no") appear together, removes the messages that
are no longer necessary, and reduces test code duplication.

This also adds cases to test leading/trailing whitespace in
otherwise nonempty strings, so it is clearer what the test is
asserting overall, and so a bug where lstrip or rstrip (or
equivalent with a regex) were used instead strip would be caught.
---
 test/test_util.py | 46 +++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/test/test_util.py b/test/test_util.py
index 647a02833..a4c06b80e 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -520,30 +520,22 @@ def run_parse(value):
             )
             return ast.literal_eval(output)
 
-        true_iff_win = os.name == "nt"  # Same as is_win, but don't depend on that here.
-
-        truthy_cases = [
-            ("unset", None),
-            ("true-seeming", "1"),
-            ("true-seeming", "true"),
-            ("true-seeming", "True"),
-            ("true-seeming", "yes"),
-            ("true-seeming", "YES"),
-        ]
-        falsy_cases = [
-            ("empty", ""),
-            ("whitespace", " "),
-            ("false-seeming", "0"),
-            ("false-seeming", "false"),
-            ("false-seeming", "False"),
-            ("false-seeming", "no"),
-            ("false-seeming", "NO"),
-        ]
-
-        for msg, env_var_value in truthy_cases:
-            with self.subTest(msg, env_var_value=env_var_value):
-                self.assertIs(run_parse(env_var_value), true_iff_win)
-
-        for msg, env_var_value in falsy_cases:
-            with self.subTest(msg, env_var_value=env_var_value):
-                self.assertIs(run_parse(env_var_value), False)
+        for env_var_value, expected_truth_value in (
+            (None, os.name == "nt"),  # True on Windows when the environment variable is unset.
+            ("", False),
+            (" ", False),
+            ("0", False),
+            ("1", os.name == "nt"),
+            ("false", False),
+            ("true", os.name == "nt"),
+            ("False", False),
+            ("True", os.name == "nt"),
+            ("no", False),
+            ("yes", os.name == "nt"),
+            ("NO", False),
+            ("YES", os.name == "nt"),
+            (" no  ", False),
+            (" yes  ", os.name == "nt"),
+        ):
+            with self.subTest(env_var_value=env_var_value):
+                self.assertIs(run_parse(env_var_value), expected_truth_value)

From f0e15e8935580a4e4dc4ed6490c9e1b229493e19 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Mon, 9 Oct 2023 04:20:09 -0400
Subject: [PATCH 18/19] Further cleanup in test_util (on new tests)

The main change here is to move the tests of how the HIDE_*
environment variables are treated up near the rmtree tests, since
they although the behaviors being tested are separate, they are
conceptually related, and also not entirely independent in that
they both involve the HIDE_WINDOWS_KNOWN_ERRORS attribute.

Other changes are mostly to formatting.
---
 test/test_util.py | 94 +++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/test/test_util.py b/test/test_util.py
index a4c06b80e..b20657fb1 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -87,7 +87,14 @@ def test_rmtree_deletes_nested_dir_with_files(self):
             td = pathlib.Path(parent, "testdir")
             for d in td, td / "q", td / "s":
                 d.mkdir()
-            for f in td / "p", td / "q" / "w", td / "q" / "x", td / "r", td / "s" / "y", td / "s" / "z":
+            for f in (
+                td / "p",
+                td / "q" / "w",
+                td / "q" / "x",
+                td / "r",
+                td / "s" / "y",
+                td / "s" / "z",
+            ):
                 f.write_bytes(b"")
 
             try:
@@ -97,7 +104,10 @@ def test_rmtree_deletes_nested_dir_with_files(self):
 
             self.assertFalse(td.exists())
 
-    @skipIf(sys.platform == "cygwin", "Cygwin can't set the permissions that make the test meaningful.")
+    @skipIf(
+        sys.platform == "cygwin",
+        "Cygwin can't set the permissions that make the test meaningful.",
+    )
     def test_rmtree_deletes_dir_with_readonly_files(self):
         # Automatically works on Unix, but requires special handling on Windows.
         # Not to be confused with what _tmpdir_to_force_permission_error sets up (see below).
@@ -117,7 +127,7 @@ def test_rmtree_deletes_dir_with_readonly_files(self):
             self.assertFalse(td.exists())
 
     def test_rmtree_can_wrap_exceptions(self):
-        """Our rmtree wraps PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is true."""
+        """rmtree wraps PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is true."""
         with _tmpdir_to_force_permission_error() as td:
             # Access the module through sys.modules so it is unambiguous which module's
             # attribute we patch: the original git.util, not git.index.util even though
@@ -135,12 +145,16 @@ def test_rmtree_can_wrap_exceptions(self):
         (True, FileNotFoundError, _tmpdir_for_file_not_found),
     )
     def test_rmtree_does_not_wrap_unless_called_for(self, case):
-        """Our rmtree doesn't wrap non-PermissionError, nor when HIDE_WINDOWS_KNOWN_ERRORS is false."""
+        """rmtree doesn't wrap non-PermissionError, nor if HIDE_WINDOWS_KNOWN_ERRORS is false."""
         hide_windows_known_errors, exception_type, tmpdir_context_factory = case
 
         with tmpdir_context_factory() as td:
             # See comments in test_rmtree_can_wrap_exceptions regarding the patching done here.
-            with mock.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", hide_windows_known_errors):
+            with mock.patch.object(
+                sys.modules["git.util"],
+                "HIDE_WINDOWS_KNOWN_ERRORS",
+                hide_windows_known_errors,
+            ):
                 with mock.patch.object(os, "chmod"), mock.patch.object(pathlib.Path, "chmod"):
                     with self.assertRaises(exception_type):
                         try:
@@ -148,6 +162,41 @@ def test_rmtree_does_not_wrap_unless_called_for(self, case):
                         except SkipTest as ex:
                             self.fail(f"rmtree unexpectedly attempts skip: {ex!r}")
 
+    @ddt.data("HIDE_WINDOWS_KNOWN_ERRORS", "HIDE_WINDOWS_FREEZE_ERRORS")
+    def test_env_vars_for_windows_tests(self, name):
+        def run_parse(value):
+            command = [
+                sys.executable,
+                "-c",
+                f"from git.util import {name}; print(repr({name}))",
+            ]
+            output = subprocess.check_output(
+                command,
+                env=None if value is None else dict(os.environ, **{name: value}),
+                text=True,
+            )
+            return ast.literal_eval(output)
+
+        for env_var_value, expected_truth_value in (
+            (None, os.name == "nt"),  # True on Windows when the environment variable is unset.
+            ("", False),
+            (" ", False),
+            ("0", False),
+            ("1", os.name == "nt"),
+            ("false", False),
+            ("true", os.name == "nt"),
+            ("False", False),
+            ("True", os.name == "nt"),
+            ("no", False),
+            ("yes", os.name == "nt"),
+            ("NO", False),
+            ("YES", os.name == "nt"),
+            (" no  ", False),
+            (" yes  ", os.name == "nt"),
+        ):
+            with self.subTest(env_var_value=env_var_value):
+                self.assertIs(run_parse(env_var_value), expected_truth_value)
+
     _norm_cygpath_pairs = (
         (R"foo\bar", "foo/bar"),
         (R"foo/bar", "foo/bar"),
@@ -504,38 +553,3 @@ def test_remove_password_from_command_line(self):
 
         assert cmd_4 == remove_password_if_present(cmd_4)
         assert cmd_5 == remove_password_if_present(cmd_5)
-
-    @ddt.data("HIDE_WINDOWS_KNOWN_ERRORS", "HIDE_WINDOWS_FREEZE_ERRORS")
-    def test_env_vars_for_windows_tests(self, name):
-        def run_parse(value):
-            command = [
-                sys.executable,
-                "-c",
-                f"from git.util import {name}; print(repr({name}))",
-            ]
-            output = subprocess.check_output(
-                command,
-                env=None if value is None else dict(os.environ, **{name: value}),
-                text=True,
-            )
-            return ast.literal_eval(output)
-
-        for env_var_value, expected_truth_value in (
-            (None, os.name == "nt"),  # True on Windows when the environment variable is unset.
-            ("", False),
-            (" ", False),
-            ("0", False),
-            ("1", os.name == "nt"),
-            ("false", False),
-            ("true", os.name == "nt"),
-            ("False", False),
-            ("True", os.name == "nt"),
-            ("no", False),
-            ("yes", os.name == "nt"),
-            ("NO", False),
-            ("YES", os.name == "nt"),
-            (" no  ", False),
-            (" yes  ", os.name == "nt"),
-        ):
-            with self.subTest(env_var_value=env_var_value):
-                self.assertIs(run_parse(env_var_value), expected_truth_value)

From a9b05ece674578d3417f8969ade17b06ab287ffe Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Mon, 9 Oct 2023 07:59:32 -0400
Subject: [PATCH 19/19] Clarify a test helper docstring

The wording was ambiguous before, because fixing a PermissionError
could mean addressing it successfully at runtime (which is the
intended meaning in that docstring) but it could also mean fixing
a bug or test case related to such an error (not intended).
---
 test/test_util.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/test_util.py b/test/test_util.py
index b20657fb1..f75231c98 100644
--- a/test/test_util.py
+++ b/test/test_util.py
@@ -58,7 +58,7 @@ def __repr__(self):
 
 @contextlib.contextmanager
 def _tmpdir_to_force_permission_error():
-    """Context manager to test permission errors in situations where we do not fix them."""
+    """Context manager to test permission errors in situations where they are not overcome."""
     if sys.platform == "cygwin":
         raise SkipTest("Cygwin can't set the permissions that make the test meaningful.")
     if sys.version_info < (3, 8):