From 2478cb9fdd54834580b97acab57031a27dc9d1b3 Mon Sep 17 00:00:00 2001 From: Eugene Cohen Date: Thu, 16 Jul 2026 23:42:16 +0000 Subject: [PATCH] fix: make clone-cache copy tolerate symlinks and existing dirs Copying a cache entry into the workspace used shutil.copytree with defaults, which broke two real cases: - A cached repo may contain a dangling symlink (e.g. edk2's EmulatorPkg X11IncludeHack, pointing at system X11 headers that need not exist); copytree followed it and aborted with ENOENT. Copy with symlinks=True at both sites: the cache -> workspace copy in Git._clone_cache and the atomic-update copy in _pathlock (hit when an existing cache entry is reused). - The destination may be an empty submodule mountpoint left by a parent clone (a dependency whose path lands inside another repo's uninitialised submodule); copytree rejected the existing dir with FileExistsError, though plain git clone tolerates it. Copy with dirs_exist_ok=True in _clone_cache (clone() already asserts the destination is empty). Add regression tests covering the dangling-symlink and existing-empty-dir cases across the cache-init and cache-reuse paths. --- src/gitws/_pathlock.py | 4 +++- src/gitws/git.py | 7 ++++++- tests/test_git.py | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/gitws/_pathlock.py b/src/gitws/_pathlock.py index 1ea3faa..83f2618 100644 --- a/src/gitws/_pathlock.py +++ b/src/gitws/_pathlock.py @@ -129,7 +129,9 @@ def atomic_update_or_create_path(path: Path): # If the path already exists, make a copy to work on: if path.exists(): if path.is_dir(): - copytree(path, tmp_path) + # symlinks=True: preserve symlinks rather than dereference them, so a + # dangling symlink in the source does not abort the copy with ENOENT. + copytree(path, tmp_path, symlinks=True) else: copyfile(path, tmp_path) diff --git a/src/gitws/git.py b/src/gitws/git.py index a76e735..8dbed54 100644 --- a/src/gitws/git.py +++ b/src/gitws/git.py @@ -356,7 +356,12 @@ def _clone_cache(self, url): tmp_cache.mkdir(parents=True) run(("git", "clone", "--", str(url), str(tmp_cache))) _LOGGER.debug("Copy %s to %s)", tmp_cache, self.path) - shutil.copytree(tmp_cache, self.path) + # symlinks=True: a repo may contain a dangling symlink (e.g. edk2's + # EmulatorPkg X11IncludeHack); following it would abort the copy with ENOENT. + # dirs_exist_ok=True: self.path may be an empty submodule mountpoint left by a + # parent clone (the assert in clone() guarantees it is empty); plain git clone + # tolerates that, but copytree otherwise rejects it with FileExistsError. + shutil.copytree(tmp_cache, self.path, symlinks=True, dirs_exist_ok=True) # Remove user/password credentials from cache self._run(("remote", "remove", "origin"), cwd=tmp_cache) diff --git a/tests/test_git.py b/tests/test_git.py index fee2620..dc7575d 100644 --- a/tests/test_git.py +++ b/tests/test_git.py @@ -286,6 +286,46 @@ def test_cache_modified(tmp_path, repos): assert not (git.path / "new.txt").exists() +def test_cache_dangling_symlink(tmp_path): + """A cached repo with a dangling symlink must clone without dereferencing it.""" + repos_path = tmp_path / "repos" + + with git_repo(repos_path / "main", commit="initial") as path: + (path / "data.txt").write_text("main") + (path / "dangling").symlink_to("does/not/exist") + + cache_path = tmp_path / "cache" + + # First clone initializes the cache: copytree cache -> destination. + git = Git(tmp_path / "main1", clone_cache=cache_path) + git.clone(path2url(repos_path / "main")) + assert (git.path / "dangling").is_symlink() + + # Second clone reuses the cache: copytree cache -> tmp (pathlock), then -> destination. + git = Git(tmp_path / "main2", clone_cache=cache_path) + git.clone(path2url(repos_path / "main")) + assert (git.path / "dangling").is_symlink() + + +def test_cache_existing_empty_dir(tmp_path): + """Cloning via the cache into an existing empty dir (submodule mountpoint) must work.""" + repos_path = tmp_path / "repos" + + with git_repo(repos_path / "main", commit="initial") as path: + (path / "data.txt").write_text("main") + + cache_path = tmp_path / "cache" + + # git leaves an empty directory for an uninitialised submodule mountpoint; the + # cache -> destination copy must merge into it instead of failing on its existence. + for name in ("main1", "main2"): # cache-init, then cache-reuse + dest = tmp_path / name + dest.mkdir(parents=True) + git = Git(dest, clone_cache=cache_path) + git.clone(path2url(repos_path / "main")) + assert (git.path / "data.txt").read_text() == "main" + + def test_empty(tmp_path): """Test is_empty().""" Git.init(tmp_path)