Fix bugs affecting exception wrapping in rmtree callback by EliahKagan · Pull Request #1700 · gitpython-developers/GitPython

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.

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.

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.

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

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.

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.

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.

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

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

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

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.

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.

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.

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.

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.