git.util.rmtree's callback wraps too many exceptions

[EliahKagan]

As noted in #790, really no exceptions should be wrapped in unittest.SkipTest in production, but there is a more specific problem with what exceptions are wrapped, which this issue is about. This issue could be fixed together with #1698 or separately, but the two are independent.

git.util.rmtree handles errors with this callback, which wraps them in unittest.SkipTest, and which explicitly reports them as PermissionError:

GitPython/git/util.py

Lines 185 to 196 in 8107cbf

	def onerror(func: Callable, path: PathLike, exc_info: str) -> None:
	# 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:
	if HIDE_WINDOWS_KNOWN_ERRORS:
	from unittest import SkipTest
	raise SkipTest("FIXME: fails with: PermissionError\n {}".format(ex)) from ex
	raise

The original logic for that was introduced in be44602. It occurs multiple times, but this is actually redundant because they all call rmtree from git.util, so the code in the callback defined and used in git.util.rmtree is sufficient. The goal--considering the context in which it was introduced, the code comments, and the explicit message--is to convert PermissionError to SkipTest.

But it catches the much more general exception type Exception. Besides PermissionError, there are some other subclasses of OSError whose instances can be raised by shutil.rmtree, such as FileNotFoundError. More surprisingly, I have found that TypeError can be raised if directory traversal is itself what fails, due to a function used in opening the directory not supporting being called with just a path.

Even if this logic were only operational when the project's tests are running, which is not the case, I would take the view that extra exceptions should not be caught and wrapped. However, there is a more confusing effect: no matter what exception is wrapped, the message claims it is a PermissionError, since that name is hard-coded.

I think this should be fixed by having the callback catch PermissionError instead of Exception, which I think is what anyone who is relying on the existing of wrapping exceptions in SkipTest is assuming. (I have found that this change does not cause any fewer tests in the test suite to fail on native Windows systems, at least when running them in Python 3.12 on my WIndows 10 system.) The duplication of that logic can be eliminated at the same time. With the same rationale as in #1698, I think it makes sense to fix this now, rather than deferring it to when a full fix for #790 can be implemented. In addition, and unlike in #1698, a fix for this would somewhat decrease the impact of #790.

I've included proposed such a fix for this in #1700 (which also would fix #1698).