René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"git.util.rmtree's callback wraps too many exceptions","articleBody":"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.\r\n\r\n`git.util.rmtree` handles errors with this callback, which wraps them in `unittest.SkipTest`, and which explicitly reports them *as `PermissionError`*:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/8107cbfeb1d56b2587d347c7f3d509ebef079950/git/util.py#L185-L196\r\n\r\nThe 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`.\r\n\r\n**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.\r\n\r\nEven 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.\r\n\r\nI 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.\r\n\r\nI've included proposed such a fix for this in #1700 (which also would fix #1698).","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-10-09T11:14:25.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":0},"url":"https://github.com/1699/GitPython/issues/1699"}