Merge pull request #1700 from EliahKagan/untangle-skips

Fix bugs affecting exception wrapping in rmtree callback

Author: Byron

git/objects/submodule/base.py

@@ -1,44 +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,
 )
-from git.util import HIDE_WINDOWS_KNOWN_ERRORS
-
-import os.path as osp
 
 from .util import (
+    SubmoduleConfigParser,
+    find_first_remote_branch,
     mkhead,
     sm_name,
     sm_section,
-    SubmoduleConfigParser,
-    find_first_remote_branch,
 )
 
 
@@ -1060,28 +1053,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
 

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
@@ -116,14 +109,33 @@
 
 log = logging.getLogger(__name__)
 
-# types############################################################
+
+def _read_env_flag(name: str, default: bool) -> bool:
+    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,
+    )
+
+    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,
 #: 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
 
@@ -177,25 +189,29 @@ 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 onerror(func: Callable, path: PathLike, exc_info: str) -> None:
-        # Is the error an access error ?
+    def handler(function: Callable, path: PathLike, _excinfo: Any) -> None:
+        """Callback for :func:`shutil.rmtree`. Works either as ``onexc`` or ``onerror``."""
+        # Is the error an access error?
         os.chmod(path, stat.S_IWUSR)
 
         try:
-            func(path)  # Will scream if still not possible to delete.
-        except Exception as ex:
+            function(path)
+        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)
+    if sys.version_info >= (3, 12):
+        shutil.rmtree(path, onexc=handler)
+    else:
+        shutil.rmtree(path, onerror=handler)
 
 
 def rmfile(path: PathLike) -> None:
@@ -995,7 +1011,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
 

test-requirements.txt

@@ -7,4 +7,5 @@ pre-commit
 pytest
 pytest-cov
 pytest-instafail
+pytest-subtests
 pytest-sugar

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.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 1062.
+    #
+    # @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

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 1011.
+    #
+    # @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
@@ -819,9 +822,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"))