From 76150aae9810e0ab8c9d99d0391639a2c73cb97c Mon Sep 17 00:00:00 2001 From: fan wu <336798967+FanWu-ai@users.noreply.github.com> Date: Sun, 11 Oct 2026 11:08:58 -0700 Subject: [PATCH 1/2] fix(util): wait for contended locks with relative filenames `BlockingLockFile('resource')` incorrectly treats the empty `dirname` of `resource.lock` as a missing directory when another holder owns the lock. Use `os.curdir` for that case so the retry and timeout paths work normally. Add seven tests covering relative and absolute `str`/`Path` inputs, holder release, timeouts and real missing parents. The complete Linux/Python 3.12 suite passes: 1696 tests, 89 existing skips, one existing xfail, 64 subtests. Pinned hooks, mypy, basedpyright, docs, builds and strict Twine also pass. Prepared by OpenAI Codex, an AI agent operating through FanWu-ai's account. --- git/util.py | 2 +- test/test_util.py | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index a694f4fe9..17a32c01a 100644 --- a/git/util.py +++ b/git/util.py @@ -1310,7 +1310,7 @@ def _obtain_lock(self) -> None: # synity check: if the directory leading to the lockfile is not # readable anymore, raise an exception curtime = time.time() - if not osp.isdir(osp.dirname(self._lock_file_path())): + if not osp.isdir(osp.dirname(self._lock_file_path()) or osp.curdir): msg = "Directory containing the lockfile %r was not readable anymore after waiting %g seconds" % ( self._lock_file_path(), curtime - starttime, diff --git a/test/test_util.py b/test/test_util.py index d759343f0..7fe507b8f 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -882,3 +882,40 @@ def test_iterable_list_name_lookup_ignores_list_attributes(name, prefix): assert name not in values with pytest.raises(IndexError, match="No item found with id"): values[name] + + +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +@pytest.mark.parametrize("relative", [True, False]) +def test_blocking_lock_waits_for_existing_holder(tmp_path, monkeypatch, path_type, relative): + monkeypatch.chdir(tmp_path) + path = path_type("resource" if relative else tmp_path / "resource") + owner = LockFile(path) + waiter = BlockingLockFile(path, check_interval_s=0.01, max_block_time_s=1) + owner._obtain_lock() + try: + with mock.patch.object(time, "sleep", side_effect=lambda _: owner._release_lock()) as sleep: + waiter._obtain_lock() + sleep.assert_called_once_with(0.01) + assert waiter._has_lock() + finally: + owner._release_lock() + waiter._release_lock() + + +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_reports_timeout(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + owner = LockFile(path_type("resource")) + waiter = BlockingLockFile(path_type("resource"), max_block_time_s=0) + owner._obtain_lock() + try: + with pytest.raises(OSError, match="Waited .* seconds for lock"): + waiter._obtain_lock() + finally: + owner._release_lock() + + +def test_blocking_lock_still_rejects_missing_directory(tmp_path): + waiter = BlockingLockFile(tmp_path / "missing" / "resource") + with pytest.raises(OSError, match="Directory containing the lockfile"): + waiter._obtain_lock() From 3b9bb6df3c3caca808e93f77e3eac7b97d24c571 Mon Sep 17 00:00:00 2001 From: Byron Date: Wed, 14 Oct 2026 09:47:31 +0200 Subject: [PATCH 2/2] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [P2] Propagate non-contention errors before retrying relative locks — git/util.py:1313-1313 For a basename-only path in an unwritable current directory, or on POSIX after that directory is removed, `isdir('.')` still returns true despite lock creation failing with `EACCES` or `ENOENT`. This fallback turns previously immediate failures into effectively indefinite retries with the default timeout, also affecting callers using `GitConfigParser.t_lock = BlockingLockFile`. Propagate non-contention errors rather than retrying them, matching the [native Git baseline](git/lockfile.c#L234-L235), which retries only `EEXIST`. Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol --- git/util.py | 14 +++++---- test/test_config.py | 20 ++++++++++++- test/test_util.py | 72 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 95 insertions(+), 11 deletions(-) diff --git a/git/util.py b/git/util.py index 17a32c01a..874b764d5 100644 --- a/git/util.py +++ b/git/util.py @@ -1270,8 +1270,8 @@ class BlockingLockFile(LockFile): specified timeout. :note: - If the directory containing the lock was removed, an exception will be raised - during the blocking period, preventing hangs as the lock can never be obtained. + Only an existing lock causes retries. Other errors, including a removed or + unwritable parent directory, are raised immediately. """ __slots__ = ("_check_interval", "_max_block_time") @@ -1297,7 +1297,7 @@ def __init__( def _obtain_lock(self) -> None: """This method blocks until it obtained the lock, or raises :exc:`IOError` if it - ran out of time or if the parent directory was not available anymore. + ran out of time or lock creation failed for a reason other than an existing lock. If this method returns, you are guaranteed to own the lock. """ @@ -1307,8 +1307,12 @@ def _obtain_lock(self) -> None: try: super()._obtain_lock() except OSError as e: - # synity check: if the directory leading to the lockfile is not - # readable anymore, raise an exception + # LockFile retains the creation error as the cause of its OSError. + # Only an existing lock is contention; other errors must not be retried. + if not isinstance(e.__cause__, FileExistsError): + raise + + # Abort if the directory containing the existing lock has disappeared. curtime = time.time() if not osp.isdir(osp.dirname(self._lock_file_path()) or osp.curdir): msg = "Directory containing the lockfile %r was not readable anymore after waiting %g seconds" % ( diff --git a/test/test_config.py b/test/test_config.py index 8d526457b..e3c30421c 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -3,6 +3,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +import errno import glob import io import os @@ -17,7 +18,7 @@ from git import GitConfigParser, Repo from git.compat import defenc from git.config import _OMD, cp -from git.util import cwd, rmfile +from git.util import BlockingLockFile, cwd, rmfile from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory _tc_lock_fpaths = osp.join(osp.dirname(__file__), "fixtures/*.lock") @@ -28,6 +29,23 @@ def _rm_lock_files(): rmfile(lfp) +@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows") +@pytest.mark.parametrize("error_number", [errno.EACCES, errno.ENOENT]) +def test_blocking_config_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number): + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(GitConfigParser, "t_lock", BlockingLockFile) + error = OSError(error_number, os.strerror(error_number), "config.lock") + with mock.patch.object(os, "open", side_effect=error) as open_lock: + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + GitConfigParser("config", read_only=False) + sleep.assert_not_called() + open_lock.assert_called_once() + assert exc_info.value.__cause__ is error + assert str(exc_info.value) == str(error) + assert not (tmp_path / "config.lock").exists() + + class TestBase(TestCase): def setUp(self): _rm_lock_files() diff --git a/test/test_util.py b/test/test_util.py index 7fe507b8f..ca592aaf7 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -4,7 +4,7 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ import ast -from datetime import datetime +import errno import os import pathlib import pickle @@ -14,6 +14,7 @@ import tempfile import threading import time +from datetime import datetime from unittest import SkipTest, mock import ddt @@ -37,12 +38,11 @@ LockFile, cygpath, decygpath, - is_cygwin_git, get_user_id, + is_cygwin_git, remove_password_if_present, rmtree, ) - from test.lib import TestBase, requires_symlinks, with_rw_repo @@ -917,5 +917,67 @@ def test_blocking_relative_lock_reports_timeout(tmp_path, monkeypatch, path_type def test_blocking_lock_still_rejects_missing_directory(tmp_path): waiter = BlockingLockFile(tmp_path / "missing" / "resource") - with pytest.raises(OSError, match="Directory containing the lockfile"): - waiter._obtain_lock() + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + assert not waiter._has_lock() + + +@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows") +@pytest.mark.parametrize( + "error_number", [errno.EACCES, errno.ENOENT, errno.ENOTDIR, errno.EROFS, errno.ENOSPC, errno.EIO] +) +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +@pytest.mark.parametrize("relative", [True, False]) +def test_blocking_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number, path_type, relative): + monkeypatch.chdir(tmp_path) + path = path_type("resource" if relative else tmp_path / "resource") + waiter = BlockingLockFile(path) + error = OSError(error_number, os.strerror(error_number), waiter._lock_file_path()) + with mock.patch.object(os, "open", side_effect=error) as open_lock: + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + open_lock.assert_called_once() + assert exc_info.value.__cause__ is error + assert str(exc_info.value) == str(error) + assert not waiter._has_lock() + assert not os.path.lexists(waiter._lock_file_path()) + + +@pytest.mark.skipif(os.name != "posix", reason="Directory write permissions require POSIX") +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_rejects_unwritable_cwd(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + original_mode = tmp_path.stat().st_mode + tmp_path.chmod(stat.S_IRUSR | stat.S_IXUSR) + try: + if os.access(".", os.W_OK): + pytest.skip("Current user can write to read-only directories") + assert os.path.isdir(".") + waiter = BlockingLockFile(path_type("resource")) + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, PermissionError) + assert not waiter._has_lock() + finally: + tmp_path.chmod(original_mode) + + +@pytest.mark.skipif(os.name != "posix", reason="Removing the current directory requires POSIX") +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_rejects_removed_cwd(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + tmp_path.rmdir() + waiter = BlockingLockFile(path_type("resource")) + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + assert not waiter._has_lock()