Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make the rmtree callback Windows-only #1739

Merged
merged 6 commits into from
Nov 15, 2023
Merged

Make the rmtree callback Windows-only #1739

merged 6 commits into from
Nov 15, 2023

Commits on Nov 14, 2023

  1. Test that rmtree doesn't chmod outside the tree 

    This adds a test in test_util that reveals the bug where
git.util.rmtree will change the permissions on files outside the
tree being removed, if the tree being removed contains a symlink to
a file outside the tree, and the symlink is in a (sub)directory
whose own permissions prevent the symlink itself from being
removed.

The new test failure shows how git.util.rmtree currently calls
os.chmod in a way that dereferences symlinks, including those that
point from inside the tree being deleted to outside it.

Another similar demonstration is temporarily included in the
perm.sh script. That script served as scratchwork for writing the
unit test, and it can be removed soon (while keeping the test).
    @EliahKagan
    EliahKagan committed Nov 14, 2023
    Configuration menu
    Copy the full SHA
    5335416 View commit details
    Browse the repository at this point in the history

  2. One approach to the symlink-following bug

    @EliahKagan
    EliahKagan committed Nov 14, 2023
    Configuration menu
    Copy the full SHA
    fd78a53 View commit details
    Browse the repository at this point in the history

  3. Refactor current fix for symlink-following bug 

    This keeps the same approach, but expresses it better. This does
not update the tests (yet), so one test is still failing.
    @EliahKagan
    EliahKagan committed Nov 14, 2023
    Configuration menu
    Copy the full SHA
    6de8e67 View commit details
    Browse the repository at this point in the history

  4. Test that PermissionError is only wrapped on Windows 

    This changes tests in test_util to verify the opposite behavior
from what was enforced before, in the unusual case (that hopefully
never happens outside of monkey-patching in test_util.py itself)
where the system is not Windows but HIDE_WINDOWS_KNOWN_ERRORS is
set to True.

The same-named environment variable will not, and never has, set
HIDE_WINDOWS_KNOWN_ERRORS to True on non-Windows systems, but it is
possible to set it to True directly. Since it is named as a
constant and no documentation has ever suggested changing its value
directly, nor otherwise attempting to use it outside Windows, it
shouldn't matter what happens in this unusual case. But asserting
that wrapping never occurs in this combination of circumstances is
what makes the most sense in light of the recent change to pass no
callback to shutil.rmtree on non-Windows systems.
    @EliahKagan
    EliahKagan committed Nov 14, 2023
    Configuration menu
    Copy the full SHA
    e8cfae8 View commit details
    Browse the repository at this point in the history

  5. Extract some shared patching code to a helper 

    Because of how it is parameterized, but only in some of the test
cases that use it, making this a pytest fixture would be cumbersome
without further refactoring, so this is a helper method instead.

(It may be feasible to further refactor the test suite to improve
this more.)

This also removes a type annotation from a test method. It may
eventually be helpful to add annotations, in which case that would
come back along with others (if that test still exists in this
form), but it's the only test with such an annotation in this
test class, and the annotation had been added accidentally.
    @EliahKagan
    EliahKagan committed Nov 14, 2023
    Configuration menu
    Copy the full SHA
    8a22352 View commit details
    Browse the repository at this point in the history

  6. Remove demonstration script 

    This removes the perm.sh script that demonstrated the cross-tree
permission change bug. Tests for test_util test for this (in case
of a regression) and the bug is fixed, so the script, which served
to inform the writing of the corresponding unit test, is no longer
needed.
    @EliahKagan
    EliahKagan committed Nov 14, 2023
    Configuration menu
    Copy the full SHA
    b226f3d View commit details
    Browse the repository at this point in the history