linkfile: Handle directory-to-symlink transitions safely
When a manifest changes from individual linkfiles inside a directory
(e.g. dest=".llms/rules", dest=".llms/skills") to a single linkfile
for the whole directory (e.g. dest=".llms", src="dot-llms"), two
things need to happen:
1. __linkIt must replace a real directory with a symlink. Use
os.rmdir() instead of platform_utils.remove() for real directories.
rmdir only removes empty directories, so user-created content is
never deleted.
2. UpdateCopyLinkfileList must handle the cleanup correctly:
- Use os.rmdir() for directories (safe for non-empty)
- Remove empty parent directories after cleaning old dests
- Retry _CopyAndLinkFiles for all projects, since in interleaved
sync mode _CopyAndLinkFiles runs before cleanup and may have
failed because the directory was not yet empty
Change-Id: I0437b80beab98bce064cea81c11c47d699be91aa
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/569243
Tested-by: Carlos Fernandez <carlosfsanz@meta.com>
Reviewed-by: Mike Frysinger <vapier@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Carlos Fernandez <carlosfsanz@meta.com>
diff --git a/platform_utils.py b/platform_utils.py
index 45ffec7..58df6a9 100644
--- a/platform_utils.py
+++ b/platform_utils.py
@@ -222,6 +222,43 @@
os.rmdir(_makelongpath(path))
+def removedirs(path: str) -> None:
+ """Remove a directory tree of symlinks and empty directories.
+
+ Walks |path| bottom-up without following symlinks. Removes symlinks
+ and empty directories. Stops at (and preserves) regular files and
+ non-empty directories. Silently succeeds if |path| does not exist.
+
+ Availability: Unix, Windows.
+ """
+ if islink(path):
+ remove(path)
+ return
+
+ if not isdir(path):
+ return
+
+ for dirpath, dirnames, filenames in walk(path, topdown=False):
+ for name in dirnames:
+ entry = os.path.join(dirpath, name)
+ if islink(entry):
+ remove(entry)
+ else:
+ try:
+ rmdir(entry)
+ except OSError:
+ pass
+ for name in filenames:
+ entry = os.path.join(dirpath, name)
+ if islink(entry):
+ remove(entry)
+
+ try:
+ rmdir(path)
+ except OSError:
+ pass
+
+
def isdir(path):
"""os.path.isdir(path) wrapper with support for long paths on Windows.
diff --git a/project.py b/project.py
index f2fe7bc..5f2a98e 100644
--- a/project.py
+++ b/project.py
@@ -453,9 +453,13 @@
platform_utils.readlink(absDest) != relSrc
):
try:
- # Remove existing file first, since it might be read-only.
+ # Remove existing path first, since it might be read-only.
if os.path.lexists(absDest):
- platform_utils.remove(absDest)
+ # removedirs handles symlinks, empty dirs, and nested
+ # trees of stale linkfile dests without deleting user
+ # content. Falls through to symlink() if the path was
+ # fully removed, or raises OSError if not.
+ platform_utils.removedirs(absDest)
else:
dest_dir = os.path.dirname(absDest)
if not platform_utils.isdir(dest_dir):
diff --git a/subcmds/sync.py b/subcmds/sync.py
index 2517694..291bc62 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -1631,9 +1631,10 @@
new_paths = {}
new_linkfile_paths = []
new_copyfile_paths = []
- for project in self.GetProjects(
+ projects = self.GetProjects(
None, missing_ok=True, manifest=manifest, all_manifests=False
- ):
+ )
+ for project in projects:
new_linkfile_paths.extend(x.dest for x in project.linkfiles)
new_copyfile_paths.extend(x.dest for x in project.copyfiles)
@@ -1669,17 +1670,36 @@
)
for need_remove_file in need_remove_files:
- # Try to remove the updated copyfile or linkfile.
- # So, if the file is not exist, nothing need to do.
- platform_utils.remove(
- os.path.join(self.client.topdir, need_remove_file),
- missing_ok=True,
+ need_remove_path = os.path.join(
+ self.client.topdir, need_remove_file
)
+ if os.path.isfile(need_remove_path):
+ platform_utils.remove(need_remove_path)
+ else:
+ platform_utils.removedirs(need_remove_path)
+
+ # Also try to remove empty parent directories.
+ parent = os.path.dirname(need_remove_path)
+ while parent != self.client.topdir:
+ try:
+ os.rmdir(parent)
+ except OSError:
+ break
+ parent = os.path.dirname(parent)
# Create copy-link-files.json, save dest path of "copyfile" and
# "linkfile".
with open(copylinkfile_path, "w", encoding="utf-8") as fp:
json.dump(new_paths, fp)
+
+ # Retry linkfile/copyfile creation for all projects. In
+ # interleaved sync mode, _CopyAndLinkFiles runs before this
+ # cleanup, so linkfiles whose dest was blocked by an old
+ # directory may have failed. _CopyAndLinkFiles is idempotent
+ # and skips dests that are already correct.
+ for project in projects:
+ project._CopyAndLinkFiles()
+
return True
def _SmartSyncSetup(self, opt, smart_sync_manifest_path, manifest):
diff --git a/tests/test_platform_utils.py b/tests/test_platform_utils.py
index e899653..919351e 100644
--- a/tests/test_platform_utils.py
+++ b/tests/test_platform_utils.py
@@ -46,3 +46,71 @@
path.touch()
platform_utils.remove(path, missing_ok=False)
assert not path.exists()
+
+
+def test_removedirs_nonexistent(tmp_path: Path) -> None:
+ """removedirs should silently succeed on nonexistent paths."""
+ platform_utils.removedirs(tmp_path / "does-not-exist")
+
+
+def test_removedirs_symlink(tmp_path: Path) -> None:
+ """removedirs should remove a symlink."""
+ link = tmp_path / "link"
+ link.symlink_to("target")
+ platform_utils.removedirs(link)
+ assert not link.exists()
+
+
+def test_removedirs_empty_dir(tmp_path: Path) -> None:
+ """removedirs should remove an empty directory."""
+ d = tmp_path / "empty"
+ d.mkdir()
+ platform_utils.removedirs(d)
+ assert not d.exists()
+
+
+def test_removedirs_nested_empty_dirs(tmp_path: Path) -> None:
+ """removedirs should remove nested empty directories."""
+ d = tmp_path / "a" / "b" / "c"
+ d.mkdir(parents=True)
+ platform_utils.removedirs(tmp_path / "a")
+ assert not (tmp_path / "a").exists()
+
+
+def test_removedirs_symlinks_inside_dir(tmp_path: Path) -> None:
+ """removedirs should remove symlinks inside a directory."""
+ d = tmp_path / "dir"
+ d.mkdir()
+ (d / "link1").symlink_to("target1")
+ (d / "link2").symlink_to("target2")
+ platform_utils.removedirs(d)
+ assert not d.exists()
+
+
+def test_removedirs_preserves_user_files(tmp_path: Path) -> None:
+ """removedirs should not delete regular files or their parent dirs."""
+ d = tmp_path / "dir"
+ d.mkdir()
+ (d / "link").symlink_to("target")
+ (d / "user-file.txt").write_text("keep me")
+ platform_utils.removedirs(d)
+ assert d.exists()
+ assert not (d / "link").exists()
+ assert (d / "user-file.txt").read_text() == "keep me"
+
+
+def test_removedirs_deep_nested_with_symlinks(tmp_path: Path) -> None:
+ """removedirs should handle deep nesting: sub/dir/target."""
+ d = tmp_path / "sub" / "dir"
+ d.mkdir(parents=True)
+ (d / "link").symlink_to("target")
+ platform_utils.removedirs(tmp_path / "sub")
+ assert not (tmp_path / "sub").exists()
+
+
+def test_removedirs_regular_file_noop(tmp_path: Path) -> None:
+ """removedirs should not delete a regular file."""
+ f = tmp_path / "file.txt"
+ f.write_text("data")
+ platform_utils.removedirs(f)
+ assert f.exists()
diff --git a/tests/test_project.py b/tests/test_project.py
index 1deec07..728d5a5 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -365,6 +365,47 @@
os.path.join("git-project", "foo.txt"), os.readlink(dest)
)
+ def test_replace_empty_dir_with_symlink(self):
+ """A linkfile should replace an empty real directory at the dest path.
+
+ This is the common case: the old linkfiles inside the directory were
+ already cleaned up by UpdateCopyLinkfileList, leaving an empty parent
+ directory behind.
+ """
+ src_dir = os.path.join(self.worktree, "dot-llms")
+ os.makedirs(src_dir)
+
+ dest = os.path.join(self.topdir, "mydir")
+ os.makedirs(dest)
+
+ lf = self.LinkFile("dot-llms", "mydir")
+ lf._Link()
+ self.assertTrue(os.path.islink(dest))
+ self.assertEqual(
+ os.path.join("git-project", "dot-llms"), os.readlink(dest)
+ )
+
+ def test_nonempty_dir_not_clobbered(self):
+ """A linkfile must not delete a non-empty directory.
+
+ If the user created files in a directory that a new linkfile wants
+ to replace, __linkIt should fail safely rather than deleting content.
+ """
+ src_dir = os.path.join(self.worktree, "dot-llms")
+ os.makedirs(src_dir)
+
+ dest = os.path.join(self.topdir, "mydir")
+ os.makedirs(dest)
+ user_file = os.path.join(dest, "user-notes.txt")
+ self.touch(user_file)
+
+ lf = self.LinkFile("dot-llms", "mydir")
+ lf._Link()
+ # The directory should NOT be replaced — user content is preserved.
+ self.assertFalse(os.path.islink(dest))
+ self.assertTrue(os.path.isdir(dest))
+ self.assertTrue(os.path.exists(user_file))
+
class MigrateWorkTreeTests(unittest.TestCase):
"""Check _MigrateOldWorkTreeGitDir handling."""
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py
index 669027c..ef16239 100644
--- a/tests/test_subcmds_sync.py
+++ b/tests/test_subcmds_sync.py
@@ -13,6 +13,7 @@
# limitations under the License.
"""Unittests for the subcmds/sync.py module."""
+import json
import os
import shutil
import tempfile
@@ -1145,3 +1146,143 @@
self.assertTrue(result.checkout_success)
project.Sync_NetworkHalf.assert_called_once()
project.Sync_LocalHalf.assert_not_called()
+
+
+class UpdateCopyLinkfileListTest(unittest.TestCase):
+ """Tests for Sync.UpdateCopyLinkfileList."""
+
+ def setUp(self):
+ self.tempdirobj = tempfile.TemporaryDirectory(prefix="repo_tests")
+ self.topdir = self.tempdirobj.name
+ self.repodir = os.path.join(self.topdir, ".repo")
+ os.makedirs(self.repodir)
+
+ manifest = mock.MagicMock()
+ manifest.subdir = self.repodir
+ self.manifest = manifest
+
+ git_event_log = mock.MagicMock(ErrorEvent=mock.Mock(return_value=None))
+ self.cmd = sync.Sync(
+ manifest=manifest,
+ outer_client=mock.MagicMock(),
+ git_event_log=git_event_log,
+ )
+ self.cmd.client = mock.MagicMock(topdir=self.topdir)
+
+ def tearDown(self):
+ self.tempdirobj.cleanup()
+
+ def _write_copylinkfile_json(self, data: dict) -> None:
+ path = os.path.join(self.repodir, "copy-link-files.json")
+ with open(path, "w") as f:
+ json.dump(data, f)
+
+ def _setup_projects(self, linkfile_dests: list) -> None:
+ project = mock.MagicMock()
+ project.linkfiles = [mock.MagicMock(dest=d) for d in linkfile_dests]
+ project.copyfiles = []
+ mock.patch.object(
+ self.cmd, "GetProjects", return_value=[project]
+ ).start()
+
+ def test_removes_old_symlink_dest(self):
+ """Old linkfile dests that are symlinks should be removed."""
+ old_dest = os.path.join(self.topdir, "old-link")
+ os.symlink("target", old_dest)
+
+ self._write_copylinkfile_json(
+ {"linkfile": ["old-link"], "copyfile": []}
+ )
+ self._setup_projects([])
+
+ self.cmd.UpdateCopyLinkfileList(self.manifest)
+ self.assertFalse(os.path.lexists(old_dest))
+
+ def test_does_not_delete_through_new_symlink(self):
+ """Old dests that resolve through a new symlink must not delete files.
+
+ When the manifest changes from individual linkfiles inside a directory
+ to a single directory linkfile, and _CopyAndLinkFiles has already
+ created the symlink (interleaved mode), cleanup must not follow the
+ symlink and delete real project files.
+ """
+ project_dir = os.path.join(self.topdir, "vendor", "tools", "llms")
+ os.makedirs(os.path.join(project_dir, "dot-llms", "rules"))
+ os.makedirs(os.path.join(project_dir, "dot-llms", "skills"))
+ with open(
+ os.path.join(project_dir, "dot-llms", "rules", "basics.md"), "w"
+ ) as f:
+ f.write("# basics")
+ with open(
+ os.path.join(project_dir, "dot-llms", "skills", "repo.md"), "w"
+ ) as f:
+ f.write("# repo")
+
+ # Simulate interleaved mode: .llms -> vendor/tools/llms/dot-llms.
+ llms_link = os.path.join(self.topdir, ".llms")
+ os.symlink("vendor/tools/llms/dot-llms", llms_link)
+
+ self._write_copylinkfile_json(
+ {"linkfile": [".llms/rules", ".llms/skills"], "copyfile": []}
+ )
+ self._setup_projects([".llms"])
+
+ self.cmd.UpdateCopyLinkfileList(self.manifest)
+
+ # Real project files must still exist.
+ self.assertTrue(
+ os.path.exists(
+ os.path.join(project_dir, "dot-llms", "rules", "basics.md")
+ )
+ )
+ self.assertTrue(
+ os.path.exists(
+ os.path.join(project_dir, "dot-llms", "skills", "repo.md")
+ ),
+ )
+ self.assertTrue(os.path.islink(llms_link))
+
+ def test_cleans_up_empty_parent_dirs(self):
+ """After removing old dests, empty parent directories are removed."""
+ llms_dir = os.path.join(self.topdir, ".llms")
+ os.makedirs(llms_dir)
+ os.symlink(
+ "../vendor/tools/llms/rules", os.path.join(llms_dir, "rules")
+ )
+ os.symlink(
+ "../vendor/tools/llms/skills",
+ os.path.join(llms_dir, "skills"),
+ )
+
+ self._write_copylinkfile_json(
+ {"linkfile": [".llms/rules", ".llms/skills"], "copyfile": []}
+ )
+ self._setup_projects([".llms"])
+
+ self.cmd.UpdateCopyLinkfileList(self.manifest)
+
+ self.assertFalse(os.path.lexists(os.path.join(llms_dir, "rules")))
+ self.assertFalse(os.path.lexists(os.path.join(llms_dir, "skills")))
+ # Parent directory should be removed since it's now empty.
+ self.assertFalse(os.path.exists(llms_dir))
+
+ def test_preserves_nonempty_parent_dirs(self):
+ """Non-empty parent directories are preserved after old dest removal."""
+ llms_dir = os.path.join(self.topdir, ".llms")
+ os.makedirs(llms_dir)
+ os.symlink(
+ "../vendor/tools/llms/rules", os.path.join(llms_dir, "rules")
+ )
+ with open(os.path.join(llms_dir, "my-notes.txt"), "w") as f:
+ f.write("user content")
+
+ self._write_copylinkfile_json(
+ {"linkfile": [".llms/rules"], "copyfile": []}
+ )
+ self._setup_projects([".llms"])
+
+ self.cmd.UpdateCopyLinkfileList(self.manifest)
+
+ self.assertFalse(os.path.lexists(os.path.join(llms_dir, "rules")))
+ self.assertTrue(os.path.exists(os.path.join(llms_dir, "my-notes.txt")))
+ self.assertTrue(os.path.isdir(llms_dir))
