Skip to content

Commit 2d60fa5

Browse files
codexByron
authored andcommitted
fix(submodule): close repositories owned by update operations
The Windows Python 3.15 CI job failed to remove submodule checkouts because persistent `cat-file` processes still used them as working directories. `Submodule.update()` relied on collection of its temporary `Repo`, but captured log records retained a `Head` argument and therefore the repository and its process. Recursive updates also opened an extra unbounded repository. Close the owned repository after updates and on errors, and reuse it for recursion with final cleanup. This preserves `keep_going` behavior while releasing processes even when logs or callbacks retain repository objects. The compatibility test now scopes its own repository and closes it before removal; allocation tracing identified those separate caller-owned handles. Four regressions retain real logging arguments and verify process cleanup for normal, failing, recursive, and recursive `keep_going` updates. Both previously failing tests pass with tracing asserting no live checkout processes at each removal. Ruff, mypy, basedpyright, and `git diff --check` pass locally. Native Windows validation will run in CI.
1 parent 71e1b1c commit 2d60fa5

2 files changed

Lines changed: 66 additions & 11 deletions

File tree

‎git/objects/submodule/base.py‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1195,17 +1195,19 @@ def fetch_remotes(module_repo: "Repo") -> None:
11951195
)
11961196
# END update to new commit only if needed
11971197
except Exception as err:
1198+
if mrepo is not None:
1199+
mrepo.close()
11981200
if not keep_going:
11991201
raise
12001202
_logger.error(str(err))
12011203
# END handle keep_going
12021204

12031205
# HANDLE RECURSION
12041206
##################
1205-
if recursive:
1207+
try:
12061208
# In dry_run mode, the module might not exist.
1207-
if mrepo is not None:
1208-
for submodule in self.iter_items(self.module()):
1209+
if recursive and mrepo is not None:
1210+
for submodule in self.iter_items(mrepo):
12091211
submodule.update(
12101212
recursive,
12111213
init,
@@ -1218,7 +1220,11 @@ def fetch_remotes(module_repo: "Repo") -> None:
12181220
)
12191221
# END handle recursive update
12201222
# END handle dry run
1221-
# END for each submodule
1223+
finally:
1224+
# Log records and progress callbacks can retain refs to this repository.
1225+
# Its cat-file processes must not keep the checkout open on Windows.
1226+
if mrepo is not None:
1227+
mrepo.close()
12221228

12231229
return self
12241230

‎test/test_submodule.py‎

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,53 @@ def capture_process(self, command, *args, **kwargs):
646646
process.wait()
647647

648648

649+
@pytest.mark.parametrize("operation", ["update", "error", "recursive", "keep-going"])
650+
def test_update_closes_checkout_processes_retained_by_logging(movable_submodule, monkeypatch, caplog, operation):
651+
sm = movable_submodule
652+
recursive = operation in ("recursive", "keep-going")
653+
with sm.module() as module:
654+
module.head.reference.set_tracking_branch(None)
655+
if recursive:
656+
child = module.create_submodule("child", "child", sm.url)
657+
module.index.commit("Add child")
658+
sm.binsha = module.head.commit.binsha
659+
with child.module() as nested:
660+
nested.head.reference.set_tracking_branch(None)
661+
if operation in ("error", "keep-going"):
662+
sm.binsha = b"\x01" * len(sm.binsha)
663+
664+
checkout = Path(sm.abspath).resolve()
665+
execute = Git.execute
666+
processes = []
667+
668+
def capture_process(self, command, *args, **kwargs):
669+
result = execute(self, command, *args, **kwargs)
670+
if kwargs.get("as_process") and "cat-file" in command:
671+
working_dir = Path(self.working_dir).resolve()
672+
if working_dir == checkout or checkout in working_dir.parents:
673+
processes.append(result.proc)
674+
return result
675+
676+
monkeypatch.setattr(Git, "execute", capture_process)
677+
try:
678+
if operation == "error":
679+
with pytest.raises(GitCommandError):
680+
sm.update(to_latest_revision=True, no_fetch=True)
681+
else:
682+
sm.update(to_latest_revision=True, no_fetch=True, recursive=recursive, keep_going=operation == "keep-going")
683+
# These records retain Head arguments and their internally opened Repos.
684+
# Collection cannot release the processes while the records remain alive.
685+
records = [record for record in caplog.records if "a tracking branch was not set" in record.msg]
686+
assert len(records) == (2 if recursive else 1)
687+
assert processes
688+
assert all(process.poll() is not None for process in processes)
689+
finally:
690+
for process in processes:
691+
if process.poll() is None:
692+
process.terminate()
693+
process.wait()
694+
695+
649696
@pytest.fixture
650697
def windows_directory_symlink_removal(monkeypatch):
651698
"""Exercise Windows rmdir semantics on POSIX, where rmdir rejects symlinks."""
@@ -1915,13 +1962,14 @@ def assert_exists(sm, value=True):
19151962
assert_exists(sm)
19161963

19171964
# Add additional submodule level.
1918-
csm = sm.module().create_submodule(
1919-
"nested-submodule",
1920-
join_path_native("nested-submodule", "working-tree"),
1921-
url=self._small_repo_url(),
1922-
)
1923-
sm.module().index.commit("added nested submodule")
1924-
sm_head_commit = sm.module().commit()
1965+
with sm.module() as module:
1966+
csm = module.create_submodule(
1967+
"nested-submodule",
1968+
join_path_native("nested-submodule", "working-tree"),
1969+
url=self._small_repo_url(),
1970+
)
1971+
module.index.commit("added nested submodule")
1972+
sm_head_commit = module.commit()
19251973
assert_exists(csm)
19261974

19271975
# Fails because there are new commits, compared to the remote we cloned from.
@@ -1951,6 +1999,7 @@ def assert_exists(sm, value=True):
19511999

19522000
# remove
19532001
sm_module_path = sm.module().git_dir
2002+
csm.repo.close()
19542003

19552004
for dry_run in (True, False):
19562005
sm.remove(dry_run=dry_run, force=True)

0 commit comments

Comments
 (0)