Skip to content

Commit 3b9bb6d

Browse files
Byroncodex
andcommitted
review
- [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 <codex@openai.com>
1 parent 76150aa commit 3b9bb6d

3 files changed

Lines changed: 95 additions & 11 deletions

File tree

‎git/util.py‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1270,8 +1270,8 @@ class BlockingLockFile(LockFile):
12701270
specified timeout.
12711271
12721272
:note:
1273-
If the directory containing the lock was removed, an exception will be raised
1274-
during the blocking period, preventing hangs as the lock can never be obtained.
1273+
Only an existing lock causes retries. Other errors, including a removed or
1274+
unwritable parent directory, are raised immediately.
12751275
"""
12761276

12771277
__slots__ = ("_check_interval", "_max_block_time")
@@ -1297,7 +1297,7 @@ def __init__(
12971297

12981298
def _obtain_lock(self) -> None:
12991299
"""This method blocks until it obtained the lock, or raises :exc:`IOError` if it
1300-
ran out of time or if the parent directory was not available anymore.
1300+
ran out of time or lock creation failed for a reason other than an existing lock.
13011301
13021302
If this method returns, you are guaranteed to own the lock.
13031303
"""
@@ -1307,8 +1307,12 @@ def _obtain_lock(self) -> None:
13071307
try:
13081308
super()._obtain_lock()
13091309
except OSError as e:
1310-
# synity check: if the directory leading to the lockfile is not
1311-
# readable anymore, raise an exception
1310+
# LockFile retains the creation error as the cause of its OSError.
1311+
# Only an existing lock is contention; other errors must not be retried.
1312+
if not isinstance(e.__cause__, FileExistsError):
1313+
raise
1314+
1315+
# Abort if the directory containing the existing lock has disappeared.
13121316
curtime = time.time()
13131317
if not osp.isdir(osp.dirname(self._lock_file_path()) or osp.curdir):
13141318
msg = "Directory containing the lockfile %r was not readable anymore after waiting %g seconds" % (

‎test/test_config.py‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# This module is part of GitPython and is released under the
44
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
55

6+
import errno
67
import glob
78
import io
89
import os
@@ -17,7 +18,7 @@
1718
from git import GitConfigParser, Repo
1819
from git.compat import defenc
1920
from git.config import _OMD, cp
20-
from git.util import cwd, rmfile
21+
from git.util import BlockingLockFile, cwd, rmfile
2122
from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory
2223

2324
_tc_lock_fpaths = osp.join(osp.dirname(__file__), "fixtures/*.lock")
@@ -28,6 +29,23 @@ def _rm_lock_files():
2829
rmfile(lfp)
2930

3031

32+
@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows")
33+
@pytest.mark.parametrize("error_number", [errno.EACCES, errno.ENOENT])
34+
def test_blocking_config_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number):
35+
monkeypatch.chdir(tmp_path)
36+
monkeypatch.setattr(GitConfigParser, "t_lock", BlockingLockFile)
37+
error = OSError(error_number, os.strerror(error_number), "config.lock")
38+
with mock.patch.object(os, "open", side_effect=error) as open_lock:
39+
with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep:
40+
with pytest.raises(OSError) as exc_info:
41+
GitConfigParser("config", read_only=False)
42+
sleep.assert_not_called()
43+
open_lock.assert_called_once()
44+
assert exc_info.value.__cause__ is error
45+
assert str(exc_info.value) == str(error)
46+
assert not (tmp_path / "config.lock").exists()
47+
48+
3149
class TestBase(TestCase):
3250
def setUp(self):
3351
_rm_lock_files()

‎test/test_util.py‎

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
55

66
import ast
7-
from datetime import datetime
7+
import errno
88
import os
99
import pathlib
1010
import pickle
@@ -14,6 +14,7 @@
1414
import tempfile
1515
import threading
1616
import time
17+
from datetime import datetime
1718
from unittest import SkipTest, mock
1819

1920
import ddt
@@ -37,12 +38,11 @@
3738
LockFile,
3839
cygpath,
3940
decygpath,
40-
is_cygwin_git,
4141
get_user_id,
42+
is_cygwin_git,
4243
remove_password_if_present,
4344
rmtree,
4445
)
45-
4646
from test.lib import TestBase, requires_symlinks, with_rw_repo
4747

4848

@@ -917,5 +917,67 @@ def test_blocking_relative_lock_reports_timeout(tmp_path, monkeypatch, path_type
917917

918918
def test_blocking_lock_still_rejects_missing_directory(tmp_path):
919919
waiter = BlockingLockFile(tmp_path / "missing" / "resource")
920-
with pytest.raises(OSError, match="Directory containing the lockfile"):
921-
waiter._obtain_lock()
920+
with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep:
921+
with pytest.raises(OSError) as exc_info:
922+
waiter._obtain_lock()
923+
sleep.assert_not_called()
924+
assert isinstance(exc_info.value.__cause__, FileNotFoundError)
925+
assert not waiter._has_lock()
926+
927+
928+
@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows")
929+
@pytest.mark.parametrize(
930+
"error_number", [errno.EACCES, errno.ENOENT, errno.ENOTDIR, errno.EROFS, errno.ENOSPC, errno.EIO]
931+
)
932+
@pytest.mark.parametrize("path_type", [str, pathlib.Path])
933+
@pytest.mark.parametrize("relative", [True, False])
934+
def test_blocking_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number, path_type, relative):
935+
monkeypatch.chdir(tmp_path)
936+
path = path_type("resource" if relative else tmp_path / "resource")
937+
waiter = BlockingLockFile(path)
938+
error = OSError(error_number, os.strerror(error_number), waiter._lock_file_path())
939+
with mock.patch.object(os, "open", side_effect=error) as open_lock:
940+
with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep:
941+
with pytest.raises(OSError) as exc_info:
942+
waiter._obtain_lock()
943+
sleep.assert_not_called()
944+
open_lock.assert_called_once()
945+
assert exc_info.value.__cause__ is error
946+
assert str(exc_info.value) == str(error)
947+
assert not waiter._has_lock()
948+
assert not os.path.lexists(waiter._lock_file_path())
949+
950+
951+
@pytest.mark.skipif(os.name != "posix", reason="Directory write permissions require POSIX")
952+
@pytest.mark.parametrize("path_type", [str, pathlib.Path])
953+
def test_blocking_relative_lock_rejects_unwritable_cwd(tmp_path, monkeypatch, path_type):
954+
monkeypatch.chdir(tmp_path)
955+
original_mode = tmp_path.stat().st_mode
956+
tmp_path.chmod(stat.S_IRUSR | stat.S_IXUSR)
957+
try:
958+
if os.access(".", os.W_OK):
959+
pytest.skip("Current user can write to read-only directories")
960+
assert os.path.isdir(".")
961+
waiter = BlockingLockFile(path_type("resource"))
962+
with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep:
963+
with pytest.raises(OSError) as exc_info:
964+
waiter._obtain_lock()
965+
sleep.assert_not_called()
966+
assert isinstance(exc_info.value.__cause__, PermissionError)
967+
assert not waiter._has_lock()
968+
finally:
969+
tmp_path.chmod(original_mode)
970+
971+
972+
@pytest.mark.skipif(os.name != "posix", reason="Removing the current directory requires POSIX")
973+
@pytest.mark.parametrize("path_type", [str, pathlib.Path])
974+
def test_blocking_relative_lock_rejects_removed_cwd(tmp_path, monkeypatch, path_type):
975+
monkeypatch.chdir(tmp_path)
976+
tmp_path.rmdir()
977+
waiter = BlockingLockFile(path_type("resource"))
978+
with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep:
979+
with pytest.raises(OSError) as exc_info:
980+
waiter._obtain_lock()
981+
sleep.assert_not_called()
982+
assert isinstance(exc_info.value.__cause__, FileNotFoundError)
983+
assert not waiter._has_lock()

0 commit comments

Comments
 (0)