René's URL Explorer Experiment

Title: Make the rmtree callback Windows-only by EliahKagan · Pull Request #1739 · gitpython-developers/GitPython · GitHub

Open Graph Title: Make the rmtree callback Windows-only by EliahKagan · Pull Request #1739 · gitpython-developers/GitPython

X Title: Make the rmtree callback Windows-only by EliahKagan · Pull Request #1739 · gitpython-developers/GitPython

Description: Fixes #1738 Changes Added a test for #1738. Fixed the bug. Updated other tests of rmtree accordingly and improves how they are organized. In addition to failing before the fix and passing afterwards, the test also is run on Windows, where it always passed and continues to pass after the fix, providing further assurance that Windows was never affected. The bug itself, as well as the high-level considerations in fixing it, why this approach is not a breaking change, and why I took this approach rather than doing something else, are presented in #1738, and I think need not be repeated here. There are a few considerations pertaining to the specific code changes here that I think are worth calling attention to briefly: The entire callback is used only on Windows now, since there remains nothing in it that should ever be done on any other system. Because of this, setting HIDE_WINDOWS_KNOWN_ERRORS to True on a non-Windows system, which should never be done (and has fortunately never happened as a result of the same-named environment variable being set), no longer affects the behavior of rmtree. I would consider this a benefit, and I've adjusted the tests for it accordingly. This is part of the gradual improvement and reduction of Windows-specific logic in git.util.rmtree, and reduction in the effect of HIDE_WINDOWS_KNOWN_ERRORS. It is thus a small further step toward #790. The reasoning that this is not a breaking change is based on what the os.chmod call does and does not do. But this also makes a failure not retry deletion (since there is nothing it would do to improve the situation for a retry). One can sometimes relinquish the rest of one's time slice with time.sleep(0) or similar to try to let another process release a resource, which making an ineffective system call and retrying might be a much less reliable way of sometimes doing. But for deleting files that should not be necessary or helpful outside Windows, which didn't have the bug and whose behavior is not changed. Thus I don't think an ineffective system call and a retry can help at all. So this is not a breaking change by that mechanism either. (Separately, and on a separate branch, I did try os.sleep(0) to see if it alleviated the PermissionError problem on Windows that is related to files being open by another process; it did not.) I've taken the opportunity to make some further improvements to other tests of git.util.rmtree in test_util.py. I think they are now somewhat more readable and a bit easier to modify further. We are still testing somewhat more Windows-related behavior on non-Windows systems than I think will ultimately be necessary--due to some previous additions I had made there--but I'm reluctant to try and pare that down until there are Windows CI job. My original motivation for looking at that code, which led to discovering the bug, was to simplify the situation around HIDE_WINDOWS_KNOWN_ERRORS so it would be easier for me to reason about its interaction with and significance to the near-future Windows CI jobs. So this may be considered a step toward that as well.

Open Graph Description: Fixes #1738 Changes Added a test for #1738. Fixed the bug. Updated other tests of rmtree accordingly and improves how they are organized. In addition to failing before the fix and passing afterwa...

X Description: Fixes #1738 Changes Added a test for #1738. Fixed the bug. Updated other tests of rmtree accordingly and improves how they are organized. In addition to failing before the fix and passing afterwa...

Opengraph URL: https://github.com/gitpython-developers/GitPython/pull/1739

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width