From d74c10431b5938f9b4c08250879e85f422051060 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 30 Oct 2026 10:43:34 -0400 Subject: [PATCH 1/2] test(util): cover `rmfile` deleting a read-only file `git.util.rmfile` exists so that a read-only file can be deleted on Windows, where `os.remove` fails on files with the read-only attribute. On Windows it calls `os.chmod` to clear that attribute before removing the file. Nothing tested this. Before this change, the only callers of `rmfile` were `LockFile._release_lock` and a lock-file helper in `test/test_repo.py`, and no test called either of them on a read-only file. Add `TestRmfile.test_deletes_readonly_file`, alongside the analogous `TestRmtree.test_deletes_dir_with_readonly_files`. Its initial state comes from a new fixture script, `tests/fixtures/rmfile.sh`. The script creates a file named `readonly` and removes its write permissions with `chmod a-w`, as `tests/fixtures/cleanup.sh` does for its read-only case. On Windows this should give the file the read-only attribute. The test uses a copy of the cached fixture output, which keeps the file's mode, so it needs no `execute=True`. The script is separate from `tests/fixtures/rmtree.sh`, whose cases are keyed by `TestRmtree` test names through the `tmp_path` fixture in `test/test_util.py`, and which that fixture runs in place for all but one of them. `rmfile` silently does nothing if the path is not a file, so the test would pass vacuously if the fixture made no file. It therefore first checks that the file exists and is not owner-writable, and only then that `rmfile` removes it. On Windows, `os.stat` reports the read-only attribute as the absence of write bits, so this also confirms that the fixture produced the condition the test is about, rather than assuming it. The test runs on every platform, but it is only meaningful on native Windows. Unlike the `rmtree` test, it is not skipped on Cygwin. That skip's reason, that Cygwin can't set the needed permissions, hasn't been checked for this test. On Cygwin `sys.platform` is `"cygwin"`, so `rmfile` uses no special handling there, and the test only checks that a read-only file can be removed, as on Unix. This adds coverage ahead of changing the mode `rmfile` passes to `os.chmod`: the test passes with the current `0o777`, and on Windows it would fail if the mode lacked the owner-write bit. Validation: on macOS with Python 3.12, the four `TestRmtree` and `TestRmfile` tests pass, the copied fixture file has mode `0o444`, and the precondition fails when the path does not exist. `shellcheck` passes on the new script. Neither Windows nor Cygwin was run locally. The `import-cygwin` workflow does not run the test suite, so Cygwin is not tested in CI either. Co-authored-by: Claude Opus 5.5 --- test/test_util.py | 16 ++++++++++++++++ tests/fixtures/rmfile.sh | 4 ++++ 2 files changed, 20 insertions(+) create mode 100644 tests/fixtures/rmfile.sh diff --git a/test/test_util.py b/test/test_util.py index bfde35aea..ab233feeb 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -41,6 +41,7 @@ is_cygwin_git, join_path, remove_password_if_present, + rmfile, rmtree, stream_copy, ) @@ -213,6 +214,21 @@ def preen_dir2(): assert old_mode == new_mode, f"Should stay {old_mode:#o}, became {new_mode:#o}." +class TestRmfile: + """Tests for :func:`git.util.rmfile`.""" + + def test_deletes_readonly_file(self): + # Automatically works on Unix, but requires special handling on Windows. + with writable_fixture("rmfile.sh") as temporary: + file = temporary.path().resolve() / "readonly" + # rmfile ignores missing paths, so check that the fixture made a read-only file. + assert file.is_file() + assert not file.stat().st_mode & stat.S_IWUSR + + rmfile(file) + assert not file.exists() + + def _xfail_param(*values, **xfail_kwargs): """Build a pytest.mark.parametrize parameter that carries an xfail mark.""" return pytest.param(*values, marks=pytest.mark.xfail(**xfail_kwargs)) diff --git a/tests/fixtures/rmfile.sh b/tests/fixtures/rmfile.sh new file mode 100644 index 000000000..147315e3a --- /dev/null +++ b/tests/fixtures/rmfile.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -eu +touch readonly +chmod a-w readonly From 53f3c6213ea2893acfdfa65715dd7bb64ce1eed0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 30 Oct 2026 10:43:53 -0400 Subject: [PATCH 2/2] refactor(util): clear read-only in `rmfile` with `stat.S_IWUSR` On Windows, `git.util.rmfile` calls `os.chmod(path, 0o777)` before `os.remove` so that read-only files can be deleted. With the CodeQL `security-extended` suite now enabled for this repository, that call is reported as `py/overly-permissive-file` ("Overly permissive mask in chmod sets file to world writable"), at high severity. The finding is a false positive in effect. On Windows, `os.chmod` can only set or clear the read-only attribute, and the Python documentation says "All other bits are ignored". CPython's implementation confirms this. Despite its name, `Modules/posixmodule.c` is also the Windows implementation: `os` takes its functions from a built-in module that is `posix` on Unix and `nt` on Windows, and both are compiled from that file, with the Windows code under `MS_WINDOWS`. There, `chmod` tests only `mode & _S_IWRITE`, clearing `FILE_ATTRIBUTE_READONLY` when that bit is set and setting it otherwise. This is the same in 3.8, the oldest Python GitPython supports, as in `main`, including the newer handle-based path. `_S_IWRITE` is `0o200`, the same value as `stat.S_IWUSR` (and `stat.S_IWRITE`). The call is only reached when `sys.platform == "win32"`, so every bit in `0o777` except `0o200` was always ignored. Pass `stat.S_IWUSR` instead. On Windows this behaves identically: it clears the read-only attribute and does nothing else. It states the intent exactly, and it matches the `os.chmod(path, stat.S_IWUSR)` already used for the same purpose by the `rmtree` error handler a few lines above, which CodeQL does not flag. Behavior is unchanged on all platforms, so no test can fail before this change and pass after it. `TestRmfile.test_deletes_readonly_file` covers the behavior being preserved, and on Windows would fail if the mode lacked the owner-write bit. Validation: on macOS with Python 3.12, the `TestRmtree` and `TestRmfile` tests pass both before and after this change, and `ruff check` and `ruff format --check` pass. Windows, where the changed line actually runs, was not run locally and is left to CI. Co-authored-by: Claude Opus 5.5 --- git/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index 8e77533fb..46891ec30 100644 --- a/git/util.py +++ b/git/util.py @@ -211,7 +211,7 @@ def rmfile(path: PathLike) -> None: treatment.""" if osp.isfile(path): if sys.platform == "win32": - os.chmod(path, 0o777) + os.chmod(path, stat.S_IWUSR) os.remove(path)