One of the tests that was commented as being skipped as a result ofSkipTest rasied in git.util.rmtree or one of the functions thatcalls it, test_git_submodule_compatibility, was not skipped in thatway and was actually failing on Windows with PermissionError. Itappears that the cause of failure changed over time, so that itonce 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 inthe test case's logic the PermissionError is currently triggered.

The remaining "ACTUALLY skipped by" comments in the test suite werefor tests that are actually skipped by SkipTest exceptions raisedfrom the code under test. But the information provided about wherein the code they were skipped was out of date, and also notdetailed enough because references to line numbers become stalewhen code is added or removed in the referenced module before thereferenced code.This updates them and also provides more detailed information aboutthe referenced code doing the skipping.The error messages are the same as before, and the paths are thesame in relevant details, so this doesn't modify those parts of thecomments.

In git.util.rmtree, exceptions are caught and conditionally(depending on the value of HIDE_WINDOWS_KNOWN_ERRORS) reraisedwrapped in a unittest.SkipTest exception. Although this logic ispart of git.util.rmtree itself, two of the calls to that rmtreefunction contain this same logic.This is not quite a refactoring: because SkipTest derives fromException, and Exception rather than PermissionError is beingcaught including in the duplicated logic, duplicated logic wheregit.util.rmtree was called added another layer of indirection inthe chained exceptions leading back to the original that was raisedin an unsuccessful attempt to delete a file or directory in rmtree.However, that appeared unintended and may be considered a minorbug. The new behavior, differing only subtly, is preferable.

This reorders them lexicographically within each group, makesspacing/formatting more consistent, and removes the old commentabout needing a dict to set .name, which had originally been onwhat later became the BytesIO import but had become separate fromit. (In Python 2, there was a cStringIO type, which could provide aspeed advantage over StringIO, but its instances, not havinginstance dictionaries, didn't support the dynamic creation of newattributes. This was changed to StringIO in00ce31a to allow .nameto be added. It was changed to BytesIO inbc8c912 to work withbytes on both Python 2 and Python 3. The comment about needing adict later ended up on the preceding line in0210e39, at whichpoint its meaning was unclear. Because Python 2 is no longersupported and Python 3 has no cStringIO type, the comment is nolonger needed, and this commit removes it.)

One of the new test cases fails, showing the bug wheregit.util.rmtree wraps any exception in SkipTest whenHIDE_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 SkipTestrightly does not occur when HIDE_WINDOWS_KNOWN_ERRORS is false.

staticmethod objects are descriptors that cause the same-namedattribute on a class or its instances to be callable (and the firstargument, representing a class or instance, not to be passed). Butthe actual staticmethod objects themselves are only callablestarting in Python 3.10. This reorgnizes the just-added test codeso it no longer wrongly relies on being able to call such objects.

The onerror function is still called on, and tries to resolve, anyexception. But now, when it re-calls the file deletion functionpassed as func, the only exception it catches to conditionallyconvert to SkipTest is PermissionError (or derived exceptions).The old behavior of catching Exception was overly broad, andinconsistent 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 wasone of the styles in the equivalent but differently coded duplicatelogic eliminated in5039df3, and seems clearer in this case). Thatchange is a pure refactoring, not affecting generated messages.

The onerror parameter of shutil.rmtree receives a 3-tuple like whatsys.exc_info() gives, not a string. Since we are not using thatparameter anyway, I've fixed it in the onerror function definedin git.util.rmtree by changing it to Any rather than hinting itnarrowly.I've also renamed the parameters so the names are based on thosethat are documented in the shutil.rmtree documentation. The namesare not part of the function's interface (this follows both fromthe documentation and the typeshed hints) but using those names maymake 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 errorautomatically, the git.util.rmtree function, like the shutil.rmtreefunction it calls, is conceptually void, so it should not have anyreturn statements with operands. Because the return statementappeared at the end, I've removed "return".

The shutil.rmtree callback defined as a local function ingit.util.rmtree was already capable of being used as both the oldonerror parameter and the new onexc parameter--introduced in Python3.12, which also deprecates onerror--because they differ only inthe meaning of their third parameter (excinfo), which the callbackdefined in git.util.rmtree does not use.This modifies git.util.rmtree to pass it as onexc on 3.12 andlater, while still passing it as onerror on 3.11 and earlier.Because the default value of ignore_errors is False, this makes thevarying logic clearer by omitting that argument and using a keywordargument both when passing onexc (which is keyword-only) and whenpassing onerror (which is not keyword-only but can only be passedpositionally if ignore_errors is passed explicitly).

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

The new test method just verifies the current behavior of theHIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORSenvironment variables. This is so there is a test to modify whenchanging that behavior. The purpose of these tests is *not* toclaim that the behavior of either variable is something code thatuses GitPython can (or has ever been able to) rely on.This also adds pytest-subtests as a dependency, so multiplefailures from the subtests can be seen in the same test run.

This warns if the HIDE_WINDOWS_KNOWN_ERRORS orHIDE_WINDOWS_FREEZE_ERRORS environment variables are set. Thesebehave unexpectedly, including (and especially) in their effect onthe same-named git.util module attributes, and neither theireffects nor those of those attributes are documented in a way thatwould have supported code outside the project relying on theirspecific 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 onlyway to disable the behavior of converting errors from some filedeletion operations into SkipTest exceptions on Windows. Since thatbehavior has not yet changed, but is unlikely to be desired outsideof testing, no *attributes* are deprecated at this time, and noeffort to warn from accessing or using attributes is attempted.

For now, this doesn't change how the correspondng environmentvariables are interpreted, in terms of truth and falsehood.But it does *convert* them to bool, so that the values of theHIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORSattributes are always bools.It also updates the tests accordingly, to validate this behavior.

This changes how HIDE_WINDOWS_KNOWN_ERRORS andHIDE_WINDOWS_FREEZE_ERRORS environment variables, if present, areinterpreted, so that values that strongly seem intuitivley torepresent 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.

Now that the expected truth values are intuitive, it is no longernecessary to group them by result and include messages thatacknowldge the unintuitive cases. This reorders them so that pairs(like "yes" and "no") appear together, removes the messages thatare no longer necessary, and reduces test code duplication.This also adds cases to test leading/trailing whitespace inotherwise nonempty strings, so it is clearer what the test isasserting overall, and so a bug where lstrip or rstrip (orequivalent with a regex) were used instead strip would be caught.

The main change here is to move the tests of how the HIDE_*environment variables are treated up near the rmtree tests, sincethey although the behaviors being tested are separate, they areconceptually related, and also not entirely independent in thatthey both involve the HIDE_WINDOWS_KNOWN_ERRORS attribute.Other changes are mostly to formatting.

The wording was ambiguous before, because fixing a PermissionErrorcould mean addressing it successfully at runtime (which is theintended meaning in that docstring) but it could also mean fixinga bug or test case related to such an error (not intended).

Byron left a comment